From 3f695333cd4c2307a2dbab05fbc1c17bd9abda4b 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 70485e690bbab456290999a2796fa46f8f55e4fd 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 785c4946ddf79c0c4454c003baa20cab6ebb63ca 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 a43ee8d752187bee8cc1dbfe8fef8b27e891b974 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 56c6f767ae7368a2b2355e002384bf4d5f672132 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 96cc9a92794309d83f03a34d34a6f2f24a68ac3e 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 b0da7e4262c128ba2355a4b07806bd19aa67701f 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 f763e230830019f4a430da7558ee4cbbb0684534 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. ///