Conversation
- Hypervisor Loads, Stores, Fences, etc. - Small change to enable H-extension in RiscvISA.py by default Change-Id: I033ea064b904dfcb6bcff2a2dc0424113e47e245
Change-Id: I137147dafbebe700213f9f1469a5f335a9062c43
|
🚀 Performance test triggered: spec06-0.8c |
📝 WalkthroughWalkthroughThis PR adds hypervisor-focused load/store and fence instructions, hypervisor-aware memory flags and templates, MIDELEG CSR read handling, new memflags definitions, and updates TLB/page-table walker permission logic to propagate HLVX/FORCE_VIRT semantics. Changes
Sequence DiagramsequenceDiagram
participant Decoder
participant Exec as InstExecutor
participant TLB
participant PTW as PageTableWalker
participant Memory
Decoder->>Exec: Decode HLV/HSV, set mem_flags (FORCE_VIRT/HLVX)
Exec->>TLB: Translate request (flags in Request)
activate TLB
TLB->>TLB: Extract hlvx / FORCE_VIRT from Request flags
alt Two-stage (G+VS)
TLB->>PTW: Start G-stage walk (hlvx)
activate PTW
PTW->>PTW: Validate PTE bits, check permissions (hlvx-aware)
PTW-->>TLB: G-stage result
deactivate PTW
TLB->>PTW: Start VS-stage walk (hlvx)
activate PTW
PTW->>PTW: Validate PTE bits, check permissions (hlvx-aware)
PTW-->>TLB: VS-stage result
deactivate PTW
else Single-stage
TLB->>PTW: Start leaf walk (hlvx)
activate PTW
PTW->>PTW: Validate PTE, check permissions (hlvx-aware)
PTW-->>TLB: Translation result
deactivate PTW
end
deactivate TLB
Exec->>Memory: Access using translated address
Memory-->>Exec: Return data / fault
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI agents
In `@src/arch/riscv/isa/decoder.isa`:
- Around line 2153-2165: The decoder currently maps sinval_vvma, sfence_w_inval,
and sfence_inval_ir to no-op stubs that only warn (leaving stale translations);
change these paths to fail-fast until proper invalidation is implemented by
replacing the warn-only handlers with a hard trap/unsupported-instruction error
(or explicit throw/terminate) so execution cannot continue with stale state;
locate the cases for sinval_vvma, sfence_w_inval, and sfence_inval_ir in the
decoder and update their bodies to return/raise an unrecoverable error (or call
the existing unsupported/op-fail helper) instead of just logging a warning.
- Around line 2432-2434: The store in HyperStore::hsv_h has a missing semicolon
after the assignment "Mem_uh = Rs2_uh" which breaks generated C++; update the
hsv_h store block to terminate the assignment with a semicolon (i.e., change
"Mem_uh = Rs2_uh" to "Mem_uh = Rs2_uh;") and keep the surrounding mem_flags
array intact so the decoder.isa block for HyperStore::hsv_h remains
syntactically valid.
In `@src/arch/riscv/tlb.cc`:
- Around line 1420-1424: The code is attempting to assign into a temporary
std::pair; replace that with a proper structured binding or two-variable
assignment from the return of checkGPermissions. Call checkGPermissions(status,
vaddr, fault_gpaddr, mode, e[0]->pte, (req->getFlags() & (Request::ARCH_BITS &
XlateFlags::HLVX)) || req->get_h_inst()) and capture its result into the
existing variables continuePtw and fault (e.g. use auto [continuePtw, fault] =
... if they are being declared here, or assign the returned pair to a temporary
std::pair and then set continuePtw = tmp.first; fault = tmp.second; if they are
already declared). Ensure you reference the function checkGPermissions and the
flags expression exactly as in the diff.
| 0xb: sinval_vvma({{ | ||
| warn("sinval_vvma not implemented"); | ||
| }}, IsNonSpeculative, IsSerializeAfter, No_OpClass); | ||
|
|
||
| 0xc: decode RS2{ | ||
| 0x0: sfence_w_inval({{ | ||
| warn("sfence_w_inval not implemented"); | ||
| }}, IsNonSpeculative, IsSerializeAfter, No_OpClass); | ||
|
|
||
| 0x1: sfence_inval_ir({{ | ||
| warn("sfence_inval_ir not implemented"); | ||
| }}, IsNonSpeculative, IsSerializeAfter, No_OpClass); | ||
| } |
There was a problem hiding this comment.
Avoid no-op invalidation instructions.
These paths only warn and continue, which can leave stale translations and break guest behavior. Either implement the invalidation or fail fast until it’s supported.
🛠️ Suggested fail-fast behavior until invalidation is implemented
- warn("sinval_vvma not implemented");
+ return std::make_shared<IllegalInstFault>(
+ "sinval_vvma not implemented", machInst);
...
- warn("sfence_w_inval not implemented");
+ return std::make_shared<IllegalInstFault>(
+ "sfence_w_inval not implemented", machInst);
...
- warn("sfence_inval_ir not implemented");
+ return std::make_shared<IllegalInstFault>(
+ "sfence_inval_ir not implemented", machInst);
...
- warn("hinval_vvma not implemented");
+ return std::make_shared<IllegalInstFault>(
+ "hinval_vvma not implemented", machInst);
...
- warn("hinval_gvma unimplemented");
+ return std::make_shared<IllegalInstFault>(
+ "hinval_gvma not implemented", machInst);Also applies to: 2189-2191, 2242-2244
🤖 Prompt for AI Agents
In `@src/arch/riscv/isa/decoder.isa` around lines 2153 - 2165, The decoder
currently maps sinval_vvma, sfence_w_inval, and sfence_inval_ir to no-op stubs
that only warn (leaving stale translations); change these paths to fail-fast
until proper invalidation is implemented by replacing the warn-only handlers
with a hard trap/unsupported-instruction error (or explicit throw/terminate) so
execution cannot continue with stale state; locate the cases for sinval_vvma,
sfence_w_inval, and sfence_inval_ir in the decoder and update their bodies to
return/raise an unrecoverable error (or call the existing unsupported/op-fail
helper) instead of just logging a warning.
| 0x33: HyperStore::hsv_h({{ | ||
| Mem_uh = Rs2_uh | ||
| }}, mem_flags=[ |
There was a problem hiding this comment.
Fix missing semicolon in hsv_h store.
This will break generated C++.
🐛 Proposed fix
- Mem_uh = Rs2_uh
+ Mem_uh = Rs2_uh;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 0x33: HyperStore::hsv_h({{ | |
| Mem_uh = Rs2_uh | |
| }}, mem_flags=[ | |
| 0x33: HyperStore::hsv_h({{ | |
| Mem_uh = Rs2_uh; | |
| }}, mem_flags=[ |
🤖 Prompt for AI Agents
In `@src/arch/riscv/isa/decoder.isa` around lines 2432 - 2434, The store in
HyperStore::hsv_h has a missing semicolon after the assignment "Mem_uh = Rs2_uh"
which breaks generated C++; update the hsv_h store block to terminate the
assignment with a semicolon (i.e., change "Mem_uh = Rs2_uh" to "Mem_uh =
Rs2_uh;") and keep the surrounding mem_flags array intact so the decoder.isa
block for HyperStore::hsv_h remains syntactically valid.
src/arch/riscv/tlb.cc
Outdated
| std::pair(continuePtw,fault) = checkGPermissions(status, vaddr, | ||
| fault_gpaddr, mode, e[0]->pte, | ||
| (req->getFlags() & | ||
| (Request::ARCH_BITS & XlateFlags::HLVX)) || | ||
| req->get_h_inst()); |
There was a problem hiding this comment.
Fix invalid std::pair assignment (won’t compile).
Line 1420 constructs a temporary pair and assigns to it; this is invalid C++.
🛠️ Proposed fix
- std::pair(continuePtw,fault) = checkGPermissions(status, vaddr,
- fault_gpaddr, mode, e[0]->pte,
- (req->getFlags() &
- (Request::ARCH_BITS & XlateFlags::HLVX)) ||
- req->get_h_inst());
+ auto check_res = checkGPermissions(status, vaddr,
+ fault_gpaddr, mode, e[0]->pte,
+ (req->getFlags() &
+ (Request::ARCH_BITS & XlateFlags::HLVX)) ||
+ req->get_h_inst());
+ continuePtw = check_res.first;
+ fault = check_res.second;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::pair(continuePtw,fault) = checkGPermissions(status, vaddr, | |
| fault_gpaddr, mode, e[0]->pte, | |
| (req->getFlags() & | |
| (Request::ARCH_BITS & XlateFlags::HLVX)) || | |
| req->get_h_inst()); | |
| auto check_res = checkGPermissions(status, vaddr, | |
| fault_gpaddr, mode, e[0]->pte, | |
| (req->getFlags() & | |
| (Request::ARCH_BITS & XlateFlags::HLVX)) || | |
| req->get_h_inst()); | |
| continuePtw = check_res.first; | |
| fault = check_res.second; |
🤖 Prompt for AI Agents
In `@src/arch/riscv/tlb.cc` around lines 1420 - 1424, The code is attempting to
assign into a temporary std::pair; replace that with a proper structured binding
or two-variable assignment from the return of checkGPermissions. Call
checkGPermissions(status, vaddr, fault_gpaddr, mode, e[0]->pte, (req->getFlags()
& (Request::ARCH_BITS & XlateFlags::HLVX)) || req->get_h_inst()) and capture its
result into the existing variables continuePtw and fault (e.g. use auto
[continuePtw, fault] = ... if they are being declared here, or assign the
returned pair to a temporary std::pair and then set continuePtw = tmp.first;
fault = tmp.second; if they are already declared). Ensure you reference the
function checkGPermissions and the flags expression exactly as in the diff.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c403cbb4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 0xb: sinval_vvma({{ | ||
| warn("sinval_vvma not implemented"); | ||
| }}, IsNonSpeculative, IsSerializeAfter, No_OpClass); |
There was a problem hiding this comment.
When a guest/OS uses sinval.vvma after updating page tables, this implementation only logs a warning and returns NoFault, so no TLB/translation invalidation occurs and no illegal-instruction trap is raised. That means stale translations can be reused, leading to accesses using old mappings or permissions after address-space changes (e.g., context switches or unmaps). If the invalidation isn’t implemented yet, this should at least raise an illegal instruction so software doesn’t continue with silently incorrect translations.
Useful? React with 👍 / 👎.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Change-Id: Icee9bfd8ad9869e0a6fdf72306aac87f5f36c32f
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/arch/riscv/tlb.cc`:
- Around line 1152-1163: The read-permission check no longer respects status.mxr
so execute-only pages are treated as unreadable; update the permission logic in
the TLB path by making exec_like consider MXR for read accesses (i.e., treat a
Read as exec_like when status.mxr is set), then keep the existing checks
(reservedPteBitsFault, createPagefault, and the pte.x / pte.r / pte.w branches)
unchanged so execute-only pages become readable under MXR; locate and modify the
definition of exec_like in this block (and/or where checkPermissions is used) to
incorporate status.mxr for BaseMMU::Read.
🧹 Nitpick comments (1)
src/arch/riscv/pagetable_walker.cc (1)
58-102: Optional: de-duplicate PBMTE helper with TLB.
pbmtEnabledmirrors the helper insrc/arch/riscv/tlb.cc; consider centralizing it to avoid drift.
| const bool exec_like = (mode == BaseMMU::Execute) || | ||
| (mode == BaseMMU::Read && hlvx); | ||
|
|
||
| if (reservedPteBitsFault(tc, pte)) | ||
| return createPagefault(vaddr, gpaddr, mode, G); | ||
|
|
||
| if (exec_like && !pte.x) { | ||
| fault = createPagefault(vaddr, gpaddr, mode, G); | ||
| } else if (mode == BaseMMU::Write && !pte.w) { | ||
| } else if (!exec_like && mode == BaseMMU::Read && !pte.r) { | ||
| fault = createPagefault(vaddr, gpaddr, mode, G); | ||
| } else if (mode == BaseMMU::Execute && !pte.x) { | ||
| } else if (mode == BaseMMU::Write && !pte.w) { | ||
| fault = createPagefault(vaddr, gpaddr, mode, G); |
There was a problem hiding this comment.
Restore MXR handling for read permissions.
checkPermissions no longer honors status.mxr, so execute-only pages become unreadable even when MXR is set. This is a functional regression for standard RISC‑V behavior.
🛠️ Proposed fix
- } else if (!exec_like && mode == BaseMMU::Read && !pte.r) {
+ } else if (!exec_like && mode == BaseMMU::Read &&
+ !(pte.r || (status.mxr && pte.x))) {
fault = createPagefault(vaddr, gpaddr, mode, G);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const bool exec_like = (mode == BaseMMU::Execute) || | |
| (mode == BaseMMU::Read && hlvx); | |
| if (reservedPteBitsFault(tc, pte)) | |
| return createPagefault(vaddr, gpaddr, mode, G); | |
| if (exec_like && !pte.x) { | |
| fault = createPagefault(vaddr, gpaddr, mode, G); | |
| } else if (mode == BaseMMU::Write && !pte.w) { | |
| } else if (!exec_like && mode == BaseMMU::Read && !pte.r) { | |
| fault = createPagefault(vaddr, gpaddr, mode, G); | |
| } else if (mode == BaseMMU::Execute && !pte.x) { | |
| } else if (mode == BaseMMU::Write && !pte.w) { | |
| fault = createPagefault(vaddr, gpaddr, mode, G); | |
| const bool exec_like = (mode == BaseMMU::Execute) || | |
| (mode == BaseMMU::Read && hlvx); | |
| if (reservedPteBitsFault(tc, pte)) | |
| return createPagefault(vaddr, gpaddr, mode, G); | |
| if (exec_like && !pte.x) { | |
| fault = createPagefault(vaddr, gpaddr, mode, G); | |
| } else if (!exec_like && mode == BaseMMU::Read && | |
| !(pte.r || (status.mxr && pte.x))) { | |
| fault = createPagefault(vaddr, gpaddr, mode, G); | |
| } else if (mode == BaseMMU::Write && !pte.w) { | |
| fault = createPagefault(vaddr, gpaddr, mode, G); |
🤖 Prompt for AI Agents
In `@src/arch/riscv/tlb.cc` around lines 1152 - 1163, The read-permission check no
longer respects status.mxr so execute-only pages are treated as unreadable;
update the permission logic in the TLB path by making exec_like consider MXR for
read accesses (i.e., treat a Read as exec_like when status.mxr is set), then
keep the existing checks (reservedPteBitsFault, createPagefault, and the pte.x /
pte.r / pte.w branches) unchanged so execute-only pages become readable under
MXR; locate and modify the definition of exec_like in this block (and/or where
checkPermissions is used) to incorporate status.mxr for BaseMMU::Read.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
2 participants
cherry-pick from upstream gem5,
https://github.com/OpenXiangShan/riscv-hyp-tests
for this unit-tests, still have bugs.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.