From c81666177e610ad641ba3a7d2cf5393ee2e0ec3c Mon Sep 17 00:00:00 2001 From: Luke Wren Date: Sat, 5 Nov 2022 18:18:33 +0000 Subject: [PATCH] Remove FIXME about considering concurrent load/store and debug entry in calculating the privilege of load/stores. This is safe because it is only the *current* debug mode state which affects load/stores, and some new properties have been added to ensure load/stores can not be in aphase at the point debug mode is entered/exited (which is achieved by delaying the trap). Therefore there is no way for debug entry to inadvertently boost the privilege of an executing U-mode load/store. Also rename a confusingly-named signal for an unsquashable bus transfer in stage 2 that delays IRQ entry. --- hdl/hazard3_core.v | 18 +++++++++++++----- hdl/hazard3_csr.v | 25 +++++++++++++++++++------ 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/hdl/hazard3_core.v b/hdl/hazard3_core.v index 973af15..844b685 100644 --- a/hdl/hazard3_core.v +++ b/hdl/hazard3_core.v @@ -324,7 +324,7 @@ reg [W_EXCEPT-1:0] xm_except; reg xm_except_to_d_mode; reg xm_sleep_wfi; reg xm_sleep_block; -reg xm_delay_irq_entry_on_ls_dphase; +reg xm_delay_irq_entry_on_ls_stagex; // ---------------------------------------------------------------------------- // Stall logic @@ -893,7 +893,7 @@ reg prev_instr_was_32_bit; always @ (posedge clk or negedge rst_n) begin if (!rst_n) begin - xm_delay_irq_entry_on_ls_dphase <= 1'b0; + xm_delay_irq_entry_on_ls_stagex <= 1'b0; prev_instr_was_32_bit <= 1'b0; end else begin // Must hold off IRQ if we are in the second cycle of an address phase or @@ -905,7 +905,7 @@ always @ (posedge clk or negedge rst_n) begin // Also hold off on AMOs, unless the AMO is transitioning to an address // phase or completing. ("completing" excludes transitions to error phase.) - xm_delay_irq_entry_on_ls_dphase <= bus_aph_req_d && !bus_aph_ready_d || + xm_delay_irq_entry_on_ls_stagex <= bus_aph_req_d && !bus_aph_ready_d || d_memop_is_amo && !( x_amo_phase == 3'h3 && bus_dph_ready_d && !bus_dph_err_d || // Read reservation failure failure also generates error @@ -944,7 +944,7 @@ wire m_dphase_in_flight = xm_memop != MEMOP_NONE && xm_memop != MEMOP_AMO; // Need to delay IRQ entry on sleep exit because, for deep sleep states, we // can't access the bus until the power handshake has completed. -wire m_delay_irq_entry = xm_delay_irq_entry_on_ls_dphase || +wire m_delay_irq_entry = xm_delay_irq_entry_on_ls_stagex || ((xm_sleep_wfi || xm_sleep_block) && !m_sleep_stall_release); wire m_pwr_allow_sleep; @@ -999,6 +999,7 @@ hazard3_csr #( .trap_enter_vld (m_trap_enter_vld), .trap_enter_rdy (m_trap_enter_rdy), .loadstore_dphase_pending (m_dphase_in_flight), + .delay_irq_entry (m_delay_irq_entry), .mepc_in (m_exception_return_addr), .pwr_allow_sleep (m_pwr_allow_sleep), @@ -1011,7 +1012,6 @@ hazard3_csr #( .m_mode_trap_entry (m_mmode_trap_entry), // IRQ and exception requests - .delay_irq_entry (m_delay_irq_entry), .irq (irq), .irq_software (soft_irq), .irq_timer (timer_irq), @@ -1085,6 +1085,14 @@ always @ (posedge clk or negedge rst_n) begin xm_sleep_wfi <= 1'b0; // TODO needed? xm_sleep_block <= 1'b0; end +`ifdef HAZARD3_ASSERTIONS + // Some of the exception->ls paths are cut as they are supposed to be + // impossible -- make sure there is no way to have a load/store that + // is not squashed. (This includes debug entry!) + if (m_trap_enter_vld) begin + assert(!bus_aph_req_d); + end +`endif end end diff --git a/hdl/hazard3_csr.v b/hdl/hazard3_csr.v index 1e6be4f..b6a1bed 100644 --- a/hdl/hazard3_csr.v +++ b/hdl/hazard3_csr.v @@ -79,9 +79,12 @@ module hazard3_csr #( // 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. + // status is still unknown, so fence them off before entering debug mode. input wire loadstore_dphase_pending, + // Asserted when the instruction in stage 2 can't be squashed (e.g. to + // keep an AHB address phase stable), or instruction fetch request can't + // be issued (e.g. waiting for wake from wfi state). + input wire delay_irq_entry, input wire [XLEN-1:0] mepc_in, // Power control signalling @@ -100,7 +103,6 @@ module hazard3_csr #( input wire except_to_d_mode, // Level-sensitive interrupt sources - input wire delay_irq_entry, input wire [NUM_IRQS-1:0] irq, input wire irq_software, input wire irq_timer, @@ -1197,7 +1199,7 @@ wire want_halt_except = DEBUG_SUPPORT && !debug_mode && ( // // 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.. +// access. Others are fine because they are never mid-instruction in stage 2. wire want_halt_irq_if_no_exception = DEBUG_SUPPORT && !debug_mode && !want_halt_except && ( (dbg_req_halt_prev && !delay_irq_entry) || (dbg_req_halt_on_reset && have_just_reset) || @@ -1299,7 +1301,6 @@ assign trap_is_irq = DEBUG_SUPPORT && (want_halt_except || want_halt_irq) ? // in progress, because that dphase could subsequently except, and sample the // IRQ vector PC instead of the load/store instruction PC. -// delay_irq_entry also applies to IRQ-like debug entries. assign trap_enter_vld = CSR_M_TRAP && (exception_req_any || !delay_irq_entry && !debug_mode && irq_active && !loadstore_dphase_pending) || @@ -1343,7 +1344,7 @@ assign m_mode_trap_entry = !U_MODE || ( // - Privilege level of load/stores on the bus // - Checking load/stores against PMP read/write permission -assign m_mode_loadstore = !U_MODE || debug_mode || ( // FIXME how does this interact with load/store racing against debug entry? +assign m_mode_loadstore = !U_MODE || debug_mode || ( mstatus_mprv ? mstatus_mpp : m_mode ); @@ -1435,6 +1436,18 @@ always @ (posedge clk) begin assert(except == EXCEPT_EBREAK); end + // Exceptions should squash load/stores before they reach dphase. + if (except != EXCEPT_NONE && !(except == EXCEPT_LOAD_FAULT || except == EXCEPT_STORE_FAULT)) begin + assert(!loadstore_dphase_pending); + end + + // Debug entries are trap entries! (we should also assert in core.v that + // trap entries are never coincident with active load/store address + // phases, which gives some confidence that debug entry does not gobble + // load/stores.) + if (enter_debug_mode) begin + assert(trap_enter_vld); + end end `endif