Fix break and single-step on a load/store access fault dropping the exception. Better fix for halt request on definitely-excepting stage M instruction giving unreachable next-instruction DPC.

hdl/hazard3_core.v

@@ -257,6 +257,7 @@ wire                 x_alu_cmp;
 wire [W_DATA-1:0]    m_trap_addr;
 wire                 m_trap_is_irq;
 wire                 m_trap_enter_vld;
+wire                 m_trap_enter_soon;
 wire                 m_trap_enter_rdy = f_jump_rdy;
 
 reg  [W_REGADDR-1:0] xm_rs1;
@@ -275,7 +276,8 @@ wire x_jump_req;
 
 // IRQs squeeze in between the instructions in X and M, so in this case X
 // stalls but M can continue. -> X always stalls on M trap, M *may* stall.
-wire x_stall_on_trap = m_trap_enter_vld && !m_trap_enter_rdy;
+wire x_stall_on_trap = m_trap_enter_vld && !m_trap_enter_rdy ||
+	m_trap_enter_soon && !m_trap_enter_vld;
 
 assign x_stall =
 	m_stall ||
@@ -368,7 +370,7 @@ always @ (*) begin
 		MEMOP_SH:  bus_hsize_d = HSIZE_HWORD;
 		default:   bus_hsize_d = HSIZE_BYTE;
 	endcase
-	bus_aph_req_d = x_memop_vld && !(x_stall_raw || x_unaligned_addr || m_trap_enter_vld);
+	bus_aph_req_d = x_memop_vld && !(x_stall_raw || x_unaligned_addr || m_trap_enter_soon);
 end
 
 // Multiply/divide
@@ -391,7 +393,7 @@ if (EXTENSION_M) begin: has_muldiv
 		else
 			x_muldiv_posted <= (x_muldiv_posted || (x_muldiv_op_vld && x_muldiv_op_rdy)) && x_stall;
 
-	wire x_muldiv_kill = m_trap_enter_vld;
+	wire x_muldiv_kill = m_trap_enter_soon;
 
 	wire x_use_fast_mul = MUL_FAST && d_aluop == ALUOP_MULDIV && d_mulop == M_OP_MUL;
 
@@ -473,6 +475,8 @@ wire [W_DATA-1:0] x_csr_wdata = d_csr_w_imm ?
 wire [W_DATA-1:0] x_csr_rdata;
 wire              x_csr_illegal_access;
 
+// "Previous" refers to next-most-recent instruction to be in D/X, i.e. the
+// most recent instruction to reach stage M (which may or may not still be in M).
 reg prev_instr_was_32_bit;
 
 always @ (posedge clk or negedge rst_n) begin
@@ -522,19 +526,21 @@ hazard3_csr #(
 	// Can generate access faults (hence traps), but do not actually perform access.
 	.addr                       (d_imm[11:0]), // todo could just connect this to the instruction bits
 	.wdata                      (x_csr_wdata),
-	.wen_soon                   (d_csr_wen && !m_trap_enter_vld),
-	.wen                        (d_csr_wen && !m_trap_enter_vld && !x_stall),
+	.wen_soon                   (d_csr_wen && !m_trap_enter_soon),
+	.wen                        (d_csr_wen && !m_trap_enter_soon && !x_stall),
 	.wtype                      (d_csr_wtype),
 	.rdata                      (x_csr_rdata),
-	.ren_soon                   (d_csr_ren && !m_trap_enter_vld),
-	.ren                        (d_csr_ren && !m_trap_enter_vld && !x_stall),
+	.ren_soon                   (d_csr_ren && !m_trap_enter_soon),
+	.ren                        (d_csr_ren && !m_trap_enter_soon && !x_stall),
 	.illegal                    (x_csr_illegal_access),
 
 	// Trap signalling
 	.trap_addr                  (m_trap_addr),
 	.trap_is_irq                (m_trap_is_irq),
+	.trap_enter_soon            (m_trap_enter_soon),
 	.trap_enter_vld             (m_trap_enter_vld),
 	.trap_enter_rdy             (m_trap_enter_rdy),
+	.loadstore_dphase_pending   (!xm_memop[3]),
 	.mepc_in                    (m_exception_return_addr),
 
 	// IRQ and exception requests
@@ -566,7 +572,7 @@ always @ (posedge clk or negedge rst_n) begin
 			// If the transfer is unaligned, make sure it is completely NOP'd on the bus
 			xm_memop <= d_memop | {x_unaligned_addr, 3'h0};
 			xm_except <= x_except;
-			if (x_stall || m_trap_enter_vld) begin
+			if (x_stall || m_trap_enter_soon) begin
 				// Insert bubble
 				xm_rd <= {W_REGADDR{1'b0}};
 				xm_memop <= MEMOP_NONE;

hdl/hazard3_csr.v

@@ -81,6 +81,15 @@ module hazard3_csr #(
 	output wire                trap_is_irq,
 	output wire                trap_enter_vld,
 	input  wire                trap_enter_rdy,
+	// True when we are about to trap into debug mode, but are waiting for an
+	// excepting or potentially-excepting instruction to clear M first. The
+	// instruction in X is suppressed, X PC does not increment but still
+	// tracks exception addresses.
+	output wire                trap_enter_soon,
+	// We need to know about load/stores in data phase because their exception
+	// status is still unknown, so we fence off on them before entering debug
+	// mode.
+	input  wire                loadstore_dphase_pending,
 	input  wire [XLEN-1:0]     mepc_in,
 
 	// Exceptions must *not* be a function of bus stall.
@@ -917,30 +926,45 @@ end
 // instructions in stage 2 and stage 3) or in an exception-like manner
 // (replace the instruction in stage 3).
 //
-// A tricky case is halt request: this normally performs an IRQ-like entry,
-// because the instruction in stage 3 can not in general be discarded, as it
-// may already have had system side effects: for example a load/store on an
-// IO region.
+// Halt request and step-break are IRQ-like. We need to be careful when a halt
+// request lines up with an instruction in M which either has generated an
+// exception (e.g. an ecall) or may yet generate an exception (a load). In
+// this case the correct thing to do is to:
 //
-// However an asynchronous halt request when the instruction in stage 3 is
-// itself generating an exception is an exception-like halt entry. Otherwise,
-// we set DPC to the instruction *after* (in X) the excepting one, which is
-// never actually reached, due to the exception.
+// - Squash whatever instruction may be in X, and inhibit X PC increment
+// - Wait until after the exception entry is taken (or the load/store
+//   completes successfully)
+// - Immediately trigger an IRQ-like debug entry.
+//
+// This ensures the debugger sees mcause/mepc set correctly, with dpc pointing
+// to the handler entry point, if the instruction excepts.
 
 wire exception_req_any;
+wire halt_delayed_by_exception = exception_req_any || loadstore_dphase_pending;
 
 // This would also include triggers, if/when those are implemented:
 wire want_halt_except = DEBUG_SUPPORT && !debug_mode && (
-	dcsr_ebreakm && except == EXCEPT_EBREAK ||
-	// This clause takes priority over the IRQ-like dbg_req_halt clause below:
-	dbg_req_halt && exception_req_any
+	dcsr_ebreakm && except == EXCEPT_EBREAK
 );
 
-// Note all exception-like causes (trigger, ebreak) are higher priority than IRQ-like
-wire want_halt_irq = DEBUG_SUPPORT && !debug_mode && !want_halt_except && (
-	dbg_req_halt || (dbg_req_halt_on_reset && have_just_reset) || step_halt_req
+// Note exception-like causes (trigger, ebreak) are higher priority than
+// IRQ-like.
+//
+// We must mask halt_req with delay_irq_entry (true on cycle 2 and beyond of
+// load/store address phase) because at that point we can't suppress the bus
+// access..
+wire want_halt_irq_if_no_exception = DEBUG_SUPPORT && !debug_mode && !want_halt_except && (
+	(dbg_req_halt && !delay_irq_entry) ||
+	(dbg_req_halt_on_reset && have_just_reset) ||
+	step_halt_req
 );
 
+// Exception (or potential exception due to load/store) in M delays halt
+// entry. The halt intention still blocks X, so we can't get blocked forever
+// by a string of load/stores. This is just here to get sequencing between an
+// exception (or potential exception) in M, and debug mode entry.
+wire want_halt_irq = want_halt_irq_if_no_exception && !halt_delayed_by_exception;
+
 assign dcause_next =
 	// Trigger would be highest priority if implemented
 	except == EXCEPT_EBREAK                                    ? 3'h1 : // ebreak (priority 3)
@@ -1045,6 +1069,8 @@ assign trap_enter_vld =
 	DEBUG_SUPPORT && (
 		(!delay_irq_entry && want_halt_irq) || want_halt_except || pending_dbg_resume);
 
+assign trap_enter_soon = trap_enter_vld || (DEBUG_SUPPORT && want_halt_irq_if_no_exception);
+
 assign mcause_irq_next = !exception_req_any;
 assign mcause_code_next = exception_req_any ? {2'h0, except} : mcause_irq_num;