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

[Jim Wilson <jimw at sifive dot com>]

On Fri, Nov 1, 2019 at 2:09 AM Nelson Chu <nelson.chu@sifive.com> wrote:
> According to the riscv privilege spec, some CSR are only valid when
> rv32 or the specific extension is set.  The current toolchain doesn't
> have enough information to let assembler know which CSR is valid.
> Therefore, we use `riscv_csrs` table, just like `riscv_opcode`, to
> record all the information we need, and then check whether the CSR is
> valid according to these information.

This looks OK to me except for one minor issue, I see
+static struct hash_control *
+init_csr_info_hash ()
+{
which is an old style function definition.  Please make that (void)
instead so that it is a prototype.

Andrew's comment looks like a valid concern.  I'd suggest adding
something like ".option csrcheck" and ".option nocsrcheck" to turn
this csr checking on/off.  Then we would have to check internally to
see how much trouble adding .option nocsrcheck to our test programs
would be, because it would be nice if this was on by default.  Or
maybe someone can suggest a better name than csrcheck.

Don't forget that binutils changes can affect gdb.  Gdb has two xml
files with a list of csrs generated from the DECLARE_CSR macros, so if
we want gdb to see the new CSRs we need to regenerate those files in
gdb.  There is also a copy of these files in qemu, though it may not
make sense to update qemu if it doesn't support the new CSRs yet.
Normally these files should be identical between gdb and qemu.  See
the gdb/features/riscv/rebuild-csr-xml.sh file.  Note that the csr
files in gdb are not quite correct because the 32-bit csrs are listed
in the 64-bit file, but this is something that can be fixed later.

We now have two sets of info for csrs, the DECLARE_CSR macros and the
riscv_csrs table.  Perhaps a future patch can move the register
numbers into the table, and eliminate the DECLARE_CSR macros.  This
might simplify opcodes/riscv-opc.c and gdb/riscv-tdep.c both of which
are using DECLARE_CSR to map a register number to a csr name via a big
switch statement.  This might complicate the
gbd/features/riscv/rebuild-csr-xml.sh script though, but a rewrite
would fix the 64-bit list of registers since we could now use the
class info to exclude the 32-bit only ones.

Jim