Re: [RFC PATCH 4/4] target/riscv: Support Ventana disassemble

[Alistair Francis]

On Wed, Aug 24, 2022 at 5:37 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
> Pass through the custom information to disassemble by the target_info
> field. In disassemble, select the decode path according to the custom
> extension.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
>  disas/riscv.c         | 56 +++++++++++++++++++++++++++++++++++++++++--
>  target/riscv/cpu.c    | 19 +++++++++++++++
>  target/riscv/custom.h | 25 +++++++++++++++++++
>  3 files changed, 98 insertions(+), 2 deletions(-)
>  create mode 100644 target/riscv/custom.h
>
> diff --git a/disas/riscv.c b/disas/riscv.c
> index aaf85b2aba..590cdba0f6 100644
> --- a/disas/riscv.c
> +++ b/disas/riscv.c
> @@ -19,6 +19,7 @@
>
>  #include "qemu/osdep.h"
>  #include "disas/dis-asm.h"
> +#include "target/riscv/custom.h"
>
>
>  /* types */
> @@ -562,6 +563,11 @@ typedef enum {
>      rv_op_xperm8 = 398,
>  } rv_op;
>
> +typedef enum {
> +    Ventana_op_vt_maskc = 0,
> +    Ventana_op_vt_maskcn = 1,
> +} rv_Ventana_op;

This is unused right?

> +
>  /* structures */
>
>  typedef struct {
> @@ -602,6 +608,7 @@ typedef struct {
>      uint8_t   bs;
>      uint8_t   rnum;
>      const rv_opcode_data *used_opcode_data;
> +    const rv_opcode_data *custom_opcode_data;
>  } rv_decode;
>
>  /* register names */
> @@ -1287,6 +1294,11 @@ const rv_opcode_data opcode_data[] = {
>      { "xperm8", rv_codec_r, rv_fmt_rd_rs1, NULL, 0, 0, 0 }
>  };
>
> +const rv_opcode_data Ventana_opcode_data[] = {
> +    { "vt.maskc", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +    { "vt.maskcn", rv_codec_r, rv_fmt_rd_rs1_rs2, NULL, 0, 0, 0 },
> +};

Could we keep this in a vendor file instead?

> +
>  /* CSR names */
>
>  static const char *csr_name(int csrno)
> @@ -2244,6 +2256,18 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa 
> isa)
>              case 0: op = rv_op_addd; break;
>              case 1: op = rv_op_slld; break;
>              case 5: op = rv_op_srld; break;
> +            case 6: /* Todo: Move custom decode to sperate decode function */
> +                if (dec->custom_opcode_data == Ventana_opcode_data) {
> +                    op = Ventana_op_vt_maskc;
> +                    dec->used_opcode_data = dec->custom_opcode_data;
> +                }
> +                break;
> +            case 7:
> +                if (dec->custom_opcode_data == Ventana_opcode_data) {
> +                    op = Ventana_op_vt_maskcn;
> +                    dec->used_opcode_data = dec->custom_opcode_data;
> +                }
> +                break;

This seems like it won't scale very well as we add more custom extensions

>              case 8: op = rv_op_muld; break;
>              case 12: op = rv_op_divd; break;
>              case 13: op = rv_op_divud; break;
> @@ -3190,15 +3214,43 @@ static void decode_inst_decompress(rv_decode *dec, 
> rv_isa isa)
>      }
>  }
>
> +static const struct {
> +    enum RISCVCustom ext;
> +    const rv_opcode_data *opcode_data;
> +} custom_opcode_table[] = {
> +    { VENTANA_CUSTOM,   Ventana_opcode_data },
> +};
> +
> +static const rv_opcode_data *
> +get_custom_opcode_data(struct disassemble_info *info)
> +{
> +    for (size_t i = 0; i < ARRAY_SIZE(custom_opcode_table); ++i) {
> +        if (info->target_info & (1ULL << custom_opcode_table[i].ext)) {
> +            return custom_opcode_table[i].opcode_data;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  /* disassemble instruction */
>
>  static void
> -disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst)
> +disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst,
> +            struct disassemble_info *info)
>  {
>      rv_decode dec = { 0 };
>      dec.pc = pc;
>      dec.inst = inst;
> +
> +    /*
> +     * Set default opcode_data.
> +     * Only overide the default opcode_data only when
> +     * 1. There is a custom opcode data.
> +     * 2. The instruction belongs to the custom extension.
> +     */
>      dec.used_opcode_data = opcode_data;
> +    dec.custom_opcode_data = get_custom_opcode_data(info);

What if something has two vendor extensions?

I'm not sure we need this, it might be better to just check if the
extension is enabled and then use that decoder

Alistair

> +
>      decode_inst_opcode(&dec, isa);
>      decode_inst_operands(&dec);
>      decode_inst_decompress(&dec, isa);
> @@ -3253,7 +3305,7 @@ print_insn_riscv(bfd_vma memaddr, struct 
> disassemble_info *info, rv_isa isa)
>          break;
>      }
>
> -    disasm_inst(buf, sizeof(buf), isa, memaddr, inst);
> +    disasm_inst(buf, sizeof(buf), isa, memaddr, inst, info);
>      (*info->fprintf_func)(info->stream, "%s", buf);
>
>      return len;
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index a5f84f211d..cc6ef9303f 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -31,6 +31,7 @@
>  #include "fpu/softfloat-helpers.h"
>  #include "sysemu/kvm.h"
>  #include "kvm_riscv.h"
> +#include "custom.h"
>
>  /* RISC-V CPU definitions */
>
> @@ -504,11 +505,29 @@ static void riscv_cpu_reset(DeviceState *dev)
>  #endif
>  }
>
> +static bool has_Ventana_ext(const RISCVCPUConfig *cfg_ptr)
> +{
> +    return cfg_ptr->ext_XVentanaCondOps;
> +}
> +
>  static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
>  {
>      RISCVCPU *cpu = RISCV_CPU(s);
>      CPURISCVState *env = &cpu->env;
>
> +    static const struct {
> +        bool (*guard_func)(const RISCVCPUConfig *);
> +        enum RISCVCustom ext;
> +    } customs[] = {
> +        { has_Ventana_ext, VENTANA_CUSTOM },
> +    };
> +
> +    for (size_t i = 0; i < ARRAY_SIZE(customs); ++i) {
> +        if (customs[i].guard_func(&(cpu->cfg))) {
> +            info->target_info |= 1ULL << customs[i].ext;
> +        }
> +    }
> +
>      switch (env->xl) {
>      case MXL_RV32:
>          info->print_insn = print_insn_riscv32;
> diff --git a/target/riscv/custom.h b/target/riscv/custom.h
> new file mode 100644
> index 0000000000..1a161984c0
> --- /dev/null
> +++ b/target/riscv/custom.h
> @@ -0,0 +1,25 @@
> +/*
> + * QEMU RISC-V CPU -- custom extensions
> + *
> + * Copyright (c) 2022 T-Head Semiconductor Co., Ltd. All rights reserved.
> + *
> + * 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/>.
> + */
> +#ifndef RISCV_CPU_CUSTOM_H
> +#define RISCV_CPU_CUSTOM_H
> +
> +enum RISCVCustom {
> +    VENTANA_CUSTOM = 0,
> +};
> +
> +#endif
> --
> 2.25.1
>
>