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

[Philipp Tomsich]

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.

> 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