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

[Alistair Francis]

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.

Alistair

>
> >
> > Alistair
> >
> > >
> > > ---
> > >
> > > Changes in v2:
> > > - Split off decode table into XVentanaCondOps.decode
> > > - Wire up XVentanaCondOps in the decoder-table
> > >
> > >  target/riscv/XVentanaCondOps.decode           | 25 ++++++++++++
> > >  target/riscv/cpu.c                            |  3 ++
> > >  target/riscv/cpu.h                            |  3 ++
> > >  .../insn_trans/trans_xventanacondops.inc      | 39 +++++++++++++++++++
> > >  target/riscv/meson.build                      |  1 +
> > >  target/riscv/translate.c                      | 13 +++++++
> > >  6 files changed, 84 insertions(+)
> > >  create mode 100644 target/riscv/XVentanaCondOps.decode
> > >  create mode 100644 target/riscv/insn_trans/trans_xventanacondops.inc
> > >
> > > diff --git a/target/riscv/XVentanaCondOps.decode 
> > > b/target/riscv/XVentanaCondOps.decode
> > > new file mode 100644
> > > index 0000000000..5aef7c3d72
> > > --- /dev/null
> > > +++ b/target/riscv/XVentanaCondOps.decode
> > > @@ -0,0 +1,25 @@
> > > +#
> > > +# RISC-V translation routines for the XVentanaCondOps extension
> > > +#
> > > +# Copyright (c) 2022 Dr. Philipp Tomsich, philipp.tomsich@vrull.eu
> > > +#
> > > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > > +#
> > > +# Reference: VTx-family custom instructions
> > > +#            Custom ISA extensions for Ventana Micro Systems RISC-V cores
> > > +#            
> > > (https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf)
> > > +
> > > +# Fields
> > > +%rs2  20:5
> > > +%rs1  15:5
> > > +%rd    7:5
> > > +
> > > +# Argument sets
> > > +&r    rd rs1 rs2  !extern
> > > +
> > > +# Formats
> > > +@r         .......  ..... ..... ... ..... ....... &r                %rs2 
> > > %rs1 %rd
> > > +
> > > +# *** RV64 Custom-3 Extension ***
> > > +vt_maskc   0000000  ..... ..... 110 ..... 1111011 @r
> > > +vt_maskcn  0000000  ..... ..... 111 ..... 1111011 @r
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 9bc25d3055..fc8ab1dc2b 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -673,6 +673,9 @@ static Property riscv_cpu_properties[] = {
> > >      DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
> > >      DEFINE_PROP_BOOL("zbs", RISCVCPU, cfg.ext_zbs, true),
> > >
> > > +    /* Vendor-specific custom extensions */
> > > +    DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, 
> > > cfg.ext_XVentanaCondOps, false),
> > > +
> > >      /* These are experimental so mark with 'x-' */
> > >      DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
> > >      /* ePMP 0.9.3 */
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 4d63086765..ffde94fd1a 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -330,6 +330,9 @@ struct RISCVCPU {
> > >          bool ext_zfh;
> > >          bool ext_zfhmin;
> > >
> > > +        /* Vendor-specific custom extensions */
> > > +        bool ext_XVentanaCondOps;
> > > +
> > >          char *priv_spec;
> > >          char *user_spec;
> > >          char *bext_spec;
> > > diff --git a/target/riscv/insn_trans/trans_xventanacondops.inc 
> > > b/target/riscv/insn_trans/trans_xventanacondops.inc
> > > new file mode 100644
> > > index 0000000000..b8a5d031b5
> > > --- /dev/null
> > > +++ b/target/riscv/insn_trans/trans_xventanacondops.inc
> > > @@ -0,0 +1,39 @@
> > > +/*
> > > + * RISC-V translation routines for the XVentanaCondOps extension.
> > > + *
> > > + * Copyright (c) 2021-2022 VRULL GmbH.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify 
> > > it
> > > + * under the terms and conditions of the GNU General Public License,
> > > + * version 2 or later, as published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope it will be useful, but WITHOUT
> > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
> > > for
> > > + * more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License 
> > > along with
> > > + * this program.  If not, see <http://www.gnu.org/licenses/>.
> > > + */
> > > +
> > > +static bool gen_condmask(DisasContext *ctx, arg_r *a, TCGCond cond)
> > > +{
> > > +    TCGv dest = dest_gpr(ctx, a->rd);
> > > +    TCGv src1 = get_gpr(ctx, a->rs1, EXT_NONE);
> > > +    TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
> > > +
> > > +    tcg_gen_movcond_tl(cond, dest, src2, ctx->zero, src1, ctx->zero);
> > > +
> > > +    gen_set_gpr(ctx, a->rd, dest);
> > > +    return true;
> > > +}
> > > +
> > > +static bool trans_vt_maskc(DisasContext *ctx, arg_r *a)
> > > +{
> > > +    return gen_condmask(ctx, a, TCG_COND_NE);
> > > +}
> > > +
> > > +static bool trans_vt_maskcn(DisasContext *ctx, arg_r *a)
> > > +{
> > > +    return gen_condmask(ctx, a, TCG_COND_EQ);
> > > +}
> > > diff --git a/target/riscv/meson.build b/target/riscv/meson.build
> > > index a32158da93..1f3a15398b 100644
> > > --- a/target/riscv/meson.build
> > > +++ b/target/riscv/meson.build
> > > @@ -4,6 +4,7 @@ dir = meson.current_source_dir()
> > >  gen = [
> > >    decodetree.process('insn16.decode', extra_args: 
> > > ['--static-decode=decode_insn16', '--insnwidth=16']),
> > >    decodetree.process('insn32.decode', extra_args: 
> > > '--static-decode=decode_insn32'),
> > > +  decodetree.process('XVentanaCondOps.decode', extra_args: 
> > > '--static-decode=decode_XVentanaCodeOps'),
> > >  ]
> > >
> > >  riscv_ss = ss.source_set()
> > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > > index 2cbf9cbb6f..efdf8a7bdb 100644
> > > --- a/target/riscv/translate.c
> > > +++ b/target/riscv/translate.c
> > > @@ -122,6 +122,15 @@ static inline bool always_true_p(CPURISCVState *env  
> > > __attribute__((__unused__))
> > >      return true;
> > >  }
> > >
> > > +#define MATERIALISE_EXT_PREDICATE(ext)  \
> > > +    static inline bool has_ ## ext ## _p(CPURISCVState *env, \
> > > +                                         DisasContext *ctx  
> > > __attribute__((__unused__)))  \
> > > +    { \
> > > +        return RISCV_CPU(ctx->cs)->cfg.ext_ ## ext ; \
> > > +    }
> > > +
> > > +MATERIALISE_EXT_PREDICATE(XVentanaCondOps);
> > > +
> > >  #ifdef TARGET_RISCV32
> > >  #define get_xl(ctx)    MXL_RV32
> > >  #elif defined(CONFIG_USER_ONLY)
> > > @@ -844,9 +853,12 @@ static uint32_t opcode_at(DisasContextBase *dcbase, 
> > > target_ulong pc)
> > >  #include "insn_trans/trans_rvb.c.inc"
> > >  #include "insn_trans/trans_rvzfh.c.inc"
> > >  #include "insn_trans/trans_privileged.c.inc"
> > > +#include "insn_trans/trans_xventanacondops.inc"
> > >
> > >  /* Include the auto-generated decoder for 16 bit insn */
> > >  #include "decode-insn16.c.inc"
> > > +/* Include decoders for factored-out extensions */
> > > +#include "decode-XVentanaCondOps.c.inc"
> > >
> > >  static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t 
> > > opcode)
> > >  {
> > > @@ -862,6 +874,7 @@ static void decode_opc(CPURISCVState *env, 
> > > DisasContext *ctx, uint16_t opcode)
> > >          bool (*decode_func)(DisasContext *, uint32_t);
> > >      } decoders[] = {
> > >          { always_true_p,  decode_insn32 },
> > > +        { has_XVentanaCondOps_p,  decode_XVentanaCodeOps },
> > >      };
> > >
> > >      /* Check for compressed insn */
> > > --
> > > 2.33.1
> > >
> > >