Repository navigation
fix(libc): track writes to shared guest memory per 4 KiB page on Windows - #4661
Conversation
|
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.
|
|
Preparing a Windows 007 cross-test of canonical9a535a47c, source-only fork branch codex/first-light-shared-page-cross-test head9a7e82b10 (production port286bca3a2, authorship retained). Our integration has multi-section read/write aliases and an older whole-view Collect fast path, so I restored this PRs per-subpage settled generations, clean-range boundary and partial reset semantics while preserving alias-run handling. Also restored the partial-collect selector and both CTest registrations missing from our older stack. Conventions pass; not built/staged yet, because a separate held-label game comparison is running. Will run the new page test plus production-only negative before reporting gameplay. No 007 attribution or FPS claim yet. AI-assisted: Codex. |
|
Windows RTX5070Ti cross-test head9a7e82b1001a7e8c614f12cfd6f05cccb25c75fa: native guest_memory plus partial-collect single/two-pass3/3PASS1.68s. Production-only negative restores WindowsMappings.hpp from87d100e4e with fixtures unchanged: both partial cases FAIL partial collect lost a neighboring guest write; guest_memory aborts at1167, collect(first,page*3,false)==1 (old whole-view tracking returns more). Restoring the port gives10/10focused native testsPASS3.94s plus agc_shared_write_tests defaultPASS; alias-run tests in guest_memory pass. Our old local whole-view Collect fast path also consumed neighboring writes, so this is not merely a test of upstream main; no main-only or 007-causality claim. No CTest skips in this selection, but host-import watched-shadow subcase remains unavailable on Windows. Conventions pass; registration checker reports an existing Windows agc_table_bench artifact without a registered test, not a new partial-collect registration issue. Staging only libc99D37E344723BB3C989841B28736F88FCB0B46EDC2CB5AEADC303A1D1E84FEE4, retaining graphics830E3AEC... for isolated game comparison. AI-assisted: Codex. |
Writes into shared (direct) guest memory on Windows are caught by write protecting each 16 KiB view and taking the fault. The fault reopened and invalidated the whole view, so a collect reported all four 4 KiB pages of a view whenever any byte of it was written, including pages nobody wrote. A GPU surface that ends or starts inside such a view then had its edge pages taken for CPU writes whenever the title wrote the memory next to it, and the storage write-back kept the guest's stale bytes there instead of the GPU's results. Each view now keeps a write generation, a seen generation and the write protection per 4 KiB page. A fault invalidates and reopens only its page, a host write only the pages it covers, and a collect reports and re-arms only the pages it covers. VirtualProtect is called once per contiguous run of pages whose protection changes, so arming or pinning a view takes one call when those pages are contiguous and two when they are not. A guest store that fills a whole view takes up to four faults instead of one. A collect does not settle a page that it cannot write protect because a host write is open on one of the page's views; it reports the page again until a collect can arm it. main settled such a page and relied on the end of the host write invalidating the whole view, so a guest store into it during the host write was reported only after the host write ended. With only the written pages invalidated there, the store would not be reported at all. A collect that runs out of room partway through a view now settles the pages it already reported there and nothing after them; before, it settled none of them and reported them again at the next collect. CheckSharedWritePages writes one byte into the second 4 KiB page of a view and requires a collect of the view's last two pages to report nothing; main reports both. It also checks a store across two pages and a file read into one page. CheckSharedWritesDuringHostWrite stores into a fault-opened page of a view after a collect while a host write is open on another page of it, and requires every collect of the view and of its alias to report that page until the host write has ended. agc_shared_write_partial_collect resets the two interior pages of a view after writes to all four and requires the outer two to stay pending. The existing shared-memory checks in guest_memory, agc_shared_write_partial_collect and guest_socket_host_write (the recvfrom sender outputs) now expect the pages written instead of the whole view. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9a535a4 to
1eb13fd
Compare
|
Revision after review, rebased on current
|
|
Picked up revised1eb13fdf025bab9283bd6d18f6c55afdfcfd2646. Our currently staged libc99D37E34 still has the earlier9a535a47c Collect behavior. Prepared author-credited port09f0f6394: https://github.com/Scramblrrrr/AnyPS5/tree/codex/first-light-shared-host-write . Only the revised Collect block and CheckSharedWritesDuringHostWrite fixture/call are added; local alias-run mapping and prior stronger partial-collect checks are preserved. The unrelated current-main mapping changes and newer socket fixture are not imported, so this is explicitly a revision port rather than whole-head validation. No builds/native tests/staging yet while the preceding007 game control runs. Next: guest_memory and shared-page collection tests with a production-only old-Collect negative control, then an isolated libc game comparison. Latest007 guest entity-streaming crash is not being attributed to this tracking issue without evidence. AI-assisted: Codex. |
|
Windows revision-port validation for09f0f6394 (#4661 Collect correction/regression from1eb13fdf025bab9283bd6d18f6c55afdfcfd2646), keeping our local alias-run mapping. guest_memory plus partial-collect single/two-pass3/3 PASS7.65s. Production-only negative restores WindowsMappings.hpp from e8cc22c with new fixture unchanged: guest_memory aborts at1272 (collect after guest store during open host write), both older partial-collect controls PASS. Restore/touch/rebuild gives3/3 PASS1.52s. Source clean. This is not a full upstream-head/platform-suite test; newer upstream socket fixture/current-main mapping changes are not imported. Preceding graphicsFFD11FE1/libc99D37E34 game control survived Only libc changed for new game run09f0f6394: SHA256 A1AE7842060DE43B76D099A9A180F739DBE8FB6D4B8731C7C0BDE7538E4730BA, graphicsFFD11FE14B53708C49F639F3826E31B6CC4CE6BD55AB267CFE07CAE3464C69DD unchanged. Normal warm-cache/cache16, no concurrent builds/native tests or sampler/profile. No game crash-fix/FPS claim yet. AI-assisted: Codex. |
|
An Intel run, the full suite, and Setup: Windows 11 (26200), WinLibs GCC 15.2.0 r7, Release, CI configure flags, an RTX 4070 Laptop GPU and an Intel Raptor Lake-S iGPU. The head 1eb13fd merged into
Written with AI assistance. |
What
On Windows, writes into shared (direct) guest memory are caught by write-protecting each 16 KiB view and taking the fault. The fault reopened and invalidated the whole view, so a collect reported all four 4 KiB pages of a view whenever any byte of it was written, including pages nobody wrote.
A GPU surface that starts or ends inside such a view then had its edge pages taken for CPU writes whenever the title wrote the memory next to it. The storage write-back kept the guest's stale bytes for that whole edge block instead of the GPU's results.
Demon's Souls (PPSA01342 v1.005) shows this in every Bink movie, the opening cinematic included: a green band along the bottom-right of the frame that never clears.
memset/memcpycalled from the title).Each view now keeps a write generation, a seen generation and the write protection per 4 KiB page:
VirtualProtectis called once per contiguous run of pages whose protection changes. Arming or pinning a view takes one call when those pages are contiguous (for example all four after a whole view was written) and two when they are not (pages 0 and 2, say). A guest store that fills a whole view takes up to four faults instead of one; this follows from the code and was not measured on its own (see Tested).A collect does not settle a page that it cannot write-protect because a host write (a file or AIO read, a socket receive) is open on one of the page's views: it reports the page again at every collect until a collect can arm it. That over-reporting lasts as long as the host write, which can be a blocking socket receive (
recv/recvfrominUdp.cpp,sceNetRecv/sceNetRecvfromin libSceNet), and every such collect reports those pages as CPU-written again. Onmaina collect settled such a page, and the end of the host write invalidated the whole view, so a guest store into the page during the host write was reported only after the host write ended. With only the written pages invalidated at the end, the first revision of this PR lost that store; the reviewer found it.When a collect runs out of room partway through a view, it now settles the pages it already reported there and nothing after them. Before, it settled none of them and reported them again at the next collect.
Tests:
CheckSharedWritePages(guest_memory) writes one byte into the second 4 KiB page of a view and requires a collect of the view's last two pages to report nothing;mainreports both. It also checks a store across two pages and a file read into one page.CheckSharedWritesDuringHostWrite(guest_memory) opens a host write on the third page of a view that has a fault-opened second page, stores into the second page after a collect, and requires every collect of the view and of an alias to report that page, during and after the host write, and the page to be armed again after it. With the first revision'sCollect(this revision's host-write check inCollectreverted) it fails atGuestMemory.cpp:1294, the collect of the first view during the host write after the store. With line 1294 removed it still fails at line 1294 (the collect of the alias, which moves up). Only with both collects during the host write after the store (lines 1294 and 1295) removed does it fail at the collect after the host write ended (line 1297 as committed), which is the lost write the reviewer described.agc_shared_write_partial_collectresets the two interior pages of a view after writes to all four and requires the outer two to stay pending, for the view and for its alias (the per-page form of the old "partial resets consumed intervening writes" check). ACollectchanged to settle the whole view on a partial reset fails it.guest_memory,agc_shared_write_partial_collectandguest_socket_host_write(the recvfrom sender outputs, added tomainsince the first revision) now expect the pages written instead of the whole view. Withmain'sWindowsMappings.hpp,guest_memoryfails at its first such check,GuestMemory.cpp:1189.Demon's Souls with the first revision of this change: the band is gone. Green pixels in the bottom band of the frame at frames ~1500, ~1714 and ~1900 of the opening cinematic go from 7,784, 10,671 and 885 to 0, 0 and 0. These runs were on
mainat 7a84b22 plus #425, #2904 and #757, which the title needs to reach the cinematic; #2904 has been merged since and is part of the base of this branch now. The write-back no longer kept any block of the planes for the CPU (it did every few frames before). In the same stretch of the cinematic the frame rate read 2.4-2.6 FPS against 1.8-1.9 before. That is one run of each build without traces or summary lines, so this PR does not claim a change in speed; the cost of the extra faults was not measured separately.Related:
demote()assignsview.seen = 0, which does not compile against the per-pageseenhere, so whichever of the two lands second needs a code change, not only a rebase.Collectlines and adds its own per-4 KiBcollectedmask toView. I propose this PR goes first, since fix(agc): defer write tracking for fresh Windows shared imports #4535's mask can then sit beside the per-page state; if fix(agc): defer write tracking for fresh Windows shared imports #4535 lands first, I will rebase this one onto it.seen/armed/rememberCleanlines of the shared-view walk; whichever merges second needs a rebase.WindowsMappingsit only clampsavailablein the privateGetWriteWatchbranch and addsProbeCleanand its mapping serial (a counter bumped inCommit,ResetandMap, read throughMappingSerial());ProbeCleangives up on any shared view.Tested
This round: Windows Server 2025, RTX 3060, GCC 15.2, Release build, branch rebased on current
main(ee49621). The tests below ran on 8a67eb2 (the one merge since, #4787, changes only CI workflow files), except theCollectmutation and themainheader comparison, which ran on 584cc4c (the merge after it, #4456, touches only the AGC pipeline library).guest_memory,agc_shared_write*,guest_socket_host_write_*,agc_write_tracking*,agc_write_watch*(18 tests) pass. With only the newCollecthost-write check reverted,guest_memoryfails atGuestMemory.cpp:1294(see Tests above for the two other variants run).Collectchanged to settle all four pages of a view on every collect,agc_shared_write_partial_collectandagc_shared_write_partial_collect_two_passfail ("full collect after an interior reset did not preserve pending subpages").ctest -R 'agc|guest|libc|kernel' -j 4, 813 tests: 802 pass, 2 skipped, 9 fail:agc_driver_flat_loads*,agc_driver_global_loads*andagc_driver_flat_apertures_2/_5abort on a guest allocation still pinned by GPU work after 60 s,agc_driver_pixel_interlockfails. The same 9 fail the same way withmain'sWindowsMappings.hppswapped in and the libraries rebuilt; with that headerguest_memoryfails atGuestMemory.cpp:1189. Before theguest_socket_host_writeexpectation was updated, its two recvfrom UDP tests failed with this change and passed withmain's header (observed in the previous round).ctest -R 'socket|net|udp|tcp|rudp|http|ssl|resolver', 29 tests): all pass.tools/check_test_registration.py: no new unregistered tests.Not run this round: no title. The Demon's Souls figures above, and Dreaming Sarah reaching FPS 59.92 in
run-checkand 59.95 inrun-own-libc-check, are from the first revision before the rebase.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)🤖 Generated with Claude Code