Repository navigation
fix(agc): run the read guard over a shader capture's lazy pages and raw word reads - #6475
Merged
adriadam10 merged 3 commits intoOct 19, 2026
Conversation
Read, ReadCommitted and ReadCommittedUnsynced each loaded the installed read guard and called it inline. GuardRead does that once, so code that reads guest memory without going through those functions can run the same check. No behaviour change.
Since boykopovar#1193 (4651980) a capture copies a page in 64-byte blocks as it reads them, instead of reading the whole page with GuestMemory::Read the first time it touches it. The page now only gets FlushGpuWrites, which runs the flush hook but not the read guard, so a capture over a multisampled color target's memory, whose samples are only on the host, read the stale guest bytes silently where it threw before. ShaderMemory::page runs GuardRead over the page where the whole-page read stood, before the flush, as Read does. The check stays once per page a capture touches, as before boykopovar#1193. The block copies need none: a pooled page is reset and goes through page() again, and later reads of a page were not checked again before boykopovar#1193 either. The test captures a shader whose SRT lies in a live 4x target's memory, then one whose SRT lies just past it, then the first again on the pooled page.
On a page that recorded GPU work writes, a capture reads each dword on its own. Where the driver's evidence says that work leaves the dword unchanged (PendingWrite::Raw), the dword is copied straight from guest memory, without the read guard the other word reads get through GuestMemory::Read. The raw path (29b4601) predates the guard (d109f39), so a capture whose raw word lies in a live multisampled target's memory has read stale bytes instead of throwing since then. The raw read now runs GuardRead over the dword first. With no target registered that is a few atomic loads, next to the pending-write query each raw word already makes. The test answers Raw for every page and word, so the capture's page is read word by word and raw, and Sync for the word reads that were already guarded.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
This was referenced Oct 15, 2026
Contributor
|
Thanks, this is what I asked for on #1193. I read the diff: the guard runs once per page at first touch, where the whole-page read used to be, and the raw-word path now has it too. That closes the gap #1193 opened, and the gap from 29b4601 that predates #419. The test covers a fresh page, a pooled page, raw words and synced words, and checks that a capture just past the 64 KiB target still reads normally, on its own page within the 5 x 64 KiB block. Good from my side. |
Contributor
Author
|
Thanks for reading it through, @MichonGoddijn231849. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
#1193 dropped a safety check: a shader capture no longer runs the multisample read guard over the guest memory it reads. @MichonGoddijn231849 confirmed it and asked for this fix with a test (comment).
The regression. Before #1193,
ShaderMemory::pageread a page withGuestMemory::Readthe first time a capture touched it, andReadruns the read guard first. The guard #419 installs (guardReadsinMultisampleTarget.cpp) makes a CPU read throw when it overlaps a multisampled color target, whose samples are only on the host, or a resident native multisample depth target, instead of returning bytes that were never written there. #1193 (4651980) replaced the whole-page read withFlushGpuWrites, and copies each 64-byte block as the capture reads it.FlushGpuWritesruns the flush hook but not the guard, so a capture over such a target read the stale guest bytes silently.The fix (commits 1-2).
GuestMemory::GuardReadexposes the guard, andRead,ReadCommittedandReadCommittedUnsynceduse it.ShaderMemory::pagecalls it over the page where the whole-page read stood, before the flush, asReaddoes. So the check runs once per page a capture touches, at the same point and over the same range as before #1193, and costs what it did then: with no target registered, a few atomic loads. The block copies don't need it:page()again;ShaderMemory) is no less checked than before, and its host-only rendering doesn't change the guest bytes a block copy reads.A sibling gap (commit 3). On a page that recorded GPU work writes, a capture reads each dword on its own. The dwords that the driver's write evidence says that work leaves unchanged (
PendingWrite::RawandVerifyRaw) are copied straight from guest memory. That path (29b4601) predates the guard (#419) and never ran it, unlike the word reads throughGuestMemory::Read. It now runsGuardReadover the dword first: a few atomic loads per raw word when nothing is registered, next to the pending-write query each raw word already makes. With it, every guest read a capture makes runs the guard: whole pages, lazy blocks (through their page), synced words and raw words. Known values (PendingWrite::KnownValue) read no guest memory.The commits:
refactor(agc): expose the guest read guard asGuestMemory::GuardRead. No behaviour change.fix(agc): run the read guard over a shader capture's lazily read pages, with its test.fix(agc): run the read guard on a shader capture's raw word reads, with its test.Scope: the driver's other direct guest reads aren't shader captures, weren't guarded before #1193, and #1193 didn't touch them. They are buffer uploads and input snapshots (
ShaderResources.cpp,GuestBufferMemory.cpp), the write-evidence copies taken before a hook read (VariantValidation.cpp,CaptureValidation.cpp, the recorder) and copy count records (BufferCopy.cpp). They lie outside the guard as #419 defines it (CPU reads throughGuestMemory::ReadandReadCommitted, DMA_DATA copies, texture and storage image lookups), and this PR leaves them as they are.Tested
agc_multisample_guest_access(multisampleGuestAccessTestsintests/Recorder.cpp), next to the existing refusals of plain reads of a live 4x target:Rawfor every page and word, so the capture reads word by word and raw, the capture must throw, and one past the target must read normally; withSync, the word reads that were already guarded must throw too.main92e5041: withmain'sShaderMemory.cppandGuestMemory.cpp,agc_multisample_guest_accessfails on Linux at "a shader capture of a resolved-only multisampled target's memory did not throw"; with the fix, the recorder binary's 39 tests pass 20 of 20 (run).ShaderMemory.cpp, the test fails on Linux at "a shader capture's raw word read of a resolved-only multisampled target's memory did not throw"; with commit 3 the 39 tests pass 20 of 20 (run).guest_kqueue, which fails onmainin the same runs. The suites pass on Linux too (75 and 143 tests).^agcsweep at the head (run): 602 of 602 pass on Linux (42 skipped). On Windows 594 of 596 pass, and both failures also fail without these commits:agc_driver_pm4_replay_toolexits with 0xc0000139 in every Windows sweep on GitHub's runners today, including branches without this change, andagc_interpolant_mappingprinted that its tests passed and then timed out at exit, a hang we've also seen without this change.ANYPS5_REQUIRE_VULKAN=1, at this head:agc_multisample_guest_accesspasses 5 of 5. Withmain's guard code (GuestMemory.hpp,GuestMemory.cppandShaderMemory.cppat 92e5041, the new test kept) it fails 5 of 5 with "a shader capture of a resolved-only multisampled target's memory did not throw".python3 tools/check_conventions.py --base origin/main: passed.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)