[rtl,pmp] Allow all accesses to Debug Module in debug mode #2232
Conversation
andreaskurth
force-pushed
the
debug-mode-pmp
branch
from
536142c to
3bf2b66
Compare
andreaskurth
force-pushed
the
debug-mode-pmp
branch
2 times, most recently
from
03b60ba to
37eb820
Compare
There was a problem hiding this comment.
Thanks @andreaskurth this looks good to me, though one thing gives me pause. The current debug mode only effects ID/EX stage behaviour, this introduces something new in the micro-architecture where debug mode also effects IF stage behaviour. We only have one debug mode flag so cannot support a execution where something in ID/EX is running in debug mode and something in IF is not or vice-versa (contrast to the privilege level which has separate ID/EX and IF signals).
From reviewing ibex_controller RTL it looks like on all debug mode entry/exit we will flush the pipeline (as we do a branch) so I don't think this will be an issue however we should write some kind of assertion that checks we never get a debug mode transition without such a flush.
I do want to run a full regression on this before it gets merged. Assuming that looks ok I'll approve.
We can add the assertion referred to above in a follow up PR (e.g. as part of the DV work).
andreaskurth
force-pushed
the
debug-mode-pmp
branch
2 times, most recently
from
792cd71 to
26372e3
Compare
Thanks for the suggestion, I added an assertion. Could you please review it?
Ready for running a full regression from my view |
rtl/ibex_controller.sv
Outdated
| // If entering or exiting debug mode, the pipeline must be flushed. This is because Ibex | ||
| // currently does not support some of the pipeline stages being in debug mode; either all or | ||
| // none of the pipeline stages must be in debug mode. | ||
| `ASSERT(IbexPipelineFlushOnChangingDebugMode, debug_mode_d != debug_mode_q |-> flush_id_o) |
There was a problem hiding this comment.
Unfortunately it's not this simple to write the assertion. For one thing flush_id only effects the ID/EX stage. It sets instr_valid_clear_i which will clear out the current instruction in ID/EX but doesn't prevent a fetched instruction from immediately coming into ID/EX the following cycle.
In particular consider some scenario where we have an instruction in the fetch stage, and it is allowed to execute in debug mode under the PMP rule mode bypass, but wouldn't be allowed to execute outside of debug mode. The PMP check is done whilst it's in the fetch stage. So should we exit debug mode and flush ID/EX (but not IF) then that instruction will proceed into the ID/EX stage and get execute where it should instead cause a PMP exception.
Ideally we'd have an assertion that just generically checks whatever is in IF isn't making it into ID/EX this cycle but that could be fiddly to write. Switch this to flush_id_o & pc_set_o could do the trick. There may be reasonable behaviours that don't meet these requirements however by inspection all of our current debug entry/exit does meet this. So seems reasonable to mandate it for now and if there's some future micro-architectural tweak that breaks the assertion (whilst still executing correctly) we can think about it again then.
There was a problem hiding this comment.
Thanks for the feedback! I extended the assertion and comment as you suggested
|
Regression run was good, though I should try again with the suggested assertion tweak before we merge. |
Signed-off-by: Andreas Kurth <[email protected]>
The RISC-V Debug Specification (current release 1.0.0-rc4) in Section A.2 states that the PMP must not disallow accesses to addresses of the Debug Module when the hart is in debug mode, regardless of how the PMP is configured. This commit changes the PMP accordingly. Signed-off-by: Andreas Kurth <[email protected]>
andreaskurth
force-pushed
the
debug-mode-pmp
branch
from
26372e3 to
68f1a9f
Compare
Great, thanks @GregAC! Could you please try again with the updated assertion? |
The RISC-V Debug Specification (current release 1.0.0-rc4) in Section A.2 states that the PMP must not disallow accesses to addresses of the Debug Module when the hart is in debug mode, regardless of how the PMP is configured. This commit changes the PMP accordingly.
This PR doesn't contain the verification, which is tracked by #2233.