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

[Alistair Francis]

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.

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