Add support for a shared memory mapping on Kernel & User Memory #1468

Giragast · 2023-11-23T02:59:12Z

This allows me to write to game memory from Unity in realtime, without having to go through GDB, Lua, etc, which are (in my experience) unable to keep up with writes being sent at 50fps+

… Memory

ghost · 2023-11-23T03:00:01Z

👇 Click on the image for a new way to code review

Legend

codecov · 2023-11-23T03:16:28Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (54c9d5d) 8.59% compared to head (728731a) 8.67%.
Report is 6 commits behind head on main.

Files	Patch %	Lines
src/support/sharedmem-unix.cc	75.00%	8 Missing ⚠️
src/gui/widgets/memory_observer.cc	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1468      +/-   ##
==========================================
+ Coverage    8.59%    8.67%   +0.07%     
==========================================
  Files         408      413       +5     
  Lines      125609   125847     +238     
==========================================
+ Hits        10802    10923     +121     
- Misses     114807   114924     +117

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nicolasnoble · 2023-11-23T04:11:09Z

So, several comments here.

I'm not at all opposed to this idea. In fact, I believe it's a great addition. This was low on my TODO list, but I definitely had it in mind. But this means I'd be a bit more opiniated on how to do it :-)
We may simply want to do it all the time. There's little downside in having the shared memory on, as opposed to just using malloc. I'd just drop the setting to enable it, and simplify the codepath in general.
We may still want to do this in a cross-platform way, not just for Windows. It feels like we should have a new class in the src/support directory, which would allow to RAII the shared memory pages.
- All allocated memory, not just the main RAM, probably want to have its own shared memory object. The name pattern could reflect that. We could even allow the SoftGPU implementation to have its VRAM shared too.
- The Unix version of shared memory pages is using the shm_open API call. I can help writing this if needed.

Suggestions:

Yeet common code to src/support/sharedmem.{h,cc} (please notice the different license scheme in this directory). The SharedMem class could simply take the filename pattern to use, and the size of the allocation, and expose the memory pointer.
Create src/support/sharedmem-windows.cc for the Windows-specific implementation of the methods of the class. Avoid splitting functions with #ifdef, and instead have the whole file being disabled with a single #ifdef at its top.
Change psxmem.{h,cc} to use this new SharedMem object, allowing for RAII to destroy it cleanly.
If you're not comfortable writing src/support/sharedmem-unix.cc using shm_open / mmap / mmunmap / shm_unlink, I can take care of this part. The actual filename prefix for Unix needs to be std::filesystem::temp_directory_path().

…atform-specific implementations for Windows and Linux (not OSX), removed toggle to enable named shared memory

Giragast · 2023-11-24T18:01:13Z

Okay, I've refactored things into something that's hopefully sensible and there's a few questions I have left

I'm assuming the implementation of linux is the same as macosx (from what I've read) - do you want sharedmem-macosx.cc (which is the old base behaviour currently) removed and rename sharedmem-linux.cc to sharedmem-nix.cc or sharedmem-other.cc or similar?
g_emulator->m_mem->m_wram.m_mem has clunky naming, so you want SharedMem::m_mem renamed to something else?
I've implemented m_wram with SharedMem and I'm torn between two ways to reference the memory:

//HEADER
  public:
    SharedMem m_wram;  // Kernel & User Memory (8 Meg)
//SOURCE
  //init
  m_wram.init(fmt::format("pcsx-redux-wram-{}", pid).c_str(), 0x00800000);
  //referenced as
  g_emulator->m_mem->m_wram.m_mem
  //no clean-up needed

VS

//HEADER
  public:
    SharedMem m_wramShared;  // Kernel & User Memory (8 Meg)
    uint8_t *m_wram = nullptr;
//SOURCE
  //init
  m_wramShared.init(fmt::format("pcsx-redux-wram-{}", pid).c_str(), 0x00800000);
  m_wram = m_wramShared.m_mem;
  //referenced as
  g_emulator->m_mem->m_wram
  //clean-up
  m_wram = nullptr;

Is the naming format pcsx-redux-[memtype]-[pid] okay?
Once everything is good, do you have a list of memory blocks to convert to SharedMem? I'm assuming m_wram, m_exp1, m_bios and m_hard. If you want m_writeLUT and m_readLUT then SharedMem needs to be templated, which should be fine

nicolasnoble · 2023-11-24T18:48:05Z

Okay, I've refactored things into something that's hopefully sensible and there's a few questions I have left

I'm assuming the implementation of linux is the same as macosx (from what I've read) - do you want sharedmem-macosx.cc (which is the old base behaviour currently) removed and rename sharedmem-linux.cc to sharedmem-nix.cc or sharedmem-other.cc or similar?

We could go with sharedmem-unix.cc, and have the guard for it be "not windows". We might want to split it off further at some point in the future, because Linux now has a "better" way to do this using a new API I've never used before, but that's not really important for now.

g_emulator->m_mem->m_wram.m_mem has clunky naming, so you want SharedMem::m_mem renamed to something else?

I'm fine with this for now. I'm thinking maybe we can try and add a casting operator on the SharedMem object and just "use" it as a pointer directly, but I'd need to experiment with this.

I've implemented m_wram with SharedMem and I'm torn between two ways to reference the memory:

//HEADER
  public:
    SharedMem m_wram;  // Kernel & User Memory (8 Meg)
//SOURCE
  //init
  m_wram.init(fmt::format("pcsx-redux-wram-{}", pid).c_str(), 0x00800000);
  //referenced as
  g_emulator->m_mem->m_wram.m_mem
  //no clean-up needed

VS

//HEADER
  public:
    SharedMem m_wramShared;  // Kernel & User Memory (8 Meg)
    uint8_t *m_wram = nullptr;
//SOURCE
  //init
  m_wramShared.init(fmt::format("pcsx-redux-wram-{}", pid).c_str(), 0x00800000);
  m_wram = m_wramShared.m_mem;
  //referenced as
  g_emulator->m_mem->m_wram
  //clean-up
  m_wram = nullptr;

First, I'm wondering if we shouldn't just have the -[pid] suffix be added by the SharedMem implementation, making it a mandatory suffix that is.

Then I think we should go with your second option for now. As per my comment prior this one, this may just be temporary if we manage to pull off a casting operation that'd work everywhere transparently, which would make this problem moot.

Is the naming format pcsx-redux-[memtype]-[pid] okay?

Yup!

Once everything is good, do you have a list of memory blocks to convert to SharedMem? I'm assuming m_wram, m_exp1, m_bios and m_hard. If you want m_writeLUT and m_readLUT then SharedMem needs to be templated, which should be fine

Let's make this PR just convert m_wram first, in order to work incrementally. Then we can see about migrating the rest, once we iron out the kinks about things like maybe casting. Templating it can come after.

Giragast · 2023-11-24T22:08:13Z

Should be ready for another pass now 🤞

nicolasnoble · 2023-11-25T01:00:40Z

src/core/psxmem.cc

@@ -89,7 +89,17 @@ int PCSX::Memory::init() {
    m_readLUT = (uint8_t **)calloc(0x10000, sizeof(void *));
    m_writeLUT = (uint8_t **)calloc(0x10000, sizeof(void *));

-    m_wram = (uint8_t *)calloc(0x00800000, 1);
+    uint32_t pid = 0;


Looks like this is no longer needed.

nicolasnoble · 2023-11-25T01:00:56Z

src/core/psxmem.h

@@ -188,6 +189,9 @@ class Memory {

    // hopefully this should become private eventually, with only certain classes having direct access.
  public:
+    // Shared memory wrappers, pointers below point to these where appropriate
+    SharedMem m_wramShared;


Probably keep it private for now.

nicolasnoble · 2023-11-25T01:01:49Z

src/support/sharedmem.h

+    ~SharedMem();
+
+    void init(const char* id, size_t size);
+    std::string getSharedName(const char* id, uint32_t pid);


This is probably private.

nicolasnoble · 2023-11-25T01:03:50Z

src/support/sharedmem-unix.cc

+                g_system->message("ftruncate failed, falling back to memory alloc, size: %zu\n", m_size);
+            } else {
+                // ftruncate completed, now map the memory at 0 offset
+                void* basePointer = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, m_fd, 0);


nicolasnoble · 2023-11-25T01:05:22Z

src/support/sharedmem-unix.cc

+#if !defined(_WIN32) && !defined(_WIN64)
+
+#include "support/sharedmem.h"
+#include "core/system.h"


The problem in bringing core/system.h here is it'd create a cyclic dependency between library projects. We should just not warn if we need to fallback; there's quite nothing the user might be able to do. At best we could imagine having a boolean saying the shared memory worked, and report it up in the UI if we really wanted to.

nicolasnoble · 2023-11-25T01:08:06Z

src/support/sharedmem-windows.cc

+
+#include "support/sharedmem.h"
+#include "support/windowswrapper.h"
+#include "core/system.h"


Same comment as with -unix.cc, we can't use core/system.h safely from support/*.

nicolasnoble · 2023-11-25T01:11:21Z

src/core/psxmem.cc

+#endif
+
+    // Init all memory as named mappings
+    m_wramShared.init(_("wram"), 0x00800000);


No, let's avoid making it "wram" a localized string. This would break external scripts depending on the selected locale.

nicolasnoble · 2023-11-25T01:13:26Z

Just one last kind of medium issue: calloc will inherently ensure the allocated memory is initialized to zero (memset that is), which I don't think the shared memory code currently does.

nicolasnoble · 2023-11-25T01:14:15Z

The rest is just minor nits, looks good otherwise :)

Add Windows-only support for a shared memory mapping on Kernel & User…

e0ee5f4

… Memory

Ken Murdock added 3 commits November 24, 2023 04:54

Refactored named shared memory handling into SharedMem class, with pl…

d79f5f3

…atform-specific implementations for Windows and Linux (not OSX), removed toggle to enable named shared memory

Add missing headers, fix line endings

bda29b5

Remove unused code

520bb2e

Refactoring SharedMem

27f90ec

nicolasnoble changed the title ~~Add Windows-only support for a shared memory mapping on Kernel & User Memory~~ Add support for a shared memory mapping on Kernel & User Memory Nov 25, 2023

nicolasnoble reviewed Nov 25, 2023

View reviewed changes

Ken Murdock added 2 commits November 25, 2023 01:47

Core review tweaks

d9cdbfb

Add missing headers

728731a

nicolasnoble approved these changes Nov 25, 2023

View reviewed changes

nicolasnoble merged commit 0861d98 into grumpycoders:main Nov 25, 2023
16 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for a shared memory mapping on Kernel & User Memory #1468

Add support for a shared memory mapping on Kernel & User Memory #1468

Giragast commented Nov 23, 2023

ghost commented Nov 23, 2023 •

edited by ghost

Loading

Legend

codecov bot commented Nov 23, 2023 •

edited

Loading

nicolasnoble commented Nov 23, 2023

Giragast commented Nov 24, 2023

nicolasnoble commented Nov 24, 2023 •

edited

Loading

Giragast commented Nov 24, 2023

nicolasnoble Nov 25, 2023

nicolasnoble Nov 25, 2023

nicolasnoble Nov 25, 2023

nicolasnoble Nov 25, 2023

nicolasnoble Nov 25, 2023

nicolasnoble Nov 25, 2023

nicolasnoble Nov 25, 2023

nicolasnoble commented Nov 25, 2023

nicolasnoble commented Nov 25, 2023

Add support for a shared memory mapping on Kernel & User Memory #1468

Add support for a shared memory mapping on Kernel & User Memory #1468

Conversation

Giragast commented Nov 23, 2023

ghost commented Nov 23, 2023 • edited by ghost Loading

Legend

codecov bot commented Nov 23, 2023 • edited Loading

Codecov Report

nicolasnoble commented Nov 23, 2023

Giragast commented Nov 24, 2023

nicolasnoble commented Nov 24, 2023 • edited Loading

Giragast commented Nov 24, 2023

nicolasnoble Nov 25, 2023

Choose a reason for hiding this comment

nicolasnoble Nov 25, 2023

Choose a reason for hiding this comment

nicolasnoble Nov 25, 2023

Choose a reason for hiding this comment

nicolasnoble Nov 25, 2023

Choose a reason for hiding this comment

nicolasnoble Nov 25, 2023

Choose a reason for hiding this comment

nicolasnoble Nov 25, 2023

Choose a reason for hiding this comment

nicolasnoble Nov 25, 2023

Choose a reason for hiding this comment

nicolasnoble commented Nov 25, 2023

nicolasnoble commented Nov 25, 2023

ghost commented Nov 23, 2023 •

edited by ghost

Loading

codecov bot commented Nov 23, 2023 •

edited

Loading

nicolasnoble commented Nov 24, 2023 •

edited

Loading