[rtl,cheriot] Implement CHERIoT ISA - #2475
Conversation
This commit takes the entire cheriot-ibex development since it forked from lowrisc/ibex (from bdf2f2b) This commit was ported from https://github.com/microsoft/cheriot-ibex Co-authored-by: Kunyan Liu <kunyanliu@microsoft.com> Signed-off-by: Samuel Riedel <sriedel@lowrisc.org>
66f04da to
3da427b
Compare
3da427b to
544fd00
Compare
d3d453e to
9778006
Compare
andreaskurth
left a comment
There was a problem hiding this comment.
Thanks @SamuelRiedel for this work! As agreed, the first commit imports the Microsoft's CHERIoT implementation, which is based on an earlier fork of Ibex, and rebases it onto the current Ibex, and the second commit reimplements the CHERIoT ISA in a way that fits Ibex's architecture and design very neatly. This two-commit approach makes code provenance and differences explicit, which is important.
I checked your CHERIoT ISA implementation against the v1.0 spec (chapter 9 Sail listings, the §8 encoding tables, and §7.13) and the approved OpenTitan RFC and am convinced that many/most aspects are correct - nice!
One item needs changing in this PR, I think: The register file doesn't seem to implement the RFC's shared 16x65-bit arrangement.
The remaining comments are functional and conformance gaps, and some of them I think are major, but none of them blocks this PR -> follow-up issues.
|
|
||
| assign rf_reg[i] = rf_reg_q; | ||
|
|
||
| if (BaseIsa == BaseIsaRV32IorCHERIoT) begin : g_rf_cap_flops |
There was a problem hiding this comment.
This gives 32 entries of 32-bit data plus a 35-bit capability, whereas the approved RFC proposes a single 16x65-bit file where each entry serves two 32-bit GPRs in base-ISA mode, right?
As implemented, the capability flops of entries 16-31 can never hold a non-zero value: CHERIoT masks register addresses to 4 bits in ibex_decoder, and base-ISA mode always writes NULL_CAP. So that's ~560 permanently-zero flops, and synthesis probably will have a hard time optimizing them away because rf_wcap_a_i isn't a constant.
Could we implement the shared 16x65 arrangement here, or is there a reason it doesn't work that we should write down? I'd rather resolve this one in this PR than defer it.
There was a problem hiding this comment.
Yes absolutely, I already started working on that but then decided to go the simple route first. I will quickly finish this and add it.
| instr_o = {{15 {instr_i[12]}}, instr_i[6:2], instr_i[11:7], {OPCODE_LUI}}; | ||
|
|
||
| if (instr_i[11:7] == 5'h02) begin | ||
| // c.incaddr16csp -> cincoffsetimm csp, csp, nzimm |
There was a problem hiding this comment.
This remap (and c.incaddr4cspn above) is needed because a plain addi writes a NULL capability in CHERIoT mode. Doesn't Zcmp need the same treatment?
cm.push/cm.pop expand via cm_sp_addi() to addi x2, x2, imm, which would clear csp's tag and bounds; cm_push_store_reg()/cm_pop_load_reg() use sw/lw, so cra's backward sentry is lost in memory and the jalr x0, 0(x1) from cm_ret_ra() then takes a tag violation (cd = cnull, cs1 = cra requires a tagged backward sentry). cm.mvsa01/cm.mva01s similarly clear tags on argument registers. The opentitan config enables RV32ZcaZcbZcmp together with BaseIsaRV32IorCHERIoT, so this is reachable.
Could we either expand these to CHERIoT equivalents? Or is Zcmp incompatible with CHERIoT? Follow-up issue is fine with me, not a blocker for this PR.
There was a problem hiding this comment.
You're right. Expanding them to CHERIoT equivalents should not be an issue. I can fix this quickly.
There was a problem hiding this comment.
I was too quick here. Implementing it is easy, but the extensions are not really compatible IMO. Because of the ra (and other capabilities) we might push to the stack. The Zcmp increments the sp by 4 bytes for each register, but if we want to allow pushing capabilities we need 8 bytes per word/capability. I didn't see anything regarding this with CHERIoT, but CHERI mentions that Zcmp is incompatible, and they are creating their own version of it which is not ratified. Let's mark them as incompatible for now and defer this to later. I will keep a copy of the RTL that just uses 8-bytes per stack element.
| end else if ((cheriot_enable_i == IbexMuBiOn) && csr_save_cause_i && ~debug_csr_save_i && | ||
| ~debug_mode_i) begin | ||
| mepc_cap <= pcc_exc_cap; | ||
| end else if ((cheriot_enable_i == IbexMuBiOn) && mepc_en) begin // legacy cssrw; NMI recover |
There was a problem hiding this comment.
On mret out of an NMI, mepc_en is asserted from the nmi_mode_i restore path, so this sets mepc_cap to NULL_CAP. mstack only saves the 32-bit mepc, not the capability.
Doesn't that make the interrupted handler's own mret install an untagged PCC and take a CHERI tag violation, i.e. turn a recoverable NMI into an unrecoverable one in CHERIoT mode? Would stacking the capability alongside mstack_epc_q be the fix?
Follow-up issue is fine, not a blocker.
There was a problem hiding this comment.
I'll have a look and fix it if it's quick, or will create an issue to track this.
| perm_vio_vec[PVIO_EX] = ~rf_fullcap_a.perms.EX; | ||
| end else if (cheriot_operator_i.CCSR_RW) begin | ||
| perm_vio_vec[PVIO_ASR] = ~pcc_cap_i.perms.SR; | ||
| illegal_scr_addr = ~debug_mode_i & (csr_addr_o < 27); |
There was a problem hiding this comment.
CSpecialRW's Sail treats only SCRs 28-31 as existing and calls handle_illegal() for everything else. Two deviations here:
< 27still admits SCR 27 (ZTOPC). Sinceztop_rdata_i/ztop_rcap_iare tied to zero inibex_core.svand there is no write path,cspecialrw cd, 27, cs1silently reads 0 and drops the write instead of trapping.- The
~debug_mode_igate makes SCRs 0-23 legal in debug mode too, where they also read 0 via thedefaultarm of the SCR read mux.
Should this become < 28, with an explicit allowance for the debug SCRs 24-26 only? Follow-up issue is fine, not a blocker.
| // mepc: exception program counter | ||
| CSR_MEPC: mepc_en = 1'b1; | ||
| // disabled for pure cap mode (only allow cap writes) | ||
| CSR_MEPC: mepc_en = ~(BaseIsa == BaseIsaRV32IorCHERIoT) |
There was a problem hiding this comment.
§7.10 says that accessing the replaced RISC-V CSRs (mtvec, mepc) via the CSR* instructions should raise a Reserved Instruction exception in CHERIoT mode.
Here, and for CSR_MTVEC a few lines below, the write is silently dropped instead, and the corresponding reads in the read mux still succeed. So software can csrw mtvec, x and get no indication that it had no effect. Should these two be forced into illegal_csr when cheriot_enable_i == IbexMuBiOn?
Follow-up issue is fine, not a blocker.
There was a problem hiding this comment.
Good catch. I fixed this quickly.
| parameter perms_t PERM_SEA_IMSK = '{default:0}; // Sealing | ||
|
|
||
| // Decode the 6-bit compressed permission encoding to the full 12-bit permissions field. | ||
| function automatic perms_t cheriot_expand_perms(cperms_t cperms); |
There was a problem hiding this comment.
perms is only assigned inside the if/else-if chain, with no final else. The six branches do cover all 64 cperms encodings today, so this is functionally correct, but an unconditional perms = '0; (or a default) up front would keep it X-free if the chain is ever edited. Follow-up is fine.
There was a problem hiding this comment.
This was also quicker to fix than create an issue.
| // Update a capability's address and recompute correction fields. Invalidate the tag if not | ||
| // representable. | ||
| // chktop/chkbase enable explicit top33/base32 bounds checks used for PCC updates. | ||
| function automatic decoded_cap_t cheriot_set_address(decoded_cap_t in_cap, logic [31:0] newptr, |
There was a problem hiding this comment.
chktop/chkbase are 0 at all three call sites (set_address_comb twice and cheriot_pcc_to_mepc), so ptr_below_top and the two extra terms in the condition below are dead. Drop the two arguments?
There was a problem hiding this comment.
Yes, I had this on my cleanup plan, we can drop them and I did it now.
| end else if (cheriot_setaddr_sel_i == SETADDR_RFA_ARITH) begin | ||
| tfcap1 = rf_fullcap_a; | ||
| taddr1 = addr_result; | ||
| end else if ((cheriot_setaddr_sel_i == SETADDR_SCR) && scr_legalization) begin |
There was a problem hiding this comment.
This branch looks redundant. MTCC/MEPCC legalization only clears low address bits, and the rf_rdata_a[1:0] != 2'b00 / rf_rdata_a[0] != 1'b0 checks in main_ex already clear the tag in exactly the cases where the address changes, so setaddr1_outcap's representability result is always overridden by trcap.valid = rf_fullcap_a.valid, and the recomputed cap_cor is identical to the input.
Can SETADDR_SCR and scr_legalization be dropped entirely?
There was a problem hiding this comment.
Yes, I dropped it.
| unique case (exc_pc_mux_i) | ||
| EXC_PC_EXC: exc_pc = { csr_mtvec_i[31:8], 8'h00 }; | ||
| EXC_PC_IRQ: exc_pc = { csr_mtvec_i[31:8], 1'b0, irq_vec, 2'b00 }; | ||
| EXC_PC_EXC: exc_pc = ((BaseIsa == BaseIsaRV32IorCHERIoT) |
There was a problem hiding this comment.
csr_mtvec_i[0] is initialised from csr_mtvec_init_i at boot, so a core that boots in base-ISA mode has it set. After a runtime switch to CHERIoT mode, exceptions would then be vectored here until software writes MTCC, even though CHERIoT supports direct mode only.
Should the direct-mode selection depend on cheriot_enable_i alone rather than also on ~csr_mtvec_i[0]? Follow-up issue is fine.
There was a problem hiding this comment.
Yes, correct. I fixed this quickly.
| } perms_t; | ||
|
|
||
| // Sealing types (spec v1.0, chapter 7.13.2) | ||
| parameter otype_t OTYPE_UNSEALED = 3'd0; |
There was a problem hiding this comment.
otype 1 (interrupt-inheriting forward sentry) isn't named here, so ibex_cheriot_ex ends up comparing against the bare literal 3'h1 in the CJALR otype checks. Add an OTYPE_SENTRY_INHERIT = 3'd1 for symmetry with the other four?
There was a problem hiding this comment.
Right. There were also other hardcoded comparisons missing the parameter. Fixed it all.
|
|
||
| input logic [31:0] trvk_heap_base_addr_i, | ||
|
|
||
| input ibex_mubi_t cheriot_enable_i, |
There was a problem hiding this comment.
This pin needs to be connected for the instance at dv/riscv_compliance/rtl/ibex_riscv_compliance.sv:168 (hence CI currently fails).
| end | ||
|
|
||
| cfg.fixed_data_write_response = secure_ibex; | ||
| // Write response data fields are X when fixed_data_write_response is unset. CHERIoT's TRVK |
There was a problem hiding this comment.
Did we not have such a DataKnown_A assertion in Ibex before?
There was a problem hiding this comment.
No, I was also surprised, but I think the DV on purpose drives X in the non-secure mode.
| input logic valid_i, | ||
| input logic id_in_ready_i, | ||
| input logic [31:0] instr_i, | ||
| input ibex_mubi_t cheriot_enable_i, |
There was a problem hiding this comment.
There is a secure adaptations between them, right? The surrounding CHERIoT system uses OT mubi.
There was a problem hiding this comment.
Yes, we had the same issue with mcounteren_writable_i
| output logic [ 3:0] rvfi_mem_wmask, | ||
| output logic [31:0] rvfi_mem_rdata, | ||
| output logic [31:0] rvfi_mem_wdata, | ||
| output logic rvfi_mem_is_cap, |
There was a problem hiding this comment.
the CHERIoT subsystem currently uses the same bit for write capabilities and hinting. keeping them separate in Ibex is fine though. When implementing the security countermeasures, we need to ensure that the capabilities between the two lockstep domains are implemented using mubi.
There was a problem hiding this comment.
True, I can combine them.
thommythomaso
left a comment
There was a problem hiding this comment.
I did a quick review focusing mainly on the interfaces to the rest of the CHERIoT system.
Integrate the CHERIoT (Capability Hardware Extension to RISC-V for IoT)
extension into Ibex, building on the previously rebased cheriot-ibex
code. This merges CHERIoT logic directly into the core Ibex RTL and
renames all `cheri_*` instances to `cheriot_*`.
New modules:
- `ibex_cheriot_ex.sv`: Standalone CHERIoT execution stage (formerly
`cheri_ex`). Handles capability arithmetic, bounds checks,
sealing/unsealing, PCC updates, and LSU capability requests.
- `ibex_cheriot_pkg.sv`: Shared types, constants, and functions matching
the v1.0 specification (formerly `cheri_pkg.sv`, heavily
restructured).
Removed cheriot-ibex modules:
- `cheri_pkg.sv`, `cheri_decoder.sv`, `cheri_ex.sv`, `cheri_regfile.sv`,
`cheri_stkz.sv`, `cheri_tbre.sv`, `cheri_tbre_wrapper.sv`,
`cheri_trvk_stage.sv`
CHERIoT integration into existing Ibex modules:
- `ibex_decoder.sv`/`ibex_compressed_decoder.sv`: Decode CHERIoT
instructions. Emit `cheriot_op_t` and selector signals to EX.
- `ibex_id_stage.sv`/`ibex_controller.sv`: Add stall and flush logic
for capability exceptions and CHERIoT execution cycles.
- `ibex_cs_registers.sv`: Add CHERIoT CSRs/SCRs.
- `ibex_load_store_unit.sv`: Support capability-tagged loads and stores.
- `ibex_register_file_{ff,fpga,latch}.sv`: Store capability metadata
(`cap_t`) alongside data registers.
ISA selection and runtime control:
- `BaseIsa`: Build parameter (`ibex_pkg::base_isa_e`) to select between
`BaseIsaRV32I` (RV32I only, CHERIoT logic not synthesized) and
`BaseIsaRV32IorCHERIoT` (CHERIoT and RV32I synthesized).
- `cheriot_enable_i`: Dynamic runtime switch (`ibex_mubi_t`) to toggle
the core between plain RV32I and CHERIoT modes.
Revocation:
- Removed the internal revocation engine. The existing `ibex_trvk.sv`
filter is now connected after the LSU in `ibex_top.sv` and updated to
use shared `ibex_cheriot_pkg` types.
Key differences from microsoft/cheriot-ibex:
- Renamed all modules (`cheri_*` -> `ibex_cheriot_*` / `ibex_*`).
- Merged all stages (except EX) into existing Ibex modules instead of
adding parallel pipelines.
- Dropped legacy instruction formats, stack zeroing (`cheri_stkz`), and
the TBRE engine (`cheri_tbre*`).
- Implemented capability types as packed structs that match the v1.0
spec bit layout exactly.
- Replaced `CHERIoTEn` and related build parameters with a single
`BaseIsa` parameter and the `cheriot_enable_i` runtime signal.
Signed-off-by: Samuel Riedel <sriedel@lowrisc.org>
Those arguments were never used anymore
9778006 to
a362219
Compare
andreaskurth
left a comment
There was a problem hiding this comment.
Thanks for turning these around so quickly, @SamuelRiedel; nice update!
I re-reviewed only the delta. Three things left: the register file is still only half-done, the NMI/MEPCC item is still open, and one of my earlier requests turned out to be too broad and has introduced a small divergence (details inline).
|
|
||
| assign rf_reg[i] = rf_reg_q; | ||
|
|
||
| if ((BaseIsa == BaseIsaRV32IorCHERIoT) && (i < 16)) begin : g_rf_cap_flops |
There was a problem hiding this comment.
Following up on the earlier thread on this line, where you said you'd finish the shared file: the i < 16 limit does remove the ~560 permanently-zero capability flops, so the dead-flop half is fixed. The RFC's actual proposal (one 16x65-bit file where each entry also serves two 32-bit GPRs in base-ISA mode) isn't there yet, so this is still 32x32 data + 16x35 cap = 1584 bits against the RFC's 1040. Are you still planning to land that here?
ibex_register_file_latch.sv still instantiates capability latches for all 31 entries, 16 of which can never be non-zero, so the same limit applies there. (The FPGA variant looks fine as-is; RAM inference wants the full depth.)
| ~debug_mode_i) begin | ||
| mepc_cap <= pcc_exc_cap; | ||
| end else if ((cheriot_enable_i == IbexMuBiOn) && mepc_en) begin // legacy cssrw; NMI recover | ||
| mepc_cap <= NULL_CAP; |
There was a problem hiding this comment.
Following up on the earlier thread on this branch: still open, so an issue to track it would be good.
One thing that changed around it: now that 164c9945 makes a legacy csrw mepc trap in CHERIoT mode, illegal_csr feeds illegal_csr_insn_o which gates csr_we_int, so the CSR_MEPC arm of the write case can no longer assert mepc_en in CHERIoT mode. The NMI restore path is therefore the only remaining way into this branch, which both sharpens the bug and makes the // legacy cssrw; NMI recover comment stale. Worth updating the comment either way.
| if (cheriot_cs2_dec_i == CHERIOT_SCR_MTCC) begin | ||
| // MTVEC/MTCC legalization (clear tag if checking fails) | ||
| csr_wdata_o = {rf_rdata_a[31:2], 2'b00}; | ||
| trcap = rf_rcap_a; |
There was a problem hiding this comment.
My earlier request to drop the SETADDR_SCR path was too broad, sorry! It also removed the cap_cor recompute, and I think that part wasn't redundant.
trcap = rf_rcap_a carries the correction factors computed for the original address, but csr_wdata_o stores the legalized one. On the path where legalization actually changes the address the tag is cleared, but cap_cor is still architecturally observable: read MTCC back with CSpecialRW and apply CGetBase/CGetTop to the untagged result. With E=0, B=0x002, T=0x008 and an address ending in 0x3, the original gives addr_mid=3 -> addr_hi=0 -> cap_cor=2'b00 and the legalized address gives addr_mid=0 -> addr_hi=1 -> cap_cor=2'b11, so the decoded base differs by 512. The old comment ("so that RTL behavior would match sail") was pointing at exactly this case.
Would you mind either recomputing cap_cor for the legalized address here, or checking whether the Sail co-simulation compares the metadata of untagged capabilities at all? If it doesn't, this is harmless and worth a short comment saying so. Same applies to the MEPCC branch below.
Implement CHERIoT ISA
This PR integrates the full CHERIoT ISA specification v1.0 into Ibex, building on the rebased microsoft/cheriot-ibex code (the first commit in this series).
Main features
This PR adds complete CHERIoT ISA support to Ibex. The key design decisions include:
ibex_cheriot_ex) is a standalone new module.ibex_cheriot_pkg): Capability types are packed structs whose lower bits match the spec encoding 1:1, so their vector forms match the format in memory. Two forms are used throughout the design:cap_t(compressed capabilities with correction factors, which live in the register file and on load/store ports) anddecoded_cap_t(uncompressed bounds and permissions, which are used inside the core). The correction factors (2 bits) are stored in the register file to prevent the need for expensive comparators on every capability read.ibex_trvk) is connected after the LSU inibex_topto keep the pipeline clean.cheri_stkzandcheri_tbremodules from the Microsoft fork are not ported. The scope of this PR is limited strictly to the ISA.cap_tmetadata word. The upper 16 registers are integer-only. All three register file variants (FF, FPGA, and latch) are updated consistently.cheri_*toibex_cheriot_*oribex_*, and legacy instruction formats are dropped.Parametrization
BaseIsa(ibex_pkg::base_isa_e): This is a static build-time switch.BaseIsaRV32Ielaborates away all CHERIoT hardware (no change through this PR).BaseIsaRV32IorCHERIoTincludes both RV32I and CHERIoT modes.cheriot_enable_i(ibex_mubi_t): This is a dynamic runtime switch. When CHERIoT hardware is present and this signal is deasserted, the core behaves as a plain RV32I core. This signal has to be held constant throughout execution with one exception. Ibex can switch from Off (RV32I mode) to On (CHERIoT mode) exactly once at runtime. Ibex can never switch from CHERIoT mode to RV32I mode without a reset.Verification
BaseIsaRV32I.BaseIsaRV32IorCHERIoTwithcheriot_enable_iturned off runs all existing RV32I tests without regressions.cheriot_enable_iturned on) is verified with basic smoketests.BaseIsaRV32IorCHERIoTenabled, meaning we have both ISAs available in this configuration. However, all current testbenches (cosim, simple systems, formal) still disable CHERIoT using thecheriot_enable_isignal.To-Dos for follow up PRs