ibex: clear cpuctrlsts.sync_exc_seen on dret, matching mret - #2487
ibex: clear cpuctrlsts.sync_exc_seen on dret, matching mret#2487john-kearney wants to merge 1 commit into
Conversation
sync_exc_seen is set on every synchronous exception and cleared by mret, but the dret arm only restores priv_lvl. A debug session that abandons an exception handler (halt inside handler, set dpc, dret) therefore leaves the flag set indefinitely, so the next synchronous exception - any class, arbitrarily later - falsely pulses double_fault_seen_o and latches the double_fault_seen audit bit. Consumers such as OpenTitan wire double_fault_seen_o into escalation, so a routine debug session can cause a spurious SoC escalation. Clear the flag in the csr_restore_dret_i arm exactly as the MRET arm does; update the lockstep RVFI double-fault predictor and the documentation accordingly. simple_system gains a small DbgCtrl bus device to drive debug_req and observe the flag for the directed regression. Reproduction, regression and a differential (laundering-tradeoff) test: https://gist.github.com/john-kearney/61911098ab6a70e8b8bad969748eaca8 Signed-off-by: John Kearney <john@authensor.com>
|
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign the Contributor License Agreement before we can accept your contribution. The CLA ensures that all users of the project are granted rights to use the submission. Before signing the CLA, please ensure that you have the authority from your organisation to grant these rights. You will be asked to sign the CLA when you first contribute to each lowRISC repository, and will be asked to re-sign if the CLA changes. Each individual who has committed in this Pull Request should sign the CLA by posting a Pull Request Comment containing the text below. I have read the CLA Document. By submitting this pull request comment, I am hereby confirming my acceptance of the terms of the CLA Document and my agreement to be legally bound by its terms. You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
Problem
cpuctrlsts.sync_exc_seenis the flag behind Ibex's double-fault detection:it is set on every synchronous exception and cleared by
mret(
rtl/ibex_cs_registers.sv,csr_restore_mret_iarm). Thedretarm(
csr_restore_dret_i) restorespriv_lvlonly — it does not clear theflag.
A debugger that halts the core inside an exception handler (a
debug_reqhalt) and then resumes somewhere else (set
dpc,dret— the standardGDB "jump"/skip-frame flow) abandons that handler. Per the documented
contract, a double fault is a synchronous exception occurring whilst
handling a previous synchronous exception, cleared by
mret(
doc/03_reference/exception_interrupts.rst,:ref:
double-fault-detect). After thedret, the core is no longerhandling anything, yet
sync_exc_seenstays 1 indefinitely (until somelater unrelated
mretor a softwarecpuctrlwrite).Consequence: the next synchronous exception — any class, any context,
arbitrarily later — falsely pulses the
double_fault_seen_oSoC escalationoutput for one cycle and falsely latches the HW-set/SW-clear
cpuctrlsts.double_fault_seenaudit bit. Consumers such as OpenTitan wiredouble_fault_seen_ointo escalation, so a routine debug session can causea spurious SoC escalation/lockout plus permanently false audit state.
Reproduction on
simple_system(Verilator): illegal instruction → handlerasserts
debug_reqand spins → debug ROM setsdpc=resume,dret→unrelated benign
ecall. Before this patch:double_fault_seen_opulsesand
double_fault_seenlatches. After: no pulse, flag reads 0 afterdret.Change
rtl/ibex_cs_registers.sv: in thecsr_restore_dret_i(DRET) arm, clearsync_exc_seenexactly as the MRET arm does (cpuctrlsts_part_we = 1'b1; cpuctrlsts_part_d.sync_exc_seen = 1'b0;). Not parameter-gated — appliesto every configuration.
rtl/ibex_top.sv: update the RVFI double-fault predictor in lockstep soit clears its modelled
sync_exc_seenondret(0x7b200073) as wellas
mret(0x30200073); otherwise theDoubleFaultPulse*assertionswould diverge from the RTL after a debug session.
doc/03_reference/exception_interrupts.rst,doc/03_reference/cs_registers.rst: document that the flag is cleared bymretordret.examples/simple_system/rtl/ibex_simple_system.sv: small test-supportaddition — a
DbgCtrlbus device (0x40000) to drive thedebug_reqpinfrom software and observe
double_fault_seen_opulses and livesync_exc_seen, enabling a directed regression test without a full debugmodule.
Design note
dretreturning into the interrupted handler would, with this change,arrive with
sync_exc_seencleared; if that handler then faults againbefore its
mret, the double fault is not flagged. This is the safedirection: the debugger has taken control of the flow (and can re-arm the
flag with a single
csrwif it deliberately resumes fault-stormanalysis), whereas the current behaviour escalates the SoC on an exception
that is provably not a double fault. If maintainers prefer, the clear
could be restricted to
drets that redirect away from the handler(
dpcoutside the handler range), at the cost of tracking handler boundsthe hardware does not currently have.
Known trade-off: a
dretthat resumes inside the handler (adversarial differential test)The unconditional clear is deliberately not semantically identical to
mret.mretisthe defined terminator of the handling window, so no true double fault can ever straddle an
mret.drethas a semanticsmretlacks: the plain debugger continue (halt on abreakpoint inside a handler,
dretwithdpcnever written) resumes in place, inside thestill-open handling window — and with this change it wipes the tracking flag. Every
subsequent fault in that handler is then invisible to double-fault detection until the next
synchronous exception re-arms the flag (a false-clean
cpuctrlsts.double_fault_seenauditbit for a real double fault during a debugged handling window — the harder-to-detect failure
mode for a HW-set/SW-clear audit bit).
We believe the trade is the right one, and say so explicitly:
could always clear the bit directly (
cpuctrlstsbit 6 is SW-writable) — the fix changes adefault for a trusted actor, it grants no new capability.
session escalates the SoC on any later, unrelated exception, arbitrarily later); the new
false negative is bounded to the current handling window and re-arms on the next sync
exception.
double_fault_seen_ois a dedicated SoC outputonly;
alert_major_internal_odoes not include it.This is proven, not argued: a differential directed test
(
launder.S (same gist), runner run_launder.sh (same gist), watchdogged) runsthe identical instruction flow on unfixed and fixed RTL. Console markers are
live MMIO/CSR readouts;
K0= handled-fault+mretsanity,s= livesync_exc_seenafter an in-handlerdret-continue,L= real double fault(
illegal insninside the still-open handler, nomretever executed)pulses,
a= bit 7 latched,M=mret-boundary count,N/b= genuineno-debug nested double fault (control, must stay detected):
Pinned expected output on fixed RTL:
K0s0L0a0M0N1b1Xwith exactly 1 pulse(the genuine control event); unfixed:
K0s1L1a1M0N2b1X, 2 pulses. Tracewindows (gist: trace-window-{fixed,unfixed}-dret.log
):dret@t198 with nocsrw dpc, resume @t206 inside the handler, flag read @t222 is the only divergence. If maintainers prefer, the clear could be restricted todrets that redirect away from the handler (dpc` outside handler bounds),at the cost of handler-extent tracking the hardware does not currently have
— happy to prototype either variant.
Debuggers that deliberately resume into handlers during fault-storm analysis
should re-arm the flag with a single
csrw(bit 6); if this PR lands, afollow-up sentence in
doc/03_reference/exception_interrupts.rstcoveringthe resume-in-place caveat would be appropriate (kept out of this patch to
keep the doc hunks minimal — the trade-off is fully stated here).
Testing
simple_system(Verilator 5.050, defaultconfig), sequence: two unrelated sync exceptions straddling a
debug session that abandons the handler via
dpc+dret(
the regression program (gist: https://gist.github.com/john-kearney/61911098ab6a70e8b8bad969748eaca8), runnerfixes/run-tests.sh):sync_exc_seenstill reads 1 afterdret,following
ecallpulsesdouble_fault_seen_oand latches bit 7;dret, no pulse, bit 7 clear(also re-ran the original bug-demonstrating binary: pulse gone).
above):
launder.S (same gist)—K0s0L0a0M0N1b1X, 1 pulse on fixed RTL;the laundering window it demonstrates is the documented trade-off of this
change, and the suppression is a visible, pinned decision rather than a
silent pass.
identical pass before and after the change;
mret-clearing behaviourunchanged.
(
dv/uvm/core_ibex) and riscv-compliance; the RVFI predictor change isexpected to be exercised by those. The
ibex_top.svpredictor block isifdef RVFIand not compiled into simple_system.Refs: double-fault escalation contract in
doc/03_reference/exception_interrupts.rst(:ref:double-fault-detect);double_fault_seen_odocumented as a dedicated escalation output indoc/02_user/integration.rst.