[RFC PATCH 0/4] Support multiple deocde path for RISC-V

[RFC PATCH 0/4] Support multiple deocde path for RISC-V

[LIU Zhiwei]

The patch set add support for disassembling XVentanaCondOps instructions.
And we can also extend it to disassemble the other custom extensions.

The target_info field in struct disassemble_info is intended to be used
by targets in any way they deem suitable. We encode the custom extension
in this field and decode the custom instructions according to this information.

LIU Zhiwei (4):
  target/riscv: Use xl instead of mxl for disassemble
  disas/riscv: Move down the struct rv_decode
  disas/riscv: Add used_opcode_data field
  target/riscv: Support Ventana disassemble

 disas/riscv.c         | 103 +++++++++++++++++++++++++++++++++---------
 target/riscv/cpu.c    |  22 ++++++++-
 target/riscv/custom.h |  25 ++++++++++
 3 files changed, 127 insertions(+), 23 deletions(-)
 create mode 100644 target/riscv/custom.h

-- 
2.25.1

[RFC PATCH 1/4] target/riscv: Use xl instead of mxl for disassemble

[LIU Zhiwei]

Disassemble function(plugin_disas, target_disas, monitor_disas) will
always call set_disas_info before disassembling instructions.

plugin_disas  and target_disas will always be called under a TB, which
has the same XLEN.

We can't ensure that monitor_disas will always be called under a TB,
but current XLEN will still be a better choice, thus we can ensure at
least the disassemble of the nearest one TB is right.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 target/riscv/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ac6f82ebd0..a5f84f211d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -507,8 +507,9 @@ static void riscv_cpu_reset(DeviceState *dev)
 static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
 {
     RISCVCPU *cpu = RISCV_CPU(s);
+    CPURISCVState *env = &cpu->env;
 
-    switch (riscv_cpu_mxl(&cpu->env)) {
+    switch (env->xl) {
     case MXL_RV32:
         info->print_insn = print_insn_riscv32;
         break;
-- 
2.25.1

Re: [RFC PATCH 1/4] target/riscv: Use xl instead of mxl for disassemble

[Richard Henderson]

On 8/24/22 06:03, LIU Zhiwei wrote:
> Disassemble function(plugin_disas, target_disas, monitor_disas) will
> always call set_disas_info before disassembling instructions.
> 
> plugin_disas  and target_disas will always be called under a TB, which
> has the same XLEN.
> 
> We can't ensure that monitor_disas will always be called under a TB,
> but current XLEN will still be a better choice, thus we can ensure at
> least the disassemble of the nearest one TB is right.
> 
> Signed-off-by: LIU Zhiwei<zhiwei_liu@linux.alibaba.com>
> ---
>   target/riscv/cpu.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC PATCH 1/4] target/riscv: Use xl instead of mxl for disassemble

[Alistair Francis]

On Wed, Aug 24, 2022 at 5:43 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
> Disassemble function(plugin_disas, target_disas, monitor_disas) will
> always call set_disas_info before disassembling instructions.
>
> plugin_disas  and target_disas will always be called under a TB, which
> has the same XLEN.
>
> We can't ensure that monitor_disas will always be called under a TB,
> but current XLEN will still be a better choice, thus we can ensure at
> least the disassemble of the nearest one TB is right.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index ac6f82ebd0..a5f84f211d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -507,8 +507,9 @@ static void riscv_cpu_reset(DeviceState *dev)
>  static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
>  {
>      RISCVCPU *cpu = RISCV_CPU(s);
> +    CPURISCVState *env = &cpu->env;
>
> -    switch (riscv_cpu_mxl(&cpu->env)) {
> +    switch (env->xl) {
>      case MXL_RV32:
>          info->print_insn = print_insn_riscv32;
>          break;
> --
> 2.25.1
>
>

[RFC PATCH 2/4] disas/riscv: Move down the struct rv_decode

[LIU Zhiwei]

We will later add a struct rv_opcode_date filed in struct rv_decode,
which is defined after struct rv_decode.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 disas/riscv.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/disas/riscv.c b/disas/riscv.c
index 7af6afc8fa..5c36ae08dc 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -564,25 +564,6 @@ typedef enum {
 
 /* structures */
 
-typedef struct {
-    uint64_t  pc;
-    uint64_t  inst;
-    int32_t   imm;
-    uint16_t  op;
-    uint8_t   codec;
-    uint8_t   rd;
-    uint8_t   rs1;
-    uint8_t   rs2;
-    uint8_t   rs3;
-    uint8_t   rm;
-    uint8_t   pred;
-    uint8_t   succ;
-    uint8_t   aq;
-    uint8_t   rl;
-    uint8_t   bs;
-    uint8_t   rnum;
-} rv_decode;
-
 typedef struct {
     const int op;
     const rvc_constraint *constraints;
@@ -603,6 +584,25 @@ typedef struct {
     const short decomp_data;
 } rv_opcode_data;
 
+typedef struct {
+    uint64_t  pc;
+    uint64_t  inst;
+    int32_t   imm;
+    uint16_t  op;
+    uint8_t   codec;
+    uint8_t   rd;
+    uint8_t   rs1;
+    uint8_t   rs2;
+    uint8_t   rs3;
+    uint8_t   rm;
+    uint8_t   pred;
+    uint8_t   succ;
+    uint8_t   aq;
+    uint8_t   rl;
+    uint8_t   bs;
+    uint8_t   rnum;
+} rv_decode;
+
 /* register names */
 
 static const char rv_ireg_name_sym[32][5] = {
-- 
2.25.1

[RFC PATCH 3/4] disas/riscv: Add used_opcode_data field

[LIU Zhiwei]

Add used_opcode_data field in rv_decode. After this patch, we will
not use opcode_data directly.

This will give the custom opcode data a choice to be selected in
the future.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 disas/riscv.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/disas/riscv.c b/disas/riscv.c
index 5c36ae08dc..aaf85b2aba 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -601,6 +601,7 @@ typedef struct {
     uint8_t   rl;
     uint8_t   bs;
     uint8_t   rnum;
+    const rv_opcode_data *used_opcode_data;
 } rv_decode;
 
 /* register names */
@@ -2532,7 +2533,7 @@ static uint32_t operand_rnum(rv_inst inst)
 static void decode_inst_operands(rv_decode *dec)
 {
     rv_inst inst = dec->inst;
-    dec->codec = opcode_data[dec->op].codec;
+    dec->codec = dec->used_opcode_data[dec->op].codec;
     switch (dec->codec) {
     case rv_codec_none:
         dec->rd = dec->rs1 = dec->rs2 = rv_ireg_zero;
@@ -2959,6 +2960,7 @@ static void format_inst(char *buf, size_t buflen, size_t tab, rv_decode *dec)
 {
     char tmp[64];
     const char *fmt;
+    const rv_opcode_data *opcode_data = dec->used_opcode_data;
 
     fmt = opcode_data[dec->op].format;
     while (*fmt) {
@@ -3111,6 +3113,7 @@ static void format_inst(char *buf, size_t buflen, size_t tab, rv_decode *dec)
 
 static void decode_inst_lift_pseudo(rv_decode *dec)
 {
+    const rv_opcode_data *opcode_data = dec->used_opcode_data;
     const rv_comp_data *comp_data = opcode_data[dec->op].pseudo;
     if (!comp_data) {
         return;
@@ -3129,6 +3132,7 @@ static void decode_inst_lift_pseudo(rv_decode *dec)
 
 static void decode_inst_decompress_rv32(rv_decode *dec)
 {
+    const rv_opcode_data *opcode_data = dec->used_opcode_data;
     int decomp_op = opcode_data[dec->op].decomp_rv32;
     if (decomp_op != rv_op_illegal) {
         if ((opcode_data[dec->op].decomp_data & rvcd_imm_nz)
@@ -3143,6 +3147,7 @@ static void decode_inst_decompress_rv32(rv_decode *dec)
 
 static void decode_inst_decompress_rv64(rv_decode *dec)
 {
+    const rv_opcode_data *opcode_data = dec->used_opcode_data;
     int decomp_op = opcode_data[dec->op].decomp_rv64;
     if (decomp_op != rv_op_illegal) {
         if ((opcode_data[dec->op].decomp_data & rvcd_imm_nz)
@@ -3157,6 +3162,7 @@ static void decode_inst_decompress_rv64(rv_decode *dec)
 
 static void decode_inst_decompress_rv128(rv_decode *dec)
 {
+    const rv_opcode_data *opcode_data = dec->used_opcode_data;
     int decomp_op = opcode_data[dec->op].decomp_rv128;
     if (decomp_op != rv_op_illegal) {
         if ((opcode_data[dec->op].decomp_data & rvcd_imm_nz)
@@ -3192,6 +3198,7 @@ disasm_inst(char *buf, size_t buflen, rv_isa isa, uint64_t pc, rv_inst inst)
     rv_decode dec = { 0 };
     dec.pc = pc;
     dec.inst = inst;
+    dec.used_opcode_data = opcode_data;
     decode_inst_opcode(&dec, isa);
     decode_inst_operands(&dec);
     decode_inst_decompress(&dec, isa);
-- 
2.25.1

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

[LIU Zhiwei]

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;
+
 /* 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 },
+};
+
 /* 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;
             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);
+
     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

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

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

[LIU Zhiwei]

Hi Alistair,

Thanks for your comments.

On 2022/8/30 17:03, Alistair Francis wrote:
> 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?

Ventana_op_vt_maskc and Ventana_op_vt_maskcn have been used in this 
patch.Actually, they will be used as the index of 
Ventana_opcode_data(the custom opcode data for Ventana).
So it should also keep strictly in pace with the items in 
Ventana_opcode_data.

After decode_inst_opcode, there are many use cases of 
"opcode_data[dec->op]" and the dec->op is
the Ventana_op_vt_maskc or Ventana_op_vt_maskcn here.

>> +
>>   /* 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?
Yes. If we split it into a file, such as disas/ventana_riscv.c, we will 
have to split some data structures into a header file, because 
Ventana_opcode_data use rv_codec_r and rv_fmt_rd_rs1_rs2.
>
>> +
>>   /* 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
Agree.
>>               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?

This is possible. Strictly, we have no reason to refuse them if two 
vendor extensions don't collide with each other.

But I still have not find a good way to check the collision, except 
decoding and checking one by one.

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

Agree. I will have a try in next version.

Thanks,
Zhiwei

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