mcause.minhv clear from WB stage #693
Conversation
… to WB. This aligns with other state changes that also happens from WB. Remoted separate single_step_pending flag for pointer fetches, as single step now can use the wb_valid for the pointer to signal step entry. Signed-off-by: Oystein Knauserud <[email protected]>
silabs-oysteink
added
the
Component:RTL
rtl/cv32e40x_controller_fsm.sv
Outdated
| // Once the first part of a table jump has finished in WB, we are not allowed to take debug before the last part finishes. This can be detected when the last | ||
| // part of a table jump is in either EX or WB. // todo: update comments related to table jump (explain general concept and to which instructions it applies) | ||
| // todo: why is clic_ptr_in_pipeline included here (and not in interrupt_allowed) | ||
| // Any multi operation instruction (tablejumps, push/pop and doublemoves) may not be interrupted once the first operation has completed its operation in WB. |
There was a problem hiding this comment.
tablejumps -> table jumps
doublemoves -> double moves
rtl/cv32e40x_controller_fsm.sv
Outdated
| // reaches the ID stage. If the pointer fetch fails, an exception will be taken from WB and debug mode is entered with dpc | ||
| // pointing at the first instruction of the exception handler. | ||
| // single step becomes pending when the last operation of an instruction is done in WB (including CLIC pointers), or we ack a non-shv interrupt. | ||
| // If a CLIC SHV interrupt is taken during single step, a pointer that reaches WB will trig the debug entry. |
rtl/cv32e40x_controller_fsm.sv
Outdated
| @@ -456,10 +454,12 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*; | |||
| // will be interruptable as they were converted to NOP in ID stage. | |||
| // The cycle after fencei enters WB, the fencei handshake will be initiated. This must complete and the fencei instruction must retire before allowing interrupts. | |||
| // TODO:OK:low May allow interuption of Zce to idempotent memories | |||
| // Once the first part of a table jump has finished in WB, we are not allowed to take interrupts before the last part finishes. This can be detected when the last | |||
| // part of a table jump is in either EX or WB. | |||
| // Any multi operation instruction (tablejumps, push/pop and doublemoves) may not be interrupted once the first operation has completed its operation in WB. | |||
There was a problem hiding this comment.
tablejumps -> table jumps
doublemoves -> double moves
|
|
||
| assign interrupt_allowed = lsu_interruptible_i && debug_interruptible && !fencei_ongoing && !xif_in_wb && sequence_interruptible && !interrupt_blanking_q; | ||
| assign interrupt_allowed = lsu_interruptible_i && debug_interruptible && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible && !interrupt_blanking_q; |
There was a problem hiding this comment.
Why is !clic_ptr_in_pipeline not added to nmi_allowed?
There was a problem hiding this comment.
It should be included in nmi_allowed, got left out by mistake. Added it.
| @@ -874,6 +874,14 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*; | |||
| if (mret_in_wb && !ctrl_fsm_o.kill_wb) begin | |||
| ctrl_fsm_o.csr_restore_mret = !debug_mode_q; | |||
| end | |||
|
|
|||
| // CLIC pointer in WB | |||
| if(clic_ptr_in_wb && !ctrl_fsm_o.kill_wb) begin | |||
There was a problem hiding this comment.
Can you add an assertion that 'clic_ptr_in_wb && !ctrl_fsm_o.kill_wb' cannot be true when in any other state than FUNCTIONAL?
There was a problem hiding this comment.
Added assertion.
| // Function pointer reached ID stage, do another jump | ||
| // if no faults happened during pointer fetch. (mcause.minhv will stay high for faults) | ||
| // todo: deal with integrity related faults for E40S. | ||
| // Function pointer reached ID stage, do another jump if no faults happened during pointer fetch. |
There was a problem hiding this comment.
Add back in // todo: deal with integrity related faults for E40S.
Add that remark everywhere where this similar MPU and bus error check is done
There was a problem hiding this comment.
Added back in remark, for the POINTER_STATE remark I believe E40S is up to date on this, but for the moved clear_minhv handling it is not yet updated (will do after merging X->S).
sva/cv32e40x_controller_fsm_sva.sv
Outdated
|
|
||
|
|
||
|
|
||
| a_mret_restore_halt: |
There was a problem hiding this comment.
Can you add a comment what the 'idea' behind this check is?
There was a problem hiding this comment.
Added comment.
sva/cv32e40x_core_sva.sv
Outdated
| // no faults, debug is entered with dpc pointing to the handler entry when the pointer reaches the WB stage. | ||
| // Otherwise, if the pointer fetch failed, we will start fetching the appropriate exception handler | ||
| // before entering debug with DPC pointing to the first exception handler instruction. | ||
| // Once the second fetch has been performed, an external debug request may cause debug entry before |
There was a problem hiding this comment.
Is the remark "Once the second fetch has been performed, an external debug request may cause debug entry before ..." really true? I thought you just made sure that this sequence would not get interfered with because of interrupts or debug.
There was a problem hiding this comment.
That is actually not a correct remark. Removed remark and added comment about blocking debug and interrupts while there is a live pointer in the pipeline.
Signed-off-by: Oystein Knauserud <[email protected]>
Moved clearing of mcause.minhv for successful pointer fetches from ID to WB. This aligns with other state changes that also happens from WB. Should fix issue where a killed mret could still lead to clearing of mcause.minhv.
Remoted separate single_step_pending flag for pointer fetches, as single step now can use the wb_valid for the pointer to signal step entry.
Signed-off-by: Oystein Knauserud [email protected]