search

PCSX2 / pcsx2

PCSX2 - The Playstation 2 Emulator
https://pcsx2.net
Other
10.56k stars 1.53k forks source link

VMManager: Clear protected pages when resetting VM #11219

Closed DaZombieKiller closed 1 month ago

DaZombieKiller commented 1 month ago

Description of Changes

This PR makes two changes:

Rationale behind Changes

mmap_ResetBlockTracking is supposed to clear everything, so it probably makes more sense for it to use TotalRam rather than ExposedRam. It is now called from VMManager::Reset to fix a crash that occurs when the VM is reset after disabling expanded memory if the program has executed code in high RAM. PCSX2 will AV here when trying to clear eeMem, because it has leftover read-only page protection flags from the previous session.

Suggested Testing Steps

  1. Download mem-reset-elfs.zip
  2. With host filesystem and 128MB RAM enabled, run main.elf
  3. Turn off 128MB RAM and then reset the VM
stenzek commented 1 month ago

Maybe it'd be worth flipping the order of operations, clearing the protection state, then updating the memory mode. That way if the memory mode transitions from high->low, it'll get cleared, and low->high should already be clear.

Saves the overhead of the mprotect() calls for a much larger page range on reset.

DaZombieKiller commented 1 month ago

I considered that originally as an alternative implementation for the same reasons. Since I was unsure if there were any edge cases that could still leave the ExtraMemory buffer with read-only pages, I went with the broader implementation. If that's not going to be a problem I can leave mmap_ResetBlockTracking as it was previously and just move the call up (I think mmap_ResetBlockTracking followed by memSetExtraMemMode makes a bit more logical sense than the opposite anyway).

stenzek commented 1 month ago

I don't think there should be, the high memory should be untouched, since it can't be TLB mapped, which after translation determines when pages get locked.

stenzek commented 1 month ago

Hmm, this is going to happen twice now, CPU reset also does it. But that's a bit of a chicken-and-egg problem, since CPU reset depends on the memory state...

So, I'd suggest removing the call here https://github.com/PCSX2/pcsx2/blob/master/pcsx2/x86/ix86-32/iR5900.cpp#L602, and adding it here https://github.com/PCSX2/pcsx2/blob/master/pcsx2/VMManager.cpp#L1379 as well.

DaZombieKiller commented 1 month ago

Makes sense. I'll get that change implemented now.