[PATCH v3 0/3] Update capstone module

[PATCH v3 0/3] Update capstone module

[Richard Henderson]

Tested vs centos7, fedora30, and bionic (with and without
system capstone installed).

Changes for v3:
  * Work around the various include directory nonsense.
  * Re-add the s390 skipdata callback, as a separate patch.

Changes for v2:
  * Drop the installed directory change.  This does force a
    different include change when building from git.
  * Drop the s390 skipdata callback for now.


r~


Richard Henderson (3):
  capstone: Update to master
  capstone: Enable disassembly for s390x
  capstone: Fix s390x skipdata

 Makefile           |  2 ++
 disas.c            | 40 ++++++++++++++++++++++++++++++++++++++++
 target/s390x/cpu.c |  4 ++++
 capstone           |  2 +-
 configure          |  2 +-
 5 files changed, 48 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] capstone: Update to master

[Richard Henderson]

Update to 418d36d695e0.  Choose this over the 4.0.1 tag because
master now includes the s390x z13 vector opcodes.

Acked-by: David Hildenbrand <david@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 Makefile  | 1 +
 capstone  | 2 +-
 configure | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 30f0abfb42..8ce48e0342 100644
--- a/Makefile
+++ b/Makefile
@@ -498,6 +498,7 @@ dtc/%: .git-submodule-status
 # Remove all the extra -Warning flags that QEMU uses that Capstone doesn't;
 # no need to annoy QEMU developers with such things.
 CAP_CFLAGS = $(patsubst -W%,,$(CFLAGS) $(QEMU_CFLAGS))
+CAP_CFLAGS += -I$(SRC_PATH)/capstone/include
 CAP_CFLAGS += -DCAPSTONE_USE_SYS_DYN_MEM
 CAP_CFLAGS += -DCAPSTONE_HAS_ARM
 CAP_CFLAGS += -DCAPSTONE_HAS_ARM64
diff --git a/capstone b/capstone
index 22ead3e0bf..418d36d695 160000
--- a/capstone
+++ b/capstone
@@ -1 +1 @@
-Subproject commit 22ead3e0bfdb87516656453336160e0a37b066bf
+Subproject commit 418d36d695e075955674ace5a1191b495da50f84
diff --git a/configure b/configure
index 08ca4bcb46..f4f1860065 100755
--- a/configure
+++ b/configure
@@ -5008,7 +5008,7 @@ case "$capstone" in
       git_submodules="${git_submodules} capstone"
     fi
     mkdir -p capstone
-    QEMU_CFLAGS="$QEMU_CFLAGS -I\$(SRC_PATH)/capstone/include"
+    QEMU_CFLAGS="$QEMU_CFLAGS -I\$(SRC_PATH)/capstone/include/capstone"
     if test "$mingw32" = "yes"; then
       LIBCAPSTONE=capstone.lib
     else
-- 
2.17.1

Re: [PATCH v3 1/3] capstone: Update to master

[Thomas Huth]

On 15/10/2019 19.51, Richard Henderson wrote:
> Update to 418d36d695e0.  Choose this over the 4.0.1 tag because
> master now includes the s390x z13 vector opcodes.
In case you respin, please mention that this (hopefully) also fixes
https://bugs.launchpad.net/qemu/+bug/1826175

 Thanks,
  Thomas

[PATCH v3 2/3] capstone: Enable disassembly for s390x

[Richard Henderson]

Enable s390x, aka SYSZ, in the git submodule build.
Set the capstone parameters for both s390x host and guest.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 Makefile           | 1 +
 disas.c            | 3 +++
 target/s390x/cpu.c | 4 ++++
 3 files changed, 8 insertions(+)

diff --git a/Makefile b/Makefile
index 8ce48e0342..97e34be162 100644
--- a/Makefile
+++ b/Makefile
@@ -503,6 +503,7 @@ CAP_CFLAGS += -DCAPSTONE_USE_SYS_DYN_MEM
 CAP_CFLAGS += -DCAPSTONE_HAS_ARM
 CAP_CFLAGS += -DCAPSTONE_HAS_ARM64
 CAP_CFLAGS += -DCAPSTONE_HAS_POWERPC
+CAP_CFLAGS += -DCAPSTONE_HAS_SYSZ
 CAP_CFLAGS += -DCAPSTONE_HAS_X86
 
 .PHONY: capstone/all
diff --git a/disas.c b/disas.c
index 3e2bfa572b..51c71534a3 100644
--- a/disas.c
+++ b/disas.c
@@ -550,6 +550,9 @@ void disas(FILE *out, void *code, unsigned long size)
     print_insn = print_insn_m68k;
 #elif defined(__s390__)
     print_insn = print_insn_s390;
+    s.info.cap_arch = CS_ARCH_SYSZ;
+    s.info.cap_insn_unit = 2;
+    s.info.cap_insn_split = 6;
 #elif defined(__hppa__)
     print_insn = print_insn_hppa;
 #endif
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 3abe7e80fd..44f40f1f8c 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -43,6 +43,7 @@
 #include "sysemu/tcg.h"
 #endif
 #include "fpu/softfloat-helpers.h"
+#include "disas/capstone.h"
 
 #define CR0_RESET       0xE0UL
 #define CR14_RESET      0xC2000000UL;
@@ -180,6 +181,9 @@ static void s390_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
 {
     info->mach = bfd_mach_s390_64;
     info->print_insn = print_insn_s390;
+    info->cap_arch = CS_ARCH_SYSZ;
+    info->cap_insn_unit = 2;
+    info->cap_insn_split = 6;
 }
 
 static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
-- 
2.17.1

[PATCH v3 3/3] capstone: Add s390x skipdata callback

[Richard Henderson]

Capstone assumes any unknown instruction is 2 bytes.
Instead, use the ilen field in the first two bits of
the instruction to stay in sync with the insn stream.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/disas.c b/disas.c
index 51c71534a3..2a000cbeb0 100644
--- a/disas.c
+++ b/disas.c
@@ -178,6 +178,39 @@ static int print_insn_od_target(bfd_vma pc, disassemble_info *info)
    to share this across calls and across host vs target disassembly.  */
 static __thread cs_insn *cap_insn;
 
+/*
+ * The capstone library always skips 2 bytes for S390X.
+ * This is less than ideal, since we can tell from the first two bits
+ * the size of the insn and thus stay in sync with the insn stream.
+ */
+static size_t CAPSTONE_API
+cap_skipdata_s390x_cb(const uint8_t *code, size_t code_size,
+                      size_t offset, void *user_data)
+{
+    size_t ilen;
+
+    /* See get_ilen() in target/s390x/internal.h.  */
+    switch (code[offset] >> 6) {
+    case 0:
+        ilen = 2;
+        break;
+    case 1:
+    case 2:
+        ilen = 4;
+        break;
+    default:
+        ilen = 6;
+        break;
+    }
+
+    return ilen;
+}
+
+static const cs_opt_skipdata cap_skipdata_s390x = {
+    .mnemonic = ".byte",
+    .callback = cap_skipdata_s390x_cb
+};
+
 /* Initialize the Capstone library.  */
 /* ??? It would be nice to cache this.  We would need one handle for the
    host and one for the target.  For most targets we can reset specific
@@ -208,6 +241,10 @@ static cs_err cap_disas_start(disassemble_info *info, csh *handle)
 
     /* "Disassemble" unknown insns as ".byte W,X,Y,Z".  */
     cs_option(*handle, CS_OPT_SKIPDATA, CS_OPT_ON);
+    if (info->cap_arch == CS_ARCH_SYSZ) {
+        cs_option(*handle, CS_OPT_SKIPDATA_SETUP,
+                  (uintptr_t)&cap_skipdata_s390x);
+    }
 
     /* Allocate temp space for cs_disasm_iter.  */
     if (cap_insn == NULL) {
-- 
2.17.1

Re: [PATCH v3 3/3] capstone: Add s390x skipdata callback

[Thomas Huth]

On 15/10/2019 19.51, Richard Henderson wrote:
> Capstone assumes any unknown instruction is 2 bytes.
> Instead, use the ilen field in the first two bits of
> the instruction to stay in sync with the insn stream.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  disas.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/disas.c b/disas.c
> index 51c71534a3..2a000cbeb0 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -178,6 +178,39 @@ static int print_insn_od_target(bfd_vma pc, disassemble_info *info)
>     to share this across calls and across host vs target disassembly.  */
>  static __thread cs_insn *cap_insn;
>  
> +/*
> + * The capstone library always skips 2 bytes for S390X.
> + * This is less than ideal, since we can tell from the first two bits
> + * the size of the insn and thus stay in sync with the insn stream.
> + */
> +static size_t CAPSTONE_API
> +cap_skipdata_s390x_cb(const uint8_t *code, size_t code_size,
> +                      size_t offset, void *user_data)
> +{
> +    size_t ilen;
> +
> +    /* See get_ilen() in target/s390x/internal.h.  */
> +    switch (code[offset] >> 6) {
> +    case 0:
> +        ilen = 2;
> +        break;
> +    case 1:
> +    case 2:
> +        ilen = 4;
> +        break;
> +    default:
> +        ilen = 6;
> +        break;
> +    }
> +
> +    return ilen;
> +}

The kernel has also a nice function to calculate this:

static inline int insn_length(unsigned char code)
{
        return ((((int) code + 64) >> 7) + 1) << 1;
}

... but the switch-case is likely easier to read, so anyway:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v3 3/3] capstone: Add s390x skipdata callback

[Richard Henderson]

On 10/15/19 11:46 AM, Thomas Huth wrote:
>> +cap_skipdata_s390x_cb(const uint8_t *code, size_t code_size,
>> +                      size_t offset, void *user_data)
>> +{
>> +    size_t ilen;
>> +
>> +    /* See get_ilen() in target/s390x/internal.h.  */
>> +    switch (code[offset] >> 6) {
>> +    case 0:
>> +        ilen = 2;
>> +        break;
>> +    case 1:
>> +    case 2:
>> +        ilen = 4;
>> +        break;
>> +    default:
>> +        ilen = 6;
>> +        break;
>> +    }
>> +
>> +    return ilen;
>> +}
> 
> The kernel has also a nice function to calculate this:
> 
> static inline int insn_length(unsigned char code)
> {
>         return ((((int) code + 64) >> 7) + 1) << 1;
> }
> 
> ... but the switch-case is likely easier to read, so anyway:

Clever.

I don't mind swapping to the kernel version, so long as we convert the
target/s390x/internal.h function as well.


r~

Re: [PATCH v3 0/3] Update capstone module

[Philippe Mathieu-Daudé]

Hi Richard,

On 10/15/19 7:51 PM, Richard Henderson wrote:
> Tested vs centos7, fedora30, and bionic (with and without
> system capstone installed).

Change noted while testing:

* Raw byte dumped as hexa

   before: no
   after:  dumped by 16-bit

   OK

* Address format

   before: "0x" TARGET_FMT_lx (16 chars)
   after:  "0x%08" PRIx64     (8 chars)

   Shorten this might be OK because we now also dump
   the raw bytes previous to the mnemonic/arguments

   -0x0000000000010014:  mvi       163,1
   -0x0000000000010018:  slr       %r0,%r0
   -0x000000000001001a:  lhi       %r1,2
   +0x00010014:  9201 00a3       mvi      0xa3, 1
   +0x00010018:  1f00            slr      %r0, %r0
   +0x0001001a:  a718 0002       lhi      %r1, 2

* Number argument format

   before: decimal
   after:  hexa

   -0x00010014:  mvi       163,1
   +0x00010014:   mvi      0xa3, 1

   OK

* (Priviledged) Instruction missing

   -0x0001001e:  sigp      %r1,%r0,18
   +0x0001001e:   .byte    0xae, 0x10, 0x00, 0x12

   -0x00010066:  lmh       %r0,%r15,0(%r13)
   +0x00010066:   .byte    0xeb, 0x0f, 0xd0, 0x00, 0x00, 0x96

   -0x0001006c:  sam64
   +0x0001006c:   .byte    0x01, 0x0e

   -0x00010088:  lctlg     %c0,%c15,512
   +0x00010088:   .byte    0xeb, 0x0f, 0x02, 0x00, 0x00, 0x2f

   -0x0001008e:  stcke     808
   +0x0001008e:   .byte    0xb2, 0x78, 0x03, 0x28

   -0x00010098:  spt       80(%r13)
   +0x00010098:   .byte    0xb2, 0x08, 0xd0, 0x50

   -0x000149b6:  stfl      0
   +0x000149b6:   .byte    0xb2, 0xb1, 0x00, 0x00

   -0x000149da:  stfle     0(%r1)
   +0x000149da:   .byte    0xb2, 0xb0, 0x10, 0x00

   -0x00011a34:  icm       %r5,3,0(%r1)
   +0x00011a34:   .byte    0xbf, 0x53, 0x10, 0x00

   -0x0010e8f6:  lpswe     160(%r15)
   +0x0010e8f6:   .byte    0xb2, 0xb2, 0xf0, 0xa0

Is it possible to fallback to the older disassembler on a 
per-instruction basis if Capstone doesn't know about an instruction?

> Changes for v3:
>    * Work around the various include directory nonsense.
>    * Re-add the s390 skipdata callback, as a separate patch.
> 
> Changes for v2:
>    * Drop the installed directory change.  This does force a
>      different include change when building from git.
>    * Drop the s390 skipdata callback for now.

Re: [PATCH v3 0/3] Update capstone module

[Richard Henderson]

On 1/3/20 6:16 PM, Philippe Mathieu-Daudé wrote:
>   -0x0010e8f6:  lpswe     160(%r15)
>   +0x0010e8f6:   .byte    0xb2, 0xb2, 0xf0, 0xa0
> 
> Is it possible to fallback to the older disassembler on a per-instruction basis
> if Capstone doesn't know about an instruction?

Not as written.  But I suppose we could rearrange both dump loops to allow such
a thing.


r~