Repository navigation
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
3391905 to
77c9bfd
Compare
handle_cpuctrl_exception_entry() checks Spike's debug mode after processor->step(). When a stepped instruction traps, Spike enters debug mode inside that same step, so the check sees debug mode and skips the cpuctrlsts sync_exc_seen/double_fault_seen update. The RTL makes the update, because the trap is taken outside debug mode, and the two disagree: riscv_debug_single_step_test reads cpuctrlsts as 0x141 on the DUT and 0x101 in the model. Read debug mode before the step and pass it in. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
Ibex's ECALL handler advances MEPC and returns with mret, but never pops the 132-byte kernel-stack frame the trap entry code pushed. After enough ECALLs a subprogram reloads its return address from a slot that now holds something else, jumps into empty memory and runs until the timeout. Spike does the same, so the cosim stays quiet. Pop the frame before the mret. MEPC is already updated by then, so the pop cannot undo it. The repaired single-step test exposed this leak. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
riscv-dv's debug exception handler is a bare dret, so each exception taken in debug mode returns without popping the frame the debug ROM pushed on entry. One seed leaked 825 frames (108 KB), running the kernel stack into the program image. Jump to debug_end instead, which pops the frame and then runs dret. An exception in debug mode does not change dpc, so the return address is still right. Without a debug section nothing is pushed, so that case keeps the bare dret. Before this, stepping silently stopped at the first exception in debug mode, and the single-step tests passed without stepping the program. Carried as vendor/patches/google_riscv-dv/0006. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
push_gpr_to_kernel_stack moves TP, the kernel stack pointer, only after it has started storing. With single step on, a debug entry can land in the middle of that sequence and build its own frame from the old TP, on top of the half-written one. The handler then pops a corrupted SP and frame pointer. For RV32 with BARE translation and a scratch CSR, which is what Ibex uses, reserve the whole 132-byte frame with one addi before the first store and release it with one addi after the last load. The frame layout is unchanged. Other configurations keep the old sequence. Carried as vendor/patches/google_riscv-dv/0007. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
riscv_debug_single_step_test lets the random debug code write MSTATUS, MEPC, MCAUSE and MTVAL. Now that stepping works, that code runs between the instructions of live trap handlers, and a random 'csrw mepc' there overwrites the handler's return address: the handler mrets to address 0. Only write cpuctrlsts (0x7c0) and secureseed (0x7c1), which is what the test is about. Also cut instr_cnt from 10000 to 2000, because every instruction is now stepped; a seed still takes hundreds of steps. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
debug_seq raises debug_req for a fixed 75 cycles. That is too short when the request wakes the core from WFI. RVFI tags the request onto the instruction after the WFI, which debug entry then flushes, and by the time the first debug ROM instruction reaches ID the request has dropped. The cosim never sees it and steps Spike past the WFI: "DUT retired 80000000 but the ISS retired 800035f4" (riscv_debug_single_step_test, seed 12). A debug module holds haltreq until the hart has halted. Do the same: keep debug_req high until RVFI reports a retirement that carries it, with a 5000-cycle timeout. debug_new_seq is unchanged; its only user runs with +no_wfi=1 and holds the request for thousands of cycles. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
|
The fixes look right: dummy instructions no longer count as a single step or increment minstret, and the Zcmp expander waits when a dummy is inserted. I didn’t find an issue with the changes. |
andreaskurth
left a comment
andreaskurth
left a comment
There was a problem hiding this comment.
Thanks @kulan-pal! Fixing all three dummy-instruction problems together, each with assertions and a directed test, is much appreciated.
I'm requesting changes for one substantive issue (Zcmp atomicity, inline) and a few smaller ones. Details below.
Could you also update the Dummy Instruction Insertion section in doc/03_reference/security.rst? It currently only says dummies have "no functional impact". It would help to state that they do not count as a single step and are excluded from minstret and mhpmcounter10, but remain visible in cycle-based counters such as mcycle and the mul/div wait counters (mhpmcounter11/12).
| .valid_i (fetch_valid & ~fetch_err), | ||
| .id_in_ready_i (id_in_ready_i & ~pc_set_i), | ||
| // Hold the expander when dummy insertion replaces its current micro-op. | ||
| .id_in_ready_i (id_in_ready_i & ~pc_set_i & ~stall_dummy_instr), |
There was a problem hiding this comment.
With the expander held, a dummy can now sit in ID in the middle of a Zcmp sequence, but it reaches ID with instr_gets_expanded_id = INSTR_NOT_EXPANDED (L529). The controller relies on that value to defer debug entry during INSTR_EXPANDED/INSTR_EXPANDED_COMMIT and IRQs during INSTR_EXPANDED_COMMIT, so both can be taken mid-sequence, and the flush restarts the cm.* instruction from its first uop.
For cm.popret, a debug request while a dummy is in ID after the COMMIT addi sp, sp, 16 re-executes the whole instruction after dret: ra is reloaded from the already-popped frame and sp is incremented twice. With single step and dummies on, a step can likewise end after a few uops of cm.pop, which StepDebugEntryHasArchitecturalProgress flags. The new tests don't catch this because none combines Zcmp with debug or interrupts.
Could the controller still see the expansion state while a dummy is in ID mid-sequence, or could insertion be suppressed while the expander is mid-sequence? A test for this might also fit #2461.
There was a problem hiding this comment.
Confirmed: with debug requests added, the Zcmp test re-ran cm.popret after its sp update. I took your second option and replaced the expander hold: no dummy is now inserted directly before or inside a Zcmp instruction. The new debug and interrupt variants fail every seed on the previous RTL and pass with this change.
|
|
||
| // A dummy must not advance the Zcmp expander. In CmIdle, rlist and sp_offset are preloaded from | ||
| // instr_i and may change. A flush reloads the expander, so only a real accept is checked. | ||
| `ASSERT(NoZcmpExpansionAdvanceOnDummy, |
There was a problem hiding this comment.
Could you add a COVER per expander state (e.g. insert_dummy_instr && id_in_ready_i && compressed_decoder_i.cm_state_q == compressed_decoder_i.CmPopRetRa, and likewise for the others)? That would show dummy_instr_zcmp_test really hits every uop, as its header claims.
There was a problem hiding this comment.
Dummies no longer land between uops, so the covers now record a dummy that falls due and is held back, one per expander state. The Zcmp tests hit all eight.
| +no_csr_instr=0 | ||
| +randomize_csr=1 | ||
| +gen_all_csrs_by_default=1 | ||
| +add_csr_write=0x7c0,0x7c1 |
There was a problem hiding this comment.
gen_all_csrs_by_default already makes cpuctrlsts writable (via custom_csr), so random CSR instructions in the program and the debug section can clear dummy_instr_en between the ibex_rand_cpuctrlsts_streams. Would +add_csr_write=0x7c1 +remove_csr_write=0x7c0 keep dummies on, as +force_dummy_instr intends?
There was a problem hiding this comment.
Yes, thanks: random CSR writes could clear dummy_instr_en. The test now uses +add_csr_write=0x7c1 +remove_csr_write=0x7c0, so after boot setup only the directed stream writes cpuctrlsts.
do_single_step_d looks only at instr_valid_i, which dummy instructions assert like real ones. A dummy that reaches ID right after DRET completes the step: the core re-enters debug mode having retired nothing, dpc is unchanged and the stepped instruction never runs. Pass dummy_instr_id to the controller and let a dummy neither arm nor clear the step; it still executes. SingleStepStableAcrossDummy checks the controller. StepDebugEntryHasArchitecturalProgress checks that a step debug entry follows exactly one instruction that completed or trapped. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
Dummy instructions must have no architectural effect, but WB counts them as retired, so they advance minstret: instr_perf_count_id_i does not exclude them. Mask them with dummy_instr_wb_o, the marker for the instruction in WB: in both WritebackStage variants, and in the speculative count that a minstret read uses. NoMinstretForDummyInstr and NoSpecMinstretForDummyInstr check it. Tested with dummy_instr_minstret_test on opentitan and on a local copy with WritebackStage = 0. No shipped configuration has dummy instructions without a WB stage. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
A dummy inserted during a Zcmp expansion takes the current micro-op's slot, and the expander drops that micro-op. The dummy also reaches ID as a plain instruction, so debug entry or an interrupt can split the sequence, and the restart repeats cm.popret's sp update. Hold insertion off while the next instruction is a Zcmp micro-op, and freeze the dummy counter meanwhile. Assertions check that debug entry and interrupts never split a sequence where the controller defers them. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
+toggle_dummy_instr only lets the random cpuctrlsts stream enable dummy instructions, about half the time. Add +force_dummy_instr, which always enables them at the highest rate. riscv_debug_single_step_dummy_test single steps a random program with it. The program takes no traps, and random CSR writes skip cpuctrlsts, so dummies stay on and every debug entry is a request or a step. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
dummy_instr_minstret_test checks that minstret advances the same over a fixed block with dummies off, on and off again. dummy_instr_zcmp_test checks the frames, registers and returns of cm.push, cm.pop, cm.mv* and cm.popret* with dummies at the highest rate. Its debug and interrupt variants add random requests from the new core_ibex_directed_stress_test. The Zcmp tests set +disable_cosim=1 because the cosim does not enable Zcmp. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
The Dummy Instruction Insertion section only says that dummies have no functional impact. State that they are not counted as retired or as a single step, and that cycle counters still see them. Also state that no dummy is inserted directly before or inside a Zcmp instruction, and that Zcmp instructions do not count towards the insertion interval. Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
77c9bfd to
48c136f
Compare
|
Thanks for the review! The |
andreaskurth
left a comment
andreaskurth
left a comment
There was a problem hiding this comment.
Thanks @kulan-pal, everything from the last round is resolved 👍 LGTM. One non-blocking question inline, for a possible follow-up.
|
|
||
| // Hold dummies back while the next instruction is a Zcmp micro-op. The controller sees a dummy | ||
| // as a separate instruction, so one inside a sequence would let debug entry or an interrupt in. | ||
| assign dummy_instr_hold = instr_gets_expanded != INSTR_NOT_EXPANDED; |
There was a problem hiding this comment.
Holding dummies off before and inside every Zcmp instruction leaves a dummy-free, fixed-timing window around each cm.push/cm.popret, which compilers emit in most function prologues and epilogues (including the ra reload). Letting the controller see the expansion state while a dummy is in ID would keep dummies inside sequences. Would you agree that's worth a follow-up issue as a countermeasure improvement?
|
@kulan-pal #2499 is in the merge queue; could you please rebase this PR after #2499 is merged? Happy to merge after the rebase |
3 participants
Depends on #2499; only the last six commits are new here. Resolves #2186.
Dummy instructions must have no architectural effect, but inside the pipeline they look like real instructions.
Three places let them have one:
do_single_step_dlooks only atinstr_valid_i, so a dummy right afterdretcompletes the step and the core halts again without retiring anything. The controller now ignores dummies for
the step, as in the "execute the dummy, then step" option from [rtl] Behaviour of debug single stepping with dummy instruction insertion may be surprising #2186.
minstret. WB counted dummies as retired. They are now masked in bothWritebackStagevariants and in thespeculative count that a
minstretread uses.cm.*expansion took the slot of the current uop, which was lost. It alsoreached ID as an ordinary instruction, so debug entry or an interrupt could split the sequence. The handler
then restarted the instruction, and
cm.popretupdatedsptwice. No dummy is now inserted directly beforeor inside a Zcmp instruction, and Zcmp instructions no longer count towards the insertion interval.
Each fix comes with assertions. Two of them check that debug entry never splits a Zcmp sequence and that an
interrupt never splits its committing uops.
security.rstnow describes these effects.The new tests are
riscv_debug_single_step_dummy_test(with a new+force_dummy_instr),dummy_instr_minstret_test, anddummy_instr_zcmp_testwith debug-request and interrupt variants.opentitan, VCSriscv_debug_single_step_dummy_test, 20 seedsdummy_instr_minstret_test, 5 seedsminstretreaddummy_instr_zcmp_testand its debug and interrupt variants, 5 seeds eachriscv_debug_single_step_test, 15 seedsWith these tests and assertions added to the previous RTL of this PR, the debug and interrupt variants failed
every seed. The Zcmp tests run with
+disable_cosim=1because the cosim does not enable Zcmp, so they rest onthe program's checks and the assertions.
On a local
opentitanconfig without the WB stage,dummy_instr_minstret_testgives 0/5 without the fix and5/5 with it. With
DummyInstructions = 0the new logic is tied off.AI disclosure (CLA §9): written with help from Claude Code and reviewed with OpenAI Codex; I have reviewed and understood every change and take full responsibility for it.