qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: LIU Zhiwei
Subject: Re: [RFC PATCH 4/4] target/riscv: Support Ventana disassemble
Date: Tue, 30 Aug 2022 20:54:05 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 
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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]