diff --git a/rtl/ibex_controller.sv b/rtl/ibex_controller.sv index 23d9326c..c209642b 100644 --- a/rtl/ibex_controller.sv +++ b/rtl/ibex_controller.sv @@ -142,8 +142,9 @@ module ibex_controller #( logic illegal_dret; logic illegal_umode; logic exc_req_lsu; - logic special_req_all; - logic special_req_branch; + logic special_req; + logic special_req_pc_change; + logic special_req_flush_only; logic enter_debug_mode_d; logic enter_debug_mode_q; logic ebreak_into_debug; @@ -219,29 +220,18 @@ module ibex_controller #( // special requests: special instructions, pipeline flushes, exceptions... + // All terms in these expressions are qualified by instr_valid_i except exc_req_lsu which can come + // from the Writeback stage with no instr_valid_i from the ID stage - // To avoid creating a path from data_err_i -> instr_req_o and to help timing the below - // special_req_all has a version that only applies to branches. For a branch the controller needs - // to set pc_set_o but only if there is no special request. If the generic special_req_all signal - // is used then a variety of signals that will never cause a special request during a branch - // instruction end up factored into pc_set_o. The special_req_branch only considers the special - // request reasons that are relevant to a branch. + // These special requests only cause a pipeline flush and in particular don't cause a PC change + // that is outside the normal execution flow + assign special_req_flush_only = wfi_insn | csr_pipe_flush; + + // These special requests cause a change in PC + assign special_req_pc_change = mret_insn | dret_insn | exc_req_d | exc_req_lsu; // generic special request signal, applies to all instructions - // All terms in this expression are qualified by instr_valid_i except exc_req_lsu which can come - // from the Writeback stage with no instr_valid_i from the ID stage - assign special_req_all = mret_insn | dret_insn | wfi_insn | csr_pipe_flush | - exc_req_d | exc_req_lsu; - - // special request that can specifically occur during branch instructions - // All terms in this expression are qualified by instr_valid_i - assign special_req_branch = instr_fetch_err & (ctrl_fsm_cs != FLUSH); - - `ASSERT(SpecialReqBranchGivesSpecialReqAll, - special_req_branch |-> special_req_all) - - `ASSERT(SpecialReqAllGivesSpecialReqBranchIfBranchInst, - special_req_all && (branch_set_i || jump_set_i) |-> special_req_branch) + assign special_req = special_req_pc_change | special_req_flush_only; // Exception/fault prioritisation is taken from Table 3.7 of Priviledged Spec v1.11 if (WritebackStage) begin : g_wb_exceptions @@ -486,7 +476,7 @@ module ibex_controller #( // Get ready for special instructions, exceptions, pipeline flushes - if (special_req_all) begin + if (special_req) begin // Halt IF but don't flush ID. This leaves a valid instruction in // ID so controller can determine appropriate action in the // FLUSH state. @@ -503,26 +493,24 @@ module ibex_controller #( end end - if (!special_req_branch) begin - if (branch_set_i || jump_set_i) begin - // Only set the PC if the branch predictor hasn't already done the branch for us - pc_set_o = BranchPredictor ? ~instr_bp_taken_i : 1'b1; + if (branch_set_i || jump_set_i) begin + // Only set the PC if the branch predictor hasn't already done the branch for us + pc_set_o = BranchPredictor ? ~instr_bp_taken_i : 1'b1; - perf_tbranch_o = branch_set_i; - perf_jump_o = jump_set_i; - end + perf_tbranch_o = branch_set_i; + perf_jump_o = jump_set_i; + end - if (BranchPredictor) begin - if (instr_bp_taken_i & branch_not_set_i) begin - // If the instruction is a branch that was predicted to be taken but was not taken - // signal a mispredict. - nt_branch_mispredict_o = 1'b1; - end + if (BranchPredictor) begin + if (instr_bp_taken_i & branch_not_set_i) begin + // If the instruction is a branch that was predicted to be taken but was not taken + // signal a mispredict. + nt_branch_mispredict_o = 1'b1; end end // pc_set signal excluding branch taken condition - if ((branch_set_spec_i || jump_set_i) && !special_req_branch) begin + if (branch_set_spec_i || jump_set_i) begin // Only speculatively set the PC if the branch predictor hasn't already done the branch // for us pc_set_spec_o = BranchPredictor ? ~instr_bp_taken_i : 1'b1; @@ -535,7 +523,7 @@ module ibex_controller #( halt_if = 1'b1; end - if (!stall && !special_req_all) begin + if (!stall && !special_req) begin if (enter_debug_mode_d) begin // enter debug mode ctrl_fsm_ns = DBG_TAKEN_IF; @@ -853,4 +841,77 @@ module ibex_controller #( // The speculative branch signal should be set whenever the actual branch signal is set `ASSERT(IbexSpecImpliesSetPC, pc_set_o |-> pc_set_spec_o) + `ifdef INC_ASSERT + // If something that causes a jump into an exception handler is seen that jump must occur before + // the next instruction executes. The logic tracks whether a jump into an exception handler is + // expected. Assertions check the jump occurs. + + logic exception_req, exception_req_pending, exception_req_accepted, exception_req_done; + logic exception_pc_set, seen_exception_pc_set, expect_exception_pc_set; + logic exception_req_needs_pc_set; + + assign exception_req = (special_req | enter_debug_mode_d | handle_irq); + // Any exception rquest will cause a transition out of DECODE, once the controller transitions + // back into DECODE we're done handling the request. + assign exception_req_done = + exception_req_pending & (ctrl_fsm_cs != DECODE) & (ctrl_fsm_ns == DECODE); + + assign exception_req_needs_pc_set = enter_debug_mode_d | handle_irq | special_req_pc_change; + + // An exception PC set uses specific PC types + assign exception_pc_set = + exception_req_pending & (pc_set_o & (pc_mux_o inside {PC_EXC, PC_ERET, PC_DRET})); + + always @(posedge clk_i or negedge rst_ni) begin + if (!rst_ni) begin + exception_req_pending <= 1'b0; + exception_req_accepted <= 1'b0; + expect_exception_pc_set <= 1'b0; + seen_exception_pc_set <= 1'b0; + end else begin + // Keep `exception_req_pending` asserted once an exception_req is seen until it is done + exception_req_pending <= (exception_req_pending | exception_req) & ~exception_req_done; + + // The exception req has been accepted once the controller transitions out of decode + exception_req_accepted <= (exception_req_accepted & ~exception_req_done) | + (exception_req & ctrl_fsm_ns != DECODE); + + // Set `expect_exception_pc_set` if exception req needs one and keep it asserted until + // exception req is done + expect_exception_pc_set <= (expect_exception_pc_set | exception_req_needs_pc_set) & + ~exception_req_done; + + // Keep `seen_exception_pc_set` asserted once an exception PC set is seen until the + // exception req is done + seen_exception_pc_set <= (seen_exception_pc_set | exception_pc_set) & ~exception_req_done; + end + end + + // Once an exception request has been accepted it must be handled before controller goes back to + // DECODE + `ASSERT(IbexNoDoubleExceptionReq, exception_req_accepted |-> ctrl_fsm_cs != DECODE) + + // Only signal ready, allowing a new instruction into ID, if there is no exception request + // pending or it is done this cycle. + `ASSERT(IbexDontSkipExceptionReq, + id_in_ready_o |-> !exception_req_pending || exception_req_done) + + // Once a PC set has been performed for an exception request there must not be any other + // excepting those to move into debug mode. + `ASSERT(IbexNoDoubleSpecialReqPCSet, + seen_exception_pc_set && + !((ctrl_fsm_cs inside {DBG_TAKEN_IF, DBG_TAKEN_ID}) && + (pc_mux_o == PC_EXC) && (exc_pc_mux_o == EXC_PC_DBD)) + |-> !pc_set_o) + + // When an exception request is done there must have been an appropriate PC set (either this + // cycle or a previous one). + `ASSERT(IbexSetExceptionPCOnSpecialReqIfExpected, + exception_req_pending && expect_exception_pc_set && exception_req_done |-> + seen_exception_pc_set || exception_pc_set) + + // If there's a pending exception req that doesn't need a PC set we must not see one + `ASSERT(IbexNoPCSetOnSpecialReqIfNotExpected, + exception_req_pending && !expect_exception_pc_set |-> ~pc_set_o) + `endif endmodule diff --git a/rtl/ibex_id_stage.sv b/rtl/ibex_id_stage.sv index aa1a9dd3..b08fac01 100644 --- a/rtl/ibex_id_stage.sv +++ b/rtl/ibex_id_stage.sv @@ -197,15 +197,17 @@ module ibex_id_stage #( logic wb_exception; logic branch_in_dec; - logic branch_spec, branch_set_spec; - logic branch_set, branch_set_d; + logic branch_spec, branch_set_spec, branch_set_raw_spec; + logic branch_set, branch_set_raw, branch_set_raw_d; + logic branch_jump_set_done_q, branch_jump_set_done_d; logic branch_not_set; logic branch_taken; logic jump_in_dec; logic jump_set_dec; - logic jump_set; + logic jump_set, jump_set_raw; logic instr_first_cycle; + logic instr_executing_spec; logic instr_executing; logic instr_done; logic controller_run; @@ -651,29 +653,52 @@ module ibex_id_stage #( if (BranchTargetALU && !DataIndTiming) begin : g_branch_set_direct // Branch set fed straight to controller with branch target ALU // (condition pass/fail used same cycle as generated instruction request) - assign branch_set = branch_set_d; - assign branch_set_spec = branch_spec; + assign branch_set_raw = branch_set_raw_d; + assign branch_set_raw_spec = branch_spec; end else begin : g_branch_set_flop // Branch set flopped without branch target ALU, or in fixed time execution mode // (condition pass/fail used next cycle where branch target is calculated) - logic branch_set_q; + logic branch_set_raw_q; always_ff @(posedge clk_i or negedge rst_ni) begin if (!rst_ni) begin - branch_set_q <= 1'b0; + branch_set_raw_q <= 1'b0; end else begin - branch_set_q <= branch_set_d; + branch_set_raw_q <= branch_set_raw_d; end end // Branches always take two cycles in fixed time execution mode, with or without the branch // target ALU (to avoid a path from the branch decision into the branch target ALU operand // muxing). - assign branch_set = (BranchTargetALU && !data_ind_timing_i) ? branch_set_d : branch_set_q; + assign branch_set_raw = (BranchTargetALU && !data_ind_timing_i) ? branch_set_raw_d : branch_set_raw_q; // Use the speculative branch signal when BTALU is enabled - assign branch_set_spec = (BranchTargetALU && !data_ind_timing_i) ? branch_spec : branch_set_q; + assign branch_set_raw_spec = (BranchTargetALU && !data_ind_timing_i) ? branch_spec : branch_set_raw_q; end + // Track whether the current instruction in ID/EX has done a branch or jump set. + assign branch_jump_set_done_d = (branch_set_raw | jump_set_raw | branch_jump_set_done_q) & + ~instr_valid_clear_o; + + always_ff @(posedge clk_i or negedge rst_ni) begin + if (!rst_ni) begin + branch_jump_set_done_q <= 1'b0; + end else begin + branch_jump_set_done_q <= branch_jump_set_done_d; + end + end + + // the _raw signals from the state machine may be asserted for multiple cycles when + // instr_executing_spec is asserted and instr_executing is not asserted. This may occur where + // a memory error is seen or a there are outstanding memory accesses (indicate a load or store is + // in the WB stage). The branch or jump speculatively begins the fetch but is held back from completing + // until it is certain the outstanding access hasn't seen a memory error. This logic ensures only + // the first cycle of a branch or jump set is sent to the controller to prevent needless extra IF + // flushes and fetches. + assign jump_set = jump_set_raw & ~branch_jump_set_done_q; + assign branch_set = branch_set_raw & ~branch_jump_set_done_q; + assign branch_set_spec = branch_set_raw_spec & ~branch_jump_set_done_q; + // Branch condition is calculated in the first cycle and flopped for use in the second cycle // (only used in fixed time execution mode to determine branch destination). if (DataIndTiming) begin : g_sec_branch_taken @@ -691,7 +716,7 @@ module ibex_id_stage #( end else begin : g_nosec_branch_taken - // Signal unused without fixed time execution mode - only taken branches will trigger branch_set + // Signal unused without fixed time execution mode - only taken branches will trigger branch_set_raw assign branch_taken = 1'b1; end @@ -711,9 +736,9 @@ module ibex_id_stage #( always_ff @(posedge clk_i or negedge rst_ni) begin : id_pipeline_reg if (!rst_ni) begin - id_fsm_q <= FIRST_CYCLE; - end else begin - id_fsm_q <= id_fsm_d; + id_fsm_q <= FIRST_CYCLE; + end else if (instr_executing) begin + id_fsm_q <= id_fsm_d; end end @@ -729,13 +754,13 @@ module ibex_id_stage #( stall_jump = 1'b0; stall_branch = 1'b0; stall_alu = 1'b0; - branch_set_d = 1'b0; + branch_set_raw_d = 1'b0; branch_spec = 1'b0; branch_not_set = 1'b0; - jump_set = 1'b0; + jump_set_raw = 1'b0; perf_branch_o = 1'b0; - if (instr_executing) begin + if (instr_executing_spec) begin unique case (id_fsm_q) FIRST_CYCLE: begin unique case (1'b1) @@ -763,10 +788,10 @@ module ibex_id_stage #( // cond branch operation // All branches take two cycles in fixed time execution mode, regardless of branch // condition. - id_fsm_d = (data_ind_timing_i || (!BranchTargetALU && branch_decision_i)) ? - MULTI_CYCLE : FIRST_CYCLE; - stall_branch = (~BranchTargetALU & branch_decision_i) | data_ind_timing_i; - branch_set_d = branch_decision_i | data_ind_timing_i; + id_fsm_d = (data_ind_timing_i || (!BranchTargetALU && branch_decision_i)) ? + MULTI_CYCLE : FIRST_CYCLE; + stall_branch = (~BranchTargetALU & branch_decision_i) | data_ind_timing_i; + branch_set_raw_d = (branch_decision_i | data_ind_timing_i); if (BranchPredictor) begin branch_not_set = ~branch_decision_i; @@ -781,7 +806,7 @@ module ibex_id_stage #( // BTALU means jumps only need one cycle id_fsm_d = BranchTargetALU ? FIRST_CYCLE : MULTI_CYCLE; stall_jump = ~BranchTargetALU; - jump_set = jump_set_dec; + jump_set_raw = jump_set_dec; end alu_multicycle_dec: begin stall_alu = 1'b1; @@ -874,14 +899,32 @@ module ibex_id_stage #( // - A load/store error // This will cause a precise exception for the instruction in WB so ID/EX instruction must not // execute + // + // instr_executing_spec is a speculative signal. It indicates an instruction can execute + // assuming there are no exceptions from writeback and any outstanding memory access won't + // receive an error. It is required so branch and jump requests don't factor in an incoming dmem + // error (that in turn would factor directly into imem requests leading to a feedthrough path). + // + // instr_executing is the full signal, it will only allow execution once any potential + // exceptions from writeback have been resolved. + assign instr_executing_spec = instr_valid_i & + ~instr_fetch_err_i & + controller_run & + ~stall_ld_hz; + assign instr_executing = instr_valid_i & ~instr_kill & ~stall_ld_hz & ~outstanding_memory_access; + `ASSERT(IbexExecutingSpecIfExecuting, instr_executing |-> instr_executing_spec) + `ASSERT(IbexStallIfValidInstrNotExecuting, instr_valid_i & ~instr_kill & ~instr_executing |-> stall_id) + `ASSERT(IbexCannotRetireWithPendingExceptions, + instr_done |-> ~(wb_exception | outstanding_memory_access)) + // Stall for reasons related to memory: // * There is an outstanding memory access that won't resolve this cycle (need to wait to allow // precise exceptions) @@ -939,7 +982,8 @@ module ibex_id_stage #( assign stall_ld_hz = 1'b0; // Without writeback stage any valid instruction that hasn't seen an error will execute - assign instr_executing = instr_valid_i & ~instr_fetch_err_i & controller_run; + assign instr_executing_spec = instr_valid_i & ~instr_fetch_err_i & controller_run; + assign instr_executing = instr_executing_spec; `ASSERT(IbexStallIfValidInstrNotExecuting, instr_valid_i & ~instr_fetch_err_i & ~instr_executing & controller_run |-> stall_id)