Fix for issue #665

bhv/cv32e40x_wrapper.sv

@@ -309,6 +309,8 @@ module cv32e40x_wrapper
                 .ctrl_pending_interrupt           (core_i.controller_i.controller_fsm_i.pending_interrupt),
                 .ctrl_interrupt_allowed           (core_i.controller_i.controller_fsm_i.interrupt_allowed),
                 .ctrl_debug_cause_n               (core_i.controller_i.controller_fsm_i.debug_cause_n),
+                .ctrl_pending_nmi                 (core_i.controller_i.controller_fsm_i.pending_nmi),
+                .ctrl_fsm_cs                      (core_i.controller_i.controller_fsm_i.ctrl_fsm_cs),
                 .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),
@@ -330,6 +332,9 @@ if (SMCLIC) begin : clic_asserts
       cv32e40x_clic_int_controller_sva
         clic_int_controller_sva (.ctrl_pending_interrupt  (core_i.controller_i.controller_fsm_i.pending_interrupt),
                                  .ctrl_interrupt_allowed  (core_i.controller_i.controller_fsm_i.interrupt_allowed),
+                                 .ctrl_pending_nmi        (core_i.controller_i.controller_fsm_i.pending_nmi),
+                                 .ctrl_pending_async_debug(core_i.controller_i.controller_fsm_i.pending_async_debug),
+                                 .ctrl_fsm_cs             (core_i.controller_i.controller_fsm_i.ctrl_fsm_cs),
                                  .ctrl_fsm                (core_i.ctrl_fsm),
                                  .dcsr                    (core_i.dcsr),
                                  .*);

rtl/cv32e40x_controller_fsm.sv

@@ -461,6 +461,15 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
   //   - This is guarded with using the sequence_interruptible, which tracks sequence progress through the WB stage.
   // When a CLIC pointer is in the pipeline stages EX or WB, we must block debug.
   //   - Debug would otherwise kill the pointer and use the address of the pointer for dpc. A following dret would then return to the mtvt table, losing program progress.
+  //
+  // Debug entry because of haltreq is disallowed when the LSU is busy and therefore
+  // haltreq can only cause debug entry on the instruction following a load or store that
+  // keep the LSU busy. If such load or store however is being single stepped or has an
+  // associated breakpoint or watchpoint, then debug will be entered because of that
+  // lower priority reason even though haltreq is asserted. This is okay because if instruction
+  // timing is considered haltreq should be considered only asserted on the following
+  // instruction (i.e. the asynchronous haltreq signal is considered asserted too late to
+  // impact the current instruction in the pipeline).
   assign async_debug_allowed = lsu_interruptible_i && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible;
 
   // synchronous debug entry have far fewer restrictions than asynchronous entries. In principle synchronous debug entry should have the same
@@ -493,6 +502,7 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
                          (pending_async_debug && async_debug_allowed)      ? DBG_CAUSE_HALTREQ :
                          (pending_single_step && single_step_allowed)      ? DBG_CAUSE_STEP    : DBG_CAUSE_NONE;
 
+
   // Debug cause to CSR from flopped version (valid during DEBUG_TAKEN)
   assign ctrl_fsm_o.debug_cause = debug_cause_q;
 
@@ -692,10 +702,8 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
             pipe_pc_mux_ctrl = PC_IF;
           end
 
-        // Debug entry (except single step which is handled later)
-        // todo: may split this into two separate if's, and then prioritize synchronous debug entry below interrupts.
-        end else if ((pending_async_debug && async_debug_allowed) ||
-                     (pending_sync_debug && sync_debug_allowed)) begin
+        // External debug entry (async)
+        end else if (pending_async_debug && async_debug_allowed) begin
           // Halt the whole pipeline
           // Halting makes sure instructions stay in the pipeline stage without propagating to the next.
           //  This is needed by the debug entry code in the DEBUG_STAKEN state to pick the correct PC for storing in dpc.
@@ -762,7 +770,20 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
             // instruction to be issued from IF to ID.
             pipe_pc_mux_ctrl = PC_IF;
           end
+        // Synchronous debug entry (ebreak, trigger match)
+        end else if (pending_sync_debug && sync_debug_allowed) begin
+          // Halt the whole pipeline
+          // Halting makes sure instructions stay in the pipeline stage without propagating to the next.
+          //  This is needed by the debug entry code in the DEBUG_STAKEN state to pick the correct PC for storing in dpc.
+          //  Note that signals like lsu_wpt_match_wb_i and lsu_mpu_status_wb_i will not be constant while WB is halted as
+          //  these behave as an rvalid. In general LSU instruction shall not be allowed to be halted, unless there is a
+          //  watchpoint address match.
+          ctrl_fsm_o.halt_if = 1'b1;
+          ctrl_fsm_o.halt_id = 1'b1;
+          ctrl_fsm_o.halt_ex = 1'b1;
+          ctrl_fsm_o.halt_wb = 1'b1;
 
+          ctrl_fsm_ns = DEBUG_TAKEN;
         end else begin
           if (exception_in_wb && exception_allowed) begin
             // Kill all stages

sva/cv32e40x_clic_int_controller_sva.sv

@@ -42,6 +42,9 @@ module cv32e40x_clic_int_controller_sva
 
    input logic        ctrl_pending_interrupt,
    input logic        ctrl_interrupt_allowed,
+   input logic        ctrl_pending_nmi,
+   input logic        ctrl_pending_async_debug,
+   input ctrl_state_e ctrl_fsm_cs,
 
    input ctrl_fsm_t   ctrl_fsm,
    input dcsr_t       dcsr
@@ -61,6 +64,22 @@ module cv32e40x_clic_int_controller_sva
   a_clic_enable: assert property(p_clic_enable)
     else `uvm_error("core", "Interrupt not taken soon enough after enabling");
 
+  // Check that only NMI and external debug take presedence over interrupts after being enabled by mret or CSR writes
+  property p_irq_pri;
+    @(posedge clk) disable iff (!rst_n)
+    ( !irq_req_ctrl_o            // No interrupt pending
+      ##1                        // Next cycle
+      irq_req_ctrl_o && $stable(clic_irq_q) && $stable(clic_irq_level_q) && !(ctrl_fsm.debug_mode || (dcsr.step && !dcsr.stepie)) && // Interrupt pending but irq inputs are unchanged
+      (ctrl_fsm_cs != DEBUG_TAKEN) &&  // Make sure we are not handling a debug entry already (could be a single stepped mret enabling interrupts for instance)
+      !(ctrl_pending_nmi || ctrl_pending_async_debug)   // No pending events with higher priority than interrupts are taking place
+      |->
+      ctrl_fsm.irq_ack  // We must take the interrupt if enabled (mret or CSR write) and no NMI or external debug is pending
+    );
+  endproperty;
+
+  a_irq_pri: assert property(p_irq_pri)
+    else `uvm_error("core", "Interrupt not taken soon enough after enabling")
+
   // Check a pending interrupt that is disabled is actually not taken
   property p_clic_disable;
     @(posedge clk) disable iff (!rst_n)

sva/cv32e40x_core_sva.sv

@@ -84,6 +84,8 @@ module cv32e40x_core_sva
   input logic        ctrl_pending_interrupt,
   input ctrl_byp_t   ctrl_byp,
   input logic [2:0]  ctrl_debug_cause_n,
+  input logic        ctrl_pending_nmi,
+  input ctrl_state_e ctrl_fsm_cs,
   // probed cs_registers signals
   input logic [31:0] cs_registers_mie_q,
   input logic [31:0] cs_registers_mepc_n,
@@ -512,6 +514,21 @@ if (!SMCLIC) begin
   a_mip_mie_write_enable: assert property(p_mip_mie_write_enable)
     else `uvm_error("core", "Interrupt not taken soon enough after enabling");
 
+  // Check that only NMI and external debug take presedence over interrupts after being enabled by mret or CSR writes
+  property p_irq_pri;
+    @(posedge clk) disable iff (!rst_ni)
+    ( !irq_req_ctrl  // No interrupt pending
+       ##1          // Next cycle
+       irq_req_ctrl && $stable(mip) && !(ctrl_fsm.debug_mode || (dcsr.step && !dcsr.stepie)) && // Interrupt pending but irq inputs are unchanged
+       (ctrl_fsm_cs != DEBUG_TAKEN) &&  // Make sure we are not handling a debug entry already (could be a single stepped mret enabling interrupts for instance)
+       !(ctrl_pending_nmi || ctrl_pending_async_debug)   // No pending events with higher priority than interrupts are taking place
+       |->
+       ctrl_fsm.irq_ack);  // We must take the interrupt if enabled (mret or CSR write) and no NMI or external debug is pending
+  endproperty;
+
+  a_irq_pri: assert property(p_irq_pri)
+    else `uvm_error("core", "Interrupt not taken soon enough after enabling");
+
   // Check a pending interrupt that is disabled is actually not taken
   property p_mip_mie_write_disable;
     @(posedge clk) disable iff (!rst_ni)