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

[Nelson Chu <nelson.chu@sifive.com>]

On Tue, Jun 28, 2022 at 1:33 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>
> 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?

Regarding the H as a common additional requirement is OK to me for
now.  We need at least two common requirements as you mentioned: RV32
and H.  And we probably will have two more common requirements: M and
S, which separate the machine/supervisor/hypervisor from the
extensions.  But we cannot be sure if we may have something like
CSR_CLASS_A_OR_B in the future, or we have to consider both "and" and
"or" situations at the same time.  We already have the
INSN_CLASS_F_OR_ZFINX enum for instructions, so maybe we will have
similar ones in the future for CSRs.  Therefore, in the short-term,
since we don't have any "or" situation for CSRs, the additional common
requirement field in the DECLARE_CSR can be acceptable and work as
expected.  But in the long-term, we probably will need another
solution to consider the more complicated situations.  Or we can wait
and think about it when we actually meet the complicated situations.

Thanks
Nelson

> 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
> >>
> >