From 750059197265776471d4f329f1f34b9588d214f4 Mon Sep 17 00:00:00 2001 From: Oystein Knauserud Date: Wed, 19 Apr 2023 11:45:37 +0200 Subject: [PATCH] For for issue #355 on cv32e40s. Changed how RVFI backpropagates "in_trap" upon debug entry. Previously the backpropagation could cause already signaled "in_trap" to be backpropagated, causing multiple "rvfi_intr" for the same trap. Signed-off-by: Oystein Knauserud --- bhv/cv32e40x_rvfi.sv | 29 +++++++++++++++++++++------- bhv/cv32e40x_wrapper.sv | 1 + sva/cv32e40x_rvfi_sva.sv | 41 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/bhv/cv32e40x_rvfi.sv b/bhv/cv32e40x_rvfi.sv index 14cf58b5..11597a8a 100644 --- a/bhv/cv32e40x_rvfi.sv +++ b/bhv/cv32e40x_rvfi.sv @@ -978,7 +978,9 @@ module cv32e40x_rvfi // but we still need the in_trap attached to the pointer target, which is // only fetched when the CLIC pointer is in ID. Thus we must not clear in_trap // when the pointer goes from IF to ID. - if ((last_op_if_i || abort_op_if_i) && !clic_ptr_if_i) begin + // Any aborted operation will not cause any other side effects than taking an exception, and + // thus that is the last part of an operation, leading to clearing in_trap and debug cause for STAGE_IF; + if ((last_op_if_i && !clic_ptr_if_i) || abort_op_if_i) begin in_trap [STAGE_IF] <= 1'b0; debug_cause[STAGE_IF] <= '0; end @@ -1003,12 +1005,25 @@ module cv32e40x_rvfi // If there is a trap in the pipeline when debug is taken, the trap will be suppressed but the side-effects will not. // The succeeding instruction therefore needs to re-trigger the intr signals if it it did not reach the rvfi output. - // When in_trap_clr is set, the in_trap[STAGE_WB] reached the RVFI outputs and we must not propagate it back to the in_trap[STAGE_IF] - in_trap[STAGE_IF] <= in_trap[STAGE_IF].intr ? in_trap[STAGE_IF] : - in_trap[STAGE_ID].intr ? in_trap[STAGE_ID] : - in_trap[STAGE_EX].intr ? in_trap[STAGE_EX] : - !in_trap_clr ? in_trap[STAGE_WB] : - '0; + // + // in_trap is attached to the first instruction of interrupt or exception handlers. If such an instruction is killed by + // an external haltrequest, the above comment holds and the in_trap info from the pipeline must be backpropagated to the IF stage. + // If however the debug entry is due to a synchronous exception, the first handler instruction must have reached WB. For most synchronous + // debug entries (except single step due to acking an interrupt) rvfi_valid == 1, thus the in_trap reached RVFI outputs and does not need to backpropagate fo IF. + if (ctrl_fsm_i.debug_cause == DBG_CAUSE_HALTREQ) begin + in_trap[STAGE_IF] <= in_trap[STAGE_IF].intr ? in_trap[STAGE_IF] : + in_trap[STAGE_ID].intr ? in_trap[STAGE_ID] : + in_trap[STAGE_EX].intr ? in_trap[STAGE_EX] : + in_trap[STAGE_WB]; + + end else begin + // Clear in_trap[STAGE_IF] if the handler instruction reached WB without being killed (trigger match, ebreak or stepping) + // In case of taking single step debug due to acking an interrupt or NMI, the in_trap_clr will be zero and the the trap information + // must be retained in the STAGE_IF to be communicated on RVFI for the first debug instruction. + if (in_trap_clr) begin + in_trap[STAGE_IF] <= '0; + end + end // In case the first instruction during debug mode gets an exception, if_stage will be killed and the clearing // of debug_cause due to last_op_if_i during (if_valid && id_ready) may never happen. This will lead to a wrong // value of debug_cause on RVFI outputs. To avoid this, debug_cause is cleared if IF stage is killed due to an exception. diff --git a/bhv/cv32e40x_wrapper.sv b/bhv/cv32e40x_wrapper.sv index c4e9d8c8..05dd95ab 100644 --- a/bhv/cv32e40x_wrapper.sv +++ b/bhv/cv32e40x_wrapper.sv @@ -488,6 +488,7 @@ endgenerate .lsu_atomic_wb_i (core_i.lsu_atomic_wb), .lsu_en_wb_i (core_i.ex_wb_pipe.lsu_en), .lsu_split_q_wb_i (core_i.load_store_unit_i.split_q), + .pc_ex_i (core_i.id_ex_pipe.pc), .*); `endif // `ifndef COREV_ASSERT_OFF diff --git a/sva/cv32e40x_rvfi_sva.sv b/sva/cv32e40x_rvfi_sva.sv index 72793175..ba0b6971 100644 --- a/sva/cv32e40x_rvfi_sva.sv +++ b/sva/cv32e40x_rvfi_sva.sv @@ -57,6 +57,9 @@ module cv32e40x_rvfi_sva input logic if_valid_i, input logic id_ready_i, input logic [31:0] pc_if_i, + input logic [31:0] pc_id_i, + input logic [31:0] pc_ex_i, + input logic [31:0] pc_wb_i, input inst_resp_t prefetch_instr_if_i, input logic prefetch_compressed_if_i, input logic [31:0] prefetch_addr_if_i, @@ -71,7 +74,10 @@ module cv32e40x_rvfi_sva input lsu_atomic_e lsu_atomic_wb_i, input logic lsu_en_wb_i, input logic lsu_split_q_wb_i, - input logic pc_mux_exception + input logic pc_mux_exception, + input logic pc_mux_debug, + input logic in_trap_clr, + input logic wb_valid_lastop ); @@ -312,6 +318,39 @@ if (A_EXT == A) begin else `uvm_error("rvfi", "Exception on misaligned AMO* atomic instruction did not set correct cause_type in rvfi_trap") end + // An external haltrequest will kill all pipeline stages. Check that in_trap_clr is never true for such debug entries. + // in_trap_clr == 1 means that the rvfi_intr associated with the first instruction of an interrupt- or exception handler + // has reached RVFI outputs, and any internal tracking of this state within RVFI can be cleared. Killing all stages makes this + // tracked in_trap to not reach RVFI outputs. + a_no_clr_on_haltreq: + assert property (@(posedge clk_i) disable iff (!rst_ni) + (pc_mux_debug) && + (ctrl_fsm_i.debug_cause == DBG_CAUSE_HALTREQ) + |-> + !in_trap_clr) + else `uvm_error("rvfi", "in_trap_clr is active when going to debug due to external haltrequest") + + // If the WB stage of RVFI contains an in_trap at the time of synchronous debug entry, the in_trap_clr + // must be 1, signaling that the trap info reached rvfi_intr and any internal tracking must be cleared. + a_clr_on_sync_dbg_entry: + assert property (@(posedge clk_i) disable iff (!rst_ni) + (pc_mux_debug) && + in_trap[STAGE_WB].intr && + (ctrl_fsm_i.debug_cause != DBG_CAUSE_HALTREQ) + |-> + in_trap_clr) + else `uvm_error("rvfi", "in_trap_clr not active when going to debug due to a synchronous cause") + + // Check that no other in_trap than the one in WB can be present in the pipeline at the same time. + a_single_in_trap_tracked: + assert property (@(posedge clk_i) disable iff (!rst_ni) + in_trap_clr + |-> + !((in_trap[STAGE_EX] && (pc_wb_i != pc_ex_i)) || + (in_trap[STAGE_ID] && (pc_wb_i != pc_id_i)) || + (in_trap[STAGE_IF] && (pc_wb_i != pc_if_i)))) + else `uvm_error("rvfi", "More than one in_trap at the same time") + /* TODO: Add back in. Currently, the alignment buffer can interpret pointers as compressed instructions and pass on two "instructions" from the IF stage. cv32e40x_rvfi_instr_obi will not be in sync with the alignment buffer until this is fixed. See https://github.com/openhwgroup/cv32e40x/issues/704