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

MRet Instruction #35

Closed mndstrmr closed 1 month ago

mndstrmr commented 4 months ago

The MRet instruction differs between RTL and Sail in two ways:

  1. The RTL should clear the mstatus MPRV if the new privilege level is not machine.
  2. The RTL currently throws an illegal instruction exception if the PCC cannot access system registers, but it should throw a CHERI exception instead according to the Sail.
kliuMsft commented 4 months ago

1: @marnovandermaas, maybe you can take a look? I think it might be an ibex thing.

2: I will try to fix - possibly merge with the fix for CSR access permission violation.

davidchisnall commented 4 months ago

I don't think 1 should affect CHERIoT Ibex, since we have no modes other than M and therefore can't transition to them, though it may affect non-CHERIoT Ibex.

mndstrmr commented 4 months ago

MRET will set the new privilege level to mstatus.mpp, which is initialised as PRIV_LVL_U, which unless I am missing something means one can easily enter unprivileged mode. I can find a full trace if you like.

marnovandermaas commented 4 months ago
1. The RTL should clear the mstatus MPRV if the new privilege level is not machine.

This is a difference between version 1.11 and 1.12 of the privileged specification. Upstream this has already been fixed: https://github.com/lowRISC/ibex/commit/423264ce5fd6b3ca560cae1749d86dbcfdd794a7 I suggest that we stay with version 1.11 for CHERIoT Ibex unless we want to rebase on upstream.

kliuMsft commented 4 months ago

RTL commit 4a739b4 changed the behavior of MRET to throw a CHERI exception (mcause = 0x1c, mtval =0x418) for MRET without ASR permission.

mndstrmr commented 4 months ago

Fix is correct, thanks.

mndstrmr commented 4 months ago

Just realised the first issue hasn't been fixed yet.

kliuMsft commented 4 months ago

Good point.. will try to merge in the ibex fix pointed out by Marno.

kliuMsft commented 4 months ago

@mndstrmr , I have made an RTL update (876a46a to include the mprv fix. Please verify.

mndstrmr commented 1 month ago

This proves, thanks.