[PATCH 0/3] capstone: update to next

[PATCH 0/3] capstone: update to next

[Richard Henderson]

I keep hoping that there will be a new official capstone
release, but I've now waited all of 2019, so I'm going to
suggest updating to the head of the development branch.

That, at least, has significant benefits wrt read-only data:
writable data down from 1.5MB to 48 bytes.


r~


Richard Henderson (3):
  capstone: Update to next
  capstone: Enable disassembly for s390x
  capstone: Add skipdata hook for s390x

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

-- 
2.20.1

[PATCH 1/3] capstone: Update to next

[Richard Henderson]

Update to aaffb38c44fa.  Choose this over the "current" 4.0.1 tag
because next now includes the s390x z13 vector opcodes, and also
the insn tables are now read-only.

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 6b5ad1121b..12e129ac9d 100644
--- a/Makefile
+++ b/Makefile
@@ -499,6 +499,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..aaffb38c44 160000
--- a/capstone
+++ b/capstone
@@ -1 +1 @@
-Subproject commit 22ead3e0bfdb87516656453336160e0a37b066bf
+Subproject commit aaffb38c44fa58f510ba9b6264f7079bfbba4c8e
diff --git a/configure b/configure
index 940bf9e87a..de96bfe017 100755
--- a/configure
+++ b/configure
@@ -5062,7 +5062,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.20.1

Re: [PATCH 1/3] capstone: Update to next

[Philippe Mathieu-Daudé]

On 1/3/20 10:24 PM, Richard Henderson wrote:
> Update to aaffb38c44fa.  Choose this over the "current" 4.0.1 tag
> because next now includes the s390x z13 vector opcodes, and also
> the insn tables are now read-only.
> 

As Thomas suggested,
Fixes: https://bugs.launchpad.net/qemu/+bug/1826175

With GCC on Linux:
Tested-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 6b5ad1121b..12e129ac9d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -499,6 +499,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..aaffb38c44 160000
> --- a/capstone
> +++ b/capstone
> @@ -1 +1 @@
> -Subproject commit 22ead3e0bfdb87516656453336160e0a37b066bf
> +Subproject commit aaffb38c44fa58f510ba9b6264f7079bfbba4c8e

Looking at https://github.com/aquynh/capstone/pull/1549, this is 
unfortunate the upstream project use the 'squash merge request' feature :(

Since I already reviewed various of the 1589 patches in the earlier 
version of this patch (22ead3e0bf..418d36d695) I'll only focus on the 
291 'squashed' commits added on top:

   $ git log --oneline 418d36d695..aaffb38c44|wc -l
   291

Most of the commits contains a single line of description, and various 
of them have hundreds of lines of changes.

Similarly to my previous review, I skipped the architecture we don't 
support and X86. Still to many ARM/Aarch64 patches to review, so I'm 
focusing on the buildsys and changes on the API we use:

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> diff --git a/configure b/configure
> index 940bf9e87a..de96bfe017 100755
> --- a/configure
> +++ b/configure
> @@ -5062,7 +5062,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
> 

[PATCH 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 12e129ac9d..df1c692ccd 100644
--- a/Makefile
+++ b/Makefile
@@ -504,6 +504,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 3937da6157..845c40fca8 100644
--- a/disas.c
+++ b/disas.c
@@ -660,6 +660,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 625daeedd1..1734ad9c3a 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;
@@ -162,6 +163,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.20.1

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

[Philippe Mathieu-Daudé]

On 1/3/20 10:24 PM, Richard Henderson wrote:
> 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>

I'm fine with this patch because I don't use the s390 disas often.
For the S390 experts, my previous analysis is here:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg667954.html

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   Makefile           | 1 +
>   disas.c            | 3 +++
>   target/s390x/cpu.c | 4 ++++
>   3 files changed, 8 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 12e129ac9d..df1c692ccd 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -504,6 +504,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 3937da6157..845c40fca8 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -660,6 +660,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 625daeedd1..1734ad9c3a 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;
> @@ -162,6 +163,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)
> 

[PATCH 3/3] capstone: Add skipdata hook for s390x

[Richard Henderson]

Capstone assumes any s390x 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.

Reviewed-by: Thomas Huth <thuth@redhat.com>
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 845c40fca8..1095bad049 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.20.1

Re: [PATCH 3/3] capstone: Add skipdata hook for s390x

[Philippe Mathieu-Daudé]

On 1/3/20 10:25 PM, Richard Henderson wrote:
> Capstone assumes any s390x 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.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   disas.c | 37 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/disas.c b/disas.c
> index 845c40fca8..1095bad049 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) {
> 

Re: [PATCH 0/3] capstone: update to next

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200103212500.14384-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH 0/3] capstone: update to next
Type: series
Message-id: 20200103212500.14384-1-richard.henderson@linaro.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

fatal: unable to write new index file
warning: Clone succeeded, but checkout failed.
You can inspect what was checked out with 'git status'
and retry the checkout with 'git checkout -f HEAD'

Traceback (most recent call last):
  File "patchew-tester2/src/patchew-cli", line 531, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf, True)
  File "patchew-tester2/src/patchew-cli", line 62, in git_clone_repo
    subprocess.check_call(clone_cmd, stderr=logf, stdout=logf)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/subprocess.py", line 291, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'clone', '-q', '/home/patchew2/.cache/patchew-git-cache/httpsgithubcompatchewprojectqemu-3c8cf5a9c21ff8782164d1def7f44bd888713384', '/var/tmp/patchew-tester-tmp-1ya_3r2x/src']' returned non-zero exit status 128.



The full log is available at
http://patchew.org/logs/20200103212500.14384-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com