[PATCH v1 0/9] plugins/next (bug fixes, hwprofile, lockstep)

[PATCH v1 0/9] plugins/next (bug fixes, hwprofile, lockstep)

[Alex Bennée]

Hi,

This is the current state of my plugins tree. It has a few minor fixes
to the headers as well as a bug I found in the cputlb which was
triggered when a pci_config write via io_writex causes the memory
regions to be reset and as a result a flush and potential re-sizing of
the TLB entries. This meant when a plugin looked up details of the
address later there was no TLB entry with the information which got
flagged as an error on the plugins part.

I found the bug while I was looking around trying to figure out what
was going wrong with my virtio code so I implemented a "hwprofile"
plugin to track exactly what devices where seeing writes. There are
some minor associated tweaks to the virtio PCI code to better name the
MemoryRegions and a new helper API which exposes the region names to
the plugin.

The lockstep plugin is essentially unchanged from previous postings
but hasn't seen any review. I'm minded to include it in the next PR
anyway just so it isn't lost next time I need to do an A-B comparison
on something that only diverges in behaviour several million
instructions into an execution.

The following need review:

 - .travis.yml: allow failure for unreliable hosts
 - plugins: new hwprofile plugin
 - plugins: add API to return a name for a IO device
 - hw/virtio/pci: include vdev name in registered PCI sections
 - cputlb: ensure we re-fill the TLB if it has reset
 - tests/plugin: correctly honour io_count
 - plugins: new lockstep plugin for debugging TCG changes

Alex Bennée (7):
  plugins: new lockstep plugin for debugging TCG changes
  tests/plugin: correctly honour io_count
  cputlb: ensure we re-fill the TLB if it has reset
  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
  .travis.yml: allow failure for unreliable hosts

Emilio G. Cota (1):
  qemu-plugin.h: add missing include <stddef.h> to define size_t

Philippe Mathieu-Daudé (1):
  scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header

 include/qemu/qemu-plugin.h |   6 +
 accel/tcg/cputlb.c         |  14 ++
 hw/virtio/virtio-pci.c     |  21 ++-
 plugins/api.c              |  18 ++
 tests/plugin/hwprofile.c   | 248 +++++++++++++++++++++++++++
 tests/plugin/lockstep.c    | 340 +++++++++++++++++++++++++++++++++++++
 tests/plugin/mem.c         |   2 +-
 .travis.yml                |   5 +
 scripts/clean-includes     |   1 +
 tests/plugin/Makefile      |   2 +
 tests/tcg/Makefile.target  |   2 +-
 11 files changed, 649 insertions(+), 10 deletions(-)
 create mode 100644 tests/plugin/hwprofile.c
 create mode 100644 tests/plugin/lockstep.c

-- 
2.20.1

[PATCH v1 1/9] plugins: new lockstep plugin for debugging TCG changes

[Alex Bennée]

When we make changes to the TCG we sometimes cause regressions that
are deep into the execution cycle of the guest. Debugging this often
requires comparing large volumes of trace information to figure out
where behaviour has diverged.

The lockstep plugin utilises a shared socket so two QEMU's running
with the plugin will write their current execution position and wait
to receive the position of their partner process. When execution
diverges the plugins output where they were and the previous few
blocks before unloading themselves and letting execution continue.

Originally I planned for this to be most useful with -icount but it
turns out you can get divergence pretty quickly due to asynchronous
qemu_cpu_kick_rr_cpus() events causing one side to eventually run into
a short block a few cycles before the other side. For this reason I've
added a bit of tracking and I think the divergence reporting could be
finessed to report only if we really start to diverge in execution.

An example run would be:

  qemu-system-sparc -monitor none -parallel none -net none \
    -M SS-20 -m 256 -kernel day11/zImage.elf \
    -plugin ./tests/plugin/liblockstep.so,arg=lockstep-sparc.sock \
    -d plugin,nochain

with an identical command in another window in the same working
directory.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Message-Id: <20200429200754.18327-1-alex.bennee@linaro.org>

---
v3
  - added verbose flag
  - basic heuristics to detect "real" divergence
  - checkpatch tweaks
---
 tests/plugin/lockstep.c   | 340 ++++++++++++++++++++++++++++++++++++++
 tests/plugin/Makefile     |   1 +
 tests/tcg/Makefile.target |   2 +-
 3 files changed, 342 insertions(+), 1 deletion(-)
 create mode 100644 tests/plugin/lockstep.c

diff --git a/tests/plugin/lockstep.c b/tests/plugin/lockstep.c
new file mode 100644
index 00000000000..a696673dff3
--- /dev/null
+++ b/tests/plugin/lockstep.c
@@ -0,0 +1,340 @@
+/*
+ * Lockstep Execution Plugin
+ *
+ * Allows you to execute two QEMU instances in lockstep and report
+ * when their execution diverges. This is mainly useful for developers
+ * who want to see where a change to TCG code generation has
+ * introduced a subtle and hard to find bug.
+ *
+ * Caveats:
+ *   - single-threaded linux-user apps only with non-deterministic syscalls
+ *   - no MTTCG enabled system emulation (icount may help)
+ *
+ * While icount makes things more deterministic it doesn't mean a
+ * particular run may execute the exact same sequence of blocks. An
+ * asynchronous event (for example X11 graphics update) may cause a
+ * block to end early and a new partial block to start. This means
+ * serial only test cases are a better bet. -d nochain may also help.
+ *
+ * This code is not thread safe!
+ *
+ * Copyright (c) 2020 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <glib.h>
+#include <inttypes.h>
+#include <unistd.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <stdio.h>
+#include <errno.h>
+
+#include <qemu-plugin.h>
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+/* saved so we can uninstall later */
+static qemu_plugin_id_t our_id;
+
+static unsigned long bb_count;
+static unsigned long insn_count;
+
+/* Information about a translated block */
+typedef struct {
+    uint64_t pc;
+    uint64_t insns;
+} BlockInfo;
+
+/* Information about an execution state in the log */
+typedef struct {
+    BlockInfo *block;
+    unsigned long insn_count;
+    unsigned long block_count;
+} ExecInfo;
+
+/* The execution state we compare */
+typedef struct {
+    uint64_t pc;
+    unsigned long insn_count;
+} ExecState;
+
+typedef struct {
+    GSList *log_pos;
+    int distance;
+} DivergeState;
+
+/* list of translated block info */
+static GSList *blocks;
+
+/* execution log and points of divergence */
+static GSList *log, *divergence_log;
+
+static int socket_fd;
+static char *path_to_unlink;
+
+static bool verbose;
+
+static void plugin_cleanup(qemu_plugin_id_t id)
+{
+    /* Free our block data */
+    g_slist_free_full(blocks, &g_free);
+    g_slist_free_full(log, &g_free);
+    g_slist_free(divergence_log);
+
+    close(socket_fd);
+    if (path_to_unlink) {
+        unlink(path_to_unlink);
+    }
+}
+
+static void plugin_exit(qemu_plugin_id_t id, void *p)
+{
+    g_autoptr(GString) out = g_string_new("No divergence :-)\n");
+    g_string_append_printf(out, "Executed %ld/%d blocks\n",
+                           bb_count, g_slist_length(log));
+    g_string_append_printf(out, "Executed ~%ld instructions\n", insn_count);
+    qemu_plugin_outs(out->str);
+
+    plugin_cleanup(id);
+}
+
+static void report_divergance(ExecState *us, ExecState *them)
+{
+    DivergeState divrec = { log, 0 };
+    g_autoptr(GString) out = g_string_new("");
+    bool diverged = false;
+
+    /*
+     * If we have diverged before did we get back on track or are we
+     * totally loosing it?
+     */
+    if (divergence_log) {
+        DivergeState *last = (DivergeState *) divergence_log->data;
+        GSList *entry;
+
+        for (entry = log; g_slist_next(entry); entry = g_slist_next(entry)) {
+            if (entry == last->log_pos) {
+                break;
+            }
+            divrec.distance++;
+        }
+
+        /*
+         * If the last two records are so close it is likely we will
+         * not recover synchronisation with the other end.
+         */
+        if (divrec.distance == 1 && last->distance == 1) {
+            diverged = true;
+        }
+    }
+    divergence_log = g_slist_prepend(divergence_log,
+                                     g_memdup(&divrec, sizeof(divrec)));
+
+    /* Output short log entry of going out of sync... */
+    if (verbose || divrec.distance == 1 || diverged) {
+        g_string_printf(out, "@ %#016lx vs %#016lx (%d/%d since last)\n",
+                        us->pc, them->pc, g_slist_length(divergence_log),
+                        divrec.distance);
+        qemu_plugin_outs(out->str);
+    }
+
+    if (diverged) {
+        int i;
+        GSList *entry;
+
+        g_string_printf(out, "Δ insn_count @ %#016lx (%ld) vs %#016lx (%ld)\n",
+                        us->pc, us->insn_count, them->pc, them->insn_count);
+
+        for (entry = log, i = 0;
+             g_slist_next(entry) && i < 5;
+             entry = g_slist_next(entry), i++) {
+            ExecInfo *prev = (ExecInfo *) entry->data;
+            g_string_append_printf(out,
+                                   "  previously @ %#016lx/%ld (%ld insns)\n",
+                                   prev->block->pc, prev->block->insns,
+                                   prev->insn_count);
+        }
+        qemu_plugin_outs(out->str);
+        qemu_plugin_outs("too much divergence... giving up.");
+        qemu_plugin_uninstall(our_id, plugin_cleanup);
+    }
+}
+
+static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
+{
+    BlockInfo *bi = (BlockInfo *) udata;
+    ExecState us, them;
+    ssize_t bytes;
+    ExecInfo *exec;
+
+    us.pc = bi->pc;
+    us.insn_count = insn_count;
+
+    /*
+     * Write our current position to the other end. If we fail the
+     * other end has probably died and we should shut down gracefully.
+     */
+    bytes = write(socket_fd, &us, sizeof(ExecState));
+    if (bytes < sizeof(ExecState)) {
+        qemu_plugin_outs(bytes < 0 ?
+                         "problem writing to socket" :
+                         "wrote less than expected to socket");
+        qemu_plugin_uninstall(our_id, plugin_cleanup);
+        return;
+    }
+
+    /*
+     * Now read where our peer has reached. Again a failure probably
+     * indicates the other end died and we should close down cleanly.
+     */
+    bytes = read(socket_fd, &them, sizeof(ExecState));
+    if (bytes < sizeof(ExecState)) {
+        qemu_plugin_outs(bytes < 0 ?
+                         "problem reading from socket" :
+                         "read less than expected");
+        qemu_plugin_uninstall(our_id, plugin_cleanup);
+        return;
+    }
+
+    /*
+     * Compare and report if we have diverged.
+     */
+    if (us.pc != them.pc) {
+        report_divergance(&us, &them);
+    }
+
+    /*
+     * Assume this block will execute fully and record it
+     * in the execution log.
+     */
+    insn_count += bi->insns;
+    bb_count++;
+    exec = g_new0(ExecInfo, 1);
+    exec->block = bi;
+    exec->insn_count = insn_count;
+    exec->block_count = bb_count;
+    log = g_slist_prepend(log, exec);
+}
+
+static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
+{
+    BlockInfo *bi = g_new0(BlockInfo, 1);
+    bi->pc = qemu_plugin_tb_vaddr(tb);
+    bi->insns = qemu_plugin_tb_n_insns(tb);
+
+    /* save a reference so we can free later */
+    blocks = g_slist_prepend(blocks, bi);
+    qemu_plugin_register_vcpu_tb_exec_cb(tb, vcpu_tb_exec,
+                                         QEMU_PLUGIN_CB_NO_REGS, (void *)bi);
+}
+
+
+/*
+ * Instead of encoding master/slave status into what is essentially
+ * two peers we shall just take the simple approach of checking for
+ * the existence of the pipe and assuming if it's not there we are the
+ * first process.
+ */
+static bool setup_socket(const char *path)
+{
+    struct sockaddr_un sockaddr;
+    int fd;
+
+    fd = socket(AF_UNIX, SOCK_STREAM, 0);
+    if (fd < 0) {
+        perror("create socket");
+        return false;
+    }
+
+    sockaddr.sun_family = AF_UNIX;
+    g_strlcpy(sockaddr.sun_path, path, sizeof(sockaddr.sun_path) - 1);
+    if (bind(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)) < 0) {
+        perror("bind socket");
+        close(fd);
+        return false;
+    }
+
+    /* remember to clean-up */
+    path_to_unlink = g_strdup(path);
+
+    if (listen(fd, 1) < 0) {
+        perror("listen socket");
+        close(fd);
+        return false;
+    }
+
+    socket_fd = accept(fd, NULL, NULL);
+    if (socket_fd < 0 && errno != EINTR) {
+        perror("accept socket");
+        return false;
+    }
+
+    qemu_plugin_outs("setup_socket::ready\n");
+
+    return true;
+}
+
+static bool connect_socket(const char *path)
+{
+    int fd;
+    struct sockaddr_un sockaddr;
+
+    fd = socket(AF_UNIX, SOCK_STREAM, 0);
+    if (fd < 0) {
+        perror("create socket");
+        return false;
+    }
+
+    sockaddr.sun_family = AF_UNIX;
+    g_strlcpy(sockaddr.sun_path, path, sizeof(sockaddr.sun_path) - 1);
+
+    if (connect(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)) < 0) {
+        perror("failed to connect");
+        return false;
+    }
+
+    qemu_plugin_outs("connect_socket::ready\n");
+
+    socket_fd = fd;
+    return true;
+}
+
+static bool setup_unix_socket(const char *path)
+{
+    if (g_file_test(path, G_FILE_TEST_EXISTS)) {
+        return connect_socket(path);
+    } else {
+        return setup_socket(path);
+    }
+}
+
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
+                                           const qemu_info_t *info,
+                                           int argc, char **argv)
+{
+    int i;
+
+    if (!argc || !argv[0]) {
+        qemu_plugin_outs("Need a socket path to talk to other instance.");
+        return -1;
+    }
+
+    for (i = 0; i < argc; i++) {
+        char *p = argv[i];
+        if (strcmp(p, "verbose") == 0) {
+            verbose = true;
+        } else if (!setup_unix_socket(argv[0])) {
+            qemu_plugin_outs("Failed to setup socket for communications.");
+            return -1;
+        }
+    }
+
+    our_id = id;
+
+    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 75467b6db85..b3250e2504c 100644
--- a/tests/plugin/Makefile
+++ b/tests/plugin/Makefile
@@ -13,6 +13,7 @@ NAMES += mem
 NAMES += hotblocks
 NAMES += howvec
 NAMES += hotpages
+NAMES += lockstep
 
 SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
 
diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
index b3cff3cad1a..075daf3d22d 100644
--- a/tests/tcg/Makefile.target
+++ b/tests/tcg/Makefile.target
@@ -128,7 +128,7 @@ RUN_TESTS=$(patsubst %,run-%, $(TESTS))
 ifeq ($(CONFIG_PLUGIN),y)
 PLUGIN_DIR=../../plugin
 VPATH+=$(PLUGIN_DIR)
-PLUGINS=$(notdir $(wildcard $(PLUGIN_DIR)/*.so))
+PLUGINS=$(filter-out liblockstep.so,$(notdir $(wildcard $(PLUGIN_DIR)/*.so)))
 
 # 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

[PATCH v1 2/9] qemu-plugin.h: add missing include <stddef.h> to define size_t

[Alex Bennée]

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

Signed-off-by: Emilio G. Cota <cota@braap.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200524202427.951784-1-cota@braap.org>
---
 include/qemu/qemu-plugin.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 89ed579f559..bab8b0d4b3a 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -12,6 +12,7 @@
 
 #include <inttypes.h>
 #include <stdbool.h>
+#include <stddef.h>
 
 /*
  * For best performance, build the plugin with -fvisibility=hidden so that
-- 
2.20.1

[PATCH v1 3/9] scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header

[Alex Bennée]

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

"qemu/qemu-plugin.h" isn't meant to be include by QEMU codebase,
but by 3rd party plugins that QEMU can use. These plugins can be
built out of QEMU and don't include "qemu/osdep.h".
Mark "qemu/qemu-plugin.h" as a special header that doesn't need
to be cleaned for "qemu/osdep.h".

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Emilio G. Cota <cota@braap.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200524215654.13256-1-f4bug@amsat.org>
---
 scripts/clean-includes | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/clean-includes b/scripts/clean-includes
index dd938daa3ec..795b3bea318 100755
--- a/scripts/clean-includes
+++ b/scripts/clean-includes
@@ -123,6 +123,7 @@ for f in "$@"; do
       ;;
     *include/qemu/osdep.h | \
     *include/qemu/compiler.h | \
+    *include/qemu/qemu-plugin.h | \
     *include/glib-compat.h | \
     *include/sysemu/os-posix.h | \
     *include/sysemu/os-win32.h | \
-- 
2.20.1

[PATCH v1 4/9] tests/plugin: correctly honour io_count

[Alex Bennée]

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

diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
index 878abf09d19..4725bd851d8 100644
--- a/tests/plugin/mem.c
+++ b/tests/plugin/mem.c
@@ -28,7 +28,7 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
 
     g_string_printf(out, "mem accesses: %" PRIu64 "\n", mem_count);
     if (do_haddr) {
-        g_string_append_printf(out, "io accesses: %" PRIu64 "\n", mem_count);
+        g_string_append_printf(out, "io accesses: %" PRIu64 "\n", io_count);
     }
     qemu_plugin_outs(out->str);
 }
-- 
2.20.1

Re: [PATCH v1 4/9] tests/plugin: correctly honour io_count

[Philippe Mathieu-Daudé]

On 6/2/20 5:46 PM, Alex Bennée wrote:

Fixes: 671f760b93b ?

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

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/plugin/mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
> index 878abf09d19..4725bd851d8 100644
> --- a/tests/plugin/mem.c
> +++ b/tests/plugin/mem.c
> @@ -28,7 +28,7 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
>  
>      g_string_printf(out, "mem accesses: %" PRIu64 "\n", mem_count);
>      if (do_haddr) {
> -        g_string_append_printf(out, "io accesses: %" PRIu64 "\n", mem_count);
> +        g_string_append_printf(out, "io accesses: %" PRIu64 "\n", io_count);
>      }
>      qemu_plugin_outs(out->str);
>  }
> 

[PATCH v1 5/9] cputlb: ensure we re-fill the TLB if it has 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. We catch
this case by checking to see if the list of entries has been cleared
and if so triggering a re-fill.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/cputlb.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index eb2cf9de5e6..b7d329f7155 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1091,6 +1091,20 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
                                MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
                                retaddr);
     }
+
+    /*
+     * The memory_region_dispatch may have triggered a flush/resize
+     * so for plugins we need to ensure we have reset the tlb_entry
+     * so any later lookup is correct.
+     */
+#ifdef CONFIG_PLUGIN
+    if (env_tlb(env)->d[mmu_idx].n_used_entries == 0) {
+        int size = op & MO_SIZE;
+        tlb_fill(env_cpu(env), addr, size, MMU_DATA_STORE,
+                 mmu_idx, retaddr);
+    }
+#endif
+
     if (locked) {
         qemu_mutex_unlock_iothread();
     }
-- 
2.20.1

Re: [PATCH v1 5/9] cputlb: ensure we re-fill the TLB if it has reset

[Richard Henderson]

On 6/2/20 8:46 AM, 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. We catch
> this case by checking to see if the list of entries has been cleared
> and if so triggering a re-fill.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  accel/tcg/cputlb.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index eb2cf9de5e6..b7d329f7155 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1091,6 +1091,20 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>                                 MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
>                                 retaddr);
>      }
> +
> +    /*
> +     * The memory_region_dispatch may have triggered a flush/resize
> +     * so for plugins we need to ensure we have reset the tlb_entry
> +     * so any later lookup is correct.
> +     */
> +#ifdef CONFIG_PLUGIN
> +    if (env_tlb(env)->d[mmu_idx].n_used_entries == 0) {
> +        int size = op & MO_SIZE;
> +        tlb_fill(env_cpu(env), addr, size, MMU_DATA_STORE,
> +                 mmu_idx, retaddr);

Ouch.  What if the target has a soft tlb fill, so this requires a call into the
OS, so this fill actually raises another exception?  This will not be happy fun
making.

I recall I had objections to recording this translation, saying that "we can
always get it back again".  Clearly I was wrong, and we should just preserve
the required CPUTLBEntry details before they're lost by a device.


r~

Re: [PATCH v1 5/9] cputlb: ensure we re-fill the TLB if it has reset

[Alex Bennée]

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

> On 6/2/20 8:46 AM, 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. We catch
>> this case by checking to see if the list of entries has been cleared
>> and if so triggering a re-fill.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  accel/tcg/cputlb.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>> 
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index eb2cf9de5e6..b7d329f7155 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -1091,6 +1091,20 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>>                                 MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
>>                                 retaddr);
>>      }
>> +
>> +    /*
>> +     * The memory_region_dispatch may have triggered a flush/resize
>> +     * so for plugins we need to ensure we have reset the tlb_entry
>> +     * so any later lookup is correct.
>> +     */
>> +#ifdef CONFIG_PLUGIN
>> +    if (env_tlb(env)->d[mmu_idx].n_used_entries == 0) {
>> +        int size = op & MO_SIZE;
>> +        tlb_fill(env_cpu(env), addr, size, MMU_DATA_STORE,
>> +                 mmu_idx, retaddr);
>
> Ouch.  What if the target has a soft tlb fill, so this requires a call into the
> OS, so this fill actually raises another exception?  This will not be happy fun
> making.
>
> I recall I had objections to recording this translation, saying that "we can
> always get it back again".  Clearly I was wrong, and we should just preserve
> the required CPUTLBEntry details before they're lost by a device.

Maybe we could just RCU the old TLB if it gets flushed thus ensuring the
whole TLB is preserved until after the critical section (i.e. between
the actual store and looking it up). However I don't know if the
MemoryRegion will be similarly preserved.

Paolo?

>
>
> r~


-- 
Alex Bennée

[PATCH v1 6/9] 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>
---
 hw/virtio/virtio-pci.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index d028c17c240..9ee4ab26cfe 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1390,7 +1390,7 @@ 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,
@@ -1437,36 +1437,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);
 }
 
@@ -1607,7 +1612,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 v1 6/9] hw/virtio/pci: include vdev name in registered PCI sections

[Philippe Mathieu-Daudé]

On 6/2/20 5:46 PM, 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>
> ---
>  hw/virtio/virtio-pci.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index d028c17c240..9ee4ab26cfe 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1390,7 +1390,7 @@ 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,
> @@ -1437,36 +1437,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);
>  }
>  
> @@ -1607,7 +1612,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);
> 

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

Re: [PATCH v1 6/9] hw/virtio/pci: include vdev name in registered PCI sections

[Michael S. Tsirkin]

On Tue, Jun 02, 2020 at 04:46:21PM +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: Michael S. Tsirkin <mst@redhat.com>

Feel free to merge with the rest of it.

> ---
>  hw/virtio/virtio-pci.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index d028c17c240..9ee4ab26cfe 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1390,7 +1390,7 @@ 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,
> @@ -1437,36 +1437,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);
>  }
>  
> @@ -1607,7 +1612,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 v1 7/9] 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>
---
 include/qemu/qemu-plugin.h |  5 +++++
 plugins/api.c              | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index bab8b0d4b3a..43c6a9e857f 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -335,6 +335,11 @@ 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. Plugin must free() it
+ */
+char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);
+
 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 bbdc5a4eb46..3c73de8c1c2 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
     return 0;
 }
 
+char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)
+{
+#ifdef CONFIG_SOFTMMU
+    if (haddr && haddr->is_io) {
+        MemoryRegionSection *mrs = haddr->v.io.section;
+        if (!mrs->mr->name) {
+            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);
+        } else {
+            return g_strdup(mrs->mr->name);
+        }
+    } else {
+        return g_strdup("RAM");
+    }
+#else
+    return g_strdup("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 v1 7/9] plugins: add API to return a name for a IO device

[Clement Deschamps]

On 6/2/20 5:46 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>
> ---
>   include/qemu/qemu-plugin.h |  5 +++++
>   plugins/api.c              | 18 ++++++++++++++++++
>   2 files changed, 23 insertions(+)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index bab8b0d4b3a..43c6a9e857f 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -335,6 +335,11 @@ 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. Plugin must free() it

s/free/g_free


> + */
> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);
> +
>   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 bbdc5a4eb46..3c73de8c1c2 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
>       return 0;
>   }
>   
> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)
> +{
> +#ifdef CONFIG_SOFTMMU
> +    if (haddr && haddr->is_io) {
> +        MemoryRegionSection *mrs = haddr->v.io.section;
> +        if (!mrs->mr->name) {
> +            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);
> +        } else {
> +            return g_strdup(mrs->mr->name);
> +        }
> +    } else {
> +        return g_strdup("RAM");
> +    }
> +#else
> +    return g_strdup("Invalid");
> +#endif
> +}
> +
>   /*
>    * Queries to the number and potential maximum number of vCPUs there
>    * will be. This helps the plugin dimension per-vcpu arrays.
> 

Reviewed-by: Clement Deschamps <clement.deschamps@greensocs.com>

Re: [PATCH v1 7/9] plugins: add API to return a name for a IO device

[Emilio G. Cota]

On Tue, Jun 02, 2020 at 16:46:22 +0100, 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>
> ---
>  include/qemu/qemu-plugin.h |  5 +++++
>  plugins/api.c              | 18 ++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index bab8b0d4b3a..43c6a9e857f 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -335,6 +335,11 @@ 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. Plugin must free() it
> + */
> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);
> +
>  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 bbdc5a4eb46..3c73de8c1c2 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
>      return 0;
>  }
>  
> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)
> +{
> +#ifdef CONFIG_SOFTMMU
> +    if (haddr && haddr->is_io) {
> +        MemoryRegionSection *mrs = haddr->v.io.section;
> +        if (!mrs->mr->name) {
> +            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);
> +        } else {
> +            return g_strdup(mrs->mr->name);
> +        }
> +    } else {
> +        return g_strdup("RAM");
> +    }
> +#else
> +    return g_strdup("Invalid");
> +#endif
> +}

I'd rather use asprintf(3) and strdup(3) here, so that plugins don't
have to worry about glib, and on the QEMU side we don't have to worry
about plugins calling free() instead of g_free().

Or given that this doesn't look perf-critical, perhaps an easier way out
is to wrap the above with:

char *g_str = above();
char *ret = strdup(g_str);
g_free(g_str);
return ret;

Not sure we should NULL-check ret, since I don't know whether
mrs->mr->name is guaranteed to be non-NULL.

Thanks,
		Emilio

Re: [PATCH v1 7/9] plugins: add API to return a name for a IO device

[Philippe Mathieu-Daudé]

On 6/8/20 5:45 AM, Emilio G. Cota wrote:
> On Tue, Jun 02, 2020 at 16:46:22 +0100, 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>
>> ---
>>  include/qemu/qemu-plugin.h |  5 +++++
>>  plugins/api.c              | 18 ++++++++++++++++++
>>  2 files changed, 23 insertions(+)
>>
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index bab8b0d4b3a..43c6a9e857f 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>> @@ -335,6 +335,11 @@ 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. Plugin must free() it
>> + */
>> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);
>> +
>>  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 bbdc5a4eb46..3c73de8c1c2 100644
>> --- a/plugins/api.c
>> +++ b/plugins/api.c
>> @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
>>      return 0;
>>  }
>>  
>> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)
>> +{
>> +#ifdef CONFIG_SOFTMMU
>> +    if (haddr && haddr->is_io) {
>> +        MemoryRegionSection *mrs = haddr->v.io.section;
>> +        if (!mrs->mr->name) {
>> +            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);
>> +        } else {
>> +            return g_strdup(mrs->mr->name);
>> +        }
>> +    } else {
>> +        return g_strdup("RAM");
>> +    }
>> +#else
>> +    return g_strdup("Invalid");
>> +#endif
>> +}
> 
> I'd rather use asprintf(3) and strdup(3) here, so that plugins don't
> have to worry about glib, and on the QEMU side we don't have to worry
> about plugins calling free() instead of g_free().

It might make sense, but it should be documented in
include/qemu/qemu-plugin.h or docs/devel/tcg-plugins.rst.

> 
> Or given that this doesn't look perf-critical, perhaps an easier way out
> is to wrap the above with:
> 
> char *g_str = above();
> char *ret = strdup(g_str);
> g_free(g_str);

free() ;)

> return ret;
> 
> Not sure we should NULL-check ret, since I don't know whether
> mrs->mr->name is guaranteed to be non-NULL.
> 
> Thanks,
> 		Emilio
> 

Re: [PATCH v1 7/9] plugins: add API to return a name for a IO device

[Alex Bennée]

Emilio G. Cota <cota@braap.org> writes:

> On Tue, Jun 02, 2020 at 16:46:22 +0100, 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>
>> ---
>>  include/qemu/qemu-plugin.h |  5 +++++
>>  plugins/api.c              | 18 ++++++++++++++++++
>>  2 files changed, 23 insertions(+)
>> 
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index bab8b0d4b3a..43c6a9e857f 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>> @@ -335,6 +335,11 @@ 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. Plugin must free() it
>> + */
>> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);
>> +
>>  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 bbdc5a4eb46..3c73de8c1c2 100644
>> --- a/plugins/api.c
>> +++ b/plugins/api.c
>> @@ -303,6 +303,24 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr *haddr
>>      return 0;
>>  }
>>  
>> +char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)
>> +{
>> +#ifdef CONFIG_SOFTMMU
>> +    if (haddr && haddr->is_io) {
>> +        MemoryRegionSection *mrs = haddr->v.io.section;
>> +        if (!mrs->mr->name) {
>> +            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);
>> +        } else {
>> +            return g_strdup(mrs->mr->name);
>> +        }
>> +    } else {
>> +        return g_strdup("RAM");
>> +    }
>> +#else
>> +    return g_strdup("Invalid");
>> +#endif
>> +}
>
> I'd rather use asprintf(3) and strdup(3) here, so that plugins don't
> have to worry about glib, and on the QEMU side we don't have to worry
> about plugins calling free() instead of g_free().

AFAIK you can actually mix free/g_free because g_free is just a NULL
checking wrapper around free. However ideally I'd be passing a
non-freeable const char to the plugin but I didn't want to expose
pointers deep inside of QEMU's guts although maybe I'm just being
paranoid there given you can easily gdb the combined operation anyway.

Perhaps there is a need for a separate memory region where we can store
copies of strings we have made for the plugins?

> Or given that this doesn't look perf-critical, perhaps an easier way out
> is to wrap the above with:
>
> char *g_str = above();
> char *ret = strdup(g_str);
> g_free(g_str);
> return ret;
>
> Not sure we should NULL-check ret, since I don't know whether
> mrs->mr->name is guaranteed to be non-NULL.

Experimentation showed you can get null mrs->name has a result of a
region being subdivided due to an some operations. That said in another
thread Peter was uncomfortable about exposing this piece of information
to plugins. Maybe we should only expose something based on the optional
-device foo,id=bar parameter?

>
> Thanks,
> 		Emilio


-- 
Alex Bennée

Re: [PATCH v1 7/9] plugins: add API to return a name for a IO device

[Emilio G. Cota]

On Mon, Jun 08, 2020 at 09:06:17 +0100, Alex Bennée wrote:
> Emilio G. Cota <cota@braap.org> writes:
> > I'd rather use asprintf(3) and strdup(3) here, so that plugins don't
> > have to worry about glib, and on the QEMU side we don't have to worry
> > about plugins calling free() instead of g_free().
> 
> AFAIK you can actually mix free/g_free because g_free is just a NULL
> checking wrapper around free.

I was just going with the documentation, but you're right:

https://github.com/GNOME/glib/blob/mainline/glib/gmem.c#L196
> void
> g_free (gpointer mem)
> {
>   free (mem);
>   TRACE(GLIB_MEM_FREE((void*) mem));
> }

The NULL-pointer check is done by free(3), though.

> However ideally I'd be passing a
> non-freeable const char to the plugin but I didn't want to expose
> pointers deep inside of QEMU's guts although maybe I'm just being
> paranoid there given you can easily gdb the combined operation anyway.
>
> Perhaps there is a need for a separate memory region where we can store
> copies of strings we have made for the plugins?

I agree with the idea of not exposing internal pointers to plugins
(e.g. we don't pass a CPUState *, only an opaque handle) so I'm OK
with returning a dup'ed string here.

(snip)
> That said in another
> thread Peter was uncomfortable about exposing this piece of information
> to plugins. Maybe we should only expose something based on the optional
> -device foo,id=bar parameter?

I have no opinion on whether exposing this is a good idea. If it turns
out that it is, please have my

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

Thanks,

		Emilio

Re: [PATCH v1 7/9] plugins: add API to return a name for a IO device

[Alex Bennée]

Emilio G. Cota <cota@braap.org> writes:

> On Mon, Jun 08, 2020 at 09:06:17 +0100, Alex Bennée wrote:
>> Emilio G. Cota <cota@braap.org> writes:
>> > I'd rather use asprintf(3) and strdup(3) here, so that plugins don't
>> > have to worry about glib, and on the QEMU side we don't have to worry
>> > about plugins calling free() instead of g_free().
>> 
>> AFAIK you can actually mix free/g_free because g_free is just a NULL
>> checking wrapper around free.
>
> I was just going with the documentation, but you're right:
>
> https://github.com/GNOME/glib/blob/mainline/glib/gmem.c#L196
>> void
>> g_free (gpointer mem)
>> {
>>   free (mem);
>>   TRACE(GLIB_MEM_FREE((void*) mem));
>> }
>
> The NULL-pointer check is done by free(3), though.
>
>> However ideally I'd be passing a
>> non-freeable const char to the plugin but I didn't want to expose
>> pointers deep inside of QEMU's guts although maybe I'm just being
>> paranoid there given you can easily gdb the combined operation anyway.
>>
>> Perhaps there is a need for a separate memory region where we can store
>> copies of strings we have made for the plugins?
>
> I agree with the idea of not exposing internal pointers to plugins
> (e.g. we don't pass a CPUState *, only an opaque handle) so I'm OK
> with returning a dup'ed string here.

How about a g_intern_string() as a non-freeable const char that can also
be treated as canonical?

>
> (snip)
>> That said in another
>> thread Peter was uncomfortable about exposing this piece of information
>> to plugins. Maybe we should only expose something based on the optional
>> -device foo,id=bar parameter?
>
> I have no opinion on whether exposing this is a good idea. If it turns
> out that it is, please have my
>
> Reviewed-by: Emilio G. Cota <cota@braap.org>
>
> Thanks,
>
> 		Emilio


-- 
Alex Bennée

Re: [PATCH v1 7/9] plugins: add API to return a name for a IO device

[Emilio G. Cota]

On Tue, Jun 09, 2020 at 12:09:54 +0100, Alex Bennée wrote:
> How about a g_intern_string() as a non-freeable const char that can also
> be treated as canonical?

I like it. Didn't know about g_intern_string (I see it's
implemented as an append-only hash table protected by a lock).

Cheers,

		Emilio

[PATCH v1 8/9] 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.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/plugin/hwprofile.c | 248 +++++++++++++++++++++++++++++++++++++++
 tests/plugin/Makefile    |   1 +
 2 files changed, 249 insertions(+)
 create mode 100644 tests/plugin/hwprofile.c

diff --git a/tests/plugin/hwprofile.c b/tests/plugin/hwprofile.c
new file mode 100644
index 00000000000..f5e0639e762
--- /dev/null
+++ b/tests/plugin/hwprofile.c
@@ -0,0 +1,248 @@
+/*
+ * 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 offset;
+    int size;
+    int cpu_read;
+    int cpu_write;
+    uint64_t reads;
+    uint64_t writes;
+} IOLocationCounts;
+
+typedef struct {
+    const char *name;
+    uint64_t base;
+    int cpu_read;
+    int cpu_write;
+    uint64_t total_writes;
+    uint64_t total_reads;
+    GHashTable *access_pattern;
+} DeviceCounts;
+
+static GMutex lock;
+static GHashTable *devices;
+static bool detail;
+
+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(g_str_hash, g_str_equal);
+}
+
+static gint sort_cmp(gconstpointer a, gconstpointer b)
+{
+    DeviceCounts *ea = (DeviceCounts *) a;
+    DeviceCounts *eb = (DeviceCounts *) b;
+    return ea->total_reads + ea->total_writes >
+        eb->total_reads + eb->total_writes ? -1 : 1;
+}
+
+static gint sort_off(gconstpointer a, gconstpointer b)
+{
+    IOLocationCounts *ea = (IOLocationCounts *) a;
+    IOLocationCounts *eb = (IOLocationCounts *) b;
+    return ea->offset > eb->offset;
+}
+
+static void plugin_exit(qemu_plugin_id_t id, void *p)
+{
+    g_autoptr(GString) report = g_string_new("");
+    GList *counts;
+
+    if (!detail) {
+        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 (detail) {
+                GList *accesses = g_hash_table_get_values(rec->access_pattern);
+                GList *io_it = g_list_sort(accesses, sort_off);
+                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, "  off:%08"PRIx64, loc->offset);
+                    if (track_reads()) {
+                        g_string_append_printf(report, ", 0x%04x, %"PRId64,
+                                               loc->cpu_read, loc->reads);
+                    }
+                    if (track_writes()) {
+                       g_string_append_printf(report, ", 0x%04x, %"PRId64,
+                                               loc->cpu_write, loc->writes);
+                    }
+                    g_string_append_c(report,'\n');
+                    io_it = io_it->next;
+                }
+            } else {
+                g_string_append_printf(report, "%s, 0x%"PRIx64,
+                                       rec->name, rec->base);
+                if (track_reads()) {
+                    g_string_append_printf(report, ", 0x%04x, %"PRId64,
+                                           rec->cpu_read, rec->total_reads);
+                }
+                if (track_writes()) {
+                    g_string_append_printf(report, ", 0x%04x, %"PRId64,
+                                           rec->cpu_write, rec->total_writes);
+                }
+                g_string_append_c(report, '\n');
+            }
+            it = it->next;
+        };
+        g_list_free(it);
+    }
+
+    qemu_plugin_outs(report->str);
+}
+
+static DeviceCounts * new_count(char *name, uint64_t base)
+{
+    DeviceCounts *count = g_new0(DeviceCounts, 1);
+    count->name = name;
+    count->base = base;
+    if (detail) {
+        count->access_pattern = g_hash_table_new(g_int64_hash, g_int64_equal);
+    }
+    g_hash_table_insert(devices, name, count);
+    return count;
+}
+
+static IOLocationCounts * new_location(uint64_t offset)
+{
+    IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
+    loc->offset = offset;
+    return loc;
+}
+
+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 {
+        char *name = qemu_plugin_hwaddr_device_name(hwaddr);
+        DeviceCounts *counts;
+
+        g_mutex_lock(&lock);
+        counts = (DeviceCounts *) g_hash_table_lookup(devices, name);
+        if (!counts) {
+            uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
+            uint64_t base = vaddr - off;
+            counts = new_count(name, base);
+        } else {
+            g_free(name);
+        }
+
+        if (detail) {
+            uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
+            IOLocationCounts *io_count = g_hash_table_lookup(counts->access_pattern, &off);
+            if (!io_count) {
+                io_count = new_location(off);
+                g_hash_table_insert(counts->access_pattern, &off, io_count);
+            }
+            if (qemu_plugin_mem_is_store(meminfo)) {
+                io_count->writes++;
+                io_count->cpu_write |= (1 << cpu_index);
+            } else {
+                io_count->reads++;
+                io_count->cpu_read |= (1 << cpu_index);
+            }
+        } else {
+            if (qemu_plugin_mem_is_store(meminfo)) {
+                counts->total_writes++;
+                counts->cpu_write |= (1 << cpu_index);
+            } else {
+                counts->total_reads++;
+                counts->cpu_read |= (1 << 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);
+        qemu_plugin_register_vcpu_mem_cb(insn, vcpu_haddr,
+                                         QEMU_PLUGIN_CB_NO_REGS,
+                                         rw, NULL);
+    }
+}
+
+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, "detail") == 0) {
+            detail = true;
+        } else {
+            fprintf(stderr, "option parsing failed: %s\n", opt);
+            return -1;
+        }
+    }
+
+    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 b3250e2504c..d87b8d40699 100644
--- a/tests/plugin/Makefile
+++ b/tests/plugin/Makefile
@@ -14,6 +14,7 @@ NAMES += hotblocks
 NAMES += howvec
 NAMES += hotpages
 NAMES += lockstep
+NAMES += hwprofile
 
 SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
 
-- 
2.20.1

Re: [PATCH v1 8/9] plugins: new hwprofile plugin

[Robert Foley]

Hi,

On Tue, 2 Jun 2020 at 11:46, Alex Bennée <alex.bennee@linaro.org> wrote:
<snip>
> diff --git a/tests/plugin/hwprofile.c b/tests/plugin/hwprofile.c
> new file mode 100644
> index 00000000000..f5e0639e762
> --- /dev/null
> +++ b/tests/plugin/hwprofile.c
<snip>
> +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 {
> +        char *name = qemu_plugin_hwaddr_device_name(hwaddr);
> +        DeviceCounts *counts;
> +
> +        g_mutex_lock(&lock);
> +        counts = (DeviceCounts *) g_hash_table_lookup(devices, name);
> +        if (!counts) {
> +            uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
> +            uint64_t base = vaddr - off;
> +            counts = new_count(name, base);
> +        } else {
> +            g_free(name);
> +        }
> +
> +        if (detail) {
> +            uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
> +            IOLocationCounts *io_count = g_hash_table_lookup(counts->access_pattern, &off);
> +            if (!io_count) {
> +                io_count = new_location(off);
> +                g_hash_table_insert(counts->access_pattern, &off, io_count);
> +            }
> +            if (qemu_plugin_mem_is_store(meminfo)) {
> +                io_count->writes++;
> +                io_count->cpu_write |= (1 << cpu_index);
> +            } else {
> +                io_count->reads++;
> +                io_count->cpu_read |= (1 << cpu_index);
> +            }
> +        } else {
> +            if (qemu_plugin_mem_is_store(meminfo)) {
> +                counts->total_writes++;
> +                counts->cpu_write |= (1 << cpu_index);
> +            } else {
> +                counts->total_reads++;
> +                counts->cpu_read |= (1 << cpu_index);

The bitmasks cpu_read and cpu_write are ints.  Maybe to account for
larger core counts
> 32, we could assert if the cpu_index is >= 32?

> +            }
> +        }
> +        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);
> +        qemu_plugin_register_vcpu_mem_cb(insn, vcpu_haddr,
> +                                         QEMU_PLUGIN_CB_NO_REGS,
> +                                         rw, NULL);
> +    }
> +}
> +
> +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, "detail") == 0) {

When testing out the options, I noticed that
if we supply arguments of "read", and "write", then we will only get
the last one set, "write", since rw gets overwritten.
One option would be to error out if more than one of these read/write
args is supplied.

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

> +            detail = true;
> +        } else {
> +            fprintf(stderr, "option parsing failed: %s\n", opt);
> +            return -1;
> +        }
> +    }
> +
> +    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 b3250e2504c..d87b8d40699 100644
> --- a/tests/plugin/Makefile
> +++ b/tests/plugin/Makefile
> @@ -14,6 +14,7 @@ NAMES += hotblocks
>  NAMES += howvec
>  NAMES += hotpages
>  NAMES += lockstep
> +NAMES += hwprofile
>
>  SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>
> --
> 2.20.1
>

Re: [PATCH v1 8/9] plugins: new hwprofile plugin

[Alex Bennée]

Robert Foley <robert.foley@linaro.org> writes:

> Hi,
>
> On Tue, 2 Jun 2020 at 11:46, Alex Bennée <alex.bennee@linaro.org> wrote:
> <snip>
>> diff --git a/tests/plugin/hwprofile.c b/tests/plugin/hwprofile.c
>> new file mode 100644
>> index 00000000000..f5e0639e762
>> --- /dev/null
>> +++ b/tests/plugin/hwprofile.c
> <snip>
>> +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 {
>> +        char *name = qemu_plugin_hwaddr_device_name(hwaddr);
>> +        DeviceCounts *counts;
>> +
>> +        g_mutex_lock(&lock);
>> +        counts = (DeviceCounts *) g_hash_table_lookup(devices, name);
>> +        if (!counts) {
>> +            uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
>> +            uint64_t base = vaddr - off;
>> +            counts = new_count(name, base);
>> +        } else {
>> +            g_free(name);
>> +        }
>> +
>> +        if (detail) {
>> +            uint64_t off = qemu_plugin_hwaddr_device_offset(hwaddr);
>> +            IOLocationCounts *io_count = g_hash_table_lookup(counts->access_pattern, &off);
>> +            if (!io_count) {
>> +                io_count = new_location(off);
>> +                g_hash_table_insert(counts->access_pattern, &off, io_count);
>> +            }
>> +            if (qemu_plugin_mem_is_store(meminfo)) {
>> +                io_count->writes++;
>> +                io_count->cpu_write |= (1 << cpu_index);
>> +            } else {
>> +                io_count->reads++;
>> +                io_count->cpu_read |= (1 << cpu_index);
>> +            }
>> +        } else {
>> +            if (qemu_plugin_mem_is_store(meminfo)) {
>> +                counts->total_writes++;
>> +                counts->cpu_write |= (1 << cpu_index);
>> +            } else {
>> +                counts->total_reads++;
>> +                counts->cpu_read |= (1 << cpu_index);
>
> The bitmasks cpu_read and cpu_write are ints.  Maybe to account for
> larger core counts

I could make them uint64_t and then just warn if we exceed that on start-up.

>> 32, we could assert if the cpu_index is >= 32?
>
>> +            }
>> +        }
>> +        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);
>> +        qemu_plugin_register_vcpu_mem_cb(insn, vcpu_haddr,
>> +                                         QEMU_PLUGIN_CB_NO_REGS,
>> +                                         rw, NULL);
>> +    }
>> +}
>> +
>> +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, "detail") == 0) {
>
> When testing out the options, I noticed that
> if we supply arguments of "read", and "write", then we will only get
> the last one set, "write", since rw gets overwritten.
> One option would be to error out if more than one of these read/write
> args is supplied.

Yeah the option parsing is a little clunky although given the way you
pass them from the QEMU command line perhaps not too worth finessing.
The default is rw so you make a conscious decision to only care about one
or the other.

All you can really do is fail to initialise the plugin. Hopefully the
output should be enough clue.

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

Thanks.

Out of interest what did you measure? Are there any useful use cases you can
think of?

>
>> +            detail = true;
>> +        } else {
>> +            fprintf(stderr, "option parsing failed: %s\n", opt);
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    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 b3250e2504c..d87b8d40699 100644
>> --- a/tests/plugin/Makefile
>> +++ b/tests/plugin/Makefile
>> @@ -14,6 +14,7 @@ NAMES += hotblocks
>>  NAMES += howvec
>>  NAMES += hotpages
>>  NAMES += lockstep
>> +NAMES += hwprofile
>>
>>  SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>>
>> --
>> 2.20.1
>>


-- 
Alex Bennée

Re: [PATCH v1 8/9] plugins: new hwprofile plugin

[Robert Foley]

On Wed, 3 Jun 2020 at 07:43, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Robert Foley <robert.foley@linaro.org> writes:
>
<snip>
> >
> > When testing out the options, I noticed that
> > if we supply arguments of "read", and "write", then we will only get
> > the last one set, "write", since rw gets overwritten.
> > One option would be to error out if more than one of these read/write
> > args is supplied.
>
> Yeah the option parsing is a little clunky although given the way you
> pass them from the QEMU command line perhaps not too worth finessing.
> The default is rw so you make a conscious decision to only care about one
> or the other.
>
> All you can really do is fail to initialise the plugin. Hopefully the
> output should be enough clue.
>
> >
> > Reviewed-by: Robert Foley <robert.foley@linaro.org>
> > Tested-by: Robert Foley <robert.foley@linaro.org>
>
> Thanks.
>
> Out of interest what did you measure? Are there any useful use cases you can
> think of?

We did some testing where we booted an aarch64 VM and an i386 VM a few times
with differentcore counts (up to 64), and viewed the counters.  We
also did a test where
we inserted another device (a virtfs mount), booted up and checked
that there was another
device listed (for virtio-9p).

There are a few useful use cases we are thinking of, in general for debug/perf
 testing of PCI devices/drivers.
For example, debug and performance test of a case where we use a queue pair,
(maybe for something like DPDK/SPDK), this plugin would be interesting for
checking that the quantity and locations of accesses are expected.

Thanks & Regards,
-Rob
>
> >
> >> +            detail = true;
> >> +        } else {
> >> +            fprintf(stderr, "option parsing failed: %s\n", opt);
> >> +            return -1;
> >> +        }
> >> +    }
> >> +
> >> +    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 b3250e2504c..d87b8d40699 100644
> >> --- a/tests/plugin/Makefile
> >> +++ b/tests/plugin/Makefile
> >> @@ -14,6 +14,7 @@ NAMES += hotblocks
> >>  NAMES += howvec
> >>  NAMES += hotpages
> >>  NAMES += lockstep
> >> +NAMES += hwprofile
> >>
> >>  SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
> >>
> >> --
> >> 2.20.1
> >>
>
>
> --
> Alex Bennée

Re: [PATCH v1 8/9] plugins: new hwprofile plugin

[Alex Bennée]

Robert Foley <robert.foley@linaro.org> writes:

> On Wed, 3 Jun 2020 at 07:43, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Robert Foley <robert.foley@linaro.org> writes:
>>
> <snip>
>> >
>> > When testing out the options, I noticed that
>> > if we supply arguments of "read", and "write", then we will only get
>> > the last one set, "write", since rw gets overwritten.
>> > One option would be to error out if more than one of these read/write
>> > args is supplied.
>>
>> Yeah the option parsing is a little clunky although given the way you
>> pass them from the QEMU command line perhaps not too worth finessing.
>> The default is rw so you make a conscious decision to only care about one
>> or the other.
>>
>> All you can really do is fail to initialise the plugin. Hopefully the
>> output should be enough clue.
>>
>> >
>> > Reviewed-by: Robert Foley <robert.foley@linaro.org>
>> > Tested-by: Robert Foley <robert.foley@linaro.org>
>>
>> Thanks.
>>
>> Out of interest what did you measure? Are there any useful use cases you can
>> think of?
>
> We did some testing where we booted an aarch64 VM and an i386 VM a few times
> with differentcore counts (up to 64), and viewed the counters.  We
> also did a test where
> we inserted another device (a virtfs mount), booted up and checked
> that there was another
> device listed (for virtio-9p).
>
> There are a few useful use cases we are thinking of, in general for debug/perf
>  testing of PCI devices/drivers.
> For example, debug and performance test of a case where we use a queue pair,
> (maybe for something like DPDK/SPDK), this plugin would be interesting for
> checking that the quantity and locations of accesses are expected.

So one thing that has come up in the VIRT-366 discussion is the
potential efficiencies of the various kick models for MMIO based
hypervisors. Each interaction with a trapped region of memory triggers a
vmexit and one thing I wanted to understand for example was the
difference between "normal" IRQs and MSIs.

-- 
Alex Bennée

Re: [PATCH v1 8/9] plugins: new hwprofile plugin

[Peter Maydell]

On Tue, 2 Jun 2020 at 16:54, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> 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.

This feels like we've let the plugin API get slightly more
access to QEMU's internals than is ideal. Whether an area
of memory happens to be an IO memory region or a memory-backed
one (or whether a device is implemented with one region or
three, or what names we happened to assign them) is kind of
a QEMU internal implementation detail.

thanks
-- PMM

Re: [PATCH v1 8/9] plugins: new hwprofile plugin

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 2 Jun 2020 at 16:54, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> 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.
>
> This feels like we've let the plugin API get slightly more
> access to QEMU's internals than is ideal. Whether an area
> of memory happens to be an IO memory region or a memory-backed
> one (or whether a device is implemented with one region or
> three, or what names we happened to assign them) is kind of
> a QEMU internal implementation detail.

I'm not so sure it's that much of an implementation detail.

The distinction is between plain RAM and everything else. The details of
the everything else is opaque but the name we pass is public information
(you can get it from "info mtree -o") and you can certainly infer useful
stuff from it. For example the virtio-pci-notify areas are regions of
access that will trap on a real hypervisor so allow us to measure how
many vmexits some software might cause.

At the moment I do make up names for regions that get re-generated due
to "reasons" (I never quite understood what the region code was doing
under the hood). Maybe we should only export names of devices the user
has explicitly tagged with -device foo,id=bar?

What should we do about the offset? Most devices export several regions
and there is no reason why those regions should all be together in the
memory map. Does just exposing a physical address make sense here?

-- 
Alex Bennée

[PATCH v1 9/9] .travis.yml: allow failure for unreliable hosts

[Alex Bennée]

They will still run but they won't get in the way of the result.

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

diff --git a/.travis.yml b/.travis.yml
index 564be50a3c1..ec6367af1f0 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -429,6 +429,7 @@ jobs:
       env:
         - TEST_CMD="make check check-tcg V=1"
         - CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS}"
+        - UNRELIABLE=true
 
     - name: "[ppc64] GCC check-tcg"
       arch: ppc64le
@@ -493,6 +494,7 @@ jobs:
       env:
         - TEST_CMD="make check check-tcg V=1"
         - CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS},s390x-linux-user"
+        - UNRELIABLE=true
       script:
         - ( cd ${SRC_DIR} ; git submodule update --init roms/SLOF )
         - BUILD_RC=0 && make -j${JOBS} || BUILD_RC=$?
@@ -535,6 +537,7 @@ jobs:
         - TEST_CMD="make check-unit"
         - CONFIG="--disable-containers --disable-tcg --enable-kvm
                   --disable-tools --host-cc=clang --cxx=clang++"
+        - UNRELIABLE=true
 
     # Release builds
     # The make-release script expect a QEMU version, so our tag must start with a 'v'.
@@ -556,3 +559,5 @@ jobs:
         - mkdir -p release-build && cd release-build
         - ../configure ${BASE_CONFIG} ${CONFIG} || { cat config.log && exit 1; }
         - make install
+  allow_failures:
+    - env: UNRELIABLE=true
-- 
2.20.1

Re: [PATCH v1 9/9] .travis.yml: allow failure for unreliable hosts

[Philippe Mathieu-Daudé]

On 6/2/20 5:46 PM, Alex Bennée wrote:
> They will still run but they won't get in the way of the result.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  .travis.yml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 564be50a3c1..ec6367af1f0 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -429,6 +429,7 @@ jobs:
>        env:
>          - TEST_CMD="make check check-tcg V=1"
>          - CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS}"
> +        - UNRELIABLE=true
>  
>      - name: "[ppc64] GCC check-tcg"
>        arch: ppc64le
> @@ -493,6 +494,7 @@ jobs:
>        env:
>          - TEST_CMD="make check check-tcg V=1"
>          - CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS},s390x-linux-user"
> +        - UNRELIABLE=true
>        script:
>          - ( cd ${SRC_DIR} ; git submodule update --init roms/SLOF )
>          - BUILD_RC=0 && make -j${JOBS} || BUILD_RC=$?
> @@ -535,6 +537,7 @@ jobs:
>          - TEST_CMD="make check-unit"
>          - CONFIG="--disable-containers --disable-tcg --enable-kvm
>                    --disable-tools --host-cc=clang --cxx=clang++"
> +        - UNRELIABLE=true
>  
>      # Release builds
>      # The make-release script expect a QEMU version, so our tag must start with a 'v'.
> @@ -556,3 +559,5 @@ jobs:
>          - mkdir -p release-build && cd release-build
>          - ../configure ${BASE_CONFIG} ${CONFIG} || { cat config.log && exit 1; }
>          - make install
> +  allow_failures:
> +    - env: UNRELIABLE=true
> 

It is frustrating when you are expecting Travis to fail to see test this
patch, but Travis is back working correctly...

Re: [PATCH v1 9/9] .travis.yml: allow failure for unreliable hosts

[Philippe Mathieu-Daudé]

On 6/3/20 10:18 AM, Philippe Mathieu-Daudé wrote:
> On 6/2/20 5:46 PM, Alex Bennée wrote:
>> They will still run but they won't get in the way of the result.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  .travis.yml | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/.travis.yml b/.travis.yml
>> index 564be50a3c1..ec6367af1f0 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -429,6 +429,7 @@ jobs:
>>        env:
>>          - TEST_CMD="make check check-tcg V=1"
>>          - CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS}"
>> +        - UNRELIABLE=true
>>  
>>      - name: "[ppc64] GCC check-tcg"
>>        arch: ppc64le
>> @@ -493,6 +494,7 @@ jobs:
>>        env:
>>          - TEST_CMD="make check check-tcg V=1"
>>          - CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS},s390x-linux-user"
>> +        - UNRELIABLE=true
>>        script:
>>          - ( cd ${SRC_DIR} ; git submodule update --init roms/SLOF )
>>          - BUILD_RC=0 && make -j${JOBS} || BUILD_RC=$?
>> @@ -535,6 +537,7 @@ jobs:
>>          - TEST_CMD="make check-unit"
>>          - CONFIG="--disable-containers --disable-tcg --enable-kvm
>>                    --disable-tools --host-cc=clang --cxx=clang++"
>> +        - UNRELIABLE=true
>>  
>>      # Release builds
>>      # The make-release script expect a QEMU version, so our tag must start with a 'v'.
>> @@ -556,3 +559,5 @@ jobs:
>>          - mkdir -p release-build && cd release-build
>>          - ../configure ${BASE_CONFIG} ${CONFIG} || { cat config.log && exit 1; }
>>          - make install
>> +  allow_failures:
>> +    - env: UNRELIABLE=true
>>
> 
> It is frustrating when you are expecting Travis to fail to see test this
> patch, but Travis is back working correctly...
> 

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

Apparently Travis fixed their problem:

$ df -h
Filesystem
                             Size  Used Avail Use% Mounted on
/var/snap/lxd/common/lxd/storage-pools/instances/containers/travis-job-philmd-qemu-694161395/rootfs
  19G  2.8G   16G  15% /

I couldn't test if allow_failures works as expected, anyway:
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v1 9/9] .travis.yml: allow failure for unreliable hosts

[Thomas Huth]

On 02/06/2020 17.46, Alex Bennée wrote:
> They will still run but they won't get in the way of the result.

What does this exactly mean? Will we still get a notification e-mail
when something went wrong during the test?

 Thomas

Re: [PATCH v1 0/9] plugins/next (bug fixes, hwprofile, lockstep)

[no-reply]

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



Hi,

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

Message-id: 20200602154624.4460-1-alex.bennee@linaro.org
Subject: [PATCH  v1 0/9] plugins/next (bug fixes, hwprofile, lockstep)
Type: series

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200602164911.5706-1-alex.bennee@linaro.org -> patchew/20200602164911.5706-1-alex.bennee@linaro.org
Switched to a new branch 'test'
54f2fc0 .travis.yml: allow failure for unreliable hosts
e3ba90e plugins: new hwprofile plugin
084ad01 plugins: add API to return a name for a IO device
77a2374 hw/virtio/pci: include vdev name in registered PCI sections
18ec36e cputlb: ensure we re-fill the TLB if it has reset
e341c5e tests/plugin: correctly honour io_count
cf0780c scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header
1ec5362 qemu-plugin.h: add missing include <stddef.h> to define size_t
3e2c523 plugins: new lockstep plugin for debugging TCG changes

=== OUTPUT BEGIN ===
1/9 Checking commit 3e2c523716d9 (plugins: new lockstep plugin for debugging TCG changes)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#55: 
new file mode 100644

total: 0 errors, 1 warnings, 355 lines checked

Patch 1/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/9 Checking commit 1ec536281b2d (qemu-plugin.h: add missing include <stddef.h> to define size_t)
3/9 Checking commit cf0780c17d3c (scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header)
4/9 Checking commit e341c5e754f4 (tests/plugin: correctly honour io_count)
5/9 Checking commit 18ec36e424f5 (cputlb: ensure we re-fill the TLB if it has reset)
6/9 Checking commit 77a2374e2c88 (hw/virtio/pci: include vdev name in registered PCI sections)
WARNING: line over 80 characters
#23: FILE: hw/virtio/virtio-pci.c:1393:
+static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy, const char *vdev_name)

total: 0 errors, 1 warnings, 63 lines checked

Patch 6/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/9 Checking commit 084ad019dd0b (plugins: add API to return a name for a IO device)
ERROR: "foo * bar" should be "foo *bar"
#23: FILE: include/qemu/qemu-plugin.h:341:
+char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);

ERROR: "foo * bar" should be "foo *bar"
#36: FILE: plugins/api.c:306:
+char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)

WARNING: line over 80 characters
#42: FILE: plugins/api.c:312:
+            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);

total: 2 errors, 1 warnings, 35 lines checked

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

8/9 Checking commit e3ba90ecbf5b (plugins: new hwprofile plugin)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#30: 
new file mode 100644

ERROR: line over 90 characters
#141: FILE: tests/plugin/hwprofile.c:107:
+                g_string_append_printf(report, "%s @ 0x%"PRIx64"\n", rec->name, rec->base);

WARNING: line over 80 characters
#144: FILE: tests/plugin/hwprofile.c:110:
+                    g_string_append_printf(report, "  off:%08"PRIx64, loc->offset);

ERROR: suspect code indent for conditional statements (20, 23)
#149: FILE: tests/plugin/hwprofile.c:115:
+                    if (track_writes()) {
+                       g_string_append_printf(report, ", 0x%04x, %"PRId64,

ERROR: space required after that ',' (ctx:VxV)
#153: FILE: tests/plugin/hwprofile.c:119:
+                    g_string_append_c(report,'\n');
                                             ^

ERROR: "foo * bar" should be "foo *bar"
#177: FILE: tests/plugin/hwprofile.c:143:
+static DeviceCounts * new_count(char *name, uint64_t base)

ERROR: "foo * bar" should be "foo *bar"
#189: FILE: tests/plugin/hwprofile.c:155:
+static IOLocationCounts * new_location(uint64_t offset)

ERROR: line over 90 characters
#219: FILE: tests/plugin/hwprofile.c:185:
+            IOLocationCounts *io_count = g_hash_table_lookup(counts->access_pattern, &off);

total: 6 errors, 2 warnings, 255 lines checked

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

9/9 Checking commit 54f2fc0935c4 (.travis.yml: allow failure for unreliable hosts)
=== OUTPUT END ===

Test command exited with code: 1


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

Re: [PATCH v1 0/9] plugins/next (bug fixes, hwprofile, lockstep)

[no-reply]

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



Hi,

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

Message-id: 20200602154624.4460-1-alex.bennee@linaro.org
Subject: [PATCH  v1 0/9] plugins/next (bug fixes, hwprofile, lockstep)
Type: series

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

Switched to a new branch 'test'
5e3c9f6 .travis.yml: allow failure for unreliable hosts
a829a1c plugins: new hwprofile plugin
fb39889 plugins: add API to return a name for a IO device
2d8b57c hw/virtio/pci: include vdev name in registered PCI sections
c275a76 cputlb: ensure we re-fill the TLB if it has reset
2ce5135 tests/plugin: correctly honour io_count
fbdf670 scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header
42a59cf qemu-plugin.h: add missing include <stddef.h> to define size_t
e3dc316 plugins: new lockstep plugin for debugging TCG changes

=== OUTPUT BEGIN ===
1/9 Checking commit e3dc3166ed69 (plugins: new lockstep plugin for debugging TCG changes)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#55: 
new file mode 100644

total: 0 errors, 1 warnings, 355 lines checked

Patch 1/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/9 Checking commit 42a59cf04068 (qemu-plugin.h: add missing include <stddef.h> to define size_t)
3/9 Checking commit fbdf67007614 (scripts/clean-includes: Mark 'qemu/qemu-plugin.h' as special header)
4/9 Checking commit 2ce513535a41 (tests/plugin: correctly honour io_count)
5/9 Checking commit c275a76fea13 (cputlb: ensure we re-fill the TLB if it has reset)
6/9 Checking commit 2d8b57ced74c (hw/virtio/pci: include vdev name in registered PCI sections)
WARNING: line over 80 characters
#23: FILE: hw/virtio/virtio-pci.c:1393:
+static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy, const char *vdev_name)

total: 0 errors, 1 warnings, 63 lines checked

Patch 6/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/9 Checking commit fb3988932bba (plugins: add API to return a name for a IO device)
ERROR: "foo * bar" should be "foo *bar"
#23: FILE: include/qemu/qemu-plugin.h:341:
+char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr);

ERROR: "foo * bar" should be "foo *bar"
#36: FILE: plugins/api.c:306:
+char * qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *haddr)

WARNING: line over 80 characters
#42: FILE: plugins/api.c:312:
+            return g_strdup_printf("anon%08lx", 0xffffffff & (uintptr_t) mrs->mr);

total: 2 errors, 1 warnings, 35 lines checked

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

8/9 Checking commit a829a1ca4c20 (plugins: new hwprofile plugin)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#30: 
new file mode 100644

ERROR: line over 90 characters
#141: FILE: tests/plugin/hwprofile.c:107:
+                g_string_append_printf(report, "%s @ 0x%"PRIx64"\n", rec->name, rec->base);

WARNING: line over 80 characters
#144: FILE: tests/plugin/hwprofile.c:110:
+                    g_string_append_printf(report, "  off:%08"PRIx64, loc->offset);

ERROR: suspect code indent for conditional statements (20, 23)
#149: FILE: tests/plugin/hwprofile.c:115:
+                    if (track_writes()) {
+                       g_string_append_printf(report, ", 0x%04x, %"PRId64,

ERROR: space required after that ',' (ctx:VxV)
#153: FILE: tests/plugin/hwprofile.c:119:
+                    g_string_append_c(report,'\n');
                                             ^

ERROR: "foo * bar" should be "foo *bar"
#177: FILE: tests/plugin/hwprofile.c:143:
+static DeviceCounts * new_count(char *name, uint64_t base)

ERROR: "foo * bar" should be "foo *bar"
#189: FILE: tests/plugin/hwprofile.c:155:
+static IOLocationCounts * new_location(uint64_t offset)

ERROR: line over 90 characters
#219: FILE: tests/plugin/hwprofile.c:185:
+            IOLocationCounts *io_count = g_hash_table_lookup(counts->access_pattern, &off);

total: 6 errors, 2 warnings, 255 lines checked

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

9/9 Checking commit 5e3c9f6b37c9 (.travis.yml: allow failure for unreliable hosts)
=== OUTPUT END ===

Test command exited with code: 1


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