fix(agc): run the unwatch hook before an import whose range leaves the write watch whatever the import does

core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp

@@ -1825,12 +1825,26 @@ bool ImportWatched(std::uint64_t address, std::size_t bytes, const std::function
             hashes[static_cast<std::size_t>((cursor - first) / page)] = HashPage(reinterpret_cast<const std::byte*>(cursor));
         }
     }
-    std::erase_if(spans, [&](const auto& span) { return !walkWrites(tracker, span.first, span.second, StampKind::Cpu, true); });
+    const auto saveBytes = [&](std::uint64_t begin, std::uint64_t end) {
+        lock.Suspend();
+        beforeUnwatch(begin, static_cast<std::size_t>(end - begin));
+        lock.Resume();
+    };
+    std::vector<std::pair<std::uint64_t, std::uint64_t>> leaving;
+    std::erase_if(spans, [&](const auto& span) {
+        if (walkWrites(tracker, span.first, span.second, StampKind::Cpu, true)) return false;
+        leaving.push_back(span);
+        return true;
+    });
+    for (const auto& [begin, end] : leaving) saveBytes(begin, end);
     if (!runImport()) return false;
     const WindowCopy window{first, hashes};
     auto unwatchedFrom = first;
     for (const auto& [begin, end] : spans) {
-        if (!walkWrites(tracker, begin, end, StampKind::Cpu, true, &window)) continue;
+        if (!walkWrites(tracker, begin, end, StampKind::Cpu, true, &window)) {
+            saveBytes(begin, end);
+            continue;
+        }
 #ifdef _WIN32
         GuestArena::GuestArenaCompareResidentWhenShared_nid_no_patch(static_cast<std::uintptr_t>(begin), static_cast<std::size_t>(end - begin));
 #endif

core/libs/prx/libSceAgcDriver/tests/WriteTracking.cpp

@@ -1718,6 +1718,80 @@ void CheckUnwatchedImport() {
 #endif
 }
 
+struct HookCall {
+    std::uint64_t address;
+    std::size_t bytes;
+    bool watched;
+};
+
+std::vector<HookCall> hookCalls;
+std::uint64_t hookProbe = 0;
+std::size_t hookProbeBytes = 0;
+
+void RecordHook(std::uint64_t address, std::size_t bytes) {
+    hookCalls.push_back({address, bytes, Watched(hookProbe, hookProbeBytes)});
+}
+
+struct RecordedHook {
+    RecordedHook() {
+        hookCalls.clear();
+        SetUnwatchHook(&RecordHook);
+    }
+    ~RecordedHook() { SetUnwatchHook(nullptr); }
+    RecordedHook(const RecordedHook&) = delete;
+    RecordedHook& operator=(const RecordedHook&) = delete;
+};
+
+void RequireHookOver(std::uint64_t address, std::size_t bytes, const std::string& path) {
+    Require(hookCalls.size() == 1, path + " did not run the unwatch hook once");
+    const auto& call = hookCalls.front();
+    Require(call.address <= address && address + bytes <= call.address + call.bytes, path + " gave the unwatch hook a range that misses its own");
+    Require(call.watched, path + " ran the unwatch hook after its range left the write watch");
+}
+
+void CheckUnwatchHooks() {
+    void* memory = AllocateWatched(8 * Block);
+    const auto base = reinterpret_cast<std::uint64_t>(memory);
+    std::memset(memory, 0x11, 8 * Block);
+    Require(CollectWritesUncached(base, 8 * Block) != 0, "the hooked block is not collected");
+    const RecordedHook recorded;
+    hookProbe = base;
+    hookProbeBytes = Block;
+    Unwatch(base, Block);
+    RequireHookOver(base, Block, "Unwatch");
+    Require(!Watched(base, Block), "Unwatch left its range watched");
+
+    hookCalls.clear();
+    hookProbe = base + Block;
+    bool hookedBeforeImport = false;
+    Require(ImportWatched(base + Block, Block, [&] {
+        hookedBeforeImport = hookCalls.size() == 1;
+        return true;
+    }, false), "an import that does not stay watched failed");
+    RequireHookOver(base + Block, Block, "an import that does not stay watched");
+    Require(hookedBeforeImport, "an import that does not stay watched ran the unwatch hook after the import");
+    Require(!Watched(base + Block, Block), "an import that does not stay watched left its range watched");
+
+    hookCalls.clear();
+    Require(ImportWatched(base + 2 * Block, Block, [] { return true; }), "an import that stays watched failed");
+    Require(hookCalls.empty() && Watched(base + 2 * Block, Block), "an import whose page walk vouched for its pages ran the unwatch hook or gave up its range");
+
+#ifndef _WIN32
+    hookCalls.clear();
+    const auto late = base + 4 * Block;
+    hookProbe = late;
+    bool hookedBeforeLoss = false;
+    Require(ImportWatched(late, 3 * Block, [&] {
+        GuestWriteWatch::GuestWriteWatchUnregister_nid_postfix(reinterpret_cast<const void*>(late + 2 * Block), Block);
+        hookedBeforeLoss = !hookCalls.empty();
+        return true;
+    }), "an import whose range lost a block during the import failed");
+    RequireHookOver(late, 3 * Block, "an import whose page walk failed after the import");
+    Require(!hookedBeforeLoss, "an import whose page walk failed after the import ran the unwatch hook before it");
+    Require(!Watched(late, Block), "an import whose page walk failed after the import left its range watched");
+#endif
+}
+
 class PartlyCommitted {
 public:
     static constexpr std::size_t Bytes = 4 * Block;
@@ -1948,6 +2022,7 @@ int main() {
         CheckStoreAcrossImports();
         CheckImportWindowFootprint();
         CheckUnwatchedImport();
+        CheckUnwatchHooks();
         CheckCommittedRanges();
         CheckAdoptLatestEpoch();
     } catch (const std::exception& error) {

docs/dev/debt/libSceAgcDriver.md

@@ -129,7 +129,7 @@
 - [Resident linear color targets](../../../core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp) (libSceAgcDriver): a CPU store inside a drawn scissor rectangle, on a pixel no primitive wrote, made before the target's store is overwritten by it, where the console keeps it. Recorded draws' results are stored before the graphics queue's next flip, label, end-of-pipe interrupt, drain or device wait, as the union of their scissor rectangles; a draw that brings a 17th rectangle that does not merge stores the target at once. A target with an attachment proxy, several mips or layers, a 3D or DCC surface, or results of a dispatch, fill or copy is stored after each draw. One that a fill or copy writes after its draws is stored whole at the next of those points, and GPU results in its CPU-written 64 KiB blocks are dropped. A store the draw pipeline's committer makes (label, end-of-pipe interrupt, device wait) skips a target whose range a queued draw still reads (`DrawPipeline::HeldReads`).
 - [Direct writes over a drawn linear target](../../../core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp) (libSceAgcDriver): a CPU store into a 64 KiB block of a drawn resident linear target that lands while the completion of a direct write recorded before the draws checks and stamps that block (`StampKeepingNewerResults`) drops the draws' results in that block, where the console keeps them: the stamp's own collect records the store per page only, so the store reads the block as written by the driver.
 - [`APS5_SKIP_REFUSED_DRAWS`](../../../core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawCapture.cpp) (libSceAgcDriver) drops a draw whose graphics stage the recompiler refuses instead of failing the queue, so the frame lacks that draw, and the stage is compiled and refused again at each such draw. Compute refusals still fail.
-- [Pending storage-image results over memory outside the write watch](../../../core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp) (libSceAgcDriver) leave to the CPU the 64 KiB blocks whose bytes differ from the ones saved at the upload, the fill, the image copy, the previous store or the moment their range left the watch (`Unwatch`, or a host import that does not stay watched), and the blocks the watch saw the CPU write before that moment. A store made on the GPU saves its bytes through a host-visible copy that becomes the saved bytes when its batch completes; a store or a refresh that needs them earlier waits for that batch. A unit without a write generation saves the guest bytes when results land in it outside the watch. One whose range leaves the watch while its results are pending saves its bytes through the unwatch hook, which also runs when a failed collect takes a range out of the watch. A range that leaves the watch without the hook running over it (an import fallback that unwatched without saving) leaves its pending results without saved bytes; storing such results throws. A unit whose results were stored while its blocks were watched keeps only a generation; when its blocks leave the watch while nothing is pending, the hook saves nothing, and results that land in it afterwards have neither saved bytes nor a watch, so storing them throws too.
+- [Pending storage-image results over memory outside the write watch](../../../core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp) (libSceAgcDriver) leave to the CPU the 64 KiB blocks whose bytes differ from the ones saved at the upload, the fill, the image copy, the previous store or the moment their range left the watch (`Unwatch`, or a host import that does not stay watched or whose page walk fails), and the blocks the watch saw the CPU write before that moment. A store made on the GPU saves its bytes through a host-visible copy that becomes the saved bytes when its batch completes; a store or a refresh that needs them earlier waits for that batch. A unit without a write generation saves the guest bytes when results land in it outside the watch. One whose range leaves the watch while its results are pending saves its bytes through the unwatch hook, which also runs when a failed collect takes a range out of the watch. A range that leaves the watch without the hook running over it leaves its pending results without saved bytes; storing such results throws. A unit whose results were stored while its blocks were watched keeps only a generation; when its blocks leave the watch while nothing is pending, the hook saves nothing, and results that land in it afterwards have neither saved bytes nor a watch, so storing them throws too.
 - [Storage images over uncommitted guest memory](../../../core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp) (libSceAgcDriver) - a storage image whose guest range has no committed page when it is created throws in the `StorageTexture` constructor, which aborts the guest. Several titles reach this intermittently in runs of a few minutes, on AMD and NVIDIA alike; in two of the three logs, lines about storage images over released or rewritten memory (`memory no longer registered`, `guest memory changed while GPU results were pending`) come shortly before it. The log does not name the image's address. What the console does with a storage descriptor over memory that is not committed is unknown, and which allocation the guest recreates there was not found.
 - Windows host-pointer imports of the guest mapping itself are compared instead of write-watched, except writable imports of private `MEM_WRITE_WATCH` memory when the import probe keeps the write watch and a settle queue exists: those of 1 MiB or more stay watched, and smaller ones return to the watch once their settle has completed, unless a storage image overlaps them (see Pages under an import window): a driver can mark imported pages dirty after the synchronous import window. Shared memory imported through a separate writable alias retains protection-based tracking on the guest views; driver access to the import alias does not disarm those views. [GuestMemory](../../../core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp) excludes every overlapping 64 KiB tracker block while its backing is retained. A full block replaced by fresh private backing regains tracking and gets a new generation; protection changes, partial replacements and shared remappings do not restore it. Excluded blocks keep their write-watch marks as a hint only (`UntrackedHint`): a storage image or a sampled texture snapshot over them compares its bytes again after a mark in its range, a driver store, a mapping change or a recorded GPU write instead of at every collect epoch or lookup, and a mark the driver makes without a CPU store costs one comparison. This preserves CPU edits through byte comparisons but limits snapshot reuse on excluded blocks to draws of one collect epoch and disables unit shadows for them; restoring tracking on import retirement without replacing the backing remains unimplemented.
 - [Pages under an import window](../../../core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp) whose protection an import resolves count as stored only when their bytes change. A same-value CPU store during that window cannot be distinguished from driver activity and does not win over pending results. On Windows, NVIDIA 617.42 marks every private MEM_WRITE_WATCH page after import until a GPU use completes. The driver behavior was measured on this driver; other drivers are unverified. Imports of at least 1 MiB wait for a four-byte read on the separate settle queue inside the window. Smaller imports stay compared until a deferred read completes; a later lookup returns their pages to write watching, stamping their blocks as CPU stores; the rest of a partly covered block stays compared. Blocks overlapping a live storage image, blocks of an import retired before rewatching, and imports without a settle queue stay compared. Shared-memory imports retain their separate-alias tracking.