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

[Nelson Chu]

Thanks you very much :)

On Sat, Nov 2, 2019 at 6:10 AM Jim Wilson <jimw@sifive.com> wrote:
> 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.

OK I will fix this.

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

Oh I forget to update the new csr for gdb and qemu side, thanks for
the reminder :)

Yes, I prefer to use the riscv_csrs table to store CSR address, this
is more convenient, and we can remove the huge number of DECLARE_CSR
and DECLARE_CSR_ALIAS macros.  For objdump and GDB, we may need to
create an array (2^12) to record the CSR name just like opcode do
(opcode just need 2^6).


On Sat, Nov 2, 2019 at 7:07 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Fri, 01 Nov 2019 10:09:30 PDT (-0700), Andrew Waterman wrote:
> > More rigorous CSR checking is going to cause some backwards
> > incompatibilities.  For test programs especially, we sometimes use
> > CSRs in contexts they aren't meant for (e.g. writing to a read-only
> > CSR).  So I think this might need to be an opt-in feature, rather than
> > a change.
>
> I agree: we're trying really hard not to have flag days if we can avoid them.
>
> Additionally, having a ".option" to go along with the command-line argument
> seems like the right way to go, as I'd want to turn on CSR checking by default
> and then explicitly disable it when I want to do something odd.
>
> Aside from the "don't change existing behavior" argument above, I think the
> idea is good.  The implementation seems half way finished, though.  I'd prefer
> that we just implement all the checking at the same time, to avoid the behavior
> drifting around on people.
>

The .option nocheckrvc/checkrvc is good to me, I agree that this may
be an opt-in feature :)

Maybe we can have the first edition to upstream that just consider the
ISA checking, and disable the checking by default until we have
implemented all the checking.  Or we just push the complete patches to
upstream that including all the checking.  Either is OK for me :))

> We should do the CSR version checks at the same time we do the instruction CSR
> checks.

I haven't considered the details about all possible cases, but maybe
just like the following example:

/* We need to update this.  */
#define DEFAULT_VER 1.12

name                 class                   address  ver_begin   ver_end
...
{"hie",              CSR_CLASS_I,     0x204,    1.9.1,         1.10},
{"vsie",            CSR_CLASS_I,     0x204,     1.12,          DEFAULT_VER},
...

> >> * Check [11:10] bits and then get the read and write access:
> >> 00, 01, 10 -> Read/Write
> >> 11 -> Read only
> >>
> >> * Check [9:8] bits and then get the privilege mode:
> >> 00 -> User-level
> >> 01 -> Supervisor-level
> >> 10 -> Hyper-vision level
> >> 11 -> Machine-level
>
> As far as I can tell this dosn't do anything with privilege modes, but I think
> doing so would be a good idea.  I'm assuming this will result in some sort of
> "-mpriv-mode=m" argument, with the associated .option like above.

Yes this is a good idea, just like the rvc checking in assembler.


Thanks again
Best Regards
Nelson