Fix ASID extraction causing TLB flush and update PS submodule to latest - #503
Open
esmakokten wants to merge 2 commits into
Open
esmakokten wants to merge 2 commits into
esmakokten wants to merge 2 commits into
Conversation
chal_pgtbl_update() extracts the ASID from pt->protdom, then hands it to chal_cached_pt_curr() and chal_cached_pt_update(), which applied PROTDOM_ASID() to it again. Every ASID below 16 collapsed onto slot 0 of tlb_asid_map, so the cached-pgtbl check missed on every component crossing and CR3_NO_FLUSH was never set: each sinv and sret flushed the TLB entries of the PCID being loaded. The helpers now take the ASID directly. tlb_asid_map also gets the slot for ASID NUM_ASID_MAX, which PROTDOM_ASID() can return and which the collapse had been masking. On bare metal (Xeon Platinum 8160) the ping_pong round trip drops from 1127 to 851 cycles, and from 1152 to 851 with sched_ping_pong. The flushes that remain are one cold miss per ASID plus reloads of a PCID with a different page table (the booter shares ASID 0), which are required. Nothing relied on the extra flushes for memory reuse: chal_flush_tlb() is empty on x86, and the quiescence checks gating retype, remap and kmem release fail closed. Revocation by unmap is weaker, though: a component whose page is unmapped keeps its stale translation on the local core until that ASID's cached page table changes. It was never enforced on remote cores (chal_remote_tlb_flush() is empty). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pinned ps (f7bcdbe) does not define PS_LOCK_INITIALIZER, which capmgr uses, so compositions with a capmgr (e.g. sched_ping_pong.toml) failed to build. master (ca50b3f) defines it; the rest of the update is comment fixes in ps_list.h and a standalone list example. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
TLB invalidation must be addressed to prevent stale translations and access violations.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes x86 ASID caching to avoid unnecessary TLB flushes and cover the full ASID range.
Changes:
- Passes extracted ASIDs directly to cache helpers.
- Expands the ASID map to include the maximum ASID.
- Critical issue: mapping changes lack required TLB invalidation before
CR3_NO_FLUSHreuse.
File summaries
| File | Summary |
|---|---|
src/platform/i386/chal/chal_proto.h |
Corrects ASID cache indexing and sizing; requires invalidation handling for modified mappings. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| */ | ||
| static inline pgtbl_t | ||
| chal_cached_pt_curr(prot_domain_t protdom) | ||
| chal_cached_pt_curr(u16_t asid) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of this Pull Request (PR)
Stops the TLB flush on every synchronous invocation.
chal_pgtbl_update()passed an already-extracted ASID tochal_cached_pt_curr()/chal_cached_pt_update(), which appliedPROTDOM_ASID()to it again. Every ASID below 16 collapsed onto slot 0 oftlb_asid_map, so the cached-pgtbl check missed on every component crossing andCR3_NO_FLUSHwas never set: eachsinvandsretflushed the TLB entries of the PCID being loaded. The helpers now take the ASID directly, andtlb_asid_mapgets the slot for ASIDNUM_ASID_MAX, which the collapse was masking.pong_callround trip on bare metal (Xeon Platinum 8160, min of 64 × 1024-call samples): 1127 → 851 cycles withping_pong.toml, 1152 → 851 withsched_ping_pong.toml.For reviewers: nothing relied on the extra flushes for memory reuse.
chal_flush_tlb()is empty on x86_64, and the quiescence checks gating retype, remap and kmem release fail closed. Revocation by unmap is weaker, though: an unmapped page's stale translation now survives on the local core until that ASID's cached page table changes. It was never enforced on remote cores (chal_remote_tlb_flush()is empty).Intent for your PR
Choose one (Mandatory):
Reviewers (Mandatory):
@gparmer
Code Quality
As part of this pull request, I've considered the following:
Style:
Code Craftsmanship:
Testing
I've tested the code using the following test programs (provide list here):
sched_ping_pong.toml, QEMU and bare metal