Re: [PATCH v3 0/4] RISC-V: Add CSRs for several supervisor extensions

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

Thanks for your opinion!

On 2022/06/28 10:40, Nelson Chu wrote:
> Thanks, passed toolchain regressions, so all committed.
> 
> On Fri, Jun 24, 2022 at 10:52 AM Tsukasa OI
> <research_trasio@irq.a4lg.com> wrote:
>>
>> v1:
>> <https://sourceware.org/pipermail/binutils/2022-February/119858.html>
>> v2:
>> <https://sourceware.org/pipermail/binutils/2022-May/120974.html>
>> CURRENT (GitHub):
>> <https://github.com/a4lg/binutils-gdb/tree/riscv-s-exts-csrs>
>>
>> This patchset adds CSRs for following extensions:
>>
>> -   Smstateen
>> -   Sscofpmf
>> -   Sstc
>>
>> Because 'H' extension and related CSR class is now implemented, we can
>> now safely and consistently implement hypervisor-related CSR feature
>> gate masking.
>>
>> [CHANGES: v2 -> v3]
>> -   Follow up to now implemented 'H' extension support
>> -   Add (and refactor) complex CSR feature gate handling (on H and RV32)
>> -   Make new CSRs independent to privileged architecture
>>     (as initially suggested by Nelson Chu)
>>
>> Thanks,
>> Tsukasa
>>
>>
>>
>>
>> Tsukasa OI (4):
>>   RISC-V: Add new CSR feature gate handling (RV32,H)
> 
> I think this one is a workaround since we may have more complicate
> macros like INSN_CLASS in the future (or, and, ...).  Maybe we should
> have the function similar to riscv_multi_subset_supports_ext for CSRs.
> Or maybe we could combine the riscv_multi_subset_supports and
> riscv_multi_subset_supports_ext functions into one, just return NULL
> if everything went well, otherwise return the error msg directly, and
> so does the CSR_CLASS.  Anyway, the fix is fine for now, since we will
> get wrong error message without it.  We could have a better fix in the
> future patches.

Yes, definitely a workaround.  Making something like INSN_CLASS_* 
handling is LGTM but I have some other thoughts.

Unlike INSN_CLASS_*, CSR_CLASS_* will have simpler and comon
"additional" requirements like RV32 and H.  Then, how about splitting 
base CSR class and additional requirements?

Current DECLARE_CSR design:
DECLARE_CSR(mstateen0, CSR_MSTATEEN0, CSR_CLASS_SMSTATEEN,...
DECLARE_CSR(hstateen0, CSR_HSTATEEN0, CSR_CLASS_SMSTATEEN_AND_H,...
DECLARE_CSR(mstateen0h, CSR_MSTATEEN0H, CSR_CLASS_SMSTATEEN_32,...
DECLARE_CSR(hstateen0h, CSR_HSTATEEN0H, CSR_CLASS_SMSTATEEN_AND_H_32,...

My idea:
DECLARE_CSR(mstateen0, CSR_MSTATEEN0, CSR_CLASS_SMSTATEEN, 0,...
DECLARE_CSR(hstateen0, CSR_HSTATEEN0, CSR_CLASS_SMSTATEEN, CSR_REQ_H,...
DECLARE_CSR(mstateen0h, CSR_MSTATEEN0H, CSR_CLASS_SMSTATEEN, CSR_REQ_RV32,...
DECLARE_CSR(hstateen0h, CSR_HSTATEEN0H, CSR_CLASS_SMSTATEEN, CSR_REQ_H | CSR_REQ_RV32,...

Of course, this can be used together with your idea.
I would like to hear everyone's thoughts.

Thanks,
Tsukasa

> 
> Thanks
> Nelson
> 
>>   RISC-V: Add 'Smstateen' extension and its CSRs
>>   RISC-V: Add 'Sscofpmf' extension with its CSRs
>>   RISC-V: Add 'Sstc' extension and its CSRs
>>
>>  bfd/elfxx-riscv.c                           |   3 +
>>  gas/config/tc-riscv.c                       |  47 ++++-
>>  gas/testsuite/gas/riscv/csr-dw-regnums.d    |  54 +++++
>>  gas/testsuite/gas/riscv/csr-dw-regnums.s    |  57 ++++++
>>  gas/testsuite/gas/riscv/csr-version-1p10.d  | 108 ++++++++++
>>  gas/testsuite/gas/riscv/csr-version-1p10.l  | 207 ++++++++++++++++++++
>>  gas/testsuite/gas/riscv/csr-version-1p11.d  | 108 ++++++++++
>>  gas/testsuite/gas/riscv/csr-version-1p11.l  | 207 ++++++++++++++++++++
>>  gas/testsuite/gas/riscv/csr-version-1p12.d  | 108 ++++++++++
>>  gas/testsuite/gas/riscv/csr-version-1p12.l  | 207 ++++++++++++++++++++
>>  gas/testsuite/gas/riscv/csr-version-1p9p1.d | 108 ++++++++++
>>  gas/testsuite/gas/riscv/csr-version-1p9p1.l | 207 ++++++++++++++++++++
>>  gas/testsuite/gas/riscv/csr.s               |  60 ++++++
>>  include/opcode/riscv-opc.h                  | 114 +++++++++++
>>  14 files changed, 1591 insertions(+), 4 deletions(-)
>>
>>
>> base-commit: 54603ee2aeaf248220f0f440c322ff02e98cd403
>> --
>> 2.34.1
>>
>