Skip to content

[rtl] Keep dummy instructions out of single step, minstret and the Zcmp expander - #2500

Draft
kulan-pal wants to merge 14 commits into
lowRISC:masterfrom
kulan-pal:fix/dummy-instr
Draft

[rtl] Keep dummy instructions out of single step, minstret and the Zcmp expander#2500
kulan-pal wants to merge 14 commits into
lowRISC:masterfrom
kulan-pal:fix/dummy-instr

Conversation

@kulan-pal

@kulan-pal kulan-pal commented Sep 14, 2026

Copy link
Copy Markdown

Depends on #2499 (reliable single stepping in the bench). Review the last five
commits only until it lands. Resolves #2186 (reported by @GregAC); #1479 asked
earlier about stall_dummy_instr interactions in the IF stage, which the third fix
touches.

With SecureIbex dummy-instruction insertion armed, three places in the core let a
dummy have an architectural effect. Dummy instructions are documented as having no
functional impact on processor state (doc/03_reference/security.rst), so each is a
defect. All three come from the same property: once in the pipeline a dummy is
deliberately indistinguishable from a real instruction.

A dummy instruction satisfies a single step (#2186)

do_single_step_d in ibex_controller.sv keys off instr_valid_i alone. A dummy
landing in the window after dret satisfies the step: debug is re-entered having
retired nothing, dpc is unchanged, the stepped instruction never runs. The Debug
Spec asks for a halt after one instruction. #2186 lists three options; one
maintainer favoured option 3 for a future release, executing the dummy and then
stepping, which is what this does. dummy_instr_id is plumbed to the controller and
dummies are excluded from both arming and clearing the step. Dummy instructions
still execute; they no longer complete the architectural single step. Assertions:
SingleStepStableAcrossDummy, StepDebugEntryHasArchitecturalProgress. Cover
points record an armed dummy under step.

Dummy instructions increment minstret

perf_instr_ret_wb_o in ibex_wb_stage.sv is qualified by instr_perf_count_id_i,
which already excludes ebreak, ecall, illegal instructions, fetch errors, minstret
writes and expanded Zcmp micro-ops, but has no notion of dummies because ID cannot
tell them apart. Mask with dummy_instr_wb_o in both WritebackStage branches and
on the speculative retire indication a CSR read of minstret consumes. Assertions:
NoMinstretForDummyInstr, NoSpecMinstretForDummyInstr.

A dummy inserted mid-expansion drops a Zcmp micro-op

The dummy mux in ibex_if_stage.sv replaces instr_decompressed after the
compressed decoder has already treated the micro-op as consumed. A dummy inserted
during a cm.push/cm.pop/cm.mvsa01 expansion advances the state machine while
the pipeline receives the dummy instead of that micro-op: a store omitted while sp
is still adjusted, a load or return omitted, half of a register-pair move. The
advance condition now includes ~stall_dummy_instr, as fetch_ready, the skid
buffer and the PC-increment checker in the same module already do. Assertion:
NoZcmpExpansionAdvanceOnDummy.

Tests

  • +force_dummy_instr (ibex_directed_instr_lib.sv): +toggle_dummy_instr only
    unpins dummy_instr_en, so a stream arms insertion about half the time and reaches
    the maximum rate one time in sixteen. The new plusarg pins enable and the highest
    rate so every seed is evidence.
  • riscv_debug_single_step_dummy_test (testlist, 20 iterations, SecureIbex: 1):
    single stepping with dummies forced. The program takes no traps of its own and
    writes only cpuctrlsts/secureseed, so every debug entry is a request or a
    completed step. Stepping through live trap handlers stays with the stock test.
  • dummy_instr_minstret_test: reads minstret across a straight-line block of
    known length with insertion off, on at the highest rate, and off again, and
    requires the same delta each time. The cosim is a second oracle.
  • dummy_instr_zcmp_test: cm.push/cm.pop with the full register list,
    cm.mvsa01, cm.mva01s, cm.popret/cm.popretz through a call, once without
    insertion and 48 times with it, checking frame contents, padding, sp, the moved
    and popped registers and the returns. Raw halfwords, so it assembles with the
    pinned toolchain. Cosim mismatches are demoted to non-fatal (+disable_cosim=1)
    because the reference model has no Zc; see [rtl] Trap on reserved Zcmp encodings without starting an expansion #2498. The test's own checks carry
    the verdict.

Verification

Baseline 34b07057. opentitan config, VCS U-2023.03-SP2, Spike ibex_cosim at
the pinned version, on top of #2499.

Test Tree Result
riscv_debug_single_step_dummy_test, 20 seeds RTL fixes + tests 20 / 20, 91 armed-dummy cover hits over 11 seeds
riscv_debug_single_step_dummy_test, 20 seeds tests only 2 / 20, every divergence right after a debug entry
dummy_instr_minstret_test, 5 seeds RTL fixes + tests 5 / 5
dummy_instr_minstret_test tests only cosim mismatch, x6 DUT 0x59 vs model 0x56
dummy_instr_minstret_test, 5 seeds, WritebackStage: 0 copy of opentitan RTL fixes + tests 5 / 5 (no shipped config combines SecureIbex with WritebackStage: 0)
dummy_instr_zcmp_test, 5 seeds RTL fixes + tests 5 / 5
dummy_instr_zcmp_test tests only fail, canary left in the s9 frame slot
mcounteren_test, 5 seeds RTL fixes + tests 5 / 5
stock riscv_debug_single_step_test, 15 seeds RTL fixes + tests 15 / 15

Verilator and Verible lint clean on all six CI configs. Not run: the full random
regression and coverage. With DummyInstructions = 0 the dummy indicators and the
insertion stall are tied low, so the new controller, counter and expander qualifiers
preserve existing behaviour.

Disclosure

Per CLA section 9: the analysis and code in this PR were produced with the
assistance of an AI coding tool (Claude Code) and independently reviewed with a
second tool (OpenAI Codex). I have reviewed and understood every change and take
full responsibility for it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Rn6C7Eyq1LchFeP9w6up9P

@kulan-pal
[dv] Add sim_opts to the directed-test schema
1a3c5e9 
run_rtl.py reads 'sim_opts' out of the test options for both riscv-dv
and directed tests, but the pydantic schema that validates a directed
test's configuration has no such field, so pydantic's default
extra="ignore" dropped it. A directed test therefore could not pass a
plusarg through its own testlist entry.

Add the field to DConfig, optional so every existing entry stays valid,
and on DConfig rather than DTest so a config can supply a default that
an individual test still overrides after the merge.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[dv] Accept a test once in filter_tests_by_config
5f4b0a7 
The append that accepts a test sat inside the loop over its rtl_params,
so a test listing N parameters was scheduled N times: make warned that
its targets were given more than once and regr.log counted every seed N
times. Every existing entry lists a single parameter, which hid it.
Append in the loop's else clause instead, once all parameters matched.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[dv] Sort directory listings in gen_testlist.py
ea69428 
gen_testlist.py enumerated the vendored test directories with ls, whose
order follows the collation of the current locale: under C/POSIX
sh-misaligned sorts before shamt, under en_US after it. Regenerating
directed_testlist.yaml therefore produced a different file depending on
the environment, and the committed file could not be checked against
its generator.

List the directories with os.listdir() and sort the names in code point
order, which does not depend on the locale. Regenerate
directed_testlist.yaml: no entry changes, only the order of the
vendored tests.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[dv/cosim] Sample debug mode before the step
9df4b2d 
handle_cpuctrl_exception_entry() tested Spike's debug-mode state after
processor->step(), but a trap taken on a stepped instruction enters
debug mode inside that same step (the catch block in execute.cc), so
the post-step check saw debug mode and skipped the sync_exc_seen and
double_fault_seen updates. The RTL sets those flags at trap take, which
is outside debug mode, so the models diverged: riscv_debug_single_step
seeds read cpuctrlsts as 0x141 on the DUT against 0x101 in the model.

Capture debug mode before the step and pass it in. The RTL behaviour is
architecturally correct; only the model's sampling point was wrong.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[dv] Pop the trap frame in the ECALL handler
f447c1c 
The Ibex ECALL handler override advanced MEPC and executed mret without
popping the kernel-stack frame the trap dispatcher had pushed on entry,
so every ECALL leaked one 132-byte frame. Once enough frames had leaked,
a generated subprogram epilogue reloaded its return address from a slot
that now held unrelated saved state and jumped into uninitialized
memory, after which the program walked mepc+4 through zeroed memory
until the wall-clock timeout. The cosimulator stayed silent because
Spike executed the same architecturally consistent corruption.

Pop the frame before the mret, restoring every interrupted GPR
including the randomized SP and TP roles. MEPC is rewritten before the
pop, so restoring the scratch register cannot lose the new return
address, and the register-dump stores keep their memory side effects.

The leak only becomes visible once single stepping runs through the
whole program (see the debug ROM fix in the vendored riscv-dv), which
is why the single-step tests were the first to hit it.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[vendor] Pop the debug frame on debug exceptions
84b2eaf 
The debug exception handler ended in a bare dret, so every exception
taken inside debug mode returned without popping the frame the debug
ROM had pushed. Each in-debug exception leaked one 132-byte frame; one
seed leaked 825 frames and walked 107.9 KB down from kernel_stack_end
into the program image.

Route the handler to <hart>debug_end (pop then dret) whenever a debug
section was generated; dpc is untouched by an in-debug exception, so
the canonical epilogue returns to the right place. Keep the bare dret
when there is no debug section and therefore no frame to release.

This restores whole-program single stepping: before it, stepping
silently stopped at the first in-debug exception, so the single-step
tests passed without stepping through the program.

Carried as vendor/patches/google_riscv-dv/0006 so that the next vendor
update re-applies it.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[vendor] Make the kernel-stack claim atomic
7d327df 
push_gpr_to_kernel_stack published its claim in TP only at the end of
the push. With dcsr.step armed, stepping a trap handler lets each debug
entry allocate a frame from the stale TP, four bytes into the handler's
half-built frame: the debug frame's slot 8 lands on the handler's saved
SP and slot 28 on its frame pointer, so the handler's pop reloads its
base mid-sequence and the register file unwinds to garbage.

On the configuration Ibex runs (RV32, BARE translation, scratch CSR
implemented) claim the frame in a single instruction (addi tp, tp, -132)
so TP publishes before any store, keep the USP slot at the top of the
frame (offset 128, so the frame layout and every address are unchanged),
restore USP while the claim is still held, and release in one
instruction. A debug entry interrupting any point of the sequence now
allocates below the active frame. Every other configuration (RV64,
address translation, no scratch CSR) keeps the original sequence
untouched.

Carried as vendor/patches/google_riscv-dv/0007 so that the next vendor
update re-applies it.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[dv] Restrict single-step test CSR writes
200dbeb 
The test randomized writes to MSTATUS, MEPC, MCAUSE and MTVAL. Under
real stepping the random debug body executes interleaved between every
instruction of a live mtvec handler, so a random 'csrw mepc, <garbage>'
from the body clobbers the return address the handler underneath still
needs, and the program mrets into zeroed memory. Trace-proven: csrrw
x1, mepc, x15 with x15 = 0, then mret, then a walk from address 0. The
defect was masked because the bare dret in the debug ROM kept the debug
body degenerate; with the debug frame fix stepping runs through the
whole program.

Trim add_csr_write to cpuctrlsts (0x7c0) and secureseed (0x7c1), the
CSRs the test is actually about, and reduce instr_cnt from 10000 to
2000 to keep simulator runtime manageable now that stepping runs
through the whole program; the test still arms hundreds of steps per
seed.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[dv] Hold debug_req until the hart has halted
fb6f11b 
debug_seq pulsed debug_req for a fixed 75 cycles. A request that wakes
the core from WFI needs longer than that to reach the debug ROM: the
clock restarts, the instruction after the WFI is refetched into ID,
debug entry flushes it and the halt address is fetched. RVFI attaches a
captured request to the first instruction entering ID, which is the
flushed one, and the debug ROM's first instruction then reports the
live debug_req, already low. The cosim never learns of the request and
steps Spike past the WFI: riscv_debug_single_step_test seed 12 with
real stepping failed with "DUT retired 80000000 but the ISS retired
800035f4".

A debug module keeps haltreq asserted until the hart reports halted,
which the debug ROM does with its first instructions. Do the same:
after the pulse, keep debug_req high until RVFI has reported a
retirement that carries the request (debug_mode alone rises at debug
entry, before the halt address is fetched, so it is too early). Not
seeing one within 5000 cycles is an error. debug_new_seq is left
alone: its only user, riscv_assorted_traps_interrupts_debug_test, runs
with +no_wfi=1 and pulses for 3000-5000 cycles.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[rtl] Do not count a dummy instruction as a step
5fc1072 
do_single_step_d keys off instr_valid_i alone, and dummy instructions
assert it like any other -- they are deliberately indistinguishable in
the pipeline. A dummy landing in the window after DRET then satisfies
the step: debug is re-entered having retired nothing, dpc is unchanged,
and the instruction being stepped never runs. Debug v0.13.2 4.8.1 asks
for a halt after a single instruction.

Plumb dummy_instr_id down to the controller and exclude dummies from
both arming and clearing the request. SingleStepStableAcrossDummy
guards the request across a dummy;
StepDebugEntryHasArchitecturalProgress requires exactly one
architectural retirement before a step-caused debug entry.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[rtl] Exclude dummy instructions from minstret
da5f016 
Dummy instructions have no functional impact on processor state
(doc/03_reference/security.rst), so retiring one must not advance the
retired-instruction counter. perf_instr_ret_wb_o is qualified by
instr_perf_count_id_i, which already excludes ebreak, ecall, illegal,
illegal-CSR, fetch error, minstret writes and expanded Zcmp uops -- but
has no notion of dummies, because ID cannot tell them apart.

Mask them in both WritebackStage branches and on the speculative retire
indication that a CSR read of minstret consumes, where dummy_instr_wb_o
is available. NoMinstretForDummyInstr and NoSpecMinstretForDummyInstr
check both branches.

Verified with dummy_instr_minstret_test (5 seeds each) on the opentitan
configuration and on a local copy of it with WritebackStage: 0, so both
generate branches are covered; no shipped configuration combines
SecureIbex with WritebackStage: 0.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[rtl] Hold the Zcmp expander on dummy insertion
276cf73 
The dummy mux replaces instr_decompressed only after the compressed
decoder has already treated the micro-op as consumed. A dummy inserted
mid-expansion therefore advances the Zcmp state machine while the
pipeline receives the dummy instead of that uop, dropping it entirely:
a cm.push store omitted while sp is still adjusted, a cm.pop load or
return omitted, or half of a cm.mvsa01. That is architectural
corruption, not a timing perturbation.

Qualify id_in_ready_i the way every other consumer of the held
instruction in this module already is -- fetch_ready, the skid buffer
and the PC increment checker. NoZcmpExpansionAdvanceOnDummy guards it.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[dv] Add a single-step test with dummies armed
29a6b7d 
+toggle_dummy_instr only removes the constraint pinning dummy_instr_en
to zero, so a stream arms the feature about half the time and reaches
the maximum insertion rate about one time in sixteen. That is fine for
spreading coverage and useless for proving a specific interaction: a
seed that never arms is not evidence of anything. +force_dummy_instr
pins enable=1 and mask=000 so every emitted stream arms at the highest
rate, and implies toggle_dummy_instr so the two cannot contradict.

riscv_debug_single_step_dummy_test drives dummy instructions against
single step and keeps to that: the generated program takes no traps of
its own (no ecall, ebreak, wfi, dret, illegal instructions or
sub-programs) and only writes cpuctrlsts/secureseed, so every debug
entry is a debug request or a completed step. Stepping through live
trap handlers is riscv_debug_single_step_test's business.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@kulan-pal
[dv] Add dummy-instruction directed tests
3391905 
Nothing in core_ibex checks that dummy instructions are architecturally
invisible beyond the register file: the tests that toggle dummy
insertion execute no Zcmp and read no counters, and the Zcmp expansion
has no random stimulus at all.

dummy_instr_minstret_test reads minstret across a straight-line block
of known length with dummy insertion disabled, enabled at the highest
rate and disabled again, and requires the same delta every time. The
cosim compares the read values against the reference model as a second
oracle.

dummy_instr_zcmp_test runs cm.push/cm.pop with the full register list,
cm.mvsa01, cm.mva01s and cm.popret/cm.popretz through a call, once
without dummy insertion and then 48 times with it at the highest rate,
and checks the frame contents and padding, sp, the popped and moved
registers and the returns. A micro-op displaced by a dummy leaves a
canary, a stale register, a wrong sp or a fall-through past the return.
Cosim mismatches are demoted to non-fatal for this test
(+disable_cosim=1) because the core_ibex cosim does not enable Zcmp for
the reference model, which therefore traps on every cm.* instruction;
the self-checks carry the verdict.

Signed-off-by: Kulan Palanichamy <kulan.palanichamy@opentitan.org>
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown

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.

@kulan-pal kulan-pal mentioned this pull request Sep 14, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[rtl] Behaviour of debug single stepping with dummy instruction insertion may be surprising

1 participant

@kulan-pal