[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension
From: |
Philipp Tomsich |
Subject: |
Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension |
Date: |
Wed, 19 Jan 2022 00:21:47 +0100 |
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
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?
> 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.
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?
>
> 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
> >
> >
Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension, Philippe Mathieu-Daudé, 2022/01/19
Re: [PATCH v2 2/2] target/riscv: Add XVentanaCondOps custom extension, Richard Henderson, 2022/01/25
Re: [PATCH v2 1/2] target/riscv: iterate over a table of decoders, Philippe Mathieu-Daudé, 2022/01/19
Re: [PATCH v2 1/2] target/riscv: iterate over a table of decoders, Richard Henderson, 2022/01/25