From 395589d34197b425d89226c590eb1e80db7f6863 Mon Sep 17 00:00:00 2001 From: Adam Surgenor Date: Sun, 24 May 2026 17:51:51 +0100 Subject: [PATCH 1/2] [rtl] Decoder: don't mask csr_access_o in illegal_insn block The current decoder masks csr_access_o = 1'b0 inside the illegal_insn catch-all. That mask is a functional no-op (downstream gating already prevents CSR side-effects on illegal instructions) but creates a long combinational arc on the longest topological path: decoder.illegal_insn -> decoder.csr_access_o -> cs_registers.illegal_csr_insn_o -> id_stage.illegal_insn_o (OR-merge) -> controller.exc_req_d -> id_stage.instr_kill -> instr_executing -> LSU FSM -> id_stage.stall_mem -> en_wb_o -> instr_id_done_o -> cs_registers.csr_op_en_i -> id_stage.csr_pipe_flush -> controller.special_req -> controller.halt_if -> id_in_ready_o -> ctrl_fsm_ns Functional argument (see code comment): csr_op_en_o already AND-gates csr_access_o with instr_executing & instr_id_done_o, and instr_executing is forced low whenever an illegal_insn raises id_exception, so no CSR side-effect can occur. The illegal_insn_dec=1 case is already OR-merged into illegal_insn_o regardless of what illegal_csr_insn_o reports. Combinational restructuring (don't-care simplification); no register added, no pipeline-stage change, no architectural side-effect change. SECURITY DISCLOSURE: leaving csr_access_o asserted for an illegal instruction means cs_registers performs its internal address decode for the illegal opcode's CSR field even though no architectural CSR side-effect occurs. In threat models that include timing or power side-channels on illegal-instruction handling, this is a side-channel surface that the original mask did not expose. See code comment. Reviewers in security-sensitive deployments (e.g. OpenTitan) may prefer to retain the mask and accept the gate cost. Single-block edit; +24 / -1 lines in ibex_decoder.sv. --- rtl/ibex_decoder.sv | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/rtl/ibex_decoder.sv b/rtl/ibex_decoder.sv index 843eb05f05..324497086b 100644 --- a/rtl/ibex_decoder.sv +++ b/rtl/ibex_decoder.sv @@ -662,7 +662,25 @@ module ibex_decoder #( jump_in_dec_o = 1'b0; jump_set_o = 1'b0; branch_in_dec_o = 1'b0; - csr_access_o = 1'b0; + // csr_access_o intentionally NOT masked here: doing so creates a + // critical-path arc from decoder/illegal_insn through + // ibex_cs_registers/illegal_csr_insn_o back into id_stage/illegal_insn_o. + // Functional safety is preserved by downstream gating: + // * csr_op_en_o = csr_access_o & instr_executing & instr_id_done_o, + // and instr_executing is forced low whenever an illegal_insn raises + // id_exception, so no CSR side-effect can occur. + // * Any illegal_insn_dec=1 case is OR-merged into illegal_insn_o + // regardless of what illegal_csr_insn_o reports. + // + // SECURITY NOTE: leaving csr_access_o asserted for an illegal + // instruction means cs_registers performs its internal address decode + // for the illegal opcode's CSR field even though no architectural CSR + // side-effect occurs (csr_op_en_o is gated low). In threat models that + // include timing or power side-channels on illegal-instruction handling, + // this is a side-channel surface that the original csr_access_o = 1'b0 + // mask did not expose. Deployments where this is a concern (e.g. + // OpenTitan-style security-sensitive cores) may prefer to retain the + // mask and accept the gate cost. end end From 43bc0443666776929670ed4b3a615160a6cff55c Mon Sep 17 00:00:00 2001 From: Adam Surgenor Date: Sun, 24 May 2026 17:52:12 +0100 Subject: [PATCH 2/2] [rtl] Controller: drop redundant instr_valid_i qualifier on csr_pipe_flush csr_pipe_flush is currently computed as assign csr_pipe_flush = csr_pipe_flush_i & instr_valid_i; The & instr_valid_i is redundant: csr_pipe_flush_i is already qualified by instr_valid_i at its source. id_stage builds csr_op_en_o (which feeds csr_pipe_flush_i upstream) as csr_op_en_o = csr_access_o & instr_executing & instr_id_done_o and instr_executing already AND-includes instr_valid_i. So the AND here is a logical identity (x & 1 = x in the cases where x can be 1). Removing the redundant gate shortens the chain csr_op_en_o -> csr_pipe_flush_i -> csr_pipe_flush -> special_req_flush_only -> special_req -> halt_if -> id_in_ready_o -> ctrl_fsm_ns which is the dominant post-stall_id portion of the longest topological path on the small-config Ibex. Combinational restructuring only; no new register, no pipeline-stage change, no functional difference. Single-line edit (plus comment); +10 / -1 lines in ibex_controller.sv. --- rtl/ibex_controller.sv | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/rtl/ibex_controller.sv b/rtl/ibex_controller.sv index 1803c44d7c..9d37a259ff 100644 --- a/rtl/ibex_controller.sv +++ b/rtl/ibex_controller.sv @@ -194,7 +194,17 @@ module ibex_controller #( assign dret_insn = dret_insn_i & instr_valid_i; assign wfi_insn = wfi_insn_i & instr_valid_i; assign ebrk_insn = ebrk_insn_i & instr_valid_i; - assign csr_pipe_flush = csr_pipe_flush_i & instr_valid_i; + // csr_pipe_flush_i is already qualified by instr_valid_i at the source + // (id_stage builds it from csr_op_en_o = csr_access_o & instr_executing & + // instr_id_done_o, and instr_executing already AND-includes instr_valid_i), + // so the extra `& instr_valid_i` here is redundant. Dropping it removes one + // gate from the long combinational chain + // csr_op_en_o -> csr_pipe_flush_i -> csr_pipe_flush -> + // special_req_flush_only -> special_req -> halt_if -> + // id_in_ready_o -> ctrl_fsm_ns + // which is the dominant post-stall_id portion of the longest topological + // path on the small-config Ibex. + assign csr_pipe_flush = csr_pipe_flush_i; assign instr_fetch_err = instr_fetch_err_i & instr_valid_i; // This is recorded in the illegal_insn_q flop to help timing. Specifically