generated from riscv/docs-spec-template
-
Notifications
You must be signed in to change notification settings - Fork 4
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
move xctrctl.DEPTH to new sctrdepth CSR
- Loading branch information
Showing
1 changed file
with
60 additions
and
43 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
77bbe80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is problematic for M-mode assisted virtualization because if M-mode is pretending that physical S-mode is VS-mode, there's no way to force
sctrdepth
to trap except by the very heavy-handed means of settingmstateen0
.CTR=0.77bbe80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. Do you think it's required to support depth constraints in such scenarios? Do we expect to see M-mode assisted virtualization in datacenters?
77bbe80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It's used to support virtualization when the hardware wasn't originally designed for it.
Logical HS mode doesn't know about M-mode assistance, so it has no way of knowing that logical VS mode can access sctrdepth freely. If someone decided to use reads of sctrdepth as a hypercall interface because our specification as written guarantees it to trap in VS-mode, they would get a nasty surprise.
Writable sctrdepth is mostly useful for migration, which I doubt will overlap much with M-mode assisted virtualization.
I wasn't following the discussion closely the last few months - was there ever a proposal of making a holistic migration feature restriction extension, that would apply to sctrdepth and other features of CTR and other extensions?
77bbe80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe there is talk of any such migration-enabling extension, would probably be the domain of the Hypervisor SIG.
M-mode can virtualize CTR for S-mode as defined, though if it wants to limit depth options it will have to take the heavy-handed approach of clearing mstateen0.CTR. Sounds like that's unlikely to be necessary. This seems consistent with the general approach of ensuring that M-mode can play the role of hypervisor, if not as efficiently as HS-mode can (e.g., no G-stage translation). It doesn't seem worth the complexity to add ISA to extend a similar depth-limiting functionality to M-mode.
77bbe80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not asking for a M-mode depth limiting functionality. Depth limiting is an optional feature and it's fine to only allow that option when the H extension exists.
If the H extension is implemented in hardware, any VS-mode access to sctrdepth raises a virtual instruction exception, which will be delegated to HS-mode. HS-mode is expected to emulate the access by providing read-only access to the CTR depth, but there's no way to enforce this.
If the H extension is not implemented in hardware, "VS"-mode access to sctrdepth provides read-only access to the CTR depth, unless M-mode interposes access to the CTR entirely. No "HS"-mode virtual instruction exception can be generated because no hardware exception was generated.
I'd like these two to match, one way or the other.
77bbe80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. I assume that, when the H extension is not implemented in HW, your "VS"-mode is actually S-mode, and "HS"-mode is M-mode?
Assuming we keep the existing trapping behavior on VS-mode accesses to sctrdepth, I think you're asking for something like: if the H extension is not implemented in HW, S-mode accesses to sctrdepth should raise an illegal instruction exception. That would provide the symmetry I think you want, but would violate CSR address mapping conventions. Less symmetrical, but perhaps close enough, would be to make sctrdepth read-only from S-mode when the H extension is not implemented. Then writes would raise an illegal instruction exception, allowing M-mode to emulate the write. Is that the idea?
I'm hesitant to add new behaviors just to make M-mode assisted virtualization faster. As you point out, M-mode can virtualize CTR properly (if slowly) by clearing mstateen0.CTR. Adding traps like I describe above would reduce virtualization overhead for M-mode assisted virtualization, but would make the common case slower by adding unneeded traps.
77bbe80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VS-mode is actually S-mode. HS-mode is also actually S-mode. The V bit is stored in M-mode accessible memory (perhaps HPV in the hstatus image) so M-mode knows where traps came from and can set TVM, TSR, TW appropriately.
I was asking for a second M-mode writable bit which would behave like mstateen0.CTR but only for sctrdepth.
I think at the time I was thinking that it would make all supervisor CTR access slow on systems that require M-mode assisted virtualization. On current look, it only affects the guest so it's probably fine.
77bbe80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like I can close this. Feel free to re-open it if I misunderstood.
77bbe80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot this was a comment, not an issue. But I'll consider it resolved.