From 5944425f58a8749c15d769edc414486592d6cbc8 Mon Sep 17 00:00:00 2001 From: Kulan Palanichamy Date: Wed, 19 Aug 2026 11:23:53 -0700 Subject: [PATCH 1/6] [dv/cosim] Sample debug mode before the step 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 --- dv/cosim/spike_cosim.cc | 9 ++++++--- dv/cosim/spike_cosim.h | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/dv/cosim/spike_cosim.cc b/dv/cosim/spike_cosim.cc index 76df135e27..9bdd8da1da 100644 --- a/dv/cosim/spike_cosim.cc +++ b/dv/cosim/spike_cosim.cc @@ -218,6 +218,9 @@ bool SpikeCosim::step(uint32_t write_reg, uint32_t write_reg_data, uint32_t pc, // (If the current step causes a synchronous trap, it will be // recorded against the current pc) initial_spike_pc = (processor->get_state()->pc & 0xffffffff); + // A trap on a stepped instruction enters debug mode within this step, but + // the cpuctrlsts update depends on the mode the trap was taken in. + bool initial_spike_debug_mode = processor->get_state()->debug_mode; processor->step(1); // ISS @@ -278,7 +281,7 @@ bool SpikeCosim::step(uint32_t write_reg, uint32_t write_reg_data, uint32_t pc, return false; } - handle_cpuctrl_exception_entry(); + handle_cpuctrl_exception_entry(initial_spike_debug_mode); // This is all the checking possible when consider a // synchronously-trapping instruction that never retired. @@ -534,8 +537,8 @@ void SpikeCosim::leave_nmi_mode() { #endif } -void SpikeCosim::handle_cpuctrl_exception_entry() { - if (!processor->get_state()->debug_mode) { +void SpikeCosim::handle_cpuctrl_exception_entry(bool debug_mode_at_trap) { + if (!debug_mode_at_trap) { bool old_sync_exc_seen = change_cpuctrlsts_sync_exc_seen(true); if (old_sync_exc_seen) { set_cpuctrlsts_double_fault_seen(); diff --git a/dv/cosim/spike_cosim.h b/dv/cosim/spike_cosim.h index 89f3773f30..87f8e8c1a1 100644 --- a/dv/cosim/spike_cosim.h +++ b/dv/cosim/spike_cosim.h @@ -88,7 +88,7 @@ class SpikeCosim : public simif_t, public Cosim { bool change_cpuctrlsts_sync_exc_seen(bool flag); void set_cpuctrlsts_double_fault_seen(); - void handle_cpuctrl_exception_entry(); + void handle_cpuctrl_exception_entry(bool debug_mode_at_trap); void initial_proc_setup(uint32_t start_pc, uint32_t start_mtvec, uint32_t mhpm_counter_num, bool rv32b_enabled); From 742cb2b3b5ecc4966973af3d139c36f91eadbae2 Mon Sep 17 00:00:00 2001 From: Kulan Palanichamy Date: Sat, 5 Sep 2026 14:39:47 -0700 Subject: [PATCH 2/6] [dv] Pop the trap frame in the ECALL handler 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 --- dv/uvm/core_ibex/riscv_dv_extension/ibex_asm_program_gen.sv | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dv/uvm/core_ibex/riscv_dv_extension/ibex_asm_program_gen.sv b/dv/uvm/core_ibex/riscv_dv_extension/ibex_asm_program_gen.sv index 4f7544b09a..f26c979b42 100644 --- a/dv/uvm/core_ibex/riscv_dv_extension/ibex_asm_program_gen.sv +++ b/dv/uvm/core_ibex/riscv_dv_extension/ibex_asm_program_gen.sv @@ -75,6 +75,8 @@ class ibex_asm_program_gen extends riscv_asm_program_gen; $sformatf("addi x%0d, x%0d, 4", cfg.gpr[0], cfg.gpr[0]), $sformatf("csrw 0x%0x, x%0d", MEPC, cfg.gpr[0]) }; + // Pop the frame pushed on trap entry. + pop_gpr_from_kernel_stack(MSTATUS, MSCRATCH, cfg.mstatus_mprv, cfg.sp, cfg.tp, instr); instr.push_back("mret"); gen_section(get_label("ecall_handler", hart), instr); endfunction From f23a2f75dca36e90637857d763e6e5f6836f5595 Mon Sep 17 00:00:00 2001 From: Kulan Palanichamy Date: Wed, 19 Aug 2026 11:23:37 -0700 Subject: [PATCH 3/6] [vendor] Pop the debug frame on debug exceptions 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 --- vendor/google_riscv-dv/src/riscv_debug_rom_gen.sv | 9 +++++++-- ...-debug-frame-on-the-debug-exception-path.patch | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 vendor/patches/google_riscv-dv/0006-pop-the-debug-frame-on-the-debug-exception-path.patch diff --git a/vendor/google_riscv-dv/src/riscv_debug_rom_gen.sv b/vendor/google_riscv-dv/src/riscv_debug_rom_gen.sv index 767540bd38..4d504c8aee 100644 --- a/vendor/google_riscv-dv/src/riscv_debug_rom_gen.sv +++ b/vendor/google_riscv-dv/src/riscv_debug_rom_gen.sv @@ -116,9 +116,14 @@ class riscv_debug_rom_gen extends riscv_asm_program_gen; endfunction // Generate exception handling routine for debug ROM - // TODO(udinator) - remains empty for now, only a DRET + // Return through debug_end, which pops the frame pushed on debug ROM entry. virtual function void gen_debug_exception_handler(); - str = {"dret"}; + if (cfg.gen_debug_section) begin + str = {$sformatf("j %0sdebug_end", hart_prefix(hart))}; + end else begin + // No debug section, so no frame to pop. + str = {"dret"}; + end gen_section($sformatf("%0sdebug_exception", hart_prefix(hart)), str); endfunction diff --git a/vendor/patches/google_riscv-dv/0006-pop-the-debug-frame-on-the-debug-exception-path.patch b/vendor/patches/google_riscv-dv/0006-pop-the-debug-frame-on-the-debug-exception-path.patch new file mode 100644 index 0000000000..c80b9965f5 --- /dev/null +++ b/vendor/patches/google_riscv-dv/0006-pop-the-debug-frame-on-the-debug-exception-path.patch @@ -0,0 +1,15 @@ +--- a/src/riscv_debug_rom_gen.sv ++++ b/src/riscv_debug_rom_gen.sv +@@ -118,5 +118,10 @@ class riscv_debug_rom_gen extends riscv_asm_program_gen; + // Generate exception handling routine for debug ROM +- // TODO(udinator) - remains empty for now, only a DRET ++ // Return through debug_end, which pops the frame pushed on debug ROM entry. + virtual function void gen_debug_exception_handler(); +- str = {"dret"}; ++ if (cfg.gen_debug_section) begin ++ str = {$sformatf("j %0sdebug_end", hart_prefix(hart))}; ++ end else begin ++ // No debug section, so no frame to pop. ++ str = {"dret"}; ++ end + gen_section($sformatf("%0sdebug_exception", hart_prefix(hart)), str); From 1388e8a6c6f8ce33c331af6fde9227d0b86fdc42 Mon Sep 17 00:00:00 2001 From: Kulan Palanichamy Date: Sat, 5 Sep 2026 22:51:20 -0700 Subject: [PATCH 4/6] [vendor] Make the kernel-stack claim atomic 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 --- vendor/google_riscv-dv/src/riscv_instr_pkg.sv | 23 +++++++- ...-the-kernel-stack-frame-claim-atomic.patch | 52 +++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 vendor/patches/google_riscv-dv/0007-make-the-kernel-stack-frame-claim-atomic.patch diff --git a/vendor/google_riscv-dv/src/riscv_instr_pkg.sv b/vendor/google_riscv-dv/src/riscv_instr_pkg.sv index 12f248b74e..922ba7d60c 100644 --- a/vendor/google_riscv-dv/src/riscv_instr_pkg.sv +++ b/vendor/google_riscv-dv/src/riscv_instr_pkg.sv @@ -1381,7 +1381,16 @@ package riscv_instr_pkg; riscv_reg_t tp, ref string instr[$]); string store_instr = (XLEN == 32) ? "sw" : "sd"; - if (scratch inside {implemented_csr}) begin + // On RV32 bare metal, reserve the whole frame with one addi before any store, so a debug + // entry in the middle of the push builds its frame below this one. + bit atomic_claim = (scratch inside {implemented_csr}) && (XLEN == 32) && (SATP_MODE == BARE); + if (atomic_claim) begin + instr.push_back($sformatf("addi x%0d, x%0d, -132", tp, tp)); + // Push USP from gpr.SP onto the kernel stack (the top slot of the frame) + instr.push_back($sformatf("%0s x%0d, 128(x%0d)", store_instr, sp, tp)); + // Move KSP to gpr.SP + instr.push_back($sformatf("add x%0d, x%0d, zero", sp, tp)); + end else if (scratch inside {implemented_csr}) begin // Push USP from gpr.SP onto the kernel stack instr.push_back($sformatf("addi x%0d, x%0d, -4", tp, tp)); instr.push_back($sformatf("%0s x%0d, (x%0d)", store_instr, sp, tp)); @@ -1409,7 +1418,9 @@ package riscv_instr_pkg; end // Push all GPRs (except for x0) to kernel stack // (gpr.SP currently holds the KSP) - instr.push_back($sformatf("addi x%0d, x%0d, -%0d", sp, sp, 32 * (XLEN/8))); + if (!atomic_claim) begin + instr.push_back($sformatf("addi x%0d, x%0d, -%0d", sp, sp, 32 * (XLEN/8))); + end for(int i = 1; i < 32; i++) begin instr.push_back($sformatf("%0s x%0d, %0d(x%0d)", store_instr, i, i * (XLEN/8), sp)); end @@ -1426,12 +1437,20 @@ package riscv_instr_pkg; riscv_reg_t tp, ref string instr[$]); string load_instr = (XLEN == 32) ? "lw" : "ld"; + // Same condition as in push_gpr_to_kernel_stack. + bit atomic_claim = (scratch inside {implemented_csr}) && (XLEN == 32) && (SATP_MODE == BARE); // Move KSP to gpr.SP instr.push_back($sformatf("add x%0d, x%0d, zero", sp, tp)); // Pop GPRs from kernel stack for(int i = 1; i < 32; i++) begin instr.push_back($sformatf("%0s x%0d, %0d(x%0d)", load_instr, i, i * (XLEN/8), sp)); end + if (atomic_claim) begin + // Restore USP, then release the whole frame with one addi. + instr.push_back($sformatf("%0s x%0d, 128(x%0d)", load_instr, sp, tp)); + instr.push_back($sformatf("addi x%0d, x%0d, 132", tp, tp)); + return; + end instr.push_back($sformatf("addi x%0d, x%0d, %0d", sp, sp, 32 * (XLEN/8))); if (scratch inside {implemented_csr}) begin // Move KSP back to gpr.TP diff --git a/vendor/patches/google_riscv-dv/0007-make-the-kernel-stack-frame-claim-atomic.patch b/vendor/patches/google_riscv-dv/0007-make-the-kernel-stack-frame-claim-atomic.patch new file mode 100644 index 0000000000..18b8862827 --- /dev/null +++ b/vendor/patches/google_riscv-dv/0007-make-the-kernel-stack-frame-claim-atomic.patch @@ -0,0 +1,52 @@ +--- a/src/riscv_instr_pkg.sv ++++ b/src/riscv_instr_pkg.sv +@@ -1381,7 +1381,16 @@ package riscv_instr_pkg; + riscv_reg_t tp, + ref string instr[$]); + string store_instr = (XLEN == 32) ? "sw" : "sd"; +- if (scratch inside {implemented_csr}) begin ++ // On RV32 bare metal, reserve the whole frame with one addi before any store, so a debug ++ // entry in the middle of the push builds its frame below this one. ++ bit atomic_claim = (scratch inside {implemented_csr}) && (XLEN == 32) && (SATP_MODE == BARE); ++ if (atomic_claim) begin ++ instr.push_back($sformatf("addi x%0d, x%0d, -132", tp, tp)); ++ // Push USP from gpr.SP onto the kernel stack (the top slot of the frame) ++ instr.push_back($sformatf("%0s x%0d, 128(x%0d)", store_instr, sp, tp)); ++ // Move KSP to gpr.SP ++ instr.push_back($sformatf("add x%0d, x%0d, zero", sp, tp)); ++ end else if (scratch inside {implemented_csr}) begin + // Push USP from gpr.SP onto the kernel stack + instr.push_back($sformatf("addi x%0d, x%0d, -4", tp, tp)); + instr.push_back($sformatf("%0s x%0d, (x%0d)", store_instr, sp, tp)); +@@ -1409,7 +1418,9 @@ package riscv_instr_pkg; + end + // Push all GPRs (except for x0) to kernel stack + // (gpr.SP currently holds the KSP) +- instr.push_back($sformatf("addi x%0d, x%0d, -%0d", sp, sp, 32 * (XLEN/8))); ++ if (!atomic_claim) begin ++ instr.push_back($sformatf("addi x%0d, x%0d, -%0d", sp, sp, 32 * (XLEN/8))); ++ end + for(int i = 1; i < 32; i++) begin + instr.push_back($sformatf("%0s x%0d, %0d(x%0d)", store_instr, i, i * (XLEN/8), sp)); + end +@@ -1426,12 +1437,20 @@ package riscv_instr_pkg; + riscv_reg_t tp, + ref string instr[$]); + string load_instr = (XLEN == 32) ? "lw" : "ld"; ++ // Same condition as in push_gpr_to_kernel_stack. ++ bit atomic_claim = (scratch inside {implemented_csr}) && (XLEN == 32) && (SATP_MODE == BARE); + // Move KSP to gpr.SP + instr.push_back($sformatf("add x%0d, x%0d, zero", sp, tp)); + // Pop GPRs from kernel stack + for(int i = 1; i < 32; i++) begin + instr.push_back($sformatf("%0s x%0d, %0d(x%0d)", load_instr, i, i * (XLEN/8), sp)); + end ++ if (atomic_claim) begin ++ // Restore USP, then release the whole frame with one addi. ++ instr.push_back($sformatf("%0s x%0d, 128(x%0d)", load_instr, sp, tp)); ++ instr.push_back($sformatf("addi x%0d, x%0d, 132", tp, tp)); ++ return; ++ end + instr.push_back($sformatf("addi x%0d, x%0d, %0d", sp, sp, 32 * (XLEN/8))); + if (scratch inside {implemented_csr}) begin + // Move KSP back to gpr.TP From 31543abd653b12f5d0519003be2a26d0e5f048d5 Mon Sep 17 00:00:00 2001 From: Kulan Palanichamy Date: Sat, 5 Sep 2026 17:46:38 -0700 Subject: [PATCH 5/6] [dv] Restrict single-step test CSR writes 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 --- dv/uvm/core_ibex/riscv_dv_extension/testlist.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dv/uvm/core_ibex/riscv_dv_extension/testlist.yaml b/dv/uvm/core_ibex/riscv_dv_extension/testlist.yaml index 17cec06f45..b5466c8b72 100644 --- a/dv/uvm/core_ibex/riscv_dv_extension/testlist.yaml +++ b/dv/uvm/core_ibex/riscv_dv_extension/testlist.yaml @@ -653,11 +653,11 @@ +no_ebreak=0 +no_ecall=0 +no_branch_jump=0 - +instr_cnt=10000 + +instr_cnt=2000 +no_csr_instr=0 +randomize_csr=1 +gen_all_csrs_by_default=1 - +add_csr_write=MSTATUS,MEPC,MCAUSE,MTVAL,0x7c0,0x7c1 + +add_csr_write=0x7c0,0x7c1 +no_fence=0 +no_wfi=0 +num_of_sub_program=1 From 9c6da426054d689ac64c37d7806d35fe8e93bd9b Mon Sep 17 00:00:00 2001 From: Kulan Palanichamy Date: Sat, 5 Sep 2026 22:51:20 -0700 Subject: [PATCH 6/6] [dv] Hold debug_req until the hart has halted 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 --- dv/uvm/core_ibex/tests/core_ibex_seq_lib.sv | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/dv/uvm/core_ibex/tests/core_ibex_seq_lib.sv b/dv/uvm/core_ibex/tests/core_ibex_seq_lib.sv index 97be142bb2..1304acc39d 100644 --- a/dv/uvm/core_ibex/tests/core_ibex_seq_lib.sv +++ b/dv/uvm/core_ibex/tests/core_ibex_seq_lib.sv @@ -175,11 +175,17 @@ class debug_seq extends core_base_seq#(irq_seq_item); `uvm_object_new int unsigned drop_delay = 75; + // Cycles to wait for the core to take the request before giving up. + int unsigned hold_timeout_cycles = 5000; + virtual core_ibex_rvfi_if rvfi_vif; virtual task body(); if (!uvm_config_db#(virtual core_ibex_dut_probe_if)::get(null, "", "dut_if", dut_vif)) begin `uvm_fatal(get_full_name(), "Cannot get dut_if") end + if (!uvm_config_db#(virtual core_ibex_rvfi_if)::get(null, "", "rvfi_if", rvfi_vif)) begin + `uvm_fatal(get_full_name(), "Cannot get rvfi_if") + end dut_vif.dut_cb.debug_req <= 1'b0; super.body(); endtask @@ -188,6 +194,19 @@ class debug_seq extends core_base_seq#(irq_seq_item); `uvm_info(get_full_name(), "Sending debug request", UVM_HIGH) dut_vif.dut_cb.debug_req <= 1'b1; clk_vif.wait_clks(drop_delay); + // Like a debug module holding haltreq, keep the request up until a retirement reports it on + // RVFI. Otherwise a request that wakes the core from WFI can drop before the cosim sees it. + `DV_SPINWAIT_EXIT(begin + wait (dut_vif.dut_cb.debug_mode == 1'b1); + do @(rvfi_vif.monitor_cb); + while (!(rvfi_vif.monitor_cb.valid && rvfi_vif.monitor_cb.ext_debug_req)); + end, + begin + clk_vif.wait_clks(hold_timeout_cycles); + `uvm_error(get_full_name(), + "No retirement reported the debug request before the hold timeout") + end, + "") dut_vif.dut_cb.debug_req <= 1'b0; endtask