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

[Tsukasa OI <research_trasio@irq.a4lg.com>]

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.


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