From 6596fe091c3e69e206b277b4adfd5c177eec5f56 Mon Sep 17 00:00:00 2001 From: Borchev <4501931+Borchev@users.noreply.github.com> Date: Tue, 20 Aug 2024 16:10:38 -0700 Subject: [PATCH 1/2] Workaround for readonly memory mapping of files issue --- src/common/io_file.cpp | 5 ++--- src/core/address_space.cpp | 28 +++++++++++++++++++++------- src/core/address_space.h | 2 +- src/core/memory.cpp | 3 ++- 4 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/common/io_file.cpp b/src/common/io_file.cpp index e1c22d2a..fbc37a10 100644 --- a/src/common/io_file.cpp +++ b/src/common/io_file.cpp @@ -217,7 +217,7 @@ void IOFile::Close() { file = nullptr; #ifdef _WIN64 - if (file_mapping) { + if (file_mapping && file_access_mode == FileAccessMode::ReadWrite) { CloseHandle(std::bit_cast(file_mapping)); } #endif @@ -259,8 +259,7 @@ uintptr_t IOFile::GetFileMapping() { mapping = CreateFileMapping2(hfile, NULL, FILE_MAP_WRITE, PAGE_READWRITE, SEC_COMMIT, 0, NULL, NULL, 0); } else { - mapping = CreateFileMapping2(hfile, NULL, FILE_MAP_READ, PAGE_READONLY, SEC_COMMIT, 0, NULL, - NULL, 0); + mapping = hfile; } file_mapping = std::bit_cast(mapping); diff --git a/src/core/address_space.cpp b/src/core/address_space.cpp index 77d021a6..6444790f 100644 --- a/src/core/address_space.cpp +++ b/src/core/address_space.cpp @@ -2,6 +2,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include +#include "common/alignment.h" #include "common/assert.h" #include "common/error.h" #include "core/address_space.h" @@ -129,9 +130,10 @@ struct AddressSpace::Impl { } void* Map(VAddr virtual_addr, PAddr phys_addr, size_t size, ULONG prot, uintptr_t fd = 0) { + const size_t aligned_size = Common::AlignUp(size, 16_KB); const auto it = placeholders.find(virtual_addr); ASSERT_MSG(it != placeholders.end(), "Cannot map already mapped region"); - ASSERT_MSG(virtual_addr >= it->lower() && virtual_addr + size <= it->upper(), + ASSERT_MSG(virtual_addr >= it->lower() && virtual_addr + aligned_size <= it->upper(), "Map range must be fully contained in a placeholder"); // Windows only allows splitting a placeholder into two. @@ -140,7 +142,7 @@ struct AddressSpace::Impl { // one at the start and at the end. const VAddr placeholder_start = it->lower(); const VAddr placeholder_end = it->upper(); - const VAddr virtual_end = virtual_addr + size; + const VAddr virtual_end = virtual_addr + aligned_size; // If the placeholder doesn't exactly start at virtual_addr, split it at the start. if (placeholder_start != virtual_addr) { @@ -161,11 +163,23 @@ struct AddressSpace::Impl { void* ptr = nullptr; if (phys_addr != -1) { HANDLE backing = fd ? reinterpret_cast(fd) : backing_handle; - ptr = MapViewOfFile3(backing, process, reinterpret_cast(virtual_addr), phys_addr, - size, MEM_REPLACE_PLACEHOLDER, prot, nullptr, 0); + if (fd && prot == PAGE_READONLY) { + DWORD resultvar; + ptr = VirtualAlloc2(process, reinterpret_cast(virtual_addr), aligned_size, + MEM_RESERVE | MEM_COMMIT | MEM_REPLACE_PLACEHOLDER, + PAGE_READWRITE, nullptr, 0); + bool ret = ReadFile(backing, ptr, size, &resultvar, NULL); + ASSERT_MSG(ret, "ReadFile failed. {}", Common::GetLastErrorMsg()); + ret = VirtualProtect(ptr, size, prot, &resultvar); + ASSERT_MSG(ret, "VirtualProtect failed. {}", Common::GetLastErrorMsg()); + } else { + ptr = MapViewOfFile3(backing, process, reinterpret_cast(virtual_addr), + phys_addr, aligned_size, MEM_REPLACE_PLACEHOLDER, prot, + nullptr, 0); + } } else { ptr = - VirtualAlloc2(process, reinterpret_cast(virtual_addr), size, + VirtualAlloc2(process, reinterpret_cast(virtual_addr), aligned_size, MEM_RESERVE | MEM_COMMIT | MEM_REPLACE_PLACEHOLDER, prot, nullptr, 0); } ASSERT_MSG(ptr, "{}", Common::GetLastErrorMsg()); @@ -455,12 +469,12 @@ void* AddressSpace::MapFile(VAddr virtual_addr, size_t size, size_t offset, u32 } void AddressSpace::Unmap(VAddr virtual_addr, size_t size, VAddr start_in_vma, VAddr end_in_vma, - PAddr phys_base, bool is_exec, bool has_backing) { + PAddr phys_base, bool is_exec, bool has_backing, bool readonly) { #ifdef _WIN32 // There does not appear to be comparable support for partial unmapping on Windows. // Unfortunately, a least one title was found to require this. The workaround is to unmap // the entire allocation and remap the portions outside of the requested unmapping range. - impl->Unmap(virtual_addr, size, has_backing); + impl->Unmap(virtual_addr, size, has_backing && !readonly); // TODO: Determine if any titles require partial unmapping support for flexible allocations. ASSERT_MSG(has_backing || (start_in_vma == 0 && end_in_vma == size), diff --git a/src/core/address_space.h b/src/core/address_space.h index 53041bcc..dc38de4d 100644 --- a/src/core/address_space.h +++ b/src/core/address_space.h @@ -92,7 +92,7 @@ public: /// Unmaps specified virtual memory area. void Unmap(VAddr virtual_addr, size_t size, VAddr start_in_vma, VAddr end_in_vma, - PAddr phys_base, bool is_exec, bool has_backing); + PAddr phys_base, bool is_exec, bool has_backing, bool readonly); void Protect(VAddr virtual_addr, size_t size, MemoryPermission perms); diff --git a/src/core/memory.cpp b/src/core/memory.cpp index 6d0d581f..6c3b5005 100644 --- a/src/core/memory.cpp +++ b/src/core/memory.cpp @@ -242,10 +242,11 @@ void MemoryManager::UnmapMemory(VAddr virtual_addr, size_t size) { vma.disallow_merge = false; vma.name = ""; MergeAdjacent(vma_map, new_it); + bool readonly = vma.prot == MemoryProt::CpuRead; // Unmap the memory region. impl.Unmap(vma_base_addr, vma_base_size, start_in_vma, start_in_vma + size, phys_base, is_exec, - has_backing); + has_backing, readonly); TRACK_FREE(virtual_addr, "VMEM"); } From fc300b526561a2d57cb69a4442c24d4c532bb4cc Mon Sep 17 00:00:00 2001 From: Borchev <4501931+Borchev@users.noreply.github.com> Date: Tue, 20 Aug 2024 20:07:32 -0700 Subject: [PATCH 2/2] Fix unmapping bug --- src/core/address_space.cpp | 4 ++-- src/core/address_space.h | 2 +- src/core/memory.cpp | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/address_space.cpp b/src/core/address_space.cpp index 6444790f..23511370 100644 --- a/src/core/address_space.cpp +++ b/src/core/address_space.cpp @@ -469,12 +469,12 @@ void* AddressSpace::MapFile(VAddr virtual_addr, size_t size, size_t offset, u32 } void AddressSpace::Unmap(VAddr virtual_addr, size_t size, VAddr start_in_vma, VAddr end_in_vma, - PAddr phys_base, bool is_exec, bool has_backing, bool readonly) { + PAddr phys_base, bool is_exec, bool has_backing, bool readonly_file) { #ifdef _WIN32 // There does not appear to be comparable support for partial unmapping on Windows. // Unfortunately, a least one title was found to require this. The workaround is to unmap // the entire allocation and remap the portions outside of the requested unmapping range. - impl->Unmap(virtual_addr, size, has_backing && !readonly); + impl->Unmap(virtual_addr, size, has_backing && !readonly_file); // TODO: Determine if any titles require partial unmapping support for flexible allocations. ASSERT_MSG(has_backing || (start_in_vma == 0 && end_in_vma == size), diff --git a/src/core/address_space.h b/src/core/address_space.h index dc38de4d..2a3488d5 100644 --- a/src/core/address_space.h +++ b/src/core/address_space.h @@ -92,7 +92,7 @@ public: /// Unmaps specified virtual memory area. void Unmap(VAddr virtual_addr, size_t size, VAddr start_in_vma, VAddr end_in_vma, - PAddr phys_base, bool is_exec, bool has_backing, bool readonly); + PAddr phys_base, bool is_exec, bool has_backing, bool readonly_file); void Protect(VAddr virtual_addr, size_t size, MemoryPermission perms); diff --git a/src/core/memory.cpp b/src/core/memory.cpp index 6c3b5005..552c4039 100644 --- a/src/core/memory.cpp +++ b/src/core/memory.cpp @@ -242,11 +242,11 @@ void MemoryManager::UnmapMemory(VAddr virtual_addr, size_t size) { vma.disallow_merge = false; vma.name = ""; MergeAdjacent(vma_map, new_it); - bool readonly = vma.prot == MemoryProt::CpuRead; + bool readonly_file = vma.prot == MemoryProt::CpuRead && type == VMAType::File; // Unmap the memory region. impl.Unmap(vma_base_addr, vma_base_size, start_in_vma, start_in_vma + size, phys_base, is_exec, - has_backing, readonly); + has_backing, readonly_file); TRACK_FREE(virtual_addr, "VMEM"); }