Re: [Qemu-devel] [PATCH v8 04/23] RISC-V Disassembler

2018-04-29 Thread Michael Clark
On Sat, Apr 28, 2018 at 12:26 AM, Peter Maydell 
wrote:

> On 2 March 2018 at 13:51, Michael Clark  wrote:
> > The RISC-V disassembler has no dependencies outside of the 'disas'
> > directory so it can be applied independently. The majority of the
> > disassembler is machine-generated from instruction set metadata:
> >
> > - https://github.com/michaeljclark/riscv-meta
>
> > +case 5:
> > +if (isa == rv128) {
> > +op = rv_op_c_sqsp;
> > +} else {
> > +op = rv_op_c_fsdsp; break;
> > +}
> > +case 6: op = rv_op_c_swsp; break;
>
> Coverity (CID1390575) points out that for the
> case 5 / isa == rv128 codepath, we set op, and
> then fall through into case 6 which overwrites it.
>
> Is there a missing "break" statement here? (If the
> fallthrough is deliberate it should be marked with a
> /* fallthrough */ comment.)
>

Thanks!

The disassembler is largely machine generated but the generator has an
issue with ISAs > 2 on a single set of encodings i.e. RVC. The generator
was originally coded for 1 or 2 encodings but I later added rv128 to the
opcodes metadata. I manually edited the generated code to add rv128
disassembly support for compressed instructions, and obviously made a
mistake in hand transcription (the reason I went to the effort of making
the generator). The compressed instructions overload the same encodings
with different meanings for the 3 ISAs, and this is where the generator
fell down, so it was only rv128 compressed instructions that I hand edited.
My own propensity to make mistakes is what attracts me to machine generated
code and of course lots of testing.

128-bit support would be interesting. Fabrice's RISCVEMU supports RV128
however it is an interpreter. If the large guest code is factored
correctly, then we could support RV128 on 64-bit hosts much like we can
support 64-bit targets on 32-bit hosts.

I've just spun and sent a patch for review. Given it's only broken on rv128
I'll just add it to the existing series.

Regards,
Michael.


Re: [Qemu-devel] [PATCH v8 04/23] RISC-V Disassembler

2018-04-27 Thread Peter Maydell
On 2 March 2018 at 13:51, Michael Clark  wrote:
> The RISC-V disassembler has no dependencies outside of the 'disas'
> directory so it can be applied independently. The majority of the
> disassembler is machine-generated from instruction set metadata:
>
> - https://github.com/michaeljclark/riscv-meta

> +case 5:
> +if (isa == rv128) {
> +op = rv_op_c_sqsp;
> +} else {
> +op = rv_op_c_fsdsp; break;
> +}
> +case 6: op = rv_op_c_swsp; break;

Coverity (CID1390575) points out that for the
case 5 / isa == rv128 codepath, we set op, and
then fall through into case 6 which overwrites it.

Is there a missing "break" statement here? (If the
fallthrough is deliberate it should be marked with a
/* fallthrough */ comment.)

thanks
-- PMM



[Qemu-devel] [PATCH v8 04/23] RISC-V Disassembler

2018-03-02 Thread Michael Clark
The RISC-V disassembler has no dependencies outside of the 'disas'
directory so it can be applied independently. The majority of the
disassembler is machine-generated from instruction set metadata:

- https://github.com/michaeljclark/riscv-meta

Expected checkpatch errors for consistency and brevity reasons:

ERROR: line over 90 characters
ERROR: trailing statements should be on next line
ERROR: space prohibited between function name and open parenthesis '('

Reviewed-by: Richard Henderson 
Signed-off-by: Michael Clark 
---
 disas.c |2 +
 disas/Makefile.objs |1 +
 disas/riscv.c   | 3048 +++
 include/disas/bfd.h |2 +
 4 files changed, 3053 insertions(+)
 create mode 100644 disas/riscv.c

diff --git a/disas.c b/disas.c
index d4ad108..5325b7e 100644
--- a/disas.c
+++ b/disas.c
@@ -522,6 +522,8 @@ void disas(FILE *out, void *code, unsigned long size)
 # ifdef _ARCH_PPC64
 s.info.cap_mode = CS_MODE_64;
 # endif
+#elif defined(__riscv__)
+print_insn = print_insn_riscv;
 #elif defined(__aarch64__) && defined(CONFIG_ARM_A64_DIS)
 print_insn = print_insn_arm_a64;
 s.info.cap_arch = CS_ARCH_ARM64;
diff --git a/disas/Makefile.objs b/disas/Makefile.objs
index 53556f8..213be2f 100644
--- a/disas/Makefile.objs
+++ b/disas/Makefile.objs
@@ -17,6 +17,7 @@ common-obj-$(CONFIG_MIPS_DIS) += mips.o
 common-obj-$(CONFIG_NIOS2_DIS) += nios2.o
 common-obj-$(CONFIG_MOXIE_DIS) += moxie.o
 common-obj-$(CONFIG_PPC_DIS) += ppc.o
+common-obj-$(CONFIG_RISCV_DIS) += riscv.o
 common-obj-$(CONFIG_S390_DIS) += s390.o
 common-obj-$(CONFIG_SH4_DIS) += sh4.o
 common-obj-$(CONFIG_SPARC_DIS) += sparc.o
diff --git a/disas/riscv.c b/disas/riscv.c
new file mode 100644
index 000..3c17501
--- /dev/null
+++ b/disas/riscv.c
@@ -0,0 +1,3048 @@
+/*
+ * QEMU RISC-V Disassembler
+ *
+ * Copyright (c) 2016-2017 Michael Clark 
+ * Copyright (c) 2017-2018 SiFive, Inc.
+ *
+ * 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 .
+ */
+
+#include "qemu/osdep.h"
+#include "disas/bfd.h"
+
+
+/* types */
+
+typedef uint64_t rv_inst;
+typedef uint16_t rv_opcode;
+
+/* enums */
+
+typedef enum {
+rv32,
+rv64,
+rv128
+} rv_isa;
+
+typedef enum {
+rv_rm_rne = 0,
+rv_rm_rtz = 1,
+rv_rm_rdn = 2,
+rv_rm_rup = 3,
+rv_rm_rmm = 4,
+rv_rm_dyn = 7,
+} rv_rm;
+
+typedef enum {
+rv_fence_i = 8,
+rv_fence_o = 4,
+rv_fence_r = 2,
+rv_fence_w = 1,
+} rv_fence;
+
+typedef enum {
+rv_ireg_zero,
+rv_ireg_ra,
+rv_ireg_sp,
+rv_ireg_gp,
+rv_ireg_tp,
+rv_ireg_t0,
+rv_ireg_t1,
+rv_ireg_t2,
+rv_ireg_s0,
+rv_ireg_s1,
+rv_ireg_a0,
+rv_ireg_a1,
+rv_ireg_a2,
+rv_ireg_a3,
+rv_ireg_a4,
+rv_ireg_a5,
+rv_ireg_a6,
+rv_ireg_a7,
+rv_ireg_s2,
+rv_ireg_s3,
+rv_ireg_s4,
+rv_ireg_s5,
+rv_ireg_s6,
+rv_ireg_s7,
+rv_ireg_s8,
+rv_ireg_s9,
+rv_ireg_s10,
+rv_ireg_s11,
+rv_ireg_t3,
+rv_ireg_t4,
+rv_ireg_t5,
+rv_ireg_t6,
+} rv_ireg;
+
+typedef enum {
+rvc_end,
+rvc_simm_6,
+rvc_imm_6,
+rvc_imm_7,
+rvc_imm_8,
+rvc_imm_9,
+rvc_imm_10,
+rvc_imm_12,
+rvc_imm_18,
+rvc_imm_nz,
+rvc_imm_x2,
+rvc_imm_x4,
+rvc_imm_x8,
+rvc_imm_x16,
+rvc_rd_b3,
+rvc_rs1_b3,
+rvc_rs2_b3,
+rvc_rd_eq_rs1,
+rvc_rd_eq_ra,
+rvc_rd_eq_sp,
+rvc_rd_eq_x0,
+rvc_rs1_eq_sp,
+rvc_rs1_eq_x0,
+rvc_rs2_eq_x0,
+rvc_rd_ne_x0_x2,
+rvc_rd_ne_x0,
+rvc_rs1_ne_x0,
+rvc_rs2_ne_x0,
+rvc_rs2_eq_rs1,
+rvc_rs1_eq_ra,
+rvc_imm_eq_zero,
+rvc_imm_eq_n1,
+rvc_imm_eq_p1,
+rvc_csr_eq_0x001,
+rvc_csr_eq_0x002,
+rvc_csr_eq_0x003,
+rvc_csr_eq_0xc00,
+rvc_csr_eq_0xc01,
+rvc_csr_eq_0xc02,
+rvc_csr_eq_0xc80,
+rvc_csr_eq_0xc81,
+rvc_csr_eq_0xc82,
+} rvc_constraint;
+
+typedef enum {
+rv_codec_illegal,
+rv_codec_none,
+rv_codec_u,
+rv_codec_uj,
+rv_codec_i,
+rv_codec_i_sh5,
+rv_codec_i_sh6,
+rv_codec_i_sh7,
+rv_codec_i_csr,
+rv_codec_s,
+rv_codec_sb,
+rv_codec_r,
+rv_codec_r_m,
+rv_codec_r4_m,
+rv_codec_r_a,
+rv_codec_r_l,
+rv_codec_r_f,
+rv_codec_cb,
+rv_codec_cb_imm,
+rv_codec_cb_sh5,
+rv_codec_cb_sh6,
+rv_codec_ci,
+rv_codec_ci_sh5,
+rv_codec_ci_sh6,
+rv_codec_ci_16sp,
+rv_codec_ci_lwsp,
+