search

microsoft / cheriot-ibex

cheriot-ibex is a RTL implementation of CHERIoT ISA based on LowRISC's Ibex core.
Apache License 2.0
73 stars 14 forks source link

Register address top bit #30

Closed mndstrmr closed 1 month ago

mndstrmr commented 5 months ago

In RV32E mode the RTL clears the top bit of all register addresses for all purposes. The Sail does not do this, creating the following issues:

  1. Attempts to read or write from a register >= 16 is an internal error in the Sail.
  2. handle_cheri_reg_exception(..., cs1) will put cs1 into MTVAL with its top bit potentially set, unlike in the RTL. This happens in
    • LoadCapImm/StoreCapImm
    • ext_data_get_addr as auth_idx
    • CJALR
  3. CSpecialRW checks if cs1 = zeros(). This should presumably also fail if cs1 = 16.

I am of the opinion that this is a specification bug, so I would have the Sail force the top bit of the register indices to zero as well.

kliuMsft commented 5 months ago

@rmn30 let me know what you think..

rmn30 commented 5 months ago

Yes, the Sail is lax about the unused register indexes. I guess it should raise Reserved Instruction exception if bit 4 is set in register operand?

mndstrmr commented 4 months ago

If it would be useful I would be happy to prove the line numbers of everywhere in the Sail where I clear the top bit of a register index.

The decision is of course yours, but depending on how the exceptions are raised in the Sail there is the possibility for this to be a much harder to prove correctness of than the version with truncation as is already seen in the RTL.

If you do choose to raise an exception, ideally this would be done during decoding, or during some stage between decode and execute.

marnovandermaas commented 4 months ago

From an implementation point of view, it's much easier and performant to ignore the top register index bit rather than throwing an exception.

kliuMsft commented 4 months ago

Yeah implementation-wise ignoring bit 4 is easier (that's why I did it ;) I think the verbal version of risc-v spec didn't really say what's supposed to happen in this case (running RV32I code on RV32E hardware). @rmn30, is there a sail model for RV32E or it's just us?

nwf commented 4 months ago

Yes, the Sail is lax about the unused register indexes. I guess it should raise Reserved Instruction exception if bit 4 is set in register operand?

Is that not what https://github.com/microsoft/cheriot-sail/commit/4185ba2e89087d4062dd0f5a7efa5ecd814b88de does?

kliuMsft commented 3 months ago

I have checked in RTL changes in the decoder design (9a23c81) to generate illegal instruction exceptions when rd/rs1/rs2 address >=16.

@mndstrmr, this is a relatively involved change so if you could prioritize verifying this I'd really appreciate it.

ColinDBates commented 2 months ago

Hi Kunjan,

I'm using the Cheri Ibex as part of the Sonata System. I ran into a problem when register x26 was accessed and came across this Issue. I see (9a23c81) now will cause an error when this happens.

I was wondering if there is a reason for not replacing CheriLimit16Regs with the RV32E parameter? My understanding is that now the encoding of the opcode auicgp has been changed there is no longer a 16 register limit when Cheri is enabled, so tying CheriLimit16Regs to CHERIoTEn is maybe no longer appropriate?

Cheers

kliuMsft commented 2 months ago

Hi Colin,

32-register is not an allowed configuration for CHERIoT (CHERIoTEn == 1 && cheri_pmode_i == 1), which is why in that mode the core always faults if it seems an attempt to access the upper 16 registers. It has to do with the arch and rtos design (e.g., spilling/scrubbing extra registers actually hurts performance and security). When pmode_i = 0 (RV32 mode) we do support both 16 and 32 registers (controlled by the RV32E parameter).

BTW I was running some verification in the background and it turns the earlier checkin didn't quite do what it supposed (still not generating exception when the upper registers are referred). The latest check in (5e4798f should fix it.

Thx

ColinDBates commented 2 months ago

Hi Kunyan,

Thanks for the quick reply on this. Unfortunately, I'm on holiday at the moment, so can't enter into a proper discussion on this. I'm back on the 15/16-Jul. So will come back to you then. For now, thanks for the heads up about the 5e4798fhttps://github.com/microsoft/cheriot-ibex/commit/5e4798f67417bcf87c831c56b8980af0e5d3966c change.

Cheers,

Colin

From: Kunyan Liu @.> Sent: 04 July 2024 01:38 To: microsoft/cheriot-ibex @.> Cc: Colin Bates @.>; Comment @.> Subject: Re: [microsoft/cheriot-ibex] Register address top bit (Issue #30)

Hi Colin,

32-register is not an allowed configuration for CHERIoT (CHERIoTEn == 1 && cheri_pmode_i == 1), which is why in that mode the core always faults if it seems an attempt to access the upper 16 registers. It has to do with the arch and rtos design (e.g., spilling/scrubbing extra registers actually hurts performance and security). When pmode_i = 0 (RV32 mode) we do support both 16 and 32 registers (controlled by the RV32E parameter).

BTW I was running some verification in the background and it turns the earlier checkin didn't quite do what it supposed (still not generating exception when the upper registers are referred). The latest check in (5e4798fhttps://github.com/microsoft/cheriot-ibex/commit/5e4798f67417bcf87c831c56b8980af0e5d3966c should fix it.

Thx

— Reply to this email directly, view it on GitHubhttps://github.com/microsoft/cheriot-ibex/issues/30#issuecomment-2207677953, or unsubscribehttps://github.com/notifications/unsubscribe-auth/BGFCAZ2G4ZEQDMGDMHDHXNLZKSKOTAVCNFSM6AAAAABF7G4HRSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBXGY3TOOJVGM. You are receiving this because you commented.

mndstrmr commented 1 month ago

I have checked in RTL changes in the decoder design (9a23c81) to generate illegal instruction exceptions when rd/rs1/rs2 address >=16.

@mndstrmr, this is a relatively involved change so if you could prioritize verifying this I'd really appreciate it.

Seems to be proving, thanks for the work. I do have local spec changes to help this, specifically I fail to decode instead of throwing Sail exceptions, which I am not convinced about: A CSR would mutate the CSR then throw an illegal instruction exception in Sail, but not in the RTL.

rmn30 commented 1 month ago

A CSR would mutate the CSR then throw an illegal instruction exception in Sail, but not in the RTL.

That is indeed very strange and looks like a bug in the current Sail specification. Failing to decode is preferable but we took the exception route to avoid having to make extensive changes to the upstream sail-riscv spec. I wonder if there are other instructions that are broken in this way? This would apply to any instruction that has side-effects prior to writing the destination register for example jump and link, or store-conditional.