CSR Instruction

[mndstrmr]

It is fairly difficult for me to verify the RTL for the CSR instruction against the Sail, since they are not really that related at the moment. I have done so, but it requires that I change a fairly large amount of the Sail. In particular:

1. There are many CSRs present in the Sail that do not exist in ibex
2. The implementation of some CSRs clearly do not match the RTL (e.g. legalisation of mcause and mstatus, and writeability of mcounteren).
3. The CHERI access_system_registers checks in the RTL clear the output bits, but do not raise an exception. This is fine but the Sail does not do this. Ideally either the RTL would throw an exception, or the Sail would look like the following:

/* snip */
if not(check_CSR(csr, cur_privilege, isWrite))
then { handle_illegal(); RETIRE_FAIL }
else {
  let can_read = pcc_access_system_regs() | (csr[11..8] == 0xb);
  // or whatever condition is precisely intended ^
  let csr_val = if can_read then readCSR(csr) else 0x00000000;
  if isWrite & pcc_access_system_regs() then {
    let new_val : xlenbits = match op {
      CSRRW => rs1_val,
      CSRRS => csr_val | rs1_val,
      CSRRC => csr_val & ~(rs1_val)
    };
    writeCSR(csr, new_val)
  };
  X(rd) = csr_val;
  RETIRE_SUCCESS
}

[kliuMsft]

Acknowledged. I have to think about this one - originally we did this in RTL b/c ibex doesn't generate any CSR-related faults at EX time (illegal csr address is considered a decoder error).

[davidchisnall]

The benefit of an exception on invalid CSRs in general is that you can trap and emulate them. I don't see a huge value in doing that for CHERIoT, since we don't expect to guarantee binary compatibility between cores (or versions of the same core). I'm happy with the Ibex behaviour. @rmn30, is there a problem making the Sail the same here (and removing the CSRs that we don't need / have)?

[kliuMsft]

RTL commit 4a739b4 changed the behavior to throw a CHERI exception for CSR accesses without an ASR permission.

Will have to check with upstream ibex about mcause/mstatus legalization.

[marnovandermaas]

For MSTATUS legalization there is the following commit upstream that has changed: lowRISC/ibex@e53a02a
The RTL marks MCOUNTEREN as read only zero which I guess means it is not enabled. It's probably easiest to remove it from Sail as well.
I couldn't find any changes upstream for MCAUSE.

What exact differences are you seeing with MSTATUS and MCAUSE?

[mndstrmr]

This almost verifies:

I believe the below two lines also need & instr_valid_i. Everything unsurprisingly falls apart otherwise, I assume that's just a simple mistake because all the other exception signals (e.g. ecall_insn, cheri_ex_err) have it too.

cheriot-ibex/rtl/ibex_controller.sv

Lines 239 to 240 in 4a739b4

	assign mret_cheri_asr_err = CHERIoTEn & cheri_pmode_i & ~csr_pcc_perm_sr_i & mret_insn;
	assign csr_cheri_asr_err = CHERIoTEn & cheri_pmode_i & ~csr_pcc_perm_sr_i & csr_access_i & ~illegal_insn_i;

Also, the Sail ext_check_CSR function does allow some CSR reads without ASR, which this doesn't. The RTL is stricter than it needs to be in that sense so you could justifiably change the Spec to always disallow any CSR operation without ASR.

[kliuMsft]

You are right - line 239 doesn't need the instr_valid_i since it's included in mret_insn already, but the csr_access_i on line 240 is straightly from the decoder and thus need to be qualified by instr_valid_i.

I have to think about how to handle those "allowed" CSR's.. It's a bit of pain since we have to decode those individually and the decision goes into instr_kill -> instr_executing which fanout to many places.

[mndstrmr]

You are right - line 239 doesn't need the instr_valid_i since it's included in mret_insn already, but the csr_access_i on line 240 is straightly from the decoder and thus need to be qualified by instr_valid_i.

Good point, thanks.

[kliuMsft]

@mndstrmr I checked in RTL changes (83a872e to allow access to CSR 0xB00-0xB03 (mcycle, etc) and 0xC00-0xC03 (unprivileged counters). Note the ibex currently don't have unprivileged counters implemented. @rmn30 could you please check if any sail-update is needed?