Skip to content

fix(libc): alias non-contiguous shared guest views on Windows - #4367

Merged
oneandonlydean merged 2 commits into
boykopovar:mainfrom
MarcZX10:codex/carry-windows-alias-4260
Oct 13, 2026
Merged

oneandonlydean merged 2 commits into
boykopovar:mainfrom
MarcZX10:codex/carry-windows-alias-4260

Conversation

@MarcZX10

@MarcZX10 MarcZX10 commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

What

Carries Angel21PC's alias-run implementation from closed PR #2254, preserving the original commit author. It lets WindowsMappings::MapAlias compose one alias from adjacent shared pages even when they come from different sections or nonconsecutive section offsets. The existing single-view path remains for contiguous pages, and ranges containing private pages are still rejected.

This covers the Windows alias part of #4260. It does not add host imports across registered ranges; that separate change remains outside this PR.

Tested

  • Windows 11 Pro 26200, WinLibs GCC 15.2.0 r7, Release: AlxcBTW reports guest_memory passed on code head 5573e3238 with all three alias checks; windows_exceptions and fiber passed too. The non-GPU suite was 589/590, with log_macros failing the same way on main.
  • Sword of the Sea (PPSA25344) on main reaches a 368 MiB buffer spanning two direct-memory allocations; MapAlias refuses it on the current base, forcing CPU copies. The exact current-main rebased head has not been run on Windows or in the title yet.
  • Linux configure could not reach the build because SDL2 is uninitialized and PipeWire, PulseAudio, and ALSA development files are unavailable.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt
  • No notes, investigation .md files or images added to the repository
  • No third-party code added
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes

@AlxcBTW

AlxcBTW commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

@MarcZX100 here is the Windows validation this draft asks for, plus a title run that needs exactly this change.

Unit tests on Windows, on exactly this head (5573e32). Windows 11 Pro 26200, Ryzen 7 7730U, WinLibs GCC 15.2.0 r7, Release. guest_memory passes (1.49 s) with all three new checks: CheckAliasAcrossDirectAllocations, CheckAliasOfReorderedSharedPages and CheckAliasRefusesPagesThatAreNotSharedViews. windows_exceptions and fiber pass too. Of the non-GPU set (ctest -E "^agc_driver|^video_out|^guest_video"), 589 of 590 pass; the one failure, log_macros, fails the same way on main on that machine.

Sword of the Sea (PPSA25344). Its main menu binds a 368 MiB buffer across two adjacent direct memory ranges (320.75 MiB + 47.3 MiB). Each range is its own allocation, so the pair is views of two sections. An import that covers the buffer needs an alias across both, and on main MapAlias refuses that ("the range is not one contiguous run of views of a section"). As a result the buffer is copied on the CPU on almost every use. A before/after on that title with this PR follows in a separate comment once the rows have run.

Reading the change. I compared it with an alias-run version we had written for the same case. Both group the 16 KiB views into runs of one section's consecutive pages. Both map each run into a split placeholder, so no run needs 64 KiB alignment, and both refuse a range with a page that is not a shared view. Where they differ, yours is better:

  • On a failed map, releaseAlias releases whatever the reservation holds by querying it (views and the remaining placeholders), so the cleanup doesn't depend on bookkeeping.
  • UnmapAlias releases every run and forgets the alias even if one unmap fails. Ours stopped at the first failure and kept the entry.

Your tests cover what ours did: a run that starts at an offset into its section, two sections in one call, the views per run, the freed range after unmapping, and the refusal of a private page. So I'd drop ours.

One thing I'd add to the PR. The host-import entry in docs/dev/TechnicalDebt.md still says: "On Windows, a run of adjacent registered ranges whose shared views are not one contiguous run of views of a section gets no read-write alias, so its ranges are imported one at a time, as before runs were imported whole, and a buffer that crosses two of them is copied." With this change only a run that is not all shared views (a private range next to a shared one) gets no alias. Shared views of several sections, or of one section at offsets that do not follow on, now get one alias made of one view per run in a placeholder reservation.

AI-assisted: yes (Claude Code)

@xXPBSXx

xXPBSXx commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Windows test: head 5573e3238 merged onto current main ffa198407 (clean merge, no conflicts). Windows 10, MinGW GCC (WinLibs), Ninja Release, -j4: full build 0 errors; ctest with the filter guest_memory: guest_memory_pool_stats passed, guest_memory passed (2 test(s), 0 failed); python tools/check_conventions.py --base upstream/main passed. I did not run the new tests against main without the change.

AI-assisted (Claude Code).

@queeeef

queeeef commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Tested on Windows at this head (5573e3238).

I reviewed the change against a local equivalent I wrote for #4260 (one placeholder split into one view per run of section offsets). The two do the same thing, so I'm dropping mine in favour of this PR. In Palworld (PPSA23126) the local version made the 43 spans that failed at the alias import, and the CPU copies of the regions under them went from 2–56 GB per 10 s to 0. I'll run this PR together with oneandonlydean's up4/host-import-bind-join on that scene and report on #4260.

AI-assisted (Claude Code)

@oneandonlydean

Copy link
Copy Markdown
Collaborator

The Windows runs above cover what the draft was waiting for, so please mark it ready. One change first: the host-import entry in docs/dev/debt/libSceAgcDriver.md still says a run of adjacent registered ranges whose shared views are not one contiguous run of views of a section gets no read-write alias. With this, only a run with a page that is not a shared view is refused, so that sentence needs to say so.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 13, 2026
@MarcZX10

Copy link
Copy Markdown
Contributor Author

Thanks, that debt sentence is stale. The current limitation is that alias creation refuses a run if any page is not a shared view; non-contiguous section offsets alone are now supported. I have not changed the PR text or marked it ready yet. I will correct the debt entry and then mark the PR ready once that edit is on the branch.

Angel21PC and others added 2 commits October 13, 2026 07:31
On Windows, a host import of shared guest memory goes through a
read-write alias from WindowsMappings::MapAlias. The alias was a single
MapViewOfFile3 of one section, so MapAlias threw when the views of the
range were not one contiguous run of that section. One
sceKernelMapDirectMemory over two adjacent direct memory allocations
gives such a range, as each allocation has its own section, and so do
pages remapped in place to another physical order.

Such ranges are now aliased run by run: a placeholder reservation is
split at each run of consecutive pages of one section, and each run
maps that section at its offset. UnmapAlias unmaps every run and forgets
the alias even when one unmap fails. A contiguous range keeps the single
view, and a range with a page that is not a shared view is still
refused.

guest_memory maps two adjacent direct memory allocations of two pages
and one page with one call, and two pages in reverse physical order.
It reads and writes every page through the alias. The first test also
checks that the alias has one view per run and that unmapping it frees
its pages. A range whose second page is flexible memory is still
refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit fc6868d)
@MarcZX10
MarcZX10 force-pushed the codex/carry-windows-alias-4260 branch from 2ddb06a to c4a61ea Compare October 13, 2026 07:31
@MarcZX10
MarcZX10 marked this pull request as ready for review October 13, 2026 07:32
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 13, 2026
@MarcZX10

Copy link
Copy Markdown
Contributor Author

Addressed the debt wording in docs/dev/debt/libSceAgcDriver.md: aliases are refused when a range contains a page that is not a shared view, while fully shared ranges can be mapped across section runs. I rebased onto current main (resolving the cleanRanges/freshRanges additions alongside the alias-run helpers) and marked the PR ready. The Windows report remains for the code at 5573e3238; the rebased head has not had a new Windows runtime run, and the PR description states that explicitly.

@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

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.

Pull request Conflicting files (lines on main) Same exports Same new files
#4603 docs/dev/debt/libSceAgcDriver.md L70-71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants