From 61621b8313b3bed1cd79069233349e286d2c800a Mon Sep 17 00:00:00 2001 From: "Shane Michael Mathews (Personal Account)" Date: Mon, 6 Jul 2026 11:35:44 -0400 Subject: [PATCH] fix(runtime): make GS CSR atomic to fix vsync-worker/guest data race (#145) 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 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 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. --- ps2xRuntime/include/runtime/ps2_memory.h | 10 +- .../src/lib/Kernel/Syscalls/Interrupt.cpp | 11 +- ps2xRuntime/src/lib/ps2_debug_panel.cpp | 4 +- ps2xRuntime/src/lib/ps2_gs_gpu.cpp | 4 +- ps2xRuntime/src/lib/ps2_memory.cpp | 162 +++++++++++------- ps2xTest/src/ps2_runtime_interrupt_tests.cpp | 121 +++++++++++++ 6 files changed, 247 insertions(+), 65 deletions(-) diff --git a/ps2xRuntime/include/runtime/ps2_memory.h b/ps2xRuntime/include/runtime/ps2_memory.h index 79495e8..e5fdfb3 100644 --- a/ps2xRuntime/include/runtime/ps2_memory.h +++ b/ps2xRuntime/include/runtime/ps2_memory.h @@ -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 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::is_always_lock_free, "GS CSR atomic must be lock-free on all supported targets"); // PS2 VIF (VPU Interface) registers struct VIFRegisters diff --git a/ps2xRuntime/src/lib/Kernel/Syscalls/Interrupt.cpp b/ps2xRuntime/src/lib/Kernel/Syscalls/Interrupt.cpp index 761befd..220eadf 100644 --- a/ps2xRuntime/src/lib/Kernel/Syscalls/Interrupt.cpp +++ b/ps2xRuntime/src/lib/Kernel/Syscalls/Interrupt.cpp @@ -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 &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) diff --git a/ps2xRuntime/src/lib/ps2_debug_panel.cpp b/ps2xRuntime/src/lib/ps2_debug_panel.cpp index 11f5262..2876e12 100644 --- a/ps2xRuntime/src/lib/ps2_debug_panel.cpp +++ b/ps2xRuntime/src/lib/ps2_debug_panel.cpp @@ -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); diff --git a/ps2xRuntime/src/lib/ps2_gs_gpu.cpp b/ps2xRuntime/src/lib/ps2_gs_gpu.cpp index 257f407..6c76c98 100644 --- a/ps2xRuntime/src/lib/ps2_gs_gpu.cpp +++ b/ps2xRuntime/src/lib/ps2_gs_gpu.cpp @@ -1878,14 +1878,14 @@ void GS::writeRegister(uint8_t regAddr, uint64_t value) uint32_t lo = static_cast(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: diff --git a/ps2xRuntime/src/lib/ps2_memory.cpp b/ps2xRuntime/src/lib/ps2_memory.cpp index 8cbfd00..a40505b 100644 --- a/ps2xRuntime/src/lib/ps2_memory.cpp +++ b/ps2xRuntime/src/lib/ps2_memory.cpp @@ -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 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 &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(expected & 0xFFFFFFFFull); + uint32_t mergedLow = (oldLow & kW1cMask) | (value & ~kW1cMask); + desired = (expected & 0xFFFFFFFF00000000ull) | static_cast(mergedLow); + desired &= ~static_cast(value & kW1cMask); + } + else + { + uint64_t mask = 0xFFFFFFFFull << (off * 8u); + desired = (expected & ~mask) | (static_cast(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 &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::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(current & 0xFFFFFFFFull); - uint32_t mergedLow = (oldLow & kW1cMask) | (value & ~kW1cMask); - current = (current & 0xFFFFFFFF00000000ull) | static_cast(mergedLow); - current &= ~static_cast(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(current & 0xFFFFFFFFull); - uint32_t mergedLow = (oldLow & kW1cMask) | (value & ~kW1cMask); - current = (current & 0xFFFFFFFF00000000ull) | static_cast(mergedLow); - current &= ~static_cast(value & kW1cMask); - *reg = current; - } - else - { - const uint64_t mask = 0xFFFFFFFFull << (off * 8u); - *reg = (*reg & ~mask) | (static_cast(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(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((gs_regs.csr.load() >> (off * 8u)) & 0xFFFFFFFFull); + } if (uint64_t *reg = gsRegPtr(gs_regs, address)) { - const uint32_t off = address & 7u; return static_cast((*reg >> (off * 8u)) & 0xFFFFFFFFull); } return 0u; diff --git a/ps2xTest/src/ps2_runtime_interrupt_tests.cpp b/ps2xTest/src/ps2_runtime_interrupt_tests.cpp index 1138499..7254973 100644 --- a/ps2xTest/src/ps2_runtime_interrupt_tests.cpp +++ b/ps2xTest/src/ps2_runtime_interrupt_tests.cpp @@ -2,6 +2,7 @@ #include "ps2_runtime.h" #include "ps2_syscalls.h" #include "Stubs/DMA.h" +#include "runtime/ps2_gs_gpu.h" #include #include @@ -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(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 setAnomaliesA{0u}, clearAnomaliesA{0u}; + std::atomic setAnomaliesB{0u}, clearAnomaliesB{0u}; + std::atomic 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 &setAnomalies, std::atomic &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();