Skip to content

fix(libkernel): query current guest mapping extents - #1731

Closed
praveenkumarappu wants to merge 1 commit into
boykopovar:mainfrom
praveenkumarappu:fix/virtual-query-split-ranges
Closed

praveenkumarappu wants to merge 1 commit into
boykopovar:mainfrom
praveenkumarappu:fix/virtual-query-split-ranges

Conversation

@praveenkumarappu

@praveenkumarappu praveenkumarappu commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Superseded by #1929. That PR includes this PR's exact live-extent fix and regression with the original author preserved, plus the correction and regression for mapped non-readable ranges. Closing this PR so the shared change is reviewed and merged once through #1929.


What

After protecting or partially unmapping a flexible allocation, sceKernelVirtualQuery still uses the original allocation address and size. This can return an extent crossing a protection boundary or an unmapped hole, and FIND_NEXT can select a surviving fragment by its former allocation address.

Use each registered range’s current address and size for containment, ordering, and returned extents. For a three-page flexible mapping whose middle page becomes read-only, queries now return each current one-page range with its own protection. After the middle page is unmapped, FIND_NEXT returns the third page.

This isolates range selection from a larger local experimental patch. No shader, memory-release, loader, or game configuration changes are included.

Tested

Windows x64, WinLibs MinGW-w64 GCC 15.2.0, Release/Ninja, rebased onto upstream main f145139676a48306681d3a4d5fabf83938759daf, PR head 1a301c48a74f879ac3136745056edd8b7bf04403:

  • Built libc and libkernel from a clean checkout with an external component-only CMake harness.
  • New regression fails on unchanged production code at the first split-range extent assertion; rebuilding with this production change passes.
  • Complete GuestMemory.cpp test executable passes with the fix, including the new regression.
  • Regression covers protection splits, surviving extents after partial unmap, FIND_NEXT from the hole, and preservation of neighboring contents.
  • python tools/check_conventions.py --base origin/main and git diff --check pass.

After rebasing, rebuilt libc/libkernel and reran the complete Windows GuestMemory.cpp executable, focused split-range regression, convention checks, and git diff --check; all passed.

Combined validation with dependent follow-up #1929: fork Build 37868880231 tested a tree containing this exact rebased PR tree plus #1929's non-readable mapping fix. Full builds and CTest passed on Linux (497/497) and Windows (488/488), with relinker CTests passing on Ubuntu, Windows, and macOS. Local Windows combined guest_memory, guest_ampr_markers, and query-probe checks also passed. These combined results include #1929; they do not establish that this isolated PR passes the full suite without that follow-up.

Ready for review. Upstream PR workflows require authorized maintainer approval; the fork Build above is separate evidence. Console behavior and gameplay using this isolated branch have not been tested.

Related work checked: #700 reports reservations, #686 handles released direct-memory bookkeeping, and #721 implements checked physical release. This change retains their current-main behavior and addresses existing flexible mapping fragments.

Checklist

  • Based on current main at preparation; searched related open PRs for overlap
  • One topic
  • No code comments added; removed an obsolete allocation-extent comment
  • No new silent stubs or import fallbacks
  • No investigation files, logs, screenshots, game binaries, or proprietary assets in the repository
  • No third-party code added
  • Generic behavior
  • Depends on: none
  • AI-assisted: yes

@MarcZX10

MarcZX10 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

The overlap with #1929 is resolved as a stacked follow-up: #1929 now depends on this PR, includes your live-range commit, and removes its duplicate range-selection logic. I found the first live-range implementation fails Linux guest_ampr_markers because the readable-only allocation lease omits mapped pages marked non-readable. The follow-up adds an all-range snapshot and a focused regression; locally, guest_memory and guest_ampr_markers pass together. Full fork CI for the stacked head is running.

@MarcZX10

MarcZX10 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

The stacked follow-up in #1929 passed its full fork Build on head 6db0682f0074a37b29a26a019b7a054c9a72f266: Linux CTest 497/497, Windows CTest 488/488, and relinker CTests on Ubuntu, Windows and macOS (run 37868880231). The PR remains explicitly dependent on #1731; the only unique addition is the all-range snapshot and its regression for mapped, non-readable pages.

@praveenkumarappu
praveenkumarappu marked this pull request as ready for review October 9, 2026 01:30
@praveenkumarappu

Copy link
Copy Markdown
Contributor Author

Thanks @MarcZX100 for resolving the overlap and adding the non-readable range fix. I also tested the combined head 6db0682f on Windows: guest_memory, guest_ampr_markers, and the query probe passed, including split ranges, no-access pages, and unmapped holes. I checked the successful fork CI run as well.

I've marked this PR ready for review. Please review it together with the dependent follow-up #1929.

@MarcZX10

MarcZX10 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Thanks for testing the combined head and making the PR ready for review. I reviewed your full diff together with dependent #1929; the combined code passes the full fork Build (Linux 497/497, Windows 488/488, all relinker jobs) and the focused tests. The remaining CONTRIBUTING item before approval is to update this branch to current upstream main: PR #1731 still has base commit 6b56175a, while upstream is f145139676a48306681d3a4d5fabf83938759daf. Please rebase and push the same change; I will recheck the updated head.

Use the current registered range after protection splits and partial unmaps, including FIND_NEXT lookups. Add a regression covering extents, protection and surviving contents.

AI-assisted: yes
@praveenkumarappu
praveenkumarappu force-pushed the fix/virtual-query-split-ranges branch from 58159dd to 1a301c4 Compare October 9, 2026 01:36

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the rebased head 1a301c48 against current upstream f1451396. The full diff remains limited to live-range selection in sceKernelVirtualQuery and its regression test. Conventions and git diff --check pass. Full Build 37868880231 passed on a tree containing this exact PR tree plus dependent #1929: Linux CTest 497/497, Windows CTest 488/488, and relinker CTests on Ubuntu, Windows, and macOS. The focused range tests and the AMPR protection regression pass. No blocking issue found.

@MarcZX10

MarcZX10 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

I approved the rebased head. One description update remains useful: the Tested section still says Linux and full-project CTest were not run. The combined fork Build 37868880231 used a tree containing the exact #1731 tree plus #1929 and passed Linux 497/497, Windows 488/488, and relinker CTests. Please update the PR description with that evidence. Upstream currently reports no checks for this fork head; CONTRIBUTING says fork workflows may need authorized maintainer approval.

@praveenkumarappu

Copy link
Copy Markdown
Contributor Author

Thanks @MarcZX100 for rechecking and approving the updated head. I've rebased onto upstream main (f1451396) and pushed the same change as 1a301c48. The Windows guest_memory executable, focused split-range regression, convention checks, and git diff --check passed. Thanks for the dependent fix in #1929 as well.

@MarcZX10

MarcZX10 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

I have now approved rebased head 1a301c48 after verifying it is based on current main and that its exact tree is present in the green stacked Build. #1929 has been rebased onto this exact commit; its unique follow-up remains separate and depends on this PR.

@MarcZX10

MarcZX10 commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

I attempted to update the Tested section with the rebase and combined Build evidence, but GitHub returned 404 because I cannot edit another contributor’s PR description. The PR description remains unchanged; please update it to reflect the rebase and the full combined fork Build. My review approval is on head 1a301c48.

@praveenkumarappu

Copy link
Copy Markdown
Contributor Author

Thanks @MarcZX100, I've now updated the PR description's Tested section with the rebase to f1451396, head 1a301c48, and the combined fork Build evidence: Linux 497/497, Windows 488/488, and relinker tests on all three platforms. It also makes clear that the full-suite results include dependent #1929. Thanks for reviewing and approving the changes.

@MarcZX10

MarcZX10 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Could maintainers consider #1929 as the next follow-up after #1731? It is explicitly based on this PR's current head and adds the separate non-readable-mapping case with regression coverage. Keeping the changes stacked preserves the partial-unmap fix here and makes #1929 available as a focused follow-up.

The approval on #1929 predates its current head, so that head still needs review and any required CI before merge.

@github-actions

github-actions Bot commented Oct 9, 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
#1929 core/libs/tests/GuestMemory.cpp L1064-1065

@MarcZX10

MarcZX10 commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Update after checking both live heads: #1929 is based on current upstream main 6cc0503d and includes the exact live-extent change from #1731 with its original commit author preserved. It also adds the all-range snapshot and regression for mapped PROT_NONE ranges, which fixes the guest_ampr_markers case that the readable-only lease misses. The focused tests pass (2/2).

#1731 still targets f1451396; #1929 is the better canonical PR because it contains the same live-extent fix plus the non-readable-range correction on current main. Its full Build is currently blocked on both Linux and Windows by the unrelated missing SPIRV-Headers include tracked in #2093; all relinker jobs pass. I recommend closing #1731 as superseded by #1929, rather than merging two PRs for the same sceKernelVirtualQuery change.

@MarcZX10

MarcZX10 commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Update: #1929 is now based on current upstream main 745b775e2d883c68560b2d87f4ab96a01e221ccf. It includes the live-extent change from this PR with the original commit author preserved, plus the distinct non-readable mapping fix and regression. The combined focused tests pass. #1929 is the canonical complete change; please close #1731 as superseded so the shared change merges once.

@praveenkumarappu

Copy link
Copy Markdown
Contributor Author

Thanks @MarcZX100. I compared our 1a301c48 commit with the preserved ff53c545 commit in #1929: both changed-file patches match exactly, and the original author is preserved. #1929 also includes the non-readable mapping correction and regression. I've updated this PR's description and am closing #1731 as superseded by #1929 so the shared fix is reviewed and merged once.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants