Merge pull request #4408 from wwylele/ro-map

ldr_ro: properly map CRS/CRO buffer

src/core/CMakeLists.txt

@@ -286,8 +286,6 @@ add_library(core STATIC
     hle/service/ldr_ro/cro_helper.h
     hle/service/ldr_ro/ldr_ro.cpp
     hle/service/ldr_ro/ldr_ro.h
-    hle/service/ldr_ro/memory_synchronizer.cpp
-    hle/service/ldr_ro/memory_synchronizer.h
     hle/service/mic_u.cpp
     hle/service/mic_u.h
     hle/service/mvd/mvd.cpp

src/core/hle/kernel/process.cpp

@@ -303,7 +303,8 @@ ResultCode Process::LinearFree(VAddr target, u32 size) {
     return RESULT_SUCCESS;
 }
 
-ResultCode Process::Map(VAddr target, VAddr source, u32 size, VMAPermission perms) {
+ResultCode Process::Map(VAddr target, VAddr source, u32 size, VMAPermission perms,
+                        bool privileged) {
     LOG_DEBUG(Kernel, "Map memory target={:08X}, source={:08X}, size={:08X}, perms={:08X}", target,
               source, size, static_cast<u8>(perms));
     if (source < Memory::HEAP_VADDR || source + size > Memory::HEAP_VADDR_END ||
@@ -320,22 +321,45 @@ ResultCode Process::Map(VAddr target, VAddr source, u32 size, VMAPermission perm
         return ERR_INVALID_ADDRESS_STATE;
     }
 
+    // Check range overlapping
+    if (source - target < size || target - source < size) {
+        if (privileged) {
+            if (source == target) {
+                // privileged Map allows identical source and target address, which simply changes
+                // the state and the permission of the memory
+                return vm_manager.ChangeMemoryState(source, size, MemoryState::Private,
+                                                    VMAPermission::ReadWrite,
+                                                    MemoryState::AliasCode, perms);
+            } else {
+                return ERR_INVALID_ADDRESS;
+            }
+        } else {
+            return ERR_INVALID_ADDRESS_STATE;
+        }
+    }
+
+    MemoryState source_state = privileged ? MemoryState::Locked : MemoryState::Aliased;
+    MemoryState target_state = privileged ? MemoryState::AliasCode : MemoryState::Alias;
+    VMAPermission source_perm = privileged ? VMAPermission::None : VMAPermission::ReadWrite;
+
     // Mark source region as Aliased
     CASCADE_CODE(vm_manager.ChangeMemoryState(source, size, MemoryState::Private,
-                                              VMAPermission::ReadWrite, MemoryState::Aliased,
+                                              VMAPermission::ReadWrite, source_state, source_perm));
-                                              VMAPermission::ReadWrite));
 
     CASCADE_RESULT(auto backing_blocks, vm_manager.GetBackingBlocksForRange(source, size));
     VAddr interval_target = target;
     for (const auto [backing_memory, block_size] : backing_blocks) {
-        auto target_vma = vm_manager.MapBackingMemory(interval_target, backing_memory, block_size,
+        auto target_vma =
-                                                      MemoryState::Alias);
+            vm_manager.MapBackingMemory(interval_target, backing_memory, block_size, target_state);
+        ASSERT(target_vma.Succeeded());
+        vm_manager.Reprotect(target_vma.Unwrap(), perms);
         interval_target += block_size;
     }
 
     return RESULT_SUCCESS;
 }
-ResultCode Process::Unmap(VAddr target, VAddr source, u32 size, VMAPermission perms) {
+ResultCode Process::Unmap(VAddr target, VAddr source, u32 size, VMAPermission perms,
+                          bool privileged) {
     LOG_DEBUG(Kernel, "Unmap memory target={:08X}, source={:08X}, size={:08X}, perms={:08X}",
               target, source, size, static_cast<u8>(perms));
     if (source < Memory::HEAP_VADDR || source + size > Memory::HEAP_VADDR_END ||
@@ -349,11 +373,29 @@ ResultCode Process::Unmap(VAddr target, VAddr source, u32 size, VMAPermission pe
     // TODO(wwylele): check that the source and the target are actually a pair created by Map
     // Should return error 0xD8E007F5 in this case
 
+    if (source - target < size || target - source < size) {
+        if (privileged) {
+            if (source == target) {
+                // privileged Unmap allows identical source and target address, which simply changes
+                // the state and the permission of the memory
+                return vm_manager.ChangeMemoryState(source, size, MemoryState::AliasCode,
+                                                    VMAPermission::None, MemoryState::Private,
+                                                    perms);
+            } else {
+                return ERR_INVALID_ADDRESS;
+            }
+        } else {
+            return ERR_INVALID_ADDRESS_STATE;
+        }
+    }
+
+    MemoryState source_state = privileged ? MemoryState::Locked : MemoryState::Aliased;
+
     CASCADE_CODE(vm_manager.UnmapRange(target, size));
 
     // Change back source region state. Note that the permission is reprotected according to param
-    CASCADE_CODE(vm_manager.ChangeMemoryState(source, size, MemoryState::Aliased,
+    CASCADE_CODE(vm_manager.ChangeMemoryState(source, size, source_state, VMAPermission::None,
-                                              VMAPermission::None, MemoryState::Private, perms));
+                                              MemoryState::Private, perms));
 
     return RESULT_SUCCESS;
 }

src/core/hle/kernel/process.h

@@ -188,8 +188,10 @@ public:
     ResultVal<VAddr> LinearAllocate(VAddr target, u32 size, VMAPermission perms);
     ResultCode LinearFree(VAddr target, u32 size);
 
-    ResultCode Map(VAddr target, VAddr source, u32 size, VMAPermission perms);
+    ResultCode Map(VAddr target, VAddr source, u32 size, VMAPermission perms,
-    ResultCode Unmap(VAddr target, VAddr source, u32 size, VMAPermission perms);
+                   bool privileged = false);
+    ResultCode Unmap(VAddr target, VAddr source, u32 size, VMAPermission perms,
+                     bool privileged = false);
 
 private:
     explicit Process(Kernel::KernelSystem& kernel);

src/core/hle/service/ldr_ro/ldr_ro.cpp

@@ -108,34 +108,11 @@ void RO::Initialize(Kernel::HLERequestContext& ctx) {
 
     ResultCode result = RESULT_SUCCESS;
 
-    if (crs_buffer_ptr != crs_address) {
+    result = process->Map(crs_address, crs_buffer_ptr, crs_size, Kernel::VMAPermission::Read, true);
-        // TODO(wwylele): should be memory aliasing
+    if (result.IsError()) {
-        std::shared_ptr<std::vector<u8>> crs_mem = std::make_shared<std::vector<u8>>(crs_size);
+        LOG_ERROR(Service_LDR, "Error mapping memory block {:08X}", result.raw);
-        Memory::ReadBlock(crs_buffer_ptr, crs_mem->data(), crs_size);
+        rb.Push(result);
-        result = process->vm_manager
+        return;
-                     .MapMemoryBlock(crs_address, crs_mem, 0, crs_size, Kernel::MemoryState::Code)
-                     .Code();
-        if (result.IsError()) {
-            LOG_ERROR(Service_LDR, "Error mapping memory block {:08X}", result.raw);
-            rb.Push(result);
-            return;
-        }
-
-        result =
-            process->vm_manager.ReprotectRange(crs_address, crs_size, Kernel::VMAPermission::Read);
-        if (result.IsError()) {
-            LOG_ERROR(Service_LDR, "Error reprotecting memory block {:08X}", result.raw);
-            rb.Push(result);
-            return;
-        }
-
-        slot->memory_synchronizer.AddMemoryBlock(crs_address, crs_buffer_ptr, crs_size);
-    } else {
-        // Do nothing if buffer_ptr == address
-        // TODO(wwylele): verify this behaviour. This is only seen in the web browser app,
-        //     and the actual behaviour is unclear. "Do nothing" is probably an incorrect implement.
-        //     There is also a chance that another issue causes the app passing wrong arguments.
-        LOG_WARNING(Service_LDR, "crs_buffer_ptr == crs_address (0x{:08X})", crs_address);
     }
 
     CROHelper crs(crs_address);
@@ -148,8 +125,6 @@ void RO::Initialize(Kernel::HLERequestContext& ctx) {
         return;
     }
 
-    slot->memory_synchronizer.SynchronizeOriginalMemory(*process);
-
     slot->loaded_crs = crs_address;
 
     rb.Push(RESULT_SUCCESS);
@@ -266,38 +241,12 @@ void RO::LoadCRO(Kernel::HLERequestContext& ctx, bool link_on_load_bug_fix) {
 
     ResultCode result = RESULT_SUCCESS;
 
-    if (cro_buffer_ptr != cro_address) {
+    result = process->Map(cro_address, cro_buffer_ptr, cro_size, Kernel::VMAPermission::Read, true);
-        // TODO(wwylele): should be memory aliasing
+    if (result.IsError()) {
-        std::shared_ptr<std::vector<u8>> cro_mem = std::make_shared<std::vector<u8>>(cro_size);
+        LOG_ERROR(Service_LDR, "Error mapping memory block {:08X}", result.raw);
-        Memory::ReadBlock(cro_buffer_ptr, cro_mem->data(), cro_size);
+        rb.Push(result);
-        result = process->vm_manager
+        rb.Push<u32>(0);
-                     .MapMemoryBlock(cro_address, cro_mem, 0, cro_size, Kernel::MemoryState::Code)
+        return;
-                     .Code();
-        if (result.IsError()) {
-            LOG_ERROR(Service_LDR, "Error mapping memory block {:08X}", result.raw);
-            rb.Push(result);
-            rb.Push<u32>(0);
-            return;
-        }
-
-        result =
-            process->vm_manager.ReprotectRange(cro_address, cro_size, Kernel::VMAPermission::Read);
-        if (result.IsError()) {
-            LOG_ERROR(Service_LDR, "Error reprotecting memory block {:08X}", result.raw);
-            process->vm_manager.UnmapRange(cro_address, cro_size);
-            rb.Push(result);
-            rb.Push<u32>(0);
-            return;
-        }
-
-        slot->memory_synchronizer.AddMemoryBlock(cro_address, cro_buffer_ptr, cro_size);
-    } else {
-        // Do nothing if buffer_ptr == address
-        // TODO(wwylele): verify this behaviour.
-        //     This is derived from the case of LoadCRS with buffer_ptr==address,
-        //     and is never seen in any game. "Do nothing" is probably an incorrect implement.
-        //     There is also a chance that this case is just prohibited.
-        LOG_WARNING(Service_LDR, "cro_buffer_ptr == cro_address (0x{:08X})", cro_address);
     }
 
     CROHelper cro(cro_address);
@@ -305,7 +254,8 @@ void RO::LoadCRO(Kernel::HLERequestContext& ctx, bool link_on_load_bug_fix) {
     result = cro.VerifyHash(cro_size, crr_address);
     if (result.IsError()) {
         LOG_ERROR(Service_LDR, "Error verifying CRO in CRR {:08X}", result.raw);
-        process->vm_manager.UnmapRange(cro_address, cro_size);
+        process->Unmap(cro_address, cro_buffer_ptr, cro_size, Kernel::VMAPermission::ReadWrite,
+                       true);
         rb.Push(result);
         rb.Push<u32>(0);
         return;
@@ -315,7 +265,8 @@ void RO::LoadCRO(Kernel::HLERequestContext& ctx, bool link_on_load_bug_fix) {
                         bss_segment_address, bss_segment_size, false);
     if (result.IsError()) {
         LOG_ERROR(Service_LDR, "Error rebasing CRO {:08X}", result.raw);
-        process->vm_manager.UnmapRange(cro_address, cro_size);
+        process->Unmap(cro_address, cro_buffer_ptr, cro_size, Kernel::VMAPermission::ReadWrite,
+                       true);
         rb.Push(result);
         rb.Push<u32>(0);
         return;
@@ -324,7 +275,8 @@ void RO::LoadCRO(Kernel::HLERequestContext& ctx, bool link_on_load_bug_fix) {
     result = cro.Link(slot->loaded_crs, link_on_load_bug_fix);
     if (result.IsError()) {
         LOG_ERROR(Service_LDR, "Error linking CRO {:08X}", result.raw);
-        process->vm_manager.UnmapRange(cro_address, cro_size);
+        process->Unmap(cro_address, cro_buffer_ptr, cro_size, Kernel::VMAPermission::ReadWrite,
+                       true);
         rb.Push(result);
         rb.Push<u32>(0);
         return;
@@ -334,23 +286,17 @@ void RO::LoadCRO(Kernel::HLERequestContext& ctx, bool link_on_load_bug_fix) {
 
     u32 fix_size = cro.Fix(fix_level);
 
-    slot->memory_synchronizer.SynchronizeOriginalMemory(*process);
+    if (fix_size != cro_size) {
-
+        result = process->Unmap(cro_address + fix_size, cro_buffer_ptr + fix_size,
-    // TODO(wwylele): verify the behaviour when buffer_ptr == address
+                                cro_size - fix_size, Kernel::VMAPermission::ReadWrite, true);
-    if (cro_buffer_ptr != cro_address) {
+        if (result.IsError()) {
-        if (fix_size != cro_size) {
+            LOG_ERROR(Service_LDR, "Error unmapping memory block {:08X}", result.raw);
-            result = process->vm_manager.UnmapRange(cro_address + fix_size, cro_size - fix_size);
+            process->Unmap(cro_address, cro_buffer_ptr, cro_size, Kernel::VMAPermission::ReadWrite,
-            if (result.IsError()) {
+                           true);
-                LOG_ERROR(Service_LDR, "Error unmapping memory block {:08X}", result.raw);
+            rb.Push(result);
-                process->vm_manager.UnmapRange(cro_address, cro_size);
+            rb.Push<u32>(0);
-                rb.Push(result);
+            return;
-                rb.Push<u32>(0);
-                return;
-            }
         }
-
-        // Changes the block size
-        slot->memory_synchronizer.ResizeMemoryBlock(cro_address, cro_buffer_ptr, fix_size);
     }
 
     auto [exe_begin, exe_size] = cro.GetExecutablePages();
@@ -359,7 +305,8 @@ void RO::LoadCRO(Kernel::HLERequestContext& ctx, bool link_on_load_bug_fix) {
                                                     Kernel::VMAPermission::ReadExecute);
         if (result.IsError()) {
             LOG_ERROR(Service_LDR, "Error reprotecting memory block {:08X}", result.raw);
-            process->vm_manager.UnmapRange(cro_address, fix_size);
+            process->Unmap(cro_address, cro_buffer_ptr, cro_size, Kernel::VMAPermission::ReadWrite,
+                           true);
             rb.Push(result);
             rb.Push<u32>(0);
             return;
@@ -433,15 +380,10 @@ void RO::UnloadCRO(Kernel::HLERequestContext& ctx) {
 
     cro.Unrebase(false);
 
-    slot->memory_synchronizer.SynchronizeOriginalMemory(*process);
+    result = process->Unmap(cro_address, cro_buffer_ptr, fixed_size,
-
+                            Kernel::VMAPermission::ReadWrite, true);
-    // TODO(wwylele): verify the behaviour when buffer_ptr == address
+    if (result.IsError()) {
-    if (cro_address != cro_buffer_ptr) {
+        LOG_ERROR(Service_LDR, "Error unmapping CRO {:08X}", result.raw);
-        result = process->vm_manager.UnmapRange(cro_address, fixed_size);
-        if (result.IsError()) {
-            LOG_ERROR(Service_LDR, "Error unmapping CRO {:08X}", result.raw);
-        }
-        slot->memory_synchronizer.RemoveMemoryBlock(cro_address, cro_buffer_ptr);
     }
 
     Core::CPU().InvalidateCacheRange(cro_address, fixed_size);
@@ -486,8 +428,6 @@ void RO::LinkCRO(Kernel::HLERequestContext& ctx) {
         LOG_ERROR(Service_LDR, "Error linking CRO {:08X}", result.raw);
     }
 
-    slot->memory_synchronizer.SynchronizeOriginalMemory(*process);
-
     rb.Push(result);
 }
 
@@ -528,8 +468,6 @@ void RO::UnlinkCRO(Kernel::HLERequestContext& ctx) {
         LOG_ERROR(Service_LDR, "Error unlinking CRO {:08X}", result.raw);
     }
 
-    slot->memory_synchronizer.SynchronizeOriginalMemory(*process);
-
     rb.Push(result);
 }
 
@@ -552,17 +490,12 @@ void RO::Shutdown(Kernel::HLERequestContext& ctx) {
     CROHelper crs(slot->loaded_crs);
     crs.Unrebase(true);
 
-    slot->memory_synchronizer.SynchronizeOriginalMemory(*process);
-
     ResultCode result = RESULT_SUCCESS;
 
-    // TODO(wwylele): verify the behaviour when buffer_ptr == address
+    result = process->Unmap(slot->loaded_crs, crs_buffer_ptr, crs.GetFileSize(),
-    if (slot->loaded_crs != crs_buffer_ptr) {
+                            Kernel::VMAPermission::ReadWrite, true);
-        result = process->vm_manager.UnmapRange(slot->loaded_crs, crs.GetFileSize());
+    if (result.IsError()) {
-        if (result.IsError()) {
+        LOG_ERROR(Service_LDR, "Error unmapping CRS {:08X}", result.raw);
-            LOG_ERROR(Service_LDR, "Error unmapping CRS {:08X}", result.raw);
-        }
-        slot->memory_synchronizer.RemoveMemoryBlock(slot->loaded_crs, crs_buffer_ptr);
     }
 
     slot->loaded_crs = 0;

src/core/hle/service/ldr_ro/ldr_ro.h

@@ -4,7 +4,6 @@
 
 #pragma once
 
-#include "core/hle/service/ldr_ro/memory_synchronizer.h"
 #include "core/hle/service/service.h"
 
 namespace Core {
@@ -14,7 +13,6 @@ class System;
 namespace Service::LDR {
 
 struct ClientSlot : public Kernel::SessionRequestHandler::SessionDataBase {
-    MemorySynchronizer memory_synchronizer;
     VAddr loaded_crs = 0; ///< the virtual address of the static module
 };
 

src/core/hle/service/ldr_ro/memory_synchronizer.cpp

@@ -1,41 +0,0 @@
-// Copyright 2016 Citra Emulator Project
-// Licensed under GPLv2 or any later version
-// Refer to the license.txt file included.
-
-#include <algorithm>
-#include "common/assert.h"
-#include "core/hle/kernel/process.h"
-#include "core/hle/service/ldr_ro/memory_synchronizer.h"
-
-namespace Service::LDR {
-
-auto MemorySynchronizer::FindMemoryBlock(VAddr mapping, VAddr original) {
-    auto block = std::find_if(memory_blocks.begin(), memory_blocks.end(),
-                              [=](MemoryBlock& b) { return b.original == original; });
-    ASSERT(block->mapping == mapping);
-    return block;
-}
-
-void MemorySynchronizer::Clear() {
-    memory_blocks.clear();
-}
-
-void MemorySynchronizer::AddMemoryBlock(VAddr mapping, VAddr original, u32 size) {
-    memory_blocks.push_back(MemoryBlock{mapping, original, size});
-}
-
-void MemorySynchronizer::ResizeMemoryBlock(VAddr mapping, VAddr original, u32 size) {
-    FindMemoryBlock(mapping, original)->size = size;
-}
-
-void MemorySynchronizer::RemoveMemoryBlock(VAddr mapping, VAddr original) {
-    memory_blocks.erase(FindMemoryBlock(mapping, original));
-}
-
-void MemorySynchronizer::SynchronizeOriginalMemory(Kernel::Process& process) {
-    for (auto& block : memory_blocks) {
-        Memory::CopyBlock(process, block.original, block.mapping, block.size);
-    }
-}
-
-} // namespace Service::LDR

src/core/hle/service/ldr_ro/memory_synchronizer.h

@@ -1,44 +0,0 @@
-// Copyright 2016 Citra Emulator Project
-// Licensed under GPLv2 or any later version
-// Refer to the license.txt file included.
-
-#pragma once
-
-#include <vector>
-#include "core/memory.h"
-
-namespace Kernel {
-class Process;
-}
-
-namespace Service::LDR {
-
-/**
- * This is a work-around before we implement memory aliasing.
- * CRS and CRO are mapped (aliased) to another memory when loading. Games can read
- * from both the original buffer and the mapping memory. So we use this to synchronize
- * all original buffers with mapping memory after modifying the content.
- */
-class MemorySynchronizer {
-public:
-    void Clear();
-
-    void AddMemoryBlock(VAddr mapping, VAddr original, u32 size);
-    void ResizeMemoryBlock(VAddr mapping, VAddr original, u32 size);
-    void RemoveMemoryBlock(VAddr mapping, VAddr original);
-
-    void SynchronizeOriginalMemory(Kernel::Process& process);
-
-private:
-    struct MemoryBlock {
-        VAddr mapping;
-        VAddr original;
-        u32 size;
-    };
-
-    std::vector<MemoryBlock> memory_blocks;
-
-    auto FindMemoryBlock(VAddr mapping, VAddr original);
-};
-
-} // namespace Service::LDR