[PATCH v2 00/19] testing and plugin updates

[PATCH v2 00/19] testing and plugin updates

[Alex Bennée]

Hi,

I've ended up combining my accumulated testing fixes with the plugin
fixes as there is some cross-over between the two. On the testing side
I still haven't seen rcutorture trip up on my branches but the final
patch that light re-factors it needs to be reviewed. I've also added
some fixes for pauth - both ensuring they compiler and tweaking
pauth-4 to take into account the occasional authentication clashes.

Plugin wise I have cleaned up the riscv parser to use extract16 where
appropriate. We also managed to diagnose a bug in the address passing
of the memory instrumentation which only showed up under alpha. The
relevant patches have been Cc'ed to qemu-stable.

The following patches need review:

  tests/tcg: take into account expected clashes pauth-4
  tests/tcg: fix typo in configure.sh test for v8.3
  tcg: save vaddr temp for plugin usage
  tests/tcg: give debug builds a little bit longer
  tracing: only allow -trace to override -D if set
  tests/iotests: be a little more forgiving on the size test
  travis.yml: single-thread build-tcg stages
  travis.yml: Fix Travis YAML configuration warnings
  tests/rcutorture: mild documenting refactor of update thread
  tests/tcg: include a skip runner for pauth3 with plugins

Alex Bennée (13):
  tests/tcg: include a skip runner for pauth3 with plugins
  tests/rcutorture: update usage hint
  tests/rcutorture: better document locking of stats
  tests/rcutorture: mild documenting refactor of update thread
  travis.yml: single-thread build-tcg stages
  tests/iotests: be a little more forgiving on the size test
  tracing: only allow -trace to override -D if set
  docs/devel: document query handle lifetimes
  target/riscv: progressively load the instruction during decode
  tests/plugins: make howvec clean-up after itself.
  tests/tcg: give debug builds a little bit longer
  tests/tcg: fix typo in configure.sh test for v8.3
  tests/tcg: take into account expected clashes pauth-4

Chen Qun (1):
  tests/plugin: prevent uninitialized warning

Emilio G. Cota (1):
  plugins/core: add missing break in cb_to_tcg_flags

Richard Henderson (1):
  tcg: save vaddr temp for plugin usage

Thomas Huth (1):
  travis.yml: Test the s390-ccw build, too

Wainer dos Santos Moschetta (1):
  travis.yml: Fix Travis YAML configuration warnings

Yoshinori Sato (1):
  qemu/bitops.h: Add extract8 and extract16

 docs/devel/tcg-plugins.rst                | 13 +++++-
 include/qemu/bitops.h                     | 38 ++++++++++++++++
 target/riscv/instmap.h                    |  8 ++--
 plugins/core.c                            |  1 +
 target/riscv/translate.c                  | 40 +++++++++--------
 tcg/tcg-op.c                              | 23 ++++++++--
 tests/plugin/bb.c                         |  6 +--
 tests/plugin/howvec.c                     | 26 +++++++----
 tests/plugin/insn.c                       |  3 +-
 tests/rcutorture.c                        | 55 +++++++++++++++++------
 tests/tcg/aarch64/pauth-4.c               | 54 +++++++++++++++-------
 trace/control.c                           | 11 +++--
 .travis.yml                               | 23 +++++++---
 tests/qemu-iotests/214                    |  4 +-
 tests/tcg/Makefile.target                 |  4 +-
 tests/tcg/aarch64/Makefile.softmmu-target |  2 +
 tests/tcg/configure.sh                    |  2 +-
 17 files changed, 225 insertions(+), 88 deletions(-)

-- 
2.20.1

[PATCH v2 01/19] tests/tcg: include a skip runner for pauth3 with plugins

[Alex Bennée]

If we have plugins enabled we still need to have built the test to be
able to run it.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/aarch64/Makefile.softmmu-target | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target
index d2299b98b76..71f72cfbe34 100644
--- a/tests/tcg/aarch64/Makefile.softmmu-target
+++ b/tests/tcg/aarch64/Makefile.softmmu-target
@@ -70,4 +70,6 @@ pauth-3:
 	$(call skip-test, "BUILD of $@", "missing compiler support")
 run-pauth-3:
 	$(call skip-test, "RUN of pauth-3", "not built")
+run-plugin-pauth-3-with-%:
+	$(call skip-test, "RUN of pauth-3 ($*)", "not built")
 endif
-- 
2.20.1

Re: [PATCH v2 01/19] tests/tcg: include a skip runner for pauth3 with plugins

[Robert Foley]

On Thu, 13 Feb 2020 at 17:51, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> If we have plugins enabled we still need to have built the test to be
> able to run it.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Robert Foley <robert.foley@linaro.org>

[PATCH v2 02/19] tests/rcutorture: update usage hint

[Alex Bennée]

Although documented in the comments we don't display all the various
invocations we can in the usage.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/rcutorture.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/rcutorture.c b/tests/rcutorture.c
index 49311c82ea4..e8b2169e7dd 100644
--- a/tests/rcutorture.c
+++ b/tests/rcutorture.c
@@ -413,7 +413,8 @@ static void gtest_stress_10_5(void)
 
 static void usage(int argc, char *argv[])
 {
-    fprintf(stderr, "Usage: %s [nreaders [ perf | stress ] ]\n", argv[0]);
+    fprintf(stderr, "Usage: %s [nreaders [ [r|u]perf | stress [duration]]\n",
+            argv[0]);
     exit(-1);
 }
 
-- 
2.20.1

[PATCH v2 03/19] tests/rcutorture: better document locking of stats

[Alex Bennée]

This is pure code motion with no functional effect.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/rcutorture.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tests/rcutorture.c b/tests/rcutorture.c
index e8b2169e7dd..256d24ed5ba 100644
--- a/tests/rcutorture.c
+++ b/tests/rcutorture.c
@@ -65,8 +65,6 @@
 #include "qemu/rcu.h"
 #include "qemu/thread.h"
 
-long long n_reads = 0LL;
-long n_updates = 0L;
 int nthreadsrunning;
 
 #define GOFLAG_INIT 0
@@ -78,11 +76,20 @@ static volatile int goflag = GOFLAG_INIT;
 #define RCU_READ_RUN 1000
 
 #define NR_THREADS 100
-static QemuMutex counts_mutex;
 static QemuThread threads[NR_THREADS];
 static struct rcu_reader_data *data[NR_THREADS];
 static int n_threads;
 
+/*
+ * Statistical counts
+ *
+ * These are the sum of local counters at the end of a run.
+ * Updates are protected by a mutex.
+ */
+static QemuMutex counts_mutex;
+long long n_reads = 0LL;
+long n_updates = 0L;
+
 static void create_thread(void *(*func)(void *))
 {
     if (n_threads >= NR_THREADS) {
@@ -230,8 +237,9 @@ struct rcu_stress {
 struct rcu_stress rcu_stress_array[RCU_STRESS_PIPE_LEN] = { { 0 } };
 struct rcu_stress *rcu_stress_current;
 int rcu_stress_idx;
-
 int n_mberror;
+
+/* Updates protected by counts_mutex */
 long long rcu_stress_count[RCU_STRESS_PIPE_LEN + 1];
 
 
-- 
2.20.1

[PATCH v2 04/19] tests/rcutorture: mild documenting refactor of update thread

[Alex Bennée]

This is mainly to help with reasoning what the test is trying to do.
We can move rcu_stress_idx to a local variable as there is only ever
one updater thread. I've also added an assert to catch the case where
we end up updating the current structure to itself which is the only
way I can see the mberror cases we are seeing on Travis.

We shall see if the rcutorture test failures go away now.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/rcutorture.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/tests/rcutorture.c b/tests/rcutorture.c
index 256d24ed5ba..9559f13cd1f 100644
--- a/tests/rcutorture.c
+++ b/tests/rcutorture.c
@@ -236,7 +236,6 @@ struct rcu_stress {
 
 struct rcu_stress rcu_stress_array[RCU_STRESS_PIPE_LEN] = { { 0 } };
 struct rcu_stress *rcu_stress_current;
-int rcu_stress_idx;
 int n_mberror;
 
 /* Updates protected by counts_mutex */
@@ -288,29 +287,48 @@ static void *rcu_read_stress_test(void *arg)
     return NULL;
 }
 
+/*
+ * Stress Test Updater
+ *
+ * The updater cycles around updating rcu_stress_current to point at
+ * one of the rcu_stress_array_entries and resets it's pipe_count. It
+ * then increments pipe_count of all the other entries. The pipe_count
+ * will be read under an rcu_read_lock() and distribution of values
+ * calculated. The final result gives an indication of how many
+ * previously current rcu_stress entries are in flight until the RCU
+ * cycle complete.
+ */
 static void *rcu_update_stress_test(void *arg)
 {
-    int i;
-    struct rcu_stress *p;
+    int i, rcu_stress_idx = 0;
+    struct rcu_stress *cp = atomic_read(&rcu_stress_current);
 
     rcu_register_thread();
-
     *(struct rcu_reader_data **)arg = &rcu_reader;
+
     while (goflag == GOFLAG_INIT) {
         g_usleep(1000);
     }
+
     while (goflag == GOFLAG_RUN) {
-        i = rcu_stress_idx + 1;
-        if (i >= RCU_STRESS_PIPE_LEN) {
-            i = 0;
+        struct rcu_stress *p;
+        rcu_stress_idx++;
+        if (rcu_stress_idx >= RCU_STRESS_PIPE_LEN) {
+            rcu_stress_idx = 0;
         }
-        p = &rcu_stress_array[i];
+        p = &rcu_stress_array[rcu_stress_idx];
+        /* catching up with ourselves would be a bug */
+        assert(p != cp);
         p->mbtest = 0;
         smp_mb();
         p->pipe_count = 0;
         p->mbtest = 1;
         atomic_rcu_set(&rcu_stress_current, p);
-        rcu_stress_idx = i;
+        cp = p;
+        /*
+         * New RCU structure is now live, update pipe counts on old
+         * ones.
+         */
         for (i = 0; i < RCU_STRESS_PIPE_LEN; i++) {
             if (i != rcu_stress_idx) {
                 rcu_stress_array[i].pipe_count++;
-- 
2.20.1

Re: [PATCH v2 04/19] tests/rcutorture: mild documenting refactor of update thread

[Paolo Bonzini]

On 13/02/20 23:50, Alex Bennée wrote:
> This is mainly to help with reasoning what the test is trying to do.
> We can move rcu_stress_idx to a local variable as there is only ever
> one updater thread. I've also added an assert to catch the case where
> we end up updating the current structure to itself which is the only
> way I can see the mberror cases we are seeing on Travis.
> 
> We shall see if the rcutorture test failures go away now.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Yes, the failures are quire mysterious and I agree that they can only happen if:

1) p == cp in your patch below

2) the memory barrier can be overtaken by the store above it.

Even then, (2) would be unlikely because then the compiler would 
coalesce the two stores to p->mbtest.  However we could add a patch such 
as this to rule it out:

diff --git a/tests/rcutorture.c b/tests/rcutorture.c
index 9559f13..969a19a 100644
--- a/tests/rcutorture.c
+++ b/tests/rcutorture.c
@@ -260,7 +260,7 @@ static void *rcu_read_stress_test(void *arg)
     while (goflag == GOFLAG_RUN) {
         rcu_read_lock();
         p = atomic_rcu_read(&rcu_stress_current);
-        if (p->mbtest == 0) {
+        if (atomic_read(&p->mbtest) == 0) {
             n_mberror++;
         }
         rcu_read_lock();
@@ -268,7 +268,7 @@ static void *rcu_read_stress_test(void *arg)
             garbage++;
         }
         rcu_read_unlock();
-        pc = p->pipe_count;
+        pc = atomic_read(&p->pipe_count);
         rcu_read_unlock();
         if ((pc > RCU_STRESS_PIPE_LEN) || (pc < 0)) {
             pc = RCU_STRESS_PIPE_LEN;
@@ -319,10 +319,10 @@ static void *rcu_update_stress_test(void *arg)
         p = &rcu_stress_array[rcu_stress_idx];
         /* catching up with ourselves would be a bug */
         assert(p != cp);
-        p->mbtest = 0;
+        atomic_set(&p->mbtest, 0);
         smp_mb();
-        p->pipe_count = 0;
-        p->mbtest = 1;
+        atomic_set(&p->pipe_count, 0);
+        atomic_set(&p->mbtest, 1);
         atomic_rcu_set(&rcu_stress_current, p);
         cp = p;
         /*
@@ -331,7 +331,8 @@ static void *rcu_update_stress_test(void *arg)
          */
         for (i = 0; i < RCU_STRESS_PIPE_LEN; i++) {
             if (i != rcu_stress_idx) {
-                rcu_stress_array[i].pipe_count++;
+                atomic_set(&rcu_stress_array[i].pipe_count,
+			   rcu_stress_array[i].pipe_count + 1);
             }
         }
         synchronize_rcu();


Finally, the "pipe_count" naming is a bit mysterious.  The idea behind the test
is that RCU can only ever see the current or the previous version of a
struct (in this case, the current or the previous index) and a "pipe_count"
of 3 means for example that the index is 3 updates behind the current
index.  To check that the RCU invariant is preserved, the test checks that
the reader does not observe an index that is 2 updates or more behind the
current index.

I have never changed it because I took it directly from Paul McKenney's
rcutorture and I didn't want to deviate too much in order to keep Paul's
code as a reference.  But perhaps we should rename it to something more
intuitive like "age" (which is short enough that "pc" could also be
renamed to "age").

Paolo

[PATCH v2 05/19] travis.yml: Test the s390-ccw build, too

[Alex Bennée]

From: Thomas Huth <thuth@redhat.com>

Since we can now use a s390x host on Travis, we can also build and
test the s390-ccw bios images there. For this we have to make sure
that roms/SLOF is checked out, too, and then move the generated *.img
files to the right location before running the tests.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Acked-by: Cornelia Huck <cohuck@redhat.com>
Message-Id: <20200206202543.7085-1-thuth@redhat.com>
---
 .travis.yml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 58870559515..ea13e071795 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -509,6 +509,16 @@ matrix:
       env:
         - TEST_CMD="make check check-tcg V=1"
         - CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS},s390x-linux-user"
+      script:
+        - ( cd ${SRC_DIR} ; git submodule update --init roms/SLOF )
+        - BUILD_RC=0 && make -j${JOBS} || BUILD_RC=$?
+        - |
+          if [ "$BUILD_RC" -eq 0 ] ; then
+              mv pc-bios/s390-ccw/*.img pc-bios/ ;
+              ${TEST_CMD} ;
+          else
+              $(exit $BUILD_RC);
+          fi
 
     # Release builds
     # The make-release script expect a QEMU version, so our tag must start with a 'v'.
-- 
2.20.1

[PATCH v2 06/19] travis.yml: Fix Travis YAML configuration warnings

[Alex Bennée]

From: Wainer dos Santos Moschetta <wainersm@redhat.com>

This fixes the following warnings Travis has detected on the
YAML configuration:

- 'on root: missing os, using the default "linux"'
- 'on root: the key matrix is an alias for jobs, using jobs'
- 'on jobs.include.python: unexpected sequence, using the first value (3.5)'
- 'on jobs.include.python: unexpected sequence, using the first value (3.6)'

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200207210124.141119-2-wainersm@redhat.com>
---
 .travis.yml | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index ea13e071795..0612998958b 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,6 +1,7 @@
 # The current Travis default is a VM based 16.04 Xenial on GCE
 # Additional builds with specific requirements for a full VM need to
 # be added as additional matrix: entries later on
+os: linux
 dist: xenial
 language: c
 compiler:
@@ -113,7 +114,7 @@ after_script:
   - if command -v ccache ; then ccache --show-stats ; fi
 
 
-matrix:
+jobs:
   include:
     - name: "GCC static (user)"
       env:
@@ -297,8 +298,7 @@ matrix:
         - CONFIG="--target-list=x86_64-softmmu"
         - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
       language: python
-      python:
-        - "3.5"
+      python: 3.5
 
 
     - name: "GCC Python 3.6 (x86_64-softmmu)"
@@ -306,8 +306,7 @@ matrix:
         - CONFIG="--target-list=x86_64-softmmu"
         - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
       language: python
-      python:
-        - "3.6"
+      python: 3.6
 
 
     # Acceptance (Functional) tests
-- 
2.20.1

[PATCH v2 07/19] travis.yml: single-thread build-tcg stages

[Alex Bennée]

This still seems to be a problem for Travis.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 .travis.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 0612998958b..f4020dcc6c8 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -400,7 +400,7 @@ jobs:
     - name: "GCC check-tcg (some-softmmu)"
       env:
         - CONFIG="--enable-debug-tcg --target-list=xtensa-softmmu,arm-softmmu,aarch64-softmmu,alpha-softmmu"
-        - TEST_BUILD_CMD="make -j${JOBS} build-tcg"
+        - TEST_BUILD_CMD="make build-tcg"
         - TEST_CMD="make check-tcg"
         - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-debug-tcg"
 
@@ -409,7 +409,7 @@ jobs:
     - name: "GCC plugins check-tcg (some-softmmu)"
       env:
         - CONFIG="--enable-plugins --enable-debug-tcg --target-list=xtensa-softmmu,arm-softmmu,aarch64-softmmu,alpha-softmmu"
-        - TEST_BUILD_CMD="make -j${JOBS} build-tcg"
+        - TEST_BUILD_CMD="make build-tcg"
         - TEST_CMD="make check-tcg"
         - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-debug-tcg"
 
-- 
2.20.1

Re: [PATCH v2 07/19] travis.yml: single-thread build-tcg stages

[Philippe Mathieu-Daudé]

On Thu, Feb 13, 2020 at 11:51 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> This still seems to be a problem for Travis.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  .travis.yml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/.travis.yml b/.travis.yml
> index 0612998958b..f4020dcc6c8 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -400,7 +400,7 @@ jobs:
>      - name: "GCC check-tcg (some-softmmu)"
>        env:
>          - CONFIG="--enable-debug-tcg --target-list=xtensa-softmmu,arm-softmmu,aarch64-softmmu,alpha-softmmu"
> -        - TEST_BUILD_CMD="make -j${JOBS} build-tcg"
> +        - TEST_BUILD_CMD="make build-tcg"

Can you explicit -j1 instead? This is self-explanatory.

>          - TEST_CMD="make check-tcg"
>          - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-debug-tcg"
>
> @@ -409,7 +409,7 @@ jobs:
>      - name: "GCC plugins check-tcg (some-softmmu)"
>        env:
>          - CONFIG="--enable-plugins --enable-debug-tcg --target-list=xtensa-softmmu,arm-softmmu,aarch64-softmmu,alpha-softmmu"
> -        - TEST_BUILD_CMD="make -j${JOBS} build-tcg"
> +        - TEST_BUILD_CMD="make build-tcg"

With -j1 in both places:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>          - TEST_CMD="make check-tcg"
>          - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-debug-tcg"
>
> --
> 2.20.1
>

[PATCH v2 08/19] tests/iotests: be a little more forgiving on the size test

[Alex Bennée]

At least on ZFS this was failing as 512 was less than or equal to 512.
I suspect the reason is additional compression done by ZFS and however
qemu-img gets the actual size.

Loosen the criteria to make sure after is not bigger than before and
also dump the values in the report.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qemu-iotests/214 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
index 3500e0c47a2..6d1324cd157 100755
--- a/tests/qemu-iotests/214
+++ b/tests/qemu-iotests/214
@@ -125,9 +125,9 @@ $QEMU_IO -c "write -P 0xcc $offset $data_size" "json:{\
 sizeB=$($QEMU_IMG info --output=json "$TEST_IMG" |
         sed -n '/"actual-size":/ s/[^0-9]//gp')
 
-if [ $sizeA -le $sizeB ]
+if [ $sizeA -lt $sizeB ]
 then
-    echo "Compression ERROR"
+    echo "Compression ERROR ($sizeA vs $sizeB)"
 fi
 
 $QEMU_IMG check --output=json "$TEST_IMG" |
-- 
2.20.1

[PATCH v2 09/19] tracing: only allow -trace to override -D if set

[Alex Bennée]

Otherwise any -D settings the user may have made get ignored.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 trace/control.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/trace/control.c b/trace/control.c
index 6c775e68eba..2ffe0008184 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -226,10 +226,15 @@ void trace_init_file(const char *file)
 #ifdef CONFIG_TRACE_SIMPLE
     st_set_trace_file(file);
 #elif defined CONFIG_TRACE_LOG
-    /* If both the simple and the log backends are enabled, "--trace file"
-     * only applies to the simple backend; use "-D" for the log backend.
+    /*
+     * If both the simple and the log backends are enabled, "--trace file"
+     * only applies to the simple backend; use "-D" for the log
+     * backend. However we should only override -D if we actually have
+     * something to override it with.
      */
-    qemu_set_log_filename(file, &error_fatal);
+    if (file) {
+        qemu_set_log_filename(file, &error_fatal);
+    }
 #else
     if (file) {
         fprintf(stderr, "error: --trace file=...: "
-- 
2.20.1

Re: [PATCH v2 09/19] tracing: only allow -trace to override -D if set

[Robert Foley]

On Thu, 13 Feb 2020 at 17:51, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Otherwise any -D settings the user may have made get ignored.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Robert Foley <robert.foley@linaro.org>

[PATCH v2 10/19] docs/devel: document query handle lifetimes

[Alex Bennée]

I forgot to document the lifetime of handles in the developer
documentation. Do so now.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Robert Foley <robert.foley@linaro.org>
Reviewed-by: Robert Foley <robert.foley@linaro.org>
---
 docs/devel/tcg-plugins.rst | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
index 718eef00f22..a05990906cc 100644
--- a/docs/devel/tcg-plugins.rst
+++ b/docs/devel/tcg-plugins.rst
@@ -51,8 +51,17 @@ about how QEMU's translation works to the plugins. While there are
 conceptions such as translation time and translation blocks the
 details are opaque to plugins. The plugin is able to query select
 details of instructions and system configuration only through the
-exported *qemu_plugin* functions. The types used to describe
-instructions and events are opaque to the plugins themselves.
+exported *qemu_plugin* functions.
+
+Query Handle Lifetime
+---------------------
+
+Each callback provides an opaque anonymous information handle which
+can usually be further queried to find out information about a
+translation, instruction or operation. The handles themselves are only
+valid during the lifetime of the callback so it is important that any
+information that is needed is extracted during the callback and saved
+by the plugin.
 
 Usage
 =====
-- 
2.20.1

[PATCH v2 11/19] plugins/core: add missing break in cb_to_tcg_flags

[Alex Bennée]

From: "Emilio G. Cota" <cota@braap.org>

Reported-by: Robert Henry <robhenry@microsoft.com>
Signed-off-by: Emilio G. Cota <cota@braap.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200105072940.32204-1-cota@braap.org>
Cc: qemu-stable@nongnu.org
---
 plugins/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/plugins/core.c b/plugins/core.c
index 9e1b9e7a915..ed863011baf 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -286,6 +286,7 @@ static inline uint32_t cb_to_tcg_flags(enum qemu_plugin_cb_flags flags)
     switch (flags) {
     case QEMU_PLUGIN_CB_RW_REGS:
         ret = 0;
+        break;
     case QEMU_PLUGIN_CB_R_REGS:
         ret = TCG_CALL_NO_WG;
         break;
-- 
2.20.1

Re: [PATCH v2 11/19] plugins/core: add missing break in cb_to_tcg_flags

[Philippe Mathieu-Daudé]

On 2/13/20 11:51 PM, Alex Bennée wrote:
> From: "Emilio G. Cota" <cota@braap.org>
> 
> Reported-by: Robert Henry <robhenry@microsoft.com>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-Id: <20200105072940.32204-1-cota@braap.org>

Fixes: 54cb65d8588

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> Cc: qemu-stable@nongnu.org
> ---
>  plugins/core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/plugins/core.c b/plugins/core.c
> index 9e1b9e7a915..ed863011baf 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -286,6 +286,7 @@ static inline uint32_t cb_to_tcg_flags(enum qemu_plugin_cb_flags flags)
>      switch (flags) {
>      case QEMU_PLUGIN_CB_RW_REGS:
>          ret = 0;
> +        break;
>      case QEMU_PLUGIN_CB_R_REGS:
>          ret = TCG_CALL_NO_WG;
>          break;
> 

[PATCH v2 12/19] tests/plugin: prevent uninitialized warning

[Alex Bennée]

From: Chen Qun <kuhn.chenqun@huawei.com>

According to the glibc function requirements, we need initialise
 the variable. Otherwise there will be compilation warnings:

glib-autocleanups.h:28:3: warning: ‘out’ may be
used uninitialized in this function [-Wmaybe-uninitialized]
   g_free (*pp);
   ^~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20200206093238.203984-1-kuhn.chenqun@huawei.com>
[AJB: uses Thomas's single line allocation]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/plugin/bb.c   | 6 +++---
 tests/plugin/insn.c | 3 +--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
index f30bea08dcc..df19fd359df 100644
--- a/tests/plugin/bb.c
+++ b/tests/plugin/bb.c
@@ -22,9 +22,9 @@ static bool do_inline;
 
 static void plugin_exit(qemu_plugin_id_t id, void *p)
 {
-    g_autofree gchar *out;
-    out = g_strdup_printf("bb's: %" PRIu64", insns: %" PRIu64 "\n",
-                          bb_count, insn_count);
+    g_autofree gchar *out = g_strdup_printf(
+        "bb's: %" PRIu64", insns: %" PRIu64 "\n",
+        bb_count, insn_count);
     qemu_plugin_outs(out);
 }
 
diff --git a/tests/plugin/insn.c b/tests/plugin/insn.c
index 0a8f5a0000e..a9a6e412373 100644
--- a/tests/plugin/insn.c
+++ b/tests/plugin/insn.c
@@ -44,8 +44,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
 
 static void plugin_exit(qemu_plugin_id_t id, void *p)
 {
-    g_autofree gchar *out;
-    out = g_strdup_printf("insns: %" PRIu64 "\n", insn_count);
+    g_autofree gchar *out = g_strdup_printf("insns: %" PRIu64 "\n", insn_count);
     qemu_plugin_outs(out);
 }
 
-- 
2.20.1

[PATCH v2 13/19] qemu/bitops.h: Add extract8 and extract16

[Alex Bennée]

From: Yoshinori Sato <ysato@users.sourceforge.jp>

Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190607091116.49044-10-ysato@users.sourceforge.jp>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200212130311.127515-3-ysato@users.sourceforge.jp>
---
 include/qemu/bitops.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 02c1ce6a5d4..f55ce8b320b 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -301,6 +301,44 @@ static inline uint32_t extract32(uint32_t value, int start, int length)
     return (value >> start) & (~0U >> (32 - length));
 }
 
+/**
+ * extract8:
+ * @value: the value to extract the bit field from
+ * @start: the lowest bit in the bit field (numbered from 0)
+ * @length: the length of the bit field
+ *
+ * Extract from the 8 bit input @value the bit field specified by the
+ * @start and @length parameters, and return it. The bit field must
+ * lie entirely within the 8 bit word. It is valid to request that
+ * all 8 bits are returned (ie @length 8 and @start 0).
+ *
+ * Returns: the value of the bit field extracted from the input value.
+ */
+static inline uint8_t extract8(uint8_t value, int start, int length)
+{
+    assert(start >= 0 && length > 0 && length <= 8 - start);
+    return extract32(value, start, length);
+}
+
+/**
+ * extract16:
+ * @value: the value to extract the bit field from
+ * @start: the lowest bit in the bit field (numbered from 0)
+ * @length: the length of the bit field
+ *
+ * Extract from the 16 bit input @value the bit field specified by the
+ * @start and @length parameters, and return it. The bit field must
+ * lie entirely within the 16 bit word. It is valid to request that
+ * all 16 bits are returned (ie @length 16 and @start 0).
+ *
+ * Returns: the value of the bit field extracted from the input value.
+ */
+static inline uint16_t extract16(uint16_t value, int start, int length)
+{
+    assert(start >= 0 && length > 0 && length <= 16 - start);
+    return extract32(value, start, length);
+}
+
 /**
  * extract64:
  * @value: the value to extract the bit field from
-- 
2.20.1

[PATCH v2 14/19] target/riscv: progressively load the instruction during decode

[Alex Bennée]

The plugin system would throw up a harmless warning when it detected
that a disassembly of an instruction didn't use all it's bytes. Fix
the riscv decoder to only load the instruction bytes it needs as it
needs them.

This drops opcode from the ctx in favour if passing the appropriately
sized opcode down a few levels of the decode.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---
v2
  - use extract16 for uint16_t opcodes

squash! target/riscv: progressively load the instruction during decode
---
 target/riscv/instmap.h   |  8 ++++----
 target/riscv/translate.c | 40 +++++++++++++++++++++-------------------
 2 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
index f8ad7d60fd5..40b6d2b64de 100644
--- a/target/riscv/instmap.h
+++ b/target/riscv/instmap.h
@@ -344,8 +344,8 @@ enum {
 #define GET_C_LW_IMM(inst)          ((extract32(inst, 6, 1) << 2) \
                                     | (extract32(inst, 10, 3) << 3) \
                                     | (extract32(inst, 5, 1) << 6))
-#define GET_C_LD_IMM(inst)          ((extract32(inst, 10, 3) << 3) \
-                                    | (extract32(inst, 5, 2) << 6))
+#define GET_C_LD_IMM(inst)          ((extract16(inst, 10, 3) << 3) \
+                                    | (extract16(inst, 5, 2) << 6))
 #define GET_C_J_IMM(inst)           ((extract32(inst, 3, 3) << 1) \
                                     | (extract32(inst, 11, 1) << 4) \
                                     | (extract32(inst, 2, 1) << 5) \
@@ -363,7 +363,7 @@ enum {
 #define GET_C_RD(inst)              GET_RD(inst)
 #define GET_C_RS1(inst)             GET_RD(inst)
 #define GET_C_RS2(inst)             extract32(inst, 2, 5)
-#define GET_C_RS1S(inst)            (8 + extract32(inst, 7, 3))
-#define GET_C_RS2S(inst)            (8 + extract32(inst, 2, 3))
+#define GET_C_RS1S(inst)            (8 + extract16(inst, 7, 3))
+#define GET_C_RS2S(inst)            (8 + extract16(inst, 2, 3))
 
 #endif
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 14dc71156be..d5de7f468a7 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -44,7 +44,6 @@ typedef struct DisasContext {
     /* pc_succ_insn points to the instruction following base.pc_next */
     target_ulong pc_succ_insn;
     target_ulong priv_ver;
-    uint32_t opcode;
     uint32_t mstatus_fs;
     uint32_t misa;
     uint32_t mem_idx;
@@ -492,45 +491,45 @@ static void gen_set_rm(DisasContext *ctx, int rm)
     tcg_temp_free_i32(t0);
 }
 
-static void decode_RV32_64C0(DisasContext *ctx)
+static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
 {
-    uint8_t funct3 = extract32(ctx->opcode, 13, 3);
-    uint8_t rd_rs2 = GET_C_RS2S(ctx->opcode);
-    uint8_t rs1s = GET_C_RS1S(ctx->opcode);
+    uint8_t funct3 = extract16(opcode, 13, 3);
+    uint8_t rd_rs2 = GET_C_RS2S(opcode);
+    uint8_t rs1s = GET_C_RS1S(opcode);
 
     switch (funct3) {
     case 3:
 #if defined(TARGET_RISCV64)
         /* C.LD(RV64/128) -> ld rd', offset[7:3](rs1')*/
         gen_load_c(ctx, OPC_RISC_LD, rd_rs2, rs1s,
-                 GET_C_LD_IMM(ctx->opcode));
+                 GET_C_LD_IMM(opcode));
 #else
         /* C.FLW (RV32) -> flw rd', offset[6:2](rs1')*/
         gen_fp_load(ctx, OPC_RISC_FLW, rd_rs2, rs1s,
-                    GET_C_LW_IMM(ctx->opcode));
+                    GET_C_LW_IMM(opcode));
 #endif
         break;
     case 7:
 #if defined(TARGET_RISCV64)
         /* C.SD (RV64/128) -> sd rs2', offset[7:3](rs1')*/
         gen_store_c(ctx, OPC_RISC_SD, rs1s, rd_rs2,
-                  GET_C_LD_IMM(ctx->opcode));
+                  GET_C_LD_IMM(opcode));
 #else
         /* C.FSW (RV32) -> fsw rs2', offset[6:2](rs1')*/
         gen_fp_store(ctx, OPC_RISC_FSW, rs1s, rd_rs2,
-                     GET_C_LW_IMM(ctx->opcode));
+                     GET_C_LW_IMM(opcode));
 #endif
         break;
     }
 }
 
-static void decode_RV32_64C(DisasContext *ctx)
+static void decode_RV32_64C(DisasContext *ctx, uint16_t opcode)
 {
-    uint8_t op = extract32(ctx->opcode, 0, 2);
+    uint8_t op = extract16(opcode, 0, 2);
 
     switch (op) {
     case 0:
-        decode_RV32_64C0(ctx);
+        decode_RV32_64C0(ctx, opcode);
         break;
     }
 }
@@ -709,22 +708,25 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
 /* Include the auto-generated decoder for 16 bit insn */
 #include "decode_insn16.inc.c"
 
-static void decode_opc(DisasContext *ctx)
+static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
 {
     /* check for compressed insn */
-    if (extract32(ctx->opcode, 0, 2) != 3) {
+    if (extract16(opcode, 0, 2) != 3) {
         if (!has_ext(ctx, RVC)) {
             gen_exception_illegal(ctx);
         } else {
             ctx->pc_succ_insn = ctx->base.pc_next + 2;
-            if (!decode_insn16(ctx, ctx->opcode)) {
+            if (!decode_insn16(ctx, opcode)) {
                 /* fall back to old decoder */
-                decode_RV32_64C(ctx);
+                decode_RV32_64C(ctx, opcode);
             }
         }
     } else {
+        uint32_t opcode32 = opcode;
+        opcode32 = deposit32(opcode32, 16, 16,
+                             translator_lduw(env, ctx->base.pc_next + 2));
         ctx->pc_succ_insn = ctx->base.pc_next + 4;
-        if (!decode_insn32(ctx, ctx->opcode)) {
+        if (!decode_insn32(ctx, opcode32)) {
             gen_exception_illegal(ctx);
         }
     }
@@ -776,9 +778,9 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPURISCVState *env = cpu->env_ptr;
+    uint16_t opcode16 = translator_lduw(env, ctx->base.pc_next);
 
-    ctx->opcode = translator_ldl(env, ctx->base.pc_next);
-    decode_opc(ctx);
+    decode_opc(env, ctx, opcode16);
     ctx->base.pc_next = ctx->pc_succ_insn;
 
     if (ctx->base.is_jmp == DISAS_NEXT) {
-- 
2.20.1

Re: [PATCH v2 14/19] target/riscv: progressively load the instruction during decode

[Alistair Francis]

On Thu, Feb 13, 2020 at 3:08 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The plugin system would throw up a harmless warning when it detected
> that a disassembly of an instruction didn't use all it's bytes. Fix
> the riscv decoder to only load the instruction bytes it needs as it
> needs them.
>
> This drops opcode from the ctx in favour if passing the appropriately
> sized opcode down a few levels of the decode.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

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

Alistair

>
> ---
> v2
>   - use extract16 for uint16_t opcodes
>
> squash! target/riscv: progressively load the instruction during decode
> ---
>  target/riscv/instmap.h   |  8 ++++----
>  target/riscv/translate.c | 40 +++++++++++++++++++++-------------------
>  2 files changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
> index f8ad7d60fd5..40b6d2b64de 100644
> --- a/target/riscv/instmap.h
> +++ b/target/riscv/instmap.h
> @@ -344,8 +344,8 @@ enum {
>  #define GET_C_LW_IMM(inst)          ((extract32(inst, 6, 1) << 2) \
>                                      | (extract32(inst, 10, 3) << 3) \
>                                      | (extract32(inst, 5, 1) << 6))
> -#define GET_C_LD_IMM(inst)          ((extract32(inst, 10, 3) << 3) \
> -                                    | (extract32(inst, 5, 2) << 6))
> +#define GET_C_LD_IMM(inst)          ((extract16(inst, 10, 3) << 3) \
> +                                    | (extract16(inst, 5, 2) << 6))
>  #define GET_C_J_IMM(inst)           ((extract32(inst, 3, 3) << 1) \
>                                      | (extract32(inst, 11, 1) << 4) \
>                                      | (extract32(inst, 2, 1) << 5) \
> @@ -363,7 +363,7 @@ enum {
>  #define GET_C_RD(inst)              GET_RD(inst)
>  #define GET_C_RS1(inst)             GET_RD(inst)
>  #define GET_C_RS2(inst)             extract32(inst, 2, 5)
> -#define GET_C_RS1S(inst)            (8 + extract32(inst, 7, 3))
> -#define GET_C_RS2S(inst)            (8 + extract32(inst, 2, 3))
> +#define GET_C_RS1S(inst)            (8 + extract16(inst, 7, 3))
> +#define GET_C_RS2S(inst)            (8 + extract16(inst, 2, 3))
>
>  #endif
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 14dc71156be..d5de7f468a7 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -44,7 +44,6 @@ typedef struct DisasContext {
>      /* pc_succ_insn points to the instruction following base.pc_next */
>      target_ulong pc_succ_insn;
>      target_ulong priv_ver;
> -    uint32_t opcode;
>      uint32_t mstatus_fs;
>      uint32_t misa;
>      uint32_t mem_idx;
> @@ -492,45 +491,45 @@ static void gen_set_rm(DisasContext *ctx, int rm)
>      tcg_temp_free_i32(t0);
>  }
>
> -static void decode_RV32_64C0(DisasContext *ctx)
> +static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
>  {
> -    uint8_t funct3 = extract32(ctx->opcode, 13, 3);
> -    uint8_t rd_rs2 = GET_C_RS2S(ctx->opcode);
> -    uint8_t rs1s = GET_C_RS1S(ctx->opcode);
> +    uint8_t funct3 = extract16(opcode, 13, 3);
> +    uint8_t rd_rs2 = GET_C_RS2S(opcode);
> +    uint8_t rs1s = GET_C_RS1S(opcode);
>
>      switch (funct3) {
>      case 3:
>  #if defined(TARGET_RISCV64)
>          /* C.LD(RV64/128) -> ld rd', offset[7:3](rs1')*/
>          gen_load_c(ctx, OPC_RISC_LD, rd_rs2, rs1s,
> -                 GET_C_LD_IMM(ctx->opcode));
> +                 GET_C_LD_IMM(opcode));
>  #else
>          /* C.FLW (RV32) -> flw rd', offset[6:2](rs1')*/
>          gen_fp_load(ctx, OPC_RISC_FLW, rd_rs2, rs1s,
> -                    GET_C_LW_IMM(ctx->opcode));
> +                    GET_C_LW_IMM(opcode));
>  #endif
>          break;
>      case 7:
>  #if defined(TARGET_RISCV64)
>          /* C.SD (RV64/128) -> sd rs2', offset[7:3](rs1')*/
>          gen_store_c(ctx, OPC_RISC_SD, rs1s, rd_rs2,
> -                  GET_C_LD_IMM(ctx->opcode));
> +                  GET_C_LD_IMM(opcode));
>  #else
>          /* C.FSW (RV32) -> fsw rs2', offset[6:2](rs1')*/
>          gen_fp_store(ctx, OPC_RISC_FSW, rs1s, rd_rs2,
> -                     GET_C_LW_IMM(ctx->opcode));
> +                     GET_C_LW_IMM(opcode));
>  #endif
>          break;
>      }
>  }
>
> -static void decode_RV32_64C(DisasContext *ctx)
> +static void decode_RV32_64C(DisasContext *ctx, uint16_t opcode)
>  {
> -    uint8_t op = extract32(ctx->opcode, 0, 2);
> +    uint8_t op = extract16(opcode, 0, 2);
>
>      switch (op) {
>      case 0:
> -        decode_RV32_64C0(ctx);
> +        decode_RV32_64C0(ctx, opcode);
>          break;
>      }
>  }
> @@ -709,22 +708,25 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
>  /* Include the auto-generated decoder for 16 bit insn */
>  #include "decode_insn16.inc.c"
>
> -static void decode_opc(DisasContext *ctx)
> +static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
>  {
>      /* check for compressed insn */
> -    if (extract32(ctx->opcode, 0, 2) != 3) {
> +    if (extract16(opcode, 0, 2) != 3) {
>          if (!has_ext(ctx, RVC)) {
>              gen_exception_illegal(ctx);
>          } else {
>              ctx->pc_succ_insn = ctx->base.pc_next + 2;
> -            if (!decode_insn16(ctx, ctx->opcode)) {
> +            if (!decode_insn16(ctx, opcode)) {
>                  /* fall back to old decoder */
> -                decode_RV32_64C(ctx);
> +                decode_RV32_64C(ctx, opcode);
>              }
>          }
>      } else {
> +        uint32_t opcode32 = opcode;
> +        opcode32 = deposit32(opcode32, 16, 16,
> +                             translator_lduw(env, ctx->base.pc_next + 2));
>          ctx->pc_succ_insn = ctx->base.pc_next + 4;
> -        if (!decode_insn32(ctx, ctx->opcode)) {
> +        if (!decode_insn32(ctx, opcode32)) {
>              gen_exception_illegal(ctx);
>          }
>      }
> @@ -776,9 +778,9 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>  {
>      DisasContext *ctx = container_of(dcbase, DisasContext, base);
>      CPURISCVState *env = cpu->env_ptr;
> +    uint16_t opcode16 = translator_lduw(env, ctx->base.pc_next);
>
> -    ctx->opcode = translator_ldl(env, ctx->base.pc_next);
> -    decode_opc(ctx);
> +    decode_opc(env, ctx, opcode16);
>      ctx->base.pc_next = ctx->pc_succ_insn;
>
>      if (ctx->base.is_jmp == DISAS_NEXT) {
> --
> 2.20.1
>
>

Re: [PATCH v2 14/19] target/riscv: progressively load the instruction during decode

[Robert Foley]

On Thu, 13 Feb 2020 at 18:00, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The plugin system would throw up a harmless warning when it detected
> that a disassembly of an instruction didn't use all it's bytes. Fix
> the riscv decoder to only load the instruction bytes it needs as it
> needs them.
>
> This drops opcode from the ctx in favour if passing the appropriately
> sized opcode down a few levels of the decode.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Robert Foley <robert.foley@linaro.org>

[PATCH v2 15/19] tests/plugins: make howvec clean-up after itself.

[Alex Bennée]

TCG plugins are responsible for their own memory usage and although
the plugin_exit is tied to the end of execution in this case it is
still poor practice. Ensure we delete the hash table and related data
when we are done to be a good plugin citizen.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Robert Foley <robert.foley@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---
v2
  - re-use counts for g_list_sort() as it modifies list
  - drop it list

squash! tests/plugins: make howvec clean-up after itself.
---
 tests/plugin/howvec.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/tests/plugin/howvec.c b/tests/plugin/howvec.c
index 4ca555e1239..3b9a6939f23 100644
--- a/tests/plugin/howvec.c
+++ b/tests/plugin/howvec.c
@@ -163,6 +163,13 @@ static gint cmp_exec_count(gconstpointer a, gconstpointer b)
     return ea->count > eb->count ? -1 : 1;
 }
 
+static void free_record(gpointer data)
+{
+    InsnExecCount *rec = (InsnExecCount *) data;
+    g_free(rec->insn);
+    g_free(rec);
+}
+
 static void plugin_exit(qemu_plugin_id_t id, void *p)
 {
     g_autoptr(GString) report = g_string_new("Instruction Classes:\n");
@@ -195,30 +202,31 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
 
     counts = g_hash_table_get_values(insns);
     if (counts && g_list_next(counts)) {
-        GList *it;
-
         g_string_append_printf(report,"Individual Instructions:\n");
+        counts = g_list_sort(counts, cmp_exec_count);
 
-        it = g_list_sort(counts, cmp_exec_count);
-
-        for (i = 0; i < limit && it->next; i++, it = it->next) {
-            InsnExecCount *rec = (InsnExecCount *) it->data;
-            g_string_append_printf(report, "Instr: %-24s\t(%ld hits)\t(op=%#08x/%s)\n",
+        for (i = 0; i < limit && g_list_next(counts);
+             i++, counts = g_list_next(counts)) {
+            InsnExecCount *rec = (InsnExecCount *) counts->data;
+            g_string_append_printf(report,
+                                   "Instr: %-24s\t(%ld hits)\t(op=%#08x/%s)\n",
                                    rec->insn,
                                    rec->count,
                                    rec->opcode,
                                    rec->class ?
                                    rec->class->class : "un-categorised");
         }
-        g_list_free(it);
+        g_list_free(counts);
     }
 
+    g_hash_table_destroy(insns);
+
     qemu_plugin_outs(report->str);
 }
 
 static void plugin_init(void)
 {
-    insns = g_hash_table_new(NULL, g_direct_equal);
+    insns = g_hash_table_new_full(NULL, g_direct_equal, NULL, &free_record);
 }
 
 static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata)
-- 
2.20.1

[PATCH v2 16/19] tests/tcg: give debug builds a little bit longer

[Alex Bennée]

When combined with heavy plugins we occasionally hit the timeouts.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/Makefile.target | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
index 3c7421a356e..b3cff3cad1a 100644
--- a/tests/tcg/Makefile.target
+++ b/tests/tcg/Makefile.target
@@ -79,7 +79,7 @@ QEMU_OPTS=
 
 # If TCG debugging is enabled things are a lot slower
 ifeq ($(CONFIG_DEBUG_TCG),y)
-TIMEOUT=45
+TIMEOUT=60
 else
 TIMEOUT=15
 endif
@@ -137,7 +137,7 @@ PLUGINS=$(notdir $(wildcard $(PLUGIN_DIR)/*.so))
 $(foreach p,$(PLUGINS), \
 	$(foreach t,$(TESTS),\
 		$(eval run-plugin-$(t)-with-$(p): $t $p) \
-		$(eval run-plugin-$(t)-with-$(p): TIMEOUT=30) \
+		$(eval run-plugin-$(t)-with-$(p): TIMEOUT=60) \
 		$(eval RUN_TESTS+=run-plugin-$(t)-with-$(p))))
 endif
 
-- 
2.20.1

Re: [PATCH v2 16/19] tests/tcg: give debug builds a little bit longer

[Philippe Mathieu-Daudé]

On 2/13/20 11:51 PM, Alex Bennée wrote:
> When combined with heavy plugins we occasionally hit the timeouts.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/tcg/Makefile.target | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
> index 3c7421a356e..b3cff3cad1a 100644
> --- a/tests/tcg/Makefile.target
> +++ b/tests/tcg/Makefile.target
> @@ -79,7 +79,7 @@ QEMU_OPTS=
>  
>  # If TCG debugging is enabled things are a lot slower
>  ifeq ($(CONFIG_DEBUG_TCG),y)
> -TIMEOUT=45
> +TIMEOUT=60
>  else
>  TIMEOUT=15
>  endif
> @@ -137,7 +137,7 @@ PLUGINS=$(notdir $(wildcard $(PLUGIN_DIR)/*.so))
>  $(foreach p,$(PLUGINS), \
>  	$(foreach t,$(TESTS),\
>  		$(eval run-plugin-$(t)-with-$(p): $t $p) \
> -		$(eval run-plugin-$(t)-with-$(p): TIMEOUT=30) \
> +		$(eval run-plugin-$(t)-with-$(p): TIMEOUT=60) \
>  		$(eval RUN_TESTS+=run-plugin-$(t)-with-$(p))))
>  endif
>  
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v2 17/19] tcg: save vaddr temp for plugin usage

[Alex Bennée]

From: Richard Henderson <richard.henderson@linaro.org>

While do_gen_mem_cb does copy (via extu_tl_i64) vaddr into a new temp
this won't help if the vaddr temp gets clobbered by the actual
load/store op. To avoid this clobbering we explicitly copy vaddr
before the op to ensure it is live my the time we do the
instrumentation.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: qemu-stable@nongnu.org
---
 tcg/tcg-op.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 7d782002e3f..e2e25ebf7db 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -2794,13 +2794,26 @@ static void tcg_gen_req_mo(TCGBar type)
     }
 }
 
+static inline TCGv plugin_prep_mem_callbacks(TCGv vaddr)
+{
+#ifdef CONFIG_PLUGIN
+    if (tcg_ctx->plugin_insn != NULL) {
+        /* Save a copy of the vaddr for use after a load.  */
+        TCGv temp = tcg_temp_new();
+        tcg_gen_mov_tl(temp, vaddr);
+        return temp;
+    }
+#endif
+    return vaddr;
+}
+
 static inline void plugin_gen_mem_callbacks(TCGv vaddr, uint16_t info)
 {
 #ifdef CONFIG_PLUGIN
-    if (tcg_ctx->plugin_insn == NULL) {
-        return;
+    if (tcg_ctx->plugin_insn != NULL) {
+        plugin_gen_empty_mem_callback(vaddr, info);
+        tcg_temp_free(vaddr);
     }
-    plugin_gen_empty_mem_callback(vaddr, info);
 #endif
 }
 
@@ -2822,6 +2835,7 @@ void tcg_gen_qemu_ld_i32(TCGv_i32 val, TCGv addr, TCGArg idx, MemOp memop)
         }
     }
 
+    addr = plugin_prep_mem_callbacks(addr);
     gen_ldst_i32(INDEX_op_qemu_ld_i32, val, addr, memop, idx);
     plugin_gen_mem_callbacks(addr, info);
 
@@ -2868,6 +2882,7 @@ void tcg_gen_qemu_st_i32(TCGv_i32 val, TCGv addr, TCGArg idx, MemOp memop)
         memop &= ~MO_BSWAP;
     }
 
+    addr = plugin_prep_mem_callbacks(addr);
     gen_ldst_i32(INDEX_op_qemu_st_i32, val, addr, memop, idx);
     plugin_gen_mem_callbacks(addr, info);
 
@@ -2905,6 +2920,7 @@ void tcg_gen_qemu_ld_i64(TCGv_i64 val, TCGv addr, TCGArg idx, MemOp memop)
         }
     }
 
+    addr = plugin_prep_mem_callbacks(addr);
     gen_ldst_i64(INDEX_op_qemu_ld_i64, val, addr, memop, idx);
     plugin_gen_mem_callbacks(addr, info);
 
@@ -2967,6 +2983,7 @@ void tcg_gen_qemu_st_i64(TCGv_i64 val, TCGv addr, TCGArg idx, MemOp memop)
         memop &= ~MO_BSWAP;
     }
 
+    addr = plugin_prep_mem_callbacks(addr);
     gen_ldst_i64(INDEX_op_qemu_st_i64, val, addr, memop, idx);
     plugin_gen_mem_callbacks(addr, info);
 
-- 
2.20.1

Re: [PATCH v2 17/19] tcg: save vaddr temp for plugin usage

[Richard Henderson]

On 2/13/20 2:51 PM, Alex Bennée wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> While do_gen_mem_cb does copy (via extu_tl_i64) vaddr into a new temp
> this won't help if the vaddr temp gets clobbered by the actual
> load/store op. To avoid this clobbering we explicitly copy vaddr
> before the op to ensure it is live my the time we do the
> instrumentation.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>  tcg/tcg-op.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)

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

r~

Re: [PATCH v2 17/19] tcg: save vaddr temp for plugin usage

[Emilio G. Cota]

On Thu, Feb 13, 2020 at 22:51:07 +0000, Alex Bennée wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> While do_gen_mem_cb does copy (via extu_tl_i64) vaddr into a new temp
> this won't help if the vaddr temp gets clobbered by the actual
> load/store op. To avoid this clobbering we explicitly copy vaddr
> before the op to ensure it is live my the time we do the
> instrumentation.

s/my the time/by the time/

Reviewed-by: Emilio G. Cota <cota@braap.org>

		E.

[PATCH v2 18/19] tests/tcg: fix typo in configure.sh test for v8.3

[Alex Bennée]

Although most people use the docker images this can trip up on
developer systems with actual valid cross-compilers!

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/configure.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/tcg/configure.sh b/tests/tcg/configure.sh
index 9eb6ba3b7ea..eaaaff6233a 100755
--- a/tests/tcg/configure.sh
+++ b/tests/tcg/configure.sh
@@ -228,7 +228,7 @@ for target in $target_list; do
                 echo "CROSS_CC_HAS_SVE=y" >> $config_target_mak
             fi
             if do_compiler "$target_compiler" $target_compiler_cflags \
-               -march=-march=armv8.3-a -o $TMPE $TMPC; then
+               -march=armv8.3-a -o $TMPE $TMPC; then
                 echo "CROSS_CC_HAS_ARMV8_3=y" >> $config_target_mak
             fi
         ;;
-- 
2.20.1

Re: [PATCH v2 18/19] tests/tcg: fix typo in configure.sh test for v8.3

[Philippe Mathieu-Daudé]

On 2/13/20 11:51 PM, Alex Bennée wrote:
> Although most people use the docker images this can trip up on
> developer systems with actual valid cross-compilers!
> 

Oops =)

Fixes: bb516dfc5b3
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/tcg/configure.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/tcg/configure.sh b/tests/tcg/configure.sh
> index 9eb6ba3b7ea..eaaaff6233a 100755
> --- a/tests/tcg/configure.sh
> +++ b/tests/tcg/configure.sh
> @@ -228,7 +228,7 @@ for target in $target_list; do
>                  echo "CROSS_CC_HAS_SVE=y" >> $config_target_mak
>              fi
>              if do_compiler "$target_compiler" $target_compiler_cflags \
> -               -march=-march=armv8.3-a -o $TMPE $TMPC; then
> +               -march=armv8.3-a -o $TMPE $TMPC; then
>                  echo "CROSS_CC_HAS_ARMV8_3=y" >> $config_target_mak
>              fi
>          ;;
> 

Re: [PATCH v2 18/19] tests/tcg: fix typo in configure.sh test for v8.3

[Richard Henderson]

On 2/13/20 2:51 PM, Alex Bennée wrote:
> Although most people use the docker images this can trip up on
> developer systems with actual valid cross-compilers!
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/tcg/configure.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

r~

[PATCH v2 19/19] tests/tcg: take into account expected clashes pauth-4

[Alex Bennée]

Pointer authentication isn't perfect so measure the percentage of
failed checks. As we want to vary the pointer that is authenticated we
recurse down the stack.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/aarch64/pauth-4.c | 54 +++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 17 deletions(-)

diff --git a/tests/tcg/aarch64/pauth-4.c b/tests/tcg/aarch64/pauth-4.c
index 1040e92aec3..24a639e36ca 100644
--- a/tests/tcg/aarch64/pauth-4.c
+++ b/tests/tcg/aarch64/pauth-4.c
@@ -1,25 +1,45 @@
 #include <stdint.h>
 #include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#define TESTS 1000
 
 int main()
 {
-  uintptr_t x, y;
+    int i, count = 0;
+    float perc;
+    void *base = malloc(TESTS);
+
+    for (i = 0; i < TESTS; i++) {
+        uintptr_t in, x, y;
+
+        in = i + (uintptr_t) base;
+
+        asm("mov %0, %[in]\n\t"
+            "pacia %0, sp\n\t"        /* sigill if pauth not supported */
+            "eor %0, %0, #4\n\t"      /* corrupt single bit */
+            "mov %1, %0\n\t"
+            "autia %1, sp\n\t"        /* validate corrupted pointer */
+            "xpaci %0\n\t"            /* strip pac from corrupted pointer */
+            : /* out */ "=r"(x), "=r"(y)
+            : /* in */ [in] "r" (in)
+            : /* clobbers */);
 
-  asm("mov %0, lr\n\t"
-      "pacia %0, sp\n\t"        /* sigill if pauth not supported */
-      "eor %0, %0, #4\n\t"      /* corrupt single bit */
-      "mov %1, %0\n\t"
-      "autia %1, sp\n\t"        /* validate corrupted pointer */
-      "xpaci %0\n\t"            /* strip pac from corrupted pointer */
-      : "=r"(x), "=r"(y));
+        /*
+         * Once stripped, the corrupted pointer is of the form 0x0000...wxyz.
+         * We expect the autia to indicate failure, producing a pointer of the
+         * form 0x000e....wxyz.  Use xpaci and != for the test, rather than
+         * extracting explicit bits from the top, because the location of the
+         * error code "e" depends on the configuration of virtual memory.
+         */
+        if (x != y) {
+            count++;
+        }
 
-  /*
-   * Once stripped, the corrupted pointer is of the form 0x0000...wxyz.
-   * We expect the autia to indicate failure, producing a pointer of the
-   * form 0x000e....wxyz.  Use xpaci and != for the test, rather than
-   * extracting explicit bits from the top, because the location of the
-   * error code "e" depends on the configuration of virtual memory.
-   */
-  assert(x != y);
-  return 0;
+    }
+    perc = (float) count / (float) TESTS;
+    printf("Checks Passed: %0.2f%%", perc * 100.0);
+    assert(perc > 0.95);
+    return 0;
 }
-- 
2.20.1

Re: [PATCH v2 19/19] tests/tcg: take into account expected clashes pauth-4

[Robert Foley]

On Thu, 13 Feb 2020 at 18:00, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Pointer authentication isn't perfect so measure the percentage of
> failed checks. As we want to vary the pointer that is authenticated we
> recurse down the stack.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Robert Foley <robert.foley@linaro.org>

Re: [PATCH v2 19/19] tests/tcg: take into account expected clashes pauth-4

[Richard Henderson]

On 2/13/20 2:51 PM, Alex Bennée wrote:
> Pointer authentication isn't perfect so measure the percentage of
> failed checks. As we want to vary the pointer that is authenticated we
> recurse down the stack.
> 

You're no longer recursing.

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/tcg/aarch64/pauth-4.c | 54 +++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 17 deletions(-)
> 
> diff --git a/tests/tcg/aarch64/pauth-4.c b/tests/tcg/aarch64/pauth-4.c
> index 1040e92aec3..24a639e36ca 100644
> --- a/tests/tcg/aarch64/pauth-4.c
> +++ b/tests/tcg/aarch64/pauth-4.c
> @@ -1,25 +1,45 @@
>  #include <stdint.h>
>  #include <assert.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +#define TESTS 1000
>  
>  int main()
>  {
> -  uintptr_t x, y;
> +    int i, count = 0;
> +    float perc;
> +    void *base = malloc(TESTS);
> +
> +    for (i = 0; i < TESTS; i++) {
> +        uintptr_t in, x, y;
> +
> +        in = i + (uintptr_t) base;

There's no reason all of these couldn't be char* or void* instead of casting to
uintptr_t.  Nothing else would have to change.

> +
> +        asm("mov %0, %[in]\n\t"
> +            "pacia %0, sp\n\t"        /* sigill if pauth not supported */
> +            "eor %0, %0, #4\n\t"      /* corrupt single bit */
> +            "mov %1, %0\n\t"
> +            "autia %1, sp\n\t"        /* validate corrupted pointer */
> +            "xpaci %0\n\t"            /* strip pac from corrupted pointer */
> +            : /* out */ "=r"(x), "=r"(y)
> +            : /* in */ [in] "r" (in)

It's weird to have some arguments named and some not.  Why not just use %2,
since this is simple enough?

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

r~

Re: [PATCH v2 00/19] testing and plugin updates

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200213225109.13120-1-alex.bennee@linaro.org/



Hi,

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

Subject: [PATCH  v2 00/19] testing and plugin updates
Message-id: 20200213225109.13120-1-alex.bennee@linaro.org
Type: series

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200213225109.13120-1-alex.bennee@linaro.org -> patchew/20200213225109.13120-1-alex.bennee@linaro.org
Switched to a new branch 'test'
3109a26 tests/tcg: take into account expected clashes pauth-4
bd088da tests/tcg: fix typo in configure.sh test for v8.3
45e0661 tcg: save vaddr temp for plugin usage
465e545 tests/tcg: give debug builds a little bit longer
1dc357e tests/plugins: make howvec clean-up after itself.
fcb8562 target/riscv: progressively load the instruction during decode
2e6734c qemu/bitops.h: Add extract8 and extract16
21ff7e3 tests/plugin: prevent uninitialized warning
81f2039 plugins/core: add missing break in cb_to_tcg_flags
80651cf docs/devel: document query handle lifetimes
9dc3d6d tracing: only allow -trace to override -D if set
eb2e0ad tests/iotests: be a little more forgiving on the size test
4d90ee2 travis.yml: single-thread build-tcg stages
3425df2 travis.yml: Fix Travis YAML configuration warnings
49b5310 travis.yml: Test the s390-ccw build, too
4ef30b9 tests/rcutorture: mild documenting refactor of update thread
23905a1 tests/rcutorture: better document locking of stats
c3a20fc tests/rcutorture: update usage hint
e31233b tests/tcg: include a skip runner for pauth3 with plugins

=== OUTPUT BEGIN ===
1/19 Checking commit e31233ba5677 (tests/tcg: include a skip runner for pauth3 with plugins)
2/19 Checking commit c3a20fc36238 (tests/rcutorture: update usage hint)
3/19 Checking commit 23905a1b8851 (tests/rcutorture: better document locking of stats)
4/19 Checking commit 4ef30b9250b9 (tests/rcutorture: mild documenting refactor of update thread)
5/19 Checking commit 49b53100439e (travis.yml: Test the s390-ccw build, too)
6/19 Checking commit 3425df244fcd (travis.yml: Fix Travis YAML configuration warnings)
7/19 Checking commit 4d90ee26396b (travis.yml: single-thread build-tcg stages)
8/19 Checking commit eb2e0addbf96 (tests/iotests: be a little more forgiving on the size test)
9/19 Checking commit 9dc3d6d877be (tracing: only allow -trace to override -D if set)
10/19 Checking commit 80651cf4dd6a (docs/devel: document query handle lifetimes)
11/19 Checking commit 81f203940a48 (plugins/core: add missing break in cb_to_tcg_flags)
12/19 Checking commit 21ff7e396207 (tests/plugin: prevent uninitialized warning)
13/19 Checking commit 2e6734c5b2b1 (qemu/bitops.h: Add extract8 and extract16)
14/19 Checking commit fcb85625f96a (target/riscv: progressively load the instruction during decode)
15/19 Checking commit 1dc357e99457 (tests/plugins: make howvec clean-up after itself.)
16/19 Checking commit 465e5454070a (tests/tcg: give debug builds a little bit longer)
17/19 Checking commit 45e066189d4e (tcg: save vaddr temp for plugin usage)
18/19 Checking commit bd088dac07cc (tests/tcg: fix typo in configure.sh test for v8.3)
19/19 Checking commit 3109a268d5ae (tests/tcg: take into account expected clashes pauth-4)
WARNING: Block comments use a leading /* on a separate line
#60: FILE: tests/tcg/aarch64/pauth-4.c:25:
+            : /* out */ "=r"(x), "=r"(y)

WARNING: Block comments use a leading /* on a separate line
#61: FILE: tests/tcg/aarch64/pauth-4.c:26:
+            : /* in */ [in] "r" (in)

ERROR: space prohibited before open square bracket '['
#61: FILE: tests/tcg/aarch64/pauth-4.c:26:
+            : /* in */ [in] "r" (in)

WARNING: Block comments use a leading /* on a separate line
#62: FILE: tests/tcg/aarch64/pauth-4.c:27:
+            : /* clobbers */);

total: 1 errors, 3 warnings, 62 lines checked

Patch 19/19 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


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