Fix CSR privilege check for hypervisor mode. #1504
Conversation
When hypervisor is enabled, `Supervisor` (HS-mode) and `VirtualSupervisor` (VS-mode) privileges are encoded as `0b01` in `privLevel_bits()`, but the CSR addresses for hypervisor and VS CSRs encode the required privilege as `0b10`. Define a private function to correct for this when checking privilege. Fixes riscv#1503.
pmundkur
force-pushed
the
fix_hypervisor_csr_check
branch
from
44b77f5 to
2bc7f82
Compare
| match p { | ||
| User => 0b00, | ||
| VirtualUser => 0b00, | ||
| Supervisor => if currentlyEnabled(Ext_H) then 0b10 else 0b01, |
There was a problem hiding this comment.
I think this would actually work if it was just 0b10 right? If hypervisor is disabled non of the 0b10 CSRs exist anyway. I dunno if that's necessarily better code though.
Also I think this is correct but I couldn't find anywhere that really explicitly says how this is meant to work in the spec. Just this
The next two bits (csr[9:8]) encode the lowest privilege level that can access the CSR.
Which is clear without hypervisor but not very clear at all with hypervisor.
There was a problem hiding this comment.
Agreed that this is not clear. Opened riscv/riscv-isa-manual#2630
There was a problem hiding this comment.
Hopefully clearer now with riscv/riscv-isa-manual#2631
There was a problem hiding this comment.
What do you think about this?
| Supervisor => if currentlyEnabled(Ext_H) then 0b10 else 0b01, | |
| Supervisor => 0b10, |
There was a problem hiding this comment.
Agreed that it may be redundant with an existence check, but I'm leaning towards keeping it as it is, to make sure we don't miss the condition when we refactor this for hypervisor in #1402.
There was a problem hiding this comment.
Maybe just add a comment for now? // Note that this if is not strictly necessary. We could return 0b10 unconditionally since hypervisor CSRs don't exist anyway if hypervisor is not enabled.
5 participants
When hypervisor is enabled,
Supervisor(HS-mode) andVirtualSupervisor(VS-mode) privileges are encoded as0b01inprivLevel_bits(), but the CSR addresses for hypervisor and VS CSRs encode the required privilege as0b10. Define a private function to correct for this when checking privilege.Fixes #1503.