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

[Palmer Dabbelt <palmer at dabbelt dot com>]

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.

> On Fri, Nov 1, 2019 at 2:10 AM Nelson Chu <nelson.chu@sifive.com> wrote:
> > Dear binutils,
> >
> > 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 patch only support the RV32 and f-ext checking currently.  If we
> > want to implement more checking for CSR, then we just need to extend
> > the `riscv_csrs` table, and do some corresponding modifications in
> > gas/conifg/tc-riscv.c.  I also update all the CSR to the current
> > privilege spec (1.12), and upadte all the CSR testsuites.

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 spec version checking may be supported in the future patch if
> > needed.  As for the privilege mode and the read/write access checking,
> > It is not necessary to record these information in the `riscv_csrs`
> > table , since we can get these information from the top 4-bits [11:8]
> > of CSR address.

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

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

> > Thanks
> > Best Regards
> > Nelson