From f2887ba33606b9dc98f451c7f0e89dba37cef94c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 12:46:52 +0000 Subject: [PATCH 01/19] tests/tcg: include a skip runner for pauth3 with plugins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Robert Foley Message-Id: <20200225124710.14152-2-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 d2299b98b7..71f72cfbe3 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 From 804c96848b18458e05d64a5fdda8ced29c2470a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 12:46:53 +0000 Subject: [PATCH 02/19] tests/rcutorture: update usage hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Although documented in the comments we don't display all the various invocations we can in the usage. Signed-off-by: Alex Bennée Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20200225124710.14152-3-alex.bennee@linaro.org> --- tests/rcutorture.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/rcutorture.c b/tests/rcutorture.c index 49311c82ea..e8b2169e7d 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); } From ea70ccff651d10625cb051521d7ac029874008c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 12:46:54 +0000 Subject: [PATCH 03/19] tests/rcutorture: better document locking of stats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is pure code motion with no functional effect. Signed-off-by: Alex Bennée Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20200225124710.14152-4-alex.bennee@linaro.org> --- tests/rcutorture.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/rcutorture.c b/tests/rcutorture.c index e8b2169e7d..256d24ed5b 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]; From a74c82c8dc73560d856dfabd22a6abb40ac907dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 12:46:55 +0000 Subject: [PATCH 04/19] tests/rcutorture: mild documenting refactor of update thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Paolo Bonzini Message-Id: <20200225124710.14152-5-alex.bennee@linaro.org> --- tests/rcutorture.c | 55 +++++++++++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/tests/rcutorture.c b/tests/rcutorture.c index 256d24ed5b..732f03abda 100644 --- a/tests/rcutorture.c +++ b/tests/rcutorture.c @@ -230,13 +230,12 @@ static void uperftest(int nupdaters, int duration) #define RCU_STRESS_PIPE_LEN 10 struct rcu_stress { - int pipe_count; + int age; /* how many update cycles while not rcu_stress_current */ int mbtest; }; 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 */ @@ -261,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(); @@ -269,7 +268,7 @@ static void *rcu_read_stress_test(void *arg) garbage++; } rcu_read_unlock(); - pc = p->pipe_count; + pc = atomic_read(&p->age); rcu_read_unlock(); if ((pc > RCU_STRESS_PIPE_LEN) || (pc < 0)) { pc = RCU_STRESS_PIPE_LEN; @@ -288,32 +287,52 @@ 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 age. It + * then increments the age of all the other entries. The age + * 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->mbtest = 0; + p = &rcu_stress_array[rcu_stress_idx]; + /* catching up with ourselves would be a bug */ + assert(p != cp); + atomic_set(&p->mbtest, 0); smp_mb(); - p->pipe_count = 0; - p->mbtest = 1; + atomic_set(&p->age, 0); + atomic_set(&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++; + atomic_set(&rcu_stress_array[i].age, + rcu_stress_array[i].age + 1); } } synchronize_rcu(); @@ -346,7 +365,7 @@ static void stresstest(int nreaders, int duration) int i; rcu_stress_current = &rcu_stress_array[0]; - rcu_stress_current->pipe_count = 0; + rcu_stress_current->age = 0; rcu_stress_current->mbtest = 1; for (i = 0; i < nreaders; i++) { create_thread(rcu_read_stress_test); @@ -376,7 +395,7 @@ static void gtest_stress(int nreaders, int duration) int i; rcu_stress_current = &rcu_stress_array[0]; - rcu_stress_current->pipe_count = 0; + rcu_stress_current->age = 0; rcu_stress_current->mbtest = 1; for (i = 0; i < nreaders; i++) { create_thread(rcu_read_stress_test); From 31c8cc4f94e33571df3ac7427359e40a6e95d0f3 Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Tue, 25 Feb 2020 12:46:56 +0000 Subject: [PATCH 05/19] travis.yml: Test the s390-ccw build, too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Alex Bennée Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Acked-by: Cornelia Huck Message-Id: <20200206202543.7085-1-thuth@redhat.com> Message-Id: <20200225124710.14152-6-alex.bennee@linaro.org> --- .travis.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.travis.yml b/.travis.yml index 5887055951..ea13e07179 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'. From 321e6ea5772f8c6140e3c5242540c97604a9730c Mon Sep 17 00:00:00 2001 From: Wainer dos Santos Moschetta Date: Tue, 25 Feb 2020 12:46:57 +0000 Subject: [PATCH 06/19] travis.yml: Fix Travis YAML configuration warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Alex Bennée Message-Id: <20200207210124.141119-2-wainersm@redhat.com> Message-Id: <20200225124710.14152-7-alex.bennee@linaro.org> --- .travis.yml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index ea13e07179..0612998958 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 From c9331e9c28184decb22ad8d66375bec03b988c7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 12:46:58 +0000 Subject: [PATCH 07/19] travis.yml: single-thread build-tcg stages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This still seems to be a problem for Travis. Signed-off-by: Alex Bennée Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20200225124710.14152-8-alex.bennee@linaro.org> --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 0612998958..f4020dcc6c 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" From 002375895c10df40615fc615e2639f49e0c442fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 20:20:09 +0000 Subject: [PATCH 08/19] tests/iotests: be a little more forgiving on the size test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Robert Foley Reviewed-by: Stefan Berger Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20200225124710.14152-9-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 3500e0c47a..af677d90b8 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 < $sizeB)" fi $QEMU_IMG check --output=json "$TEST_IMG" | From 3d88754e2b096ace0f9c2e86dbbb84de4290d421 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 12:47:00 +0000 Subject: [PATCH 09/19] tracing: only allow -trace to override -D if set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Otherwise any -D settings the user may have made get ignored. Signed-off-by: Alex Bennée Tested-by: Laurent Vivier Reviewed-by: Robert Foley Message-Id: <20200225124710.14152-10-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 6c775e68eb..2ffe000818 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=...: " From 9675a9c6e85398f370aed0157cb50426e8351c55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 12:47:01 +0000 Subject: [PATCH 10/19] docs/devel: document query handle lifetimes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I forgot to document the lifetime of handles in the developer documentation. Do so now. Signed-off-by: Alex Bennée Reviewed-by: Robert Foley Reviewed-by: Robert Foley Message-Id: <20200225124710.14152-11-alex.bennee@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 718eef00f2..a05990906c 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 ===== From dcc474c69e6a59044b9bb54624bd636cbfd98aa9 Mon Sep 17 00:00:00 2001 From: "Emilio G. Cota" Date: Tue, 25 Feb 2020 12:47:02 +0000 Subject: [PATCH 11/19] plugins/core: add missing break in cb_to_tcg_flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: 54cb65d8588 Reported-by: Robert Henry Signed-off-by: Emilio G. Cota Signed-off-by: Alex Bennée Reviewed-by: Richard Henderson Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20200105072940.32204-1-cota@braap.org> Cc: qemu-stable@nongnu.org Message-Id: <20200225124710.14152-12-alex.bennee@linaro.org> --- plugins/core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/core.c b/plugins/core.c index 9e1b9e7a91..ed863011ba 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; From 413368611039ce435e74fd0ce1b677ccc08a380e Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Tue, 25 Feb 2020 12:47:03 +0000 Subject: [PATCH 12/19] tests/plugin: prevent uninitialized warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Chen Qun Reviewed-by: Thomas Huth Message-Id: <20200206093238.203984-1-kuhn.chenqun@huawei.com> [AJB: uses Thomas's single line allocation] Signed-off-by: Alex Bennée Reviewed-by: Richard Henderson Message-Id: <20200225124710.14152-13-alex.bennee@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 f30bea08dc..df19fd359d 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 0a8f5a0000..a9a6e41237 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); } From ed04c8b14c8bed6b8d940547ed237d309fca7dfc Mon Sep 17 00:00:00 2001 From: Yoshinori Sato Date: Tue, 25 Feb 2020 12:47:04 +0000 Subject: [PATCH 13/19] qemu/bitops.h: Add extract8 and extract16 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Yoshinori Sato Reviewed-by: Richard Henderson Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Signed-off-by: Richard Henderson Signed-off-by: Alex Bennée Message-Id: <20200212130311.127515-3-ysato@users.sourceforge.jp> Message-Id: <20200225124710.14152-14-alex.bennee@linaro.org> --- include/qemu/bitops.h | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h index 02c1ce6a5d..f55ce8b320 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 From 25139bf7f87a0d0e758d4198579a227ab551802a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 12:47:05 +0000 Subject: [PATCH 14/19] target/riscv: progressively load the instruction during decode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Richard Henderson Reviewed-by: Alistair Francis Reviewed-by: Robert Foley Message-Id: <20200225124710.14152-15-alex.bennee@linaro.org> --- 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 f8ad7d60fd..40b6d2b64d 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 14dc71156b..d5de7f468a 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) { From ec11c4a8ec7e858ec328772910def6e2fca9079a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 12:47:06 +0000 Subject: [PATCH 15/19] tests/plugins: make howvec clean-up after itself. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Robert Foley Reviewed-by: Richard Henderson Message-Id: <20200225124710.14152-16-alex.bennee@linaro.org> --- 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 4ca555e123..3b9a6939f2 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) From a0dafafebaefadc1eb883c124830b22d64ad0f8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 12:47:07 +0000 Subject: [PATCH 16/19] tests/tcg: give debug builds a little bit longer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When combined with heavy plugins we occasionally hit the timeouts. Signed-off-by: Alex Bennée Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20200225124710.14152-17-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 3c7421a356..b3cff3cad1 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 From fcc54ab5c7ca84ae72e8bf3781c33c9193a911aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 17:49:08 +0000 Subject: [PATCH 17/19] tcg: save vaddr temp for plugin usage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Alex Bennée Reviewed-by: Richard Henderson Reviewed-by: Emilio G. Cota Cc: qemu-stable@nongnu.org Message-Id: <20200225124710.14152-18-alex.bennee@linaro.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 7d782002e3..e2e25ebf7d 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); From a62f849dc5410b649781355a2635cd2956817c74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 12:47:09 +0000 Subject: [PATCH 18/19] tests/tcg: fix typo in configure.sh test for v8.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Although most people use the docker images this can trip up on developer systems with actual valid cross-compilers! Fixes: bb516dfc5b3 Signed-off-by: Alex Bennée Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Richard Henderson Message-Id: <20200225124710.14152-19-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 9eb6ba3b7e..eaaaff6233 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 ;; From bc97f9f64f8a4a84d0d06949749e9dbec143b9f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Tue, 25 Feb 2020 12:47:10 +0000 Subject: [PATCH 19/19] tests/tcg: take into account expected clashes pauth-4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pointer authentication isn't perfect so measure the percentage of failed checks. As we want to vary the pointer we work through a bunch of different addresses. Signed-off-by: Alex Bennée Reviewed-by: Robert Foley Reviewed-by: Richard Henderson Message-Id: <20200225124710.14152-20-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 1040e92aec..24a639e36c 100644 --- a/tests/tcg/aarch64/pauth-4.c +++ b/tests/tcg/aarch64/pauth-4.c @@ -1,25 +1,45 @@ #include #include +#include +#include + +#define TESTS 1000 int main() { - uintptr_t x, y; + int i, count = 0; + float perc; + void *base = malloc(TESTS); - 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)); + for (i = 0; i < TESTS; i++) { + uintptr_t in, x, 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. - */ - assert(x != y); - return 0; + 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 */); + + /* + * 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++; + } + + } + perc = (float) count / (float) TESTS; + printf("Checks Passed: %0.2f%%", perc * 100.0); + assert(perc > 0.95); + return 0; }