Repository navigation
feat(libkernel): initialize created guest thread MXCSR to 0x9fc0 at entry boundary - #7565
pushkarreddyy wants to merge 3 commits into
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
One test coverage gap: hostMxcsrBefore and hostMxcsrAfter are sampled on the test process main thread, while scePthreadCreate runs Aps5CallGuestEntry_nid_no_patch on a separate native worker. That assertion would remain unchanged even if the wrapper failed to restore the worker’s MXCSR after a returning guest callback. The explicit restore instruction is present in the diff, but the PR’s stated host-preservation test does not exercise it. Could you add a same-worker post-callback check if practical, or narrow the Tested claim to what the current test observes?
|
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.
|
|
Two corrections about what #7548 covers, since this PR cites it:
|
|
The new #7548 report follow-up confirms a scope gap beyond my earlier test comment: the console main thread also reads MXCSR |
|
Ran this on macOS (x86-64 under Rosetta 2), head Machine: Apple M2 Max, macOS 27.2, Apple clang 21, full x86-64 build. Base:
It passes on the same tree without the PR. Keeping the MXCSR values in their own slot below the record fixes it: mov %rsp, %rbp
+ sub $16, %rsp
+ .cfi_adjust_cfa_offset 16
stmxcsr (%rsp)
movl $0x9fc0, 4(%rsp)
ldmxcsr 4(%rsp)
mov %rdi, %rax
mov %rsi, %rdi
call *%rax
ldmxcsr (%rsp)
- add $16, %rsp
- .cfi_adjust_cfa_offset -16
+ add $32, %rsp
+ .cfi_adjust_cfa_offset -32With that change, Other results:
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine. |
pushkarreddyy
changed the title
|
Pushed commit
|
|
One remaining state-consistency issue from the new destructor probe: when the guest entry returns normally, |
|
Update on the main-entry gap I noted earlier: I opened #7594 for the Linux and Windows relinker startup stubs. It sets guest MXCSR to 0x9fc0 at main ELF entry and restores the host MXCSR if that entry returns. This is separate from the created-thread trampoline covered here; macOS’s distinct launcher path remains outside #7594. |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Re-ran on macOS (x86-64 under Rosetta 2), head 251ef0f28. Linux and Windows CI are still skipped on this PR.
Same setup as my earlier run: Apple M2 Max, macOS 27.2, Apple clang 21, full x86-64 build. Base: main d3d8d79 with @mugurc's macOS series and the open macOS fixes it uses.
guest_pthread_frame_chainnow passes (5/5), so the zero record is intact.guest_pthread_startpasses (5/5). The new destructor check covers the restore. With only theldmxcsr (%rsp)after the call removed, it fails every run withWorker thread host MXCSR was not restored upon guest return. With only the test applied on the base it still fails withGuest thread MXCSR does not have FTZ and DAZ bits set.- Non-GPU suite: same failures with and without the PR, all of them macOS gaps that the series does not cover yet. The 51
pthread/thread/raise_exceptiontests pass on both.
This does not address @MarcZX10's last point: on a normal return the key destructors still run under the host MXCSR. The new check asserts that state.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Pushed commit
|
|
@jotitadev Please note repository review etiquette: formal GitHub Reviews (Submit Review / Review Decisions) are reserved exclusively for AnyPS5 repository maintainers with merge permissions. If you have local reproduction data or technical observations to share, post them strictly as standard PR discussion comments rather than submitting formal reviews. Regarding the technical point: that state-consistency issue was already addressed in commit Specifically:
|
|
The stubs look right. One question before merge: with the main thread (here) and created guest threads (#7565) at 0x9fc0, host code called from the guest, and host threads created from a guest thread on Linux (which inherit MXCSR), will run with FTZ/DAZ. Which host work runs in that context, especially the shader recompiler's constant folding and the driver workers? Should host-owned worker threads set the host default MXCSR when they start, so console FP semantics stay on guest code? |
|
@oneandonlydean Thanks for the question! Here is an analysis of what host work executes in that context and how host threads are affected:
Also, whenever you have a moment, could you please take a look at two open PRs that are cleanly rebased on latest
Thanks! |
|
The host-worker boundary you asked about is now covered in follow-up #7622. Its helper resets MXCSR to 0x1f80 at native std::thread entry across runtime, AGC, shader cache, AvPlayer and libkernel workers, and at the shader-registration pthread trampoline. The focused Linux regression verifies worker MXCSR 0x1f80 while the caller remains at 0x9fc0. This addresses worker inheritance; it does not claim to isolate synchronous host work that runs on a guest thread, which remains a separate boundary question. |
|
@MarcZX10 @oneandonlydean Thanks for putting together follow-up #7622! As noted in #7622's description and test suite, the host-worker MXCSR isolation specifically builds upon the guest caller environment remaining at Because #7565 provides the foundational guest execution state that #7622's tests verify caller inheritance against, having #7565 take precedence and merge first establishes the core guest baseline cleanly, allowing #7622 to land smoothly on top to handle host background worker defaults. |
|
At the current head (1f99bd9), the restore instruction is present after finishThread, but the committed host-preservation assertion still samples MXCSR only on the test process main thread. The worker dtor check runs before RunThread restores host MXCSR, so it verifies guest cleanup state rather than the worker state after guest return. The implementation looks straightforward; please either add a same-worker observation if practical or narrow the Tested claim to avoid saying this test verifies host preservation. |
|
Linux run of Machine: Linux x86_64, i7-8700, Ubuntu 26.04, GCC 15.2, Release,
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine. |
1f99bd9 to
8659e82
Compare
|
Pushed commit
|
f685cc3 to
bf99760
Compare
github-actions
Bot
added
the
review queue
b20fd5f to
24b3017
Compare
github-actions
Bot
added
review queue
24b3017 to
ac92c61
Compare
github-actions
Bot
added
review queue
ac92c61 to
7826de0
Compare
github-actions
Bot
added
review queue
7826de0 to
84b8fca
Compare
github-actions
Bot
removed
the
review queue
84b8fca to
e0a574c
Compare
github-actions
Bot
added
the
review queue
e0a574c to
c91c5f9
Compare
github-actions
Bot
added
review queue
Superseabas
left a comment
Superseabas
left a comment
There was a problem hiding this comment.
Windows run of the current head, c91c5f98f4, since the earlier reviews didn't build or run the Windows paths (_beginthreadex, _endthreadex in scePthreadExit, adoption in scePthreadSelf). They work here, and the test catches the loss of each restore that matters. One non-blocking suggestion for the adoption capture, which the test can't see on Windows.
Head merged onto main 492ed5c5fe (clean merge, local only). Windows 11 Home 10.0.26200, Ryzen 7 7700X, WinLibs GCC 16.2.0 (x86_64-ucrt-posix-seh, r3), Release, BUILD_TESTING=ON.
Tests. The 343 tests matching XairRrR's set (pthread|thread_self|thread_atexit|thread_callback|kernel_sys_thread|libc_threads|raise_exception|cancel|thread_local|tls|exception) pass 3 of 3 on main and on this PR. guest_pthread_start and guest_pthread_frame_chain pass 5 of 5. The non-GPU suite (ctest -E '^agc|gpu|vulkan|video_out' -j 8, 2,886 tests) gives 2,883 on both trees. The same three fail on main here: windows_crash_thread_name (#15578), plus test_registration and select_tests_MissingCommandTests, which need the Windows symlink privilege.
Red checks on guest_pthread_start, each a local edit of the merged tree, 3 runs:
| Edit | Result |
|---|---|
| none | passes |
Thread.cpp from main (the PR's header kept) |
fails: Guest thread MXCSR does not have FTZ and DAZ bits set |
the restore in scePthreadExit removed |
fails: Worker thread host MXCSR was not restored upon guest return |
the restore in RunThread removed |
passes |
the restore in the Windows StartNativeThread removed |
passes |
| both of those removed | fails: the same message |
the capture in the Windows StartNativeThread removed |
passes |
the capture in the Windows scePthreadSelf adoption removed |
passes |
On Windows, as on Linux, the restores in RunThread and StartNativeThread cover each other on a normal return; only the one in scePthreadExit runs before _endthreadex.
XairRrR's second point, on Windows. I set workerMxcsrInDtor and workerMxcsrAfterReturn to 0 before each scePthreadCreate and before nativeWorker starts. The test still passes 3 of 3, and it still fails without the scePthreadExit restore or without both normal-return restores. So each check, the adopted-thread case included, reads its own thread's value here, and the TLS hook runs on threads that end through _endthreadex. With a print added to the guest-MXCSR creator case, the main thread reads 0x1f80, and a worker created while its creator held 0x9fc0 reads 0x1f80 coming in and 0x1f80 after it returns. Windows threads don't inherit the creator's MXCSR.
Non-blocking. On Windows the two captures can't be told apart from the 0x1f80 default, since every thread starts there, so removing either one passes. For StartNativeThread that's harmless. For adoption, a native thread can hold any MXCSR before calling scePthreadSelf, and setting one in the test covers the capture:
std::thread nativeWorker([&] {
workerTlsHook.active = true;
const std::uint32_t adoptedMxcsr = 0x1fc0;
asm volatile("ldmxcsr %0" : : "m"(adoptedMxcsr));
Pthread adoptedSelf = scePthreadSelf();with the check after nativeWorker.join() comparing against 0x1fc0 instead of hostMxcsrBefore. I tried it locally: it passes 3 of 3 on this head and fails 3 of 3 (Adopted thread host MXCSR was corrupted upon scePthreadExit) with the Windows adoption capture removed. Nothing in it is Windows-specific, so it should cover the POSIX adoption capture too, which I didn't run.
check_conventions.py --base main passes. No title run, and #7594 and #7622 aren't on this tree.
AI-assisted: yes (Claude Code)
…oss guest execution
c91c5f9 to
db410ee
Compare
|
Thank you @Superseabas for testing the Windows execution paths and for the adoption coverage suggestion! I have rebased cleanly onto current |
github-actions
Bot
removed
the
review queue
Superseabas
left a comment
Superseabas
left a comment
There was a problem hiding this comment.
Re-checked the new head, db410ee8f6, on Windows. Thanks for adding the adoption case. git range-diff against c91c5f98f4, which I approved, shows your two earlier commits unchanged; the new commit only adds the 0x1fc0 adoption check in PthreadStart.cpp. It passes here and now catches the missing adoption capture on Windows.
Head merged onto main 9f2b939606 (clean merge, local only). Windows 11 Home 10.0.26200, Ryzen 7 7700X, WinLibs GCC 16.2.0 (x86_64-ucrt-posix-seh, r3), Release.
- The 344 tests matching
pthread|thread_self|thread_atexit|thread_callback|kernel_sys_thread|libc_threads|raise_exception|cancel|thread_local|tls|exceptionpass 3 of 3.guest_pthread_startandguest_pthread_frame_chainpass 5 of 5. - Red checks on
guest_pthread_start, 3 runs each:
| Edit | Result |
|---|---|
the capture in the Windows scePthreadSelf adoption removed |
fails: Adopted thread host MXCSR was corrupted upon scePthreadExit (passed on the previous head) |
Thread.cpp from main |
fails: Guest thread MXCSR does not have FTZ and DAZ bits set |
the restore in scePthreadExit removed |
fails: Worker thread host MXCSR was not restored upon guest return |
the restores in RunThread and the Windows StartNativeThread removed |
fails: the same message |
the capture in the Windows StartNativeThread removed |
passes, as before: every Windows thread starts at 0x1f80 |
check_conventions.py --base main passes.
AI-assisted: yes (Claude Code)
github-actions
Bot
added
the
review queue
What
Measured on PS5 firmware 12.70: threads of a PS5 payload process start with MXCSR
0x9fc0(Flush-to-Zero and Denormals-are-Zero enabled, all FP exceptions masked).In
Aps5CallGuestEntry_nid_no_patch, created guest threads now set0x9fc0upon entering guest execution. The 16-byte MXCSR scratch slot is allocated below%rbp(sub $16, %rsp/add $32, %rsp), preserving the sentinel frame-pointer zero record forguest_pthread_frame_chain. InRunThreadandscePthreadExit, host worker MXCSR is recorded and restored only afterfinishThreadhas completed, ensuring that guest thread destructors and TSD key destructors execute symmetrically under guest MXCSR0x9fc0across both explicit exit and normal return paths, while host native workers return to host MXCSR upon thread completion. Note: the main ELF thread entry stub is handled separately in #7594 and is not covered here.Tested
Linux x86_64 and Windows:
core/libs/tests/PthreadStart.cppexercises 10,000 guest thread starts across normal return andscePthreadExitexplicit exits. Tests verify that guest execution and cleanup (WorkerThreadDtor) observe0x9fc0, the native worker thread (WorkerTlsHookTLS destructor) restores host MXCSR upon guest completion, and the process main thread preserves its host MXCSR throughout;python tools/check_conventions.py --base origin/mainpasses with 0 findings.Checklist