Legalize xret privilege with respect to misa. #1539
Conversation
|
cc @nwf |
| private function legalize_xret_privilege(p : Privilege) -> Privilege = | ||
| match p { | ||
| Machine => Machine, | ||
| // If S-mode is not legal, then go to M-mode. |
There was a problem hiding this comment.
I'm not sure about this. The spec says:
When executing an xRET instruction, supposing xPP holds the value y, xIE is set to xPIE; the privilege mode is changed to y;
If x is M, y is S and misa.S is clear (and assuming misa.S == 0 implies S is not supported), and U is supported, should privilege mode be changed to M or U?
There was a problem hiding this comment.
mstatus.MPP is WARL, so based on the CSR Field Modulation rules, this is implementation specific. Presumably that means we need a config option for it.
If a write to one CSR changes the set of legal values allowed for a field of a second CSR, then unless specified otherwise, the second CSR’s field immediately gets an UNSPECIFIED value from among its new legal values. This is true even if the field’s value before the write remains legal after the write; the value of the field may be changed in consequence of the write to the controlling CSR.
Timmmm
left a comment
Timmmm
left a comment
There was a problem hiding this comment.
I think this probably isn't the right way to handle this. The thing that we don't actually do is the CSR field modulation of mstatus.MPP:
xPP fields are WARL fields that can hold only privilege mode x and any implemented privilege mode lower than x. If privilege mode x is not implemented, then xPP must be read-only 0.
So it's not really that we're legalising the xret privilege, instead we should be modulating the xPP field so that it dynamically reads a legal value (or 0 if there are none).
As a concrete example of where this fix would still be broken, if you set MPP to S, write misa.S=0, and then read mstatus using csrr you will still read S.
I think unfortunately we need to add a read_mstatus function like we did for something else recently that I can't remember. :-D
The other option is to actually modify mstatus.xPP when misa[S/U] are modified. That is also legal behaviour (so in theory we might want to support it), however it also is a let less elegant and difficult to do in a bug-free way, so I don't think we should - until someone really wants it at least.
| mstatus[SPP] = 0b0; | ||
| cur_privilege = legalize_xret_privilege(if mstatus[SPP] == 0b1 then Supervisor else User); | ||
| // "xPP is set to the least-privileged supported mode (U if U-mode is implemented, else M)." | ||
| mstatus[SPP] = if currentlyEnabled(Ext_U) then 0b0 else 0b1; |
There was a problem hiding this comment.
Probably should use the same style as line 268.
There was a problem hiding this comment.
Also... seems very weird that you could execute sret on an M-only machine but I couldn't see anything disallowing it at a quick glance.
3 participants
Check
misafor enabled privilege modes atxret, since modifications tomisamight have disabled the privileges inmstatus.{MPP, SPP}. Add a test for this case.This leaves handling of misa.U == 0 for later, to be addressed with the other misa bits.
While here, use an assert in place of an internal error as it should have been caught by config validation.
Fixes #1522.