Fix incorrect mip.SEIP handling by adding hardware SEIP latch & correcting PLIC aliasing#2218
Fix incorrect mip.SEIP handling by adding hardware SEIP latch & correcting PLIC aliasing#2218rmkhurana28 wants to merge 2 commits intoriscv-software-src:masterfrom
Conversation
…tch and correcting PLIC aliasing behavior
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix the handling of the Supervisor External Interrupt Pending (SEIP) bit in Spike's implementation of the RISC-V privileged specification by introducing a hardware SEIP latch and updating the interaction between mip, mvip, mvien, and the PLIC.
Changes:
- Added a
seip_hw_latchfield tomip_csr_tto track hardware-pending SEIP state - Updated
mip_csr_t::read()to merge the hardware latch with other interrupt sources - Modified
mip_csr_t::backdoor_write_with_mask()to route SEIP writes to the hardware latch - Changed
plic_t::context_update()to check mvien.SEIP and conditionally update either mvip or the hardware latch
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| riscv/csrs.h | Adds seip_hw_latch field and set_seip_hw_latch() method to mip_csr_t class |
| riscv/csrs.cc | Implements hardware latch read logic and updates backdoor write handling for SEIP |
| riscv/plic.cc | Modifies PLIC context update to respect mvien.SEIP aliasing when setting SEIP |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If mvien.SEIP == 0 → SEIP is aliased to mvip (virtual interrupt) | ||
| if (!(c->proc->state.mvien->read() & MIP_SEIP)) { | ||
| c->proc->state.mvip->write_with_mask(MIP_SEIP, pending); | ||
| } else { | ||
| // Otherwise SEIP is physical → update hardware latch, not mip.val | ||
| mip->set_seip_hw_latch((pending & MIP_SEIP) ? MIP_SEIP : 0); | ||
| } |
There was a problem hiding this comment.
The PLIC logic only handles the SEIP case (when c->mmode is false) but doesn't handle MEIP (when c->mmode is true). When handling machine-mode contexts (MEIP), the code should use backdoor_write_with_mask to update mip directly since mvien/mvip only apply to supervisor interrupts. The current implementation will incorrectly attempt to check mvien and update mvip or seip_hw_latch for MEIP, which is not correct behavior.
riscv/csrs.cc
Outdated
| } | ||
|
|
||
| // For all other bits, perform the normal backdoor write | ||
| this->val = (this->val & ~mask) | (val & mask); |
There was a problem hiding this comment.
The backdoor_write_with_mask function has a logic error. When mask contains MIP_SEIP, the function updates both seip_hw_latch (lines 842-843) and this->val (line 847). This means the SEIP bit will be set in both places, which will be OR'd together in the read() function (line 830 and potentially in val). The line 847 should exclude MIP_SEIP from the mask when updating this->val to prevent double-setting the SEIP bit.
| this->val = (this->val & ~mask) | (val & mask); | |
| const reg_t non_seip_mask = mask & ~MIP_SEIP; | |
| this->val = (this->val & ~non_seip_mask) | (val & non_seip_mask); |
riscv/plic.cc
Outdated
| reg_t mask = c->mmode ? MIP_MEIP : MIP_SEIP; | ||
|
|
||
| c->proc->state.mip->backdoor_write_with_mask(mask, best_id ? mask : 0); | ||
| // Determine SEIP pending status from PLIC |
There was a problem hiding this comment.
The comment on line 133 says "Determine SEIP pending status from PLIC" but this is misleading because the variable 'mask' on line 131 can be either MIP_MEIP or MIP_SEIP depending on whether the context is in machine mode or supervisor mode. The comment should be more generic to reflect that this line determines the pending status for either MEIP or SEIP.
| // Determine SEIP pending status from PLIC | |
| // Determine external interrupt pending (MEIP/SEIP) status from PLIC |
…ate SEIP from val, update PLIC logic
|
Updated the PR to address all follow-up issues: • Correct MEIP bypass logic (MEIP no longer goes through SEIP/mvien/mvip path) This version reflects proper interrupt routing per RISC-V Privileged Spec v1.12. |
This PR fixes a long-standing correctness issue in Spike’s handling of the
Supervisor External Interrupt Pending bit (mip.SEIP).
Root Cause
SEIP was incorrectly read/written directly through
mip->val,causing mismatches when:
mvien.SEIP = 0(SEIP is aliased to mvip)mvien.SEIP = 1(SEIP must come only from hardware/PLIC)Fix
This PR introduces a hardware SEIP latch inside
mip_csr_tand updates allcall sites to follow the correct read/write ordering.
Changes:
seip_hw_latchfield tomip_csr_tmip_csr_t::read()to merge hvip + mvip alias + hardware latchmip_csr_t::backdoor_write_with_mask()to route SEIP writes properlyplic_t::context_update()to respect mvien.SEIP aliasing rulesset_seip_hw_latch()for safe PLIC updatesResult
This restores correct SEIP semantics according to privileged spec 1.12, improves
behavior under virtualization (H-extension), and eliminates SEIP mis-ordering.
This is a correctness fix (not a functional extension), and has no effect unless
SEIP is toggled via hardware or mvien.