Merge pull request #10277 from AdmiralCurtiss/memarena-cleanup

MemArena: Cleanup & interface changes for safer memory mapping.

Source/Core/Common/CMakeLists.txt

@@ -85,7 +85,6 @@ add_library(common
   Matrix.h
   MD5.cpp
   MD5.h
-  MemArena.cpp
   MemArena.h
   MemoryUtil.cpp
   MemoryUtil.h
@@ -185,16 +184,19 @@ if(ANDROID)
     AndroidAnalytics.cpp
     AndroidAnalytics.h
     Logging/ConsoleListenerDroid.cpp
+    MemArenaAndroid.cpp
   )
 elseif(WIN32)
   target_sources(common PRIVATE
     LdrWatcher.cpp
     LdrWatcher.h
     Logging/ConsoleListenerWin.cpp
+    MemArenaWin.cpp
   )
 else()
   target_sources(common PRIVATE
     Logging/ConsoleListenerNix.cpp
+    MemArenaUnix.cpp
   )
 endif()
 

Source/Core/Common/MemArena.h

@@ -15,18 +15,87 @@ namespace Common
 {
 // This class lets you create a block of anonymous RAM, and then arbitrarily map views into it.
 // Multiple views can mirror the same section of the block, which makes it very convenient for
-// emulating
-// memory mirrors.
-class MemArena
+// emulating memory mirrors.
+class MemArena final
 {
 public:
+  MemArena();
+  ~MemArena();
+  MemArena(const MemArena&) = delete;
+  MemArena(MemArena&&) = delete;
+  MemArena& operator=(const MemArena&) = delete;
+  MemArena& operator=(MemArena&&) = delete;
+
+  ///
+  /// Allocate the singular memory segment handled by this MemArena. This will be the actual
+  /// 'physical' available memory for this arena. After allocation, it can be interacted with using
+  /// CreateView() and ReleaseView(). Used to make a mappable region for emulated memory.
+  ///
+  /// @param size The amount of bytes that should be allocated in this region.
+  ///
   void GrabSHMSegment(size_t size);
+
+  ///
+  /// Release the memory segment previously allocated with GrabSHMSegment().
+  /// Should not be called before all views have been released.
+  ///
   void ReleaseSHMSegment();
-  void* CreateView(s64 offset, size_t size, void* base = nullptr);
+
+  ///
+  /// Map a memory region in the memory segment previously allocated with GrabSHMSegment().
+  ///
+  /// @param offset Offset within the memory segment to map at.
+  /// @param size Size of the region to map.
+  ///
+  /// @return Pointer to the memory region, or nullptr on failure.
+  ///
+  void* CreateView(s64 offset, size_t size);
+
+  ///
+  /// Unmap a memory region previously mapped with CreateView().
+  /// Should not be called on a view that is still mapped into the virtual memory region.
+  ///
+  /// @param view Pointer returned by CreateView().
+  /// @param size Size passed to the corresponding CreateView() call.
+  ///
   void ReleaseView(void* view, size_t size);
 
-  // This finds 1 GB in 32-bit, 16 GB in 64-bit.
-  static u8* FindMemoryBase();
+  ///
+  /// Reserve the singular 'virtual' memory region handled by this MemArena. This is used to create
+  /// our 'fastmem' memory area for the emulated game code to access directly.
+  ///
+  /// @param memory_size Size in bytes of the memory region to reserve.
+  ///
+  /// @return Pointer to the memory region, or nullptr on failure.
+  ///
+  u8* ReserveMemoryRegion(size_t memory_size);
+
+  ///
+  /// Release the memory region previously reserved with ReserveMemoryRegion().
+  /// Should not be called while any memory region is still mapped.
+  ///
+  void ReleaseMemoryRegion();
+
+  ///
+  /// Map a section from the memory segment previously allocated with GrabSHMSegment()
+  /// into the region previously reserved with ReserveMemoryRegion().
+  ///
+  /// @param offset Offset within the memory segment previous allocated by GrabSHMSegment() to map
+  /// from.
+  /// @param size Size of the region to map.
+  /// @param base Address within the memory region from ReserveMemoryRegion() where to map it.
+  ///
+  /// @return The address we actually ended up mapping, which should be the given 'base'.
+  ///
+  void* MapInMemoryRegion(s64 offset, size_t size, void* base);
+
+  ///
+  /// Unmap a memory region previously mapped with MapInMemoryRegion().
+  ///
+  /// @param view Pointer returned by MapInMemoryRegion().
+  /// @param size Size passed to the corresponding MapInMemoryRegion() call.
+  ///
+  void UnmapFromMemoryRegion(void* view, size_t size);
 
 private:
 #ifdef _WIN32

Source/Core/Common/MemArenaAndroid.cpp

@@ -3,35 +3,28 @@
 
 #include "Common/MemArena.h"
 
+#include <cerrno>
 #include <cstddef>
 #include <cstdlib>
+#include <cstring>
 #include <set>
 #include <string>
 
+#include <dlfcn.h>
+#include <fcntl.h>
+#include <linux/ashmem.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
 #include "Common/CommonFuncs.h"
 #include "Common/CommonTypes.h"
 #include "Common/Logging/Log.h"
 #include "Common/MsgHandler.h"
 #include "Common/StringUtil.h"
 
-#ifdef _WIN32
-#include <windows.h>
-#else
-#include <cerrno>
-#include <cstring>
-#include <fcntl.h>
-#include <sys/mman.h>
-#include <unistd.h>
-#ifdef ANDROID
-#include <dlfcn.h>
-#include <linux/ashmem.h>
-#include <sys/ioctl.h>
-#endif
-#endif
-
 namespace Common
 {
-#ifdef ANDROID
 #define ASHMEM_DEVICE "/dev/ashmem"
 
 static int AshmemCreateFileMapping(const char* name, size_t size)
@@ -65,50 +58,54 @@ static int AshmemCreateFileMapping(const char* name, size_t size)
   }
   return fd;
 }
-#endif
+
+MemArena::MemArena() = default;
+MemArena::~MemArena() = default;
 
 void MemArena::GrabSHMSegment(size_t size)
 {
-#ifdef _WIN32
-  const std::string name = "dolphin-emu." + std::to_string(GetCurrentProcessId());
-  hMemoryMapping = CreateFileMapping(INVALID_HANDLE_VALUE, nullptr, PAGE_READWRITE, 0,
-                                     static_cast<DWORD>(size), UTF8ToTStr(name).c_str());
-#elif defined(ANDROID)
   fd = AshmemCreateFileMapping(("dolphin-emu." + std::to_string(getpid())).c_str(), size);
   if (fd < 0)
-  {
     NOTICE_LOG_FMT(MEMMAP, "Ashmem allocation failed");
-    return;
-  }
-#else
-  const std::string file_name = "/dolphin-emu." + std::to_string(getpid());
-  fd = shm_open(file_name.c_str(), O_RDWR | O_CREAT | O_EXCL, 0600);
-  if (fd == -1)
-  {
-    ERROR_LOG_FMT(MEMMAP, "shm_open failed: {}", strerror(errno));
-    return;
-  }
-  shm_unlink(file_name.c_str());
-  if (ftruncate(fd, size) < 0)
-    ERROR_LOG_FMT(MEMMAP, "Failed to allocate low memory space");
-#endif
 }
 
 void MemArena::ReleaseSHMSegment()
 {
-#ifdef _WIN32
-  CloseHandle(hMemoryMapping);
-  hMemoryMapping = 0;
-#else
   close(fd);
-#endif
 }
 
-void* MemArena::CreateView(s64 offset, size_t size, void* base)
+void* MemArena::CreateView(s64 offset, size_t size)
+{
+  return MapInMemoryRegion(offset, size, nullptr);
+}
+
+void MemArena::ReleaseView(void* view, size_t size)
+{
+  UnmapFromMemoryRegion(view, size);
+}
+
+u8* MemArena::ReserveMemoryRegion(size_t memory_size)
+{
+  // Android 4.3 changed how mmap works.
+  // if we map it private and then munmap it, we can't use the base returned.
+  // This may be due to changes in them to support a full SELinux implementation.
+  const int flags = MAP_ANON | MAP_SHARED;
+  void* base = mmap(nullptr, memory_size, PROT_NONE, flags, -1, 0);
+  if (base == MAP_FAILED)
+  {
+    PanicAlertFmt("Failed to map enough memory space: {}", LastStrerrorString());
+    return nullptr;
+  }
+  munmap(base, memory_size);
+  return static_cast<u8*>(base);
+}
+
+void MemArena::ReleaseMemoryRegion()
+{
+}
+
+void* MemArena::MapInMemoryRegion(s64 offset, size_t size, void* base)
 {
-#ifdef _WIN32
-  return MapViewOfFileEx(hMemoryMapping, FILE_MAP_ALL_ACCESS, 0, (DWORD)((u64)offset), size, base);
-#else
   void* retval = mmap(base, size, PROT_READ | PROT_WRITE,
                       MAP_SHARED | ((base == nullptr) ? 0 : MAP_FIXED), fd, offset);
 
@@ -121,53 +118,10 @@ void* MemArena::CreateView(s64 offset, size_t size, void* base)
   {
     return retval;
   }
-#endif
 }
 
-void MemArena::ReleaseView(void* view, size_t size)
+void MemArena::UnmapFromMemoryRegion(void* view, size_t size)
 {
-#ifdef _WIN32
-  UnmapViewOfFile(view);
-#else
   munmap(view, size);
-#endif
 }
-
-u8* MemArena::FindMemoryBase()
-{
-#if _ARCH_32
-  const size_t memory_size = 0x31000000;
-#else
-  const size_t memory_size = 0x400000000;
-#endif
-
-#ifdef _WIN32
-  u8* base = static_cast<u8*>(VirtualAlloc(nullptr, memory_size, MEM_RESERVE, PAGE_READWRITE));
-  if (!base)
-  {
-    PanicAlertFmt("Failed to map enough memory space: {}", GetLastErrorString());
-    return nullptr;
-  }
-  VirtualFree(base, 0, MEM_RELEASE);
-  return base;
-#else
-#ifdef ANDROID
-  // Android 4.3 changed how mmap works.
-  // if we map it private and then munmap it, we can't use the base returned.
-  // This may be due to changes in them support a full SELinux implementation.
-  const int flags = MAP_ANON | MAP_SHARED;
-#else
-  const int flags = MAP_ANON | MAP_PRIVATE;
-#endif
-  void* base = mmap(nullptr, memory_size, PROT_NONE, flags, -1, 0);
-  if (base == MAP_FAILED)
-  {
-    PanicAlertFmt("Failed to map enough memory space: {}", LastStrerrorString());
-    return nullptr;
-  }
-  munmap(base, memory_size);
-  return static_cast<u8*>(base);
-#endif
-}
-
 }  // namespace Common

Source/Core/Common/MemArenaUnix.cpp

@@ -0,0 +1,94 @@
+// Copyright 2008 Dolphin Emulator Project
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include "Common/MemArena.h"
+
+#include <cerrno>
+#include <cstddef>
+#include <cstdlib>
+#include <cstring>
+#include <set>
+#include <string>
+
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+#include "Common/CommonFuncs.h"
+#include "Common/CommonTypes.h"
+#include "Common/Logging/Log.h"
+#include "Common/MsgHandler.h"
+#include "Common/StringUtil.h"
+
+namespace Common
+{
+MemArena::MemArena() = default;
+MemArena::~MemArena() = default;
+
+void MemArena::GrabSHMSegment(size_t size)
+{
+  const std::string file_name = "/dolphin-emu." + std::to_string(getpid());
+  fd = shm_open(file_name.c_str(), O_RDWR | O_CREAT | O_EXCL, 0600);
+  if (fd == -1)
+  {
+    ERROR_LOG_FMT(MEMMAP, "shm_open failed: {}", strerror(errno));
+    return;
+  }
+  shm_unlink(file_name.c_str());
+  if (ftruncate(fd, size) < 0)
+    ERROR_LOG_FMT(MEMMAP, "Failed to allocate low memory space");
+}
+
+void MemArena::ReleaseSHMSegment()
+{
+  close(fd);
+}
+
+void* MemArena::CreateView(s64 offset, size_t size)
+{
+  return MapInMemoryRegion(offset, size, nullptr);
+}
+
+void MemArena::ReleaseView(void* view, size_t size)
+{
+  UnmapFromMemoryRegion(view, size);
+}
+
+u8* MemArena::ReserveMemoryRegion(size_t memory_size)
+{
+  const int flags = MAP_ANON | MAP_PRIVATE;
+  void* base = mmap(nullptr, memory_size, PROT_NONE, flags, -1, 0);
+  if (base == MAP_FAILED)
+  {
+    PanicAlertFmt("Failed to map enough memory space: {}", LastStrerrorString());
+    return nullptr;
+  }
+  munmap(base, memory_size);
+  return static_cast<u8*>(base);
+}
+
+void MemArena::ReleaseMemoryRegion()
+{
+}
+
+void* MemArena::MapInMemoryRegion(s64 offset, size_t size, void* base)
+{
+  void* retval = mmap(base, size, PROT_READ | PROT_WRITE,
+                      MAP_SHARED | ((base == nullptr) ? 0 : MAP_FIXED), fd, offset);
+
+  if (retval == MAP_FAILED)
+  {
+    NOTICE_LOG_FMT(MEMMAP, "mmap failed");
+    return nullptr;
+  }
+  else
+  {
+    return retval;
+  }
+}
+
+void MemArena::UnmapFromMemoryRegion(void* view, size_t size)
+{
+  munmap(view, size);
+}
+}  // namespace Common

Source/Core/Common/MemArenaWin.cpp

@@ -0,0 +1,72 @@
+// Copyright 2008 Dolphin Emulator Project
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include "Common/MemArena.h"
+
+#include <cstddef>
+#include <cstdlib>
+#include <set>
+#include <string>
+
+#include <windows.h>
+
+#include "Common/CommonFuncs.h"
+#include "Common/CommonTypes.h"
+#include "Common/Logging/Log.h"
+#include "Common/MsgHandler.h"
+#include "Common/StringUtil.h"
+
+namespace Common
+{
+MemArena::MemArena() = default;
+MemArena::~MemArena() = default;
+
+void MemArena::GrabSHMSegment(size_t size)
+{
+  const std::string name = "dolphin-emu." + std::to_string(GetCurrentProcessId());
+  hMemoryMapping = CreateFileMapping(INVALID_HANDLE_VALUE, nullptr, PAGE_READWRITE, 0,
+                                     static_cast<DWORD>(size), UTF8ToTStr(name).c_str());
+}
+
+void MemArena::ReleaseSHMSegment()
+{
+  CloseHandle(hMemoryMapping);
+  hMemoryMapping = 0;
+}
+
+void* MemArena::CreateView(s64 offset, size_t size)
+{
+  return MapInMemoryRegion(offset, size, nullptr);
+}
+
+void MemArena::ReleaseView(void* view, size_t size)
+{
+  UnmapFromMemoryRegion(view, size);
+}
+
+u8* MemArena::ReserveMemoryRegion(size_t memory_size)
+{
+  u8* base = static_cast<u8*>(VirtualAlloc(nullptr, memory_size, MEM_RESERVE, PAGE_READWRITE));
+  if (!base)
+  {
+    PanicAlertFmt("Failed to map enough memory space: {}", GetLastErrorString());
+    return nullptr;
+  }
+  VirtualFree(base, 0, MEM_RELEASE);
+  return base;
+}
+
+void MemArena::ReleaseMemoryRegion()
+{
+}
+
+void* MemArena::MapInMemoryRegion(s64 offset, size_t size, void* base)
+{
+  return MapViewOfFileEx(hMemoryMapping, FILE_MAP_ALL_ACCESS, 0, (DWORD)((u64)offset), size, base);
+}
+
+void MemArena::UnmapFromMemoryRegion(void* view, size_t size)
+{
+  UnmapViewOfFile(view);
+}
+}  // namespace Common

Source/Core/Core/HW/Memmap.cpp

@@ -314,7 +314,12 @@ void Init()
 
 bool InitFastmemArena()
 {
-  physical_base = Common::MemArena::FindMemoryBase();
+#if _ARCH_32
+  const size_t memory_size = 0x31000000;
+#else
+  const size_t memory_size = 0x400000000;
+#endif
+  physical_base = g_arena.ReserveMemoryRegion(memory_size);
 
   if (!physical_base)
   {
@@ -328,7 +333,7 @@ bool InitFastmemArena()
       continue;
 
     u8* base = physical_base + region.physical_address;
-    u8* view = (u8*)g_arena.CreateView(region.shm_position, region.size, base);
+    u8* view = (u8*)g_arena.MapInMemoryRegion(region.shm_position, region.size, base);
 
     if (base != view)
     {
@@ -354,7 +359,7 @@ void UpdateLogicalMemory(const PowerPC::BatTable& dbat_table)
 
   for (auto& entry : logical_mapped_entries)
   {
-    g_arena.ReleaseView(entry.mapped_pointer, entry.mapped_size);
+    g_arena.UnmapFromMemoryRegion(entry.mapped_pointer, entry.mapped_size);
   }
   logical_mapped_entries.clear();
   for (u32 i = 0; i < dbat_table.size(); ++i)
@@ -381,7 +386,7 @@ void UpdateLogicalMemory(const PowerPC::BatTable& dbat_table)
           u8* base = logical_base + logical_address + intersection_start - translated_address;
           u32 mapped_size = intersection_end - intersection_start;
 
-          void* mapped_pointer = g_arena.CreateView(position, mapped_size, base);
+          void* mapped_pointer = g_arena.MapInMemoryRegion(position, mapped_size, base);
           if (!mapped_pointer)
           {
             PanicAlertFmt("Memory::UpdateLogicalMemory(): Failed to map memory region at 0x{:08X} "
@@ -439,15 +444,17 @@ void ShutdownFastmemArena()
       continue;
 
     u8* base = physical_base + region.physical_address;
-    g_arena.ReleaseView(base, region.size);
+    g_arena.UnmapFromMemoryRegion(base, region.size);
   }
 
   for (auto& entry : logical_mapped_entries)
   {
-    g_arena.ReleaseView(entry.mapped_pointer, entry.mapped_size);
+    g_arena.UnmapFromMemoryRegion(entry.mapped_pointer, entry.mapped_size);
   }
   logical_mapped_entries.clear();
 
+  g_arena.ReleaseMemoryRegion();
+
   physical_base = nullptr;
   logical_base = nullptr;
 

Source/Core/DolphinLib.props

@@ -726,7 +726,7 @@
     <ClCompile Include="Common\MathUtil.cpp" />
     <ClCompile Include="Common\Matrix.cpp" />
     <ClCompile Include="Common\MD5.cpp" />
-    <ClCompile Include="Common\MemArena.cpp" />
+    <ClCompile Include="Common\MemArenaWin.cpp" />
     <ClCompile Include="Common\MemoryUtil.cpp" />
     <ClCompile Include="Common\MsgHandler.cpp" />
     <ClCompile Include="Common\NandPaths.cpp" />