Bugfix: mnxti data forwarding causing wrong operand values

bhv/cv32e40x_wrapper.sv

@@ -259,6 +259,13 @@ module cv32e40x_wrapper
                 .id_stage_multi_cycle_id_stall    (core_i.id_stage_i.multi_cycle_id_stall),
 
                 .id_stage_id_valid                (core_i.id_stage_i.id_valid_o),
+                .alu_op_a_mux_sel_id_i            (core_i.id_stage_i.alu_op_a_mux_sel),
+                .alu_op_b_mux_sel_id_i            (core_i.id_stage_i.alu_op_b_mux_sel),
+                .operand_a_id_i                   (core_i.id_stage_i.operand_a),
+                .operand_b_id_i                   (core_i.id_stage_i.operand_b),
+                .jalr_fw_id_i                     (core_i.id_stage_i.jalr_fw),
+                .alu_en_raw_id_i                  (core_i.alu_en_raw_id),
+                .alu_jmpr_id_i                    (core_i.alu_jmpr_id),
                 .irq_ack                          (core_i.irq_ack),
                 .*);
 
@@ -470,8 +477,8 @@ module cv32e40x_wrapper
          .csr_mip_q_i              ( core_i.cs_registers_i.mip_i                                          ),
          .csr_mip_we_i             ( core_i.cs_registers_i.csr_we_int &&
                                      (core_i.cs_registers_i.csr_waddr == CSR_MIP)                         ),
-         .csr_mnxti_n_i            ( core_i.cs_registers_i.mnxti_n                                        ),
-         .csr_mnxti_q_i            ( core_i.cs_registers_i.mnxti_q                                        ),
+         .csr_mnxti_n_i            ( '0/*todo: handle mnxti and rvfi*/                                    ),
+         .csr_mnxti_q_i            ( '0/*todo: handle mnxti and rvfi*/                                    ),
          .csr_mnxti_we_i           ( core_i.cs_registers_i.mnxti_we                                       ),
          .csr_mintstatus_n_i       ( core_i.cs_registers_i.mintstatus_n                                   ),
          .csr_mintstatus_q_i       ( core_i.cs_registers_i.mintstatus_q                                   ),

rtl/cv32e40x_controller.sv

@@ -89,6 +89,7 @@ module cv32e40x_controller import cv32e40x_pkg::*;
 
   // CSR raddr in ex
   input  logic        csr_counter_read_i,         // A performance counter is read in CSR (EX)
+  input  logic        csr_mnxti_read_i,           // MNXTI is read in CSR (EX)
 
   input logic [REGFILE_NUM_READ_PORTS-1:0] rf_re_id_i,
   input rf_addr_t     rf_raddr_id_i[REGFILE_NUM_READ_PORTS],
@@ -219,6 +220,7 @@ module cv32e40x_controller import cv32e40x_pkg::*;
 
     // From EX
     .csr_counter_read_i         ( csr_counter_read_i       ),
+    .csr_mnxti_read_i           ( csr_mnxti_read_i         ),
 
     // From WB
     .wb_ready_i                 ( wb_ready_i               ),

rtl/cv32e40x_controller_bypass.sv

@@ -54,6 +54,7 @@ module cv32e40x_controller_bypass import cv32e40x_pkg::*;
 
   // From EX
   input  logic        csr_counter_read_i,         // CSR is reading a counter (EX).
+  input  logic        csr_mnxti_read_i,           // CSR is reading mnxti (EX)
 
   // From WB
   input  logic        wb_ready_i,                 // WB stage is ready
@@ -129,6 +130,12 @@ module cv32e40x_controller_bypass import cv32e40x_pkg::*;
   // Prevent load/store following a WFI in the pipeline
   assign ctrl_byp_o.wfi_stall = (id_ex_pipe_i.sys_en && id_ex_pipe_i.sys_wfi_insn && id_ex_pipe_i.instr_valid);
 
+  // Stall ID when mnxti CSR is accessed in EX
+  // This is needed because the data bypass from EX uses csr_rdata, and for mnxti this is actually mstatus and not the result
+  // that will be written to the register file. Could be optimized to only stall when the result from the CSR instruction is used in ID,
+  // but the common usecase is a CSR access followed by a branch using the mnxti result in the RF, so it would likely stall in most cases anyway.
+  assign ctrl_byp_o.mnxti_stall = csr_mnxti_read_i;
+
   genvar i;
   generate
     for(i=0; i<REGFILE_NUM_READ_PORTS; i++) begin : gen_forward_signals
@@ -175,7 +182,13 @@ module cv32e40x_controller_bypass import cv32e40x_pkg::*;
 
     // Stall because of jalr path. Stall if a result is to be forwarded to the PC except if result from WB is an ALU result.
     // No need to deassert anything in ID as ID stage is stalled anyway. alu_jmpr_id_i implies rf_re_id_i[0].
-    if (alu_jmpr_id_i && alu_en_raw_id_i && ((rf_we_wb && rf_rd_wb_jalr_match && lsu_en_wb) || (rf_we_ex && rf_rd_ex_jalr_match))) begin
+    // Also stalling when a CSR access to MNXTI is in WB. This is because the jalr forward uses the ex_wb_pipe.rf_wdata as data,
+    // and forwarding the CSR result (pointer address for mnxti) would need the forward mux to also include the pointer address from cs_registers.
+    // Cannot use wb_stage.rf_wdata_o due to the timing path from the data OBI.
+    if ((alu_jmpr_id_i && alu_en_raw_id_i) &&
+         ((rf_we_wb && rf_rd_wb_jalr_match && lsu_en_wb) ||
+          (rf_we_wb && rf_rd_wb_jalr_match && (ex_wb_pipe_i.csr_mnxti_access && ex_wb_pipe_i.csr_en)) ||
+          (rf_we_ex && rf_rd_ex_jalr_match))) begin
       ctrl_byp_o.jalr_stall = 1'b1;
     end else begin
       ctrl_byp_o.jalr_stall = 1'b0;

rtl/cv32e40x_controller_fsm.sv

@@ -469,7 +469,7 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
     // ID stage is halted for regular stalls (i.e. stalls for which the instruction
     // currently in ID is not ready to be issued yet). Also halted if interrupt or debug pending
     // but not allowed to be taken. This is to create an interruptible bubble in WB.
-    ctrl_fsm_o.halt_id = ctrl_byp_i.jalr_stall || ctrl_byp_i.load_stall || ctrl_byp_i.csr_stall || ctrl_byp_i.wfi_stall ||
+    ctrl_fsm_o.halt_id = ctrl_byp_i.jalr_stall || ctrl_byp_i.load_stall || ctrl_byp_i.csr_stall || ctrl_byp_i.wfi_stall || ctrl_byp_i.mnxti_stall ||
                          (pending_interrupt && !interrupt_allowed) ||
                          (pending_debug && !debug_allowed) ||
                          (pending_nmi && !nmi_allowed) ||

rtl/cv32e40x_core.sv

@@ -201,6 +201,7 @@ module cv32e40x_core import cv32e40x_pkg::*;
 
   logic [31:0] csr_rdata;
   logic csr_counter_read;
+  logic csr_mnxti_read;
 
   // CLIC signals for returning pointer addresses
   // when mnxti is accessed
@@ -515,6 +516,7 @@ module cv32e40x_core import cv32e40x_pkg::*;
     // CSR interface
     .csr_rdata_i                ( csr_rdata                    ),
     .csr_illegal_i              ( csr_illegal                  ),
+    .csr_mnxti_read_i           ( csr_mnxti_read               ),
 
     // Branch decision
     .branch_decision_o          ( branch_decision_ex           ),
@@ -705,6 +707,7 @@ module cv32e40x_core import cv32e40x_pkg::*;
 
     // Raddr from first stage (EX)
     .csr_counter_read_o         ( csr_counter_read       ),
+    .csr_mnxti_read_o           ( csr_mnxti_read         ),
 
     // Interrupt related control signals
     .mie_o                      ( mie                    ),
@@ -756,6 +759,7 @@ module cv32e40x_core import cv32e40x_pkg::*;
     .id_ex_pipe_i                   ( id_ex_pipe             ),
 
     .csr_counter_read_i             ( csr_counter_read       ),
+    .csr_mnxti_read_i               ( csr_mnxti_read         ),
 
     // From EX/WB pipeline
     .ex_wb_pipe_i                   ( ex_wb_pipe             ),
@@ -899,6 +903,11 @@ module cv32e40x_core import cv32e40x_pkg::*;
       // CLIC shv not used in basic mode
       assign irq_clic_shv = 1'b0;
       assign irq_clic_level = 8'h00;
+
+      // CLIC mnxti not used in basic mode
+      assign mnxti_irq_pending = 1'b0;
+      assign mnxti_irq_id      = '0;
+      assign mnxti_irq_level   = '0;
     end
   endgenerate
 

rtl/cv32e40x_cs_registers.sv

@@ -77,6 +77,7 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
 
   // To controller bypass logic
   output logic            csr_counter_read_o,
+  output logic            csr_mnxti_read_o,
 
   // Interface to registers (SRAM like)
   output logic [31:0]     csr_rdata_o,
@@ -275,8 +276,9 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
   // read logic
   always_comb
   begin
-    illegal_csr_read = 1'b0;
+    illegal_csr_read   = 1'b0;
     csr_counter_read_o = 1'b0;
+    csr_mnxti_read_o   = 1'b0;
 
     case (csr_raddr)
       // jvt: Jump vector table
@@ -361,6 +363,7 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
           // For mnxti, this is actually mstatus. The value written back to the GPR will be the address of
           // the function pointer to the interrupt handler. This is muxed in the WB stage.
           csr_rdata_int = mstatus_q;
+          csr_mnxti_read_o = 1'b1;
         end else begin
           csr_rdata_int    = '0;
           illegal_csr_read = 1'b1;

rtl/cv32e40x_ex_stage.sv

@@ -45,6 +45,7 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;
   // CSR interface
   input  logic [31:0] csr_rdata_i,
   input  logic        csr_illegal_i,
+  input  logic        csr_mnxti_read_i,
 
   // EX/WB pipeline
   output ex_wb_pipe_t ex_wb_pipe_o,
@@ -334,6 +335,7 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;
       ex_wb_pipe_o.csr_op             <= CSR_OP_READ;
       ex_wb_pipe_o.csr_addr           <= 12'h000;
       ex_wb_pipe_o.csr_wdata          <= 32'h00000000;
+      ex_wb_pipe_o.csr_mnxti_access   <= 1'b0;
       ex_wb_pipe_o.xif_en             <= 1'b0;
       ex_wb_pipe_o.xif_meta           <= '0;
     end
@@ -363,9 +365,10 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;
         // to avoid writing to CSRs inside the core.
         ex_wb_pipe_o.csr_en     <= (csr_illegal_i || xif_csr_error_o) ? 1'b0 : id_ex_pipe_i.csr_en;
         if (id_ex_pipe_i.csr_en) begin
-          ex_wb_pipe_o.csr_addr  <= id_ex_pipe_i.alu_operand_b[11:0];
-          ex_wb_pipe_o.csr_wdata <= id_ex_pipe_i.alu_operand_a;
-          ex_wb_pipe_o.csr_op    <= id_ex_pipe_i.csr_op;
+          ex_wb_pipe_o.csr_addr         <= id_ex_pipe_i.alu_operand_b[11:0];
+          ex_wb_pipe_o.csr_wdata        <= id_ex_pipe_i.alu_operand_a;
+          ex_wb_pipe_o.csr_op           <= id_ex_pipe_i.csr_op;
+          ex_wb_pipe_o.csr_mnxti_access <= csr_mnxti_read_i;
         end
 
         // Propagate signals needed for exception handling in WB

rtl/cv32e40x_id_stage.sv

@@ -49,7 +49,7 @@ module cv32e40x_id_stage import cv32e40x_pkg::*;
   // IF/ID pipeline
   input  if_id_pipe_t if_id_pipe_i,
 
-  // ID/EX pipeline 
+  // ID/EX pipeline
   output id_ex_pipe_t id_ex_pipe_o,
 
   // EX/WB pipeline
@@ -113,7 +113,7 @@ module cv32e40x_id_stage import cv32e40x_pkg::*;
   logic                 rf_we;
   logic                 rf_we_dec;
   rf_addr_t             rf_waddr;
-  
+
   // ALU Control
   logic                 alu_en;
   logic                 alu_en_raw;
@@ -130,7 +130,7 @@ module cv32e40x_id_stage import cv32e40x_pkg::*;
   // Divider control
   logic                 div_en;
   div_opcode_e          div_operator;
-  
+
   // LSU
   logic                 lsu_en;
   logic                 lsu_we;
@@ -314,7 +314,7 @@ module cv32e40x_id_stage import cv32e40x_pkg::*;
 
   always_comb begin: jalr_fw_mux
     case (ctrl_byp_i.jalr_fw_mux_sel)
-      SELJ_FW_WB:   jalr_fw = ex_wb_pipe_i.rf_wdata;
+      SELJ_FW_WB:   jalr_fw = ex_wb_pipe_i.rf_wdata;  // todo: This won't allow forwarding from the XIF.
       SELJ_REGFILE: jalr_fw = rf_rdata_i[0];
       default:      jalr_fw = rf_rdata_i[0];
     endcase
@@ -413,7 +413,7 @@ module cv32e40x_id_stage import cv32e40x_pkg::*;
     .sys_ecall_insn_o                ( sys_ecall_insn            ),
     .sys_wfi_insn_o                  ( sys_wfi_insn              ),
     .sys_fencei_insn_o               ( sys_fencei_insn           ),
-    
+
     // from IF/ID pipeline
     .instr_rdata_i                   ( instr_merged              ), // todo: temporary hack while merging decoders
     .illegal_c_insn_i                ( if_id_pipe_i.illegal_c_insn ),
@@ -473,7 +473,7 @@ module cv32e40x_id_stage import cv32e40x_pkg::*;
   // Register writeback is enabled either by the decoder or by the XIF
   assign rf_we          = rf_we_dec || xif_we;
 
-  
+
   /////////////////////////////////////////////////////////////////////////////////
   //   ___ ____        _______  __  ____ ___ ____  _____ _     ___ _   _ _____   //
   //  |_ _|  _ \      | ____\ \/ / |  _ \_ _|  _ \| ____| |   |_ _| \ | | ____|  //
@@ -541,15 +541,15 @@ module cv32e40x_id_stage import cv32e40x_pkg::*;
       // normal pipeline unstall case
       if (id_valid_o && ex_ready_i) begin
         id_ex_pipe_o.instr_valid  <= 1'b1;
-        
+
         // Operands
         if (alu_op_a_mux_sel != OP_A_NONE) begin
           id_ex_pipe_o.alu_operand_a        <= operand_a;               // Used by most ALU, CSR and LSU instructions
         end
         if (alu_op_b_mux_sel != OP_B_NONE) begin
           id_ex_pipe_o.alu_operand_b        <= operand_b;               // Used by most ALU, CSR and LSU instructions
         end
-        
+
         if (op_c_mux_sel != OP_C_NONE)
         begin
           id_ex_pipe_o.operand_c            <= operand_c;               // Used by LSU stores and some ALU instructions
@@ -568,7 +568,7 @@ module cv32e40x_id_stage import cv32e40x_pkg::*;
         if (div_en) begin
           id_ex_pipe_o.div_operator         <= div_operator;
         end
-        
+
         id_ex_pipe_o.mul_en                 <= mul_en;
         if (mul_en) begin
           id_ex_pipe_o.mul_operator         <= mul_operator;
@@ -725,7 +725,7 @@ module cv32e40x_id_stage import cv32e40x_pkg::*;
         end
       end
 
-      assign xif_issue_if.issue_req.ecs       = 6'b111111; // todo: hookup to related mstatus bits (for now just reporting all state as dirty) 
+      assign xif_issue_if.issue_req.ecs       = 6'b111111; // todo: hookup to related mstatus bits (for now just reporting all state as dirty)
                                                            // and make sure that instruction after ecs update sees correct bits
       assign xif_issue_if.issue_req.ecs_valid = 1'b1; // todo: needs to take into account if mstatus extension context writes are in flight
                                                       // todo: use xif_issue_if.issue_resp.ecswrite

rtl/include/cv32e40x_pkg.sv

@@ -1124,6 +1124,7 @@ typedef struct packed {
   csr_opcode_e  csr_op;
   logic [11:0]  csr_addr;
   logic [31:0]  csr_wdata;
+  logic         csr_mnxti_access;
 
   // LSU
   logic         lsu_en;
@@ -1180,6 +1181,7 @@ typedef struct packed {
   logic         load_stall;             // Stall due to load operation
   logic         csr_stall;
   logic         wfi_stall;
+  logic         mnxti_stall;            // Stall due to mnxti CSR access in EX
   logic         minstret_stall;         // Stall due to minstret/h read in EX
   logic         deassert_we;            // Deassert write enable and special insn bits
   logic         xif_exception_stall;    // Stall (EX) if xif insn in WB can cause an exception

sva/cv32e40x_core_sva.sv

@@ -40,6 +40,7 @@ module cv32e40x_core_sva
   input logic [31:0] mip,
   input dcsr_t       dcsr,
   input              if_id_pipe_t if_id_pipe,
+  input id_ex_pipe_t id_ex_pipe,
   input              id_stage_multi_cycle_id_stall,
   input logic        id_stage_id_valid,
   input logic        ex_ready,
@@ -48,6 +49,17 @@ module cv32e40x_core_sva
   input logic        wb_valid,
   input logic        branch_taken_in_ex,
 
+  input alu_op_a_mux_e alu_op_a_mux_sel_id_i,
+  input alu_op_b_mux_e alu_op_b_mux_sel_id_i,
+  input logic [31:0]   operand_a_id_i,
+  input logic [31:0]   operand_b_id_i,
+  input logic [31:0]   jalr_fw_id_i,
+  input logic [31:0]   rf_wdata_wb,
+  input logic          rf_we_wb,
+
+  input logic        alu_jmpr_id_i,
+  input logic        alu_en_raw_id_i,
+
   // probed OBI signals
   input logic [1:0]  instr_memtype_o,
   input logic [1:0]  data_memtype_o,
@@ -60,6 +72,7 @@ module cv32e40x_core_sva
   input logic        ctrl_pending_debug,
   input logic        ctrl_debug_allowed,
   input              ctrl_state_e ctrl_fsm_ns,
+  input ctrl_byp_t   ctrl_byp,
    // probed cs_registers signals
   input logic [31:0] cs_registers_mie_q,
   input logic [31:0] cs_registers_mepc_n,
@@ -361,6 +374,58 @@ always_ff @(posedge clk , negedge rst_ni)
   endgenerate
 
 
+  // Check that operand_a data forwarded from EX to ID actually is written to RF in WB
+  property p_opa_fwd_ex;
+    logic [31:0] opa;
+    @(posedge clk) disable iff (!rst_ni)
+    (id_stage_id_valid && ex_ready && (alu_op_a_mux_sel_id_i == OP_A_REGA_OR_FWD) && (ctrl_byp.operand_a_fw_mux_sel == SEL_FW_EX), opa=operand_a_id_i)
+    |=> (opa == rf_wdata_wb) && (rf_we_wb || (ctrl_fsm.kill_id || ctrl_fsm.halt_id));
+  endproperty
+
+  a_opa_fwd_ex: assert property (p_opa_fwd_ex)
+    else `uvm_error("core", "Forwarded data (operand_a) from EX not written to RF the following cycle")
+
+  // Check that operand_a data forwarded from WB to ID actually is written to RF in WB
+  property p_opa_fwd_wb;
+    @(posedge clk) disable iff (!rst_ni)
+    (id_stage_id_valid && ex_ready && (alu_op_a_mux_sel_id_i == OP_A_REGA_OR_FWD) && (ctrl_byp.operand_a_fw_mux_sel == SEL_FW_WB))
+    |-> (operand_a_id_i == rf_wdata_wb) && (rf_we_wb || (ctrl_fsm.kill_id || ctrl_fsm.halt_id));
+  endproperty
+
+  a_opa_fwd_wb: assert property (p_opa_fwd_wb)
+    else `uvm_error("core", "Forwarded data (operand_a) from WB not written to RF in the same cycle")
+
+  // Check that operand_b data forwarded from EX to ID actually is written to RF in WB
+  property p_opb_fwd_ex;
+    logic [31:0] opb;
+    @(posedge clk) disable iff (!rst_ni)
+    (id_stage_id_valid && ex_ready && (alu_op_b_mux_sel_id_i == OP_B_REGB_OR_FWD) && (ctrl_byp.operand_b_fw_mux_sel == SEL_FW_EX), opb=operand_b_id_i)
+    |=> (opb == rf_wdata_wb) && (rf_we_wb || (ctrl_fsm.kill_id || ctrl_fsm.halt_id));
+  endproperty
+
+  a_opb_fwd_ex: assert property (p_opb_fwd_ex)
+    else `uvm_error("core", "Forwarded data (operand_b) from EX not written to RF the following cycle")
+
+  // Check that operand_b data forwarded from WB to ID actually is written to RF in WB
+  property p_opb_fwd_wb;
+    @(posedge clk) disable iff (!rst_ni)
+    (id_stage_id_valid && ex_ready && (alu_op_b_mux_sel_id_i == OP_B_REGB_OR_FWD) && (ctrl_byp.operand_b_fw_mux_sel == SEL_FW_WB))
+    |-> (operand_b_id_i == rf_wdata_wb) && (rf_we_wb || (ctrl_fsm.kill_id || ctrl_fsm.halt_id));
+  endproperty
+
+  a_opb_fwd_wb: assert property (p_opb_fwd_wb)
+    else `uvm_error("core", "Forwarded data (operand_b) from WB not written to RF in the same cycle")
+
+  // Check that data forwarded from WB to a JALR instruction in ID is actully written to the RF
+  property p_jalr_fwd;
+    @(posedge clk) disable iff (!rst_ni)
+    (alu_jmpr_id_i && alu_en_raw_id_i) && (ctrl_byp.jalr_fw_mux_sel == SELJ_FW_WB) && !ctrl_byp.jalr_stall
+    |->
+    (jalr_fw_id_i == rf_wdata_wb) && (rf_we_wb || (ctrl_fsm.kill_id || ctrl_fsm.halt_id));
+  endproperty
+
+  a_jalr_fwd: assert property(p_jalr_fwd)
+    else `uvm_error("core", "Forwarded jalr data from WB to ID not written to RF")
 
 endmodule // cv32e40x_core_sva