From 6df9611059e21d003fda4a0f8bd6773736438643 Mon Sep 17 00:00:00 2001 From: yzct12345 <87620833+yzct12345@users.noreply.github.com> Date: Thu, 5 Aug 2021 20:11:14 +0000 Subject: memory: Clean up code --- src/core/memory.cpp | 306 +++++++++++++--------------------------------------- 1 file changed, 77 insertions(+), 229 deletions(-) (limited to 'src/core/memory.cpp') diff --git a/src/core/memory.cpp b/src/core/memory.cpp index f285c6f63..7b23c189c 100644 --- a/src/core/memory.cpp +++ b/src/core/memory.cpp @@ -1,11 +1,9 @@ -// Copyright 2015 Citra Emulator Project +// Copyright 2021 Citra Emulator Project // Licensed under GPLv2 or any later version // Refer to the license.txt file included. #include #include -#include -#include #include "common/assert.h" #include "common/atomic_ops.h" @@ -14,12 +12,10 @@ #include "common/page_table.h" #include "common/settings.h" #include "common/swap.h" -#include "core/arm/arm_interface.h" #include "core/core.h" #include "core/device_memory.h" #include "core/hle/kernel/k_page_table.h" #include "core/hle/kernel/k_process.h" -#include "core/hle/kernel/physical_memory.h" #include "core/memory.h" #include "video_core/gpu.h" @@ -62,17 +58,7 @@ struct Memory::Impl { } } - bool IsValidVirtualAddress(const Kernel::KProcess& process, const VAddr vaddr) const { - const auto& page_table = process.PageTable().PageTableImpl(); - const auto [pointer, type] = page_table.pointers[vaddr >> PAGE_BITS].PointerType(); - return pointer != nullptr || type == Common::PageType::RasterizerCachedMemory; - } - - bool IsValidVirtualAddress(VAddr vaddr) const { - return IsValidVirtualAddress(*system.CurrentProcess(), vaddr); - } - - u8* GetPointerFromRasterizerCachedMemory(VAddr vaddr) const { + [[nodiscard]] u8* GetPointerFromRasterizerCachedMemory(VAddr vaddr) const { const PAddr paddr{current_page_table->backing_addr[vaddr >> PAGE_BITS]}; if (!paddr) { @@ -82,7 +68,7 @@ struct Memory::Impl { return system.DeviceMemory().GetPointer(paddr) + vaddr; } - u8* GetPointer(const VAddr vaddr) const { + [[nodiscard]] u8* GetPointer(const VAddr vaddr) const { const uintptr_t raw_pointer = current_page_table->pointers[vaddr >> PAGE_BITS].Raw(); if (u8* const pointer = Common::PageTable::PageInfo::ExtractPointer(raw_pointer)) { return pointer + vaddr; @@ -179,7 +165,7 @@ struct Memory::Impl { std::string string; string.reserve(max_length); for (std::size_t i = 0; i < max_length; ++i) { - const char c = Read8(vaddr); + const char c = Read(vaddr); if (c == '\0') { break; } @@ -190,15 +176,14 @@ struct Memory::Impl { return string; } - void ReadBlock(const Kernel::KProcess& process, const VAddr src_addr, void* dest_buffer, - const std::size_t size) { + void WalkBlock(const Kernel::KProcess& process, VAddr addr, const std::size_t size, + auto on_unmapped, auto on_memory, auto on_rasterizer, auto increment) { const auto& page_table = process.PageTable().PageTableImpl(); - std::size_t remaining_size = size; - std::size_t page_index = src_addr >> PAGE_BITS; - std::size_t page_offset = src_addr & PAGE_MASK; + std::size_t page_index = addr >> PAGE_BITS; + std::size_t page_offset = addr & PAGE_MASK; - while (remaining_size > 0) { + while (remaining_size) { const std::size_t copy_amount = std::min(static_cast(PAGE_SIZE) - page_offset, remaining_size); const auto current_vaddr = static_cast((page_index << PAGE_BITS) + page_offset); @@ -206,22 +191,18 @@ struct Memory::Impl { const auto [pointer, type] = page_table.pointers[page_index].PointerType(); switch (type) { case Common::PageType::Unmapped: { - LOG_ERROR(HW_Memory, - "Unmapped ReadBlock @ 0x{:016X} (start address = 0x{:016X}, size = {})", - current_vaddr, src_addr, size); - std::memset(dest_buffer, 0, copy_amount); + on_unmapped(copy_amount, current_vaddr); break; } case Common::PageType::Memory: { DEBUG_ASSERT(pointer); - const u8* const src_ptr = pointer + page_offset + (page_index << PAGE_BITS); - std::memcpy(dest_buffer, src_ptr, copy_amount); + u8* mem_ptr = pointer + page_offset + (page_index << PAGE_BITS); + on_memory(copy_amount, mem_ptr); break; } case Common::PageType::RasterizerCachedMemory: { - const u8* const host_ptr{GetPointerFromRasterizerCachedMemory(current_vaddr)}; - system.GPU().FlushRegion(current_vaddr, copy_amount); - std::memcpy(dest_buffer, host_ptr, copy_amount); + u8* const host_ptr{GetPointerFromRasterizerCachedMemory(current_vaddr)}; + on_rasterizer(current_vaddr, copy_amount, host_ptr); break; } default: @@ -230,199 +211,98 @@ struct Memory::Impl { page_index++; page_offset = 0; - dest_buffer = static_cast(dest_buffer) + copy_amount; + addr += static_cast(copy_amount); + increment(copy_amount); remaining_size -= copy_amount; } } - void ReadBlockUnsafe(const Kernel::KProcess& process, const VAddr src_addr, void* dest_buffer, - const std::size_t size) { - const auto& page_table = process.PageTable().PageTableImpl(); - - std::size_t remaining_size = size; - std::size_t page_index = src_addr >> PAGE_BITS; - std::size_t page_offset = src_addr & PAGE_MASK; - - while (remaining_size > 0) { - const std::size_t copy_amount = - std::min(static_cast(PAGE_SIZE) - page_offset, remaining_size); - const auto current_vaddr = static_cast((page_index << PAGE_BITS) + page_offset); - - const auto [pointer, type] = page_table.pointers[page_index].PointerType(); - switch (type) { - case Common::PageType::Unmapped: { + template + void ReadBlockImpl(const Kernel::KProcess& process, const VAddr src_addr, void* dest_buffer, + const std::size_t size) { + WalkBlock( + process, src_addr, size, + [src_addr, size, &dest_buffer](const std::size_t copy_amount, + const VAddr current_vaddr) { LOG_ERROR(HW_Memory, "Unmapped ReadBlock @ 0x{:016X} (start address = 0x{:016X}, size = {})", current_vaddr, src_addr, size); std::memset(dest_buffer, 0, copy_amount); - break; - } - case Common::PageType::Memory: { - DEBUG_ASSERT(pointer); - const u8* const src_ptr = pointer + page_offset + (page_index << PAGE_BITS); + }, + [&dest_buffer](const std::size_t copy_amount, const u8* const src_ptr) { std::memcpy(dest_buffer, src_ptr, copy_amount); - break; - } - case Common::PageType::RasterizerCachedMemory: { - const u8* const host_ptr{GetPointerFromRasterizerCachedMemory(current_vaddr)}; + }, + [&system = system, &dest_buffer](const VAddr current_vaddr, + const std::size_t copy_amount, + const u8* const host_ptr) { + if (!UNSAFE) { + system.GPU().FlushRegion(current_vaddr, copy_amount); + } std::memcpy(dest_buffer, host_ptr, copy_amount); - break; - } - default: - UNREACHABLE(); - } - - page_index++; - page_offset = 0; - dest_buffer = static_cast(dest_buffer) + copy_amount; - remaining_size -= copy_amount; - } + }, + [&dest_buffer](const std::size_t copy_amount) { + dest_buffer = static_cast(dest_buffer) + copy_amount; + }); } void ReadBlock(const VAddr src_addr, void* dest_buffer, const std::size_t size) { - ReadBlock(*system.CurrentProcess(), src_addr, dest_buffer, size); + ReadBlockImpl(*system.CurrentProcess(), src_addr, dest_buffer, size); } void ReadBlockUnsafe(const VAddr src_addr, void* dest_buffer, const std::size_t size) { - ReadBlockUnsafe(*system.CurrentProcess(), src_addr, dest_buffer, size); + ReadBlockImpl(*system.CurrentProcess(), src_addr, dest_buffer, size); } - void WriteBlock(const Kernel::KProcess& process, const VAddr dest_addr, const void* src_buffer, - const std::size_t size) { - const auto& page_table = process.PageTable().PageTableImpl(); - std::size_t remaining_size = size; - std::size_t page_index = dest_addr >> PAGE_BITS; - std::size_t page_offset = dest_addr & PAGE_MASK; - - while (remaining_size > 0) { - const std::size_t copy_amount = - std::min(static_cast(PAGE_SIZE) - page_offset, remaining_size); - const auto current_vaddr = static_cast((page_index << PAGE_BITS) + page_offset); - - const auto [pointer, type] = page_table.pointers[page_index].PointerType(); - switch (type) { - case Common::PageType::Unmapped: { + template + void WriteBlockImpl(const Kernel::KProcess& process, const VAddr dest_addr, + const void* src_buffer, const std::size_t size) { + WalkBlock( + process, dest_addr, size, + [dest_addr, size](const std::size_t copy_amount, const VAddr current_vaddr) { LOG_ERROR(HW_Memory, "Unmapped WriteBlock @ 0x{:016X} (start address = 0x{:016X}, size = {})", current_vaddr, dest_addr, size); - break; - } - case Common::PageType::Memory: { - DEBUG_ASSERT(pointer); - u8* const dest_ptr = pointer + page_offset + (page_index << PAGE_BITS); + }, + [&src_buffer](const std::size_t copy_amount, u8* const dest_ptr) { std::memcpy(dest_ptr, src_buffer, copy_amount); - break; - } - case Common::PageType::RasterizerCachedMemory: { - u8* const host_ptr{GetPointerFromRasterizerCachedMemory(current_vaddr)}; - system.GPU().InvalidateRegion(current_vaddr, copy_amount); - std::memcpy(host_ptr, src_buffer, copy_amount); - break; - } - default: - UNREACHABLE(); - } - - page_index++; - page_offset = 0; - src_buffer = static_cast(src_buffer) + copy_amount; - remaining_size -= copy_amount; - } - } - - void WriteBlockUnsafe(const Kernel::KProcess& process, const VAddr dest_addr, - const void* src_buffer, const std::size_t size) { - const auto& page_table = process.PageTable().PageTableImpl(); - std::size_t remaining_size = size; - std::size_t page_index = dest_addr >> PAGE_BITS; - std::size_t page_offset = dest_addr & PAGE_MASK; - - while (remaining_size > 0) { - const std::size_t copy_amount = - std::min(static_cast(PAGE_SIZE) - page_offset, remaining_size); - const auto current_vaddr = static_cast((page_index << PAGE_BITS) + page_offset); - - const auto [pointer, type] = page_table.pointers[page_index].PointerType(); - switch (type) { - case Common::PageType::Unmapped: { - LOG_ERROR(HW_Memory, - "Unmapped WriteBlock @ 0x{:016X} (start address = 0x{:016X}, size = {})", - current_vaddr, dest_addr, size); - break; - } - case Common::PageType::Memory: { - DEBUG_ASSERT(pointer); - u8* const dest_ptr = pointer + page_offset + (page_index << PAGE_BITS); - std::memcpy(dest_ptr, src_buffer, copy_amount); - break; - } - case Common::PageType::RasterizerCachedMemory: { - u8* const host_ptr{GetPointerFromRasterizerCachedMemory(current_vaddr)}; + }, + [&system = system, &src_buffer](const VAddr current_vaddr, + const std::size_t copy_amount, u8* const host_ptr) { + if (!UNSAFE) { + system.GPU().InvalidateRegion(current_vaddr, copy_amount); + } std::memcpy(host_ptr, src_buffer, copy_amount); - break; - } - default: - UNREACHABLE(); - } - - page_index++; - page_offset = 0; - src_buffer = static_cast(src_buffer) + copy_amount; - remaining_size -= copy_amount; - } + }, + [&src_buffer](const std::size_t copy_amount) { + src_buffer = static_cast(src_buffer) + copy_amount; + }); } void WriteBlock(const VAddr dest_addr, const void* src_buffer, const std::size_t size) { - WriteBlock(*system.CurrentProcess(), dest_addr, src_buffer, size); + WriteBlockImpl(*system.CurrentProcess(), dest_addr, src_buffer, size); } void WriteBlockUnsafe(const VAddr dest_addr, const void* src_buffer, const std::size_t size) { - WriteBlockUnsafe(*system.CurrentProcess(), dest_addr, src_buffer, size); + WriteBlockImpl(*system.CurrentProcess(), dest_addr, src_buffer, size); } void ZeroBlock(const Kernel::KProcess& process, const VAddr dest_addr, const std::size_t size) { - const auto& page_table = process.PageTable().PageTableImpl(); - std::size_t remaining_size = size; - std::size_t page_index = dest_addr >> PAGE_BITS; - std::size_t page_offset = dest_addr & PAGE_MASK; - - while (remaining_size > 0) { - const std::size_t copy_amount = - std::min(static_cast(PAGE_SIZE) - page_offset, remaining_size); - const auto current_vaddr = static_cast((page_index << PAGE_BITS) + page_offset); - - const auto [pointer, type] = page_table.pointers[page_index].PointerType(); - switch (type) { - case Common::PageType::Unmapped: { + WalkBlock( + process, dest_addr, size, + [dest_addr, size](const std::size_t copy_amount, const VAddr current_vaddr) { LOG_ERROR(HW_Memory, "Unmapped ZeroBlock @ 0x{:016X} (start address = 0x{:016X}, size = {})", current_vaddr, dest_addr, size); - break; - } - case Common::PageType::Memory: { - DEBUG_ASSERT(pointer); - u8* const dest_ptr = pointer + page_offset + (page_index << PAGE_BITS); + }, + [](const std::size_t copy_amount, u8* const dest_ptr) { std::memset(dest_ptr, 0, copy_amount); - break; - } - case Common::PageType::RasterizerCachedMemory: { - u8* const host_ptr{GetPointerFromRasterizerCachedMemory(current_vaddr)}; + }, + [&system = system](const VAddr current_vaddr, const std::size_t copy_amount, + u8* const host_ptr) { system.GPU().InvalidateRegion(current_vaddr, copy_amount); std::memset(host_ptr, 0, copy_amount); - break; - } - default: - UNREACHABLE(); - } - - page_index++; - page_offset = 0; - remaining_size -= copy_amount; - } - } - - void ZeroBlock(const VAddr dest_addr, const std::size_t size) { - ZeroBlock(*system.CurrentProcess(), dest_addr, size); + }, + [](const std::size_t copy_amount) {}); } void CopyBlock(const Kernel::KProcess& process, VAddr dest_addr, VAddr src_addr, @@ -432,7 +312,7 @@ struct Memory::Impl { std::size_t page_index = src_addr >> PAGE_BITS; std::size_t page_offset = src_addr & PAGE_MASK; - while (remaining_size > 0) { + while (remaining_size) { const std::size_t copy_amount = std::min(static_cast(PAGE_SIZE) - page_offset, remaining_size); const auto current_vaddr = static_cast((page_index << PAGE_BITS) + page_offset); @@ -449,13 +329,13 @@ struct Memory::Impl { case Common::PageType::Memory: { DEBUG_ASSERT(pointer); const u8* src_ptr = pointer + page_offset + (page_index << PAGE_BITS); - WriteBlock(process, dest_addr, src_ptr, copy_amount); + WriteBlockImpl(process, dest_addr, src_ptr, copy_amount); break; } case Common::PageType::RasterizerCachedMemory: { const u8* const host_ptr{GetPointerFromRasterizerCachedMemory(current_vaddr)}; system.GPU().FlushRegion(current_vaddr, copy_amount); - WriteBlock(process, dest_addr, host_ptr, copy_amount); + WriteBlockImpl(process, dest_addr, host_ptr, copy_amount); break; } default: @@ -470,10 +350,6 @@ struct Memory::Impl { } } - void CopyBlock(VAddr dest_addr, VAddr src_addr, std::size_t size) { - return CopyBlock(*system.CurrentProcess(), dest_addr, src_addr, size); - } - void RasterizerMarkRegionCached(VAddr vaddr, u64 size, bool cached) { if (vaddr == 0) { return; @@ -517,7 +393,6 @@ struct Memory::Impl { case Common::PageType::Unmapped: // It is not necessary for a process to have this region mapped into its address // space, for example, a system module need not have a VRAM mapping. - break; case Common::PageType::Memory: // There can be more than one GPU region mapped per CPU region, so it's common // that this area is already unmarked as cached. @@ -789,12 +664,11 @@ void Memory::UnmapRegion(Common::PageTable& page_table, VAddr base, u64 size) { impl->UnmapRegion(page_table, base, size); } -bool Memory::IsValidVirtualAddress(const Kernel::KProcess& process, const VAddr vaddr) const { - return impl->IsValidVirtualAddress(process, vaddr); -} - bool Memory::IsValidVirtualAddress(const VAddr vaddr) const { - return impl->IsValidVirtualAddress(vaddr); + const Kernel::KProcess& process = *system.CurrentProcess(); + const auto& pageTable = process.PageTable().PageTableImpl(); + const auto [pointer, type] = pageTable.pointers[vaddr >> PAGE_BITS].PointerType(); + return pointer != nullptr || type == Common::PageType::RasterizerCachedMemory; } u8* Memory::GetPointer(VAddr vaddr) { @@ -863,64 +737,38 @@ std::string Memory::ReadCString(VAddr vaddr, std::size_t max_length) { void Memory::ReadBlock(const Kernel::KProcess& process, const VAddr src_addr, void* dest_buffer, const std::size_t size) { - impl->ReadBlock(process, src_addr, dest_buffer, size); + impl->ReadBlockImpl(process, src_addr, dest_buffer, size); } void Memory::ReadBlock(const VAddr src_addr, void* dest_buffer, const std::size_t size) { impl->ReadBlock(src_addr, dest_buffer, size); } -void Memory::ReadBlockUnsafe(const Kernel::KProcess& process, const VAddr src_addr, - void* dest_buffer, const std::size_t size) { - impl->ReadBlockUnsafe(process, src_addr, dest_buffer, size); -} - void Memory::ReadBlockUnsafe(const VAddr src_addr, void* dest_buffer, const std::size_t size) { impl->ReadBlockUnsafe(src_addr, dest_buffer, size); } void Memory::WriteBlock(const Kernel::KProcess& process, VAddr dest_addr, const void* src_buffer, std::size_t size) { - impl->WriteBlock(process, dest_addr, src_buffer, size); + impl->WriteBlockImpl(process, dest_addr, src_buffer, size); } void Memory::WriteBlock(const VAddr dest_addr, const void* src_buffer, const std::size_t size) { impl->WriteBlock(dest_addr, src_buffer, size); } -void Memory::WriteBlockUnsafe(const Kernel::KProcess& process, VAddr dest_addr, - const void* src_buffer, std::size_t size) { - impl->WriteBlockUnsafe(process, dest_addr, src_buffer, size); -} - void Memory::WriteBlockUnsafe(const VAddr dest_addr, const void* src_buffer, const std::size_t size) { impl->WriteBlockUnsafe(dest_addr, src_buffer, size); } -void Memory::ZeroBlock(const Kernel::KProcess& process, VAddr dest_addr, std::size_t size) { - impl->ZeroBlock(process, dest_addr, size); -} - -void Memory::ZeroBlock(VAddr dest_addr, std::size_t size) { - impl->ZeroBlock(dest_addr, size); -} - void Memory::CopyBlock(const Kernel::KProcess& process, VAddr dest_addr, VAddr src_addr, const std::size_t size) { impl->CopyBlock(process, dest_addr, src_addr, size); } -void Memory::CopyBlock(VAddr dest_addr, VAddr src_addr, std::size_t size) { - impl->CopyBlock(dest_addr, src_addr, size); -} - void Memory::RasterizerMarkRegionCached(VAddr vaddr, u64 size, bool cached) { impl->RasterizerMarkRegionCached(vaddr, size, cached); } -bool IsKernelVirtualAddress(const VAddr vaddr) { - return KERNEL_REGION_VADDR <= vaddr && vaddr < KERNEL_REGION_END; -} - } // namespace Core::Memory -- cgit v1.2.3 From 4edfa6ad8ff65e91dd6363e10546e1b894d3460b Mon Sep 17 00:00:00 2001 From: yzct12345 <87620833+yzct12345@users.noreply.github.com> Date: Thu, 5 Aug 2021 20:29:43 +0000 Subject: memory: Address lioncash's review --- src/core/memory.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'src/core/memory.cpp') diff --git a/src/core/memory.cpp b/src/core/memory.cpp index 7b23c189c..2e578f189 100644 --- a/src/core/memory.cpp +++ b/src/core/memory.cpp @@ -1,4 +1,4 @@ -// Copyright 2021 Citra Emulator Project +// Copyright 2015 Citra Emulator Project // Licensed under GPLv2 or any later version // Refer to the license.txt file included. @@ -235,7 +235,7 @@ struct Memory::Impl { [&system = system, &dest_buffer](const VAddr current_vaddr, const std::size_t copy_amount, const u8* const host_ptr) { - if (!UNSAFE) { + if constexpr (!UNSAFE) { system.GPU().FlushRegion(current_vaddr, copy_amount); } std::memcpy(dest_buffer, host_ptr, copy_amount); @@ -268,7 +268,7 @@ struct Memory::Impl { }, [&system = system, &src_buffer](const VAddr current_vaddr, const std::size_t copy_amount, u8* const host_ptr) { - if (!UNSAFE) { + if constexpr (!UNSAFE) { system.GPU().InvalidateRegion(current_vaddr, copy_amount); } std::memcpy(host_ptr, src_buffer, copy_amount); @@ -390,9 +390,10 @@ struct Memory::Impl { } else { // Switch page type to uncached if now uncached switch (page_type) { - case Common::PageType::Unmapped: + case Common::PageType::Unmapped: // NOLINT(bugprone-branch-clone) // It is not necessary for a process to have this region mapped into its address // space, for example, a system module need not have a VRAM mapping. + break; case Common::PageType::Memory: // There can be more than one GPU region mapped per CPU region, so it's common // that this area is already unmarked as cached. @@ -666,8 +667,8 @@ void Memory::UnmapRegion(Common::PageTable& page_table, VAddr base, u64 size) { bool Memory::IsValidVirtualAddress(const VAddr vaddr) const { const Kernel::KProcess& process = *system.CurrentProcess(); - const auto& pageTable = process.PageTable().PageTableImpl(); - const auto [pointer, type] = pageTable.pointers[vaddr >> PAGE_BITS].PointerType(); + const auto& page_table = process.PageTable().PageTableImpl(); + const auto [pointer, type] = page_table.pointers[vaddr >> PAGE_BITS].PointerType(); return pointer != nullptr || type == Common::PageType::RasterizerCachedMemory; } -- cgit v1.2.3 From e611f522c28edcc6f6850a5884e8b1bb41004a57 Mon Sep 17 00:00:00 2001 From: yzct12345 <87620833+yzct12345@users.noreply.github.com> Date: Thu, 5 Aug 2021 21:09:08 +0000 Subject: memory: Clean up CopyBlock too --- src/core/memory.cpp | 51 +++++++++++++++------------------------------------ 1 file changed, 15 insertions(+), 36 deletions(-) (limited to 'src/core/memory.cpp') diff --git a/src/core/memory.cpp b/src/core/memory.cpp index 2e578f189..0b8e36b08 100644 --- a/src/core/memory.cpp +++ b/src/core/memory.cpp @@ -176,7 +176,7 @@ struct Memory::Impl { return string; } - void WalkBlock(const Kernel::KProcess& process, VAddr addr, const std::size_t size, + void WalkBlock(const Kernel::KProcess& process, const VAddr addr, const std::size_t size, auto on_unmapped, auto on_memory, auto on_rasterizer, auto increment) { const auto& page_table = process.PageTable().PageTableImpl(); std::size_t remaining_size = size; @@ -211,7 +211,6 @@ struct Memory::Impl { page_index++; page_offset = 0; - addr += static_cast(copy_amount); increment(copy_amount); remaining_size -= copy_amount; } @@ -307,47 +306,27 @@ struct Memory::Impl { void CopyBlock(const Kernel::KProcess& process, VAddr dest_addr, VAddr src_addr, const std::size_t size) { - const auto& page_table = process.PageTable().PageTableImpl(); - std::size_t remaining_size = size; - std::size_t page_index = src_addr >> PAGE_BITS; - std::size_t page_offset = src_addr & PAGE_MASK; - - while (remaining_size) { - const std::size_t copy_amount = - std::min(static_cast(PAGE_SIZE) - page_offset, remaining_size); - const auto current_vaddr = static_cast((page_index << PAGE_BITS) + page_offset); - - const auto [pointer, type] = page_table.pointers[page_index].PointerType(); - switch (type) { - case Common::PageType::Unmapped: { + WalkBlock( + process, dest_addr, size, + [this, &process, &dest_addr, &src_addr, size](const std::size_t copy_amount, + const VAddr current_vaddr) { LOG_ERROR(HW_Memory, "Unmapped CopyBlock @ 0x{:016X} (start address = 0x{:016X}, size = {})", current_vaddr, src_addr, size); ZeroBlock(process, dest_addr, copy_amount); - break; - } - case Common::PageType::Memory: { - DEBUG_ASSERT(pointer); - const u8* src_ptr = pointer + page_offset + (page_index << PAGE_BITS); + }, + [this, &process, &dest_addr](const std::size_t copy_amount, const u8* const src_ptr) { WriteBlockImpl(process, dest_addr, src_ptr, copy_amount); - break; - } - case Common::PageType::RasterizerCachedMemory: { - const u8* const host_ptr{GetPointerFromRasterizerCachedMemory(current_vaddr)}; + }, + [this, &system = system, &process, &dest_addr]( + const VAddr current_vaddr, const std::size_t copy_amount, u8* const host_ptr) { system.GPU().FlushRegion(current_vaddr, copy_amount); WriteBlockImpl(process, dest_addr, host_ptr, copy_amount); - break; - } - default: - UNREACHABLE(); - } - - page_index++; - page_offset = 0; - dest_addr += static_cast(copy_amount); - src_addr += static_cast(copy_amount); - remaining_size -= copy_amount; - } + }, + [&dest_addr, &src_addr](const std::size_t copy_amount) { + dest_addr += static_cast(copy_amount); + src_addr += static_cast(copy_amount); + }); } void RasterizerMarkRegionCached(VAddr vaddr, u64 size, bool cached) { -- cgit v1.2.3 From 70cc4c0f46ed68a4d660aa9867b5b8de41b77549 Mon Sep 17 00:00:00 2001 From: yzct12345 <87620833+yzct12345@users.noreply.github.com> Date: Sat, 7 Aug 2021 01:32:06 +0000 Subject: memory: Dedup Read and Write and fix logging bugs --- src/core/memory.cpp | 244 +++++++++++++++++++++++++--------------------------- 1 file changed, 115 insertions(+), 129 deletions(-) (limited to 'src/core/memory.cpp') diff --git a/src/core/memory.cpp b/src/core/memory.cpp index 0b8e36b08..778d152dd 100644 --- a/src/core/memory.cpp +++ b/src/core/memory.cpp @@ -5,6 +5,10 @@ #include #include +#define BOOST_HANA_CONFIG_ENABLE_STRING_UDL +#include +#undef BOOST_HANA_CONFIG_ENABLE_STRING_UDL + #include "common/assert.h" #include "common/atomic_ops.h" #include "common/common_types.h" @@ -19,6 +23,8 @@ #include "core/memory.h" #include "video_core/gpu.h" +using namespace boost::hana::literals; + namespace Core::Memory { // Implementation class used to keep the specifics of the memory subsystem hidden @@ -68,18 +74,6 @@ struct Memory::Impl { return system.DeviceMemory().GetPointer(paddr) + vaddr; } - [[nodiscard]] u8* GetPointer(const VAddr vaddr) const { - const uintptr_t raw_pointer = current_page_table->pointers[vaddr >> PAGE_BITS].Raw(); - if (u8* const pointer = Common::PageTable::PageInfo::ExtractPointer(raw_pointer)) { - return pointer + vaddr; - } - const auto type = Common::PageTable::PageInfo::ExtractType(raw_pointer); - if (type == Common::PageType::RasterizerCachedMemory) { - return GetPointerFromRasterizerCachedMemory(vaddr); - } - return nullptr; - } - u8 Read8(const VAddr addr) { return Read(addr); } @@ -453,51 +447,106 @@ struct Memory::Impl { } /** - * Reads a particular data type out of memory at the given virtual address. + * Returns a message like "Unmapped NameBits @ 0x{:016X}Suffix". * - * @param vaddr The virtual address to read the data type from. - * - * @tparam T The data type to read out of memory. This type *must* be - * trivially copyable, otherwise the behavior of this function - * is undefined. - * - * @returns The instance of T read from the specified virtual address. + * @tparam NAME The caller name like "Read"_s or "Write"_s. + * @tparam BYTES The number of bits written. 0 is for read and sizeof(T) is for write. + * @tparam SUFFIX A suffix. ""_s is for read and " = 0x{:016X}" is for write. */ - template - T Read(VAddr vaddr) { + template + static consteval const char* GetPointerImplError() { + constexpr auto unmapped_fmt = ([]() { + constexpr auto prefix = "Unmapped "_s + NAME; + constexpr auto suffix = " @ 0x{:016X}"_s + SUFFIX; + const char* result = nullptr; + switch (BYTES * 8) { + case 0: + result = (prefix + suffix).c_str(); + break; +#define BITS_CASE(x) \ + case x: \ + result = (prefix + BOOST_HANA_STRING(#x) + suffix).c_str(); \ + break; + BITS_CASE(8) + BITS_CASE(16) + BITS_CASE(32) + BITS_CASE(64) + BITS_CASE(128) +#undef BITS_CASE + default: + break; + } + return result; + })(); + static_assert(unmapped_fmt); + return unmapped_fmt; + } + + [[nodiscard]] u8* GetPointerImpl(VAddr vaddr, auto on_unmapped, auto on_rasterizer) const { // AARCH64 masks the upper 16 bit of all memory accesses vaddr &= 0xffffffffffffLL; if (vaddr >= 1uLL << current_page_table->GetAddressSpaceBits()) { - LOG_ERROR(HW_Memory, "Unmapped Read{} @ 0x{:08X}", sizeof(T) * 8, vaddr); - return 0; + on_unmapped(); + return nullptr; } // Avoid adding any extra logic to this fast-path block const uintptr_t raw_pointer = current_page_table->pointers[vaddr >> PAGE_BITS].Raw(); - if (const u8* const pointer = Common::PageTable::PageInfo::ExtractPointer(raw_pointer)) { - T value; - std::memcpy(&value, &pointer[vaddr], sizeof(T)); - return value; + if (u8* const pointer = Common::PageTable::PageInfo::ExtractPointer(raw_pointer)) { + return &pointer[vaddr]; } switch (Common::PageTable::PageInfo::ExtractType(raw_pointer)) { case Common::PageType::Unmapped: - LOG_ERROR(HW_Memory, "Unmapped Read{} @ 0x{:08X}", sizeof(T) * 8, vaddr); - return 0; + on_unmapped(); + return nullptr; case Common::PageType::Memory: - ASSERT_MSG(false, "Mapped memory page without a pointer @ {:016X}", vaddr); - break; + ASSERT_MSG(false, "Mapped memory page without a pointer @ 0x{:016X}", vaddr); + return nullptr; case Common::PageType::RasterizerCachedMemory: { - const u8* const host_ptr{GetPointerFromRasterizerCachedMemory(vaddr)}; - system.GPU().FlushRegion(vaddr, sizeof(T)); - T value; - std::memcpy(&value, host_ptr, sizeof(T)); - return value; + u8* const host_ptr{GetPointerFromRasterizerCachedMemory(vaddr)}; + on_rasterizer(); + return host_ptr; } default: UNREACHABLE(); } - return {}; + return nullptr; + } + + [[nodiscard]] u8* GetPointer(const VAddr vaddr) const { + return GetPointerImpl( + vaddr, + [vaddr]() { + LOG_ERROR(HW_Memory, GetPointerImplError<"GetPointer"_s, 0, ""_s>(), vaddr); + }, + []() {}); + } + + /** + * Reads a particular data type out of memory at the given virtual address. + * + * @param vaddr The virtual address to read the data type from. + * + * @tparam T The data type to read out of memory. This type *must* be + * trivially copyable, otherwise the behavior of this function + * is undefined. + * + * @returns The instance of T read from the specified virtual address. + */ + template + T Read(VAddr vaddr) { + T result = 0; + const u8* const ptr = GetPointerImpl( + vaddr, + [vaddr]() { + LOG_ERROR(HW_Memory, GetPointerImplError<"Read"_s, sizeof(T), ""_s>(), vaddr); + }, + [&system = system, vaddr]() { system.GPU().FlushRegion(vaddr, sizeof(T)); }); + if (ptr) { + std::memcpy(&result, ptr, sizeof(T)); + } + return result; } /** @@ -511,110 +560,47 @@ struct Memory::Impl { */ template void Write(VAddr vaddr, const T data) { - // AARCH64 masks the upper 16 bit of all memory accesses - vaddr &= 0xffffffffffffLL; - - if (vaddr >= 1uLL << current_page_table->GetAddressSpaceBits()) { - LOG_ERROR(HW_Memory, "Unmapped Write{} 0x{:08X} @ 0x{:016X}", sizeof(data) * 8, - static_cast(data), vaddr); - return; - } - - // Avoid adding any extra logic to this fast-path block - const uintptr_t raw_pointer = current_page_table->pointers[vaddr >> PAGE_BITS].Raw(); - if (u8* const pointer = Common::PageTable::PageInfo::ExtractPointer(raw_pointer)) { - std::memcpy(&pointer[vaddr], &data, sizeof(T)); - return; - } - switch (Common::PageTable::PageInfo::ExtractType(raw_pointer)) { - case Common::PageType::Unmapped: - LOG_ERROR(HW_Memory, "Unmapped Write{} 0x{:08X} @ 0x{:016X}", sizeof(data) * 8, - static_cast(data), vaddr); - return; - case Common::PageType::Memory: - ASSERT_MSG(false, "Mapped memory page without a pointer @ {:016X}", vaddr); - break; - case Common::PageType::RasterizerCachedMemory: { - u8* const host_ptr{GetPointerFromRasterizerCachedMemory(vaddr)}; - system.GPU().InvalidateRegion(vaddr, sizeof(T)); - std::memcpy(host_ptr, &data, sizeof(T)); - break; - } - default: - UNREACHABLE(); + u8* const ptr = GetPointerImpl( + vaddr, + [vaddr, data]() { + LOG_ERROR(HW_Memory, GetPointerImplError<"Write"_s, sizeof(T), " = 0x{:016X}"_s>(), + vaddr, static_cast(data)); + }, + [&system = system, vaddr]() { system.GPU().InvalidateRegion(vaddr, sizeof(T)); }); + if (ptr) { + std::memcpy(ptr, &data, sizeof(T)); } } template bool WriteExclusive(VAddr vaddr, const T data, const T expected) { - // AARCH64 masks the upper 16 bit of all memory accesses - vaddr &= 0xffffffffffffLL; - - if (vaddr >= 1uLL << current_page_table->GetAddressSpaceBits()) { - LOG_ERROR(HW_Memory, "Unmapped Write{} 0x{:08X} @ 0x{:016X}", sizeof(data) * 8, - static_cast(data), vaddr); - return true; - } - - const uintptr_t raw_pointer = current_page_table->pointers[vaddr >> PAGE_BITS].Raw(); - if (u8* const pointer = Common::PageTable::PageInfo::ExtractPointer(raw_pointer)) { - // NOTE: Avoid adding any extra logic to this fast-path block - const auto volatile_pointer = reinterpret_cast(&pointer[vaddr]); + u8* const ptr = GetPointerImpl( + vaddr, + [vaddr, data]() { + LOG_ERROR(HW_Memory, GetPointerImplError<"Write"_s, sizeof(T), " = 0x{:016X}"_s>(), + vaddr, static_cast(data)); + }, + [&system = system, vaddr]() { system.GPU().InvalidateRegion(vaddr, sizeof(T)); }); + if (ptr) { + const auto volatile_pointer = reinterpret_cast(ptr); return Common::AtomicCompareAndSwap(volatile_pointer, data, expected); } - switch (Common::PageTable::PageInfo::ExtractType(raw_pointer)) { - case Common::PageType::Unmapped: - LOG_ERROR(HW_Memory, "Unmapped Write{} 0x{:08X} @ 0x{:016X}", sizeof(data) * 8, - static_cast(data), vaddr); - return true; - case Common::PageType::Memory: - ASSERT_MSG(false, "Mapped memory page without a pointer @ {:016X}", vaddr); - break; - case Common::PageType::RasterizerCachedMemory: { - u8* host_ptr{GetPointerFromRasterizerCachedMemory(vaddr)}; - system.GPU().InvalidateRegion(vaddr, sizeof(T)); - auto* pointer = reinterpret_cast(&host_ptr); - return Common::AtomicCompareAndSwap(pointer, data, expected); - } - default: - UNREACHABLE(); - } return true; } bool WriteExclusive128(VAddr vaddr, const u128 data, const u128 expected) { - // AARCH64 masks the upper 16 bit of all memory accesses - vaddr &= 0xffffffffffffLL; - - if (vaddr >= 1uLL << current_page_table->GetAddressSpaceBits()) { - LOG_ERROR(HW_Memory, "Unmapped Write{} 0x{:08X} @ 0x{:016X}", sizeof(data) * 8, - static_cast(data[0]), vaddr); - return true; - } - - const uintptr_t raw_pointer = current_page_table->pointers[vaddr >> PAGE_BITS].Raw(); - if (u8* const pointer = Common::PageTable::PageInfo::ExtractPointer(raw_pointer)) { - // NOTE: Avoid adding any extra logic to this fast-path block - const auto volatile_pointer = reinterpret_cast(&pointer[vaddr]); + u8* const ptr = GetPointerImpl( + vaddr, + [vaddr, data]() { + LOG_ERROR(HW_Memory, + GetPointerImplError<"Write"_s, sizeof(u128), " = 0x{:016X}{:016X}"_s>(), + vaddr, static_cast(data[1]), static_cast(data[0])); + }, + [&system = system, vaddr]() { system.GPU().InvalidateRegion(vaddr, sizeof(u128)); }); + if (ptr) { + const auto volatile_pointer = reinterpret_cast(ptr); return Common::AtomicCompareAndSwap(volatile_pointer, data, expected); } - switch (Common::PageTable::PageInfo::ExtractType(raw_pointer)) { - case Common::PageType::Unmapped: - LOG_ERROR(HW_Memory, "Unmapped Write{} 0x{:08X} @ 0x{:016X}{:016X}", sizeof(data) * 8, - static_cast(data[1]), static_cast(data[0]), vaddr); - return true; - case Common::PageType::Memory: - ASSERT_MSG(false, "Mapped memory page without a pointer @ {:016X}", vaddr); - break; - case Common::PageType::RasterizerCachedMemory: { - u8* host_ptr{GetPointerFromRasterizerCachedMemory(vaddr)}; - system.GPU().InvalidateRegion(vaddr, sizeof(u128)); - auto* pointer = reinterpret_cast(&host_ptr); - return Common::AtomicCompareAndSwap(pointer, data, expected); - } - default: - UNREACHABLE(); - } return true; } -- cgit v1.2.3 From 5f97f74a9aeed0b00b7592becef58a6fc4943d6a Mon Sep 17 00:00:00 2001 From: yzct12345 <87620833+yzct12345@users.noreply.github.com> Date: Sat, 7 Aug 2021 03:03:21 +0000 Subject: memory: Address lioncash's review --- src/core/memory.cpp | 58 ++++++----------------------------------------------- 1 file changed, 6 insertions(+), 52 deletions(-) (limited to 'src/core/memory.cpp') diff --git a/src/core/memory.cpp b/src/core/memory.cpp index 778d152dd..51c4dea26 100644 --- a/src/core/memory.cpp +++ b/src/core/memory.cpp @@ -5,10 +5,6 @@ #include #include -#define BOOST_HANA_CONFIG_ENABLE_STRING_UDL -#include -#undef BOOST_HANA_CONFIG_ENABLE_STRING_UDL - #include "common/assert.h" #include "common/atomic_ops.h" #include "common/common_types.h" @@ -23,8 +19,6 @@ #include "core/memory.h" #include "video_core/gpu.h" -using namespace boost::hana::literals; - namespace Core::Memory { // Implementation class used to keep the specifics of the memory subsystem hidden @@ -446,42 +440,6 @@ struct Memory::Impl { } } - /** - * Returns a message like "Unmapped NameBits @ 0x{:016X}Suffix". - * - * @tparam NAME The caller name like "Read"_s or "Write"_s. - * @tparam BYTES The number of bits written. 0 is for read and sizeof(T) is for write. - * @tparam SUFFIX A suffix. ""_s is for read and " = 0x{:016X}" is for write. - */ - template - static consteval const char* GetPointerImplError() { - constexpr auto unmapped_fmt = ([]() { - constexpr auto prefix = "Unmapped "_s + NAME; - constexpr auto suffix = " @ 0x{:016X}"_s + SUFFIX; - const char* result = nullptr; - switch (BYTES * 8) { - case 0: - result = (prefix + suffix).c_str(); - break; -#define BITS_CASE(x) \ - case x: \ - result = (prefix + BOOST_HANA_STRING(#x) + suffix).c_str(); \ - break; - BITS_CASE(8) - BITS_CASE(16) - BITS_CASE(32) - BITS_CASE(64) - BITS_CASE(128) -#undef BITS_CASE - default: - break; - } - return result; - })(); - static_assert(unmapped_fmt); - return unmapped_fmt; - } - [[nodiscard]] u8* GetPointerImpl(VAddr vaddr, auto on_unmapped, auto on_rasterizer) const { // AARCH64 masks the upper 16 bit of all memory accesses vaddr &= 0xffffffffffffLL; @@ -516,10 +474,7 @@ struct Memory::Impl { [[nodiscard]] u8* GetPointer(const VAddr vaddr) const { return GetPointerImpl( - vaddr, - [vaddr]() { - LOG_ERROR(HW_Memory, GetPointerImplError<"GetPointer"_s, 0, ""_s>(), vaddr); - }, + vaddr, [vaddr]() { LOG_ERROR(HW_Memory, "Unmapped GetPointer @ 0x{:016X}", vaddr); }, []() {}); } @@ -540,7 +495,7 @@ struct Memory::Impl { const u8* const ptr = GetPointerImpl( vaddr, [vaddr]() { - LOG_ERROR(HW_Memory, GetPointerImplError<"Read"_s, sizeof(T), ""_s>(), vaddr); + LOG_ERROR(HW_Memory, "Unmapped Read{} @ 0x{:016X}", sizeof(T) * 8, vaddr); }, [&system = system, vaddr]() { system.GPU().FlushRegion(vaddr, sizeof(T)); }); if (ptr) { @@ -563,7 +518,7 @@ struct Memory::Impl { u8* const ptr = GetPointerImpl( vaddr, [vaddr, data]() { - LOG_ERROR(HW_Memory, GetPointerImplError<"Write"_s, sizeof(T), " = 0x{:016X}"_s>(), + LOG_ERROR(HW_Memory, "Unmapped Write{} @ 0x{:016X} = 0x{:016X}", sizeof(T) * 8, vaddr, static_cast(data)); }, [&system = system, vaddr]() { system.GPU().InvalidateRegion(vaddr, sizeof(T)); }); @@ -577,8 +532,8 @@ struct Memory::Impl { u8* const ptr = GetPointerImpl( vaddr, [vaddr, data]() { - LOG_ERROR(HW_Memory, GetPointerImplError<"Write"_s, sizeof(T), " = 0x{:016X}"_s>(), - vaddr, static_cast(data)); + LOG_ERROR(HW_Memory, "Unmapped WriteExclusive{} @ 0x{:016X} = 0x{:016X}", + sizeof(T) * 8, vaddr, static_cast(data)); }, [&system = system, vaddr]() { system.GPU().InvalidateRegion(vaddr, sizeof(T)); }); if (ptr) { @@ -592,8 +547,7 @@ struct Memory::Impl { u8* const ptr = GetPointerImpl( vaddr, [vaddr, data]() { - LOG_ERROR(HW_Memory, - GetPointerImplError<"Write"_s, sizeof(u128), " = 0x{:016X}{:016X}"_s>(), + LOG_ERROR(HW_Memory, "Unmapped WriteExclusive128 @ 0x{:016X} = 0x{:016X}{:016X}", vaddr, static_cast(data[1]), static_cast(data[0])); }, [&system = system, vaddr]() { system.GPU().InvalidateRegion(vaddr, sizeof(u128)); }); -- cgit v1.2.3