mirror/PS2Recomp
1
0
Fork 0
mirror of https://github.com/ran-j/PS2Recomp.git synced 2026-09-26 08:51:05 -04:00
Code Issues Packages Projects Releases Wiki Activity

fix(runtime): make GS CSR atomic to fix vsync-worker/guest data race (#145)

Browse Source 
updateGsCsrFieldForVSync runs on the detached vsync worker and updated the
FIELD bit with a non-atomic read-modify-write of GSRegisters::csr while
guest threads concurrently read/write the same word (MMIO read32/read64 and
the W1C handling in write32/write64) and the GIF path sets SIGNAL/FINISH.
Even though the writers touch disjoint bits, a whole-word RMW loses the
other side's update: a clobbered SIGNAL/FINISH set hangs a game
synchronizing on GS completion, a clobbered W1C clear re-asserts a handled
interrupt, and a clobbered FIELD toggle stalls interlace field polling.
ThreadSanitizer flags the race on main (updateGsCsrFieldForVSync vs
PS2Memory::write64 and GS::writeRegister).

Make the field std::atomic<uint64_t> and perform every update as a single
atomic RMW:

- vsync FIELD toggle -> fetch_or / fetch_and
- MMIO W1C writes -> compare_exchange loop in shared helpers (a
  load-then-store pair would still race); a 32-bit store to the CSR's upper
  dword previously bypassed the W1C special case entirely and went through
  the plain merge branch - both halves now share the same atomic helper
  with unchanged guest-visible semantics
- GIF SIGNAL/FINISH -> fetch_or
- reads -> load()

std::atomic<uint64_t> is lock-free on all supported targets (static_assert
added), the struct's size/alignment asserts are unchanged, GSRegisters is
never copied by value, and default (seq_cst) ordering is used throughout -
these operations are rare (vblank ticks, GIF signals, CSR MMIO), so
reviewability wins over micro-optimization.

New regression test: two racer threads each own one status bit (SIGNAL /
FINISH) and loop 80k GIF-set + W1C-clear cycles verifying their own bit
after each half-op while the vsync worker toggles FIELD. Fails 20/20 runs
against the previous code, passes 50/50 with the fix, ~350ms runtime, no
sanitizer needed.
This commit is contained in:
Shane Michael Mathews (Personal Account)
2026-07-06 11:35:44 -04:00
committed by GitHub
parent 5196a6672a
commit 61621b8313
6 changed files with 247 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ps2xRuntime/include/runtime/ps2_memory.h
+9 -1
View File
@@ -202,13 +202,21 @@ struct GSRegisters
uint64_t extdata; // External data
uint64_t extwrite; // External write
uint64_t bgcolor; // Background color
uint64_t csr; // Status
// Status. Concurrency contract: the vsync worker thread toggles the FIELD bit
// (bit 13) once per tick; guest threads issue write-one-to-clear writes against
// the SIGNAL/FINISH status bits (0..1) via the MMIO path; the GIF sets SIGNAL
// and FINISH from yet another thread. All three interleave, so this register
// must be updated with atomic RMWs only (no load-then-store pairs anywhere).
std::atomic<uint64_t> csr;
uint64_t imr; // Interrupt mask
uint64_t busdir; // Bus direction
uint64_t siglblid; // Signal label ID
};
static_assert(sizeof(GSRegisters) == (19u * sizeof(uint64_t)), "GSRegisters layout changed unexpectedly");
static_assert(alignof(GSRegisters) == alignof(uint64_t), "GSRegisters alignment must remain 64-bit");
// CSR is written by the vsync worker while guest threads concurrently read/write it
// (MMIO) and the GIF sets SIGNAL/FINISH; a lock-free atomic keeps that path wait-free.
static_assert(std::atomic<uint64_t>::is_always_lock_free, "GS CSR atomic must be lock-free on all supported targets");
// PS2 VIF (VPU Interface) registers
struct VIFRegisters
ps2xRuntime/src/lib/Kernel/Syscalls/Interrupt.cpp
+9 -2
View File
@@ -286,8 +286,15 @@ namespace ps2_syscalls
}
constexpr uint64_t kGsCsrFieldMask = 0x2000ull;
uint64_t &csr = runtime->memory().gs().csr;
csr = (csr & ~kGsCsrFieldMask) | ((tickValue & 1ull) ? kGsCsrFieldMask : 0ull);
std::atomic<uint64_t> &csr = runtime->memory().gs().csr;
if (tickValue & 1ull)
{
csr.fetch_or(kGsCsrFieldMask);
}
else
{
csr.fetch_and(~kGsCsrFieldMask);
}
}
static uint64_t signalVSyncFlag(uint8_t *rdram, PS2Runtime *runtime)
ps2xRuntime/src/lib/ps2_debug_panel.cpp
+2 -2
View File
@@ -468,7 +468,7 @@ namespace
out << "DISPFB2 0x" << std::setw(16) << regs.dispfb2 << "\n";
out << "DISPLAY2 0x" << std::setw(16) << regs.display2 << "\n";
out << "BGCOLOR 0x" << std::setw(16) << regs.bgcolor << "\n";
out << "CSR 0x" << std::setw(16) << regs.csr << "\n";
out << "CSR 0x" << std::setw(16) << regs.csr.load() << "\n";
out << "IMR 0x" << std::setw(16) << regs.imr << "\n";
out << "BUSDIR 0x" << std::setw(16) << regs.busdir << "\n";
out << "SIGLBLID 0x" << std::setw(16) << regs.siglblid << "\n\n";
@@ -1485,7 +1485,7 @@ namespace
row64("DISPFB2", regs.dispfb2);
row64("DISPLAY2", regs.display2);
row64("BGCOLOR", regs.bgcolor);
row64("CSR", regs.csr);
row64("CSR", regs.csr.load());
row64("IMR", regs.imr);
row64("BUSDIR", regs.busdir);
row64("SIGLBLID", regs.siglblid);
ps2xRuntime/src/lib/ps2_gs_gpu.cpp
+2 -2
View File
@@ -1878,14 +1878,14 @@ void GS::writeRegister(uint8_t regAddr, uint64_t value)
uint32_t lo = static_cast<uint32_t>(m_privRegs->siglblid & 0xFFFFFFFF);
lo = (lo & ~mask) | (id & mask);
m_privRegs->siglblid = (m_privRegs->siglblid & 0xFFFFFFFF00000000ULL) | lo;
m_privRegs->csr |= 0x1;
m_privRegs->csr.fetch_or(0x1);
}
break;
}
case GS_REG_FINISH:
{
if (m_privRegs)
m_privRegs->csr |= 0x2;
m_privRegs->csr.fetch_or(0x2);
break;
}
case GS_REG_LABEL:
ps2xRuntime/src/lib/ps2_memory.cpp
+104 -58
View File
@@ -75,8 +75,10 @@ namespace
return &gs.extwrite;
case 0x00E0:
return &gs.bgcolor;
case 0x1000:
return &gs.csr;
// CSR (offset 0x1000) is intentionally not handled here: it is
// std::atomic<uint64_t> and no longer converts to uint64_t*. Callers must
// check for offset 0x1000 themselves and go through writeCsrHalf/
// writeCsrFull/gs.csr.load() instead of gsRegPtr().
case 0x1010:
return &gs.imr;
case 0x1040:
@@ -88,6 +90,50 @@ namespace
}
}
constexpr uint32_t kGsCsrRegOffset = 0x1000u;
// Atomically apply a 32-bit write to one half (off=0 low dword, off=4 high
// dword) of the GS CSR register. Bits 0..1 of the low dword (SIGNAL/FINISH) are
// write-one-to-clear; everything else is a plain merge. Uses compare_exchange
// so the whole read-modify-write is a single atomic step -- this register is
// also touched by the vsync worker (FIELD bit) and the GIF (SIGNAL/FINISH) on
// other threads, so a load-then-store here would race with them.
inline void writeCsrHalf(std::atomic<uint64_t> &csr, uint32_t off, uint32_t value)
{
constexpr uint32_t kW1cMask = 0x3u;
uint64_t expected = csr.load();
uint64_t desired;
do
{
if (off == 0u)
{
uint32_t oldLow = static_cast<uint32_t>(expected & 0xFFFFFFFFull);
uint32_t mergedLow = (oldLow & kW1cMask) | (value & ~kW1cMask);
desired = (expected & 0xFFFFFFFF00000000ull) | static_cast<uint64_t>(mergedLow);
desired &= ~static_cast<uint64_t>(value & kW1cMask);
}
else
{
uint64_t mask = 0xFFFFFFFFull << (off * 8u);
desired = (expected & ~mask) | (static_cast<uint64_t>(value) << (off * 8u));
}
} while (!csr.compare_exchange_weak(expected, desired));
}
// Same as writeCsrHalf but for a full 64-bit CSR write (bits 0..1 are still
// write-one-to-clear against the current value).
inline void writeCsrFull(std::atomic<uint64_t> &csr, uint64_t value)
{
constexpr uint64_t kW1cMask = 0x3ull;
uint64_t expected = csr.load();
uint64_t desired;
do
{
desired = (expected & kW1cMask) | (value & ~kW1cMask);
desired &= ~(value & kW1cMask);
} while (!csr.compare_exchange_weak(expected, desired));
}
constexpr uint32_t kEeTimer0Count = 0x10000000u;
constexpr uint32_t kEeTimer0Mode = 0x10000010u;
constexpr uint32_t kEeTimer0Compare = 0x10000020u;
@@ -241,6 +287,9 @@ bool PS2Memory::initialize(size_t ramSize)
// Initialize GS registers
memset(&gs_regs, 0, sizeof(gs_regs));
// memset zero-fills std::atomic<uint64_t>::csr's bytes, which is not itself
// a guaranteed-valid atomic store; make the zero-initialization explicit.
gs_regs.csr.store(0);
gs_regs.dispfb1 = (0ULL << 0) | (10ULL << 9) | (0ULL << 15) | (0ULL << 32) | (0ULL << 43);
gs_regs.display1 = (0ULL << 0) | (0ULL << 12) | (0ULL << 23) | (0ULL << 27) | (639ULL << 32) | (447ULL << 44);
gs_regs.dispfb2 = gs_regs.dispfb1;
@@ -532,10 +581,16 @@ uint32_t PS2Memory::read32(uint32_t address)
if (isGsPrivReg(address))
{
uint32_t off = address & 7;
const uint32_t regOff = (address - PS2_GS_PRIV_REG_BASE) & ~0x7u;
if (regOff == kGsCsrRegOffset)
{
uint64_t val = gs_regs.csr.load();
return (uint32_t)(val >> (off * 8));
}
uint64_t *reg = gsRegPtr(gs_regs, address);
if (!reg)
return 0;
uint32_t off = address & 7;
uint64_t val = *reg;
return (uint32_t)(val >> (off * 8));
}
@@ -574,6 +629,11 @@ uint64_t PS2Memory::read64(uint32_t address)
if (isGsPrivReg(address))
{
const uint32_t regOff = (address - PS2_GS_PRIV_REG_BASE) & ~0x7u;
if (regOff == kGsCsrRegOffset)
{
return gs_regs.csr.load();
}
uint64_t *reg = gsRegPtr(gs_regs, address);
return reg ? *reg : 0;
}
@@ -722,28 +782,19 @@ void PS2Memory::write32(uint32_t address, uint32_t value)
if (isGsPrivReg(address))
{
uint64_t *reg = gsRegPtr(gs_regs, address);
if (reg)
uint32_t off = address & 7;
const uint32_t regOff = (address - PS2_GS_PRIV_REG_BASE) & ~0x7u;
if (regOff == kGsCsrRegOffset)
{
uint32_t off = address & 7;
const uint32_t regOff = (address - PS2_GS_PRIV_REG_BASE) & ~0x7u;
if (regOff == 0x1000u && off == 0u)
{
// CSR low dword: bits 0..1 are write-one-to-clear status bits.
constexpr uint32_t kW1cMask = 0x3u;
uint64_t current = *reg;
uint32_t oldLow = static_cast<uint32_t>(current & 0xFFFFFFFFull);
uint32_t mergedLow = (oldLow & kW1cMask) | (value & ~kW1cMask);
current = (current & 0xFFFFFFFF00000000ull) | static_cast<uint64_t>(mergedLow);
current &= ~static_cast<uint64_t>(value & kW1cMask);
*reg = current;
}
else
{
uint64_t mask = 0xFFFFFFFFULL << (off * 8);
uint64_t newVal = (*reg & ~mask) | ((uint64_t)value << (off * 8));
*reg = newVal;
}
// CSR: bits 0..1 of the low dword are write-one-to-clear status bits.
// Done as a single atomic RMW -- see writeCsrHalf's comment.
writeCsrHalf(gs_regs.csr, off, value);
}
else if (uint64_t *reg = gsRegPtr(gs_regs, address))
{
uint64_t mask = 0xFFFFFFFFULL << (off * 8);
uint64_t newVal = (*reg & ~mask) | ((uint64_t)value << (off * 8));
*reg = newVal;
}
return;
}
@@ -787,22 +838,16 @@ void PS2Memory::write64(uint32_t address, uint64_t value)
if (isGsPrivReg(address))
{
uint64_t *reg = gsRegPtr(gs_regs, address);
if (reg)
const uint32_t regOff = (address - PS2_GS_PRIV_REG_BASE) & ~0x7u;
if (regOff == kGsCsrRegOffset)
{
const uint32_t regOff = (address - PS2_GS_PRIV_REG_BASE) & ~0x7u;
if (regOff == 0x1000u)
{
// CSR: bits 0..1 are write-one-to-clear status bits.
constexpr uint64_t kW1cMask = 0x3ull;
uint64_t next = (*reg & kW1cMask) | (value & ~kW1cMask);
next &= ~(value & kW1cMask);
*reg = next;
}
else
{
*reg = value;
}
// CSR: bits 0..1 are write-one-to-clear status bits. Done as a single
// atomic RMW -- see writeCsrFull's comment.
writeCsrFull(gs_regs.csr, value);
}
else if (uint64_t *reg = gsRegPtr(gs_regs, address))
{
*reg = value;
}
return;
}
@@ -903,26 +948,20 @@ bool PS2Memory::writeIORegister(uint32_t address, uint32_t value)
if (isGsPrivReg(address))
{
// NB: unreachable from write8/16/32/64 today since those all funnel IO
// register writes through addresses in PS2_IO_BASE's range, which is
// disjoint from PS2_GS_PRIV_REG_BASE; kept correct for direct callers.
m_ioRegisters[address] = value;
if (uint64_t *reg = gsRegPtr(gs_regs, address))
const uint32_t off = address & 7u;
const uint32_t regOff = (address - PS2_GS_PRIV_REG_BASE) & ~0x7u;
if (regOff == kGsCsrRegOffset)
{
const uint32_t off = address & 7u;
const uint32_t regOff = (address - PS2_GS_PRIV_REG_BASE) & ~0x7u;
if (regOff == 0x1000u && off == 0u)
{
constexpr uint32_t kW1cMask = 0x3u;
uint64_t current = *reg;
uint32_t oldLow = static_cast<uint32_t>(current & 0xFFFFFFFFull);
uint32_t mergedLow = (oldLow & kW1cMask) | (value & ~kW1cMask);
current = (current & 0xFFFFFFFF00000000ull) | static_cast<uint64_t>(mergedLow);
current &= ~static_cast<uint64_t>(value & kW1cMask);
*reg = current;
}
else
{
const uint64_t mask = 0xFFFFFFFFull << (off * 8u);
*reg = (*reg & ~mask) | (static_cast<uint64_t>(value) << (off * 8u));
}
writeCsrHalf(gs_regs.csr, off, value);
}
else if (uint64_t *reg = gsRegPtr(gs_regs, address))
{
const uint64_t mask = 0xFFFFFFFFull << (off * 8u);
*reg = (*reg & ~mask) | (static_cast<uint64_t>(value) << (off * 8u));
}
m_gsWriteCount.fetch_add(1, std::memory_order_relaxed);
return true;
@@ -1663,9 +1702,16 @@ uint32_t PS2Memory::readIORegister(uint32_t address)
{
if (isGsPrivReg(address))
{
// NB: unreachable from read8/16/32/64 today, same reasoning as the write
// path above; kept correct for direct callers.
const uint32_t off = address & 7u;
const uint32_t regOff = (address - PS2_GS_PRIV_REG_BASE) & ~0x7u;
if (regOff == kGsCsrRegOffset)
{
return static_cast<uint32_t>((gs_regs.csr.load() >> (off * 8u)) & 0xFFFFFFFFull);
}
if (uint64_t *reg = gsRegPtr(gs_regs, address))
{
const uint32_t off = address & 7u;
return static_cast<uint32_t>((*reg >> (off * 8u)) & 0xFFFFFFFFull);
}
return 0u;
ps2xTest/src/ps2_runtime_interrupt_tests.cpp
+121
View File
@@ -2,6 +2,7 @@
#include "ps2_runtime.h"
#include "ps2_syscalls.h"
#include "Stubs/DMA.h"
#include "runtime/ps2_gs_gpu.h"
#include <atomic>
#include <chrono>
@@ -253,6 +254,126 @@ void register_ps2_runtime_interrupt_tests()
cleanupRuntime(env);
});
// Regression test for the GS CSR data race: a two-writer word-level
// lost-update guard. Pre-fix, every CSR update was a plain (non-atomic)
// 64-bit load-modify-store of the WHOLE word, so two threads that own
// logically disjoint bits could still clobber each other: thread A's
// read-modify-write of the word can overwrite thread B's bit with the
// stale value A loaded before B's update landed.
//
// Two racer threads with disjoint bit ownership run concurrently:
// - racer A owns SIGNAL (bit 0): sets it via the GIF register path
// (GS_REG_SIGNAL) then W1C-clears ONLY bit 0 via the MMIO write path;
// - racer B owns FINISH (bit 1): same protocol with GS_REG_FINISH and
// a W1C write of only bit 1.
// Each racer checks only its own bit after each half-op. With the fix
// (std::atomic CSR, every update a single atomic RMW) each racer is the
// sole writer of its bit, so its bit deterministically reflects its own
// last operation: zero anomalies are possible. Pre-fix, the racers'
// whole-word W1C RMWs constantly interleave and lose each other's
// set/clear, lighting up the anomaly counters.
//
// Why racer-vs-racer instead of racer-vs-vsync: the vsync worker (which
// motivated the fix) writes CSR only once per ~16.7ms tick, a window far
// too narrow to hit deterministically in a bounded test. The corrupting
// mechanism -- a non-atomic whole-word RMW clobbering a concurrently
// written disjoint bit -- is identical, so guarding it with two
// high-frequency writers also guards the vsync FIELD interleaving. The
// real vsync worker still runs throughout (started via the same
// SetVSyncFlag syscall production uses) and its FIELD (bit 13) toggling
// is asserted when at least two ticks were observed.
tc.Run("Disjoint-bit GS CSR writers (SIGNAL vs FINISH vs vsync FIELD) never lose word-level updates", [](TestCase &t)
{
notifyRuntimeStop();
TestEnv env;
t.IsTrue(env.runtime.memory().initialize(), "runtime memory initialize should succeed");
constexpr uint32_t kFlagAddr = 0x1180u;
constexpr uint32_t kTickAddr = 0x1190u;
constexpr uint64_t kGsCsrFieldMask = 0x2000ull;
constexpr uint32_t kCsrAddr = PS2_GS_PRIV_REG_BASE + 0x1000u;
constexpr uint32_t kIterations = 80000u;
GS gs;
gs.init(env.runtime.memory().getGSVRAM(), static_cast<uint32_t>(PS2_GS_VRAM_SIZE),
&env.runtime.memory().gs());
// Drive the real vsync worker via the same syscall path production
// code uses; it runs on its own thread and toggles CSR.FIELD once
// per tick via updateGsCsrFieldForVSync.
R5900Context ctx{};
setRegU32(ctx, 4, kFlagAddr);
setRegU32(ctx, 5, kTickAddr);
t.IsTrue(callSyscall(0x73u, env.rdram.data(), &ctx, &env.runtime), "SetVSyncFlag syscall should dispatch");
const uint64_t tickBefore = GetCurrentVSyncTick();
std::atomic<uint32_t> setAnomaliesA{0u}, clearAnomaliesA{0u};
std::atomic<uint32_t> setAnomaliesB{0u}, clearAnomaliesB{0u};
std::atomic<uint32_t> racersDone{0u};
// ownBit: the single CSR status bit this racer exclusively owns.
// Each iteration: raise the bit via the GIF register-write path,
// verify it reads back set, W1C-clear only that bit via the guest
// MMIO path, verify it reads back clear. The other racer and the
// vsync worker never touch this bit, so under atomic RMWs both
// checks are exact -- any anomaly is a lost word-level update.
auto racerBody = [&](uint8_t gifReg, uint64_t gifValue, uint64_t ownBit,
std::atomic<uint32_t> &setAnomalies, std::atomic<uint32_t> &clearAnomalies) {
for (uint32_t i = 0; i < kIterations; ++i)
{
gs.writeRegister(gifReg, gifValue);
if ((env.runtime.memory().gs().csr.load() & ownBit) == 0ull)
{
setAnomalies.fetch_add(1u, std::memory_order_relaxed);
}
env.runtime.memory().write64(kCsrAddr, ownBit);
if ((env.runtime.memory().gs().csr.load() & ownBit) != 0ull)
{
clearAnomalies.fetch_add(1u, std::memory_order_relaxed);
}
}
racersDone.fetch_add(1u, std::memory_order_relaxed);
};
const uint64_t signalValue = (0xFFFFFFFFull << 32) | 0x11223344ull;
std::thread racerA(racerBody, GS_REG_SIGNAL, signalValue, 0x1ull,
std::ref(setAnomaliesA), std::ref(clearAnomaliesA));
std::thread racerB(racerBody, GS_REG_FINISH, 0ull, 0x2ull,
std::ref(setAnomaliesB), std::ref(clearAnomaliesB));
// While the racers hammer bits 0..1, watch for CSR.FIELD (bit 13)
// flips from the vsync worker. Polling ends when both racers finish,
// so this adds no fixed wall-clock cost.
const uint64_t initialField = env.runtime.memory().gs().csr.load() & kGsCsrFieldMask;
bool fieldFlipped = false;
while (racersDone.load(std::memory_order_relaxed) < 2u)
{
if ((env.runtime.memory().gs().csr.load() & kGsCsrFieldMask) != initialField)
{
fieldFlipped = true;
}
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
racerA.join();
racerB.join();
const uint64_t ticksElapsed = GetCurrentVSyncTick() - tickBefore;
t.Equals(setAnomaliesA.load(), 0u, "racer A: SIGNAL set must never be lost to a concurrent whole-word CSR RMW");
t.Equals(clearAnomaliesA.load(), 0u, "racer A: SIGNAL W1C-clear must never be lost to a concurrent whole-word CSR RMW");
t.Equals(setAnomaliesB.load(), 0u, "racer B: FINISH set must never be lost to a concurrent whole-word CSR RMW");
t.Equals(clearAnomaliesB.load(), 0u, "racer B: FINISH W1C-clear must never be lost to a concurrent whole-word CSR RMW");
t.Equals(env.runtime.memory().gs().csr.load() & 0x3ull, 0x0ull,
"final CSR status bits must match both racers' ledgers (last op on each bit was a clear)");
if (ticksElapsed >= 2u)
{
t.IsTrue(fieldFlipped, "VSync worker should toggle GS CSR FIELD while the racers run");
}
cleanupRuntime(env);
});
tc.Run("INTC VBLANK handlers respect EnableIntc and DisableIntc masks", [](TestCase &t)
{
notifyRuntimeStop();