AR: Clarify mcontext/hcontext. #882
Conversation
timsifive
force-pushed
the
context
branch
2 times, most recently
from
c686356 to
72af5a2
Compare
xml/hwbp_registers.xml
Outdated
| This optional register may only be implemented if the H extension is | ||
| implemented. If it is implemented, \RcsrMcontext must also be implemented. |
There was a problem hiding this comment.
"may only be implemented" seems ambiguous because "may only" is kind of like "may not" which is like "you may not have a piece of candy."
Somehow this slight tweak seems less ambiguous to me:
| This optional register may only be implemented if the H extension is | |
| implemented. If it is implemented, \RcsrMcontext must also be implemented. | |
| This optional register may be implemented only if the H extension is | |
| implemented. If it is implemented, \RcsrMcontext must also be implemented. |
xml/hwbp_registers.xml
Outdated
| If the H extension is not implemented then this register is not | ||
| implemented, though the underlying state may be accessible via | ||
| the optional \RcsrMcontext alias. | ||
| if Smstateen is implemented, then accessibility of this CSR in HS-Mode |
There was a problem hiding this comment.
| if Smstateen is implemented, then accessibility of this CSR in HS-Mode | |
| If Smstateen is implemented, then accessibility of this CSR in HS-Mode |
xml/hwbp_registers.xml
Outdated
| This register is an alias of the \RcsrHcontext register, and provides | ||
| access to the \FcsrHcontextHcontext field even if the \RcsrHcontext | ||
| register is not implemented. |
There was a problem hiding this comment.
This seems confusing to me. It's an alias of a register that's not implemented.
I was going to suggest that we say: If hcontext is implemented then this is an alias of that. If hcontext is not implemented then this is its own register.
Then it occurred to me that there's no definition of mcontext fields in the latter scenario. Maybe we should reverse things and define the fields here. Then mcontext doesn't need to say anything about hcontext except that it must exist if hcontext exists (which it says in the previous paragraph). And hcontext simply says that it's an optional alias of mcontext and which modes it's accessible in.
The only complication is that the field name within this register is named hcontext. But maybe that's OK if we explain that one primary purpose of mcontext is for hypervisors running in M mode.
There was a problem hiding this comment.
I'll flip it like you suggest, and see if we like that better.
It was a bit confusing before. Hopefully now it's more clear how they relate, and why you might want each one.
There was a problem hiding this comment.
I see another reason to have the field defined under mcontext. Previously, it was defined under hcontext which only exists if H is implemented but then the field itself said "If the H extension is implemented..." which was just confusing if you looked at this register in isolation. So this is much better.
It was a bit confusing before. Hopefully now it's more clear how they relate, and why you might want each one.