Re: [PATCH v3 1/4] RISC-V: Update the CSR to privilege spec 1.12.

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

On Fri, 31 Jan 2020 16:24:29 PST (-0800), Andrew Waterman wrote:
> On Fri, Jan 31, 2020 at 4:19 PM Jim Wilson <jimw@sifive.com> wrote:
> > On Sun, Dec 15, 2019 at 9:22 PM Nelson Chu <nelson.chu@sifive.com> wrote:
> > >         gas/
> > >         * testsuite/gas/riscv/priv-reg.s: Rename to priv-reg-all.s  Update
> > >         the CSR to privilege spec 1.12.
> > >         * testsuite/gas/riscv/priv-reg.d: Likewise.
> > >         * testsuite/gas/riscv/bad-csr.s: Rename to priv-reg-fail-nonexistent.
> > >         * testsuite/gas/riscv/bad-csr.d: Likewise.
> > >         * testsuite/gas/riscv/bad-csr.l: Likewise.
> > >         * testsuite/gas/riscv/satp.s: Deleted.  Duplicate of priv-reg-all.s
> > >         * testsuite/gas/riscv/satp.d: Likewise.
> > >         * testsuite/gas/riscv/csr-dw-regnums.s: Updated.
> > >         * testsuite/gas/riscv/csr-dw-regnums.d: Likewise.
> > >
> > >         include/
> > >         * opcode/riscv-opc.h: Update the CSR to privilege spec 1.12.
> > >
> > >         gdb/
> > >         * features/riscv/32bit-csr.xml: Regenerated.
> > >         * features/riscv/64bit-csr.xml: Regenerated.
> >
> > Privilege spec 1.11 is ratified, and spec 1.12 is in draft form.  Do
> > we want to implement a draft?
>
> It seems inadvisable to incorporate draft standards into the FSF
> binutils port.  In particular, the hypervisor ISA is virtually certain
> to change in backwards-incompatible ways prior to being frozen.  It
> seems to me that pushing this stuff upstream at this time will cause
> more problems than it solves.
>
> We could consider maintaining these patches on branches in the RISC-V
> github repositories, and push them to the FSF tree at the appropriate
> time.  That way, we can support software development against these
> draft standards while reducing the scope of backwards
> incompatibilities.

That's the approach we're taking everywhere else, and I think it's the right
way to go here.  Sorry, I must have been off by one when I was looking.

> > I see some hypervisor registers missing, e.g. hgeie and hgeip which
> > were added to the 1.12 draft on November 20.  Hie was added Oct 29,
> > and is listed as dropped in your patch.  It isn't clear to me which
> > version of the 1.12 draft you are using, but it seems to be an old
> > one.  If we are implementing
> > 1.12 draft we should probably implement the current one.
> >
> > Overall it looks reasonable, but we need to be clear which spec we are
> > implementing, so we can verify that the patch matches the spec.
> >
> > I see that the csr-dw-regnums.s test doesn't have any of the csr
> > aliases, but that seems OK.  priv-reg-all.s is the important one and
> > does have all of them.
> >
> > You are modifying gdb xml files.  These need to be sent to gdb-patches
> > for approval instead of the binutils list, and I don't have gdb patch
> > approval permission.  These are shared with qemu, and ideally should
> > be kept in sync.  As a practical matter though, gdb doesn't use the
> > csr xml files as yet.  And updating qemu means adding priv spec 1.12
> > draft support to qemu which is probably non-trivial, unless we can
> > ignore the as yet unsupported registers in the gdb stub which should
> > work.  I believe WDC is working on the qemu hypervisor support, so it
> > might actually be tracking the 1.12 draft already.  It is probably OK
> > to submit the gdb xml changes, and then file a bug somewhere asking
> > for gdb and qemu xml support to be synchronized.
> >
> > Jim