| Message ID | 20220113202033.3320854-2-philipp.tomsich@vrull.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] target/riscv: iterate over a table of decoders | expand |
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. Vendor extensions can not interfere with other extensions and can not be obtrusive to the RISC-V target code. 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 > >
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 > > > >
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 > > > > > >
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
On 13/1/22 21:20, Philipp Tomsich 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> > > --- > > Changes in v2: > - Split off decode table into XVentanaCondOps.decode > - Wire up XVentanaCondOps in the decoder-table > 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 }, "always_true" is the first entry, > + { has_XVentanaCondOps_p, decode_XVentanaCodeOps }, so is that ever called? > }; > > /* Check for compressed insn */
On Wed, 19 Jan 2022 at 12:17, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 13/1/22 21:20, Philipp Tomsich 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> > > > > --- > > > > Changes in v2: > > - Split off decode table into XVentanaCondOps.decode > > - Wire up XVentanaCondOps in the decoder-table > > > 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 }, > > "always_true" is the first entry, > > > + { has_XVentanaCondOps_p, decode_XVentanaCodeOps }, > > so is that ever called? Please refer to patch 1/2: 1. The guard-function only gates whether a decoder function is enabled/called. 2. Enabled decoders are iterated over until a decoder handles the instruction-word—or we run out of decoders. 3. If no enabled decoder handled an instruction word, we raise an illegal instruction. This really is just a table-based form of the what would be equivalent to the following pseudocode: if (guard_func_1() && decoder1(…)) /* pass */ ; else if (guard_func_2() && decoder2(...)) /* pass */ ; [...] else raise_illegal(); And just as an aside (before we start discussing performance), let's make sure we all agree that this is perfectly optimizable (I may be missing a 'const') by a compiler: 1. The number of entries in the array are known at compile-time and small integer — a compiler can thus peel the loop. 2. The function pointers are in the same compilation unit, so this can be converted from indirect to direct calls (a special case of constant-propagation). 3. Predicate functions (given that they will be very small) can be inlined. Best, Philipp. > > > > }; > > > > /* Check for compressed insn */ >
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
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
On 1/14/22 7:20 AM, Philipp Tomsich wrote: > new file mode 100644 > index 0000000000..b8a5d031b5 > --- /dev/null > +++ b/target/riscv/insn_trans/trans_xventanacondops.inc The filename suffix should be ".c.inc". > +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); > +} Implementation looks good. > +#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 ; \ > + } Again, no inline. Don't look back to RISCV_CPU here. We shouldn't even have access to that here, as it leads to temptation to do invalid things at translation time (this isn't one of them, since it only accesses constant state). What we have been doing is copying ext_foo into DisasContext in riscv_tr_init_disas_context. Though it might be time to revisit that. Perhaps give the cpu->cfg structure type a name, e.g. RISCVCPUConfig. Add "const RISCVCPUConfig *cfg" to DisasContext and copy the pointer across in init_disas_context. r~
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 */
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> --- 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