Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension

[Alistair Francis]

On Fri, Jan 21, 2022 at 1:38 AM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> Thanks for taking the time to write this up!
>
> On Wed, 19 Jan 2022 at 02:30, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Jan 19, 2022 at 11:19 AM Alistair Francis <alistair23@gmail.com> 
> > wrote:
> > >
> > > On Wed, Jan 19, 2022 at 9:22 AM Philipp Tomsich
> > > <philipp.tomsich@vrull.eu> wrote:
> > > >
> > > > Alistair,
> > > >
> > > > Some of us (the merit almost exclusively goes to Kito) have been
> > > > working towards a similar policy for GCC/binutils and LLVM.
> > > > This currently lives in:
> > > >    https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
> > >
> > > Ah cool! We can use that as a good starting point.
> > >
> > > >
> > > > A few comments & a question below.
> > > >
> > > > Thanks,
> > > > Philipp.
> > > >
> > > > On Tue, 18 Jan 2022 at 23:53, Alistair Francis <alistair23@gmail.com> 
> > > > wrote:
> > > > >
> > > > > On Fri, Jan 14, 2022 at 6:22 AM Philipp Tomsich
> > > > > <philipp.tomsich@vrull.eu> wrote:
> > > > > >
> > > > > > This adds the decoder and translation for the XVentanaCondOps custom
> > > > > > extension (vendor-defined by Ventana Micro Systems), which is
> > > > > > documented at 
> > > > > > https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf
> > > > > >
> > > > > > This commit then also adds a guard-function (has_XVentanaCondOps_p)
> > > > > > and the decoder function to the table of decoders, enabling the
> > > > > > support for the XVentanaCondOps extension.
> > > > > >
> > > > > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > > > >
> > > > > This looks reasonable to me.
> > > > >
> > > > > I'm going to leave this for a bit in case there are any more comments.
> > > > >
> > > > > I was a little worried that taking vendor extensions isn't the right
> > > > > move, as we might get stuck with a large number of them. But this is
> > > > > pretty self contained and I think with the growing RISC-V interest
> > > > > it's something we will eventually need to support.
> > > > >
> > > > > I'm going to update the QEMU RISC-V wiki page with this to make the
> > > > > position clear (comments very welcome)
> > > > >
> > > > > === RISC-V Extensions ===
> > > > > As RISC-V has a range of possible extensions, QEMU has guidelines for
> > > > > supporting them all.
> > > > >
> > > > > If an extension is frozen or ratified by the RISC-V foundation, it can
> > > > > be supported in QEMU.
> > > > >
> > > > > If an official RISC-V foundation extension is in a reasonable draft
> > > > > state, that is not too many changes are still expected, it can be
> > > > > supported experimentally by QEMU. Experimental support means it must
> > > > > be disabled by default and marked with a "x-" in the properties. QEMU
> > > > > will only support the latest version of patches submitted for a draft
> > > > > extension. A draft extension can also be removed at any time if it
> > > > > conflicts with other extensions.
> > > > >
> > > > > QEMU will also support vendor extensions. Vendor extensions must be
> > > > > disabled by default, but can be enabled for specific vendor CPUs and
> > > > > boards. Vendor extensions must be maintained and tested by the vendor.
> > > >
> > > > I guess I should create a v3 with appropriate paths in the MAINTAINERS 
> > > > file?
> > >
> > > Hmm... Good point. I don't think you have to if you don't want to.
> > >
> > > My point here was more to just make it clear that upstream QEMU is not
> > > a dumping ground for vendor extensions to get them maintained by
> > > someone else. Obviously we won't purposely break things just for fun.
> > > There is an expectation that the vendor tests their extensions and
> > > responds to bug reports and things like that.
> > >
> > > >
> > > > > Vendor extensions can not interfere with other extensions and can not
> > > > > be obtrusive to the RISC-V target code.
> > > >
> > > > I know that there is some interest to have the XtheadV (the
> > > > instructions previously known as vectors 0.7.1-draft) supported and we
> > > > have the reality of a deployed base that implements it in hardware.
> > > > This would conflict with the opcode space used by the standard RISC-V
> > > > vectors, so it makes for an interesting test case (even if just to
> > > > clarify our intent)...
> > > > Personally, I would like to avoid precluding inclusion of something
> > > > useful (of course, "Vendor extensions must be maintained and tested by
> > > > the vendor." has to apply!), if a vendor was going to step up and also
> > > > offers to maintain it.
> > >
> > > Yeah... this is unfortunate. I agree that having the 0.7.1-draft
> > > extensions supported would be great. There is hardware that supports
> > > it.
> > >
> > > I think this point still stands though. IF the XtheadV implementation
> > > is self contained and doesn't interfere with the vector extensions,
> > > then that's great and we can support it. If instead it adds a large
> > > amount of conditionals to the released vector extension code then I
> > > don't think we can take it.
> > >
> > > There is some wiggle room, but the RISC-V tree already has enough
> > > going on and very little reviewers. If in the future we get more
> > > reviewers and testers we can re-evaulate what is acceptable, but for
> > > now I think we need to be a little strict. (Hint to any companies to
> > > give developers time to review)
> > >
> > > >
> > > > So let's assume such a (very significant) addition were factored out
> > > > similarly, interfacing just through a hook in decode_op and
> > > > argument-parsing logic that ensures that the conflicting
> > > > standard-extension is turned off: would this still be acceptable under
> > > > this policy — or would it trip the "obtrusive" condition?
> > >
> > > I think that would be acceptable, I wouldn't say that is obtrusive as
> > > it's self contained.
> >
> > Ok, take two:
> >
> > === RISC-V Foundation Extensions ===
> > As RISC-V has a range of possible extensions, QEMU has guidelines for
> > supporting them all.
> >
> > If an extension is frozen or ratified by the RISC-V foundation, it can
> > be supported in QEMU. Generally we will try to support as many versions
> > as feasible, following the usual QEMU deprecation policy to remove old
> > versions.
> >
> > If an official RISC-V foundation extension is in a reasonable draft
> > state, that is not too many changes are still expected, it can be
> > supported experimentally by QEMU. Experimental support means it must
> > be disabled by default and marked with a "x-" in the CPU/board properties.
> > Draft extensions can be enabled by specific CPUs or boards if the hardware
> > supports that extension.
>
> Should we include a version number on experimental versions?
>
> LLVM requires users to fully specify the version, when using
> experimental versions.
> This may be a useful stereotype also for QEmu, as it ensures that
> users are aware that the underlying specification version has changed
> (e.g., when someone requests x-zbb-0p92 and our implementation moves
> to x-zbb-0p93 (there was a difference in encoding of the
> minimum/maximum operations in-between)), an error will be raised early
> instead of having a computation go wrong later.

We traditionally haven't and it's up to anyone using an experimental
extension to check the version.

It's probably worth including though, I have added a sentence there.

Thanks for the feedback, I have added that to the wiki

Alistair

>
> > QEMU will only support the latest version of patches submitted for a draft
> > extension. A draft extension can also be removed at any time and does not
> > follow QEMU's deprecation policy.
> >
> > === RISC-V Custom Extensions/Instructions ===
> > Support for custom instruction set extensions are an important part of 
> > RISC-V,
> > with large encoding spaces reserved of vendor extensions.
> >
> > QEMU follows similar rules to the RISC-V toolchain convention, as described
> > here: https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/17
> >
> > QEMU will support vendor extensions. Vendor extensions must be
> > disabled by default, but can be enabled for specific vendor CPUs and
> > boards. The vendor extensions should use prefixes and names as described in
> > https://github.com/riscv-non-isa/riscv-toolchain-conventions
> >
> > Vendor extensions must be maintained and tested by the vendor. Upstream will
> > take efforts to not break extensions, but testing and bug fixes should be
> > done by the vendor. Patches to add support for open source toolchains are
> > unlikely to be accepted without specification documents being made available
> > publicly.
> >
> > Vendor extensions can not interfere with other extensions and can not
> > be obtrusive to the core RISC-V target code.
> >
> > If you are looking to add support for vendor extensions, it is recommended
> > that you get involved in the QEMU review process. It is also recommended 
> > that
> > you send your patches as early as possible to get community feedback before
> > they are fully implemented. This is especially important if you are 
> > modifying
> > core code.
> >
> > Alistair