Merged
Conversation
Ensure only writable MSTATUS fields are updated, improving clarity and correctness in the ISA implementation. Change-Id: I283e0dbfdac4939dbef2d48185a7688881a1a79d
Implement checks for FPU and Vector CSR accesses to ensure they are only allowed when the respective functional units are enabled. This improves the correctness of CSR operations in the RISC-V ISA implementation. Change-Id: Ixxxxxxx
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughRemoves several user-mode and delegation CSRs from public/GDB exposure (replaced with reserved placeholders), centralizes mstatus writes via NEMU_MSTATUS_WMASK, adds VIRMODE-aware FPU/Vector CSR and instruction checks using VSSTATUS, and eliminates user-mode interrupt delegation and related handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Core as Core/CPU
participant CSR as CSRExecute
participant State as HartState (mstatus/VSSTATUS)
participant FPU as FPU/Vector Unit
Core->>CSR: request CSR read/write or execute FPU/V instruction
CSR->>State: read mstatus.FS, mstatus.VS, VIRMODE
alt VIRMODE == 1
CSR->>State: read VSSTATUS
State-->>CSR: VSSTATUS.fs
end
State-->>CSR: FS/VS values
alt FS/VS disabled (or VSSTATUS.fs == OFF)
CSR-->>Core: IllegalInstFault
else
CSR->>FPU: allow operation / perform CSR action
FPU-->>CSR: result/ack
CSR-->>Core: success / value
end
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🧪 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
🤖 Fix all issues with AI agents
In `@src/arch/riscv/isa/formats/standard.isa`:
- Around line 389-392: The error message produced when checking vsstatus.vs in
the vector CSR access path is confusing due to the duplicated "VS" string;
update the message in the branch that returns
std::make_shared<IllegalInstFault>("Vector CSR access with VS VS off\n",
machInst) to a clearer text such as "Vector CSR access with VSSTATUS.VS off" (or
similar), ensuring the IllegalInstFault instantiation still includes machInst
and the newline if needed; locate this change where vsstatus.vs is checked and
the IllegalInstFault is constructed.
🧹 Nitpick comments (1)
src/arch/riscv/isa/formats/standard.isa (1)
375-395: UseVPUStatus::OFFinstead of raw0for vector extension status checks to match the FPU pattern.The FPU block uses
FPUStatus::OFF(line 362), and the codebase defines aVPUStatusenum with identical structure (OFF = 0, INITIAL = 1, CLEAN = 2, DIRTY = 3). The vector block should useVPUStatus::OFFat lines 383 and 389 instead of raw0for consistency and improved readability.
![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: c60b632656
ℹ️ 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".
Enhance the FloatExecute template to include a check for the FPU status when operating in virtual mode. This ensures that illegal instructions are properly handled if the FPU is off in this context, improving the correctness of the RISC-V ISA implementation. Change-Id: I08a7e0333e32b48018fafafc38dfbdf9527c5c7e
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/arch/riscv/isa/formats/fp.isa`:
- Around line 42-48: The current check reads MISCREG_VIRMODE and uses
IllegalInstFault when VSSTATUS.fs == FPUStatus::OFF, but per the Privileged Spec
this must raise a virtual-instruction exception (code 22); replace the
IllegalInstFault creation with an HVFault (virtual instruction exception) in the
block that reads MISCREG_VIRMODE and MISCREG_VSSTATUS (symbols: MISCREG_VIRMODE,
VSSTATUS, MISCREG_VSSTATUS, IllegalInstFault) so the fault raised is HVFault
with an explanatory message like "FPU is off in virtual mode"; apply the same
replacement to the analogous checks in standard.isa (the blocks around the
checks at lines noted in the review).
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Align FP load/store permission checks with NEMU under virtualization. When VIRMODE=1, FP memory instructions must trap if vsstatus.fs is OFF. This fixes difftest divergence at fsw where gem5 continued in S mode while NEMU raised illegal instruction and trapped to M mode. Change-Id: Ib2ab957ab62d28b4a391107ed416d7bd940b56a2
Collaborator
Author
|
fix #757 /nfs/home/liuhaoyuan/GEM5/img_file/seeds_99_.img |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
jueshiwenli
previously approved these changes
Feb 12, 2026
Collaborator
jueshiwenli
left a comment
jueshiwenli
left a comment
There was a problem hiding this comment.
May try to merge it, but due to the NEMU version issue, I am not sure whether it will pass the CI checks.
(align to upstream PR 2341) This commit eliminates support for delegating interrupts to user mode in the RISC-V architecture. The changes include the removal of user mode-specific CSRs (USTATUS, UIE, UTVEC, USCRATCH, UEPC, UCAUSE, UTVAL, UIP) and adjustments to the interrupt handling logic to ensure that all interrupts are managed in supervisor or machine mode. This aligns the implementation with the intended behavior of the RISC-V ISA and improves overall clarity and correctness. Change-Id: Ic6a48b590804dea158e7961b6afd8e93db1bd346
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/arch/riscv/faults.cc (1)
117-117: Clarify panic text to cover all trap types.Line 117 says “interrupt,” but this branch is in the general trap-target switch. Consider using “trap” to avoid ambiguity in logs.
Small wording tweak
- panic("Delegating interrupt to user mode is removed."); + panic("Delegating trap to user mode is removed.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/arch/riscv/faults.cc` at line 117, The panic message currently reads "Delegating interrupt to user mode is removed." but sits inside the general trap-target switch and should cover all trap types; update the panic string (the call to panic(...) in the trap-target switch, within the trap-handling function) to say "Delegating trap to user mode is removed." (or similar using the word "trap") so logs are unambiguous; keep the rest of the panic call unchanged.src/arch/riscv/isa.cc (1)
213-214: Prefer explicit placeholder names for reserved CSRs in logs.Using
""works, but it makesDPRINTFoutput harder to inspect when reserved indices are accessed. Consider stable labels likeRESERVED01…RESERVED07for observability.🔧 Suggested tweak
- [MISCREG_RESERVED01] = "", - [MISCREG_RESERVED02] = "", + [MISCREG_RESERVED01] = "RESERVED01", + [MISCREG_RESERVED02] = "RESERVED02", ... - [MISCREG_RESERVED03] = "", - [MISCREG_RESERVED04] = "", - [MISCREG_RESERVED05] = "", - [MISCREG_RESERVED06] = "", - [MISCREG_RESERVED07] = "", + [MISCREG_RESERVED03] = "RESERVED03", + [MISCREG_RESERVED04] = "RESERVED04", + [MISCREG_RESERVED05] = "RESERVED05", + [MISCREG_RESERVED06] = "RESERVED06", + [MISCREG_RESERVED07] = "RESERVED07",Also applies to: 223-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/arch/riscv/isa.cc` around lines 213 - 214, Change the empty-string placeholders for reserved CSRs to stable labels so DPRINTF logs are easier to read: replace MISCREG_RESERVED01..MISCREG_RESERVED07 string values (the entries currently set to "") with explicit names like "RESERVED01", "RESERVED02", … "RESERVED07" in src/arch/riscv/isa.cc (the MISCREG_RESERVED0x array entries around the current MISCREG_RESERVED01 and the block at the other affected indices), so reserved indices print stable, identifiable labels instead of empty strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/arch/riscv/isa/decoder.isa`:
- Around line 66-72: Handlers c_fld and fld currently set status.fs = 3 but
never call xc->setMiscReg(MISCREG_STATUS, status), causing inconsistent FS state
compared to c_fldsp and flw; update the c_fld and fld handlers to call
xc->setMiscReg(MISCREG_STATUS, status) immediately after modifying status.fs and
before the virtual-mode VSSTATUS update (i.e., mirror the ordering used in
c_fldsp/flw), and keep the existing XC checks/read/write of MISCREG_VIRMODE and
MISCREG_VSSTATUS (vsstatus) unchanged.
In `@src/cpu/o3/commit.cc`:
- Around line 1700-1704: The code currently hard-asserts that priv ==
RiscvISA::PRV_U before taking the "fallback to MCAUSE" path, which prevents
handling other unexpected privilege values; replace the assert with a
conditional and robust handling: if priv is not PRV_U, emit a warning (e.g.,
DPRINTF or inform log) that an unexpected privilege level was seen and then
proceed to read the MCAUSE register via
cpu->readMiscReg(RiscvISA::MiscRegIndex::MISCREG_MCAUSE, tid) to set cause,
ensuring the fallback works for any priv value rather than aborting on non-PRV_U
cases.
---
Nitpick comments:
In `@src/arch/riscv/faults.cc`:
- Line 117: The panic message currently reads "Delegating interrupt to user mode
is removed." but sits inside the general trap-target switch and should cover all
trap types; update the panic string (the call to panic(...) in the trap-target
switch, within the trap-handling function) to say "Delegating trap to user mode
is removed." (or similar using the word "trap") so logs are unambiguous; keep
the rest of the panic call unchanged.
In `@src/arch/riscv/isa.cc`:
- Around line 213-214: Change the empty-string placeholders for reserved CSRs to
stable labels so DPRINTF logs are easier to read: replace
MISCREG_RESERVED01..MISCREG_RESERVED07 string values (the entries currently set
to "") with explicit names like "RESERVED01", "RESERVED02", … "RESERVED07" in
src/arch/riscv/isa.cc (the MISCREG_RESERVED0x array entries around the current
MISCREG_RESERVED01 and the block at the other affected indices), so reserved
indices print stable, identifiable labels instead of empty strings.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/arch/riscv/faults.ccsrc/arch/riscv/faults.hhsrc/arch/riscv/gdb-xml/riscv-64bit-csr.xmlsrc/arch/riscv/interrupts.hhsrc/arch/riscv/isa.ccsrc/arch/riscv/isa/decoder.isasrc/arch/riscv/isa/formats/standard.isasrc/arch/riscv/regs/misc.hhsrc/arch/riscv/remote_gdb.ccsrc/arch/riscv/remote_gdb.hhsrc/cpu/o3/commit.cc
💤 Files with no reviewable changes (3)
- src/arch/riscv/remote_gdb.hh
- src/arch/riscv/faults.hh
- src/arch/riscv/remote_gdb.cc
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Change-Id: I009dae0ab8a6afaaf3abb72a6dfde0df333f2fac
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/gem5.yml (1)
22-24: Upgradeactions/checkoutto v6, the latest available version.The workflow uses
actions/checkout@v2, which is significantly outdated. Version 6 (released Nov 2025, latest is v6.0.2 as of Feb 2026) offers improved performance, enhanced security, and better handling of submodules. This applies to all 8 checkout steps in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/gem5.yml around lines 22 - 24, Update all occurrences of the checkout action by replacing the pinned version string "actions/checkout@v2" with the current major release "actions/checkout@v6" (ensure any step that uses the checkout action — the lines containing uses: actions/checkout@v2 — are updated), and keep the existing "with: ref: ${{ github.event.inputs.branch_name || github.ref }}" settings unchanged so behavior remains identical while benefiting from v6 improvements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/gem5.yml:
- Around line 22-24: Update all occurrences of the checkout action by replacing
the pinned version string "actions/checkout@v2" with the current major release
"actions/checkout@v6" (ensure any step that uses the checkout action — the lines
containing uses: actions/checkout@v2 — are updated), and keep the existing
"with: ref: ${{ github.event.inputs.branch_name || github.ref }}" settings
unchanged so behavior remains identical while benefiting from v6 improvements.
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Change-Id: Ib1fedfbffe573dab4c44f5977c4e39525c082d5d
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
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
2 participants
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 by CodeRabbit
Bug Fixes
Refactor
Chores