Re: [PATCH] RISC-V: Support more rigorous check for CSR, and update them to spec 1.12.

["Palmer Dabbelt via binutils" <binutils at sourceware dot org>]

On Fri, 22 Nov 2019 00:09:27 PST (-0800), nelson.chu@sifive.com wrote:
> Dear binutils,
>
> I do some modifications for the previous CSR checking patches.
>
> There are four updated patches as follows:
> 0001-RISC-V-Update-the-CSR-to-privilege-spec-1.12
> 0002-RISC-V-Support-the-ISA-dependent-CSR-checking
> 0003-RISC-V-Support-read-only-CSR-checking
> 0004-RISC-V-Disable-the-CSR-checking-by-default
>
> The first patch is the same as the previous version.  In the second
> patch, the `csr_extra_hash` only record the extra information of CSR.
> For now we just need to record the `csr_class` in the `csr_extra_hash`
> to support the ISA-dependent CSR checking.  The CRS addresses are
> stored in the `reg_names_hash` as before.  I also update some comments
> and fix typo for the third and fourth patches.
>
> Any comment or suggestion is appreciated :)

Can you use git-send-email to send your patches?  They're a lot harder to read
this way.

> Thanks and regards
> Nelson
>
>
> On Wed, Nov 6, 2019 at 5:57 PM Nelson Chu <nelson.chu@sifive.com> wrote:
> > After the discussion, we now promote a new implementation for the CSR
> > checking.  Keep the original DECLARE_CSR and DECLARE_CSR_ALIAS in the
> > include/opcode/riscv-opc.h, and then extend them to record the extra
> > information for CSR.
> >
> > DECLARE_CSR(ustatus, CSR_USTATUS, CSR_CLASS_I)
> > DECLARE_CSR(uie, CSR_UIE, CSR_CLASS_I)
> > DECLARE_CSR(utvec, CSR_UTVEC, CSR_CLASS_I)
> > DECLARE_CSR(uscratch, CSR_USCRATCH, CSR_CLASS_I)
> > ...
> >
> > Then we can create and insert hash table with the key (CSR name) to
> > store the CSR address and ISA class for the CSR.  As for GDB, I have
> > regenerated the 32bit-csr.xml and 64bit-csr.xml files through the
> > modified rebuild-csr-xml.sh.  The modified rebuild-csr-xml.sh can
> > filter the rv32-only CSR for the 64bit-csr.xml.
> >
> > If we keep and extend the original DECLARE_SCR macro, then we still
> > need a big switch statement in the opcodes/riscv-dis.c and
> > gdb/riscv-tdep.c.  If we use the previous method that record the CSR
> > information via table in opcodes/riscv-opc.c, then we probably need to
> > parse the table in the rebuild-csr-xml.sh or other methods.  I'm not
> > sure which one is better, either is OK for me, so I just implement the
> > minimal changed method in this mail.
> >
> > The CSR version checking and privilege mode checking are more
> > complicated.  Currently, I have no idea that GCC should choose what
> > format and method to pass the CSR version and privilege mode to
> > assembler, maybe use the .option, --mxxx options, or the ELF
> > attribute, or all three are.  Therefore, I prefer to support these two
> > CSR checking in the later patches after the full discussion :)
> >
> > There are four new patches to support the CSR checking as follows:
> > 0001-RISC-V-Update-the-CSR-to-privilege-spec-1.12.patch
> > 0002-RISC-V-Support-more-rigorous-check-for-CSR.patch
> > 0003-RISC-V-Support-read-only-CSR-checking.patch
> > 0004-RISC-V-Disable-the-CSR-checking-by-default.patch
> >
> > The first patch is used to update the CSR to the privilege spec 1.12,
> > the two xml files are also regenerated.  The second patch is used to
> > support the ISA-dependent CSR checking.  The third patch is used to
> > support the Read-only CSR checking.  The CSR checking should be an
> > opt-in feature for now.  Therefore, we support the new options
> > "-mcsrcheck/-mno-csrcheck" and ".option csrcheck/nocsrcheck" to
> > enable/disable the CSR checking, and disable the CSR checking by
> > default in the fourth patch.
> >
> > If the first patch is accepted, then I will update the CSR to the
> > privilege spec 1.12 for Qemu and OpenOCD later.
> >
> > Thanks
> > Best Regards
> > Nelson