From 4ec718b3a1a7f19eb1745d429fa61f50cb54c069 Mon Sep 17 00:00:00 2001 From: Halfdan Bechmann Date: Thu, 24 Mar 2022 13:52:07 +0100 Subject: [PATCH 1/2] Added rvfi_wu and rvfi_sleep signals to rvfi, fixed issue with rvfi_csr_mip_rdata signal Signed-off-by: Halfdan Bechmann --- bhv/cv32e40x_rvfi.sv | 53 +++++++++++++++++++++++++++++--- bhv/cv32e40x_wrapper.sv | 10 +++++- bhv/include/cv32e40x_rvfi_pkg.sv | 7 +++++ bhv/include/cv32e40x_wrapper.vh | 3 ++ sva/cv32e40x_rvfi_sva.sv | 47 +++++++++++++++++++++++++++- 5 files changed, 114 insertions(+), 6 deletions(-) diff --git a/bhv/cv32e40x_rvfi.sv b/bhv/cv32e40x_rvfi.sv index d1a3a768..d5dcd6bd 100644 --- a/bhv/cv32e40x_rvfi.sv +++ b/bhv/cv32e40x_rvfi.sv @@ -74,6 +74,7 @@ module cv32e40x_rvfi input logic wb_valid_i, input logic ebreak_in_wb_i, input logic [31:0] instr_rdata_wb_i, + input logic csr_en_wb_i, // Register writes input logic rf_we_wb_i, input logic [4:0] rf_addr_wb_i, @@ -85,12 +86,20 @@ module cv32e40x_rvfi input privlvl_t priv_lvl_i, - // Controller FSM probe + // Controller FSM probes input ctrl_fsm_t ctrl_fsm_i, + input ctrl_state_e ctrl_fsm_cs_i, + input ctrl_state_e ctrl_fsm_ns_i, input logic pending_single_step_i, input logic single_step_allowed_i, input logic nmi_pending_i, input logic nmi_is_store_i, + input logic pending_debug_i, + input logic debug_mode_q_i, + + // Interrupt Controller probes + input logic irq_wu_ctrl_i, + input logic [4:0] irq_id_ctrl_i, //// CSR Probes //// input jvt_t csr_jvt_n_i, @@ -223,6 +232,9 @@ module cv32e40x_rvfi output logic [ 2:0] rvfi_dbg, output logic [ 0:0] rvfi_dbg_mode, + output rvfi_wu_t rvfi_wu, + output logic [ 0:0] rvfi_sleep, + output logic [ 4:0] rvfi_rd_addr, output logic [31:0] rvfi_rd_wdata, output logic [ 4:0] rvfi_rs1_addr, @@ -514,8 +526,6 @@ module cv32e40x_rvfi logic [31:0] mhpmcounter_h_during_wb; - - logic [63:0] data_wdata_ror; // Intermediate rotate signal, as direct part-select not supported in all tools logic pc_mux_debug; @@ -571,6 +581,34 @@ module cv32e40x_rvfi assign in_trap_clr = wb_valid_i && in_trap[STAGE_WB].intr; + // Store information about wakeup until fist instruction is retired + // This information needs to be stored at the wakeup because other interrupts + // can be triggered between wakeup and the first instruction executed after wakeup + rvfi_wu_t rvfi_wu_next; + logic store_irq_wu_cause; + + assign rvfi_wu_next.wu = rvfi_sleep; // If sleep is still set, it is the firs instruction after wakeup + always_ff @(posedge clk_i or negedge rst_ni) begin + if (!rst_ni) begin + store_irq_wu_cause <= 1'b0; + rvfi_wu_next.interrupt <= 1'b0; + rvfi_wu_next.debug <= 1'b0; + rvfi_wu_next.cause <= '0; + end else begin + // Store wake-up sources when core exits sleep + if ((ctrl_fsm_cs_i == SLEEP) && (ctrl_fsm_ns_i != SLEEP)) begin + rvfi_wu_next.interrupt <= irq_wu_ctrl_i; + store_irq_wu_cause <= irq_wu_ctrl_i; + rvfi_wu_next.debug <= pending_debug_i || debug_mode_q_i; + end + // IRQ cause is flopped and is therefore one cycle behind wakeup triggers + if (store_irq_wu_cause) begin + store_irq_wu_cause <= 1'b0; + rvfi_wu_next.cause <= {'0, irq_id_ctrl_i}; // NMIs will not wake up the core + end + end + end + // Set rvfi_trap for instructions causing exception or debug entry. rvfi_trap_t rvfi_trap_next; @@ -645,6 +683,8 @@ module cv32e40x_rvfi ex_csr_rdata <= '0; rvfi_dbg <= '0; rvfi_dbg_mode <= '0; + rvfi_wu <= '0; + rvfi_sleep <= 1'b0; rvfi_valid <= 1'b0; rvfi_order <= '0; rvfi_insn <= '0; @@ -817,11 +857,16 @@ module cv32e40x_rvfi rvfi_dbg <= debug_cause[STAGE_WB]; rvfi_dbg_mode <= debug_mode [STAGE_WB]; + rvfi_sleep <= (ctrl_fsm_ns_i == SLEEP); + rvfi_wu <= rvfi_wu_next; + // Set expected next PC, half-word aligned // Predict synchronous exceptions and synchronous debug entry in WB to include all causes rvfi_pc_wdata <= (pc_mux_debug || pc_mux_exception) ? branch_addr_n_i & ~32'b1 : (pc_mux_dret) ? csr_dpc_q_i : pc_wdata[STAGE_WB] & ~32'b1; + end else begin // if (wb_valid_i) + rvfi_sleep <= rvfi_sleep; // Keep sleep signal asserted until next valid WB end end @@ -965,7 +1010,7 @@ module cv32e40x_rvfi assign rvfi_csr_wmask_d.mtval = '0; assign ex_csr_rdata_d.mip = csr_mip_q_i; - assign rvfi_csr_rdata_d.mip = ex_csr_rdata.mip; + assign rvfi_csr_rdata_d.mip = csr_en_wb_i ? ex_csr_rdata.mip : csr_mip_q_i; assign rvfi_csr_wdata_d.mip = csr_mip_n_i; assign rvfi_csr_wmask_d.mip = csr_mip_we_i ? '1 : '0; diff --git a/bhv/cv32e40x_wrapper.sv b/bhv/cv32e40x_wrapper.sv index 24ac87c5..9d8661b9 100644 --- a/bhv/cv32e40x_wrapper.sv +++ b/bhv/cv32e40x_wrapper.sv @@ -254,7 +254,7 @@ module cv32e40x_wrapper .branch_taken_in_ex (core_i.controller_i.controller_fsm_i.branch_taken_ex), .exc_cause (core_i.controller_i.controller_fsm_i.exc_cause), // probed controller signals - .ctrl_fsm_ns (core_i.controller_i.controller_fsm_i.ctrl_fsm_ns), + .ctrl_fsm_ns (core_i.controller_i.controller_fsm_i.ctrl_fsm_ns), .ctrl_debug_mode_n (core_i.controller_i.controller_fsm_i.debug_mode_n), .ctrl_pending_debug (core_i.controller_i.controller_fsm_i.pending_debug), .ctrl_debug_allowed (core_i.controller_i.controller_fsm_i.debug_allowed), @@ -339,6 +339,7 @@ module cv32e40x_wrapper .dbg_ack(core_i.dbg_ack), .ebreak_in_wb_i(core_i.controller_i.controller_fsm_i.ebreak_in_wb), .nmi_addr_i(core_i.nmi_addr_i), + .core_sleep_i(core_i.core_sleep_o), .*); `endif // `ifndef COREV_ASSERT_OFF @@ -365,6 +366,7 @@ module cv32e40x_wrapper .wb_valid_i ( core_i.wb_stage_i.wb_valid_o ), .wb_ready_i ( core_i.wb_stage_i.wb_ready_o ), .instr_rdata_wb_i ( core_i.wb_stage_i.ex_wb_pipe_i.instr.bus_resp.rdata ), + .csr_en_wb_i ( core_i.wb_stage_i.ex_wb_pipe_i.csr_en ), .ebreak_in_wb_i ( core_i.controller_i.controller_fsm_i.ebreak_in_wb ), .rs1_addr_id_i ( core_i.register_file_wrapper_i.raddr_i[0] ), @@ -414,10 +416,16 @@ module cv32e40x_wrapper .priv_lvl_i ( PRIV_LVL_M /* Not implemented in cv32e40x */ ), .ctrl_fsm_i ( core_i.ctrl_fsm ), + .ctrl_fsm_cs_i ( core_i.controller_i.controller_fsm_i.ctrl_fsm_cs ), + .ctrl_fsm_ns_i ( core_i.controller_i.controller_fsm_i.ctrl_fsm_ns ), .pending_single_step_i ( core_i.controller_i.controller_fsm_i.pending_single_step ), .single_step_allowed_i ( core_i.controller_i.controller_fsm_i.single_step_allowed ), .nmi_pending_i ( core_i.controller_i.controller_fsm_i.nmi_pending_q ), .nmi_is_store_i ( core_i.controller_i.controller_fsm_i.nmi_is_store_q ), + .pending_debug_i ( core_i.controller_i.controller_fsm_i.pending_debug ), + .debug_mode_q_i ( core_i.controller_i.controller_fsm_i.debug_mode_q ), + .irq_wu_ctrl_i ( core_i.irq_wu_ctrl ), + .irq_id_ctrl_i ( core_i.irq_id_ctrl ), // CSRs .csr_jvt_n_i ( core_i.cs_registers_i.jvt_n ), .csr_jvt_q_i ( core_i.cs_registers_i.jvt_q ), diff --git a/bhv/include/cv32e40x_rvfi_pkg.sv b/bhv/include/cv32e40x_rvfi_pkg.sv index 707f14f4..810c3018 100644 --- a/bhv/include/cv32e40x_rvfi_pkg.sv +++ b/bhv/include/cv32e40x_rvfi_pkg.sv @@ -99,6 +99,13 @@ package cv32e40x_rvfi_pkg; } rvfi_csr_map_t; + typedef struct packed { + logic [10:0] cause; + logic debug; + logic interrupt; + logic wu; + } rvfi_wu_t; + typedef struct packed { logic [10:0] cause; logic interrupt; diff --git a/bhv/include/cv32e40x_wrapper.vh b/bhv/include/cv32e40x_wrapper.vh index 906a77c7..16428ce8 100644 --- a/bhv/include/cv32e40x_wrapper.vh +++ b/bhv/include/cv32e40x_wrapper.vh @@ -40,6 +40,9 @@ .rvfi_dbg(),\ .rvfi_dbg_mode(),\ \ +.rvfi_wu(),\ +.rvfi_sleep(),\ +\ .rvfi_rd_addr(),\ .rvfi_rd_wdata(),\ .rvfi_rs1_addr(),\ diff --git a/sva/cv32e40x_rvfi_sva.sv b/sva/cv32e40x_rvfi_sva.sv index 44456298..dd73de56 100644 --- a/sva/cv32e40x_rvfi_sva.sv +++ b/sva/cv32e40x_rvfi_sva.sv @@ -36,7 +36,11 @@ module cv32e40x_rvfi_sva input rvfi_trap_t rvfi_trap, input logic [2:0] rvfi_dbg, - input ctrl_fsm_t ctrl_fsm_i, + input rvfi_wu_t rvfi_wu, + input logic rvfi_sleep, + + input logic core_sleep_i, + input ctrl_fsm_t ctrl_fsm_i, input logic [31:0] rvfi_csr_dcsr_rdata, input logic [31:0] rvfi_csr_mcause_rdata, input logic [31:0] rvfi_pc_rdata, @@ -168,6 +172,47 @@ module cv32e40x_rvfi_sva ((rvfi_csr_mcause_rdata[7:0] == INT_CAUSE_LSU_LOAD_FAULT) || (rvfi_csr_mcause_rdata[7:0] == INT_CAUSE_LSU_STORE_FAULT))) else `uvm_error("rvfi", "dcsr.nmip not followed by rvfi_intr and NMI handler") + + // Check that rvfi_sleep is always followed by rvfi_wu + a_always_rvfi_wu_after_rvfi_sleep: + assert property (@(posedge clk_i) disable iff (!rst_ni) + s_goto_next_rvfi_valid(rvfi_sleep) |-> rvfi_wu.wu) + else `uvm_error("rvfi", "rvfi_sleep asserted without being followed by rvfi_wu") + + + logic previous_instruction_was_sleep; + always_ff @(posedge clk_i or negedge rst_ni) begin + if (!rst_ni) begin + previous_instruction_was_sleep <= 1'b0; + end else begin + if ( rvfi_valid ) begin + // Store sleep signal from previous valid instruction + previous_instruction_was_sleep <= rvfi_sleep; + end + end + end + + // Check that rvfi_wu is only asserted after fist having signalled rvfi_sleep + a_always_rvfi_sleep_before_rvfi_wu: + assert property (@(posedge clk_i) disable iff (!rst_ni) + (rvfi_wu.wu && rvfi_valid) |-> previous_instruction_was_sleep) + else `uvm_error("rvfi", "rvfi_wu asserted without a previous rvfi_sleep") + + // Check that the last instruction before core_sleep_o had rvfi_sleep set + a_always_core_sleep_always_preceeded_by_instr_with_rvfi_sleep: + assert property (@(posedge clk_i) disable iff (!rst_ni) + core_sleep_i |-> previous_instruction_was_sleep) + else `uvm_error("rvfi", "The instruction before asserted core_sleep_o did not have rvfi_sleep set") + + // Check that rvfi_sleep is always set when the core is sleeping + // This assertion is stricter than needed (behavior only specified for valid insructions) + // but is added as a sanity check for this implementation + a_always_rvfi_sleep_when_core_sleep: + assert property (@(posedge clk_i) disable iff (!rst_ni) + core_sleep_i |-> rvfi_sleep) + else `uvm_error("rvfi", "rvfi_sleep not asserted even though core_sleep_o was set") + + endmodule : cv32e40x_rvfi_sva From affb0702cf262dbab9bd427e3eb0e888150ef723 Mon Sep 17 00:00:00 2001 From: Halfdan Bechmann Date: Mon, 28 Mar 2022 17:47:52 +0200 Subject: [PATCH 2/2] Account for multicycle WFIs in rvfi Signed-off-by: Halfdan Bechmann --- bhv/cv32e40x_rvfi.sv | 37 +++++++++++++++++++++++++++++-------- bhv/cv32e40x_wrapper.sv | 2 ++ sva/cv32e40x_rvfi_sva.sv | 8 ++++---- 3 files changed, 35 insertions(+), 12 deletions(-) diff --git a/bhv/cv32e40x_rvfi.sv b/bhv/cv32e40x_rvfi.sv index d5dcd6bd..995d7112 100644 --- a/bhv/cv32e40x_rvfi.sv +++ b/bhv/cv32e40x_rvfi.sv @@ -75,6 +75,7 @@ module cv32e40x_rvfi input logic ebreak_in_wb_i, input logic [31:0] instr_rdata_wb_i, input logic csr_en_wb_i, + input logic sys_wfi_insn_wb_i, // Register writes input logic rf_we_wb_i, input logic [4:0] rf_addr_wb_i, @@ -98,6 +99,7 @@ module cv32e40x_rvfi input logic debug_mode_q_i, // Interrupt Controller probes + input logic [31:0] irq_i, input logic irq_wu_ctrl_i, input logic [4:0] irq_id_ctrl_i, @@ -528,6 +530,9 @@ module cv32e40x_rvfi logic [63:0] data_wdata_ror; // Intermediate rotate signal, as direct part-select not supported in all tools + logic wb_valid_wfi_delayed; + logic wb_valid_adjusted; + logic pc_mux_debug; logic pc_mux_dret; logic pc_mux_exception; @@ -578,23 +583,25 @@ module cv32e40x_rvfi logic in_trap_clr; // Clear in trap pipeline when it reaches rvfi_intr // This is done to avoid reporting already signaled triggers as supressed during by debug - assign in_trap_clr = wb_valid_i && in_trap[STAGE_WB].intr; + assign in_trap_clr = wb_valid_adjusted && in_trap[STAGE_WB].intr; - // Store information about wakeup until fist instruction is retired + // Store information about wakeup until first instruction is retired // This information needs to be stored at the wakeup because other interrupts // can be triggered between wakeup and the first instruction executed after wakeup rvfi_wu_t rvfi_wu_next; logic store_irq_wu_cause; - assign rvfi_wu_next.wu = rvfi_sleep; // If sleep is still set, it is the firs instruction after wakeup + assign rvfi_wu_next.wu = rvfi_sleep; // If sleep is still set, it is the first instruction after wakeup always_ff @(posedge clk_i or negedge rst_ni) begin if (!rst_ni) begin + wb_valid_wfi_delayed <= 1'b0; store_irq_wu_cause <= 1'b0; rvfi_wu_next.interrupt <= 1'b0; rvfi_wu_next.debug <= 1'b0; rvfi_wu_next.cause <= '0; end else begin + wb_valid_wfi_delayed <= wb_valid_i && (ctrl_fsm_ns_i == SLEEP); // Store wake-up sources when core exits sleep if ((ctrl_fsm_cs_i == SLEEP) && (ctrl_fsm_ns_i != SLEEP)) begin rvfi_wu_next.interrupt <= irq_wu_ctrl_i; @@ -662,6 +669,15 @@ module cv32e40x_rvfi rvfi_trap_next.trap = rvfi_trap_next.exception || rvfi_trap_next.debug; end + + // WFI instructions can use two cycles in WB if there are no flopped pending wakeup sources. + // In this case their retirement happens in the second cycle, but becuase it makes no difference functionally in rtl, + // wb_valid is set in the first WFI cycle to avoid introducing support logic for a multicycle WB stage. + // Moving wb_valid to second cycle of WFI instructions in these cases is therefore needed. + assign wb_valid_adjusted = (sys_wfi_insn_wb_i && ((ctrl_fsm_ns_i == SLEEP) || (ctrl_fsm_cs_i == SLEEP))) ? + wb_valid_wfi_delayed : + wb_valid_i; + // Pipeline stage model // always_ff @(posedge clk_i or negedge rst_ni) begin @@ -824,8 +840,9 @@ module cv32e40x_rvfi //// WB Stage //// - rvfi_valid <= wb_valid_i; - if (wb_valid_i) begin + rvfi_valid <= wb_valid_adjusted; + if (wb_valid_adjusted) begin + rvfi_order <= rvfi_order + 64'b1; rvfi_pc_rdata <= pc_wb_i; rvfi_insn <= instr_rdata_wb_i; @@ -865,8 +882,8 @@ module cv32e40x_rvfi rvfi_pc_wdata <= (pc_mux_debug || pc_mux_exception) ? branch_addr_n_i & ~32'b1 : (pc_mux_dret) ? csr_dpc_q_i : pc_wdata[STAGE_WB] & ~32'b1; - end else begin // if (wb_valid_i) - rvfi_sleep <= rvfi_sleep; // Keep sleep signal asserted until next valid WB + end else begin + rvfi_sleep <= rvfi_sleep; // Keep sleep signal asserted until next valid WB end end @@ -1009,8 +1026,12 @@ module cv32e40x_rvfi assign rvfi_csr_wdata_d.mtval = '0; // Not implemented, read 0 assign rvfi_csr_wmask_d.mtval = '0; + // MIP is read in EX by CSR instructions, evaluated combinatorically in WB by the WFI instruction, + // and is evaluated in WB for all other instructions assign ex_csr_rdata_d.mip = csr_mip_q_i; - assign rvfi_csr_rdata_d.mip = csr_en_wb_i ? ex_csr_rdata.mip : csr_mip_q_i; + assign rvfi_csr_rdata_d.mip = csr_en_wb_i ? ex_csr_rdata.mip : + sys_wfi_insn_wb_i ? irq_i : + csr_mip_q_i; assign rvfi_csr_wdata_d.mip = csr_mip_n_i; assign rvfi_csr_wmask_d.mip = csr_mip_we_i ? '1 : '0; diff --git a/bhv/cv32e40x_wrapper.sv b/bhv/cv32e40x_wrapper.sv index 9d8661b9..552939a1 100644 --- a/bhv/cv32e40x_wrapper.sv +++ b/bhv/cv32e40x_wrapper.sv @@ -367,6 +367,7 @@ module cv32e40x_wrapper .wb_ready_i ( core_i.wb_stage_i.wb_ready_o ), .instr_rdata_wb_i ( core_i.wb_stage_i.ex_wb_pipe_i.instr.bus_resp.rdata ), .csr_en_wb_i ( core_i.wb_stage_i.ex_wb_pipe_i.csr_en ), + .sys_wfi_insn_wb_i ( core_i.wb_stage_i.ex_wb_pipe_i.sys_wfi_insn ), .ebreak_in_wb_i ( core_i.controller_i.controller_fsm_i.ebreak_in_wb ), .rs1_addr_id_i ( core_i.register_file_wrapper_i.raddr_i[0] ), @@ -424,6 +425,7 @@ module cv32e40x_wrapper .nmi_is_store_i ( core_i.controller_i.controller_fsm_i.nmi_is_store_q ), .pending_debug_i ( core_i.controller_i.controller_fsm_i.pending_debug ), .debug_mode_q_i ( core_i.controller_i.controller_fsm_i.debug_mode_q ), + .irq_i ( core_i.irq_i & IRQ_MASK ), .irq_wu_ctrl_i ( core_i.irq_wu_ctrl ), .irq_id_ctrl_i ( core_i.irq_id_ctrl ), // CSRs diff --git a/sva/cv32e40x_rvfi_sva.sv b/sva/cv32e40x_rvfi_sva.sv index dd73de56..835ae90e 100644 --- a/sva/cv32e40x_rvfi_sva.sv +++ b/sva/cv32e40x_rvfi_sva.sv @@ -198,11 +198,11 @@ module cv32e40x_rvfi_sva (rvfi_wu.wu && rvfi_valid) |-> previous_instruction_was_sleep) else `uvm_error("rvfi", "rvfi_wu asserted without a previous rvfi_sleep") - // Check that the last instruction before core_sleep_o had rvfi_sleep set - a_always_core_sleep_always_preceeded_by_instr_with_rvfi_sleep: + // Check that the last instruction before (or at the same time as) core_sleep_o goes high has rvfi_sleep set + a_core_sleep_always_preceeded_by_instr_with_rvfi_sleep: assert property (@(posedge clk_i) disable iff (!rst_ni) - core_sleep_i |-> previous_instruction_was_sleep) - else `uvm_error("rvfi", "The instruction before asserted core_sleep_o did not have rvfi_sleep set") + core_sleep_i |-> (rvfi_sleep || previous_instruction_was_sleep)) + else `uvm_error("rvfi", "The last instruction before asserted core_sleep_o did not have rvfi_sleep set") // Check that rvfi_sleep is always set when the core is sleeping // This assertion is stricter than needed (behavior only specified for valid insructions)