From af4e074a021e878f0a84232d7cc379de00a941ef Mon Sep 17 00:00:00 2001 From: Dean Galvin Date: Wed, 14 Oct 2026 18:51:21 +1300 Subject: [PATCH] fix(agc): save pending storage results in every 64 KiB block that leaves the write watch On Windows the write watch is dropped in whole 64 KiB blocks (WriteWatchCoverage::Exclude), but the unwatch hook was given the page range of the import. An image with results pending in the same block as an imported page, outside that page, lost the watch without its guest bytes saved, and storing its results threw. The hook now gets the span of blocks Exclude drops (ExcludedSpan, the same block math), so every image with results pending in those blocks saves its bytes first. --- .../Execution/include/WriteWatchCoverage.hpp | 36 +++++++++----- .../Execution/src/GuestMemory.cpp | 16 +++++- .../tests/WriteWatchCoverage.cpp | 7 +++ .../tests/execution/StoreOverUntracked.cpp | 49 ++++++++++++------- 4 files changed, 77 insertions(+), 31 deletions(-) diff --git a/core/libs/prx/libSceAgcDriver/Execution/include/WriteWatchCoverage.hpp b/core/libs/prx/libSceAgcDriver/Execution/include/WriteWatchCoverage.hpp index a35d3fb193..124d638998 100644 --- a/core/libs/prx/libSceAgcDriver/Execution/include/WriteWatchCoverage.hpp +++ b/core/libs/prx/libSceAgcDriver/Execution/include/WriteWatchCoverage.hpp @@ -26,18 +26,18 @@ public: return std::none_of(excluded.begin() + first, excluded.begin() + last + 1, [](auto generation) { return generation != 0; }); } + bool ExcludedSpan(std::uint64_t address, std::size_t bytes, std::uint64_t* begin, std::uint64_t* end) const { + std::size_t first = 0; + std::size_t last = 0; + if (!excludedBlocks(address, bytes, first, last)) return false; + *begin = base + first * BlockBytes; + *end = std::min(base + (last + 1) * BlockBytes, base + size); + return true; + } bool Exclude(std::uint64_t address, std::size_t bytes, std::uint64_t generation = 1) { - if (bytes == 0 || size == 0) return false; - if (address < base) { - if (base - address >= bytes) return false; - bytes -= base - address; - address = base; - } - const auto offset = address - base; - if (offset >= size) return false; - bytes = static_cast(std::min(bytes, size - offset)); - const auto first = offset / BlockBytes; - const auto last = (offset + bytes - 1) / BlockBytes; + std::size_t first = 0; + std::size_t last = 0; + if (!excludedBlocks(address, bytes, first, last)) return false; bool changed = false; for (auto block = first; block <= last; ++block) { changed = changed || excluded[block] == 0; @@ -73,6 +73,20 @@ public: } private: + bool excludedBlocks(std::uint64_t address, std::size_t bytes, std::size_t& first, std::size_t& last) const { + if (bytes == 0 || size == 0) return false; + if (address < base) { + if (base - address >= bytes) return false; + bytes -= base - address; + address = base; + } + const auto offset = address - base; + if (offset >= size) return false; + bytes = static_cast(std::min(bytes, size - offset)); + first = offset / BlockBytes; + last = (offset + bytes - 1) / BlockBytes; + return true; + } static constexpr std::size_t BlockBytes = 65536; std::uint64_t base = 0; std::size_t size = 0; diff --git a/core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp b/core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp index 8850453b34..09f6502367 100644 --- a/core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp +++ b/core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp @@ -1267,7 +1267,21 @@ namespace { std::atomic unwatchHook{nullptr}; void beforeUnwatch(std::uint64_t address, std::size_t bytes) { - if (const auto hook = unwatchHook.load(std::memory_order_acquire)) hook(address, bytes); + const auto hook = unwatchHook.load(std::memory_order_acquire); + if (hook == nullptr) return; +#ifdef _WIN32 + std::uint64_t begin = 0; + std::uint64_t end = 0; + { + auto& tracker = Tracker(); + const auto lock = lockTracker(tracker); + tracker.initialize(); + if (!tracker.watched || !tracker.coverage.ExcludedSpan(address, bytes, &begin, &end)) return; + } + hook(begin, static_cast(end - begin)); +#else + hook(address, bytes); +#endif } void unwatchLocked(WriteTracker& tracker, std::uint64_t address, std::size_t bytes) { diff --git a/core/libs/prx/libSceAgcDriver/tests/WriteWatchCoverage.cpp b/core/libs/prx/libSceAgcDriver/tests/WriteWatchCoverage.cpp index 083a984de1..806c3334e7 100644 --- a/core/libs/prx/libSceAgcDriver/tests/WriteWatchCoverage.cpp +++ b/core/libs/prx/libSceAgcDriver/tests/WriteWatchCoverage.cpp @@ -18,6 +18,13 @@ int main() { require(coverage.Covers(base, 4 * block + 1), "the initialized arena is not watched"); require(!coverage.Covers(base - 1, 1) && !coverage.Covers(base + 4 * block + 1, 1), "outside memory is watched"); require(!coverage.Covers(base, 0) && !coverage.Covers(base, std::numeric_limits::max()), "an empty or overflowing range is watched"); + std::uint64_t spanBegin = 0; + std::uint64_t spanEnd = 0; + require(coverage.ExcludedSpan(base + block + 4096, 4096, &spanBegin, &spanEnd) && spanBegin == base + block && spanEnd == base + 2 * block, "the span of an imported page is not its whole block"); + require(coverage.ExcludedSpan(base + 3 * block - 1, 2, &spanBegin, &spanEnd) && spanBegin == base + 2 * block && spanEnd == base + 4 * block, "the span of a boundary import is not both blocks"); + require(coverage.ExcludedSpan(base + 4 * block, std::numeric_limits::max(), &spanBegin, &spanEnd) && spanBegin == base + 4 * block && spanEnd == base + 4 * block + 1, "the span of the last partial block is not clipped to the arena"); + require(coverage.ExcludedSpan(base - 1, 2, &spanBegin, &spanEnd) && spanBegin == base && spanEnd == base + block, "the span of an import crossing the arena start is not clipped"); + require(!coverage.ExcludedSpan(base - block, block, &spanBegin, &spanEnd) && !coverage.ExcludedSpan(base + 5 * block, block, &spanBegin, &spanEnd) && !coverage.ExcludedSpan(base, 0, &spanBegin, &spanEnd), "an outside or empty import has a span"); require(coverage.Exclude(base + block + 4096, 1), "an imported page was not excluded"); require(!coverage.Covers(base + block, block), "the imported block is still watched"); require(!coverage.Covers(base + block - 1, 2), "a range crossing into the import is watched"); diff --git a/core/libs/prx/libSceAgcDriver/tests/execution/StoreOverUntracked.cpp b/core/libs/prx/libSceAgcDriver/tests/execution/StoreOverUntracked.cpp index 3641153174..3431424ec1 100644 --- a/core/libs/prx/libSceAgcDriver/tests/execution/StoreOverUntracked.cpp +++ b/core/libs/prx/libSceAgcDriver/tests/execution/StoreOverUntracked.cpp @@ -37,6 +37,8 @@ using AgcDriver::Graphics::StorageTexture; constexpr std::uint32_t Side = 256; constexpr std::uint32_t Format32UInt = 20; constexpr std::uint32_t TileR64KBX = 0x1b; +constexpr std::uint32_t TileLinear = 0; +std::uint32_t surfaceTile = TileR64KBX; constexpr std::uint32_t Type2D = 9; constexpr std::size_t Page = 0x1000; constexpr std::size_t Block = 0x10000; @@ -68,7 +70,7 @@ std::array TextureDescriptor(const std::uint32_t* texels, std: static_cast(address >> 8u), static_cast((address >> 40u) & 0xffu) | (Format32UInt << 20u) | (((side - 1u) & 3u) << 30u), ((side - 1u) >> 2u) | ((side - 1u) << 14u), - 0xfacu | (TileR64KBX << 20u) | (Type2D << 28u), + 0xfacu | (surfaceTile << 20u) | (Type2D << 28u), 0u, 0u, 0u, @@ -120,40 +122,42 @@ void Fill(const std::uint32_t* texels) { class Surface { public: - explicit Surface(bool watched = false) : watched(watched) { + explicit Surface(bool watched = false, std::size_t lead = 0) : watched(watched), regionBytes(lead == 0 ? SurfaceBytes : SurfaceBytes + Block) { if (watched) { #ifdef _WIN32 - texels = static_cast(GuestArena::GuestArenaAllocate_nid_postfix(SurfaceBytes, Block)); - if (texels != nullptr) GuestArena::GuestArenaCommit_nid_postfix(texels, SurfaceBytes, PAGE_READWRITE, SurfaceBytes); + region = static_cast(GuestArena::GuestArenaAllocate_nid_postfix(regionBytes, Block)); + if (region != nullptr) GuestArena::GuestArenaCommit_nid_postfix(region, regionBytes, PAGE_READWRITE, regionBytes); #else - void* mapped = mmap(nullptr, SurfaceBytes + Block, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + void* mapped = mmap(nullptr, regionBytes + Block, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); if (mapped != MAP_FAILED) { const auto begin = reinterpret_cast(mapped); const auto aligned = (begin + Block - 1) & ~std::uintptr_t{Block - 1}; if (aligned != begin) munmap(mapped, aligned - begin); - if (aligned != begin + Block) munmap(reinterpret_cast(aligned + SurfaceBytes), begin + Block - aligned); - texels = reinterpret_cast(aligned); - GuestWriteWatch::GuestWriteWatchRegister_nid_postfix(texels, SurfaceBytes); + if (aligned != begin + Block) munmap(reinterpret_cast(aligned + regionBytes), begin + Block - aligned); + region = reinterpret_cast(aligned); + GuestWriteWatch::GuestWriteWatchRegister_nid_postfix(region, regionBytes); } #endif + if (region != nullptr) texels = reinterpret_cast(region + lead); } else { storage.resize((SurfaceBytes + Block) / 4u); texels = reinterpret_cast((reinterpret_cast(storage.data()) + Block - 1) & ~std::uintptr_t{Block - 1}); } Require(texels != nullptr, "store over untracked: cannot allocate the guest surface"); std::fill(texels, texels + SurfaceBytes / 4u, Untouched); - GuestAllocations::Mutation().Add(texels, SurfaceBytes, true, true, true); + allocation = region != nullptr ? static_cast(region) : static_cast(texels); + GuestAllocations::Mutation().Add(allocation, region != nullptr ? regionBytes : SurfaceBytes, true, true, true); } ~Surface() { - GuestAllocations::Mutation().Remove(texels); + GuestAllocations::Mutation().Remove(allocation); if (!watched) return; #ifdef _WIN32 - GuestArena::GuestArenaReset_nid_postfix(texels, SurfaceBytes); - GuestArena::GuestArenaRelease_nid_postfix(texels, SurfaceBytes); + GuestArena::GuestArenaReset_nid_postfix(region, regionBytes); + GuestArena::GuestArenaRelease_nid_postfix(region, regionBytes); #else - munmap(texels, SurfaceBytes); - GuestWriteWatch::GuestWriteWatchUnregister_nid_postfix(texels, SurfaceBytes); + munmap(region, regionBytes); + GuestWriteWatch::GuestWriteWatchUnregister_nid_postfix(region, regionBytes); #endif } @@ -166,7 +170,10 @@ public: private: bool watched; + std::size_t regionBytes; std::vector storage; + std::uint8_t* region = nullptr; + void* allocation = nullptr; std::uint32_t* texels = nullptr; }; @@ -210,7 +217,7 @@ void Settle(THold& hold, const Surface& surface, std::uint32_t result) { hold.unlock(); } -enum class Kind { ReadAfter, ReadBefore, Filled, Copied, LeftWatch, WrittenThenLeftWatch, ImportLeftWatch, NeighbourCopied }; +enum class Kind { ReadAfter, ReadBefore, Filled, Copied, LeftWatch, WrittenThenLeftWatch, ImportLeftWatch, NeighbourCopied, NeighbourLeftWatch }; // One 64 KiB block of the surface: the image of its last block, and the source copied into it. constexpr std::uint32_t BlockSide = 128; @@ -289,8 +296,10 @@ Outcome RunSecondStore(const char* name) { Outcome Run(const Case& test) { std::optional source; if (test.kind == Kind::Copied || test.kind == Kind::NeighbourCopied) source.emplace(); - const bool leaves = test.kind == Kind::LeftWatch || test.kind == Kind::WrittenThenLeftWatch || test.kind == Kind::ImportLeftWatch; - Surface surface(leaves); + const bool neighbour = test.kind == Kind::NeighbourLeftWatch; + const bool leaves = test.kind == Kind::LeftWatch || test.kind == Kind::WrittenThenLeftWatch || test.kind == Kind::ImportLeftWatch || neighbour; + surfaceTile = neighbour ? TileLinear : TileR64KBX; + Surface surface(leaves, neighbour ? Page : 0); struct WaitForWork { ~WaitForWork() { AgcDriverWaitIdle_nid_postfix(); } } waitForWork; @@ -385,8 +394,9 @@ Outcome Run(const Case& test) { Require(image != nullptr, "store over untracked: the surface has no storage image"); Require(!image->GuestSnapshotValid(), "store over untracked: the pending result had saved guest bytes while its memory was write-watched"); if (test.kind == Kind::ImportLeftWatch) Require(AgcDriver::GuestMemory::ImportWatched(address, SurfaceBytes, [] { return true; }, false), "store over untracked: the import that leaves the write watch failed"); + else if (neighbour) Require(AgcDriver::GuestMemory::ImportWatched(address - Page, Page, [] { return true; }, false), "store over untracked: the import of the page before the surface failed"); else AgcDriver::GuestMemory::Unwatch(address, SurfaceBytes); - Require(!AgcDriver::GuestMemory::Watched(address, SurfaceBytes), "store over untracked: the surface is still write-watched"); + if (!neighbour) Require(!AgcDriver::GuestMemory::Watched(address, SurfaceBytes), "store over untracked: the surface is still write-watched"); } std::array read{}; if (test.kind == Kind::ReadBefore) AgcDriver::GuestMemory::Read(address + Block + Page, read); @@ -418,7 +428,7 @@ int main() { Surface probe; Require(AgcDriver::Graphics::DescribeSurface(AgcDriver::Graphics::DecodeTextureResource(TextureDescriptor(probe.Texels()))).guestBytes == SurfaceBytes, "store over untracked: unexpected surface size"); } - constexpr std::array cases{{ + constexpr std::array cases{{ {"another block read after the CPU write", Kind::ReadAfter, Block + Page}, {"another block read before the CPU write", Kind::ReadBefore, 0xe000}, {"a fill of the surface", Kind::Filled, 0xe000}, @@ -427,6 +437,7 @@ int main() { {"a surface written before it left the write watch", Kind::WrittenThenLeftWatch, 0xe000}, {"a surface whose import left the write watch", Kind::ImportLeftWatch, 0xe000}, {"an image copy into another block of the surface", Kind::NeighbourCopied, 0xe000}, + {"a surface whose first block left the write watch with the page before it", Kind::NeighbourLeftWatch, 0xe000}, }}; bool kept = true; for (const auto& test : cases) {