arch-riscv: Add H-mode L1 TLB compression - #1006
Open
Gekyume777 wants to merge 4 commits into
Open
Gekyume777 wants to merge 4 commits into
Gekyume777 wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change extends RISC-V L1 compression to direct, VS-stage, and G-stage translations. It updates compressed-entry lookup, merging, synthesized PTE handling, walker refill, two-stage next-line walks, checkpoint restoration, and LSQ counter diagnostics. ChangesCompressed TLB and page-walk handling
O3 LSQ request diagnostics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PageTableWalker
participant L2TLB
participant GstagePageTable
PageTableWalker->>L2TLB: insert returned Gstage line
PageTableWalker->>GstagePageTable: issue Gstage L0 read
GstagePageTable-->>PageTableWalker: return nextline-only response
PageTableWalker->>L2TLB: populate nextline entries
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/arch/riscv/tlb.cc (1)
1453-1541: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve each surviving sub-page when reinserting partially overlapped L1-compressed blocks.
When
prepareL1CompressedInsert()removes the incoming entry’s bits from an existing multi-sub-page L1-compressed entry,validIdxcan still have more than one bit set, butreinsert_narrow()collapses it tofirstValidIdx()only. The narrowed entry is re-keyed under one page and the remaining sub-pages are left masked out, making pages that are still valid invalidIdxunreachable by trie lookup orlookupL1CompressedFallback()until a miss re-walks the page table. Preserve each remaining sub-page in a separate narrow insertion instead of keeping them masked in the same TLB entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/arch/riscv/tlb.cc` around lines 1453 - 1541, Update reinsert_narrow and its caller in prepareL1CompressedInsert to preserve every set bit remaining in validIdx after masking the incoming entry. Create or reinsert one narrow TLB entry per surviving sub-page, with each entry keyed to that sub-page’s virtual address and corresponding pteIdx bit, rather than collapsing to firstValidIdx; ensure trie entries and narrow-insert statistics remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/arch/riscv/pagetable_walker.cc`:
- Around line 735-768: Prevent L1 refill entries from being silently dropped
when compression construction fails. In src/arch/riscv/pagetable_walker.cc lines
735-768, update each direct, VS-stage, and G-stage branch in dol2TLBHit() with a
fallback or panic consistent with TLB::insert()'s invariant; apply the same
fallback/panic to twoStageStepWalk() at lines 1059-1081 and twoStageWalk() at
lines 1288-1311 when compression is enabled but buildL1CompressedEntry or
buildSingleL1CompressedEntry fails.
---
Outside diff comments:
In `@src/arch/riscv/tlb.cc`:
- Around line 1453-1541: Update reinsert_narrow and its caller in
prepareL1CompressedInsert to preserve every set bit remaining in validIdx after
masking the incoming entry. Create or reinsert one narrow TLB entry per
surviving sub-page, with each entry keyed to that sub-page’s virtual address and
corresponding pteIdx bit, rather than collapsing to firstValidIdx; ensure trie
entries and narrow-insert statistics remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a404513f-9133-4c6d-940f-db53541f6ec4
📒 Files selected for processing (9)
src/arch/riscv/isa.ccsrc/arch/riscv/pagetable_walker.ccsrc/arch/riscv/pagetable_walker.hhsrc/arch/riscv/regs/misc.hhsrc/arch/riscv/tlb.ccsrc/arch/riscv/tlb.hhsrc/cpu/base.ccsrc/cpu/o3/fetch.ccsrc/cpu/o3/lsq.cc
Gekyume777
force-pushed
the
pr-h-debug-fixes
branch
from
July 31, 2026 08:22
3f45a03 to
8777746
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/arch/riscv/tlb.cc (1)
1038-1044: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInvalidate compressed VS-stage entries with normalized virtual addresses.
Line 1039 limits normalized matching to
direct. Compressed VS-stage entries now storetlbKeyComparableVaddr()-normalized bases, but the generic comparison uses the sign-extendedvpn. An invalidation for a high-half guest virtual address can therefore leave its VS-stage compressed entry resident.Apply the normalized comparison for
vsstagecompressed entries. Add a regression test that inserts a compressed VS-stage entry for a sign-extended address, performs the matching invalidation, and verifies a miss.Proposed fix
- if (entry.isCompressed && entry.translateMode == direct) { + if (entry.isCompressed && + (entry.translateMode == direct || entry.translateMode == vsstage)) {Also applies to: 1062-1078
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/arch/riscv/tlb.cc` around lines 1038 - 1044, Update the vaddrMatches lambda and the corresponding invalidation logic around it to use tlbKeyComparablePageBase() for compressed entries in both direct and vsstage translate modes, ensuring sign-extended high-half guest addresses match their normalized stored bases. Add a regression test that inserts a compressed VS-stage entry with a sign-extended address, invalidates that address, and verifies the subsequent lookup misses.
🧹 Nitpick comments (1)
src/arch/riscv/tlb.cc (1)
85-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse lower_snake_case for the new functions and methods.
Rename
tlbKeyComparableVaddr,tlbKeyComparablePageBase,l1CompressedBlockBase,isL1CompressionMode,l1CompressionContextId,sameL1CompressionContext,currentMemPriv,getMemPriv,setOldPriv, anduseNewPriv. Update their declarations and call sites in the same change.Also applies to: 2829-2878
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/arch/riscv/tlb.cc` around lines 85 - 124, Rename the listed helper functions and methods to lower_snake_case: tlb_key_comparable_vaddr, tlb_key_comparable_page_base, l1_compressed_block_base, is_l1_compression_mode, l1_compression_context_id, same_l1_compression_context, current_mem_priv, get_mem_priv, set_old_priv, and use_new_priv. Update every declaration, definition, and call site consistently, including the related code around the additional referenced range.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/arch/riscv/tlb.cc`:
- Around line 1038-1044: Update the vaddrMatches lambda and the corresponding
invalidation logic around it to use tlbKeyComparablePageBase() for compressed
entries in both direct and vsstage translate modes, ensuring sign-extended
high-half guest addresses match their normalized stored bases. Add a regression
test that inserts a compressed VS-stage entry with a sign-extended address,
invalidates that address, and verifies the subsequent lookup misses.
---
Nitpick comments:
In `@src/arch/riscv/tlb.cc`:
- Around line 85-124: Rename the listed helper functions and methods to
lower_snake_case: tlb_key_comparable_vaddr, tlb_key_comparable_page_base,
l1_compressed_block_base, is_l1_compression_mode, l1_compression_context_id,
same_l1_compression_context, current_mem_priv, get_mem_priv, set_old_priv, and
use_new_priv. Update every declaration, definition, and call site consistently,
including the related code around the additional referenced range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60cfc732-abcf-4c7d-8091-e660642415af
📒 Files selected for processing (5)
src/arch/riscv/pagetable_walker.ccsrc/arch/riscv/pagetable_walker.hhsrc/arch/riscv/tlb.ccsrc/arch/riscv/tlb.hhsrc/cpu/o3/lsq.cc
🚧 Files skipped from review as they are similar to previous changes (4)
- src/cpu/o3/lsq.cc
- src/arch/riscv/pagetable_walker.hh
- src/arch/riscv/tlb.hh
- src/arch/riscv/pagetable_walker.cc
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/arch/riscv/pagetable_walker.hh (1)
261-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse lower_snake_case for the new private methods.
src/arch/riscv/pagetable_walker.hh#L261-L264: rename the declarations totwo_stage_nextline_walkandstart_two_stage_nextline.src/arch/riscv/pagetable_walker.cc#L912-L913: rename thetwoStageNextlineWalkdefinition.src/arch/riscv/pagetable_walker.cc#L941-L944: rename thestartTwoStageNextlinedefinition.src/arch/riscv/pagetable_walker.cc#L1244-L1258: update bothstartTwoStageNextlinecall sites.src/arch/riscv/pagetable_walker.cc#L2415-L2418: update thetwoStageNextlineWalkcall site.As per coding guidelines, functions and methods should use lower_snake_case naming convention.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/arch/riscv/pagetable_walker.hh` around lines 261 - 264, Rename the private methods Fault twoStageNextlineWalk and startTwoStageNextline to two_stage_nextline_walk and start_two_stage_nextline throughout the declarations in src/arch/riscv/pagetable_walker.hh (261-264), definitions in src/arch/riscv/pagetable_walker.cc (912-913 and 941-944), and all call sites (1244-1258 and 2415-2418), preserving their behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/arch/riscv/pagetable_walker.cc`:
- Around line 918-930: The L2 hit path in twoStageNextlineWalk must revalidate
prefetched G-stage entries before using them as the final GPADDR. Before
returning or scheduling a walk for an L2 hit, apply the same pma->check() and
pmp->pmpCheck() validation used by the normal two-stage miss path across the
cached entry range, and only retain the hit when validation succeeds.
---
Nitpick comments:
In `@src/arch/riscv/pagetable_walker.hh`:
- Around line 261-264: Rename the private methods Fault twoStageNextlineWalk and
startTwoStageNextline to two_stage_nextline_walk and start_two_stage_nextline
throughout the declarations in src/arch/riscv/pagetable_walker.hh (261-264),
definitions in src/arch/riscv/pagetable_walker.cc (912-913 and 941-944), and all
call sites (1244-1258 and 2415-2418), preserving their behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ef71c40-11f6-41ea-8955-4b46bbf062bc
📒 Files selected for processing (2)
src/arch/riscv/pagetable_walker.ccsrc/arch/riscv/pagetable_walker.hh
Comment on lines
+918
to
+930
| for (int i = 0; i < l2tlbLineSize; i++) { | ||
| nextlineEntry.gpaddr = | ||
| (((nextlineBasicGpaddr >> (PageShift + L2TLB_BLK_OFFSET)) | ||
| << L2TLB_BLK_OFFSET) + i) | ||
| << PageShift; | ||
| nextlineEntry.vaddr = nextlineEntry.gpaddr; | ||
| nextlineEntry.pte = read->getLE_l2tlb<uint64_t>(i); | ||
| nextlineEntry.paddr = nextlineEntry.pte.ppn; | ||
| nextlineEntry.index = | ||
| (nextlineEntry.gpaddr >> (PageShift + L2TLB_BLK_OFFSET)) & | ||
| walker->tlb->L2TLB_L0_MASK; | ||
| walker->tlb->L2TLBInsert(nextlineEntry.gpaddr, nextlineEntry, 0, | ||
| L_L2L0, i, false, gstage); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/arch/riscv/tlb.cc --items all --type function --match 'L2TLB|translate' || true
rg -n -C 10 --type=cpp \
'L2TLB(Insert|Lookup)|twoStage|pmpCheck|pma->check' \
src/arch/riscv/tlb.cc src/arch/riscv/pagetable_walker.ccRepository: OpenXiangShan/GEM5
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pagetable_walker relevant sections =="
sed -n '800,1225p' src/arch/riscv/pagetable_walker.cc
echo "== tlb G-stage L2 hit section =="
sed -n '2140,2370p' src/arch/riscv/tlb.cc
echo "== tlb doTwoStageTranslate section =="
sed -n '2389,2800p' src/arch/riscv/tlb.cc
echo "== targeted structural search for pma/pmp in page table read paths =="
python3 - <<'PY'
import pathlib, re
for path in [pathlib.Path('src/arch/riscv/tlb.cc'), pathlib.Path('src/arch/riscv/pagetable_walker.cc')]:
s=pathlib.Path(path).read_text()
print(f'-- {path} --')
for pat in ['pma->check', 'pmpCheck', 'TLBLookup', 'lookupGTLB']:
for m in re.finditer(re.escape(pat), s):
line=s.count('\n',0,m.start())+1
print(f'{path}:{line}:{pat}')
start=max(0,m.start()-600); end=min(len(s),m.start()+1200)
sec=s[start:end]
sec=s[sec.find('\n',-20):sec.find('\n',1000)]
print(sec[:1200])
print('='*80)
PYRepository: OpenXiangShan/GEM5
Length of output: 50374
Check G-stage L2 page-table reads before prefilled L2 entries.
twoStageNextlineWalk() prefetches adjacent G-stage L0 page-table entries and inserts them into L2TLB without pma->check() or pmp->pmpCheck(), while the normal two-stage step walk checks both for misses before non-hit L2 insertions. A later G-stage L2 hit can then hit these prefetched entries, so make the L2 hit path revalidate the cached entry range before returning or scheduling the walk as the final GPADDR.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/arch/riscv/pagetable_walker.cc` around lines 918 - 930, The L2 hit path
in twoStageNextlineWalk must revalidate prefetched G-stage entries before using
them as the final GPADDR. Before returning or scheduling a walk for an L2 hit,
apply the same pma->check() and pmp->pmpCheck() validation used by the normal
two-stage miss path across the cached entry range, and only retain the hit when
validation succeeds.
added 4 commits
August 10, 2026 10:14
Extend L1 TLB compressed-entry handling from one-stage direct translations to H-mode VS-stage and G-stage translations. All-stage entries remain uncompressed for this PR version. The refill paths now build compressed VS/G L1 entries from PTW leaf blocks when compression is enabled, and keep the old normal-entry behavior when compression is disabled. L2-hit delayed refill also converts VS/G entries before writing them back into L1. Compressed H-mode lookup reconstructs the selected sub-entry PTE before permission checks and address calculation, so the selected PPN low bits are used consistently. Delayed L2-hit state stores stable TlbEntry copies instead of raw TLB-entry pointers, and checkpoint restore rebuilds trie keys using the entry translation mode and ASID/VMID context. Temporary H-mode trace statistics used during profiling are intentionally removed from the PR diff. Validation: scons build/RISCV/gem5.opt --gold-linker -j64; H-mode omnetpp/540 smoke with --maxinsts=1000000 and FS0 NEMU ref passed.
Failing checkpoints: mcf/1068 and leslie3d/42136 fail with compression off; mcf/1068, mcf/5712, and mcf/5827 fail with compression on in the H-mode idealkmhv3 545-checkpoint run. Bug cause: The LSQ SingleDataRequest and SplitDataRequest constructors used a hard debug assertion that aborted the simulator when the number of live request objects exceeded 400. This counter tracks live gem5 LSQRequest objects, including self-owned requests that have already been detached from LSQ entries but are still waiting for translation, memory response, or writeback completion. It is not a modeled hardware capacity limit. Under H-mode idealkmhv3 with DRAMsim3 and 40M instruction windows, several valid checkpoints can temporarily exceed the old debug threshold and then make forward progress. The assertion therefore kills otherwise valid runs before stats are dumped. Solution: Keep the existing lifetime accounting unchanged, but replace the hard debug assertions with one-time warnings when the old threshold is crossed. This preserves observability for suspicious pressure while avoiding false aborts in long-latency H-mode workloads. The five representative failing checkpoint/config pairs were rerun after the fix and all completed 40M instructions with normal stats output.
Bug symptom: When an H-mode translation missed the all-stage L1 entry but then hit both the VS-stage and G-stage L1 entries, checkHL1Tlb() completed through doL2TLBHitSchedule() with a three-cycle delay. This path did not perform an L2 TLB lookup, yet it modeled the same delay as a distant L2 completion. Bug cause: The shared VS/G split-hit path reused the L2-hit completion scheduler to retain PMA/PMP checks and timing translation completion, but passed delaytick=3 even though all required translation state was already available in L1. The branch is shared by ITLB and DTLB. Fix: Keep the completion callback and change only its delay from three cycles to one cycle. This preserves PMA/PMP validation and translation->finish() ownership without modeling a nonexistent L2 access. Validation: Built build/RISCV/gem5.opt and ran the H-mode bzip2_program/6601 checkpoint for 1M instructions with DRAMsim3 and the FS0 NEMU reference; the run completed without difftest failures, panics, or fatal errors.
Failing checkpoint: bzip2_html/13707 is the representative failure. Bug symptom: H-mode configuration exposed open_nextline, but the two-stage walker cleared the nextline enable state during setup, so no H nextline request could run. After enabling the missing path, bzip2_html/13707 first aborted with "translateWithTLB missed after PTW" and then exposed a requestor-lifetime segmentation fault when the CPU translation was completed before the prefetch response. Bug cause: One-stage nextline reuses the current WalkerState and waits for the extra PTE-line response before ending the walk. That is not safe for two-stage translation: the G-stage L0 prefetch is independent of the completed HPA translation, and extending the original state either delays the CPU request long enough for unrelated TLB replacement to invalidate a concurrent walk's entry, or leaves a completed Translation pointer reachable by the state. Fix: Propagate the configured nextline controls through H setup. After a final G-stage L0 leaf, issue only the next physical G-stage PTE line within the same page-table page. Run that request in a standalone, non-coalescing WalkerState that reserves its own level-0 resource and only refills gstage L2L0 prefetch entries. The original H WalkerState follows its existing completion path and is never retained by the prefetch response. Validation: bzip2_html/13707 completed 1M instructions with H nextline enabled. The trace recorded H nextline issue and refill events, and the shared L2 TLB recorded 976 prefetch entries (122 PTE lines), with no panic, segfault, or difftest failure.
Gekyume777
force-pushed
the
pr-h-debug-fixes
branch
from
August 10, 2026 02:21
ee186e7 to
23e20c8
Compare
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
1 participant
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.
Motivation
Add L1 TLB compression support for H-mode VS-stage and G-stage translations.
Changes
three-cycle L2-like delay despite performing no L2 lookup; retain the
completion callback and reduce the delay to one cycle.
Summary by CodeRabbit
Performance
Reliability