[cheriot,rtl] Update Ibex to implement CHERIoT ISA on top of CHERIoT subsystem - #31058
Conversation
44ad92b to
32d6338
Compare
9dfffd3 to
bfb86ea
Compare
nasahlpa
left a comment
There was a problem hiding this comment.
Thanks @SamuelRiedel! I looked into it from a FI perspective (so mainly the rv_core_ibex module) and I also had a look into the pentest changes. Apart from one additional hardening (CHERIoT switch) it LGTM!
There was a problem hiding this comment.
I have reviewed commits that are not part of #31048
I have a few inline comments and I would like to have a bit more descriptions in the commit messages:
e.g. here:
The commit message of [cheriot,rtl] Instantiate CHERIoT subsys and meta SRAM ctrl in Earl Grey should be extended with more information since this commit is introducing many changes, e.g.:
- bkdr_loader (add new meta sram target)
- instantiating cheriot subsys
- connecting alerts and adjusting the tests
Ok these commits are already merged to master. It's ok
a9d1078 to
fb444a9
Compare
Thanks for your feedback @gautschimi. It seems most of your comments concerned #30541 and were resolved with rebasing. The remaining changes here are the ones you already reviewed in #31048, and this PR includes all the feedback you gave there. |
gautschimi
left a comment
There was a problem hiding this comment.
Thanks for all this work. Looks good to me!
andreaskurth
left a comment
There was a problem hiding this comment.
Thanks @SamuelRiedel for the clean rebase, this mostly LGTM, just a few smaller points below.
The mode-switch documentation now contradicts the code: hw/ip_templates/rv_core_ibex/doc/theory_of_operation.md.tpl:115 (and the generated hw/top_earlgrey/ip_autogen/rv_core_ibex/doc/theory_of_operation.md:114) still say "The selected mode gates the CHERIoT memory subsystem, but is not routed to Ibex yet", which is exactly what this PR changes. Could that sentence be updated and the doc regenerated?
While in that file: the error-state section (theory_of_operation.md.tpl:134) says the fatal_hw_err alert "also disables Ibex fetch through the local escalation path". cheriot_switch_error only feeds alert_events[2], not local_fetch_enable_d, so as far as I can tell that was not quite true before this PR; it is this PR that makes fetch actually stop, indirectly through Ibex's invalid-MuBi major alert (and only while instr_exec is high). Would it be worth spelling out that mechanism now that it exists?
|
CHANGE AUTHORIZED: hw/vendor/lowrisc_ibex/rtl/ibex_cheriot_ex.sv These files are the vendored copy of the CHERIoT ISA implementation reviewed and merged upstream in lowRISC/ibex#2475, and the tree is a clean copy of upstream 8b8ee08 with no local patches left (the ibex_trvk lint patch is dropped because the fix landed upstream). CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/rv_core_ibex/rtl/rv_core_ibex.sv Regenerated from hw/ip_templates/rv_core_ibex/rtl/rv_core_ibex.sv.tpl: it connects the execution-mode switch output to ibex_top.cheriot_enable_i through a strict MuBi conversion, and adds import ibex_pkg::* with the resulting parameter re-alignment. The switch resets to ePMP mode, so the connection has no effect until software locks CHERIoT mode in. CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/rv_core_ibex/data/rv_core_ibex.hjson The rv_core_ibex parameters RndCnstIbexKeyDefault and RndCnstIbexNonceDefault clashed with the identically named ibex_pkg constants once that package is imported, so they were renamed to RndCnstIbexKey/RndCnstIbexNonce. These three files only follow that rename; the parameter values themselves are unchanged. |
nasahlpa
left a comment
There was a problem hiding this comment.
Thanks Sam - I had a look into the rv_core_ibex module regarding FI resilience. The enable/disable signal there look good to me, an invalid encoding is detected within the ibex_core module where a major alert is triggered.
|
CHANGE AUTHORIZED: hw/vendor/lowrisc_ibex/rtl/ibex_cheriot_ex.sv These files are the vendored copy of the CHERIoT ISA implementation reviewed and merged upstream in lowRISC/ibex#2475, and the tree is a clean copy of upstream 8b8ee08 with no local patches left (the ibex_trvk lint patch is dropped because the fix landed upstream). CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/rv_core_ibex/rtl/rv_core_ibex.sv Regenerated from hw/ip_templates/rv_core_ibex/rtl/rv_core_ibex.sv.tpl: it connects the execution-mode switch output to ibex_top.cheriot_enable_i through a strict MuBi conversion, and adds import ibex_pkg::* with the resulting parameter re-alignment. The switch resets to ePMP mode, so the connection has no effect until software locks CHERIoT mode in. CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/rv_core_ibex/data/rv_core_ibex.hjson The rv_core_ibex parameters RndCnstIbexKeyDefault and RndCnstIbexNonceDefault clashed with the identically named ibex_pkg constants once that package is imported, so they were renamed to RndCnstIbexKey/RndCnstIbexNonce. These three files only follow that rename; the parameter values themselves are unchanged. |
Update code from upstream repository https://github.com/lowRISC/ibex.git to revision 8b8ee086aef72e0833b7f0493d9d33f1e4d3c8e2 * [rtl] Raise an alert if `cheriot_enable` is an invalid MuBi (Samuel Riedel) * [rtl] Imply RV32E when CHERIoT is enabled (Samuel Riedel) * [rtl] Share registers between RV32I and CHERIoT in register file (Samuel Riedel) * [rtl] Unify capability register file ECC ports to match data path (Samuel Riedel) * [rtl] Implement CHERIoT ISA (Samuel Riedel) * [rtl] Rebase CHERIoT Ibex (Samuel Riedel) Signed-off-by: Samuel Riedel <sriedel@lowrisc.org>
Signed-off-by: Samuel Riedel <sriedel@lowrisc.org>
This signal was already waived, but we now have two instances to allow gating the signal if necessary. However, the same waiver applies. Signed-off-by: Samuel Riedel <sriedel@lowrisc.org>
Add new ports to rv_core_ibex and connect convert the mubi4 cheriot_ena_o signal to an ibex_mubi to connect to Ibex's cheriot_enable_i. Ensure that any invalid mubis are forwarded to Ibex as invalid ibex_mubis. Signed-off-by: Samuel Riedel <sriedel@lowrisc.org>
A lot of the parameters heavily rely on ibex_pkg types and constants. Importing the package directly in the module header allows us to save a lot of `ibex_pkg::` typing and thereby allows us to fit all parameters aligned into 100 char wide lines. Also rename rv_core_ibex parameter clashing with ibex package parameters Signed-off-by: Samuel Riedel <sriedel@lowrisc.org>
fb444a9 to
781356d
Compare
Thanks @andreaskurth for the feedback. I updated the documentation and the comment and squashed the changes into the other commits. |
andreaskurth
left a comment
There was a problem hiding this comment.
Great, thanks @SamuelRiedel!
Update Ibex to include lowRISC/ibex#2475, which implements the CHERIoT ISA in the core. This enables a synthesis-time parameter that allows Ibex to support both RV32I + ePMP (the same functionality as current Ibex) and the CHERIoT ISA in EarlGrey. The dynamic mode switch defaults to RV32I for now, so the core's behavior does not change, and CHERIoT is disabled at synthesis time for the other tops.
With updating Ibex, this PR also:
A first version of this PR was opened here: #31048 where it only contained the Ibex changes. This PR was rebased on PRs it depended on, which are now merged, making this PR ready to review/merge and equivalent to #31048.