fix(vk): preserve CPU write watches during pool imports #1434

github-sync · 2026-11-10T02:20:42Z

klimovich008 commented

2026-11-10 02:20:42 +00:00

(Migrated from github.com)

What this fixes

CPU write diagnostics can miss a write immediately after a GPU memory pool is imported when a built-in watch mode is enabled. This hides the write being investigated until the watcher rearms.

Why merge

Uses the existing query walk for the four current watch switches, preserving watched pages.
Default imports keep the fast single commit and its existing failure fallback.
Chunk spans, GPU bindings, writeback and the host/renderer API stay unchanged. No new setting.

Change

gpu/vk/vk_memory.cpp: select the existing query walk when CPU write watching is requested.
tests/test_import_write_watch.py: check real Windows page protection and write exceptions on synthetic pages.
tests/run_all.py: register the check.

Tested

python -I -B tests/test_import_write_watch.py --cxx clang++ covers all four watch modes, ordinary commits and forced commit failure.
Fails on the old code, passes now. Each watch receives one real write fault; ordinary commits use zero queries; commit failure reaches the fallback.
Windows/RX 9070 XT PC: six CPU cases pass. Existing import-span/import-window tests and the full vk_memory.cpp syntax check pass.

Not covered

Native Windows VM checks only; the new test explicitly skips other platforms.
No full-game or GPU run of this patch.

## What this fixes CPU write diagnostics can miss a write immediately after a GPU memory pool is imported when a built-in watch mode is enabled. This hides the write being investigated until the watcher rearms. ## Why merge - Uses the existing query walk for the four current watch switches, preserving watched pages. - Default imports keep the fast single commit and its existing failure fallback. - Chunk spans, GPU bindings, writeback and the host/renderer API stay unchanged. No new setting. ## Change - `gpu/vk/vk_memory.cpp`: select the existing query walk when CPU write watching is requested. - `tests/test_import_write_watch.py`: check real Windows page protection and write exceptions on synthetic pages. - `tests/run_all.py`: register the check. ## Tested - `python -I -B tests/test_import_write_watch.py --cxx clang++` covers all four watch modes, ordinary commits and forced commit failure. - Fails on the old code, passes now. Each watch receives one real write fault; ordinary commits use zero queries; commit failure reaches the fallback. - Windows/RX 9070 XT PC: six CPU cases pass. Existing import-span/import-window tests and the full `vk_memory.cpp` syntax check pass. ## Not covered - Native Windows VM checks only; the new test explicitly skips other platforms. - No full-game or GPU run of this patch.

hckrman101 (Migrated from github.com) approved these changes

2026-11-10 04:16:19 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks right to me. On Windows, MEM_COMMIT over pages that are already committed does reset their protection, so the one-call commit could quietly unprotect a watched page until wts_rearm put it back (up to about 2 ms later). The old walk skips anything that isn't MEM_RESERVE, so watched pages stay read-only.

I checked the switch list against main. All four switches reach hos_watch_guest_writes or watch_texels_source: RX_NVN_WATCH_TEX and RX_NVN_WATCH_TEXELS_SRC directly in nvn_native.c, and RX_VK_WATCH_CPU (vk_replay.cpp) and RX_VK_VERIFY_DIM (vk_texture.cpp verify_report) through host->watch_guest_writes. Nothing else calls VirtualProtect on guest memory, so the list is complete today. Linux doesn't change: the posix VirtualAlloc returns early for commits inside the guest window, and VirtualQuery reports that window as a single committed region.

The new test runs and passes on the windows-2022 CI job (not just a skip), and it's registered in run_all.py. CI is green. No overlap with #1127 (its vk_memory.cpp hunks are the includes and mirror_make) or with #1353's run_all.py line.

Two small optional things:

(nit) The env list could get out of sync with the callers. A flag set inside hos_watch_guest_writes / watch_texels_source ("some guest page is protected") would be exact, and it would keep the fast path until a watch is actually armed. Your comment already says this, and the static list is fine for a diagnostics-only path.
(nit) The test pulls the block out of vk_memory.cpp by matching the comment text and the next #ifdef __linux__. If someone rewords that comment, the test fails with a bare ValueError. A clearer error message there would help whoever hits it.

Looks right to me. On Windows, `MEM_COMMIT` over pages that are already committed does reset their protection, so the one-call commit could quietly unprotect a watched page until `wts_rearm` put it back (up to about 2 ms later). The old walk skips anything that isn't `MEM_RESERVE`, so watched pages stay read-only. I checked the switch list against main. All four switches reach `hos_watch_guest_writes` or `watch_texels_source`: `RX_NVN_WATCH_TEX` and `RX_NVN_WATCH_TEXELS_SRC` directly in nvn_native.c, and `RX_VK_WATCH_CPU` (vk_replay.cpp) and `RX_VK_VERIFY_DIM` (vk_texture.cpp `verify_report`) through `host->watch_guest_writes`. Nothing else calls `VirtualProtect` on guest memory, so the list is complete today. Linux doesn't change: the posix `VirtualAlloc` returns early for commits inside the guest window, and `VirtualQuery` reports that window as a single committed region. The new test runs and passes on the windows-2022 CI job (not just a skip), and it's registered in run_all.py. CI is green. No overlap with #1127 (its vk_memory.cpp hunks are the includes and `mirror_make`) or with #1353's run_all.py line. Two small optional things: - (nit) The env list could get out of sync with the callers. A flag set inside `hos_watch_guest_writes` / `watch_texels_source` ("some guest page is protected") would be exact, and it would keep the fast path until a watch is actually armed. Your comment already says this, and the static list is fine for a diagnostics-only path. - (nit) The test pulls the block out of vk_memory.cpp by matching the comment text and the next `#ifdef __linux__`. If someone rewords that comment, the test fails with a bare `ValueError`. A clearer error message there would help whoever hits it.

gpu/vk/vk_memory.cpp

					
				@ -358,3 +357,4 @@

				    if (page_watches || VirtualAlloc(ptr, (SIZE_T)bytes, MEM_COMMIT, PAGE_READWRITE) != ptr)

				        for (uint8_t* q = ptr; q < ptr + bytes;) {

				            MEMORY_BASIC_INFORMATION mi{};

				            if (VirtualQuery(q, &mi, sizeof mi) != sizeof mi) break;