riscv / riscv-isa-manual

RISC-V Instruction Set Manual
https://riscv.org/
Creative Commons Attribution 4.0 International
3.7k stars 645 forks source link

[CFI-zicfiss] updating pseudocode in ssamoswap unpriv #1729

Open kacouane opened 6 days ago

kacouane commented 6 days ago

as M mode behavior was unclear on priority vs g/page fault ...

ved-rivos commented 6 days ago

I don't think this edit is required. The memory accesses are performed at the effective privilege level. When the effective privilege level is M, virtual memory system is not active. When effective privilege level is not M, then virtual memory system is always active. Given these two are mutually exclusive, there is no prioritization required.

aswaterman commented 6 days ago

I defer to Ved on this matter.

kacouane commented 4 days ago

Yes, I agree no virtual mem in machine mode. I meant there might be confusion since we have a sentence defining the M mode behavior in the privilege spec, and it is not reflected here in the unpriv side pseudocode (which explains other privileges). While the (guest) page fault rules descriptions leave in privilege spec ...

Also, I felt we could remind people that some cases might trigger a fault before the memory swap is performed.

So, I added the memory checks in the pseudocode, but I can only add the M mode case if you think it is better.

ved-rivos commented 2 days ago

We have a sentence about the effective privilege mode of the access. This instruction can be used in M mode but it cannot be used with effective privilege mode of M.

This code may be misleading due to that. If ssamoswap was invoked with effective privilege mode of S and at privilege mode M, then this pseudocode would incorrectly imply that if the memory access with effective privilege mode of S did not cause a page/guest-page/access-fault at lines 2 then it would cause an access-fault at line 4 due to privilege being M.

1.      if page-fault / guest-page-fault / access-fault
2.           raise accordingly
3.       else if privilege_mode == M
4.           raise store/amo access-fault exception
5.       else

How about we not add pseudo-code, but we instead add a cross reference to the Priv spec.

Please see "Shadow Stack Memory Protection" section in Volume II of this manual for the exceptions raised by SSAMOSWAP.W/D

kacouane commented 1 day ago

The cross-ref seems like a good idea. I will update the PR accordingly

ved-rivos commented 22 hours ago

Instead of repeating the cross reference twice, could we place it as a single paragraph i.e. between the paragraph beginning with "Just as for AMOs in the A extension..." and the non-normative note that follows it.