Implement turning on and off PC hardening. #395
Conversation
…ning. Includes the following bugfixes: - For a pipeline flush due to CSR writes, the controller could kill the pipeline for debug or interrupts before the flush was completed. - The id_ex_pipe.pc_next was wrong for dummy branches (should have been +0 instead of +4) Signed-off-by: Oystein Knauserud <Oystein.Knauserud@silabs.com>
silabs-oysteink
added
the
Component:RTL
rtl/cv32e40s_controller_fsm.sv
Outdated
|
|
||
| // Allowing NMI's follow the same rule as regular interrupts, except we don't need to regard blanking of NMIs after a load/store. | ||
| // If the core woke up from sleep due to either debug or regular interrupts, the wakeup reason is honored by not allowing NMIs in the cycle after | ||
| // waking up to such an event. | ||
| assign nmi_allowed = lsu_interruptible_i && debug_interruptible && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible && !(woke_to_debug_q || woke_to_interrupt_q); | ||
| assign nmi_allowed = lsu_interruptible_i && debug_interruptible && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible && !(woke_to_debug_q || woke_to_interrupt_q || csr_flush_ack_q); |
There was a problem hiding this comment.
Why did you add csr_flush_ack_q in '!(woke_to_debug_q || woke_to_interrupt_q || csr_flush_ack_q)' instead of using '&& !csr_flush_ack_q && !(woke_to_debug_q || woke_to_interrupt_q)'. Of course it is logically the same, but !(woke_to_debug_q || woke_to_interrupt_q) was there for a very specific reason different from why not to allow an NMI during csr_flush_ack_q
There was a problem hiding this comment.
Refactored, it looks more clear when not included within the parentheses.
| @@ -186,6 +194,7 @@ always_ff @(posedge clk, negedge rst_n) begin | |||
| if_id_q <= 1'b0; | |||
| jmp_taken_q <= 1'b0; | |||
| bch_taken_q <= 1'b0; | |||
| enable_q <= 1'b0; | |||
There was a problem hiding this comment.
There is a lot of state in this file. I think we need to guarantee that all state goes back to its reset values when PC hardening is disabled. Now it is very difficult to see if disabling and reenabling PC hardening leaves the state consistent here.
There was a problem hiding this comment.
Fixed, resetting state when enable==1'b0.
…ing is not enabled. Refactored interrupt_allowed slightly for clarity. Signed-off-by: Oystein Knauserud <Oystein.Knauserud@silabs.com>
| @@ -140,7 +144,8 @@ assign check_addr = !pc_set_q ? incr_addr : {ctrl_flow_addr[31:1], 1'b0}; | |||
| // Comparator for address | |||
| // Comparison is only valid the cycle after pc_set or the cycle | |||
| // after an instruction goes from IF to ID. | |||
| assign addr_err = (pc_set_q || if_id_q) ? (check_addr != pc_if_i) : 1'b0; | |||
| assign addr_err = !enable ? 1'b0 : | |||
There was a problem hiding this comment.
Why is this error only suppressed based on 'enable' while the other errors are also suppressed based on enable_q?
There was a problem hiding this comment.
This depends on either pc_set_q or if_id_q being 1'b1, which will only happen after enable->flush->pc_set. For the others, the pipeline content may be in a state which may flag an error during the flush.
Implemented turning pc_hardening on and off by using cpuctrl.pc_hardening.
Includes the following bugfixes:
Signed-off-by: Oystein Knauserud Oystein.Knauserud@silabs.com