From c4fde2713d64812e45a1f44cae9c950e5bc4dfb1 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 24 Jul 2019 06:50:15 -0400 Subject: [PATCH 1/8] kernel/vm_manager: Simplify some assertion messages Assertions already log out the function name, so there's no need to manually include the function name in the assertion strings. --- src/core/hle/kernel/vm_manager.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/core/hle/kernel/vm_manager.cpp b/src/core/hle/kernel/vm_manager.cpp index 40cea1e7cc..21ab012401 100644 --- a/src/core/hle/kernel/vm_manager.cpp +++ b/src/core/hle/kernel/vm_manager.cpp @@ -330,7 +330,7 @@ ResultCode VMManager::MapPhysicalMemory(VAddr target, u64 size) { cur_addr = target; auto iter = FindVMA(target); - ASSERT_MSG(iter != vma_map.end(), "MapPhysicalMemory iter != end"); + ASSERT(iter != vma_map.end()); while (true) { const auto& vma = iter->second; @@ -360,7 +360,7 @@ ResultCode VMManager::MapPhysicalMemory(VAddr target, u64 size) { // Advance to the next block. cur_addr = vma_end; iter = FindVMA(cur_addr); - ASSERT_MSG(iter != vma_map.end(), "MapPhysicalMemory iter != end"); + ASSERT(iter != vma_map.end()); } } @@ -368,7 +368,7 @@ ResultCode VMManager::MapPhysicalMemory(VAddr target, u64 size) { if (result.IsError()) { for (const auto [unmap_address, unmap_size] : mapped_regions) { ASSERT_MSG(UnmapRange(unmap_address, unmap_size).IsSuccess(), - "MapPhysicalMemory un-map on error"); + "Failed to unmap memory range."); } return result; @@ -407,7 +407,7 @@ ResultCode VMManager::UnmapPhysicalMemory(VAddr target, u64 size) { cur_addr = target; auto iter = FindVMA(target); - ASSERT_MSG(iter != vma_map.end(), "UnmapPhysicalMemory iter != end"); + ASSERT(iter != vma_map.end()); while (true) { const auto& vma = iter->second; @@ -434,7 +434,7 @@ ResultCode VMManager::UnmapPhysicalMemory(VAddr target, u64 size) { // Advance to the next block. cur_addr = vma_end; iter = FindVMA(cur_addr); - ASSERT_MSG(iter != vma_map.end(), "UnmapPhysicalMemory iter != end"); + ASSERT(iter != vma_map.end()); } } @@ -445,7 +445,7 @@ ResultCode VMManager::UnmapPhysicalMemory(VAddr target, u64 size) { const auto remap_res = MapMemoryBlock(map_address, std::make_shared(map_size, 0), 0, map_size, MemoryState::Heap, VMAPermission::None); - ASSERT_MSG(remap_res.Succeeded(), "UnmapPhysicalMemory re-map on error"); + ASSERT_MSG(remap_res.Succeeded(), "Failed to remap a memory block."); } } @@ -965,7 +965,7 @@ ResultVal VMManager::SizeOfAllocatedVMAsInRange(VAddr address, VAddr cur_addr = address; auto iter = FindVMA(cur_addr); - ASSERT_MSG(iter != vma_map.end(), "SizeOfAllocatedVMAsInRange iter != end"); + ASSERT(iter != vma_map.end()); while (true) { const auto& vma = iter->second; @@ -986,7 +986,7 @@ ResultVal VMManager::SizeOfAllocatedVMAsInRange(VAddr address, // Advance to the next block. cur_addr = vma_end; iter = std::next(iter); - ASSERT_MSG(iter != vma_map.end(), "SizeOfAllocatedVMAsInRange iter != end"); + ASSERT(iter != vma_map.end()); } return MakeResult(mapped_size); @@ -1000,7 +1000,7 @@ ResultVal VMManager::SizeOfUnmappablePhysicalMemoryInRange(VAddr ad VAddr cur_addr = address; auto iter = FindVMA(cur_addr); - ASSERT_MSG(iter != vma_map.end(), "SizeOfUnmappablePhysicalMemoryInRange iter != end"); + ASSERT(iter != vma_map.end()); while (true) { const auto& vma = iter->second; @@ -1029,7 +1029,7 @@ ResultVal VMManager::SizeOfUnmappablePhysicalMemoryInRange(VAddr ad // Advance to the next block. cur_addr = vma_end; iter = std::next(iter); - ASSERT_MSG(iter != vma_map.end(), "SizeOfUnmappablePhysicalMemoryInRange iter != end"); + ASSERT(iter != vma_map.end()); } return MakeResult(mapped_size); From bd55b991209d6e6cc82fdf7e2ce14869fab89caf Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 24 Jul 2019 06:57:57 -0400 Subject: [PATCH 2/8] kernel/vm_manager: Simplify some std::vector constructor calls Same behavior, one less magic constant to read. --- src/core/hle/kernel/vm_manager.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/hle/kernel/vm_manager.cpp b/src/core/hle/kernel/vm_manager.cpp index 21ab012401..231e42baaf 100644 --- a/src/core/hle/kernel/vm_manager.cpp +++ b/src/core/hle/kernel/vm_manager.cpp @@ -342,7 +342,7 @@ ResultCode VMManager::MapPhysicalMemory(VAddr target, u64 size) { const auto map_size = std::min(end_addr - cur_addr, vma_end - cur_addr); if (vma.state == MemoryState::Unmapped) { const auto map_res = - MapMemoryBlock(cur_addr, std::make_shared(map_size, 0), 0, + MapMemoryBlock(cur_addr, std::make_shared(map_size), 0, map_size, MemoryState::Heap, VMAPermission::ReadWrite); result = map_res.Code(); if (result.IsError()) { @@ -443,7 +443,7 @@ ResultCode VMManager::UnmapPhysicalMemory(VAddr target, u64 size) { if (result.IsError()) { for (const auto [map_address, map_size] : unmapped_regions) { const auto remap_res = - MapMemoryBlock(map_address, std::make_shared(map_size, 0), 0, + MapMemoryBlock(map_address, std::make_shared(map_size), 0, map_size, MemoryState::Heap, VMAPermission::None); ASSERT_MSG(remap_res.Succeeded(), "Failed to remap a memory block."); } From 1249e837bac915b96ba58b113b23f9c95d6c084d Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 24 Jul 2019 07:14:47 -0400 Subject: [PATCH 3/8] kernel/vm_manager: Deduplicate iterator creation in MergeAdjacentVMA Avoids needing to read the same long sequence of code in both code paths. Also makes it slightly nicer to read and debug, as the locals will be able to be shown in the debugger. --- src/core/hle/kernel/vm_manager.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/core/hle/kernel/vm_manager.cpp b/src/core/hle/kernel/vm_manager.cpp index 231e42baaf..e86796ba58 100644 --- a/src/core/hle/kernel/vm_manager.cpp +++ b/src/core/hle/kernel/vm_manager.cpp @@ -757,19 +757,22 @@ void VMManager::MergeAdjacentVMA(VirtualMemoryArea& left, const VirtualMemoryAre // Always merge allocated memory blocks, even when they don't share the same backing block. if (left.type == VMAType::AllocatedMemoryBlock && (left.backing_block != right.backing_block || left.offset + left.size != right.offset)) { + const auto right_begin = right.backing_block->begin() + right.offset; + const auto right_end = right_begin + right.size; + // Check if we can save work. if (left.offset == 0 && left.size == left.backing_block->size()) { // Fast case: left is an entire backing block. - left.backing_block->insert(left.backing_block->end(), - right.backing_block->begin() + right.offset, - right.backing_block->begin() + right.offset + right.size); + left.backing_block->insert(left.backing_block->end(), right_begin, right_end); } else { + const auto left_begin = left.backing_block->begin() + left.offset; + const auto left_end = left_begin + left.size; + // Slow case: make a new memory block for left and right. auto new_memory = std::make_shared(); - new_memory->insert(new_memory->end(), left.backing_block->begin() + left.offset, - left.backing_block->begin() + left.offset + left.size); - new_memory->insert(new_memory->end(), right.backing_block->begin() + right.offset, - right.backing_block->begin() + right.offset + right.size); + new_memory->insert(new_memory->end(), left_begin, left_end); + new_memory->insert(new_memory->end(), right_begin, right_end); + left.backing_block = new_memory; left.offset = 0; } From eb1f6e7cddd78a7dce85a527d527fe4a73a6147e Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 24 Jul 2019 07:18:48 -0400 Subject: [PATCH 4/8] kernel/vm_manager: std::move shared_ptr instance in MergeAdjacentVMA Avoids an unnecessary atomic reference count increment and decrement. --- src/core/hle/kernel/vm_manager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/hle/kernel/vm_manager.cpp b/src/core/hle/kernel/vm_manager.cpp index e86796ba58..721f7cc44d 100644 --- a/src/core/hle/kernel/vm_manager.cpp +++ b/src/core/hle/kernel/vm_manager.cpp @@ -773,7 +773,7 @@ void VMManager::MergeAdjacentVMA(VirtualMemoryArea& left, const VirtualMemoryAre new_memory->insert(new_memory->end(), left_begin, left_end); new_memory->insert(new_memory->end(), right_begin, right_end); - left.backing_block = new_memory; + left.backing_block = std::move(new_memory); left.offset = 0; } From aed2815ba60e5e7efb38ef8c04e65b21a3af8ca1 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 24 Jul 2019 07:24:19 -0400 Subject: [PATCH 5/8] kernel/vm_manager: Reserve memory ahead of time for slow path in MergeAdjacentVMA Avoids potentially expensive (depending on the size of the memory block) allocations by reserving the necessary memory before performing both insertions. This avoids scenarios where the second insert may cause a reallocation to occur. --- src/core/hle/kernel/vm_manager.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/core/hle/kernel/vm_manager.cpp b/src/core/hle/kernel/vm_manager.cpp index 721f7cc44d..6b2d78cc81 100644 --- a/src/core/hle/kernel/vm_manager.cpp +++ b/src/core/hle/kernel/vm_manager.cpp @@ -765,11 +765,14 @@ void VMManager::MergeAdjacentVMA(VirtualMemoryArea& left, const VirtualMemoryAre // Fast case: left is an entire backing block. left.backing_block->insert(left.backing_block->end(), right_begin, right_end); } else { + // Slow case: make a new memory block for left and right. const auto left_begin = left.backing_block->begin() + left.offset; const auto left_end = left_begin + left.size; + const auto left_size = static_cast(std::distance(left_begin, left_end)); + const auto right_size = static_cast(std::distance(right_begin, right_end)); - // Slow case: make a new memory block for left and right. auto new_memory = std::make_shared(); + new_memory->reserve(left_size + right_size); new_memory->insert(new_memory->end(), left_begin, left_end); new_memory->insert(new_memory->end(), right_begin, right_end); From ff201725eddfb48abe5a41e5b044087a8825f9d4 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 24 Jul 2019 07:33:37 -0400 Subject: [PATCH 6/8] kernel/vm_manager: Correct behavior in failure case of UnmapPhysicalMemory() If an unmapping operation fails, we shouldn't be decrementing the amount of memory mapped and returning that the operation was successful. We should actually be returning the error code in this case. --- src/core/hle/kernel/vm_manager.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/core/hle/kernel/vm_manager.cpp b/src/core/hle/kernel/vm_manager.cpp index 6b2d78cc81..6ec4159cab 100644 --- a/src/core/hle/kernel/vm_manager.cpp +++ b/src/core/hle/kernel/vm_manager.cpp @@ -447,6 +447,8 @@ ResultCode VMManager::UnmapPhysicalMemory(VAddr target, u64 size) { map_size, MemoryState::Heap, VMAPermission::None); ASSERT_MSG(remap_res.Succeeded(), "Failed to remap a memory block."); } + + return result; } // Update mapped amount From 4ca5db278f1e765813fce74f539394d5b9e39e5f Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 24 Jul 2019 07:40:02 -0400 Subject: [PATCH 7/8] kernel/vm_manager: Move variables closer to usage spots in MapPhysicalMemory/UnmapPhysicalMemory Narrows the scope of variables down to where they're only necessary. --- src/core/hle/kernel/vm_manager.cpp | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/core/hle/kernel/vm_manager.cpp b/src/core/hle/kernel/vm_manager.cpp index 6ec4159cab..c7af870732 100644 --- a/src/core/hle/kernel/vm_manager.cpp +++ b/src/core/hle/kernel/vm_manager.cpp @@ -296,12 +296,6 @@ ResultVal VMManager::SetHeapSize(u64 size) { } ResultCode VMManager::MapPhysicalMemory(VAddr target, u64 size) { - const auto end_addr = target + size; - const auto last_addr = end_addr - 1; - VAddr cur_addr = target; - - ResultCode result = RESULT_SUCCESS; - // Check how much memory we've already mapped. const auto mapped_size_result = SizeOfAllocatedVMAsInRange(target, size); if (mapped_size_result.Failed()) { @@ -324,10 +318,13 @@ ResultCode VMManager::MapPhysicalMemory(VAddr target, u64 size) { // Keep track of the memory regions we unmap. std::vector> mapped_regions; + ResultCode result = RESULT_SUCCESS; // Iterate, trying to map memory. { - cur_addr = target; + const auto end_addr = target + size; + const auto last_addr = end_addr - 1; + VAddr cur_addr = target; auto iter = FindVMA(target); ASSERT(iter != vma_map.end()); @@ -381,12 +378,6 @@ ResultCode VMManager::MapPhysicalMemory(VAddr target, u64 size) { } ResultCode VMManager::UnmapPhysicalMemory(VAddr target, u64 size) { - const auto end_addr = target + size; - const auto last_addr = end_addr - 1; - VAddr cur_addr = target; - - ResultCode result = RESULT_SUCCESS; - // Check how much memory is currently mapped. const auto mapped_size_result = SizeOfUnmappablePhysicalMemoryInRange(target, size); if (mapped_size_result.Failed()) { @@ -401,10 +392,13 @@ ResultCode VMManager::UnmapPhysicalMemory(VAddr target, u64 size) { // Keep track of the memory regions we unmap. std::vector> unmapped_regions; + ResultCode result = RESULT_SUCCESS; // Try to unmap regions. { - cur_addr = target; + const auto end_addr = target + size; + const auto last_addr = end_addr - 1; + VAddr cur_addr = target; auto iter = FindVMA(target); ASSERT(iter != vma_map.end()); @@ -443,8 +437,8 @@ ResultCode VMManager::UnmapPhysicalMemory(VAddr target, u64 size) { if (result.IsError()) { for (const auto [map_address, map_size] : unmapped_regions) { const auto remap_res = - MapMemoryBlock(map_address, std::make_shared(map_size), 0, - map_size, MemoryState::Heap, VMAPermission::None); + MapMemoryBlock(map_address, std::make_shared(map_size), 0, map_size, + MemoryState::Heap, VMAPermission::None); ASSERT_MSG(remap_res.Succeeded(), "Failed to remap a memory block."); } From c4cdbfdbff2c091dd8250351dddc1fa08368b38d Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 24 Jul 2019 07:45:45 -0400 Subject: [PATCH 8/8] kernel/vm_manager: Correct doxygen comment parameter tags for MapPhysicalMemory/UnmapPhysicalMemory Corrects the parameter names within the doxygen comments so that they resolve properly. --- src/core/hle/kernel/vm_manager.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/hle/kernel/vm_manager.h b/src/core/hle/kernel/vm_manager.h index b18cde6197..850a7ebc3f 100644 --- a/src/core/hle/kernel/vm_manager.h +++ b/src/core/hle/kernel/vm_manager.h @@ -454,8 +454,8 @@ public: /// Maps memory at a given address. /// - /// @param addr The virtual address to map memory at. - /// @param size The amount of memory to map. + /// @param target The virtual address to map memory at. + /// @param size The amount of memory to map. /// /// @note The destination address must lie within the Map region. /// @@ -468,8 +468,8 @@ public: /// Unmaps memory at a given address. /// - /// @param addr The virtual address to unmap memory at. - /// @param size The amount of memory to unmap. + /// @param target The virtual address to unmap memory at. + /// @param size The amount of memory to unmap. /// /// @note The destination address must lie within the Map region. ///