[PATCH v2 00/11] misc fixes for rc0 (docker, plugins, softfloat)

[PATCH v2 00/11] misc fixes for rc0 (docker, plugins, softfloat)

[Alex Bennée]

Hi,

These are some candidate patches for rc0 along with a few plugin
patches that haven't yet gotten review. The new functionality won't
get added to the PR but I'd like to get the cputlb fix in. I've also
had another run at fixing the -Wpsabi problem.

The following still need review:

 - docs/devel: fix grammar in multi-thread-tcg
 - tests/plugins: don't unconditionally add -Wpsabi
 - cputlb: ensure we save the IOTLB data in case of reset

Alex Bennée (9):
  configure: remove all dependencies on a (re)configure
  docker.py: fix fetching of FROM layers
  tests/plugins: don't unconditionally add -Wpsabi
  cputlb: ensure we save the IOTLB data in case of reset
  plugins: expand the bb plugin to be thread safe and track per-cpu
  docs/devel: fix grammar in multi-thread-tcg
  hw/virtio/pci: include vdev name in registered PCI sections
  plugins: add API to return a name for a IO device
  plugins: new hwprofile plugin

LIU Zhiwei (1):
  fpu/softfloat: fix up float16 nan recognition

Thomas Huth (1):
  tests/docker: Remove the libssh workaround from the ubuntu 20.04 image

 docs/devel/multi-thread-tcg.rst            |   2 +-
 configure                                  |  18 +-
 include/hw/core/cpu.h                      |  16 ++
 include/qemu/qemu-plugin.h                 |   6 +
 include/qemu/typedefs.h                    |   1 +
 accel/tcg/cputlb.c                         |  38 ++-
 fpu/softfloat-specialize.inc.c             |   4 +-
 hw/virtio/virtio-pci.c                     |  22 +-
 plugins/api.c                              |  20 ++
 tests/plugin/bb.c                          |  97 ++++++-
 tests/plugin/hwprofile.c                   | 305 +++++++++++++++++++++
 tests/docker/docker.py                     |  16 +-
 tests/docker/dockerfiles/ubuntu2004.docker |   3 -
 tests/plugin/Makefile                      |  23 +-
 tests/tcg/Makefile.target                  |  12 +-
 15 files changed, 533 insertions(+), 50 deletions(-)
 create mode 100644 tests/plugin/hwprofile.c

-- 
2.20.1

[PATCH v2 01/11] configure: remove all dependencies on a (re)configure

[Alex Bennée]

The previous code was brittle and missed cases such as the mipn32
variants which for some reason has the 64 bit syscalls. This leads to
a number of binary targets having deps lines like:

  all.clang-sanitizer/mipsn32el-linux-user/linux-user/signal.d
  140:  /home/alex/lsrc/qemu.git/linux-user/mips64/syscall_nr.h \
  455:/home/alex/lsrc/qemu.git/linux-user/mips64/syscall_nr.h:

  all.clang-sanitizer/mipsn32el-linux-user/linux-user/syscall.d
  146:  /home/alex/lsrc/qemu.git/linux-user/mips64/syscall_nr.h \
  485:/home/alex/lsrc/qemu.git/linux-user/mips64/syscall_nr.h:

which in turn would trigger the re-generation of syscall_nr.h in the
source tree (thanks to generic %/syscall_nr.h rules). The previous
code attempts to clean it out but misses edge cases but fails.

After spending a day trying to understand how this was happening I'm
unconvinced that there are not other such breakages possible with this
"caching". As we add more auto-generated code to the build it is likely
to trip up again. Apply a hammer to the problem.

Fixes: 91e5998f18 (which fixes 5f29856b852d and 4d6a835dea47)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 configure | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/configure b/configure
index bc3b9ad931..e1de2f5b24 100755
--- a/configure
+++ b/configure
@@ -1955,23 +1955,20 @@ EOF
 exit 0
 fi
 
-# Remove old dependency files to make sure that they get properly regenerated
-rm -f */config-devices.mak.d
-
 # Remove syscall_nr.h to be sure they will be regenerated in the build
 # directory, not in the source directory
 for arch in alpha hppa m68k xtensa sh4 microblaze arm ppc s390x sparc sparc64 \
     i386 x86_64 mips mips64 ; do
     # remove the file if it has been generated in the source directory
     rm -f "${source_path}/linux-user/${arch}/syscall_nr.h"
-    # remove the dependency files
-    for target in ${arch}*-linux-user ; do
-        test -d "${target}" && find "${target}" -type f -name "*.d" \
-             -exec grep -q "${source_path}/linux-user/${arch}/syscall_nr.h" {} \; \
-             -print | while read file ; do rm "${file}" "${file%.d}.o" ; done
-    done
 done
 
+# Clean out all old dependency files. As more files are generated we
+# run the risk of old dependencies triggering generation in the wrong
+# places. Previous brittle attempts to be surgical tend to miss edge
+# cases leading to wasted time and much confusion.
+find -type f -name "*.d" -exec rm -f {} \;
+
 if test -z "$python"
 then
     error_exit "Python not found. Use --python=/path/to/python"
-- 
2.20.1

Re: [PATCH v2 01/11] configure: remove all dependencies on a (re)configure

[Philippe Mathieu-Daudé]

On 7/13/20 10:04 PM, Alex Bennée wrote:
> The previous code was brittle and missed cases such as the mipn32
> variants which for some reason has the 64 bit syscalls. This leads to
> a number of binary targets having deps lines like:
> 
>   all.clang-sanitizer/mipsn32el-linux-user/linux-user/signal.d
>   140:  /home/alex/lsrc/qemu.git/linux-user/mips64/syscall_nr.h \
>   455:/home/alex/lsrc/qemu.git/linux-user/mips64/syscall_nr.h:
> 
>   all.clang-sanitizer/mipsn32el-linux-user/linux-user/syscall.d
>   146:  /home/alex/lsrc/qemu.git/linux-user/mips64/syscall_nr.h \
>   485:/home/alex/lsrc/qemu.git/linux-user/mips64/syscall_nr.h:
> 
> which in turn would trigger the re-generation of syscall_nr.h in the
> source tree (thanks to generic %/syscall_nr.h rules). The previous
> code attempts to clean it out but misses edge cases but fails.
> 
> After spending a day trying to understand how this was happening I'm
> unconvinced that there are not other such breakages possible with this
> "caching". As we add more auto-generated code to the build it is likely
> to trip up again. Apply a hammer to the problem.
> 
> Fixes: 91e5998f18 (which fixes 5f29856b852d and 4d6a835dea47)
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  configure | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/configure b/configure
> index bc3b9ad931..e1de2f5b24 100755
> --- a/configure
> +++ b/configure
> @@ -1955,23 +1955,20 @@ EOF
>  exit 0
>  fi
>  
> -# Remove old dependency files to make sure that they get properly regenerated
> -rm -f */config-devices.mak.d
> -
>  # Remove syscall_nr.h to be sure they will be regenerated in the build
>  # directory, not in the source directory
>  for arch in alpha hppa m68k xtensa sh4 microblaze arm ppc s390x sparc sparc64 \
>      i386 x86_64 mips mips64 ; do
>      # remove the file if it has been generated in the source directory
>      rm -f "${source_path}/linux-user/${arch}/syscall_nr.h"
> -    # remove the dependency files
> -    for target in ${arch}*-linux-user ; do
> -        test -d "${target}" && find "${target}" -type f -name "*.d" \
> -             -exec grep -q "${source_path}/linux-user/${arch}/syscall_nr.h" {} \; \
> -             -print | while read file ; do rm "${file}" "${file%.d}.o" ; done
> -    done
>  done
>  
> +# Clean out all old dependency files. As more files are generated we
> +# run the risk of old dependencies triggering generation in the wrong
> +# places. Previous brittle attempts to be surgical tend to miss edge
> +# cases leading to wasted time and much confusion.
> +find -type f -name "*.d" -exec rm -f {} \;
> +
>  if test -z "$python"
>  then
>      error_exit "Python not found. Use --python=/path/to/python"
> 

[PATCH v2 02/11] tests/docker: Remove the libssh workaround from the ubuntu 20.04 image

[Alex Bennée]

From: Thomas Huth <thuth@redhat.com>

The libssh problem only exists in Ubuntu 18.04 - we can enable it
in 20.04 again.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20200713185237.9419-1-thuth@redhat.com>
---
 tests/docker/dockerfiles/ubuntu2004.docker | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tests/docker/dockerfiles/ubuntu2004.docker b/tests/docker/dockerfiles/ubuntu2004.docker
index f7aac840bf..8d10934a2a 100644
--- a/tests/docker/dockerfiles/ubuntu2004.docker
+++ b/tests/docker/dockerfiles/ubuntu2004.docker
@@ -65,9 +65,6 @@ RUN apt-get update && \
 RUN dpkg -l $PACKAGES | sort > /packages.txt
 ENV FEATURES clang tsan pyyaml sdl2
 
-# https://bugs.launchpad.net/qemu/+bug/1838763
-ENV QEMU_CONFIGURE_OPTS --disable-libssh
-
 # Apply patch https://reviews.llvm.org/D75820
 # This is required for TSan in clang-10 to compile with QEMU.
 RUN sed -i 's/^const/static const/g' /usr/lib/llvm-10/lib/clang/10.0.0/include/sanitizer/tsan_interface.h
-- 
2.20.1

[PATCH v2 03/11] docker.py: fix fetching of FROM layers

[Alex Bennée]

This worked on a system that was already bootstrapped because the
stage 2 images already existed even if they wouldn't be used. What we
should have pulled down was the FROM line containers first because
building on gitlab doesn't have the advantage of using our build
system to build the pre-requisite bits.

We still pull the image we want to build just in case we can use the
cached data.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/docker/docker.py | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 2d67bbd15a..c9f20d8d09 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -306,14 +306,18 @@ class Docker(object):
         checksum = _text_checksum(_dockerfile_preprocess(dockerfile))
 
         if registry is not None:
-            # see if we can fetch a cache copy, may fail...
-            pull_args = ["pull", "%s/%s" % (registry, tag)]
-            if self._do(pull_args, quiet=quiet) == 0:
+            sources = re.findall("FROM qemu\/(.*)", dockerfile)
+            # Fetch any cache layers we can, may fail
+            for s in sources:
+                pull_args = ["pull", "%s/qemu/%s" % (registry, s)]
+                if self._do(pull_args, quiet=quiet) != 0:
+                    registry = None
+                    break
+            # Make substitutions
+            if registry is not None:
                 dockerfile = dockerfile.replace("FROM qemu/",
                                                 "FROM %s/qemu/" %
                                                 (registry))
-            else:
-                registry = None
 
         tmp_df = tempfile.NamedTemporaryFile(mode="w+t",
                                              encoding='utf-8',
@@ -339,6 +343,8 @@ class Docker(object):
             build_args += ["--build-arg", "BUILDKIT_INLINE_CACHE=1"]
 
         if registry is not None:
+            pull_args = ["pull", "%s/%s" % (registry, tag)]
+            self._do(pull_args, quiet=quiet)
             cache = "%s/%s" % (registry, tag)
             build_args += ["--cache-from", cache]
         build_args += argv
-- 
2.20.1

[PATCH v2 04/11] fpu/softfloat: fix up float16 nan recognition

[Alex Bennée]

From: LIU Zhiwei <zhiwei_liu@c-sky.com>

Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200712234521.3972-2-zhiwei_liu@c-sky.com>
---
 fpu/softfloat-specialize.inc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fpu/softfloat-specialize.inc.c b/fpu/softfloat-specialize.inc.c
index 44f5b661f8..034d18199c 100644
--- a/fpu/softfloat-specialize.inc.c
+++ b/fpu/softfloat-specialize.inc.c
@@ -254,7 +254,7 @@ bool float16_is_quiet_nan(float16 a_, float_status *status)
     if (snan_bit_is_one(status)) {
         return (((a >> 9) & 0x3F) == 0x3E) && (a & 0x1FF);
     } else {
-        return ((a & ~0x8000) >= 0x7C80);
+        return ((a >> 9) & 0x3F) == 0x3F;
     }
 #endif
 }
@@ -271,7 +271,7 @@ bool float16_is_signaling_nan(float16 a_, float_status *status)
 #else
     uint16_t a = float16_val(a_);
     if (snan_bit_is_one(status)) {
-        return ((a & ~0x8000) >= 0x7C80);
+        return ((a >> 9) & 0x3F) == 0x3F;
     } else {
         return (((a >> 9) & 0x3F) == 0x3E) && (a & 0x1FF);
     }
-- 
2.20.1

[PATCH v2 05/11] tests/plugins: don't unconditionally add -Wpsabi

[Alex Bennée]

Not all compilers support the -Wpsabi (clang-9 in my case). To handle
this gracefully we pare back the shared build machinery so the
Makefile is relatively "standalone". We still take advantage of
config-host.mak as configure has done a bunch of probing for us but
that is it.

Fixes: bac8d222a
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - separate from main build system and check probe
---
 configure             |  3 +++
 tests/plugin/Makefile | 22 ++++++++++++++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index e1de2f5b24..08eaa99d19 100755
--- a/configure
+++ b/configure
@@ -7112,6 +7112,9 @@ echo "GIT_UPDATE=$git_update" >> $config_host_mak
 
 echo "ARCH=$ARCH" >> $config_host_mak
 
+echo "GLIB_CFLAGS=$glib_cflags" >> $config_host_mak
+echo "GLIB_LDFLAGS=$glib_ldflags" >> $config_host_mak
+
 if test "$default_devices" = "yes" ; then
   echo "CONFIG_MINIKCONF_MODE=--defconfig" >> $config_host_mak
 else
diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
index 3a50451428..e9348fde4a 100644
--- a/tests/plugin/Makefile
+++ b/tests/plugin/Makefile
@@ -1,9 +1,16 @@
+# -*- Mode: makefile -*-
+#
+# This Makefile example is fairly independent from the main makefile
+# so users can take and adapt it for their build. We only really
+# include config-host.mak so we don't have to repeat probing for
+# cflags that the main configure has already done for us.
+#
+
 BUILD_DIR := $(CURDIR)/../..
 
 include $(BUILD_DIR)/config-host.mak
-include $(SRC_PATH)/rules.mak
 
-$(call set-vpath, $(SRC_PATH)/tests/plugin)
+VPATH += $(SRC_PATH)/tests/plugin
 
 NAMES :=
 NAMES += bb
@@ -17,11 +24,18 @@ NAMES += lockstep
 
 SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
 
-QEMU_CFLAGS += -fPIC -Wpsabi
-QEMU_CFLAGS += -I$(SRC_PATH)/include/qemu
+# The main QEMU uses Glib extensively so it's perfectly fine to use it
+# in plugins (which many example do).
+CFLAGS = $(GLIB_CFLAGS)
+CFLAGS += -fPIC
+CFLAGS += $(if $(findstring no-psabi,$(QEMU_CFLAGS)),-Wpsabi)
+CFLAGS += -I$(SRC_PATH)/include/qemu
 
 all: $(SONAMES)
 
+%.o: %.c
+	$(CC) $(CFLAGS) -c -o $@ $<
+
 lib%.so: %.o
 	$(CC) -shared -Wl,-soname,$@ -o $@ $^ $(LDLIBS)
 
-- 
2.20.1

Re: [PATCH v2 05/11] tests/plugins: don't unconditionally add -Wpsabi

[Thomas Huth]

On 13/07/2020 22.04, Alex Bennée wrote:
> Not all compilers support the -Wpsabi (clang-9 in my case). To handle
> this gracefully we pare back the shared build machinery so the
> Makefile is relatively "standalone". We still take advantage of
> config-host.mak as configure has done a bunch of probing for us but
> that is it.
> 
> Fixes: bac8d222a
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>   - separate from main build system and check probe
> ---
>  configure             |  3 +++
>  tests/plugin/Makefile | 22 ++++++++++++++++++----
>  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/configure b/configure
> index e1de2f5b24..08eaa99d19 100755
> --- a/configure
> +++ b/configure
> @@ -7112,6 +7112,9 @@ echo "GIT_UPDATE=$git_update" >> $config_host_mak
>  
>  echo "ARCH=$ARCH" >> $config_host_mak
>  
> +echo "GLIB_CFLAGS=$glib_cflags" >> $config_host_mak
> +echo "GLIB_LDFLAGS=$glib_ldflags" >> $config_host_mak
> +
>  if test "$default_devices" = "yes" ; then
>    echo "CONFIG_MINIKCONF_MODE=--defconfig" >> $config_host_mak
>  else
> diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
> index 3a50451428..e9348fde4a 100644
> --- a/tests/plugin/Makefile
> +++ b/tests/plugin/Makefile
> @@ -1,9 +1,16 @@
> +# -*- Mode: makefile -*-
> +#
> +# This Makefile example is fairly independent from the main makefile
> +# so users can take and adapt it for their build. We only really
> +# include config-host.mak so we don't have to repeat probing for
> +# cflags that the main configure has already done for us.
> +#
> +
>  BUILD_DIR := $(CURDIR)/../..
>  
>  include $(BUILD_DIR)/config-host.mak
> -include $(SRC_PATH)/rules.mak
>  
> -$(call set-vpath, $(SRC_PATH)/tests/plugin)
> +VPATH += $(SRC_PATH)/tests/plugin
>  
>  NAMES :=
>  NAMES += bb
> @@ -17,11 +24,18 @@ NAMES += lockstep
>  
>  SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>  
> -QEMU_CFLAGS += -fPIC -Wpsabi
> -QEMU_CFLAGS += -I$(SRC_PATH)/include/qemu
> +# The main QEMU uses Glib extensively so it's perfectly fine to use it
> +# in plugins (which many example do).
> +CFLAGS = $(GLIB_CFLAGS)
> +CFLAGS += -fPIC
> +CFLAGS += $(if $(findstring no-psabi,$(QEMU_CFLAGS)),-Wpsabi)
> +CFLAGS += -I$(SRC_PATH)/include/qemu
>  
>  all: $(SONAMES)
>  
> +%.o: %.c
> +	$(CC) $(CFLAGS) -c -o $@ $<
> +
>  lib%.so: %.o
>  	$(CC) -shared -Wl,-soname,$@ -o $@ $^ $(LDLIBS)
>  
> 

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

[PATCH v2 06/11] cputlb: ensure we save the IOTLB data in case of reset

[Alex Bennée]

Any write to a device might cause a re-arrangement of memory
triggering a TLB flush and potential re-size of the TLB invalidating
previous entries. This would cause users of qemu_plugin_get_hwaddr()
to see the warning:

  invalid use of qemu_plugin_get_hwaddr

because of the failed tlb_lookup which should always succeed. To
prevent this we save the IOTLB data in case it is later needed by a
plugin doing a lookup.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - save the entry instead of re-running the tlb_fill.
v3
  - don't abuse TLS, use CPUState to store data
  - just use g_free_rcu() to avoid ugliness
  - verify addr matches before returning data
  - ws fix
v4
  - don't both with RCU, just store it in CPUState
  - clean-up #ifdef'ery
  - checkpatch
---
 include/hw/core/cpu.h   | 16 ++++++++++++++++
 include/qemu/typedefs.h |  1 +
 accel/tcg/cputlb.c      | 38 +++++++++++++++++++++++++++++++++++---
 3 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 5542577d2b..8f145733ce 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -259,6 +259,18 @@ struct CPUWatchpoint {
     QTAILQ_ENTRY(CPUWatchpoint) entry;
 };
 
+#ifdef CONFIG_PLUGIN
+/*
+ * For plugins we sometime need to save the resolved iotlb data before
+ * the memory regions get moved around  by io_writex.
+ */
+typedef struct SavedIOTLB {
+    hwaddr addr;
+    MemoryRegionSection *section;
+    hwaddr mr_offset;
+} SavedIOTLB;
+#endif
+
 struct KVMState;
 struct kvm_run;
 
@@ -417,7 +429,11 @@ struct CPUState {
 
     DECLARE_BITMAP(plugin_mask, QEMU_PLUGIN_EV_MAX);
 
+#ifdef CONFIG_PLUGIN
     GArray *plugin_mem_cbs;
+    /* saved iotlb data from io_writex */
+    SavedIOTLB saved_iotlb;
+#endif
 
     /* TODO Move common fields from CPUArchState here. */
     int cpu_index;
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 15f5047bf1..427027a970 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -116,6 +116,7 @@ typedef struct QObject QObject;
 typedef struct QString QString;
 typedef struct RAMBlock RAMBlock;
 typedef struct Range Range;
+typedef struct SavedIOTLB SavedIOTLB;
 typedef struct SHPCDevice SHPCDevice;
 typedef struct SSIBus SSIBus;
 typedef struct VirtIODevice VirtIODevice;
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 1e815357c7..d370aedb47 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1073,6 +1073,24 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     return val;
 }
 
+/*
+ * Save a potentially trashed IOTLB entry for later lookup by plugin.
+ *
+ * We also need to track the thread storage address because the RCU
+ * cleanup that runs when we leave the critical region (the current
+ * execution) is actually in a different thread.
+ */
+static void save_iotlb_data(CPUState *cs, hwaddr addr,
+                            MemoryRegionSection *section, hwaddr mr_offset)
+{
+#ifdef CONFIG_PLUGIN
+    SavedIOTLB *saved = &cs->saved_iotlb;
+    saved->addr = addr;
+    saved->section = section;
+    saved->mr_offset = mr_offset;
+#endif
+}
+
 static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
                       int mmu_idx, uint64_t val, target_ulong addr,
                       uintptr_t retaddr, MemOp op)
@@ -1092,6 +1110,12 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     }
     cpu->mem_io_pc = retaddr;
 
+    /*
+     * The memory_region_dispatch may trigger a flush/resize
+     * so for plugins we save the iotlb_data just in case.
+     */
+    save_iotlb_data(cpu, iotlbentry->addr, section, mr_offset);
+
     if (mr->global_locking && !qemu_mutex_iothread_locked()) {
         qemu_mutex_lock_iothread();
         locked = true;
@@ -1381,8 +1405,11 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
  * in the softmmu lookup code (or helper). We don't handle re-fills or
  * checking the victim table. This is purely informational.
  *
- * This should never fail as the memory access being instrumented
- * should have just filled the TLB.
+ * This almost never fails as the memory access being instrumented
+ * should have just filled the TLB. The one corner case is io_writex
+ * which can cause TLB flushes and potential resizing of the TLBs
+ * loosing the information we need. In those cases we need to recover
+ * data from a copy of the io_tlb entry.
  */
 
 bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
@@ -1406,8 +1433,13 @@ bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
             data->v.ram.hostaddr = addr + tlbe->addend;
         }
         return true;
+    } else {
+        SavedIOTLB *saved = &cpu->saved_iotlb;
+        data->is_io = true;
+        data->v.io.section = saved->section;
+        data->v.io.offset = saved->mr_offset;
+        return true;
     }
-    return false;
 }
 
 #endif
-- 
2.20.1

Re: [PATCH v2 06/11] cputlb: ensure we save the IOTLB data in case of reset

[Richard Henderson]

On 7/13/20 1:04 PM, Alex Bennée wrote:
> Any write to a device might cause a re-arrangement of memory
> triggering a TLB flush and potential re-size of the TLB invalidating
> previous entries. This would cause users of qemu_plugin_get_hwaddr()
> to see the warning:
> 
>   invalid use of qemu_plugin_get_hwaddr
> 
> because of the failed tlb_lookup which should always succeed. To
> prevent this we save the IOTLB data in case it is later needed by a
> plugin doing a lookup.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

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

r~

Re: [PATCH v2 06/11] cputlb: ensure we save the IOTLB data in case of reset

[Emilio G. Cota]

On Mon, Jul 13, 2020 at 21:04:10 +0100, Alex Bennée wrote:
> Any write to a device might cause a re-arrangement of memory
> triggering a TLB flush and potential re-size of the TLB invalidating
> previous entries. This would cause users of qemu_plugin_get_hwaddr()
> to see the warning:
> 
>   invalid use of qemu_plugin_get_hwaddr
> 
> because of the failed tlb_lookup which should always succeed. To
> prevent this we save the IOTLB data in case it is later needed by a
> plugin doing a lookup.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
(snip)
> +/*
> + * Save a potentially trashed IOTLB entry for later lookup by plugin.
> + *
> + * We also need to track the thread storage address because the RCU
> + * cleanup that runs when we leave the critical region (the current
> + * execution) is actually in a different thread.

As I mentioned in the previous iteration of this series, this comment is
outdated -- there is no thread storage nor RCU to worry about here.

> + * This almost never fails as the memory access being instrumented
> + * should have just filled the TLB. The one corner case is io_writex
> + * which can cause TLB flushes and potential resizing of the TLBs
> + * loosing the information we need. In those cases we need to recover
> + * data from a copy of the io_tlb entry.
>   */

s/loosing/losing/

About the approach in this patch: it works as long as the caller is in
the same vCPU thread, otherwise we'd need a seqlock to avoid races
between readers and the writing vCPU. I see that qemu_plugin_get_hwaddr
does not even take a vCPU index, so this should be OK -- as long as this
is called only from a mem callback, it's in the same vCPU thread and it's
therefore safe.

With the above comments fixed,

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

Thanks,
		Emilio

[PATCH v2 07/11] plugins: expand the bb plugin to be thread safe and track per-cpu

[Alex Bennée]

While there isn't any easy way to make the inline counts thread safe
we can ensure the callback based ones are. While we are at it we can
reduce introduce a new option ("idle") to dump a report of the current
bb and insn count each time a vCPU enters the idle state.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Robert Foley <robert.foley@linaro.org>
Cc: Dave Bort <dbort@dbort.com>

---
v2
  - fixup for non-inline linux-user case
  - minor cleanup and re-factor
v3
  - checkpatch
---
 tests/plugin/bb.c | 97 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 84 insertions(+), 13 deletions(-)

diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c
index df19fd359d..e4cc7fdd6e 100644
--- a/tests/plugin/bb.c
+++ b/tests/plugin/bb.c
@@ -16,24 +16,67 @@
 
 QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
 
-static uint64_t bb_count;
-static uint64_t insn_count;
+typedef struct {
+    GMutex lock;
+    int index;
+    uint64_t bb_count;
+    uint64_t insn_count;
+} CPUCount;
+
+/* Used by the inline & linux-user counts */
 static bool do_inline;
+static CPUCount inline_count;
+
+/* Dump running CPU total on idle? */
+static bool idle_report;
+static GPtrArray *counts;
+static int max_cpus;
+
+static void gen_one_cpu_report(CPUCount *count, GString *report)
+{
+    if (count->bb_count) {
+        g_string_append_printf(report, "CPU%d: "
+                               "bb's: %" PRIu64", insns: %" PRIu64 "\n",
+                               count->index,
+                               count->bb_count, count->insn_count);
+    }
+}
 
 static void plugin_exit(qemu_plugin_id_t id, void *p)
 {
-    g_autofree gchar *out = g_strdup_printf(
-        "bb's: %" PRIu64", insns: %" PRIu64 "\n",
-        bb_count, insn_count);
-    qemu_plugin_outs(out);
+    g_autoptr(GString) report = g_string_new("");
+
+    if (do_inline || !max_cpus) {
+        g_string_printf(report, "bb's: %" PRIu64", insns: %" PRIu64 "\n",
+                        inline_count.bb_count, inline_count.insn_count);
+    } else {
+        g_ptr_array_foreach(counts, (GFunc) gen_one_cpu_report, report);
+    }
+    qemu_plugin_outs(report->str);
+}
+
+static void vcpu_idle(qemu_plugin_id_t id, unsigned int cpu_index)
+{
+    CPUCount *count = g_ptr_array_index(counts, cpu_index);
+    g_autoptr(GString) report = g_string_new("");
+    gen_one_cpu_report(count, report);
+
+    if (report->len > 0) {
+        g_string_prepend(report, "Idling ");
+        qemu_plugin_outs(report->str);
+    }
 }
 
 static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
 {
-    unsigned long n_insns = (unsigned long)udata;
+    CPUCount *count = max_cpus ?
+        g_ptr_array_index(counts, cpu_index) : &inline_count;
 
-    insn_count += n_insns;
-    bb_count++;
+    unsigned long n_insns = (unsigned long)udata;
+    g_mutex_lock(&count->lock);
+    count->insn_count += n_insns;
+    count->bb_count++;
+    g_mutex_unlock(&count->lock);
 }
 
 static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
@@ -42,9 +85,10 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
 
     if (do_inline) {
         qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
-                                                 &bb_count, 1);
+                                                 &inline_count.bb_count, 1);
         qemu_plugin_register_vcpu_tb_exec_inline(tb, QEMU_PLUGIN_INLINE_ADD_U64,
-                                                 &insn_count, n_insns);
+                                                 &inline_count.insn_count,
+                                                 n_insns);
     } else {
         qemu_plugin_register_vcpu_tb_exec_cb(tb, vcpu_tb_exec,
                                              QEMU_PLUGIN_CB_NO_REGS,
@@ -56,8 +100,35 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
                                            const qemu_info_t *info,
                                            int argc, char **argv)
 {
-    if (argc && strcmp(argv[0], "inline") == 0) {
-        do_inline = true;
+    int i;
+
+    for (i = 0; i < argc; i++) {
+        char *opt = argv[i];
+        if (g_strcmp0(opt, "inline") == 0) {
+            do_inline = true;
+        } else if (g_strcmp0(opt, "idle") == 0) {
+            idle_report = true;
+        } else {
+            fprintf(stderr, "option parsing failed: %s\n", opt);
+            return -1;
+        }
+    }
+
+    if (info->system_emulation && !do_inline) {
+        max_cpus = info->system.max_vcpus;
+        counts = g_ptr_array_new();
+        for (i = 0; i < max_cpus; i++) {
+            CPUCount *count = g_new0(CPUCount, 1);
+            g_mutex_init(&count->lock);
+            count->index = i;
+            g_ptr_array_add(counts, count);
+        }
+    } else if (!do_inline) {
+        g_mutex_init(&inline_count.lock);
+    }
+
+    if (idle_report) {
+        qemu_plugin_register_vcpu_idle_cb(id, vcpu_idle);
     }
 
     qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
-- 
2.20.1

[PATCH v2 08/11] docs/devel: fix grammar in multi-thread-tcg

[Alex Bennée]

Review comment came just too late ;-)

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/multi-thread-tcg.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/devel/multi-thread-tcg.rst b/docs/devel/multi-thread-tcg.rst
index 42158b77c7..21483870db 100644
--- a/docs/devel/multi-thread-tcg.rst
+++ b/docs/devel/multi-thread-tcg.rst
@@ -107,7 +107,7 @@ including:
 
   - debugging operations (breakpoint insertion/removal)
   - some CPU helper functions
-  - linux-user spawning it's first thread
+  - linux-user spawning its first thread
 
 This is done with the async_safe_run_on_cpu() mechanism to ensure all
 vCPUs are quiescent when changes are being made to shared global
-- 
2.20.1

Re: [PATCH v2 08/11] docs/devel: fix grammar in multi-thread-tcg

[Richard Henderson]

On 7/13/20 1:04 PM, Alex Bennée wrote:
> Review comment came just too late ;-)
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  docs/devel/multi-thread-tcg.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

r~

Re: [PATCH v2 08/11] docs/devel: fix grammar in multi-thread-tcg

[Thomas Huth]

On 13/07/2020 22.04, Alex Bennée wrote:
> Review comment came just too late ;-)
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  docs/devel/multi-thread-tcg.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/devel/multi-thread-tcg.rst b/docs/devel/multi-thread-tcg.rst
> index 42158b77c7..21483870db 100644
> --- a/docs/devel/multi-thread-tcg.rst
> +++ b/docs/devel/multi-thread-tcg.rst
> @@ -107,7 +107,7 @@ including:
>  
>    - debugging operations (breakpoint insertion/removal)
>    - some CPU helper functions
> -  - linux-user spawning it's first thread
> +  - linux-user spawning its first thread
>  
>  This is done with the async_safe_run_on_cpu() mechanism to ensure all
>  vCPUs are quiescent when changes are being made to shared global
> 

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

Re: [PATCH v2 08/11] docs/devel: fix grammar in multi-thread-tcg

[Philippe Mathieu-Daudé]

On 7/13/20 10:04 PM, Alex Bennée wrote:
> Review comment came just too late ;-)
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  docs/devel/multi-thread-tcg.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/devel/multi-thread-tcg.rst b/docs/devel/multi-thread-tcg.rst
> index 42158b77c7..21483870db 100644
> --- a/docs/devel/multi-thread-tcg.rst
> +++ b/docs/devel/multi-thread-tcg.rst
> @@ -107,7 +107,7 @@ including:
>  
>    - debugging operations (breakpoint insertion/removal)
>    - some CPU helper functions
> -  - linux-user spawning it's first thread
> +  - linux-user spawning its first thread
>  
>  This is done with the async_safe_run_on_cpu() mechanism to ensure all
>  vCPUs are quiescent when changes are being made to shared global
> 

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

[PATCH v2 09/11] hw/virtio/pci: include vdev name in registered PCI sections

[Alex Bennée]

When viewing/debugging memory regions it is sometimes hard to figure
out which PCI device something belongs to. Make the names unique by
including the vdev name in the name string.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

---
v2
  - swap ()'s for an extra -
---
 hw/virtio/virtio-pci.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 8554cf2a03..215e680c71 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1406,7 +1406,8 @@ static void virtio_pci_device_write(void *opaque, hwaddr addr,
     }
 }
 
-static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
+static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy,
+                                           const char *vdev_name)
 {
     static const MemoryRegionOps common_ops = {
         .read = virtio_pci_common_read,
@@ -1453,36 +1454,41 @@ static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
         },
         .endianness = DEVICE_LITTLE_ENDIAN,
     };
+    g_autoptr(GString) name = g_string_new(NULL);
 
-
+    g_string_printf(name, "virtio-pci-common-%s", vdev_name);
     memory_region_init_io(&proxy->common.mr, OBJECT(proxy),
                           &common_ops,
                           proxy,
-                          "virtio-pci-common",
+                          name->str,
                           proxy->common.size);
 
+    g_string_printf(name, "virtio-pci-isr-%s", vdev_name);
     memory_region_init_io(&proxy->isr.mr, OBJECT(proxy),
                           &isr_ops,
                           proxy,
-                          "virtio-pci-isr",
+                          name->str,
                           proxy->isr.size);
 
+    g_string_printf(name, "virtio-pci-device-%s", vdev_name);
     memory_region_init_io(&proxy->device.mr, OBJECT(proxy),
                           &device_ops,
                           virtio_bus_get_device(&proxy->bus),
-                          "virtio-pci-device",
+                          name->str,
                           proxy->device.size);
 
+    g_string_printf(name, "virtio-pci-notify-%s", vdev_name);
     memory_region_init_io(&proxy->notify.mr, OBJECT(proxy),
                           &notify_ops,
                           virtio_bus_get_device(&proxy->bus),
-                          "virtio-pci-notify",
+                          name->str,
                           proxy->notify.size);
 
+    g_string_printf(name, "virtio-pci-notify-pio-%s", vdev_name);
     memory_region_init_io(&proxy->notify_pio.mr, OBJECT(proxy),
                           &notify_pio_ops,
                           virtio_bus_get_device(&proxy->bus),
-                          "virtio-pci-notify-pio",
+                          name->str,
                           proxy->notify_pio.size);
 }
 
@@ -1623,7 +1629,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
 
         struct virtio_pci_cfg_cap *cfg_mask;
 
-        virtio_pci_modern_regions_init(proxy);
+        virtio_pci_modern_regions_init(proxy, vdev->name);
 
         virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap);
         virtio_pci_modern_mem_region_map(proxy, &proxy->isr, &cap);
-- 
2.20.1

Re: [PATCH v2 09/11] hw/virtio/pci: include vdev name in registered PCI sections

[Michael S. Tsirkin]

On Mon, Jul 13, 2020 at 09:04:13PM +0100, Alex Bennée wrote:
> When viewing/debugging memory regions it is sometimes hard to figure
> out which PCI device something belongs to. Make the names unique by
> including the vdev name in the name string.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

I wonder here: why don't we see the owner name when debugging? Maybe that's a better
way to address that.

> ---
> v2
>   - swap ()'s for an extra -
> ---
>  hw/virtio/virtio-pci.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 8554cf2a03..215e680c71 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1406,7 +1406,8 @@ static void virtio_pci_device_write(void *opaque, hwaddr addr,
>      }
>  }
>  
> -static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
> +static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy,
> +                                           const char *vdev_name)
>  {
>      static const MemoryRegionOps common_ops = {
>          .read = virtio_pci_common_read,
> @@ -1453,36 +1454,41 @@ static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
>          },
>          .endianness = DEVICE_LITTLE_ENDIAN,
>      };
> +    g_autoptr(GString) name = g_string_new(NULL);
>  
> -
> +    g_string_printf(name, "virtio-pci-common-%s", vdev_name);
>      memory_region_init_io(&proxy->common.mr, OBJECT(proxy),
>                            &common_ops,
>                            proxy,
> -                          "virtio-pci-common",
> +                          name->str,
>                            proxy->common.size);
>  
> +    g_string_printf(name, "virtio-pci-isr-%s", vdev_name);
>      memory_region_init_io(&proxy->isr.mr, OBJECT(proxy),
>                            &isr_ops,
>                            proxy,
> -                          "virtio-pci-isr",
> +                          name->str,
>                            proxy->isr.size);
>  
> +    g_string_printf(name, "virtio-pci-device-%s", vdev_name);
>      memory_region_init_io(&proxy->device.mr, OBJECT(proxy),
>                            &device_ops,
>                            virtio_bus_get_device(&proxy->bus),
> -                          "virtio-pci-device",
> +                          name->str,
>                            proxy->device.size);
>  
> +    g_string_printf(name, "virtio-pci-notify-%s", vdev_name);
>      memory_region_init_io(&proxy->notify.mr, OBJECT(proxy),
>                            &notify_ops,
>                            virtio_bus_get_device(&proxy->bus),
> -                          "virtio-pci-notify",
> +                          name->str,
>                            proxy->notify.size);
>  
> +    g_string_printf(name, "virtio-pci-notify-pio-%s", vdev_name);
>      memory_region_init_io(&proxy->notify_pio.mr, OBJECT(proxy),
>                            &notify_pio_ops,
>                            virtio_bus_get_device(&proxy->bus),
> -                          "virtio-pci-notify-pio",
> +                          name->str,
>                            proxy->notify_pio.size);
>  }
>  
> @@ -1623,7 +1629,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>  
>          struct virtio_pci_cfg_cap *cfg_mask;
>  
> -        virtio_pci_modern_regions_init(proxy);
> +        virtio_pci_modern_regions_init(proxy, vdev->name);
>  
>          virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap);
>          virtio_pci_modern_mem_region_map(proxy, &proxy->isr, &cap);
> -- 
> 2.20.1

Re: [PATCH v2 09/11] hw/virtio/pci: include vdev name in registered PCI sections

[Philippe Mathieu-Daudé]

On 7/14/20 11:07 AM, Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2020 at 09:04:13PM +0100, Alex Bennée wrote:
>> When viewing/debugging memory regions it is sometimes hard to figure
>> out which PCI device something belongs to. Make the names unique by
>> including the vdev name in the name string.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> I wonder here: why don't we see the owner name when debugging? Maybe that's a better
> way to address that.

It depends what you mean mean by "debugging", gdb? trace events?
monitor? Something else?

From the monitor you can use the '-o' option:

(qemu) help info mtree
info mtree [-f][-d][-o][-D] -- show memory tree (-f: dump flat view for
address spaces;-d: dump dispatch tree, valid with -f only);-o: dump
region owners/parents;-D: dump disabled regions
(qemu) info mtree -o
address-space: memory
  0000000000000000-ffffffffffffffff (prio 0, i/o): system parent:{obj
path=/machine/unattached}
    0000000000000000-0000000007ffffff (prio 0, ram): alias ram-below-4g
@pc.ram 0000000000000000-0000000007ffffff parent:{obj
path=/machine/unattached}
    0000000000000000-ffffffffffffffff (prio -1, i/o): pci parent:{obj
path=/machine/unattached}
      00000000000a0000-00000000000bffff (prio 1, i/o): vga-lowmem
owner:{dev path=/machine/unattached/device[28]}
      00000000000c0000-00000000000dffff (prio 1, rom): pc.rom
parent:{obj path=/machine/unattached}
      00000000000e0000-00000000000fffff (prio 1, rom): alias isa-bios
@pc.bios 0000000000020000-000000000003ffff parent:{obj
path=/machine/unattached}
      00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios
parent:{obj path=/machine/unattached}
    00000000000a0000-00000000000bffff (prio 1, i/o): alias smram-region
@pci 00000000000a0000-00000000000bffff owner:{dev path=/machine/q35/mch}
    00000000000c0000-00000000000c3fff (prio 1, i/o): alias pam-pci @pci
00000000000c0000-00000000000c3fff owner:{dev path=/machine/q35/mch}
    00000000000c4000-00000000000c7fff (prio 1, i/o): alias pam-pci @pci
00000000000c4000-00000000000c7fff owner:{dev path=/machine/q35/mch}
    00000000000c8000-00000000000cbfff (prio 1, i/o): alias pam-pci @pci
00000000000c8000-00000000000cbfff owner:{dev path=/machine/q35/mch}
    00000000000cc000-00000000000cffff (prio 1, i/o): alias pam-pci @pci
00000000000cc000-00000000000cffff owner:{dev path=/machine/q35/mch}
    00000000000d0000-00000000000d3fff (prio 1, i/o): alias pam-pci @pci
00000000000d0000-00000000000d3fff owner:{dev path=/machine/q35/mch}
    00000000000d4000-00000000000d7fff (prio 1, i/o): alias pam-pci @pci
00000000000d4000-00000000000d7fff owner:{dev path=/machine/q35/mch}
    00000000000d8000-00000000000dbfff (prio 1, i/o): alias pam-pci @pci
00000000000d8000-00000000000dbfff owner:{dev path=/machine/q35/mch}
    00000000000dc000-00000000000dffff (prio 1, i/o): alias pam-pci @pci
00000000000dc000-00000000000dffff owner:{dev path=/machine/q35/mch}
    00000000000e0000-00000000000e3fff (prio 1, i/o): alias pam-pci @pci
00000000000e0000-00000000000e3fff owner:{dev path=/machine/q35/mch}
    00000000000e4000-00000000000e7fff (prio 1, i/o): alias pam-pci @pci
00000000000e4000-00000000000e7fff owner:{dev path=/machine/q35/mch}
    00000000000e8000-00000000000ebfff (prio 1, i/o): alias pam-pci @pci
00000000000e8000-00000000000ebfff owner:{dev path=/machine/q35/mch}
    00000000000ec000-00000000000effff (prio 1, i/o): alias pam-pci @pci
00000000000ec000-00000000000effff owner:{dev path=/machine/q35/mch}
    00000000000f0000-00000000000fffff (prio 1, i/o): alias pam-pci @pci
00000000000f0000-00000000000fffff owner:{dev path=/machine/q35/mch}
    00000000fec00000-00000000fec00fff (prio 0, i/o): ioapic owner:{dev
path=/machine/q35/ioapic}
    00000000fed00000-00000000fed003ff (prio 0, i/o): hpet owner:{dev
path=/machine/unattached/device[5]}
    00000000fee00000-00000000feefffff (prio 4096, i/o): apic-msi
owner:{dev path=/machine/unattached/device[0]/lapic}

For trace events it has been suggested to use
object_get_canonical_path_component() but there is a discussion about if
it is practical or not in its current state, see:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg722110.html

For gdb I guess remember Marc-André sent a patch for the Python script,
but it hasn't been merged apparently:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg659085.html

> 
>> ---
>> v2
>>   - swap ()'s for an extra -
>> ---
>>  hw/virtio/virtio-pci.c | 22 ++++++++++++++--------
>>  1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 8554cf2a03..215e680c71 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1406,7 +1406,8 @@ static void virtio_pci_device_write(void *opaque, hwaddr addr,
>>      }
>>  }
>>  
>> -static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
>> +static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy,
>> +                                           const char *vdev_name)
>>  {
>>      static const MemoryRegionOps common_ops = {
>>          .read = virtio_pci_common_read,
>> @@ -1453,36 +1454,41 @@ static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
>>          },
>>          .endianness = DEVICE_LITTLE_ENDIAN,
>>      };
>> +    g_autoptr(GString) name = g_string_new(NULL);
>>  
>> -
>> +    g_string_printf(name, "virtio-pci-common-%s", vdev_name);
>>      memory_region_init_io(&proxy->common.mr, OBJECT(proxy),
>>                            &common_ops,
>>                            proxy,
>> -                          "virtio-pci-common",
>> +                          name->str,
>>                            proxy->common.size);
>>  
>> +    g_string_printf(name, "virtio-pci-isr-%s", vdev_name);
>>      memory_region_init_io(&proxy->isr.mr, OBJECT(proxy),
>>                            &isr_ops,
>>                            proxy,
>> -                          "virtio-pci-isr",
>> +                          name->str,
>>                            proxy->isr.size);
>>  
>> +    g_string_printf(name, "virtio-pci-device-%s", vdev_name);
>>      memory_region_init_io(&proxy->device.mr, OBJECT(proxy),
>>                            &device_ops,
>>                            virtio_bus_get_device(&proxy->bus),
>> -                          "virtio-pci-device",
>> +                          name->str,
>>                            proxy->device.size);
>>  
>> +    g_string_printf(name, "virtio-pci-notify-%s", vdev_name);
>>      memory_region_init_io(&proxy->notify.mr, OBJECT(proxy),
>>                            &notify_ops,
>>                            virtio_bus_get_device(&proxy->bus),
>> -                          "virtio-pci-notify",
>> +                          name->str,
>>                            proxy->notify.size);
>>  
>> +    g_string_printf(name, "virtio-pci-notify-pio-%s", vdev_name);
>>      memory_region_init_io(&proxy->notify_pio.mr, OBJECT(proxy),
>>                            &notify_pio_ops,
>>                            virtio_bus_get_device(&proxy->bus),
>> -                          "virtio-pci-notify-pio",
>> +                          name->str,
>>                            proxy->notify_pio.size);
>>  }
>>  
>> @@ -1623,7 +1629,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>>  
>>          struct virtio_pci_cfg_cap *cfg_mask;
>>  
>> -        virtio_pci_modern_regions_init(proxy);
>> +        virtio_pci_modern_regions_init(proxy, vdev->name);
>>  
>>          virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap);
>>          virtio_pci_modern_mem_region_map(proxy, &proxy->isr, &cap);
>> -- 
>> 2.20.1
> 

[PATCH v2 10/11] plugins: add API to return a name for a IO device

[Alex Bennée]

This may well end up being anonymous but it should always be unique.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
[r-b provisional given change to g_intern_string]
Reviewed-by: Clement Deschamps <clement.deschamps@greensocs.com>
Reviewed-by: Emilio G. Cota <cota@braap.org>

---
v3
  - return a non-freeable const g_intern_string()
  - checkpatch cleanups
---
 include/qemu/qemu-plugin.h |  6 ++++++
 plugins/api.c              | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index bab8b0d4b3..c98c18d6b0 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -335,6 +335,12 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
 bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
 uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr);
 
+/*
+ * Returns a string representing the device. The string is valid for
+ * the lifetime of the plugin.
+ */
+const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h);
+
 typedef void
 (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
                              qemu_plugin_meminfo_t info, uint64_t vaddr,
diff --git a/plugins/api.c b/plugins/api.c
index bbdc5a4eb4..4304e63f0c 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -303,6 +303,26 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
     return 0;
 }
 
+const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h)
+{
+#ifdef CONFIG_SOFTMMU
+    if (h && h->is_io) {
+        MemoryRegionSection *mrs = h->v.io.section;
+        if (!mrs->mr->name) {
+            unsigned long maddr = 0xffffffff & (uintptr_t) mrs->mr;
+            g_autofree char *temp = g_strdup_printf("anon%08lx", maddr);
+            return g_intern_string(temp);
+        } else {
+            return g_intern_string(mrs->mr->name);
+        }
+    } else {
+        return g_intern_string("RAM");
+    }
+#else
+    return g_intern_string("Invalid");
+#endif
+}
+
 /*
  * Queries to the number and potential maximum number of vCPUs there
  * will be. This helps the plugin dimension per-vcpu arrays.
-- 
2.20.1

Re: [PATCH v2 10/11] plugins: add API to return a name for a IO device

[Richard Henderson]

On 7/13/20 1:04 PM, Alex Bennée wrote:
> This may well end up being anonymous but it should always be unique.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> [r-b provisional given change to g_intern_string]
> Reviewed-by: Clement Deschamps <clement.deschamps@greensocs.com>
> Reviewed-by: Emilio G. Cota <cota@braap.org>
> 
> ---
> v3
>   - return a non-freeable const g_intern_string()
>   - checkpatch cleanups
> ---
>  include/qemu/qemu-plugin.h |  6 ++++++
>  plugins/api.c              | 20 ++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index bab8b0d4b3..c98c18d6b0 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -335,6 +335,12 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
>  bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
>  uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr);
>  
> +/*
> + * Returns a string representing the device. The string is valid for
> + * the lifetime of the plugin.
> + */
> +const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h);
> +
>  typedef void
>  (*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
>                               qemu_plugin_meminfo_t info, uint64_t vaddr,
> diff --git a/plugins/api.c b/plugins/api.c
> index bbdc5a4eb4..4304e63f0c 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -303,6 +303,26 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
>      return 0;
>  }
>  
> +const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h)
> +{
> +#ifdef CONFIG_SOFTMMU
> +    if (h && h->is_io) {
> +        MemoryRegionSection *mrs = h->v.io.section;
> +        if (!mrs->mr->name) {
> +            unsigned long maddr = 0xffffffff & (uintptr_t) mrs->mr;
> +            g_autofree char *temp = g_strdup_printf("anon%08lx", maddr);
> +            return g_intern_string(temp);
> +        } else {
> +            return g_intern_string(mrs->mr->name);
> +        }
> +    } else {
> +        return g_intern_string("RAM");

g_intern_static_string.

> +    }
> +#else
> +    return g_intern_string("Invalid");

Likewise.

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

r~

[PATCH v2 11/11] plugins: new hwprofile plugin

[Alex Bennée]

This is a plugin intended to help with profiling access to various
bits of system hardware. It only really makes sense for system
emulation.

It takes advantage of the recently exposed helper API that allows us
to see the device name (memory region name) associated with a device.

You can specify arg=read or arg=write to limit the tracking to just
reads or writes (by default it does both).

The pattern option:

  -plugin ./tests/plugin/libhwprofile.so,arg=pattern

will allow you to see the access pattern to devices, eg:

  gic_cpu @ 0xffffffc010040000
    off:00000000, 8, 1, 8, 1
    off:00000000, 4, 1, 4, 1
    off:00000000, 2, 1, 2, 1
    off:00000000, 1, 1, 1, 1

The source option:

  -plugin ./tests/plugin/libhwprofile.so,arg=source

will track the virtual source address of the instruction making the
access:

  pl011 @ 0xffffffc010031000
    pc:ffffffc0104c785c, 1, 4, 0, 0
    pc:ffffffc0104c7898, 1, 4, 0, 0
    pc:ffffffc010512bcc, 2, 1867, 0, 0

You cannot mix source and pattern.

Finally the match option allow you to limit the tracking to just the
devices you care about.

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

---
v2
  - use 64 bit cpu masks
  - warn if we will exceed cpu mask capability
  - don't run in linux-user mode
  - skip in check-tcg for linux-user test
v3
  - update device name API
  - re-factor and clean-up
  - add source tracking
  - use direct hash for source/pattern
  - add match function
  - expand the commit message
  - checkpatch cleanups
---
 tests/plugin/hwprofile.c  | 305 ++++++++++++++++++++++++++++++++++++++
 tests/plugin/Makefile     |   1 +
 tests/tcg/Makefile.target |  12 +-
 3 files changed, 316 insertions(+), 2 deletions(-)
 create mode 100644 tests/plugin/hwprofile.c

diff --git a/tests/plugin/hwprofile.c b/tests/plugin/hwprofile.c
new file mode 100644
index 0000000000..6dac1d5f85
--- /dev/null
+++ b/tests/plugin/hwprofile.c
@@ -0,0 +1,305 @@
+/*
+ * Copyright (C) 2020, Alex Bennée <alex.bennee@linaro.org>
+ *
+ * HW Profile - breakdown access patterns for IO to devices
+ *
+ * License: GNU GPL, version 2 or later.
+ *   See the COPYING file in the top-level directory.
+ */
+
+#include <inttypes.h>
+#include <assert.h>
+#include <stdlib.h>
+#include <inttypes.h>
+#include <string.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <glib.h>
+
+#include <qemu-plugin.h>
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+
+typedef struct {
+    uint64_t cpu_read;
+    uint64_t cpu_write;
+    uint64_t reads;
+    uint64_t writes;
+} IOCounts;
+
+typedef struct {
+    uint64_t off_or_pc;
+    IOCounts counts;
+} IOLocationCounts;
+
+typedef struct {
+    const char *name;
+    uint64_t   base;
+    IOCounts   totals;
+    GHashTable *detail;
+} DeviceCounts;
+
+static GMutex lock;
+static GHashTable *devices;
+
+/* track the access pattern to a piece of HW */
+static bool pattern;
+/* track the source address of access to HW */
+static bool source;
+/* track only matched regions of HW */
+static bool check_match;
+static gchar **matches;
+
+static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
+
+static inline bool track_reads(void)
+{
+    return rw == QEMU_PLUGIN_MEM_RW || rw == QEMU_PLUGIN_MEM_R;
+}
+
+static inline bool track_writes(void)
+{
+    return rw == QEMU_PLUGIN_MEM_RW || rw == QEMU_PLUGIN_MEM_W;
+}
+
+static void plugin_init(void)
+{
+    devices = g_hash_table_new(NULL, NULL);
+}
+
+static gint sort_cmp(gconstpointer a, gconstpointer b)
+{
+    DeviceCounts *ea = (DeviceCounts *) a;
+    DeviceCounts *eb = (DeviceCounts *) b;
+    return ea->totals.reads + ea->totals.writes >
+           eb->totals.reads + eb->totals.writes ? -1 : 1;
+}
+
+static gint sort_loc(gconstpointer a, gconstpointer b)
+{
+    IOLocationCounts *ea = (IOLocationCounts *) a;
+    IOLocationCounts *eb = (IOLocationCounts *) b;
+    return ea->off_or_pc > eb->off_or_pc;
+}
+
+static void fmt_iocount_record(GString *s, IOCounts *rec)
+{
+    if (track_reads()) {
+        g_string_append_printf(s, ", %"PRIx64", %"PRId64,
+                               rec->cpu_read, rec->reads);
+    }
+    if (track_writes()) {
+        g_string_append_printf(s, ", %"PRIx64", %"PRId64,
+                               rec->cpu_write, rec->writes);
+    }
+}
+
+static void fmt_dev_record(GString *s, DeviceCounts *rec)
+{
+    g_string_append_printf(s, "%s, 0x%"PRIx64,
+                           rec->name, rec->base);
+    fmt_iocount_record(s, &rec->totals);
+    g_string_append_c(s, '\n');
+}
+
+static void plugin_exit(qemu_plugin_id_t id, void *p)
+{
+    g_autoptr(GString) report = g_string_new("");
+    GList *counts;
+
+    if (!(pattern || source)) {
+        g_string_printf(report, "Device, Address");
+        if (track_reads()) {
+            g_string_append_printf(report, ", RCPUs, Reads");
+        }
+        if (track_writes()) {
+            g_string_append_printf(report, ",  WCPUs, Writes");
+        }
+        g_string_append_c(report, '\n');
+    }
+
+    counts = g_hash_table_get_values(devices);
+    if (counts && g_list_next(counts)) {
+        GList *it;
+
+        it = g_list_sort(counts, sort_cmp);
+
+        while (it) {
+            DeviceCounts *rec = (DeviceCounts *) it->data;
+            if (rec->detail) {
+                GList *accesses = g_hash_table_get_values(rec->detail);
+                GList *io_it = g_list_sort(accesses, sort_loc);
+                const char *prefix = pattern ? "off" : "pc";
+                g_string_append_printf(report, "%s @ 0x%"PRIx64"\n",
+                                       rec->name, rec->base);
+                while (io_it) {
+                    IOLocationCounts *loc = (IOLocationCounts *) io_it->data;
+                    g_string_append_printf(report, "  %s:%08"PRIx64,
+                                           prefix, loc->off_or_pc);
+                    fmt_iocount_record(report, &loc->counts);
+                    g_string_append_c(report, '\n');
+                    io_it = io_it->next;
+                }
+            } else {
+                fmt_dev_record(report, rec);
+            }
+            it = it->next;
+        };
+        g_list_free(it);
+    }
+
+    qemu_plugin_outs(report->str);
+}
+
+static DeviceCounts *new_count(const char *name, uint64_t base)
+{
+    DeviceCounts *count = g_new0(DeviceCounts, 1);
+    count->name = name;
+    count->base = base;
+    if (pattern || source) {
+        count->detail = g_hash_table_new(NULL, NULL);
+    }
+    g_hash_table_insert(devices, (gpointer) name, count);
+    return count;
+}
+
+static IOLocationCounts *new_location(GHashTable *table, uint64_t off_or_pc)
+{
+    IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
+    loc->off_or_pc = off_or_pc;
+    g_hash_table_insert(table, (gpointer) off_or_pc, loc);
+    return loc;
+}
+
+static void hwprofile_match_hit(DeviceCounts *rec, uint64_t off)
+{
+    g_autoptr(GString) report = g_string_new("hwprofile: match @ offset");
+    g_string_append_printf(report, "%"PRIx64", previous hits\n", off);
+    fmt_dev_record(report, rec);
+    qemu_plugin_outs(report->str);
+}
+
+static void inc_count(IOCounts *count, bool is_write, unsigned int cpu_index)
+{
+    if (is_write) {
+        count->writes++;
+        count->cpu_write |= (1 << cpu_index);
+    } else {
+        count->reads++;
+        count->cpu_read |= (1 << cpu_index);
+    }
+}
+
+static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
+                       uint64_t vaddr, void *udata)
+{
+    struct qemu_plugin_hwaddr *hwaddr = qemu_plugin_get_hwaddr(meminfo, vaddr);
+
+    if (!hwaddr || !qemu_plugin_hwaddr_is_io(hwaddr)) {
+        return;
+    } else {
+        const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
+        uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
+        bool is_write = qemu_plugin_mem_is_store(meminfo);
+        DeviceCounts *counts;
+
+        g_mutex_lock(&lock);
+        counts = (DeviceCounts *) g_hash_table_lookup(devices, name);
+
+        if (!counts) {
+            uint64_t base = vaddr - off;
+            counts = new_count(name, base);
+        }
+
+        if (check_match) {
+            if (g_strv_contains((const char * const *)matches, counts->name)) {
+                hwprofile_match_hit(counts, off);
+                inc_count(&counts->totals, is_write, cpu_index);
+            }
+        } else {
+            inc_count(&counts->totals, is_write, cpu_index);
+        }
+
+        /* either track offsets or source of access */
+        if (source) {
+            off = (uint64_t) udata;
+        }
+
+        if (pattern || source) {
+            IOLocationCounts *io_count = g_hash_table_lookup(counts->detail,
+                                                             (gpointer) off);
+            if (!io_count) {
+                io_count = new_location(counts->detail, off);
+            }
+            inc_count(&io_count->counts, is_write, cpu_index);
+        }
+
+        g_mutex_unlock(&lock);
+    }
+}
+
+static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
+{
+    size_t n = qemu_plugin_tb_n_insns(tb);
+    size_t i;
+
+    for (i = 0; i < n; i++) {
+        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
+        gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
+        qemu_plugin_register_vcpu_mem_cb(insn, vcpu_haddr,
+                                         QEMU_PLUGIN_CB_NO_REGS,
+                                         rw, udata);
+    }
+}
+
+QEMU_PLUGIN_EXPORT
+int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
+                        int argc, char **argv)
+{
+    int i;
+
+    for (i = 0; i < argc; i++) {
+        char *opt = argv[i];
+        if (g_strcmp0(opt, "read") == 0) {
+            rw = QEMU_PLUGIN_MEM_R;
+        } else if (g_strcmp0(opt, "write") == 0) {
+            rw = QEMU_PLUGIN_MEM_W;
+        } else if (g_strcmp0(opt, "pattern") == 0) {
+            pattern = true;
+        } else if (g_strcmp0(opt, "source") == 0) {
+            source = true;
+        } else if (g_str_has_prefix(opt, "match")) {
+            gchar **parts = g_strsplit(opt, "=", 2);
+            check_match = true;
+            matches = g_strsplit(parts[1], ",", -1);
+            g_strfreev(parts);
+        } else {
+            fprintf(stderr, "option parsing failed: %s\n", opt);
+            return -1;
+        }
+    }
+
+    if (source && pattern) {
+        fprintf(stderr, "can only currently track either source or pattern.\n");
+        return -1;
+    }
+
+    if (!info->system_emulation) {
+        fprintf(stderr, "hwprofile: plugin only useful for system emulation\n");
+        return -1;
+    }
+
+    /* Just warn about overflow */
+    if (info->system.smp_vcpus > 64 ||
+        info->system.max_vcpus > 64) {
+        fprintf(stderr, "hwprofile: can only track up to 64 CPUs\n");
+    }
+
+    plugin_init();
+
+    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
+    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
+    return 0;
+}
diff --git a/tests/plugin/Makefile b/tests/plugin/Makefile
index e9348fde4a..e9d1947751 100644
--- a/tests/plugin/Makefile
+++ b/tests/plugin/Makefile
@@ -21,6 +21,7 @@ NAMES += hotblocks
 NAMES += howvec
 NAMES += hotpages
 NAMES += lockstep
+NAMES += hwprofile
 
 SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
 
diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
index 4b2b696fce..bc595b0588 100644
--- a/tests/tcg/Makefile.target
+++ b/tests/tcg/Makefile.target
@@ -129,8 +129,16 @@ ifeq ($(CONFIG_PLUGIN),y)
 PLUGIN_SRC=$(SRC_PATH)/tests/plugin
 PLUGIN_LIB=../../plugin
 VPATH+=$(PLUGIN_LIB)
-PLUGINS=$(filter-out liblockstep.so,\
-		$(patsubst %.c, lib%.so, $(notdir $(wildcard $(PLUGIN_SRC)/*.c))))
+
+# Some plugins aren't testable automatically
+SKIP_PLUGINS=liblockstep.so
+ifdef CONFIG_USER_ONLY
+SKIP_PLUGINS+=libhwprofile.so
+endif
+
+PLUGINS=$(filter-out $(SKIP_PLUGINS),\
+		$(patsubst %.c, lib%.so, \
+			$(notdir $(wildcard $(PLUGIN_SRC)/*.c))))
 
 # We need to ensure expand the run-plugin-TEST-with-PLUGIN
 # pre-requistes manually here as we can't use stems to handle it. We
-- 
2.20.1