From d4c1b9d311c978a6354574d09c451522ceb74e82 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 6 Dec 2018 10:59:22 -0500 Subject: [PATCH 1/2] vm_manager: Make vma_map private This was only ever public so that code could check whether or not a handle was valid or not. Instead of exposing the object directly and allowing external code to potentially mess with the map contents, we just provide a member function that allows checking whether or not a handle is valid. This makes all member variables of the VMManager class private except for the page table. --- src/core/hle/kernel/shared_memory.cpp | 8 ++++---- src/core/hle/kernel/svc.cpp | 20 ++++++++++++-------- src/core/hle/kernel/vm_manager.cpp | 4 ++++ src/core/hle/kernel/vm_manager.h | 26 ++++++++++++++++---------- src/core/memory.cpp | 13 ++++++------- 5 files changed, 42 insertions(+), 29 deletions(-) diff --git a/src/core/hle/kernel/shared_memory.cpp b/src/core/hle/kernel/shared_memory.cpp index 0494581f56..d1ca601254 100644 --- a/src/core/hle/kernel/shared_memory.cpp +++ b/src/core/hle/kernel/shared_memory.cpp @@ -39,15 +39,15 @@ SharedPtr SharedMemory::Create(KernelCore& kernel, SharedPtrbacking_block.get()); } } else { - auto& vm_manager = shared_memory->owner_process->VMManager(); + const auto& vm_manager = shared_memory->owner_process->VMManager(); // The memory is already available and mapped in the owner process. - auto vma = vm_manager.FindVMA(address); - ASSERT_MSG(vma != vm_manager.vma_map.end(), "Invalid memory address"); + const auto vma = vm_manager.FindVMA(address); + ASSERT_MSG(vm_manager.IsValidHandle(vma), "Invalid memory address"); ASSERT_MSG(vma->second.backing_block, "Backing block doesn't exist for address"); // The returned VMA might be a bigger one encompassing the desired address. - auto vma_offset = address - vma->first; + const auto vma_offset = address - vma->first; ASSERT_MSG(vma_offset + size <= vma->second.size, "Shared memory exceeds bounds of mapped block"); diff --git a/src/core/hle/kernel/svc.cpp b/src/core/hle/kernel/svc.cpp index e6c77f9db8..049d756d07 100644 --- a/src/core/hle/kernel/svc.cpp +++ b/src/core/hle/kernel/svc.cpp @@ -239,7 +239,7 @@ static ResultCode SetMemoryPermission(VAddr addr, u64 size, u32 prot) { } const VMManager::VMAHandle iter = vm_manager.FindVMA(addr); - if (iter == vm_manager.vma_map.end()) { + if (!vm_manager.IsValidHandle(iter)) { LOG_ERROR(Kernel_SVC, "Unable to find VMA for address=0x{:016X}", addr); return ERR_INVALID_ADDRESS_STATE; } @@ -1077,19 +1077,23 @@ static ResultCode QueryProcessMemory(MemoryInfo* memory_info, PageInfo* /*page_i process_handle); return ERR_INVALID_HANDLE; } - auto vma = process->VMManager().FindVMA(addr); + + const auto& vm_manager = process->VMManager(); + const auto vma = vm_manager.FindVMA(addr); + memory_info->attributes = 0; - if (vma == process->VMManager().vma_map.end()) { - memory_info->base_address = 0; - memory_info->permission = static_cast(VMAPermission::None); - memory_info->size = 0; - memory_info->type = static_cast(MemoryState::Unmapped); - } else { + if (vm_manager.IsValidHandle(vma)) { memory_info->base_address = vma->second.base; memory_info->permission = static_cast(vma->second.permissions); memory_info->size = vma->second.size; memory_info->type = static_cast(vma->second.meminfo_state); + } else { + memory_info->base_address = 0; + memory_info->permission = static_cast(VMAPermission::None); + memory_info->size = 0; + memory_info->type = static_cast(MemoryState::Unmapped); } + return RESULT_SUCCESS; } diff --git a/src/core/hle/kernel/vm_manager.cpp b/src/core/hle/kernel/vm_manager.cpp index 100f8f6bfe..6187993ce3 100644 --- a/src/core/hle/kernel/vm_manager.cpp +++ b/src/core/hle/kernel/vm_manager.cpp @@ -87,6 +87,10 @@ VMManager::VMAHandle VMManager::FindVMA(VAddr target) const { } } +bool VMManager::IsValidHandle(VMAHandle handle) const { + return handle != vma_map.cend(); +} + ResultVal VMManager::MapMemoryBlock(VAddr target, std::shared_ptr> block, std::size_t offset, u64 size, diff --git a/src/core/hle/kernel/vm_manager.h b/src/core/hle/kernel/vm_manager.h index d522404fe8..a12419d1e2 100644 --- a/src/core/hle/kernel/vm_manager.h +++ b/src/core/hle/kernel/vm_manager.h @@ -113,16 +113,10 @@ struct VirtualMemoryArea { * - http://duartes.org/gustavo/blog/post/page-cache-the-affair-between-memory-and-files/ */ class VMManager final { + using VMAMap = std::map; + public: - /** - * A map covering the entirety of the managed address space, keyed by the `base` field of each - * VMA. It must always be modified by splitting or merging VMAs, so that the invariant - * `elem.base + elem.size == next.base` is preserved, and mergeable regions must always be - * merged when possible so that no two similar and adjacent regions exist that have not been - * merged. - */ - std::map vma_map; - using VMAHandle = decltype(vma_map)::const_iterator; + using VMAHandle = VMAMap::const_iterator; VMManager(); ~VMManager(); @@ -133,6 +127,9 @@ public: /// Finds the VMA in which the given address is included in, or `vma_map.end()`. VMAHandle FindVMA(VAddr target) const; + /// Indicates whether or not the given handle is within the VMA map. + bool IsValidHandle(VMAHandle handle) const; + // TODO(yuriks): Should these functions actually return the handle? /** @@ -281,7 +278,7 @@ public: Memory::PageTable page_table; private: - using VMAIter = decltype(vma_map)::iterator; + using VMAIter = VMAMap::iterator; /// Converts a VMAHandle to a mutable VMAIter. VMAIter StripIterConstness(const VMAHandle& iter); @@ -328,6 +325,15 @@ private: /// Clears out the page table void ClearPageTable(); + /** + * A map covering the entirety of the managed address space, keyed by the `base` field of each + * VMA. It must always be modified by splitting or merging VMAs, so that the invariant + * `elem.base + elem.size == next.base` is preserved, and mergeable regions must always be + * merged when possible so that no two similar and adjacent regions exist that have not been + * merged. + */ + VMAMap vma_map; + u32 address_space_width = 0; VAddr address_space_base = 0; VAddr address_space_end = 0; diff --git a/src/core/memory.cpp b/src/core/memory.cpp index 41fd2a6a01..76f468c788 100644 --- a/src/core/memory.cpp +++ b/src/core/memory.cpp @@ -125,14 +125,13 @@ void RemoveDebugHook(PageTable& page_table, VAddr base, u64 size, MemoryHookPoin * using a VMA from the current process */ static u8* GetPointerFromVMA(const Kernel::Process& process, VAddr vaddr) { + const auto& vm_manager = process.VMManager(); + + const auto it = vm_manager.FindVMA(vaddr); + ASSERT(vm_manager.IsValidHandle(it)); + u8* direct_pointer = nullptr; - - auto& vm_manager = process.VMManager(); - - auto it = vm_manager.FindVMA(vaddr); - ASSERT(it != vm_manager.vma_map.end()); - - auto& vma = it->second; + const auto& vma = it->second; switch (vma.type) { case Kernel::VMAType::AllocatedMemoryBlock: direct_pointer = vma.backing_block->data() + vma.offset; From 15e3d4f357f02275bc10818536f563b08f3326c9 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Thu, 6 Dec 2018 14:21:22 -0500 Subject: [PATCH 2/2] memory: Convert ASSERT into a DEBUG_ASSERT within GetPointerFromVMA() Given memory should always be expected to be valid during normal execution, this should be a debug assertion, rather than a check in regular builds. --- src/core/memory.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/memory.cpp b/src/core/memory.cpp index 76f468c788..643afdee87 100644 --- a/src/core/memory.cpp +++ b/src/core/memory.cpp @@ -128,7 +128,7 @@ static u8* GetPointerFromVMA(const Kernel::Process& process, VAddr vaddr) { const auto& vm_manager = process.VMManager(); const auto it = vm_manager.FindVMA(vaddr); - ASSERT(vm_manager.IsValidHandle(it)); + DEBUG_ASSERT(vm_manager.IsValidHandle(it)); u8* direct_pointer = nullptr; const auto& vma = it->second;