Closed ubc-guy closed 2 months ago
I think this is one of those cases where the extension was implicitly defined as part of defining a profile, specifically RVA22 https://github.com/riscv/riscv-profiles/blob/1de106abfd3e04be418cb0fae5cb6a8d4665495c/profiles.adoc#L953-L964
When all these docs are integrated, this can be improved by hyperlinking the extensions listed in the profiles to their definitions elsewhere in the doc. In this case, it would be a link back to the RVA22 profile. Or the Smstateen chapter could be rewritten to define Ssstateen (which doesn't break RVIA protocol, since Ssstateen was ratified as part of RVA22). Or ...
@wmat Adding on to Andrew's comments ...
A secondary problem introduced by the integration of the stateen extension (into the Priv spec) was adding on the extension name "Smstateen" in the title - suggesting that this only specifies that extension. Whereas it should be "Smstateen/Ssstateen" in the title.
The primary problem, as already noted, is that this extension does not explicitly spell out what constitutes the Smstateen extension and what constitutes the Ssstateen extension. For now (until the much bigger, broader rewrite of this chapter in phase 2), I propose that a cut-down version of Andrew's excerpt above be added as a paragraph right after the section header "Proposal". And the header should be changed from "Proposal" to "Smstateen/Ssstateen Extension". The net is:
Smstateen/Ssstateen Extension
These extensions specify both machine-mode and supervisor-mode features. The Smstateen extension specification comprises the mstateen CSRs and their functionality. The Ssstateen extension specification comprises the sstateen and hstateen* CSRs and their functionality.
Andrew, how does this sound and would you wordsmith it a bit further?
I think it's actually the case that Smstateen is a superset, so how about
These extensions collectively specify machine-mode and supervisor-mode features. The Smstateen extension specification comprises the mstateen, sstateen, and hstateen CSRs and their functionality. The Ssstateen extension specification comprises only the sstateen and hstateen* CSRs and their functionality.
SGTM. @wmat, go ahead and create a PR with this change and the title change I first mentioned.
Done. Please see PR. Is versioning the chapter as 1.0.0 still valid? And does this PR resolve this issue? I'll leave it to @gfavor or @aswaterman to close.
I would say yes to this being v1.0.0 even though a phase 2 rewrite is expected (which maybe bumps this to 1.1.0). @aswaterman?
Neither extension ever changed, so their version numbers shouldn’t increment.
I'm aware of an official extension named Smstateen.
The rva23-profile.adoc links to Smstateen extension spec in github.
The RVA profile also refers to an Sstateen extension, with no link. However I can't find the difference between Smstateen and Ssstateen documented anywhere. The Smstateen extension uses only the term "Smstateen" once on the cover page and not anywhere within the body of the specification; in particular it does not use the Sstateen term anywhere. This is likely a shortcoming of the Smastaten specification, but it is trickling into a lack of precision about what RVA23 means (and also affected RVA22).
https://github.com/riscv/riscv-state-enable https://github.com/riscv/riscv-state-enable/releases/download/v1.0.0/Smstateen.pdf
More precisely, RVA23 requires Ssstateen (because RVA23 requires the hypervisor extension), but does not define it. It refers to a document Smstateen that also does not define the term or require the Smstateen extension directly.
Similarly, RVB23 only requires Ssstateen if hypervisor option is included, but suffers from similar imprecision. It does state ("Ssstateen Supervisor-mode view of the state-enable extension. The supervisor-mode (sstateen0-3) and hypervisor-mode (hstateen0-3) state-enable registers must be provided.", but notice this says nothing about the M-mode view (Smstateen) being required, or the difference between these Ss and Sm extensions.
In contrast, the more recently ratified Indirect CSR Access extension seems to do things correctly. It uses the terms Smcsrind and Sscsrind, and defines what they mean in the introduction of the spec (https://github.com/riscvarchive/riscv-indirect-csr-access/blob/main/intro.adoc).