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

[Alistair Francis]

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
>
>