[PATCH 1/3] RISC-V: Add 'Smstateen' extension and its CSRs

[Nelson Chu]

On Fri, Feb 25, 2022 at 6:51 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>
> On 2022/02/25 15:32, Nelson Chu wrote:
> >
> > The privileged spec doesn't define these CSRs, they are defined and
> > controlled by smstaeen extension.  So I think the defined and aborted
> > versions of these DECLARE_CSR should be PRIV_SPEC_CLASS_NONE, just
> > like other unprivileged CSRs did.  In the future, they should be
> > controlled by smstaeen extension versions, but for now we don't need
> > to care about this.
> >
> > Thanks
> > Nelson
> >
> >>  /* Dropped CSRs.  */
> >>  DECLARE_CSR(mbase, CSR_MBASE, CSR_CLASS_I, PRIV_SPEC_CLASS_1P9P1, PRIV_SPEC_CLASS_1P10)
> >>  DECLARE_CSR(mbound, CSR_MBOUND, CSR_CLASS_I, PRIV_SPEC_CLASS_1P9P1, PRIV_SPEC_CLASS_1P10)
> >> --
> >> 2.32.0
> >>
> >
>
> 'Sscofpmf' patch (PATCH 2/3) is modified as per your advise.
>
> For the rest, I noticed that there are hypervisor-related CSRs.
>
> For consistency with CSR_CLASS_I CSRs, I think hypervisor-related CSRs
> should be masked with privileged architecture version 1.12.  However, if
> need_check_version == true (gas/config/tc-riscv.c) for specific CSR
> class, there's no option to set PRIV_SPEC_CLASS_NONE because it would
> just mean "not supported in any version".
>
> Separating CSR classes (H/non-H) might be an option.  However, I felt
> this is "too much".
>
>
> Option A: Set minimum privileged specification to 1.9.1 for non-H CSRs
> Option B: Disable version checking on certain conditions
>
> [Option A]
>
> How about this?
>
> -   For 'Sscofpmf' (with no hypervisor-related CSRs),
>     use PRIV_SPEC_CLASS_NONE, PRIV_SPEC_CLASS_NONE
>     with need_check_version = false (as you recommended).
> -   For 'Smstateen' and 'Sstc' (with some hypervisor-related CSRs),
>     need_check_version = true and...
>     -   Use PRIV_SPEC_CLASS_1P12, PRIV_SPEC_CLASS_DRAFT
>         for hypervisor-related CSRs
>         (mask with both given extension and Privileged version 1.12)
>     -   Use PRIV_SPEC_CLASS_1P9P1, PRIV_SPEC_CLASS_DRAFT
>         for other CSRs
>         (much like unprivileged counter CSRs like `cycle')
>
> That means my 'Smstateen' patch (PATCH 1/3) will not be modified.
> On the other hand, 'Sstc' patch (PATCH 3/3) will be (relax minimum
> privileged version from 1.11 to 1.9.1 [minimum supported]).
>
> [Option B]
>
> Another option is to disable version checking if both define_version and
> abort_version are PRIV_SPEC_CLASS_NONE (or more simply, if
> define_version is PRIV_SPEC_CLASS_NONE).  With current design, that
> would remove `need_check_version' variable too.

I have heard that we will have extension names of supervisor, machine
and hypervisor mode in the future.  So the CSRs will be controlled by
the corresponding extension names with different versions.  The
PRIV_SPEC_CLASS_<versions> will be replaced with the version of
extensions, and the privileged spec versions will no longer be useful.
Therefore, I had suggested we just set both define_version and
abort_version to PRIV_SPEC_CLASS_NONE.  The hypervisor CSRs will be
controlled by the "h" extension if the ISA spec is updated, so it
isn't worth spending so much time to discuss these similar issues,
unless there are other new proposals in the future.

Thanks
Nelson

> TBH, I see both options look equally good but have their cons.
>
> I would like to hear everyone else's (including your) thoughts.
>
>
> Thanks,
> Tsukasa