[Qemu-devel] [PATCH v6 00/22] instrument: Add basic event instrumentation

[Qemu-devel] [PATCH v6 00/22] instrument: Add basic event instrumentation

[Lluís Vilanova]

This series adds an API to add instrumentation events.

It also provides additional APIs for:
* Controlling tracing events.
* Peek/poke guest memory.

TODO:
* Replace qi_event_gen_* with generating calls to arbitrary functions (e.g.,
  qi_event_gen_call(num_args, va_list)).
* Flush all TBs when an execution-time event is unset (to ensure it won't be
  called in the future).
* Flush all TBs when a translation-time event is set (to ensure no future events
  will be lost).

Undecided:
* Alternatively to the two last points above, provide an API to request a TB
  flush (much more flexible and can be more efficient, but requires instrumentor
  to clearly know differences between translation and execution).
* Pass a user-provided pointer to events (i.e., to avoid using global
  variables).
* Provide something like tracing's per-vCPU trace states (i.e., so that each
  vCPU can have different instrumentation code). Useful mainly for sampling
  (enable/disable instrumentation multiple times without re-translating guest
  code) and more complex use cases like tracing a guest process in softmmu mode.
  It's still not clear to me if we should extend the per-vCPU bitmap with
  instrumentation events, or otherwise somehow reuse the bits in tracing events
  (since they're currently limited).
* Allow multiple callbacks per event (both to support multiple callbacks
  installed by a library, and multiple libraries at the same time).
* Allow instr libraries to iterate on the list of guest CPUs (info is already
  available through guest_cpu_enter/guest_cpu_exit, but forces libs to be
  prepared for hot-plugging guest CPUs).

Future APIs (for later series):
* Peek/poke guest registers.
* Add breakpoints to trigger instrumentation functions.
* Trigger instrumentation functions from guest code (former hypertrace).
* Add events for guest code translation/execution (once the respective tracing
  events are accepted upstream).
* Add events for exceptions/syscalls.
* Add events for TB invalidation (necessary for libraries to deallocate any data
  they might have allocated for the TBs they instrumented).

The instrumentation code is dynamically loaded as a library into QEMU either
when it starts or later using its remote control interfaces. The loaded code
only has access to function explicitly exported through the QI_VPUBLIC macro.

This series is branch 'devel-instrument' in
https://code.gso.ac.upc.edu/git/qemu-dbi.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---

Changes in v6
=============

* Fix a minor style warning.
* Fix a minor compilation error.


Changes in v5
=============

* Rebase on fcea73709b.
* Minor changes to pass checkpatch.
* Fix symbol availability to external libraries by adding missing default symbol
  visibility flag.
* Use a string to identify instrumentation handles [Markus Armbruster].
* Use stubs for command line initialization.
* Use stubs to signal unsupported QAPI commands [Markus Armbruster].
* Use error messages instead of codes in QAPI commands [Markus Armbruster].
* Move symbol visibility macros to internal "qemu/compiler.h" header.
* Trigger event 'guest_cpu_enter' when library is loaded.
* Trigger event 'guest_cpu_exit' and flush TBs when library is unloaded.
* Rename instr_cpu_get/instr_cpu_set into clearer
  instr_cpu_to_qicpu/instr_cpu_from_qicpu.
* Rename handle_get/handle_put to clearer handle_new/handle_destroy.
* Ensure qi_event_set_* are called only on the proper mode and targets.


Changes in v4
=============

* Add missing stub function.


Changes in v3
=============

* Use a separate event set for instrumentation (i.e., do not instrument tracing
  events) [Stefan Hajnoczi].
* Add API for peek/poke guest memory.


Changes in v2
=============

* Update QEMU version in QAPI [Eric Blake].
* Clarify 'msg' result in QAPI is for humans only.
* Make 'msg' and 'handle' results optional in QAPI.
* Use a list of 'str' in 'instr-load' QAPI command.
* Update MAINTAINERS.
* Add macros for error-reporting in API.


Lluís Vilanova (22):
      instrument: Add documentation
      instrument: Add configure-time flag
      instrument: Add generic library loader
      instrument: [linux-user] Add command line library loader
      instrument: [bsd-user] Add command line library loader
      instrument: [softmmu] Add command line library loader
      instrument: [qapi] Add library loader
      instrument: [hmp] Add library loader
      instrument: Add basic control interface
      instrument: Add support for tracing events
      instrument: Track vCPUs
      instrument: Add event 'guest_cpu_enter'
      instrument: Support synchronous modification of vCPU state
      exec: Add function to synchronously flush TB on a stopped vCPU
      instrument: Add event 'guest_cpu_exit'
      instrument: Add event 'guest_cpu_reset'
      trace: Introduce a proper structure to describe memory accesses
      instrument: Add event 'guest_mem_before_trans'
      instrument: Add event 'guest_mem_before_exec'
      instrument: Add event 'guest_user_syscall'
      instrument: Add event 'guest_user_syscall_ret'
      instrument: Add API to manipulate guest memory


 .gitignore                                |    1 
 MAINTAINERS                               |    8 +
 Makefile                                  |    8 +
 Makefile.objs                             |    4 +
 Makefile.target                           |    1 
 accel/stubs/tcg-stub.c                    |    3 
 accel/tcg/translate-all.c                 |    7 +
 bsd-user/main.c                           |   17 ++
 bsd-user/syscall.c                        |   14 ++
 configure                                 |   13 ++
 cpus-common.c                             |    9 +
 docs/instrument.txt                       |  173 ++++++++++++++++++++++
 hmp-commands.hx                           |   32 ++++
 include/exec/cpu_ldst_template.h          |   19 +-
 include/exec/cpu_ldst_useronly_template.h |   19 +-
 include/exec/exec-all.h                   |    1 
 include/exec/helper-gen.h                 |    1 
 include/exec/helper-proto.h               |    1 
 include/exec/helper-tcg.h                 |    1 
 include/qemu/compiler.h                   |   19 ++
 instrument/Makefile.objs                  |    8 +
 instrument/cmdline.c                      |  128 ++++++++++++++++
 instrument/cmdline.h                      |   51 ++++++
 instrument/control.c                      |  228 +++++++++++++++++++++++++++++
 instrument/control.h                      |  153 +++++++++++++++++++
 instrument/control.inc.h                  |   67 +++++++++
 instrument/error.h                        |   34 ++++
 instrument/events.h                       |   86 +++++++++++
 instrument/events.inc.h                   |  109 ++++++++++++++
 instrument/helpers.h                      |    2 
 instrument/load.c                         |  210 +++++++++++++++++++++++++++
 instrument/load.h                         |   88 +++++++++++
 instrument/qemu-instr/control.h           |  177 +++++++++++++++++++++++
 instrument/qemu-instr/state.h             |  104 +++++++++++++
 instrument/qemu-instr/types.h             |  115 +++++++++++++++
 instrument/qemu-instr/types.inc.h         |   15 ++
 instrument/qmp.c                          |   82 ++++++++++
 instrument/state.c                        |   73 +++++++++
 instrument/trace.c                        |  125 ++++++++++++++++
 linux-user/main.c                         |   21 +++
 linux-user/syscall.c                      |    7 +
 monitor.c                                 |   43 +++++
 qapi-schema.json                          |    3 
 qapi/instrument.json                      |   49 ++++++
 qemu-options.hx                           |   19 ++
 qom/cpu.c                                 |    2 
 stubs/Makefile.objs                       |    1 
 stubs/instrument.c                        |   73 +++++++++
 tcg/tcg-op.c                              |   27 ++-
 trace/control-target.c                    |    2 
 trace/control.c                           |    4 -
 trace/control.h                           |   24 +++
 trace/mem-internal.h                      |   22 ++-
 trace/mem.h                               |    8 +
 vl.c                                      |   15 ++
 55 files changed, 2486 insertions(+), 40 deletions(-)
 create mode 100644 docs/instrument.txt
 create mode 100644 instrument/Makefile.objs
 create mode 100644 instrument/cmdline.c
 create mode 100644 instrument/cmdline.h
 create mode 100644 instrument/control.c
 create mode 100644 instrument/control.h
 create mode 100644 instrument/control.inc.h
 create mode 100644 instrument/error.h
 create mode 100644 instrument/events.h
 create mode 100644 instrument/events.inc.h
 create mode 100644 instrument/helpers.h
 create mode 100644 instrument/load.c
 create mode 100644 instrument/load.h
 create mode 100644 instrument/qemu-instr/control.h
 create mode 100644 instrument/qemu-instr/state.h
 create mode 100644 instrument/qemu-instr/types.h
 create mode 100644 instrument/qemu-instr/types.inc.h
 create mode 100644 instrument/qmp.c
 create mode 100644 instrument/state.c
 create mode 100644 instrument/trace.c
 create mode 100644 qapi/instrument.json
 create mode 100644 stubs/instrument.c


To: qemu-devel@nongnu.org
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Emilio G. Cota <cota@braap.org>
Cc: Eric Blake <eblake@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 MAINTAINERS         |    6 ++
 docs/instrument.txt |  173 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 179 insertions(+)
 create mode 100644 docs/instrument.txt

diff --git a/MAINTAINERS b/MAINTAINERS
index 36eeb42d19..fb0eaee06a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1486,6 +1486,12 @@ F: scripts/tracetool/
 F: docs/tracing.txt
 T: git git://github.com/stefanha/qemu.git tracing
 
+Event instrumentation
+M: Lluís Vilanova <vilanova@ac.upc.edu>
+M: Stefan Hajnoczi <stefanha@redhat.com>
+S: Maintained
+F: docs/instrument.txt
+
 TPM
 S: Orphan
 F: tpm.c
diff --git a/docs/instrument.txt b/docs/instrument.txt
new file mode 100644
index 0000000000..24a0d21fc7
--- /dev/null
+++ b/docs/instrument.txt
@@ -0,0 +1,173 @@
+= Event instrumentation =
+
+== Introduction ==
+
+Event instrumentation allows users to execute their own host-native code on a
+set of pre-defined events provided by QEMU. QEMU also exposes other
+functionality to peek/poke at the guest state (e.g., memory or registers), as
+well as interacting with tracing events. For those familiar with the term, this
+provides dynamic binary instrumentation, works on all QEMU-supported
+architectures, as well as works in both 'user' (standalone application) and
+'system' (full-system emulation) modes.
+
+Look at the headers installed by QEMU on the "qemu-instr" directory for further
+information beyond this document.
+
+
+== Loading an instrumentation library ==
+
+Instrumentation code can be bundled into a dynamic library, which can be later
+loaded into QEMU:
+
+* Using the command-line "-instr" argument.
+
+* Using the "instr-load" and "instr-unload" commands in the HMP and QMP
+  interfaces.
+
+
+== Example ==
+
+1. Configure QEMU with event instrumentation:
+
+    # instrument guest_cpu_enter and guest_mem_before
+    mkdir -p /path/to/qemu-build
+    cd /path/to/qemu-build
+    /path/to/qemu-source/configure \
+      --enable-instrument \
+      --prefix=/path/to/qemu-install
+
+2. Build and install QEMU:
+
+    make install
+
+3. Create the "Makefile" to build the instrumentation library:
+
+    mkdir -p /tmp/my-instrument
+    
+    cat > /tmp/my-instrument/Makefile <<EOF
+    QEMU_PATH=/tmp/qemu-install/
+    
+    CFLAGS += -g
+    CFLAGS += -O3
+    CFLAGS += -Werror -Wall
+    CFLAGS += -I$(QEMU_PATH)/include
+    
+    all: libtrace-instrument.la
+    
+    libtrace-instrument.la: instrument.lo
+            libtool --mode=link --tag=CC $(CC) -module -rpath /usr/local/lib -o $@ $^
+    
+    %.lo: %.c
+            libtool --mode=compile --tag=CC $(CC) $(CFLAGS) -c $^
+    
+    clean:
+            $(RM) -f *.o *.so *.lo
+            $(RM) -Rf .libs
+    EOF
+
+4. Write your instrumentation library:
+
+    cat > /tmp/my-instrument/instrument.c <<EOF
+    #include <stdio.h>
+    #include <assert.h>
+    
+    #include <qemu-instr/control.h>         /* manipulate events */
+    #include <qemu-instr/trace.h>           /* manipulate tracing */
+    
+    /* the address for the memory access is not known at translation time */
+    void guest_mem_before_trans(QICPU vcpu_trans, QITCGv_cpu vcpu_exec,
+                                QITCGv vaddr, QIMemInfo info)
+    {
+        printf("%s: %p %p %p %d %d %d %d\n", __func__, vcpu_trans, vcpu_exec, vaddr,
+               1 << info.size_shift, info.sign_extend, info.endianness, info.store);
+        if (info.store) {
+            /* generate at execution time only for memory writes */
+            qi_event_gen_guest_mem_before_exec(vcpu_exec, vaddr, info);
+        }
+    }
+    
+    /* called when QEMU executes a memory access */
+    void guest_mem_before_exec(QICPU vcpu, uint64_t vaddr, QIMemInfo info)
+    {
+        if (info.store) {
+            /* if called by TCG code, we'll only get writes (see above) */
+            printf("%s: %p %lx %d %d %d %d\n", __func__, vcpu, vaddr,
+                   1 << info.size_shift, info.sign_extend, info.endianness, info.store);
+        }
+    }
+    
+    /* called every time QEMU hotplugs a CPU */
+    void guest_cpu_enter(QICPU vcpu)
+    {
+        printf("%s: %p\n", __func__, vcpu);
+    
+        /* disable instrumentation and tracing after the first call */
+        static bool found = false;
+        if (found) {
+            qi_event_set_guest_cpu_enter(NULL);
+            QITraceEvent *ev = qi_trace_event_name("guest_cpu_enter");
+            assert(ev);
+            qi_trace_event_set_state_dynamic(ev, true);
+        } else {
+            found = true;
+        }
+    }
+    
+    static void fini(void *data)
+    {
+        /* diable all tracing events */
+        QITraceEventIter iter;
+        qi_trace_event_iter_init(&iter, NULL);
+        QITraceEvent *ev;
+        while ((ev = qi_trace_event_iter_next(&iter)) != NULL) {
+            if (qi_trace_event_get_state_static(ev)) {
+                qi_trace_event_set_state_dynamic(ev, false);
+            }
+        }
+    
+        /* instrumentation callbacks are automatically reset by QEMU */
+    }
+    
+    /* mandatory initialization function */
+    int main(int argc, const char **argv)
+    {
+        int i;
+        printf("init!\n");
+        printf("    argc :: %d\n", argc);
+        for (i = 0; i < argc; i++) {
+            printf("            -> %s\n", argv[i]);
+        }
+    
+        qi_set_fini(fini, NULL);
+    
+        /* instrument and trace events */
+        QITraceEvent *ev;
+    
+        qi_event_set_guest_cpu_enter(guest_cpu_enter);
+        ev = qi_trace_event_name("guest_cpu_enter");
+        assert(ev);
+        qi_trace_event_set_state_dynamic(ev, true);
+    
+        qi_event_set_guest_mem_before_trans(guest_mem_before_trans);
+        ev = qi_trace_event_name("guest_mem_before_trans");
+        assert(ev);
+        qi_trace_event_set_state_dynamic(ev, true);
+    
+        qi_event_set_guest_mem_before_exec(guest_mem_before_exec);
+        ev = qi_trace_event_name("guest_mem_before_exec");
+        assert(ev);
+        qi_trace_event_set_state_dynamic(ev, true);
+    
+        return 0;
+    }
+    EOF
+
+5. Compile the instrumentation library:
+
+    make -C /tmp/my-instrument
+
+6. Start QEMU with the instrumentation library:
+
+    /tmp/qemu-install/bin/qemu-system-x86_64 \
+        -instr file=/tmp/my-dinstrument/.libs/libtrace-instrument.so, \
+               arg=foo,arg=bar

[Qemu-devel] [PATCH v6 02/22] instrument: Add configure-time flag

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 configure |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/configure b/configure
index fd7e3a5e81..a21d1bceb9 100755
--- a/configure
+++ b/configure
@@ -356,6 +356,7 @@ pie=""
 qom_cast_debug="yes"
 trace_backends="log"
 trace_file="trace"
+instrument="no"
 spice=""
 rbd=""
 smartcard=""
@@ -886,6 +887,8 @@ for opt do
   ;;
   --with-trace-file=*) trace_file="$optarg"
   ;;
+  --enable-instrument) instrument="yes"
+  ;;
   --enable-gprof) gprof="yes"
   ;;
   --enable-gcov) gcov="yes"
@@ -1436,6 +1439,7 @@ Advanced options (experts only):
                            Available backends: $trace_backend_list
   --with-trace-file=NAME   Full PATH,NAME of file to store traces
                            Default:trace-<pid>
+  --enable-instrument      enable event instrumentation
   --disable-slirp          disable SLIRP userspace network connectivity
   --enable-tcg-interpreter enable TCG with bytecode interpreter (TCI)
   --oss-lib                path to OSS library
@@ -5366,6 +5370,7 @@ echo "Trace backends    $trace_backends"
 if have_backend "simple"; then
 echo "Trace output file $trace_file-<pid>"
 fi
+echo "instrumentation   $instrument"
 echo "spice support     $spice $(echo_version $spice $spice_protocol_version/$spice_server_version)"
 echo "rbd support       $rbd"
 echo "xfsctl support    $xfs"
@@ -6019,6 +6024,10 @@ if have_backend "syslog"; then
 fi
 echo "CONFIG_TRACE_FILE=$trace_file" >> $config_host_mak
 
+if test "$instrument" = "yes"; then
+  echo "CONFIG_INSTRUMENT=y" >> $config_host_mak
+fi
+
 if test "$rdma" = "yes" ; then
   echo "CONFIG_RDMA=y" >> $config_host_mak
 fi

[Qemu-devel] [PATCH v6 03/22] instrument: Add generic library loader

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 MAINTAINERS              |    1 
 Makefile.objs            |    4 +
 configure                |    3 +
 instrument/Makefile.objs |    4 +
 instrument/cmdline.c     |  128 +++++++++++++++++++++++++++++++++++
 instrument/cmdline.h     |   51 ++++++++++++++
 instrument/load.c        |  166 ++++++++++++++++++++++++++++++++++++++++++++++
 instrument/load.h        |   88 ++++++++++++++++++++++++
 stubs/Makefile.objs      |    1 
 stubs/instrument.c       |   18 +++++
 10 files changed, 464 insertions(+)
 create mode 100644 instrument/Makefile.objs
 create mode 100644 instrument/cmdline.c
 create mode 100644 instrument/cmdline.h
 create mode 100644 instrument/load.c
 create mode 100644 instrument/load.h
 create mode 100644 stubs/instrument.c

diff --git a/MAINTAINERS b/MAINTAINERS
index fb0eaee06a..6c0b12a69a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1491,6 +1491,7 @@ M: Lluís Vilanova <vilanova@ac.upc.edu>
 M: Stefan Hajnoczi <stefanha@redhat.com>
 S: Maintained
 F: docs/instrument.txt
+F: instrument/
 
 TPM
 S: Orphan
diff --git a/Makefile.objs b/Makefile.objs
index 24a4ea08b8..81a9218e14 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -97,6 +97,10 @@ version-obj-$(CONFIG_WIN32) += $(BUILD_DIR)/version.o
 util-obj-y +=  trace/
 target-obj-y += trace/
 
+######################################################################
+# instrument
+target-obj-y += instrument/
+
 ######################################################################
 # guest agent
 
diff --git a/configure b/configure
index a21d1bceb9..5175151317 100755
--- a/configure
+++ b/configure
@@ -6025,6 +6025,9 @@ fi
 echo "CONFIG_TRACE_FILE=$trace_file" >> $config_host_mak
 
 if test "$instrument" = "yes"; then
+  LDFLAGS="-rdynamic $LDFLAGS"          # limit symbols available to clients
+  QEMU_CFLAGS="-fvisibility=hidden $QEMU_CFLAGS"
+  LIBS="-ldl $LIBS"
   echo "CONFIG_INSTRUMENT=y" >> $config_host_mak
 fi
 
diff --git a/instrument/Makefile.objs b/instrument/Makefile.objs
new file mode 100644
index 0000000000..71994a4c85
--- /dev/null
+++ b/instrument/Makefile.objs
@@ -0,0 +1,4 @@
+# -*- mode: makefile -*-
+
+target-obj-$(CONFIG_INSTRUMENT) += cmdline.o
+target-obj-$(CONFIG_INSTRUMENT) += load.o
diff --git a/instrument/cmdline.c b/instrument/cmdline.c
new file mode 100644
index 0000000000..da7a7cbceb
--- /dev/null
+++ b/instrument/cmdline.c
@@ -0,0 +1,128 @@
+/*
+ * Control instrumentation during program (de)initialization.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include <dlfcn.h>
+#include "instrument/cmdline.h"
+#include "instrument/load.h"
+#include "qemu/config-file.h"
+#include "qemu/error-report.h"
+
+
+QemuOptsList qemu_instr_opts = {
+    .name = "instrument",
+    .implied_opt_name = "file",
+    .merge_lists = true,
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_instr_opts.head),
+    .desc = {
+        {
+            .name = "file",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "arg",
+            .type = QEMU_OPT_STRING,
+        },
+        { /* end of list */ }
+    },
+};
+
+void instr_opt_parse(const char *optarg, char **path,
+                     int *argc, const char ***argv)
+{
+    const char *arg;
+    QemuOptsIter iter;
+    QemuOpts *opts = qemu_opts_parse_noisily(qemu_find_opts("instrument"),
+                                             optarg, true);
+    if (!opts) {
+        exit(1);
+    } else {
+#if !defined(CONFIG_INSTRUMENT)
+        error_report("instrumentation not enabled on this build");
+        exit(1);
+#endif
+    }
+
+
+    arg = qemu_opt_get(opts, "file");
+    if (arg != NULL) {
+        g_free(*path);
+        *path = g_strdup(arg);
+    }
+
+    qemu_opt_iter_init(&iter, opts, "arg");
+    while ((arg = qemu_opt_iter_next(&iter)) != NULL) {
+        *argv = realloc(*argv, sizeof(**argv) * (*argc + 1));
+        (*argv)[*argc] = g_strdup(arg);
+        (*argc)++;
+    }
+
+    qemu_opts_del(opts);
+}
+
+void instr_init(const char *path, int argc, const char **argv)
+{
+#if defined(CONFIG_INSTRUMENT)
+    InstrLoadError err;
+
+    if (path == NULL) {
+        return;
+    }
+
+    if (atexit(instr_fini) != 0) {
+        fprintf(stderr, "error: atexit: %s\n", strerror(errno));
+        abort();
+    }
+
+    const char *id = "cmdline";
+    err = instr_load(path, argc, argv, &id);
+    switch (err) {
+    case INSTR_LOAD_OK:
+        error_report("instrument: loaded library with ID '%s'", id);
+        return;
+    case INSTR_LOAD_TOO_MANY:
+        error_report("instrument: tried to load too many libraries");
+        break;
+    case INSTR_LOAD_ID_EXISTS:
+        g_assert_not_reached();
+        break;
+    case INSTR_LOAD_ERROR:
+        error_report("instrument: library initialization returned non-zero");
+        break;
+    case INSTR_LOAD_DLERROR:
+        error_report("instrument: error loading library: %s", dlerror());
+        break;
+    }
+#else
+    error_report("instrument: not available");
+#endif
+
+    exit(1);
+}
+
+void instr_fini(void)
+{
+#if defined(CONFIG_INSTRUMENT)
+    InstrUnloadError err = instr_unload_all();
+
+    switch (err) {
+    case INSTR_UNLOAD_OK:
+        return;
+    case INSTR_UNLOAD_INVALID:
+        /* the user might have already unloaded it */
+        return;
+    case INSTR_UNLOAD_DLERROR:
+        error_report("instrument: error unloading library: %s", dlerror());
+        break;
+    }
+#else
+    error_report("instrument: not available");
+#endif
+
+    exit(1);
+}
diff --git a/instrument/cmdline.h b/instrument/cmdline.h
new file mode 100644
index 0000000000..3734f5f438
--- /dev/null
+++ b/instrument/cmdline.h
@@ -0,0 +1,51 @@
+/*
+ * Control instrumentation during program (de)initialization.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef INSTRUMENT__CMDLINE_H
+#define INSTRUMENT__CMDLINE_H
+
+#include "qemu/typedefs.h"
+
+
+/**
+ * Definition of QEMU options describing instrumentation subsystem
+ * configuration.
+ */
+extern QemuOptsList qemu_instr_opts;
+
+/**
+ * instr_opt_parse:
+ * @optarg: A string argument of --instrument command line argument
+ *
+ * Initialize instrument subsystem.
+ */
+void instr_opt_parse(const char *optarg, char **path,
+                     int *argc, const char ***argv);
+
+/**
+ * instr_init:
+ * @path: Path to dynamic trace instrumentation library.
+ * @argc: Number of arguments to the library's #qi_init routine.
+ * @argv: Arguments to the library's #qi_init routine.
+ *
+ * Load and initialize the given instrumentation library. Calls exit() if the
+ * library's initialization function returns a non-zero value.
+ *
+ * Installs instr_fini() as an atexit() callback.
+ */
+void instr_init(const char *path, int argc, const char **argv);
+
+/**
+ * instr_fini:
+ *
+ * Deinitialize and unload all instrumentation libraries.
+ */
+void instr_fini(void);
+
+#endif  /* INSTRUMENT__CMDLINE_H */
diff --git a/instrument/load.c b/instrument/load.c
new file mode 100644
index 0000000000..af98f4ce38
--- /dev/null
+++ b/instrument/load.c
@@ -0,0 +1,166 @@
+/*
+ * Interface for (un)loading instrumentation libraries.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+
+#include <dlfcn.h>
+#include "instrument/load.h"
+#include "qemu/config-file.h"
+#include "qemu/error-report.h"
+
+
+typedef struct InstrHandle {
+    char *id;
+    void *dlhandle;
+    QLIST_ENTRY(InstrHandle) list;
+} InstrHandle;
+
+
+static unsigned int handle_auto_id;
+static QLIST_HEAD(, InstrHandle) handles = QLIST_HEAD_INITIALIZER(handles);
+static QemuMutex instr_lock;
+
+
+static InstrHandle *handle_new(const char **id)
+{
+    /* instr_lock is locked */
+    InstrHandle *res = g_malloc0(sizeof(InstrHandle));
+    if (!*id) {
+        *id = g_strdup_printf("lib%d", handle_auto_id);
+        handle_auto_id++;
+    }
+    res->id = g_strdup(*id);
+    QLIST_INSERT_HEAD(&handles, res, list);
+    return res;
+}
+
+static void handle_destroy(InstrHandle *handle)
+{
+    /* instr_lock is locked */
+    QLIST_REMOVE(handle, list);
+    g_free(handle->id);
+    g_free(handle);
+}
+
+static InstrHandle *handle_find(const char *id)
+{
+    /* instr_lock is locked */
+    InstrHandle *handle;
+    QLIST_FOREACH(handle, &handles, list) {
+        if (strcmp(handle->id, id) == 0) {
+            return handle;
+        }
+    }
+    return NULL;
+}
+
+InstrLoadError instr_load(const char *path, int argc, const char **argv,
+                          const char **id)
+{
+    InstrLoadError res;
+    InstrHandle *handle;
+    int (*main_cb)(int, const char **);
+    int main_res;
+
+    qemu_rec_mutex_lock(&instr_lock);
+
+    if (*id && handle_find(*id)) {
+        res = INSTR_LOAD_ID_EXISTS;
+        goto out;
+    }
+
+    if (!QLIST_EMPTY(&handles) > 0) {
+        /* XXX: This is in fact a hard-coded limit, but there's no reason why a
+         *      real multi-library implementation should fail.
+         */
+        res = INSTR_LOAD_TOO_MANY;
+        goto out;
+    }
+
+    handle = handle_new(id);
+    handle->dlhandle = dlopen(path, RTLD_NOW);
+    if (handle->dlhandle == NULL) {
+        res = INSTR_LOAD_DLERROR;
+        goto err;
+    }
+
+    main_cb = dlsym(handle->dlhandle, "main");
+    if (main_cb == NULL) {
+        res = INSTR_LOAD_DLERROR;
+        goto err;
+    }
+
+    main_res = main_cb(argc, argv);
+
+    if (main_res != 0) {
+        res = INSTR_LOAD_ERROR;
+        goto err;
+    }
+
+    res = INSTR_LOAD_OK;
+    goto out;
+
+err:
+    handle_destroy(handle);
+out:
+    qemu_rec_mutex_unlock(&instr_lock);
+    return res;
+}
+
+InstrUnloadError instr_unload(const char *id)
+{
+    InstrUnloadError res;
+
+    qemu_rec_mutex_lock(&instr_lock);
+
+    InstrHandle *handle = handle_find(id);
+    if (handle == NULL) {
+        res = INSTR_UNLOAD_INVALID;
+        goto out;
+    }
+
+    /* this should never fail */
+    if (dlclose(handle->dlhandle) < 0) {
+        res = INSTR_UNLOAD_DLERROR;
+    } else {
+        res = INSTR_UNLOAD_OK;
+    }
+    handle_destroy(handle);
+
+out:
+    qemu_rec_mutex_unlock(&instr_lock);
+    return res;
+}
+
+InstrUnloadError instr_unload_all(void)
+{
+    InstrUnloadError res = INSTR_UNLOAD_OK;
+
+    qemu_rec_mutex_lock(&instr_lock);
+    while (true) {
+        InstrHandle *handle = QLIST_FIRST(&handles);
+        if (handle == NULL) {
+            break;
+        } else {
+            res = instr_unload(handle->id);
+            if (res != INSTR_UNLOAD_OK) {
+                break;
+            }
+        }
+    }
+    qemu_rec_mutex_unlock(&instr_lock);
+
+    return res;
+}
+
+static void __attribute__((constructor)) instr_lock_init(void)
+{
+    qemu_rec_mutex_init(&instr_lock);
+}
diff --git a/instrument/load.h b/instrument/load.h
new file mode 100644
index 0000000000..162e09f9c9
--- /dev/null
+++ b/instrument/load.h
@@ -0,0 +1,88 @@
+/*
+ * Interface for (un)loading instrumentation libraries.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+
+#ifndef INSTRUMENT_LOAD_H
+#define INSTRUMENT_LOAD_H
+
+#include "qemu/osdep.h"
+
+#include "qapi-types.h"
+#include "qemu/queue.h"
+#include "qemu/thread.h"
+
+
+/**
+ * InstrLoadError:
+ * @INSTR_LOAD_OK: Correctly loaded.
+ * @INSTR_LOAD_ID_EXISTS: Tried to load an instrumentation libraries with an
+ *     existing ID.
+ * @INSTR_LOAD_TOO_MANY: Tried to load too many instrumentation libraries.
+ * @INSTR_LOAD_ERROR: The library's main() function returned a non-zero value.
+ * @INSTR_LOAD_DLERROR: Error with libdl (see dlerror).
+ *
+ * Error codes for instr_load().
+ */
+typedef enum {
+    INSTR_LOAD_OK,
+    INSTR_LOAD_ID_EXISTS,
+    INSTR_LOAD_TOO_MANY,
+    INSTR_LOAD_ERROR,
+    INSTR_LOAD_DLERROR,
+} InstrLoadError;
+
+/**
+ * InstrUnloadError:
+ * @INSTR_UNLOAD_OK: Correctly unloaded.
+ * @INSTR_UNLOAD_INVALID: Invalid handle.
+ * @INSTR_UNLOAD_DLERROR: Error with libdl (see dlerror).
+ *
+ * Error codes for instr_unload().
+ */
+typedef enum {
+    INSTR_UNLOAD_OK,
+    INSTR_UNLOAD_INVALID,
+    INSTR_UNLOAD_DLERROR,
+} InstrUnloadError;
+
+/**
+ * instr_load:
+ * @path: Path to the shared library to load.
+ * @argc: Number of arguments passed to the initialization function of the
+ *     library.
+ * @argv: Arguments passed to the initialization function of the library.
+ * @id: Instrumentation library id.
+ *
+ * Load a dynamic trace instrumentation library.
+ *
+ * Returns: Whether the library could be loaded.
+ */
+InstrLoadError instr_load(const char *path, int argc, const char **argv,
+                          const char **id);
+
+/**
+ * instr_unload:
+ * @id: Instrumentation library id passed to instr_load().
+ *
+ * Unload the given instrumentation library.
+ *
+ * Returns: Whether the library could be unloaded.
+ */
+InstrUnloadError instr_unload(const char *id);
+
+/**
+ * instr_unload_all:
+ *
+ * Unload all instrumentation libraries.
+ *
+ * Returns: Whether any library could not be unloaded.
+ */
+InstrUnloadError instr_unload_all(void);
+
+#endif  /* INSTRUMENT_LOAD_H */
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 4a33495911..4bf342cb96 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -13,6 +13,7 @@ stub-obj-y += error-printf.o
 stub-obj-y += fdset.o
 stub-obj-y += gdbstub.o
 stub-obj-y += get-vm-name.o
+stub-obj-y += instrument.o
 stub-obj-y += iothread.o
 stub-obj-y += iothread-lock.o
 stub-obj-y += is-daemonized.o
diff --git a/stubs/instrument.c b/stubs/instrument.c
new file mode 100644
index 0000000000..7d66f75454
--- /dev/null
+++ b/stubs/instrument.c
@@ -0,0 +1,18 @@
+/*
+ * Instrumentation placeholders.
+ *
+ * Copyright (C) 2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "instrument/cmdline.h"
+
+
+void instr_init(const char *path, int argc, const char **argv)
+{
+}
+void instr_fini(void)
+{
+}

[Qemu-devel] [PATCH v6 04/22] instrument: [linux-user] Add command line library loader

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 linux-user/main.c    |   21 +++++++++++++++++++++
 linux-user/syscall.c |    4 ++++
 2 files changed, 25 insertions(+)

diff --git a/linux-user/main.c b/linux-user/main.c
index 03666ef657..ac5c30c1fb 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -36,6 +36,7 @@
 #include "exec/log.h"
 #include "trace/control.h"
 #include "glib-compat.h"
+#include "instrument/cmdline.h"
 
 char *exec_path;
 
@@ -4017,6 +4018,17 @@ static void handle_arg_trace(const char *arg)
     trace_file = trace_opt_parse(arg);
 }
 
+static char *instrument_path;
+static int instrument_argc;
+static const char **instrument_argv;
+#if defined(CONFIG_INSTRUMENT)
+static void handle_arg_instrument(const char *arg)
+{
+    instr_opt_parse(arg, &instrument_path,
+                    &instrument_argc, &instrument_argv);
+}
+#endif
+
 struct qemu_argument {
     const char *argv;
     const char *env;
@@ -4066,6 +4078,10 @@ static const struct qemu_argument arg_table[] = {
      "",           "Seed for pseudo-random number generator"},
     {"trace",      "QEMU_TRACE",       true,  handle_arg_trace,
      "",           "[[enable=]<pattern>][,events=<file>][,file=<file>]"},
+#if defined(CONFIG_INSTRUMENT)
+    {"instr",      "QEMU_INSTR",       true,  handle_arg_instrument,
+     "",           "[file=]<file>[,arg=<string>]"},
+#endif
     {"version",    "QEMU_VERSION",     false, handle_arg_version,
      "",           "display version information and exit"},
     {NULL, NULL, false, NULL, NULL, NULL}
@@ -4257,6 +4273,9 @@ int main(int argc, char **argv, char **envp)
     srand(time(NULL));
 
     qemu_add_opts(&qemu_trace_opts);
+#if defined(CONFIG_INSTRUMENT)
+    qemu_add_opts(&qemu_instr_opts);
+#endif
 
     optind = parse_args(argc, argv);
 
@@ -4265,6 +4284,8 @@ int main(int argc, char **argv, char **envp)
     }
     trace_init_file(trace_file);
 
+    instr_init(instrument_path, instrument_argc, instrument_argv);
+
     /* Zero out regs */
     memset(regs, 0, sizeof(struct target_pt_regs));
 
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9b6364a266..e73a07fa6f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -115,6 +115,8 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
 #include "uname.h"
 
 #include "qemu.h"
+#include "instrument/cmdline.h"
+
 
 #ifndef CLONE_IO
 #define CLONE_IO                0x80000000      /* Clone io context */
@@ -7765,6 +7767,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         _mcleanup();
 #endif
         gdb_exit(cpu_env, arg1);
+        instr_fini();
         _exit(arg1);
         ret = 0; /* avoid warning */
         break;
@@ -9821,6 +9824,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         _mcleanup();
 #endif
         gdb_exit(cpu_env, arg1);
+        instr_fini();
         ret = get_errno(exit_group(arg1));
         break;
 #endif

[Qemu-devel] [PATCH v6 05/22] instrument: [bsd-user] Add command line library loader

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 bsd-user/main.c    |   17 +++++++++++++++++
 bsd-user/syscall.c |    5 +++++
 2 files changed, 22 insertions(+)

diff --git a/bsd-user/main.c b/bsd-user/main.c
index 8a6706a1c8..104844edfc 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -33,6 +33,7 @@
 #include "exec/log.h"
 #include "trace/control.h"
 #include "glib-compat.h"
+#include "instrument/cmdline.h"
 
 int singlestep;
 unsigned long mmap_min_addr;
@@ -667,6 +668,11 @@ static void usage(void)
            "-B address        set guest_base address to address\n"
            "-bsd type         select emulated BSD type FreeBSD/NetBSD/OpenBSD (default)\n"
            "\n"
+#if defined(CONFIG_INSTRUMENT)
+           "-instr [file=]<file>[,arg=<string>]\n"
+           "                  load an instrumentation library\n"
+           "\n"
+#endif
            "Debug options:\n"
            "-d item1[,...]    enable logging of specified items\n"
            "                  (use '-d help' for a list of log items)\n"
@@ -738,6 +744,9 @@ int main(int argc, char **argv)
     envlist_t *envlist = NULL;
     char *trace_file = NULL;
     bsd_type = target_openbsd;
+    char *instrument_path = NULL;
+    int instrument_argc = 0;
+    const char **instrument_argv = NULL;
 
     if (argc <= 1)
         usage();
@@ -756,6 +765,9 @@ int main(int argc, char **argv)
     cpu_model = NULL;
 
     qemu_add_opts(&qemu_trace_opts);
+#if defined(CONFIG_INSTRUMENT)
+    qemu_add_opts(&qemu_instr_opts);
+#endif
 
     optind = 1;
     for (;;) {
@@ -843,6 +855,9 @@ int main(int argc, char **argv)
         } else if (!strcmp(r, "trace")) {
             g_free(trace_file);
             trace_file = trace_opt_parse(optarg);
+        } else if (!strcmp(r, "instr")) {
+            instr_opt_parse(optarg, &instrument_path,
+                            &instrument_argc, &instrument_argv);
         } else {
             usage();
         }
@@ -872,6 +887,8 @@ int main(int argc, char **argv)
     }
     trace_init_file(trace_file);
 
+    instr_init(instrument_path, instrument_argc, instrument_argv);
+
     /* Zero out regs */
     memset(regs, 0, sizeof(struct target_pt_regs));
 
diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
index 66492aaf5d..3230f722f3 100644
--- a/bsd-user/syscall.c
+++ b/bsd-user/syscall.c
@@ -26,6 +26,8 @@
 
 #include "qemu.h"
 #include "qemu-common.h"
+#include "instrument/cmdline.h"
+
 
 //#define DEBUG
 
@@ -332,6 +334,7 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, abi_long arg1,
         _mcleanup();
 #endif
         gdb_exit(cpu_env, arg1);
+        instr_fini();
         /* XXX: should free thread stack and CPU env */
         _exit(arg1);
         ret = 0; /* avoid warning */
@@ -430,6 +433,7 @@ abi_long do_netbsd_syscall(void *cpu_env, int num, abi_long arg1,
         _mcleanup();
 #endif
         gdb_exit(cpu_env, arg1);
+        instr_fini();
         /* XXX: should free thread stack and CPU env */
         _exit(arg1);
         ret = 0; /* avoid warning */
@@ -505,6 +509,7 @@ abi_long do_openbsd_syscall(void *cpu_env, int num, abi_long arg1,
         _mcleanup();
 #endif
         gdb_exit(cpu_env, arg1);
+        instr_fini();
         /* XXX: should free thread stack and CPU env */
         _exit(arg1);
         ret = 0; /* avoid warning */

[Qemu-devel] [PATCH v6 06/22] instrument: [softmmu] Add command line library loader

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 qemu-options.hx |   19 +++++++++++++++++++
 vl.c            |   15 +++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 9f6e2adfff..6947388aab 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4077,6 +4077,25 @@ HXCOMM HX does not support conditional compilation of text.
 @findex -trace
 @include qemu-option-trace.texi
 ETEXI
+#if defined(CONFIG_INSTRUMENT)
+DEF("instr", HAS_ARG, QEMU_OPTION_instr,
+    "-instr [file=]<file>[,arg=<string>]\n"
+    "                load an instrumentation library\n",
+    QEMU_ARCH_ALL)
+#endif
+STEXI
+@item -instr file=@var{file}[,arg=@var{string}]
+@findex -instr
+
+Load a dynamic trace instrumentation library.
+
+@table @option
+@item file=@var{file}
+Load the given dynamic trace instrumentation library.
+@item arg=@var{string}
+String argument passed as to the library's @code{qi_init} routine (can be given multiple times).
+@end table
+ETEXI
 
 HXCOMM Internal use
 DEF("qtest", HAS_ARG, QEMU_OPTION_qtest, "", QEMU_ARCH_ALL)
diff --git a/vl.c b/vl.c
index fb1f05b937..aea05ed4cc 100644
--- a/vl.c
+++ b/vl.c
@@ -118,6 +118,7 @@ int main(int argc, char **argv)
 
 #include "trace-root.h"
 #include "trace/control.h"
+#include "instrument/cmdline.h"
 #include "qemu/queue.h"
 #include "sysemu/arch_init.h"
 
@@ -3037,6 +3038,9 @@ int main(int argc, char **argv, char **envp)
     } BlockdevOptions_queue;
     QSIMPLEQ_HEAD(, BlockdevOptions_queue) bdo_queue
         = QSIMPLEQ_HEAD_INITIALIZER(bdo_queue);
+    char *instrument_path = NULL;
+    int instrument_argc = 0;
+    const char **instrument_argv = NULL;
 
     module_call_init(MODULE_INIT_TRACE);
 
@@ -3064,6 +3068,9 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_global_opts);
     qemu_add_opts(&qemu_mon_opts);
     qemu_add_opts(&qemu_trace_opts);
+#if defined(CONFIG_INSTRUMENT)
+    qemu_add_opts(&qemu_instr_opts);
+#endif
     qemu_add_opts(&qemu_option_rom_opts);
     qemu_add_opts(&qemu_machine_opts);
     qemu_add_opts(&qemu_accel_opts);
@@ -4009,6 +4016,12 @@ int main(int argc, char **argv, char **envp)
                 g_free(trace_file);
                 trace_file = trace_opt_parse(optarg);
                 break;
+#if defined(CONFIG_INSTRUMENT)
+            case QEMU_OPTION_instr:
+                instr_opt_parse(optarg, &instrument_path,
+                                &instrument_argc, &instrument_argv);
+                break;
+#endif
             case QEMU_OPTION_readconfig:
                 {
                     int ret = qemu_read_config_file(optarg);
@@ -4196,6 +4209,8 @@ int main(int argc, char **argv, char **envp)
     }
     trace_init_file(trace_file);
 
+    instr_init(instrument_path, instrument_argc, instrument_argv);
+
     /* Open the logfile at this point and set the log mask if necessary.
      */
     if (log_file) {

[Qemu-devel] [PATCH v6 07/22] instrument: [qapi] Add library loader

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 MAINTAINERS              |    1 +
 Makefile                 |    1 +
 instrument/Makefile.objs |    1 +
 instrument/qmp.c         |   82 ++++++++++++++++++++++++++++++++++++++++++++++
 monitor.c                |    4 ++
 qapi-schema.json         |    3 ++
 qapi/instrument.json     |   49 +++++++++++++++++++++++++++
 stubs/instrument.c       |   22 ++++++++++++
 8 files changed, 163 insertions(+)
 create mode 100644 instrument/qmp.c
 create mode 100644 qapi/instrument.json

diff --git a/MAINTAINERS b/MAINTAINERS
index 6c0b12a69a..edddab0502 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1492,6 +1492,7 @@ M: Stefan Hajnoczi <stefanha@redhat.com>
 S: Maintained
 F: docs/instrument.txt
 F: instrument/
+F: qapi/instrument.json
 
 TPM
 S: Orphan
diff --git a/Makefile b/Makefile
index 337a1f6f9b..3861b3f49c 100644
--- a/Makefile
+++ b/Makefile
@@ -412,6 +412,7 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
                $(SRC_PATH)/qapi/block.json $(SRC_PATH)/qapi/block-core.json \
                $(SRC_PATH)/qapi/char.json \
                $(SRC_PATH)/qapi/crypto.json \
+               $(SRC_PATH)/qapi/instrument.json \
                $(SRC_PATH)/qapi/introspect.json \
                $(SRC_PATH)/qapi/migration.json \
                $(SRC_PATH)/qapi/net.json \
diff --git a/instrument/Makefile.objs b/instrument/Makefile.objs
index 71994a4c85..7bf4e27e3c 100644
--- a/instrument/Makefile.objs
+++ b/instrument/Makefile.objs
@@ -2,3 +2,4 @@
 
 target-obj-$(CONFIG_INSTRUMENT) += cmdline.o
 target-obj-$(CONFIG_INSTRUMENT) += load.o
+target-obj-$(CONFIG_INSTRUMENT) += qmp.o
diff --git a/instrument/qmp.c b/instrument/qmp.c
new file mode 100644
index 0000000000..e4464aa5eb
--- /dev/null
+++ b/instrument/qmp.c
@@ -0,0 +1,82 @@
+/*
+ * QMP interface for instrumentation control commands.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include <dlfcn.h>
+
+#include "instrument/load.h"
+#include "qemu-common.h"
+#include "qapi/qmp/qerror.h"
+#include "qmp-commands.h"
+
+
+InstrLoadResult *qmp_instr_load(const char *path,
+                                bool has_id, const char *id,
+                                bool have_args, strList *args,
+                                Error **errp)
+{
+    InstrLoadResult *res = g_malloc0(sizeof(*res));
+    int argc = 0;
+    const char **argv = NULL;
+    InstrLoadError code;
+
+    if (!has_id) {
+        id = NULL;
+    }
+
+    strList *entry = have_args ? args : NULL;
+    while (entry != NULL) {
+        argv = realloc(argv, sizeof(*argv) * (argc + 1));
+        argv[argc] = entry->value;
+        argc++;
+        entry = entry->next;
+    }
+
+    code = instr_load(path, argc, argv, &id);
+    switch (code) {
+    case INSTR_LOAD_OK:
+        res->id = g_strdup(id);
+        break;
+    case INSTR_LOAD_ID_EXISTS:
+        error_setg(errp, "Library ID exists");
+        break;
+    case INSTR_LOAD_TOO_MANY:
+        error_setg(errp, "Tried to load too many libraries");
+        break;
+    case INSTR_LOAD_ERROR:
+        error_setg(errp, "Library initialization returned non-zero");
+        break;
+    case INSTR_LOAD_DLERROR:
+        error_setg(errp, "Error loading library: %s",
+                   dlerror());
+        break;
+    }
+
+    if (*errp) {
+        g_free(res);
+        res = NULL;
+    }
+
+    return res;
+}
+
+void qmp_instr_unload(const char *id, Error **errp)
+{
+    InstrUnloadError code = instr_unload(id);
+    switch (code) {
+    case INSTR_UNLOAD_OK:
+        break;
+    case INSTR_UNLOAD_INVALID:
+        error_setg(errp, "Unknown library ID");
+        break;
+    case INSTR_UNLOAD_DLERROR:
+        error_setg(errp, "Error unloading library: %s", dlerror());
+        break;
+    }
+}
diff --git a/monitor.c b/monitor.c
index 9239f7adde..e031aa2687 100644
--- a/monitor.c
+++ b/monitor.c
@@ -978,6 +978,10 @@ static void qmp_unregister_commands_hack(void)
     qmp_unregister_command(&qmp_commands, "query-xen-replication-status");
     qmp_unregister_command(&qmp_commands, "xen-colo-do-checkpoint");
 #endif
+#ifndef CONFIG_INSTRUMENT
+    qmp_unregister_command(&qmp_commands, "instr-load");
+    qmp_unregister_command(&qmp_commands, "instr-unload");
+#endif
 #ifndef TARGET_I386
     qmp_unregister_command(&qmp_commands, "rtc-reset-reinjection");
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index f3af2cb851..706c64659f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -93,6 +93,9 @@
 { 'include': 'qapi/trace.json' }
 { 'include': 'qapi/introspect.json' }
 
+# Instrumentation commands
+{ 'include': 'qapi/instrument.json' }
+
 ##
 # = Miscellanea
 ##
diff --git a/qapi/instrument.json b/qapi/instrument.json
new file mode 100644
index 0000000000..c59bee74cb
--- /dev/null
+++ b/qapi/instrument.json
@@ -0,0 +1,49 @@
+# *-*- Mode: Python -*-*
+#
+# Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.
+# See the COPYING file in the top-level directory.
+
+##
+# QAPI instrumentation control commands.
+##
+
+##
+# @InstrLoadResult:
+#
+# Result of an 'instr-load' command.
+#
+# @id: instrumentation library ID
+#
+# Since: 2.11
+##
+{ 'struct': 'InstrLoadResult',
+  'data': { 'id': 'str' } }
+
+##
+# @instr-load:
+#
+# Load an instrumentation library.
+#
+# @path: path to the dynamic instrumentation library
+# @id: unique ID for the loaded library
+# @args: arguments to the dynamic instrumentation library
+#
+# Since: 2.11
+##
+{ 'command': 'instr-load',
+  'data':    { 'path': 'str', '*id': 'str', '*args': ['str'] },
+  'returns': 'InstrLoadResult' }
+
+##
+# @instr-unload:
+#
+# Unload an instrumentation library.
+#
+# @id: unique ID passed to instr-load().
+#
+# Since: 2.11
+##
+{ 'command': 'instr-unload',
+  'data': { 'id': 'str' } }
diff --git a/stubs/instrument.c b/stubs/instrument.c
index 7d66f75454..292a2cdf26 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -7,7 +7,11 @@
  * See the COPYING file in the top-level directory.
  */
 
+#include "qemu/osdep.h"
+
 #include "instrument/cmdline.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
 
 
 void instr_init(const char *path, int argc, const char **argv)
@@ -16,3 +20,21 @@ void instr_init(const char *path, int argc, const char **argv)
 void instr_fini(void)
 {
 }
+
+InstrLoadResult *qmp_instr_load(const char *path,
+                                bool has_id, const char *id,
+                                bool have_args, strList *args,
+                                Error **errp);
+InstrLoadResult *qmp_instr_load(const char *path,
+                                bool has_id, const char *id,
+                                bool have_args, strList *args,
+                                Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+void qmp_instr_unload(const char *id, Error **errp);
+void qmp_instr_unload(const char *id, Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+}

[Qemu-devel] [PATCH v6 08/22] instrument: [hmp] Add library loader

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 hmp-commands.hx |   32 ++++++++++++++++++++++++++++++++
 monitor.c       |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 1941e19932..2e8ebe8422 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1858,6 +1858,38 @@ ETEXI
         .sub_table  = info_cmds,
     },
 
+#ifdef CONFIG_INSTRUMENT
+    {
+        .name       = "instr-load",
+        .args_type  = "path:F,id:s?,arg:s?",
+        .params     = "path [id] [arg]",
+        .help       = "load an instrumentation library",
+        .cmd        = hmp_instr_load,
+    },
+#endif
+
+STEXI
+@item instr-load @var{path} [@var{id}] [@var{arg}]
+@findex instr-load
+Load an instrumentation library.
+ETEXI
+
+#ifdef CONFIG_INSTRUMENT
+    {
+        .name       = "instr-unload",
+        .args_type  = "id:s",
+        .params     = "id",
+        .help       = "unload an instrumentation library",
+        .cmd        = hmp_instr_unload,
+    },
+#endif
+
+STEXI
+@item instr-unload
+@findex instr-unload
+Unload an instrumentation library.
+ETEXI
+
 STEXI
 @end table
 ETEXI
diff --git a/monitor.c b/monitor.c
index e031aa2687..7b80d5351f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2323,6 +2323,45 @@ int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp)
     return fd;
 }
 
+#ifdef CONFIG_INSTRUMENT
+static void hmp_instr_load(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    const char *path = qdict_get_str(qdict, "path");
+    const char *id = qdict_get_try_str(qdict, "id");
+    const char *str = qdict_get_try_str(qdict, "arg");
+    strList args;
+
+    args.value = (char *)str;
+    args.next = NULL;
+
+    InstrLoadResult *res = qmp_instr_load(path,
+                                          id != NULL, id,
+                                          args.value != NULL, &args,
+                                          &err);
+    if (err) {
+        error_report_err(err);
+    } else {
+        monitor_printf(mon, "Handle: %s\n", res->id);
+        monitor_printf(mon, "OK\n");
+    }
+    qapi_free_InstrLoadResult(res);
+}
+
+static void hmp_instr_unload(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    const char *id = qdict_get_str(qdict, "id");
+
+    qmp_instr_unload(id, &err);
+    if (err) {
+        error_report_err(err);
+    } else {
+        monitor_printf(mon, "OK\n");
+    }
+}
+#endif
+
 /* Please update hmp-commands.hx when adding or changing commands */
 static mon_cmd_t info_cmds[] = {
 #include "hmp-commands-info.h"

[Qemu-devel] [PATCH v6 09/22] instrument: Add basic control interface

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 Makefile                        |    4 +++
 configure                       |    1 +
 include/qemu/compiler.h         |   19 ++++++++++++++++
 instrument/Makefile.objs        |    1 +
 instrument/control.c            |   28 ++++++++++++++++++++++++
 instrument/control.h            |   44 +++++++++++++++++++++++++++++++++++++
 instrument/control.inc.h        |   25 +++++++++++++++++++++
 instrument/error.h              |   28 ++++++++++++++++++++++++
 instrument/events.h             |   37 +++++++++++++++++++++++++++++++
 instrument/events.inc.h         |   11 +++++++++
 instrument/load.c               |   13 +++++++++++
 instrument/qemu-instr/control.h |   46 +++++++++++++++++++++++++++++++++++++++
 stubs/instrument.c              |    4 +++
 13 files changed, 261 insertions(+)
 create mode 100644 instrument/control.c
 create mode 100644 instrument/control.h
 create mode 100644 instrument/control.inc.h
 create mode 100644 instrument/error.h
 create mode 100644 instrument/events.h
 create mode 100644 instrument/events.inc.h
 create mode 100644 instrument/qemu-instr/control.h

diff --git a/Makefile b/Makefile
index 3861b3f49c..c3d9a4bcd9 100644
--- a/Makefile
+++ b/Makefile
@@ -599,6 +599,10 @@ ifdef CONFIG_VIRTFS
 	$(INSTALL_DIR) "$(DESTDIR)$(mandir)/man1"
 	$(INSTALL_DATA) fsdev/virtfs-proxy-helper.1 "$(DESTDIR)$(mandir)/man1"
 endif
+ifdef CONFIG_INSTRUMENT
+	$(INSTALL_DIR) "$(DESTDIR)$(includedir)/qemu-instr/"
+	$(INSTALL_DATA) $(SRC_PATH)/instrument/qemu-instr/control.h "$(DESTDIR)$(includedir)/qemu-instr/"
+endif
 
 install-datadir:
 	$(INSTALL_DIR) "$(DESTDIR)$(qemu_datadir)"
diff --git a/configure b/configure
index 5175151317..18810eae84 100755
--- a/configure
+++ b/configure
@@ -6030,6 +6030,7 @@ if test "$instrument" = "yes"; then
   LIBS="-ldl $LIBS"
   echo "CONFIG_INSTRUMENT=y" >> $config_host_mak
 fi
+QEMU_INCLUDES="-I\$(SRC_PATH)/instrument $QEMU_INCLUDES"
 
 if test "$rdma" = "yes" ; then
   echo "CONFIG_RDMA=y" >> $config_host_mak
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 340e5fdc09..e86bd34e2c 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -111,4 +111,23 @@
 #define GCC_FMT_ATTR(n, m)
 #endif
 
+/*
+ * Export symbol to dlopen()'ed libraries'.
+ *
+ * This code is taken from http://gcc.gnu.org/wiki/Visibility.
+ */
+#if defined _WIN32 || defined __CYGWIN__
+  #ifdef __GNUC__
+    #define SYM_PUBLIC __attribute__ ((dllimport))
+  #else
+    #define SYM_PUBLIC __declspec(dllimport)
+  #endif
+#else
+  #if __GNUC__ >= 4
+    #define SYM_PUBLIC __attribute__ ((visibility("default")))
+  #else
+    #define SYM_PUBLIC
+  #endif
+#endif
+
 #endif /* COMPILER_H */
diff --git a/instrument/Makefile.objs b/instrument/Makefile.objs
index 7bf4e27e3c..ec76b2080b 100644
--- a/instrument/Makefile.objs
+++ b/instrument/Makefile.objs
@@ -3,3 +3,4 @@
 target-obj-$(CONFIG_INSTRUMENT) += cmdline.o
 target-obj-$(CONFIG_INSTRUMENT) += load.o
 target-obj-$(CONFIG_INSTRUMENT) += qmp.o
+target-obj-$(CONFIG_INSTRUMENT) += control.o
diff --git a/instrument/control.c b/instrument/control.c
new file mode 100644
index 0000000000..3630d6b3be
--- /dev/null
+++ b/instrument/control.c
@@ -0,0 +1,28 @@
+/*
+ * Control instrumentation during program (de)initialization.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "instrument/control.h"
+#include "instrument/error.h"
+#include "instrument/events.h"
+#include "instrument/load.h"
+#include "instrument/qemu-instr/control.h"
+#include "qemu/compiler.h"
+
+__thread InstrState instr_cur_state;
+
+
+qi_fini_fn instr_event__fini_fn;
+void *instr_event__fini_data;
+
+SYM_PUBLIC void qi_set_fini(qi_fini_fn fn, void *data)
+{
+    ERROR_IF(!instr_get_state(), "called outside instrumentation");
+    instr_set_event(fini_fn, fn);
+    instr_set_event(fini_data, data);
+}
diff --git a/instrument/control.h b/instrument/control.h
new file mode 100644
index 0000000000..f2b085f69b
--- /dev/null
+++ b/instrument/control.h
@@ -0,0 +1,44 @@
+/*
+ * Control instrumentation during program (de)initialization.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef INSTRUMENT__CONTROL_H
+#define INSTRUMENT__CONTROL_H
+
+
+/**
+ * InstrState:
+ * @INSTR_STATE_DISABLE: Intrumentation API not available.
+ * @INSTR_STATE_ENABLE: Intrumentation API available.
+ *
+ * Instrumentation state of current host thread. Used to ensure instrumentation
+ * clients use QEMU's API only in expected points.
+ */
+typedef enum {
+    INSTR_STATE_DISABLE,
+    INSTR_STATE_ENABLE,
+} InstrState;
+
+/**
+ * instr_set_state:
+ *
+ * Set the instrumentation state of the current host thread.
+ */
+static inline void instr_set_state(InstrState state);
+
+/**
+ * instr_get_state:
+ *
+ * Get the instrumentation state of the current host thread.
+ */
+static inline InstrState instr_get_state(void);
+
+
+#include "instrument/control.inc.h"
+
+#endif  /* INSTRUMENT__CONTROL_H */
diff --git a/instrument/control.inc.h b/instrument/control.inc.h
new file mode 100644
index 0000000000..0f649f4caa
--- /dev/null
+++ b/instrument/control.inc.h
@@ -0,0 +1,25 @@
+/*
+ * Control instrumentation during program (de)initialization.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/atomic.h"
+#include "qemu/compiler.h"
+#include <stdbool.h>
+
+
+extern __thread InstrState instr_cur_state;
+
+static inline void instr_set_state(InstrState state)
+{
+    atomic_store_release(&instr_cur_state, state);
+}
+
+static inline InstrState instr_get_state(void)
+{
+    return atomic_load_acquire(&instr_cur_state);
+}
diff --git a/instrument/error.h b/instrument/error.h
new file mode 100644
index 0000000000..f8d1dd4b16
--- /dev/null
+++ b/instrument/error.h
@@ -0,0 +1,28 @@
+/*
+ * Helpers for controlling errors in instrumentation libraries.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef INSTRUMENT_ERROR_H
+#define INSTRUMENT_ERROR_H
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+
+
+#define _ERROR(msg, args...)                            \
+    do {                                                \
+        error_report("%s:" msg, __func__, ##args);      \
+    } while (0)
+
+#define ERROR_IF(cond, msg, args...) \
+    if (unlikely(cond)) {            \
+        _ERROR(msg, ##args);         \
+        return;                      \
+    }
+
+#endif  /* INSTRUMENT_ERROR_H */
diff --git a/instrument/events.h b/instrument/events.h
new file mode 100644
index 0000000000..82ad0bd827
--- /dev/null
+++ b/instrument/events.h
@@ -0,0 +1,37 @@
+/*
+ * Internal API for triggering instrumentation events.
+ *
+ * Copyright (C) 2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef INSTRUMENT__EVENTS_H
+#define INSTRUMENT__EVENTS_H
+
+#include "instrument/qemu-instr/control.h"
+
+/**
+ * instr_get_event:
+ *
+ * Get value set by instrumentation library.
+ */
+#define instr_get_event(name)                   \
+    atomic_load_acquire(&instr_event__ ## name)
+
+/**
+ * instr_get_event:
+ *
+ * Set value from instrumentation library.
+ */
+#define instr_set_event(name, fn)               \
+    atomic_store_release(&instr_event__ ## name, fn)
+
+
+extern qi_fini_fn instr_event__fini_fn;
+extern void *instr_event__fini_data;
+
+#include "instrument/events.inc.h"
+
+#endif  /* INSTRUMENT__EVENTS_H */
diff --git a/instrument/events.inc.h b/instrument/events.inc.h
new file mode 100644
index 0000000000..8b1ce7fcb2
--- /dev/null
+++ b/instrument/events.inc.h
@@ -0,0 +1,11 @@
+/*
+ * Internal API for triggering instrumentation events.
+ *
+ * Copyright (C) 2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+
+
diff --git a/instrument/load.c b/instrument/load.c
index af98f4ce38..a01d66a4d4 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -11,6 +11,8 @@
 #include "qemu-common.h"
 
 #include <dlfcn.h>
+#include "instrument/control.h"
+#include "instrument/events.h"
 #include "instrument/load.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
@@ -96,8 +98,11 @@ InstrLoadError instr_load(const char *path, int argc, const char **argv,
         res = INSTR_LOAD_DLERROR;
         goto err;
     }
+    instr_set_event(fini_fn, NULL);
 
+    instr_set_state(INSTR_STATE_ENABLE);
     main_res = main_cb(argc, argv);
+    instr_set_state(INSTR_STATE_DISABLE);
 
     if (main_res != 0) {
         res = INSTR_LOAD_ERROR;
@@ -126,6 +131,14 @@ InstrUnloadError instr_unload(const char *id)
         goto out;
     }
 
+    qi_fini_fn fini_fn = instr_get_event(fini_fn);
+    if (fini_fn) {
+        void *fini_data = instr_get_event(fini_data);
+        fini_fn(fini_data);
+    }
+
+    instr_set_event(fini_fn, NULL);
+
     /* this should never fail */
     if (dlclose(handle->dlhandle) < 0) {
         res = INSTR_UNLOAD_DLERROR;
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
new file mode 100644
index 0000000000..b841afaa31
--- /dev/null
+++ b/instrument/qemu-instr/control.h
@@ -0,0 +1,46 @@
+/*
+ * Main instrumentation interface for QEMU.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QI__CONTROL_H
+#define QI__CONTROL_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdbool.h>
+#include <stddef.h>
+
+
+/**
+ * SECTION:control
+ * @section_id: qi-control
+ * @title: Event control API for QEMU event instrumentation
+ */
+
+typedef void (*qi_fini_fn)(void *arg);
+
+/**
+ * qi_set_fini:
+ * @fn: Finalization function.
+ * @data: Argument to pass to the finalization function.
+ *
+ * Set the function to call when finalizing (unloading) the instrumentation
+ * library.
+ *
+ * NOTE: Calls to printf() might not be shown if the library is unloaded when
+ *       QEMU terminates.
+ */
+void qi_set_fini(qi_fini_fn fn, void *data);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif  /* QI__CONTROL_H */
diff --git a/stubs/instrument.c b/stubs/instrument.c
index 292a2cdf26..560844469c 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -10,6 +10,7 @@
 #include "qemu/osdep.h"
 
 #include "instrument/cmdline.h"
+#include "instrument/control.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
 
@@ -38,3 +39,6 @@ void qmp_instr_unload(const char *id, Error **errp)
 {
     error_setg(errp, QERR_UNSUPPORTED);
 }
+
+
+__thread InstrState instr_cur_state;

[Qemu-devel] [PATCH v6 10/22] instrument: Add support for tracing events

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 .gitignore                        |    1 
 Makefile                          |    3 +
 instrument/Makefile.objs          |    1 
 instrument/error.h                |    6 ++
 instrument/qemu-instr/types.h     |   51 +++++++++++++++
 instrument/qemu-instr/types.inc.h |   15 ++++
 instrument/trace.c                |  125 +++++++++++++++++++++++++++++++++++++
 trace/control.h                   |    1 
 8 files changed, 203 insertions(+)
 create mode 100644 instrument/qemu-instr/types.h
 create mode 100644 instrument/qemu-instr/types.inc.h
 create mode 100644 instrument/trace.c

diff --git a/.gitignore b/.gitignore
index cf65316863..5ffcb9a091 100644
--- a/.gitignore
+++ b/.gitignore
@@ -134,3 +134,4 @@ trace-dtrace-root.h
 trace-dtrace-root.dtrace
 trace-ust-all.h
 trace-ust-all.c
+!/instrument/*
diff --git a/Makefile b/Makefile
index c3d9a4bcd9..646fe2f327 100644
--- a/Makefile
+++ b/Makefile
@@ -602,6 +602,9 @@ endif
 ifdef CONFIG_INSTRUMENT
 	$(INSTALL_DIR) "$(DESTDIR)$(includedir)/qemu-instr/"
 	$(INSTALL_DATA) $(SRC_PATH)/instrument/qemu-instr/control.h "$(DESTDIR)$(includedir)/qemu-instr/"
+	$(INSTALL_DATA) $(SRC_PATH)/instrument/qemu-instr/trace.h "$(DESTDIR)$(includedir)/qemu-instr/"
+	$(INSTALL_DATA) $(SRC_PATH)/instrument/qemu-instr/types.h "$(DESTDIR)$(includedir)/qemu-instr/"
+	$(INSTALL_DATA) $(SRC_PATH)/instrument/qemu-instr/types.inc.h "$(DESTDIR)$(includedir)/qemu-instr/"
 endif
 
 install-datadir:
diff --git a/instrument/Makefile.objs b/instrument/Makefile.objs
index ec76b2080b..d7e6c760c3 100644
--- a/instrument/Makefile.objs
+++ b/instrument/Makefile.objs
@@ -4,3 +4,4 @@ target-obj-$(CONFIG_INSTRUMENT) += cmdline.o
 target-obj-$(CONFIG_INSTRUMENT) += load.o
 target-obj-$(CONFIG_INSTRUMENT) += qmp.o
 target-obj-$(CONFIG_INSTRUMENT) += control.o
+target-obj-$(CONFIG_INSTRUMENT) += trace.o
diff --git a/instrument/error.h b/instrument/error.h
index f8d1dd4b16..7a51d62fdb 100644
--- a/instrument/error.h
+++ b/instrument/error.h
@@ -25,4 +25,10 @@
         return;                      \
     }
 
+#define ERROR_IF_RET(cond, ret, msg, args...)   \
+    if (unlikely(cond)) {                       \
+        _ERROR(msg, ##args);                    \
+        return ret;                             \
+    }                                           \
+
 #endif  /* INSTRUMENT_ERROR_H */
diff --git a/instrument/qemu-instr/types.h b/instrument/qemu-instr/types.h
new file mode 100644
index 0000000000..ea3a032b4f
--- /dev/null
+++ b/instrument/qemu-instr/types.h
@@ -0,0 +1,51 @@
+/*
+ * QEMU-specific types for instrumentation clients.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QI__TYPES_H
+#define QI__TYPES_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * SECTION: types
+ * @section_id: qi-types
+ * @title: Common types
+ */
+
+/**
+ * QITraceEvent:
+ *
+ * Opaque structure defining a tracing event.
+ */
+typedef struct QITraceEvent QITraceEvent;
+
+/**
+ * QITraceEventIter:
+ *
+ * Opaque structure defining a tracing event iterator.
+ */
+typedef struct QITraceEventIter QITraceEventIter;
+
+/**
+ * QICPU:
+ *
+ * Opaque guest CPU pointer.
+ */
+typedef struct QICPU_d *QICPU;
+
+
+#include <qemu-instr/types.inc.h>
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif  /* QI__TYPES_H */
diff --git a/instrument/qemu-instr/types.inc.h b/instrument/qemu-instr/types.inc.h
new file mode 100644
index 0000000000..0d99ea59a2
--- /dev/null
+++ b/instrument/qemu-instr/types.inc.h
@@ -0,0 +1,15 @@
+/*
+ * QEMU-specific types for instrumentation clients.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <stdlib.h>
+
+
+struct QITraceEventIter {
+    char buffer[(sizeof(size_t) * 2) + sizeof(char *)];
+};
diff --git a/instrument/trace.c b/instrument/trace.c
new file mode 100644
index 0000000000..6a437039b4
--- /dev/null
+++ b/instrument/trace.c
@@ -0,0 +1,125 @@
+/*
+ * API for QEMU's tracing events.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "instrument/error.h"
+#include "qemu/compiler.h"
+#include "qemu-instr/trace.h"
+#include "trace/control.h"
+
+
+SYM_PUBLIC
+QITraceEvent *qi_trace_event_name(const char *name)
+{
+    ERROR_IF_RET(!name, NULL, "must provide a name");
+    return (QITraceEvent *)trace_event_name(name);
+}
+
+SYM_PUBLIC
+void qi_trace_event_iter_init(QITraceEventIter *iter, const char *pattern)
+{
+    TraceEventIter *iter_ = (TraceEventIter *)iter;
+    ERROR_IF(!iter_, "must provide an iterator");
+    trace_event_iter_init(iter_, pattern);
+}
+
+SYM_PUBLIC
+QITraceEvent *qi_trace_event_iter_next(QITraceEventIter *iter)
+{
+    TraceEventIter *iter_ = (TraceEventIter *)iter;
+    ERROR_IF_RET(!iter_, NULL, "must provide an iterator");
+    return (QITraceEvent *)trace_event_iter_next(iter_);
+}
+
+
+SYM_PUBLIC
+bool qi_trace_event_is_vcpu(QITraceEvent *ev)
+{
+    TraceEvent *ev_ = (TraceEvent *)ev;
+    ERROR_IF_RET(!ev_, false, "must provide an event");
+    return trace_event_is_vcpu(ev_);
+}
+
+SYM_PUBLIC
+const char *qi_trace_event_get_name(QITraceEvent *ev)
+{
+    TraceEvent *ev_ = (TraceEvent *)ev;
+    ERROR_IF_RET(!ev_, false, "must provide an event");
+    return trace_event_get_name(ev_);
+}
+
+
+SYM_PUBLIC
+bool qi_trace_event_get_state(QITraceEvent *ev)
+{
+    TraceEvent *ev_ = (TraceEvent *)ev;
+    ERROR_IF_RET(!ev_, false, "must provide an event");
+    return trace_event_get_state_static(ev_) &&
+        trace_event_get_state_dynamic(ev_);
+}
+
+SYM_PUBLIC
+bool qi_trace_event_get_vcpu_state(QICPU *vcpu, QITraceEvent *ev)
+{
+    CPUState *vcpu_ = (CPUState *)vcpu;
+    TraceEvent *ev_ = (TraceEvent *)ev;
+    ERROR_IF_RET(!vcpu_, false, "must provide a vCPU");
+    ERROR_IF_RET(!ev_, false, "must provide an event");
+    return trace_event_get_state_static(ev_) &&
+        trace_event_get_vcpu_state_dynamic(vcpu_, ev_);
+}
+
+SYM_PUBLIC
+bool qi_trace_event_get_state_static(QITraceEvent *ev)
+{
+    TraceEvent *ev_ = (TraceEvent *)ev;
+    ERROR_IF_RET(!ev_, false, "must provide an event");
+    return trace_event_get_state_static(ev_);
+}
+
+SYM_PUBLIC
+bool qi_trace_event_get_state_dynamic(QITraceEvent *ev)
+{
+    TraceEvent *ev_ = (TraceEvent *)ev;
+    ERROR_IF_RET(!ev_, false, "must provide an event");
+    return trace_event_get_state_dynamic(ev_);
+}
+
+SYM_PUBLIC
+bool qi_trace_event_get_vcpu_state_dynamic(QICPU *vcpu, QITraceEvent *ev)
+{
+    CPUState *vcpu_ = (CPUState *)vcpu;
+    TraceEvent *ev_ = (TraceEvent *)ev;
+    ERROR_IF_RET(!vcpu_, false, "must provide a vCPU");
+    ERROR_IF_RET(!ev_, false, "must provide an event");
+    return trace_event_get_vcpu_state_dynamic(vcpu_, ev_);
+}
+
+SYM_PUBLIC
+void qi_trace_event_set_state_dynamic(QITraceEvent *ev, bool state)
+{
+    TraceEvent *ev_ = (TraceEvent *)ev;
+    ERROR_IF(!ev_, "must provide an event");
+    ERROR_IF(!trace_event_get_state_static(ev_),
+             "event must be statically enabled");
+    trace_event_set_state_dynamic(ev_, state);
+}
+
+SYM_PUBLIC
+void qi_trace_event_set_vcpu_state_dynamic(QICPU *vcpu,
+                                           QITraceEvent *ev, bool state)
+{
+    CPUState *vcpu_ = (CPUState *)vcpu;
+    TraceEvent *ev_ = (TraceEvent *)ev;
+    ERROR_IF(!vcpu_, "must provide a vCPU");
+    ERROR_IF(!ev_, "must provide an event");
+    ERROR_IF(!trace_event_get_state_static(ev_),
+             "event must be statically enabled");
+    trace_event_set_vcpu_state_dynamic(vcpu_, ev_, state);
+}
diff --git a/trace/control.h b/trace/control.h
index 1903e22975..3e6da24c98 100644
--- a/trace/control.h
+++ b/trace/control.h
@@ -13,6 +13,7 @@
 #include "qemu-common.h"
 #include "event-internal.h"
 
+/* NOTE: Keep in sync with size of QITraceEventIter */
 typedef struct TraceEventIter {
     size_t event;
     size_t group;

[Qemu-devel] [PATCH v6 11/22] instrument: Track vCPUs

[Lluís Vilanova]

Keep a translation between instrumentation's QICPU and CPUState objects to avoid
exposing QEMU's internals to instrumentation clients.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 cpus-common.c            |    9 +++++++++
 instrument/control.c     |   23 +++++++++++++++++++++++
 instrument/control.h     |   36 ++++++++++++++++++++++++++++++++++++
 instrument/control.inc.h |   23 +++++++++++++++++++++++
 4 files changed, 91 insertions(+)

diff --git a/cpus-common.c b/cpus-common.c
index 59f751ecf9..ec5f46cc3d 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -22,6 +22,9 @@
 #include "exec/cpu-common.h"
 #include "qom/cpu.h"
 #include "sysemu/cpus.h"
+#if defined(CONFIG_INSTRUMENT)
+#include "instrument/control.h"
+#endif
 
 static QemuMutex qemu_cpu_list_lock;
 static QemuCond exclusive_cond;
@@ -84,6 +87,9 @@ void cpu_list_add(CPUState *cpu)
     } else {
         assert(!cpu_index_auto_assigned);
     }
+#if defined(CONFIG_INSTRUMENT)
+    instr_cpu_add(cpu);
+#endif
     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
     qemu_mutex_unlock(&qemu_cpu_list_lock);
 
@@ -102,6 +108,9 @@ void cpu_list_remove(CPUState *cpu)
     assert(!(cpu_index_auto_assigned && cpu != QTAILQ_LAST(&cpus, CPUTailQ)));
 
     QTAILQ_REMOVE(&cpus, cpu, node);
+#if defined(CONFIG_INSTRUMENT)
+    instr_cpu_remove(cpu);
+#endif
     cpu->cpu_index = UNASSIGNED_CPU_INDEX;
     qemu_mutex_unlock(&qemu_cpu_list_lock);
 }
diff --git a/instrument/control.c b/instrument/control.c
index 3630d6b3be..8cf2b4f967 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -13,10 +13,33 @@
 #include "instrument/load.h"
 #include "instrument/qemu-instr/control.h"
 #include "qemu/compiler.h"
+#include "qom/cpu.h"
+
 
 __thread InstrState instr_cur_state;
 
 
+unsigned int instr_cpus_count;
+CPUState **instr_cpus;
+
+void instr_cpu_add(CPUState *vcpu)
+{
+    unsigned int idx = vcpu->cpu_index;
+    if (idx >= instr_cpus_count) {
+        instr_cpus_count = idx + 1;
+        instr_cpus = realloc(instr_cpus,
+                             sizeof(*instr_cpus) * instr_cpus_count);
+    }
+    instr_cpus[idx] = vcpu;
+}
+
+void instr_cpu_remove(CPUState *vcpu)
+{
+    unsigned int idx = vcpu->cpu_index;
+    instr_cpus[idx] = NULL;
+}
+
+
 qi_fini_fn instr_event__fini_fn;
 void *instr_event__fini_data;
 
diff --git a/instrument/control.h b/instrument/control.h
index f2b085f69b..57cea07fa7 100644
--- a/instrument/control.h
+++ b/instrument/control.h
@@ -10,6 +10,42 @@
 #ifndef INSTRUMENT__CONTROL_H
 #define INSTRUMENT__CONTROL_H
 
+#include "qemu/typedefs.h"
+#include "instrument/qemu-instr/types.h"
+
+
+/**
+ * instr_cpu_add:
+ *
+ * Make @vcpu available to instrumentation clients.
+ *
+ * Precondition: cpu_list_lock().
+ */
+void instr_cpu_add(CPUState *vcpu);
+
+/**
+ * instr_cpu_remove:
+ *
+ * Make @vcpu unavailable to instrumentation clients.
+ *
+ * Precondition: cpu_list_lock().
+ */
+void instr_cpu_remove(CPUState *vcpu);
+
+/**
+ * instr_cpu_to_qicpu:
+ *
+ * Get the #QICPU corresponding to the given #CPUState.
+ */
+static inline QICPU instr_cpu_to_qicpu(CPUState *vcpu);
+
+/**
+ * instr_cpu_from_qicpu:
+ *
+ * Get the #CPUState corresponding to the given #QICPU.
+ */
+static inline CPUState *instr_cpu_from_qicpu(QICPU vcpu);
+
 
 /**
  * InstrState:
diff --git a/instrument/control.inc.h b/instrument/control.inc.h
index 0f649f4caa..45daae7d1d 100644
--- a/instrument/control.inc.h
+++ b/instrument/control.inc.h
@@ -7,9 +7,32 @@
  * See the COPYING file in the top-level directory.
  */
 
+#include "qemu/osdep.h"
 #include "qemu/atomic.h"
 #include "qemu/compiler.h"
+#include "qom/cpu.h"
 #include <stdbool.h>
+#include <stdint.h>
+
+
+extern unsigned int instr_cpus_count;
+extern CPUState **instr_cpus;
+
+static inline QICPU instr_cpu_to_qicpu(CPUState *vcpu)
+{
+    uintptr_t idx = vcpu->cpu_index;
+    return (QICPU)idx;
+}
+
+static inline CPUState *instr_cpu_from_qicpu(QICPU vcpu)
+{
+    unsigned int idx = (uintptr_t)vcpu;
+    if (idx >= instr_cpus_count) {
+        return NULL;
+    } else {
+        return instr_cpus[idx];
+    }
+}
 
 
 extern __thread InstrState instr_cur_state;

[Qemu-devel] [PATCH v6 12/22] instrument: Add event 'guest_cpu_enter'

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 instrument/control.c            |    9 ++++++++
 instrument/events.h             |    5 ++++
 instrument/events.inc.h         |   11 +++++++++
 instrument/load.c               |    9 ++++++++
 instrument/qemu-instr/control.h |   46 +++++++++++++++++++++++++++++++++++++++
 stubs/instrument.c              |    1 +
 trace/control-target.c          |    2 ++
 7 files changed, 83 insertions(+)

diff --git a/instrument/control.c b/instrument/control.c
index 8cf2b4f967..c4b3ca0440 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -49,3 +49,12 @@ SYM_PUBLIC void qi_set_fini(qi_fini_fn fn, void *data)
     instr_set_event(fini_fn, fn);
     instr_set_event(fini_data, data);
 }
+
+
+void (*instr_event__guest_cpu_enter)(QICPU vcpu);
+
+SYM_PUBLIC void qi_event_set_guest_cpu_enter(void (*fn)(QICPU vcpu))
+{
+    ERROR_IF(!instr_get_state(), "called outside instrumentation");
+    instr_set_event(guest_cpu_enter, fn);
+}
diff --git a/instrument/events.h b/instrument/events.h
index 82ad0bd827..947f120aa9 100644
--- a/instrument/events.h
+++ b/instrument/events.h
@@ -11,6 +11,7 @@
 #define INSTRUMENT__EVENTS_H
 
 #include "instrument/qemu-instr/control.h"
+#include "instrument/qemu-instr/types.h"
 
 /**
  * instr_get_event:
@@ -32,6 +33,10 @@
 extern qi_fini_fn instr_event__fini_fn;
 extern void *instr_event__fini_data;
 
+extern void (*instr_event__guest_cpu_enter)(QICPU vcpu);
+static inline void instr_guest_cpu_enter(CPUState *vcpu);
+
+
 #include "instrument/events.inc.h"
 
 #endif  /* INSTRUMENT__EVENTS_H */
diff --git a/instrument/events.inc.h b/instrument/events.inc.h
index 8b1ce7fcb2..e3f8024716 100644
--- a/instrument/events.inc.h
+++ b/instrument/events.inc.h
@@ -7,5 +7,16 @@
  * See the COPYING file in the top-level directory.
  */
 
+#include "instrument/control.h"
 
 
+static inline void instr_guest_cpu_enter(CPUState *vcpu)
+{
+    void (*cb)(QICPU vcpu) = instr_get_event(guest_cpu_enter);
+    if (cb) {
+        QICPU vcpu_ = instr_cpu_to_qicpu(vcpu);
+        instr_set_state(INSTR_STATE_ENABLE);
+        (*cb)(vcpu_);
+        instr_set_state(INSTR_STATE_DISABLE);
+    }
+}
diff --git a/instrument/load.c b/instrument/load.c
index a01d66a4d4..218bca74b2 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -11,6 +11,7 @@
 #include "qemu-common.h"
 
 #include <dlfcn.h>
+#include "exec/cpu-common.h"
 #include "instrument/control.h"
 #include "instrument/events.h"
 #include "instrument/load.h"
@@ -109,6 +110,13 @@ InstrLoadError instr_load(const char *path, int argc, const char **argv,
         goto err;
     }
 
+    cpu_list_lock();
+    CPUState *cpu;
+    CPU_FOREACH(cpu) {
+        instr_guest_cpu_enter(cpu);
+    }
+    cpu_list_unlock();
+
     res = INSTR_LOAD_OK;
     goto out;
 
@@ -138,6 +146,7 @@ InstrUnloadError instr_unload(const char *id)
     }
 
     instr_set_event(fini_fn, NULL);
+    instr_set_event(guest_cpu_enter, NULL);
 
     /* this should never fail */
     if (dlclose(handle->dlhandle) < 0) {
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
index b841afaa31..f61e7a2b6e 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -16,6 +16,7 @@ extern "C" {
 
 #include <stdbool.h>
 #include <stddef.h>
+#include <qemu-instr/types.h>
 
 
 /**
@@ -39,6 +40,51 @@ typedef void (*qi_fini_fn)(void *arg);
  */
 void qi_set_fini(qi_fini_fn fn, void *data);
 
+
+/*
+ * Set callbacks for available events. Each event has a short description and
+ * various indicators of when it can be triggered:
+ *
+ * - Mode :: user
+ *   Triggered in QEMU user application emulation (e.g., linux-user).
+ *
+ * - Mode :: softmmy
+ *   Triggered in QEMU full-system emulation.
+ *
+ *
+ * - Targets :: all
+ *   Triggered on all targets, both using TCG or native hardware virtualization
+ *   (e.g., KVM).
+ *
+ * - Targets :: TCG(<arch>)
+ *   Triggered on the given guest target architectures when executing with TCG
+ *   (no native hardware virtualization).
+ *
+ *
+ * - Time :: exec
+ *   Triggered when the guest executes the described operation.
+ *
+ * - Time :: trans
+ *   Triggered when QEMU translates a guest operation. This is only available
+ *   when executing with TCG. Guest instructions are decompiled and translated
+ *   into the intermediate TCG language (when "Time: trans" events are
+ *   triggered). Then, the TCG compiler translates TCG code into the native host
+ *   code that QEMU will execute to emulate the guest (when "Time: exec" events
+ *   are triggered). As QEMU uses a cache of translated code, the same
+ *   instruction might be translated more than once (when the cache overflows).
+ */
+
+/*
+ * Hot-plug a new virtual (guest) CPU.
+ *
+ * Also triggered on each CPU when an instrumentation library is loaded.
+ *
+ * Mode: user, softmmu
+ * Targets: all
+ * Time: exec
+ */
+void qi_event_set_guest_cpu_enter(void (*fn)(QICPU vcpu));
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/stubs/instrument.c b/stubs/instrument.c
index 560844469c..abed583a56 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -42,3 +42,4 @@ void qmp_instr_unload(const char *id, Error **errp)
 
 
 __thread InstrState instr_cur_state;
+void (*instr_event__guest_cpu_enter)(QICPU *vcpu);
diff --git a/trace/control-target.c b/trace/control-target.c
index 706b2cee9d..f22688bcd5 100644
--- a/trace/control-target.c
+++ b/trace/control-target.c
@@ -9,6 +9,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
+#include "instrument/events.h"
 #include "trace-root.h"
 #include "trace/control.h"
 #include "translate-all.h"
@@ -146,5 +147,6 @@ void trace_init_vcpu(CPUState *vcpu)
             }
         }
     }
+    instr_guest_cpu_enter(vcpu);
     trace_guest_cpu_enter(vcpu);
 }

[Qemu-devel] [PATCH v6 13/22] instrument: Support synchronous modification of vCPU state

[Lluís Vilanova]

Stops all vCPUs to allow performing management operations like TB
invalidations. These are later necessary to ensure translated code does not
reference unloaded instrumentation libraries.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 instrument/control.c     |   67 ++++++++++++++++++++++++++++++++++++++++++++++
 instrument/control.h     |   26 ++++++++++++++++++
 instrument/control.inc.h |   11 ++++++++
 3 files changed, 104 insertions(+)

diff --git a/instrument/control.c b/instrument/control.c
index c4b3ca0440..4f9c138ccf 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -13,6 +13,7 @@
 #include "instrument/load.h"
 #include "instrument/qemu-instr/control.h"
 #include "qemu/compiler.h"
+#include "qemu/main-loop.h"
 #include "qom/cpu.h"
 
 
@@ -40,6 +41,72 @@ void instr_cpu_remove(CPUState *vcpu)
 }
 
 
+static void instr_cpu_stop_all__cb(CPUState *cpu, run_on_cpu_data data)
+{
+    InstrCPUStop *info = data.host_ptr;
+    /* run posted function */
+    if (info->fun) {
+        info->fun(cpu, info->data);
+    }
+#if !defined(CONFIG_USER_ONLY)
+    /* signal we're out of the main vCPU loop */
+    unsigned int count = atomic_load_acquire(&info->count);
+    atomic_store_release(&info->count, count + 1);
+    atomic_store_release(&info->stopped, true);
+    /* wait until we're good to go again */
+    qemu_cond_wait(&info->cond, &info->mutex);
+    count = atomic_load_acquire(&info->count);
+    atomic_store_release(&info->count, count - 1);
+    qemu_mutex_unlock(&info->mutex);
+#endif
+}
+
+void instr_cpu_stop_all_begin(InstrCPUStop *info,
+                              instr_cpu_stop_fun fun, void *data)
+{
+    CPUState *cpu;
+
+    info->fun = fun;
+    info->data = data;
+
+#if !defined(CONFIG_USER_ONLY)
+    info->count = 0;
+    qemu_cond_init(&info->cond);
+    qemu_mutex_init(&info->mutex);
+
+    /* main dispatch loop and run_on_cpu() lock the BQL */
+    qemu_mutex_unlock_iothread();
+#endif
+
+    CPU_FOREACH(cpu) {
+#if !defined(CONFIG_USER_ONLY)
+        atomic_store_release(&info->stopped, false);
+        qemu_mutex_lock(&info->mutex);
+        async_run_on_cpu(cpu, instr_cpu_stop_all__cb,
+                         RUN_ON_CPU_HOST_PTR(info));
+        while (!atomic_load_acquire(&info->stopped)) {
+            /* wait for vCPU to signal it's stopped */
+        }
+#else
+        instr_cpu_stop_all__cb(cpu, RUN_ON_CPU_HOST_PTR(info));
+#endif
+    }
+}
+
+void instr_cpu_stop_all_end(InstrCPUStop *info)
+{
+#if !defined(CONFIG_USER_ONLY)
+    qemu_cond_broadcast(&info->cond);
+    while (atomic_load_acquire(&info->count)) {
+        /* wait for all vCPUs to continue before we can destroy info */
+    }
+    qemu_cond_destroy(&info->cond);
+    qemu_mutex_destroy(&info->mutex);
+    qemu_mutex_lock_iothread();
+#endif
+}
+
+
 qi_fini_fn instr_event__fini_fn;
 void *instr_event__fini_data;
 
diff --git a/instrument/control.h b/instrument/control.h
index 57cea07fa7..03e87b2b8f 100644
--- a/instrument/control.h
+++ b/instrument/control.h
@@ -46,6 +46,32 @@ static inline QICPU instr_cpu_to_qicpu(CPUState *vcpu);
  */
 static inline CPUState *instr_cpu_from_qicpu(QICPU vcpu);
 
+typedef struct InstrCPUStop InstrCPUStop;
+typedef void (*instr_cpu_stop_fun)(CPUState *cpu, void *data);
+
+/**
+ * instr_cpu_stop_all_begin:
+ * @info: Opaque structure describing the operation.
+ * @fun: Function to run on the context of each vCPU once stopped.
+ * @data: Pointer to pass to @fun.
+ *
+ * Ensure all vCPUs stop executing guest code, and execute @fun on their context
+ * in turn. Returns with all vCPUs still stopped.
+ *
+ * Assumes cpu_list_lock() and that the QBL is locked before calling.
+ */
+void instr_cpu_stop_all_begin(InstrCPUStop *info,
+                              instr_cpu_stop_fun fun, void *data);
+
+/**
+ * instr_cpu_stop_all_end:
+ * @info: Opaque structure passed to a previous instr_cpu_stop_all_begin()
+ *     call.
+ *
+ * Resume execution on all vCPUs stopped by instr_cpu_stop_all_begin().
+ */
+void instr_cpu_stop_all_end(InstrCPUStop *info);
+
 
 /**
  * InstrState:
diff --git a/instrument/control.inc.h b/instrument/control.inc.h
index 45daae7d1d..6d65b23ead 100644
--- a/instrument/control.inc.h
+++ b/instrument/control.inc.h
@@ -15,6 +15,17 @@
 #include <stdint.h>
 
 
+struct InstrCPUStop {
+    instr_cpu_stop_fun fun;
+    void *data;
+#if !defined(CONFIG_USER_ONLY)
+    bool stopped;
+    unsigned int count;
+    QemuCond cond;
+    QemuMutex mutex;
+#endif
+};
+
 extern unsigned int instr_cpus_count;
 extern CPUState **instr_cpus;
 

[Qemu-devel] [PATCH v6 14/22] exec: Add function to synchronously flush TB on a stopped vCPU

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 accel/stubs/tcg-stub.c    |    3 +++
 accel/tcg/translate-all.c |    7 +++++++
 include/exec/exec-all.h   |    1 +
 3 files changed, 11 insertions(+)

diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
index 5dd480b1a2..5226c4a8a4 100644
--- a/accel/stubs/tcg-stub.c
+++ b/accel/stubs/tcg-stub.c
@@ -20,3 +20,6 @@
 void tb_flush(CPUState *cpu)
 {
 }
+void tb_flush_sync(CPUState *cpu)
+{
+}
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 2d1ed06065..a334ac4ccb 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -929,6 +929,13 @@ done:
     tb_unlock();
 }
 
+void tb_flush_sync(CPUState *cpu)
+{
+    unsigned tb_flush_count = atomic_mb_read(&tcg_ctx.tb_ctx.tb_flush_count);
+    assert(cpu == current_cpu);
+    do_tb_flush(cpu, RUN_ON_CPU_HOST_INT(tb_flush_count));
+}
+
 void tb_flush(CPUState *cpu)
 {
     if (tcg_enabled()) {
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 673fc066d0..3f38186a5e 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -358,6 +358,7 @@ struct TranslationBlock {
 
 void tb_free(TranslationBlock *tb);
 void tb_flush(CPUState *cpu);
+void tb_flush_sync(CPUState *cpu);
 void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
 TranslationBlock *tb_htable_lookup(CPUState *cpu, target_ulong pc,
                                    target_ulong cs_base, uint32_t flags);

[Qemu-devel] [PATCH v6 15/22] instrument: Add event 'guest_cpu_exit'

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 instrument/control.c            |    9 +++++++++
 instrument/events.h             |    3 +++
 instrument/events.inc.h         |   11 +++++++++++
 instrument/load.c               |   17 +++++++++++++++++
 instrument/qemu-instr/control.h |   11 +++++++++++
 stubs/instrument.c              |    1 +
 trace/control.c                 |    4 +++-
 7 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/instrument/control.c b/instrument/control.c
index 4f9c138ccf..7ed4bf3505 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -125,3 +125,12 @@ SYM_PUBLIC void qi_event_set_guest_cpu_enter(void (*fn)(QICPU vcpu))
     ERROR_IF(!instr_get_state(), "called outside instrumentation");
     instr_set_event(guest_cpu_enter, fn);
 }
+
+
+void (*instr_event__guest_cpu_exit)(QICPU vcpu);
+
+SYM_PUBLIC void qi_event_set_guest_cpu_exit(void (*fn)(QICPU vcpu))
+{
+    ERROR_IF(!instr_get_state(), "called outside instrumentation");
+    instr_set_event(guest_cpu_exit, fn);
+}
diff --git a/instrument/events.h b/instrument/events.h
index 947f120aa9..c743cb8180 100644
--- a/instrument/events.h
+++ b/instrument/events.h
@@ -36,6 +36,9 @@ extern void *instr_event__fini_data;
 extern void (*instr_event__guest_cpu_enter)(QICPU vcpu);
 static inline void instr_guest_cpu_enter(CPUState *vcpu);
 
+extern void (*instr_event__guest_cpu_exit)(QICPU vcpu);
+static inline void instr_guest_cpu_exit(CPUState *vcpu);
+
 
 #include "instrument/events.inc.h"
 
diff --git a/instrument/events.inc.h b/instrument/events.inc.h
index e3f8024716..c88df7e42f 100644
--- a/instrument/events.inc.h
+++ b/instrument/events.inc.h
@@ -20,3 +20,14 @@ static inline void instr_guest_cpu_enter(CPUState *vcpu)
         instr_set_state(INSTR_STATE_DISABLE);
     }
 }
+
+static inline void instr_guest_cpu_exit(CPUState *vcpu)
+{
+    void (*cb)(QICPU vcpu) = instr_get_event(guest_cpu_exit);
+    if (cb) {
+        QICPU vcpu_ = instr_cpu_to_qicpu(vcpu);
+        instr_set_state(INSTR_STATE_ENABLE);
+        (*cb)(vcpu_);
+        instr_set_state(INSTR_STATE_DISABLE);
+    }
+}
diff --git a/instrument/load.c b/instrument/load.c
index 218bca74b2..6808d361b5 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -11,7 +11,9 @@
 #include "qemu-common.h"
 
 #include <dlfcn.h>
+#include "cpu.h"
 #include "exec/cpu-common.h"
+#include "exec/exec-all.h"
 #include "instrument/control.h"
 #include "instrument/events.h"
 #include "instrument/load.h"
@@ -127,6 +129,13 @@ out:
     return res;
 }
 
+
+static void instr_unload__cb(CPUState *cpu, void *data)
+{
+    tb_flush_sync(cpu);
+    instr_guest_cpu_exit(cpu);
+}
+
 InstrUnloadError instr_unload(const char *id)
 {
     InstrUnloadError res;
@@ -139,6 +148,10 @@ InstrUnloadError instr_unload(const char *id)
         goto out;
     }
 
+    InstrCPUStop info;
+    cpu_list_lock();
+    instr_cpu_stop_all_begin(&info, instr_unload__cb, NULL);
+
     qi_fini_fn fini_fn = instr_get_event(fini_fn);
     if (fini_fn) {
         void *fini_data = instr_get_event(fini_data);
@@ -147,6 +160,10 @@ InstrUnloadError instr_unload(const char *id)
 
     instr_set_event(fini_fn, NULL);
     instr_set_event(guest_cpu_enter, NULL);
+    instr_set_event(guest_cpu_exit, NULL);
+
+    instr_cpu_stop_all_end(&info);
+    cpu_list_unlock();
 
     /* this should never fail */
     if (dlclose(handle->dlhandle) < 0) {
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
index f61e7a2b6e..107ee8afe0 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -85,6 +85,17 @@ void qi_set_fini(qi_fini_fn fn, void *data);
  */
 void qi_event_set_guest_cpu_enter(void (*fn)(QICPU vcpu));
 
+/*
+ * Hot-unplug a virtual (guest) CPU.
+ *
+ * Also triggered on each CPU when an instrumentation library is unloaded.
+ *
+ * Mode: user, softmmu
+ * Targets: all
+ * Time: exec
+ */
+void qi_event_set_guest_cpu_exit(void (*fn)(QICPU vcpu));
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/stubs/instrument.c b/stubs/instrument.c
index abed583a56..e7adea1aad 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -43,3 +43,4 @@ void qmp_instr_unload(const char *id, Error **errp)
 
 __thread InstrState instr_cur_state;
 void (*instr_event__guest_cpu_enter)(QICPU *vcpu);
+void (*instr_event__guest_cpu_exit)(QICPU *vcpu);
diff --git a/trace/control.c b/trace/control.c
index 82d8989c4d..946a0af818 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -1,13 +1,14 @@
 /*
  * Interface for configuring and controlling the state of tracing events.
  *
- * Copyright (C) 2011-2016 Lluís Vilanova <vilanova@ac.upc.edu>
+ * Copyright (C) 2011-2017 Lluís Vilanova <vilanova@ac.upc.edu>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
  */
 
 #include "qemu/osdep.h"
+#include "instrument/events.h"
 #include "trace/control.h"
 #include "qemu/help_option.h"
 #ifdef CONFIG_TRACE_SIMPLE
@@ -272,6 +273,7 @@ void trace_fini_vcpu(CPUState *vcpu)
     TraceEventIter iter;
     TraceEvent *ev;
 
+    instr_guest_cpu_exit(vcpu);
     trace_guest_cpu_exit(vcpu);
 
     trace_event_iter_init(&iter, NULL);

[Qemu-devel] [PATCH v6 16/22] instrument: Add event 'guest_cpu_reset'

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 instrument/control.c            |    9 +++++++++
 instrument/events.h             |    3 +++
 instrument/events.inc.h         |   11 +++++++++++
 instrument/load.c               |    1 +
 instrument/qemu-instr/control.h |    9 +++++++++
 qom/cpu.c                       |    2 ++
 stubs/instrument.c              |    1 +
 7 files changed, 36 insertions(+)

diff --git a/instrument/control.c b/instrument/control.c
index 7ed4bf3505..cb397639ce 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -134,3 +134,12 @@ SYM_PUBLIC void qi_event_set_guest_cpu_exit(void (*fn)(QICPU vcpu))
     ERROR_IF(!instr_get_state(), "called outside instrumentation");
     instr_set_event(guest_cpu_exit, fn);
 }
+
+
+void (*instr_event__guest_cpu_reset)(QICPU vcpu);
+
+SYM_PUBLIC void qi_event_set_guest_cpu_reset(void (*fn)(QICPU vcpu))
+{
+    ERROR_IF(!instr_get_state(), "called outside instrumentation");
+    instr_set_event(guest_cpu_reset, fn);
+}
diff --git a/instrument/events.h b/instrument/events.h
index c743cb8180..4a0560490a 100644
--- a/instrument/events.h
+++ b/instrument/events.h
@@ -39,6 +39,9 @@ static inline void instr_guest_cpu_enter(CPUState *vcpu);
 extern void (*instr_event__guest_cpu_exit)(QICPU vcpu);
 static inline void instr_guest_cpu_exit(CPUState *vcpu);
 
+extern void (*instr_event__guest_cpu_reset)(QICPU vcpu);
+static inline void instr_guest_cpu_reset(CPUState *vcpu);
+
 
 #include "instrument/events.inc.h"
 
diff --git a/instrument/events.inc.h b/instrument/events.inc.h
index c88df7e42f..a126ba5ae6 100644
--- a/instrument/events.inc.h
+++ b/instrument/events.inc.h
@@ -31,3 +31,14 @@ static inline void instr_guest_cpu_exit(CPUState *vcpu)
         instr_set_state(INSTR_STATE_DISABLE);
     }
 }
+
+static inline void instr_guest_cpu_reset(CPUState *vcpu)
+{
+    void (*cb)(QICPU vcpu) = instr_get_event(guest_cpu_reset);
+    if (cb) {
+        QICPU vcpu_ = instr_cpu_to_qicpu(vcpu);
+        instr_set_state(INSTR_STATE_ENABLE);
+        (*cb)(vcpu_);
+        instr_set_state(INSTR_STATE_DISABLE);
+    }
+}
diff --git a/instrument/load.c b/instrument/load.c
index 6808d361b5..8c15a73a8c 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -161,6 +161,7 @@ InstrUnloadError instr_unload(const char *id)
     instr_set_event(fini_fn, NULL);
     instr_set_event(guest_cpu_enter, NULL);
     instr_set_event(guest_cpu_exit, NULL);
+    instr_set_event(guest_cpu_reset, NULL);
 
     instr_cpu_stop_all_end(&info);
     cpu_list_unlock();
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
index 107ee8afe0..322009100d 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -96,6 +96,15 @@ void qi_event_set_guest_cpu_enter(void (*fn)(QICPU vcpu));
  */
 void qi_event_set_guest_cpu_exit(void (*fn)(QICPU vcpu));
 
+/*
+ * Reset the state of a virtual (guest) CPU.
+ *
+ * Mode: user, softmmu
+ * Targets: all
+ * Time: exec
+ */
+void qi_event_set_guest_cpu_reset(void (*fn)(QICPU vcpu));
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/qom/cpu.c b/qom/cpu.c
index dc5392dbeb..6336d63f66 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -19,6 +19,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "instrument/events.h"
 #include "qapi/error.h"
 #include "qemu-common.h"
 #include "qom/cpu.h"
@@ -275,6 +276,7 @@ void cpu_reset(CPUState *cpu)
         (*klass->reset)(cpu);
     }
 
+    instr_guest_cpu_reset(cpu);
     trace_guest_cpu_reset(cpu);
 }
 
diff --git a/stubs/instrument.c b/stubs/instrument.c
index e7adea1aad..752c66e3a4 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -44,3 +44,4 @@ void qmp_instr_unload(const char *id, Error **errp)
 __thread InstrState instr_cur_state;
 void (*instr_event__guest_cpu_enter)(QICPU *vcpu);
 void (*instr_event__guest_cpu_exit)(QICPU *vcpu);
+void (*instr_event__guest_cpu_reset)(QICPU *vcpu);

[Qemu-devel] [PATCH v6 17/22] trace: Introduce a proper structure to describe memory accesses

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 include/exec/cpu_ldst_template.h          |   15 ++++++--------
 include/exec/cpu_ldst_useronly_template.h |   15 ++++++--------
 tcg/tcg-op.c                              |   22 +++++++++++++--------
 trace/mem-internal.h                      |   22 ++++++++++++---------
 trace/mem.h                               |   31 +++++++++++++++++++++++++----
 5 files changed, 66 insertions(+), 39 deletions(-)

diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
index 4db2302962..debbabcfb2 100644
--- a/include/exec/cpu_ldst_template.h
+++ b/include/exec/cpu_ldst_template.h
@@ -88,9 +88,8 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     TCGMemOpIdx oi;
 
 #if !defined(SOFTMMU_CODE_ACCESS)
-    trace_guest_mem_before_exec(
-        ENV_GET_CPU(env), ptr,
-        trace_mem_build_info(SHIFT, false, MO_TE, false));
+    TraceMemInfo meminfo = trace_mem_build_info(SHIFT, false, MO_TE, false);
+    trace_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo.raw);
 #endif
 
     addr = ptr;
@@ -126,9 +125,8 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     TCGMemOpIdx oi;
 
 #if !defined(SOFTMMU_CODE_ACCESS)
-    trace_guest_mem_before_exec(
-        ENV_GET_CPU(env), ptr,
-        trace_mem_build_info(SHIFT, true, MO_TE, false));
+    TraceMemInfo meminfo = trace_mem_build_info(SHIFT, true, MO_TE, false);
+    trace_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo.raw);
 #endif
 
     addr = ptr;
@@ -168,9 +166,8 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     TCGMemOpIdx oi;
 
 #if !defined(SOFTMMU_CODE_ACCESS)
-    trace_guest_mem_before_exec(
-        ENV_GET_CPU(env), ptr,
-        trace_mem_build_info(SHIFT, false, MO_TE, true));
+    TraceMemInfo meminfo = trace_mem_build_info(SHIFT, false, MO_TE, true);
+    trace_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo.raw);
 #endif
 
     addr = ptr;
diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
index 7b8c7c506e..b0b3fc1b8d 100644
--- a/include/exec/cpu_ldst_useronly_template.h
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -61,9 +61,8 @@ static inline RES_TYPE
 glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
 {
 #if !defined(CODE_ACCESS)
-    trace_guest_mem_before_exec(
-        ENV_GET_CPU(env), ptr,
-        trace_mem_build_info(DATA_SIZE, false, MO_TE, false));
+    TraceMemInfo meminfo = trace_mem_build_info(DATA_SIZE, false, MO_TE, false);
+    trace_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo.raw);
 #endif
     return glue(glue(ld, USUFFIX), _p)(g2h(ptr));
 }
@@ -81,9 +80,8 @@ static inline int
 glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
 {
 #if !defined(CODE_ACCESS)
-    trace_guest_mem_before_exec(
-        ENV_GET_CPU(env), ptr,
-        trace_mem_build_info(DATA_SIZE, true, MO_TE, false));
+    TraceMemInfo meminfo = trace_mem_build_info(DATA_SIZE, true, MO_TE, false);
+    trace_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo.raw);
 #endif
     return glue(glue(lds, SUFFIX), _p)(g2h(ptr));
 }
@@ -103,9 +101,8 @@ glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
                                       RES_TYPE v)
 {
 #if !defined(CODE_ACCESS)
-    trace_guest_mem_before_exec(
-        ENV_GET_CPU(env), ptr,
-        trace_mem_build_info(DATA_SIZE, false, MO_TE, true));
+    TraceMemInfo meminfo = trace_mem_build_info(DATA_SIZE, false, MO_TE, true);
+    trace_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo.raw);
 #endif
     glue(glue(st, SUFFIX), _p)(g2h(ptr), v);
 }
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 688d91755b..6edf70bdfc 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -2676,24 +2676,28 @@ static void tcg_gen_req_mo(TCGBar type)
 
 void tcg_gen_qemu_ld_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
 {
+    TraceMemInfo meminfo;
     tcg_gen_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     memop = tcg_canonicalize_memop(memop, 0, 0);
-    trace_guest_mem_before_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env,
-                               addr, trace_mem_get_info(memop, 0));
+    meminfo = trace_mem_get_info(memop, 0);
+    trace_guest_mem_before_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env, addr, meminfo.raw);
     gen_ldst_i32(INDEX_op_qemu_ld_i32, val, addr, memop, idx);
 }
 
 void tcg_gen_qemu_st_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
 {
+    TraceMemInfo meminfo;
     tcg_gen_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     memop = tcg_canonicalize_memop(memop, 0, 1);
-    trace_guest_mem_before_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env,
-                               addr, trace_mem_get_info(memop, 1));
+    meminfo = trace_mem_get_info(memop, 1);
+    trace_guest_mem_before_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env, addr, meminfo.raw);
     gen_ldst_i32(INDEX_op_qemu_st_i32, val, addr, memop, idx);
 }
 
 void tcg_gen_qemu_ld_i64(TCGv_i64 val, TCGv addr, TCGArg idx, TCGMemOp memop)
 {
+    TraceMemInfo meminfo;
+
     tcg_gen_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     if (TCG_TARGET_REG_BITS == 32 && (memop & MO_SIZE) < MO_64) {
         tcg_gen_qemu_ld_i32(TCGV_LOW(val), addr, idx, memop);
@@ -2706,13 +2710,15 @@ void tcg_gen_qemu_ld_i64(TCGv_i64 val, TCGv addr, TCGArg idx, TCGMemOp memop)
     }
 
     memop = tcg_canonicalize_memop(memop, 1, 0);
-    trace_guest_mem_before_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env,
-                               addr, trace_mem_get_info(memop, 0));
+    meminfo = trace_mem_get_info(memop, 0);
+    trace_guest_mem_before_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env, addr, meminfo.raw);
     gen_ldst_i64(INDEX_op_qemu_ld_i64, val, addr, memop, idx);
 }
 
 void tcg_gen_qemu_st_i64(TCGv_i64 val, TCGv addr, TCGArg idx, TCGMemOp memop)
 {
+    TraceMemInfo meminfo;
+
     tcg_gen_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     if (TCG_TARGET_REG_BITS == 32 && (memop & MO_SIZE) < MO_64) {
         tcg_gen_qemu_st_i32(TCGV_LOW(val), addr, idx, memop);
@@ -2720,8 +2726,8 @@ void tcg_gen_qemu_st_i64(TCGv_i64 val, TCGv addr, TCGArg idx, TCGMemOp memop)
     }
 
     memop = tcg_canonicalize_memop(memop, 1, 1);
-    trace_guest_mem_before_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env,
-                               addr, trace_mem_get_info(memop, 1));
+    meminfo = trace_mem_get_info(memop, 1);
+    trace_guest_mem_before_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env, addr, meminfo.raw);
     gen_ldst_i64(INDEX_op_qemu_st_i64, val, addr, memop, idx);
 }
 
diff --git a/trace/mem-internal.h b/trace/mem-internal.h
index ddda934253..b77079527f 100644
--- a/trace/mem-internal.h
+++ b/trace/mem-internal.h
@@ -1,7 +1,7 @@
 /*
  * Helper functions for guest memory tracing
  *
- * Copyright (C) 2016 Lluís Vilanova <vilanova@ac.upc.edu>
+ * Copyright (C) 2016-2017 Lluís Vilanova <vilanova@ac.upc.edu>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
@@ -10,8 +10,9 @@
 #ifndef TRACE__MEM_INTERNAL_H
 #define TRACE__MEM_INTERNAL_H
 
-static inline uint8_t trace_mem_get_info(TCGMemOp op, bool store)
+static inline TraceMemInfo trace_mem_get_info(TCGMemOp op, bool store)
 {
+    TraceMemInfo res_;
     uint8_t res = op;
     bool be = (op & MO_BSWAP) == MO_BE;
 
@@ -27,19 +28,22 @@ static inline uint8_t trace_mem_get_info(TCGMemOp op, bool store)
         res |= 1ULL << 4;
     }
 
-    return res;
+    res_.raw = res;
+    return res_;
 }
 
-static inline uint8_t trace_mem_build_info(
+static inline TraceMemInfo trace_mem_build_info(
     TCGMemOp size, bool sign_extend, TCGMemOp endianness, bool store)
 {
-    uint8_t res = 0;
-    res |= size;
-    res |= (sign_extend << 2);
+    TraceMemInfo res;
+    res.size_shift = size;
+    res.sign_extend = sign_extend;
     if (endianness == MO_BE) {
-        res |= (1ULL << 3);
+        res.endianness = 1;
+    } else {
+        res.endianness = 0;
     }
-    res |= (store << 4);
+    res.store = store;
     return res;
 }
 
diff --git a/trace/mem.h b/trace/mem.h
index 9c88bcb4e6..9866b41401 100644
--- a/trace/mem.h
+++ b/trace/mem.h
@@ -1,7 +1,7 @@
 /*
  * Helper functions for guest memory tracing
  *
- * Copyright (C) 2016 Lluís Vilanova <vilanova@ac.upc.edu>
+ * Copyright (C) 2016-2017 Lluís Vilanova <vilanova@ac.upc.edu>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
@@ -12,21 +12,44 @@
 
 #include "tcg/tcg.h"
 
+/**
+ * TraceMemInfo:
+ * @size_shift: Memoy access size, interpreted as "1 << size_shift" bytes.
+ * @sign_extend: Whether the access is sign-extended.
+ * @endianness: Endinness type (0: little, 1: big).
+ * @store: Whether it's a store operation.
+ *
+ * Memory access information.
+ *
+ * NOTE: Keep in sync with QIMemInfo.
+ */
+typedef struct TraceMemInfo {
+    union {
+        struct {
+            uint8_t size_shift : 2;
+            bool    sign_extend: 1;
+            uint8_t endianness : 1;
+            bool    store      : 1;
+        };
+        uint8_t raw;
+    };
+} TraceMemInfo;
+
 
 /**
  * trace_mem_get_info:
  *
  * Return a value for the 'info' argument in guest memory access traces.
  */
-static uint8_t trace_mem_get_info(TCGMemOp op, bool store);
+static TraceMemInfo trace_mem_get_info(TCGMemOp op, bool store);
 
 /**
  * trace_mem_build_info:
  *
  * Return a value for the 'info' argument in guest memory access traces.
  */
-static uint8_t trace_mem_build_info(TCGMemOp size, bool sign_extend,
-                                    TCGMemOp endianness, bool store);
+static TraceMemInfo trace_mem_build_info(TCGMemOp size, bool sign_extend,
+                                         TCGMemOp endianness, bool store);
 
 
 #include "trace/mem-internal.h"

[Qemu-devel] [PATCH v6 18/22] instrument: Add event 'guest_mem_before_trans'

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 Makefile.target                 |    1 +
 instrument/control.c            |   15 +++++++++
 instrument/control.h            |   36 +++++++++++++++++++++-
 instrument/control.inc.h        |   16 +++++++---
 instrument/events.h             |   21 +++++++++++++
 instrument/events.inc.h         |   20 ++++++++++++
 instrument/load.c               |    1 +
 instrument/qemu-instr/control.h |   16 ++++++++++
 instrument/qemu-instr/types.h   |   64 +++++++++++++++++++++++++++++++++++++++
 stubs/instrument.c              |    4 ++
 tcg/tcg-op.c                    |    5 +++
 trace/control.h                 |   23 ++++++++++++++
 trace/mem.h                     |   23 --------------
 13 files changed, 214 insertions(+), 31 deletions(-)

diff --git a/Makefile.target b/Makefile.target
index 7f42c45db8..6997b921c9 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -196,6 +196,7 @@ $(QEMU_PROG_BUILD): config-devices.mak
 COMMON_LDADDS = ../libqemuutil.a ../libqemustub.a
 
 # build either PROG or PROGW
+$(QEMU_PROG_BUILD): CFLAGS += -DQEMU_TARGET_BUILD=1
 $(QEMU_PROG_BUILD): $(all-obj-y) $(COMMON_LDADDS)
 	$(call LINK, $(filter-out %.mak, $^))
 ifdef CONFIG_DARWIN
diff --git a/instrument/control.c b/instrument/control.c
index cb397639ce..2e910f963b 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -17,7 +17,7 @@
 #include "qom/cpu.h"
 
 
-__thread InstrState instr_cur_state;
+__thread InstrInfo instr_cur_info;
 
 
 unsigned int instr_cpus_count;
@@ -143,3 +143,16 @@ SYM_PUBLIC void qi_event_set_guest_cpu_reset(void (*fn)(QICPU vcpu))
     ERROR_IF(!instr_get_state(), "called outside instrumentation");
     instr_set_event(guest_cpu_reset, fn);
 }
+
+
+void (*instr_event__guest_mem_before_trans)(
+    QICPU vcpu_trans, QITCGv_cpu vcpu_exec, QITCGv vaddr, QIMemInfo info);
+
+SYM_PUBLIC void qi_event_set_guest_mem_before_trans(
+    void (*fn)(QICPU vcpu_trans, QITCGv_cpu vcpu_exec,
+               QITCGv vaddr, QIMemInfo info))
+{
+    ERROR_IF(!instr_get_state(), "called outside instrumentation");
+    ERROR_IF(!tcg_enabled(), "called without TCG");
+    instr_set_event(guest_mem_before_trans, fn);
+}
diff --git a/instrument/control.h b/instrument/control.h
index 03e87b2b8f..3e44702f75 100644
--- a/instrument/control.h
+++ b/instrument/control.h
@@ -86,12 +86,21 @@ typedef enum {
     INSTR_STATE_ENABLE,
 } InstrState;
 
+#define INSTR_MAX_TCG_REGS 16
+
+typedef struct InstrInfo {
+    InstrState state;
+    unsigned int max;
+    void *tcg_regs[INSTR_MAX_TCG_REGS];
+} InstrInfo;
+
 /**
  * instr_set_state:
  *
- * Set the instrumentation state of the current host thread.
+ * Set the instrumentation state of the current host thread, and return its
+ * #InstrInfo.
  */
-static inline void instr_set_state(InstrState state);
+static inline InstrInfo *instr_set_state(InstrState state);
 
 /**
  * instr_get_state:
@@ -100,6 +109,29 @@ static inline void instr_set_state(InstrState state);
  */
 static inline InstrState instr_get_state(void);
 
+/**
+ * instr_tcg_to_qitcg:
+ * @info: Pointer to #InstrInfo.
+ * @num: Number of TCG register used by instrumentation.
+ * @arg: TCG register.
+ *
+ * Get a suitable QITCGv* from a TCGv* value.
+ */
+#define instr_tcg_to_qitcg(info, num, arg) \
+    ({                                \
+        info->tcg_regs[num] = arg;    \
+        (void *)num;                  \
+    })
+
+/**
+ * instr_tcg_count:
+ * @info: Pointer to #InstrInfo.
+ * @count: Number of TCG registers used by instrumentation.
+ *
+ * Set the number of TCG registers used by instrumentation.
+ */
+static inline void instr_tcg_count(InstrInfo *info, unsigned int count);
+
 
 #include "instrument/control.inc.h"
 
diff --git a/instrument/control.inc.h b/instrument/control.inc.h
index 6d65b23ead..3eba9b7c85 100644
--- a/instrument/control.inc.h
+++ b/instrument/control.inc.h
@@ -46,14 +46,22 @@ static inline CPUState *instr_cpu_from_qicpu(QICPU vcpu)
 }
 
 
-extern __thread InstrState instr_cur_state;
+extern __thread InstrInfo instr_cur_info;
 
-static inline void instr_set_state(InstrState state)
+static inline InstrInfo *instr_set_state(InstrState state)
 {
-    atomic_store_release(&instr_cur_state, state);
+    InstrInfo *info = &instr_cur_info;
+    atomic_store_release(&info->state, state);
+    return info;
 }
 
 static inline InstrState instr_get_state(void)
 {
-    return atomic_load_acquire(&instr_cur_state);
+    return atomic_load_acquire(&instr_cur_info.state);
+}
+
+
+static inline void instr_tcg_count(InstrInfo *info, unsigned int count)
+{
+    info->max = count;
 }
diff --git a/instrument/events.h b/instrument/events.h
index 4a0560490a..1cc4dbb052 100644
--- a/instrument/events.h
+++ b/instrument/events.h
@@ -12,6 +12,8 @@
 
 #include "instrument/qemu-instr/control.h"
 #include "instrument/qemu-instr/types.h"
+#include "trace/control.h"
+
 
 /**
  * instr_get_event:
@@ -30,6 +32,20 @@
     atomic_store_release(&instr_event__ ## name, fn)
 
 
+/*
+ * Re-define types used by some instrumentation events. We need some arbitrary
+ * definition for non-target objects.
+ */
+#if defined(QEMU_TARGET_BUILD)
+#include "tcg/tcg.h"
+#else
+typedef struct TCGv_d *TCGv;
+typedef struct TCGv_env_d *TCGv_env;
+typedef struct TCGv_i32_d *TCGv_i32;
+typedef struct TCGv_i64_d *TCGv_i64;
+#endif
+
+
 extern qi_fini_fn instr_event__fini_fn;
 extern void *instr_event__fini_data;
 
@@ -42,6 +58,11 @@ static inline void instr_guest_cpu_exit(CPUState *vcpu);
 extern void (*instr_event__guest_cpu_reset)(QICPU vcpu);
 static inline void instr_guest_cpu_reset(CPUState *vcpu);
 
+extern void (*instr_event__guest_mem_before_trans)(
+    QICPU vcpu_trans, QITCGv_cpu vcpu_exec, QITCGv vaddr, QIMemInfo info);
+static inline void instr_guest_mem_before_trans(
+    CPUState *vcpu_trans, TCGv_env vcpu_exec, TCGv vaddr, TraceMemInfo info);
+
 
 #include "instrument/events.inc.h"
 
diff --git a/instrument/events.inc.h b/instrument/events.inc.h
index a126ba5ae6..365c715db4 100644
--- a/instrument/events.inc.h
+++ b/instrument/events.inc.h
@@ -8,6 +8,7 @@
  */
 
 #include "instrument/control.h"
+#include "trace/control.h"
 
 
 static inline void instr_guest_cpu_enter(CPUState *vcpu)
@@ -42,3 +43,22 @@ static inline void instr_guest_cpu_reset(CPUState *vcpu)
         instr_set_state(INSTR_STATE_DISABLE);
     }
 }
+
+static inline void instr_guest_mem_before_trans(
+    CPUState *vcpu_trans, TCGv_env vcpu_exec, TCGv vaddr, TraceMemInfo info)
+{
+    void (*cb)(QICPU vcpu_trans, QITCGv_cpu vcpu_exec,
+               QITCGv vaddr, QIMemInfo info)
+        = instr_get_event(guest_mem_before_trans);
+    if (cb) {
+        InstrInfo *iinfo = instr_set_state(INSTR_STATE_ENABLE);
+        QICPU vcpu_trans_ = instr_cpu_to_qicpu(vcpu_trans);
+        QITCGv_cpu vcpu_exec_ = instr_tcg_to_qitcg(iinfo, 0, vcpu_exec);
+        QITCGv vaddr_ = instr_tcg_to_qitcg(iinfo, 1, vaddr);
+        QIMemInfo info_;
+        info_.raw = info.raw;
+        instr_tcg_count(iinfo, 2);
+        (*cb)(vcpu_trans_, vcpu_exec_, vaddr_, info_);
+        instr_set_state(INSTR_STATE_DISABLE);
+    }
+}
diff --git a/instrument/load.c b/instrument/load.c
index 8c15a73a8c..e8f869201b 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -162,6 +162,7 @@ InstrUnloadError instr_unload(const char *id)
     instr_set_event(guest_cpu_enter, NULL);
     instr_set_event(guest_cpu_exit, NULL);
     instr_set_event(guest_cpu_reset, NULL);
+    instr_set_event(guest_mem_before_trans, NULL);
 
     instr_cpu_stop_all_end(&info);
     cpu_list_unlock();
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
index 322009100d..c3c8c3988d 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -105,6 +105,22 @@ void qi_event_set_guest_cpu_exit(void (*fn)(QICPU vcpu));
  */
 void qi_event_set_guest_cpu_reset(void (*fn)(QICPU vcpu));
 
+/*
+ * Start virtual memory access (before any potential access violation).
+ *
+ * @vaddr: Access' virtual address.
+ * @info : Access' information.
+ *
+ * Does not include memory accesses performed by devices.
+ *
+ * Mode: user, softmmu
+ * Targets: TCG(all)
+ * Time: trans
+ */
+void qi_event_set_guest_mem_before_trans(
+    void (*fn)(QICPU vcpu_trans, QITCGv_cpu vcpu_exec,
+               QITCGv vaddr, QIMemInfo info));
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/instrument/qemu-instr/types.h b/instrument/qemu-instr/types.h
index ea3a032b4f..11cbe1ccaa 100644
--- a/instrument/qemu-instr/types.h
+++ b/instrument/qemu-instr/types.h
@@ -14,10 +14,18 @@
 extern "C" {
 #endif
 
+#include <stdbool.h>
+#include <stdint.h>
+
+
 /**
  * SECTION: types
  * @section_id: qi-types
  * @title: Common types
+ *
+ * Data of architecture-specific length is always passed as an #int64_t to
+ * provide binary compatibility between the instrumentation library and QEMU,
+ * regardless of the guest architecture being instrumented.
  */
 
 /**
@@ -41,6 +49,62 @@ typedef struct QITraceEventIter QITraceEventIter;
  */
 typedef struct QICPU_d *QICPU;
 
+/**
+ * QIMemInfo:
+ * @size_shift: Memoy access size, interpreted as "1 << size_shift" bytes.
+ * @sign_extend: Whether the access is sign-extended.
+ * @endianness: Endianness type (0: little, 1: big).
+ * @store: Whether it's a store operation.
+ *
+ * Memory access information.
+ */
+typedef struct QIMemInfo {
+    union {
+        struct {
+            uint8_t size_shift : 2;
+            bool    sign_extend: 1;
+            uint8_t endianness : 1;
+            bool    store      : 1;
+        };
+        uint8_t raw;
+    };
+} QIMemInfo;
+
+/**
+ * QITCGv_cpu:
+ *
+ * TCG register with QICPU.
+ */
+typedef struct QITCGv_cpu_d *QITCGv_cpu;
+
+/**
+ * QITCGv:
+ *
+ * TCG register with data of architecture-specific length.
+ */
+typedef struct QITCGv_d *QITCGv;
+
+/**
+ * QITCGv_i32:
+ *
+ * TCG register with 32-bit data.
+ */
+typedef struct QITCGv_i32_d *QITCGv_i32;
+
+/**
+ * QITCGv_i64:
+ *
+ * TCG register with 64-bit data.
+ */
+typedef struct QITCGv_i64_d *QITCGv_i64;
+
+/*
+ * QITCGv_ptr:
+ *
+ * TCG register with pointer of architecture-specific length.
+ */
+typedef struct QITCGv_ptr_d *QITCGv_ptr;
+
 
 #include <qemu-instr/types.inc.h>
 
diff --git a/stubs/instrument.c b/stubs/instrument.c
index 752c66e3a4..ef4eeba603 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -41,7 +41,9 @@ void qmp_instr_unload(const char *id, Error **errp)
 }
 
 
-__thread InstrState instr_cur_state;
+__thread InstrInfo instr_cur_info;
 void (*instr_event__guest_cpu_enter)(QICPU *vcpu);
 void (*instr_event__guest_cpu_exit)(QICPU *vcpu);
 void (*instr_event__guest_cpu_reset)(QICPU *vcpu);
+void (*instr_event__guest_mem_before_trans)(
+    QICPU vcpu_trans, QITCGv_cpu vcpu_exec, QITCGv vaddr, QIMemInfo info);
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 6edf70bdfc..295c0c5a4a 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -26,6 +26,7 @@
 #include "qemu-common.h"
 #include "cpu.h"
 #include "exec/exec-all.h"
+#include "instrument/events.h"
 #include "tcg.h"
 #include "tcg-op.h"
 #include "tcg-mo.h"
@@ -2680,6 +2681,7 @@ void tcg_gen_qemu_ld_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
     tcg_gen_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     memop = tcg_canonicalize_memop(memop, 0, 0);
     meminfo = trace_mem_get_info(memop, 0);
+    instr_guest_mem_before_trans(tcg_ctx.cpu, tcg_ctx.tcg_env, addr, meminfo);
     trace_guest_mem_before_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env, addr, meminfo.raw);
     gen_ldst_i32(INDEX_op_qemu_ld_i32, val, addr, memop, idx);
 }
@@ -2690,6 +2692,7 @@ void tcg_gen_qemu_st_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
     tcg_gen_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     memop = tcg_canonicalize_memop(memop, 0, 1);
     meminfo = trace_mem_get_info(memop, 1);
+    instr_guest_mem_before_trans(tcg_ctx.cpu, tcg_ctx.tcg_env, addr, meminfo);
     trace_guest_mem_before_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env, addr, meminfo.raw);
     gen_ldst_i32(INDEX_op_qemu_st_i32, val, addr, memop, idx);
 }
@@ -2711,6 +2714,7 @@ void tcg_gen_qemu_ld_i64(TCGv_i64 val, TCGv addr, TCGArg idx, TCGMemOp memop)
 
     memop = tcg_canonicalize_memop(memop, 1, 0);
     meminfo = trace_mem_get_info(memop, 0);
+    instr_guest_mem_before_trans(tcg_ctx.cpu, tcg_ctx.tcg_env, addr, meminfo);
     trace_guest_mem_before_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env, addr, meminfo.raw);
     gen_ldst_i64(INDEX_op_qemu_ld_i64, val, addr, memop, idx);
 }
@@ -2727,6 +2731,7 @@ void tcg_gen_qemu_st_i64(TCGv_i64 val, TCGv addr, TCGArg idx, TCGMemOp memop)
 
     memop = tcg_canonicalize_memop(memop, 1, 1);
     meminfo = trace_mem_get_info(memop, 1);
+    instr_guest_mem_before_trans(tcg_ctx.cpu, tcg_ctx.tcg_env, addr, meminfo);
     trace_guest_mem_before_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env, addr, meminfo.raw);
     gen_ldst_i64(INDEX_op_qemu_st_i64, val, addr, memop, idx);
 }
diff --git a/trace/control.h b/trace/control.h
index 3e6da24c98..6b3fe9a28f 100644
--- a/trace/control.h
+++ b/trace/control.h
@@ -20,6 +20,29 @@ typedef struct TraceEventIter {
     const char *pattern;
 } TraceEventIter;
 
+/**
+ * TraceMemInfo:
+ * @size_shift: Memoy access size, interpreted as "1 << size_shift" bytes.
+ * @sign_extend: Whether the access is sign-extended.
+ * @endianness: Endinness type (0: little, 1: big).
+ * @store: Whether it's a store operation.
+ *
+ * Memory access information.
+ *
+ * NOTE: Keep in sync with QIMemInfo.
+ */
+typedef struct TraceMemInfo {
+    union {
+        struct {
+            uint8_t size_shift : 2;
+            bool    sign_extend: 1;
+            uint8_t endianness : 1;
+            bool    store      : 1;
+        };
+        uint8_t raw;
+    };
+} TraceMemInfo;
+
 
 /**
  * trace_event_iter_init:
diff --git a/trace/mem.h b/trace/mem.h
index 9866b41401..bc89673272 100644
--- a/trace/mem.h
+++ b/trace/mem.h
@@ -12,29 +12,6 @@
 
 #include "tcg/tcg.h"
 
-/**
- * TraceMemInfo:
- * @size_shift: Memoy access size, interpreted as "1 << size_shift" bytes.
- * @sign_extend: Whether the access is sign-extended.
- * @endianness: Endinness type (0: little, 1: big).
- * @store: Whether it's a store operation.
- *
- * Memory access information.
- *
- * NOTE: Keep in sync with QIMemInfo.
- */
-typedef struct TraceMemInfo {
-    union {
-        struct {
-            uint8_t size_shift : 2;
-            bool    sign_extend: 1;
-            uint8_t endianness : 1;
-            bool    store      : 1;
-        };
-        uint8_t raw;
-    };
-} TraceMemInfo;
-
 
 /**
  * trace_mem_get_info:

[Qemu-devel] [PATCH v6 19/22] instrument: Add event 'guest_mem_before_exec'

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 include/exec/cpu_ldst_template.h          |    4 +++
 include/exec/cpu_ldst_useronly_template.h |    4 +++
 include/exec/helper-gen.h                 |    1 +
 include/exec/helper-proto.h               |    1 +
 include/exec/helper-tcg.h                 |    1 +
 instrument/control.c                      |   37 +++++++++++++++++++++++++++++
 instrument/control.h                      |   15 ++++++++++++
 instrument/events.h                       |    5 ++++
 instrument/events.inc.h                   |   18 +++++++++++++-
 instrument/helpers.h                      |    2 ++
 instrument/load.c                         |    1 +
 instrument/qemu-instr/control.h           |   21 ++++++++++++++++
 stubs/instrument.c                        |   21 ++++++++++++++++
 13 files changed, 129 insertions(+), 2 deletions(-)
 create mode 100644 instrument/helpers.h

diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
index debbabcfb2..8018e8b16a 100644
--- a/include/exec/cpu_ldst_template.h
+++ b/include/exec/cpu_ldst_template.h
@@ -28,6 +28,7 @@
 #include "trace-root.h"
 #endif
 
+#include "instrument/events.h"
 #include "trace/mem.h"
 
 #if DATA_SIZE == 8
@@ -89,6 +90,7 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
 
 #if !defined(SOFTMMU_CODE_ACCESS)
     TraceMemInfo meminfo = trace_mem_build_info(SHIFT, false, MO_TE, false);
+    instr_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo);
     trace_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo.raw);
 #endif
 
@@ -126,6 +128,7 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
 
 #if !defined(SOFTMMU_CODE_ACCESS)
     TraceMemInfo meminfo = trace_mem_build_info(SHIFT, true, MO_TE, false);
+    instr_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo);
     trace_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo.raw);
 #endif
 
@@ -167,6 +170,7 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
 
 #if !defined(SOFTMMU_CODE_ACCESS)
     TraceMemInfo meminfo = trace_mem_build_info(SHIFT, false, MO_TE, true);
+    instr_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo);
     trace_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo.raw);
 #endif
 
diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
index b0b3fc1b8d..c36c50ae41 100644
--- a/include/exec/cpu_ldst_useronly_template.h
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -27,6 +27,7 @@
 #include "trace-root.h"
 #endif
 
+#include "instrument/events.h"
 #include "trace/mem.h"
 
 #if DATA_SIZE == 8
@@ -62,6 +63,7 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
 {
 #if !defined(CODE_ACCESS)
     TraceMemInfo meminfo = trace_mem_build_info(DATA_SIZE, false, MO_TE, false);
+    instr_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo);
     trace_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo.raw);
 #endif
     return glue(glue(ld, USUFFIX), _p)(g2h(ptr));
@@ -81,6 +83,7 @@ glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
 {
 #if !defined(CODE_ACCESS)
     TraceMemInfo meminfo = trace_mem_build_info(DATA_SIZE, true, MO_TE, false);
+    instr_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo);
     trace_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo.raw);
 #endif
     return glue(glue(lds, SUFFIX), _p)(g2h(ptr));
@@ -102,6 +105,7 @@ glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
 {
 #if !defined(CODE_ACCESS)
     TraceMemInfo meminfo = trace_mem_build_info(DATA_SIZE, false, MO_TE, true);
+    instr_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo);
     trace_guest_mem_before_exec(ENV_GET_CPU(env), ptr, meminfo.raw);
 #endif
     glue(glue(st, SUFFIX), _p)(g2h(ptr), v);
diff --git a/include/exec/helper-gen.h b/include/exec/helper-gen.h
index 8239ffc77c..f351c3d050 100644
--- a/include/exec/helper-gen.h
+++ b/include/exec/helper-gen.h
@@ -57,6 +57,7 @@ static inline void glue(gen_helper_, name)(dh_retvar_decl(ret)          \
 }
 
 #include "helper.h"
+#include "instrument/helpers.h"
 #include "trace/generated-helpers.h"
 #include "trace/generated-helpers-wrappers.h"
 #include "tcg-runtime.h"
diff --git a/include/exec/helper-proto.h b/include/exec/helper-proto.h
index 954bef85ce..8fdd02c132 100644
--- a/include/exec/helper-proto.h
+++ b/include/exec/helper-proto.h
@@ -27,6 +27,7 @@ dh_ctype(ret) HELPER(name) (dh_ctype(t1), dh_ctype(t2), dh_ctype(t3), \
                             dh_ctype(t4), dh_ctype(t5));
 
 #include "helper.h"
+#include "instrument/helpers.h"
 #include "trace/generated-helpers.h"
 #include "tcg-runtime.h"
 
diff --git a/include/exec/helper-tcg.h b/include/exec/helper-tcg.h
index b0c5bafa99..255e73c3e6 100644
--- a/include/exec/helper-tcg.h
+++ b/include/exec/helper-tcg.h
@@ -40,6 +40,7 @@
     | dh_sizemask(t5, 5) },
 
 #include "helper.h"
+#include "instrument/helpers.h"
 #include "trace/generated-helpers.h"
 #include "tcg-runtime.h"
 
diff --git a/instrument/control.c b/instrument/control.c
index 2e910f963b..3fcacb2853 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -15,6 +15,8 @@
 #include "qemu/compiler.h"
 #include "qemu/main-loop.h"
 #include "qom/cpu.h"
+#include "exec/helper-proto.h"
+#include "exec/helper-gen.h"
 
 
 __thread InstrInfo instr_cur_info;
@@ -156,3 +158,38 @@ SYM_PUBLIC void qi_event_set_guest_mem_before_trans(
     ERROR_IF(!tcg_enabled(), "called without TCG");
     instr_set_event(guest_mem_before_trans, fn);
 }
+
+
+SYM_PUBLIC void qi_event_gen_guest_mem_before_exec(
+    QITCGv_cpu vcpu, QITCGv vaddr, QIMemInfo info)
+{
+    ERROR_IF(instr_get_state() != INSTR_STATE_ENABLE_TCG,
+             "called outside instrumentation");
+    ERROR_IF(!tcg_enabled(), "called without TCG");
+    InstrInfo *iinfo = &instr_cur_info;
+    TCGv_env vcpu_ = instr_tcg_from_qitcg(iinfo, vcpu);
+    TCGv vaddr_ = instr_tcg_from_qitcg(iinfo, vaddr);
+    TCGv_i32 info_ = tcg_const_i32(info.raw);
+    gen_helper_instr_guest_mem_before_exec(vcpu_, vaddr_, info_);
+    tcg_temp_free_i32(info_);
+}
+
+void helper_instr_guest_mem_before_exec(
+    CPUArchState *vcpu, target_ulong vaddr, uint32_t info)
+{
+    TraceMemInfo info_;
+    info_.raw = info;
+    instr_guest_mem_before_exec(ENV_GET_CPU(vcpu), vaddr, info_);
+}
+
+
+void (*instr_event__guest_mem_before_exec)(
+    QICPU vcpu, uint64_t vaddr, QIMemInfo info);
+
+SYM_PUBLIC void qi_event_set_guest_mem_before_exec(
+    void (*fn)(QICPU vcpu, uint64_t vaddr, QIMemInfo info))
+{
+    ERROR_IF(!instr_get_state(), "called outside instrumentation");
+    ERROR_IF(!tcg_enabled(), "called without TCG");
+    instr_set_event(guest_mem_before_exec, fn);
+}
diff --git a/instrument/control.h b/instrument/control.h
index 3e44702f75..3b1d5c5344 100644
--- a/instrument/control.h
+++ b/instrument/control.h
@@ -84,6 +84,7 @@ void instr_cpu_stop_all_end(InstrCPUStop *info);
 typedef enum {
     INSTR_STATE_DISABLE,
     INSTR_STATE_ENABLE,
+    INSTR_STATE_ENABLE_TCG,
 } InstrState;
 
 #define INSTR_MAX_TCG_REGS 16
@@ -123,6 +124,20 @@ static inline InstrState instr_get_state(void);
         (void *)num;                  \
     })
 
+/**
+ * instr_tcg_from_qitcg:
+ * @info: Pointer to #InstrInfo.
+ * @arg: QITCG register.
+ *
+ * Get a suitable TCGv* from a QITCGv* value.
+ */
+#define instr_tcg_from_qitcg(info, arg)                                \
+    ({                                                          \
+        unsigned int idx = (uintptr_t)arg;                      \
+        ERROR_IF(info->max <= idx, "invalid QITCGv register");  \
+        info->tcg_regs[idx];                                  \
+    })
+
 /**
  * instr_tcg_count:
  * @info: Pointer to #InstrInfo.
diff --git a/instrument/events.h b/instrument/events.h
index 1cc4dbb052..6507b26867 100644
--- a/instrument/events.h
+++ b/instrument/events.h
@@ -63,6 +63,11 @@ extern void (*instr_event__guest_mem_before_trans)(
 static inline void instr_guest_mem_before_trans(
     CPUState *vcpu_trans, TCGv_env vcpu_exec, TCGv vaddr, TraceMemInfo info);
 
+extern void (*instr_event__guest_mem_before_exec)(
+    QICPU vcpu, uint64_t vaddr, QIMemInfo info);
+static inline void instr_guest_mem_before_exec(
+    CPUState *vcpu, uint64_t vaddr, TraceMemInfo info);
+
 
 #include "instrument/events.inc.h"
 
diff --git a/instrument/events.inc.h b/instrument/events.inc.h
index 365c715db4..ebc8020715 100644
--- a/instrument/events.inc.h
+++ b/instrument/events.inc.h
@@ -51,7 +51,7 @@ static inline void instr_guest_mem_before_trans(
                QITCGv vaddr, QIMemInfo info)
         = instr_get_event(guest_mem_before_trans);
     if (cb) {
-        InstrInfo *iinfo = instr_set_state(INSTR_STATE_ENABLE);
+        InstrInfo *iinfo = instr_set_state(INSTR_STATE_ENABLE_TCG);
         QICPU vcpu_trans_ = instr_cpu_to_qicpu(vcpu_trans);
         QITCGv_cpu vcpu_exec_ = instr_tcg_to_qitcg(iinfo, 0, vcpu_exec);
         QITCGv vaddr_ = instr_tcg_to_qitcg(iinfo, 1, vaddr);
@@ -62,3 +62,19 @@ static inline void instr_guest_mem_before_trans(
         instr_set_state(INSTR_STATE_DISABLE);
     }
 }
+
+static inline void instr_guest_mem_before_exec(
+    CPUState *vcpu, uint64_t vaddr, TraceMemInfo info)
+{
+    void (*cb)(QICPU vcpu, uint64_t vaddr, QIMemInfo info)
+        = instr_get_event(guest_mem_before_exec);
+    if (cb) {
+        InstrInfo *iinfo = instr_set_state(INSTR_STATE_ENABLE);
+        QICPU vcpu_ = instr_cpu_to_qicpu(vcpu);
+        QIMemInfo info_;
+        info_.raw = info.raw;
+        instr_tcg_count(iinfo, 2);
+        (*cb)(vcpu_, vaddr, info_);
+        instr_set_state(INSTR_STATE_DISABLE);
+    }
+}
diff --git a/instrument/helpers.h b/instrument/helpers.h
new file mode 100644
index 0000000000..199f781b89
--- /dev/null
+++ b/instrument/helpers.h
@@ -0,0 +1,2 @@
+DEF_HELPER_FLAGS_3(instr_guest_mem_before_exec, TCG_CALL_NO_RWG,
+                   void, env, tl, i32)
diff --git a/instrument/load.c b/instrument/load.c
index e8f869201b..f1d769b92d 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -163,6 +163,7 @@ InstrUnloadError instr_unload(const char *id)
     instr_set_event(guest_cpu_exit, NULL);
     instr_set_event(guest_cpu_reset, NULL);
     instr_set_event(guest_mem_before_trans, NULL);
+    instr_set_event(guest_mem_before_exec, NULL);
 
     instr_cpu_stop_all_end(&info);
     cpu_list_unlock();
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
index c3c8c3988d..acd4b10f03 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -121,6 +121,27 @@ void qi_event_set_guest_mem_before_trans(
     void (*fn)(QICPU vcpu_trans, QITCGv_cpu vcpu_exec,
                QITCGv vaddr, QIMemInfo info));
 
+/*
+ * Generate code to trigger a 'guest_mem_before_exec' from
+ * 'guest_mem_before_trans'.
+ *
+ * Mode: user, softmmu
+ * Targets: TCG(all)
+ * Time: trans
+ */
+void qi_event_gen_guest_mem_before_exec(
+    QITCGv_cpu vcpu, QITCGv vaddr, QIMemInfo info);
+
+/*
+ * Execution-time equivalent of 'guest_mem_before_trans'.
+ *
+ * Mode: user, softmmu
+ * Targets: TCG(all)
+ * Time: exec
+ */
+void qi_event_set_guest_mem_before_exec(
+    void (*fn)(QICPU vcpu, uint64_t vaddr, QIMemInfo info));
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/stubs/instrument.c b/stubs/instrument.c
index ef4eeba603..640c91f470 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -7,6 +7,9 @@
  * See the COPYING file in the top-level directory.
  */
 
+/* Unpoison missing types */
+#define HW_POISON_H
+
 #include "qemu/osdep.h"
 
 #include "instrument/cmdline.h"
@@ -15,6 +18,11 @@
 #include "qapi/qmp/qerror.h"
 
 
+/* Declare missing types */
+typedef struct CPUArchState CPUArchState;
+typedef int target_ulong;
+
+
 void instr_init(const char *path, int argc, const char **argv)
 {
 }
@@ -46,4 +54,15 @@ void (*instr_event__guest_cpu_enter)(QICPU *vcpu);
 void (*instr_event__guest_cpu_exit)(QICPU *vcpu);
 void (*instr_event__guest_cpu_reset)(QICPU *vcpu);
 void (*instr_event__guest_mem_before_trans)(
-    QICPU vcpu_trans, QITCGv_cpu vcpu_exec, QITCGv vaddr, QIMemInfo info);
+    QICPU vcpu_trans, QITCGv_cpu vcpu_exec,
+    QITCGv vaddr, QIMemInfo info);
+void helper_instr_guest_mem_before_exec(
+    CPUArchState *vcpu, target_ulong vaddr, uint32_t info);
+void helper_instr_guest_mem_before_exec(
+    CPUArchState *vcpu, target_ulong vaddr, uint32_t info)
+{
+    assert(false);
+}
+void (*instr_event__guest_mem_before_exec)(
+    QICPU vcpu_trans, QITCGv_cpu vcpu_exec,
+    QITCGv vaddr, QIMemInfo info);

[Qemu-devel] [PATCH v6 20/22] instrument: Add event 'guest_user_syscall'

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 bsd-user/syscall.c              |    6 ++++++
 instrument/control.c            |   18 ++++++++++++++++++
 instrument/events.h             |    7 +++++++
 instrument/events.inc.h         |   16 ++++++++++++++++
 instrument/load.c               |    1 +
 instrument/qemu-instr/control.h |   15 +++++++++++++++
 linux-user/syscall.c            |    2 ++
 stubs/instrument.c              |    3 +++
 8 files changed, 68 insertions(+)

diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
index 3230f722f3..0d92eaf8c4 100644
--- a/bsd-user/syscall.c
+++ b/bsd-user/syscall.c
@@ -324,6 +324,8 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, abi_long arg1,
 #ifdef DEBUG
     gemu_log("freebsd syscall %d\n", num);
 #endif
+    instr_guest_user_syscall(cpu, num,
+                             arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8);
     trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8);
     if(do_strace)
         print_freebsd_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6);
@@ -423,6 +425,8 @@ abi_long do_netbsd_syscall(void *cpu_env, int num, abi_long arg1,
 #ifdef DEBUG
     gemu_log("netbsd syscall %d\n", num);
 #endif
+    instr_guest_user_syscall(cpu, num,
+                             arg1, arg2, arg3, arg4, arg5, arg6, 0, 0);
     trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, 0, 0);
     if(do_strace)
         print_netbsd_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6);
@@ -499,6 +503,8 @@ abi_long do_openbsd_syscall(void *cpu_env, int num, abi_long arg1,
 #ifdef DEBUG
     gemu_log("openbsd syscall %d\n", num);
 #endif
+    instr_guest_user_syscall(cpu, num,
+                             arg1, arg2, arg3, arg4, arg5, arg6, 0, 0);
     trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, 0, 0);
     if(do_strace)
         print_openbsd_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6);
diff --git a/instrument/control.c b/instrument/control.c
index 3fcacb2853..45efeef7ac 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -193,3 +193,21 @@ SYM_PUBLIC void qi_event_set_guest_mem_before_exec(
     ERROR_IF(!tcg_enabled(), "called without TCG");
     instr_set_event(guest_mem_before_exec, fn);
 }
+
+
+void (*instr_event__guest_user_syscall)(
+    QICPU vcpu, uint64_t num, uint64_t arg1, uint64_t arg2, uint64_t arg3,
+    uint64_t arg4, uint64_t arg5, uint64_t arg6, uint64_t arg7, uint64_t arg8);
+
+SYM_PUBLIC void qi_event_set_guest_user_syscall(
+    void (*fn)(QICPU vcpu, uint64_t num, uint64_t arg1, uint64_t arg2,
+               uint64_t arg3, uint64_t arg4, uint64_t arg5, uint64_t arg6,
+               uint64_t arg7, uint64_t arg8))
+{
+    ERROR_IF(!instr_get_state(), "called outside instrumentation");
+    ERROR_IF(!tcg_enabled(), "called without TCG");
+#if !defined(CONFIG_USER_ONLY)
+    ERROR_IF(true, "called in full-system mode");
+#endif
+    instr_set_event(guest_user_syscall, fn);
+}
diff --git a/instrument/events.h b/instrument/events.h
index 6507b26867..8c944e1f91 100644
--- a/instrument/events.h
+++ b/instrument/events.h
@@ -68,6 +68,13 @@ extern void (*instr_event__guest_mem_before_exec)(
 static inline void instr_guest_mem_before_exec(
     CPUState *vcpu, uint64_t vaddr, TraceMemInfo info);
 
+extern void (*instr_event__guest_user_syscall)(
+    QICPU vcpu, uint64_t num, uint64_t arg1, uint64_t arg2, uint64_t arg3,
+    uint64_t arg4, uint64_t arg5, uint64_t arg6, uint64_t arg7, uint64_t arg8);
+static inline void instr_guest_user_syscall(
+    CPUState *vcpu, uint64_t num, uint64_t arg1, uint64_t arg2, uint64_t arg3,
+    uint64_t arg4, uint64_t arg5, uint64_t arg6, uint64_t arg7, uint64_t arg8);
+
 
 #include "instrument/events.inc.h"
 
diff --git a/instrument/events.inc.h b/instrument/events.inc.h
index ebc8020715..e2f4315fb0 100644
--- a/instrument/events.inc.h
+++ b/instrument/events.inc.h
@@ -78,3 +78,19 @@ static inline void instr_guest_mem_before_exec(
         instr_set_state(INSTR_STATE_DISABLE);
     }
 }
+
+static inline void instr_guest_user_syscall(
+    CPUState *vcpu, uint64_t num, uint64_t arg1, uint64_t arg2, uint64_t arg3,
+    uint64_t arg4, uint64_t arg5, uint64_t arg6, uint64_t arg7, uint64_t arg8)
+{
+    void (*cb)(QICPU vcpu, uint64_t num, uint64_t arg1, uint64_t arg2,
+               uint64_t arg3, uint64_t arg4, uint64_t arg5, uint64_t arg6,
+               uint64_t arg7, uint64_t arg8)
+        = instr_get_event(guest_user_syscall);
+    if (cb) {
+        instr_set_state(INSTR_STATE_ENABLE);
+        QICPU vcpu_ = instr_cpu_to_qicpu(vcpu);
+        (*cb)(vcpu_, num, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8);
+        instr_set_state(INSTR_STATE_DISABLE);
+    }
+}
diff --git a/instrument/load.c b/instrument/load.c
index f1d769b92d..a76f76e1d1 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -164,6 +164,7 @@ InstrUnloadError instr_unload(const char *id)
     instr_set_event(guest_cpu_reset, NULL);
     instr_set_event(guest_mem_before_trans, NULL);
     instr_set_event(guest_mem_before_exec, NULL);
+    instr_set_event(guest_user_syscall, NULL);
 
     instr_cpu_stop_all_end(&info);
     cpu_list_unlock();
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
index acd4b10f03..136058af4f 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -142,6 +142,21 @@ void qi_event_gen_guest_mem_before_exec(
 void qi_event_set_guest_mem_before_exec(
     void (*fn)(QICPU vcpu, uint64_t vaddr, QIMemInfo info));
 
+/*
+ * Start executing a guest system call in syscall emulation mode.
+ *
+ * @num: System call number.
+ * @arg*: System call argument value.
+ *
+ * Mode: user
+ * Targets: TCG(all)
+ * Time: exec
+ */
+void qi_event_set_guest_user_syscall(
+    void (*fn)(QICPU vcpu, uint64_t num, uint64_t arg1, uint64_t arg2,
+               uint64_t arg3, uint64_t arg4, uint64_t arg5, uint64_t arg6,
+               uint64_t arg7, uint64_t arg8));
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e73a07fa6f..c9f0b9fa56 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7723,6 +7723,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #ifdef DEBUG
     gemu_log("syscall %d", num);
 #endif
+    instr_guest_user_syscall(cpu, num,
+                             arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8);
     trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8);
     if(do_strace)
         print_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6);
diff --git a/stubs/instrument.c b/stubs/instrument.c
index 640c91f470..bb585c69ce 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -66,3 +66,6 @@ void helper_instr_guest_mem_before_exec(
 void (*instr_event__guest_mem_before_exec)(
     QICPU vcpu_trans, QITCGv_cpu vcpu_exec,
     QITCGv vaddr, QIMemInfo info);
+void (*instr_event__guest_user_syscall)(
+    QICPU vcpu, uint64_t num, uint64_t arg1, uint64_t arg2, uint64_t arg3,
+    uint64_t arg4, uint64_t arg5, uint64_t arg6, uint64_t arg7, uint64_t arg8);

[Qemu-devel] [PATCH v6 21/22] instrument: Add event 'guest_user_syscall_ret'

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 bsd-user/syscall.c              |    3 +++
 instrument/control.c            |   15 +++++++++++++++
 instrument/events.h             |    5 +++++
 instrument/events.inc.h         |   13 +++++++++++++
 instrument/load.c               |    1 +
 instrument/qemu-instr/control.h |   13 +++++++++++++
 linux-user/syscall.c            |    1 +
 stubs/instrument.c              |    2 ++
 8 files changed, 53 insertions(+)

diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
index 0d92eaf8c4..fb468c0574 100644
--- a/bsd-user/syscall.c
+++ b/bsd-user/syscall.c
@@ -407,6 +407,7 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, abi_long arg1,
 #endif
     if (do_strace)
         print_freebsd_syscall_ret(num, ret);
+    instr_guest_user_syscall_ret(cpu, num, ret);
     trace_guest_user_syscall_ret(cpu, num, ret);
     return ret;
  efault:
@@ -485,6 +486,7 @@ abi_long do_netbsd_syscall(void *cpu_env, int num, abi_long arg1,
 #endif
     if (do_strace)
         print_netbsd_syscall_ret(num, ret);
+    instr_guest_user_syscall_ret(cpu, num, ret);
     trace_guest_user_syscall_ret(cpu, num, ret);
     return ret;
  efault:
@@ -563,6 +565,7 @@ abi_long do_openbsd_syscall(void *cpu_env, int num, abi_long arg1,
 #endif
     if (do_strace)
         print_openbsd_syscall_ret(num, ret);
+    instr_guest_user_syscall_ret(cpu, num, ret);
     trace_guest_user_syscall_ret(cpu, num, ret);
     return ret;
  efault:
diff --git a/instrument/control.c b/instrument/control.c
index 45efeef7ac..e58a38b7db 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -211,3 +211,18 @@ SYM_PUBLIC void qi_event_set_guest_user_syscall(
 #endif
     instr_set_event(guest_user_syscall, fn);
 }
+
+
+void (*instr_event__guest_user_syscall_ret)(
+    QICPU vcpu, uint64_t num, uint64_t ret);
+
+SYM_PUBLIC void qi_event_set_guest_user_syscall_ret(
+    void (*fn)(QICPU vcpu, uint64_t num, uint64_t ret))
+{
+    ERROR_IF(!instr_get_state(), "called outside instrumentation");
+    ERROR_IF(!tcg_enabled(), "called without TCG");
+#if !defined(CONFIG_USER_ONLY)
+    ERROR_IF(true, "called in full-system mode");
+#endif
+    instr_set_event(guest_user_syscall_ret, fn);
+}
diff --git a/instrument/events.h b/instrument/events.h
index 8c944e1f91..6197ece466 100644
--- a/instrument/events.h
+++ b/instrument/events.h
@@ -75,6 +75,11 @@ static inline void instr_guest_user_syscall(
     CPUState *vcpu, uint64_t num, uint64_t arg1, uint64_t arg2, uint64_t arg3,
     uint64_t arg4, uint64_t arg5, uint64_t arg6, uint64_t arg7, uint64_t arg8);
 
+extern void (*instr_event__guest_user_syscall_ret)(
+    QICPU vcpu, uint64_t num, uint64_t ret);
+static inline void instr_guest_user_syscall_ret(
+    CPUState *vcpu, uint64_t num, uint64_t ret);
+
 
 #include "instrument/events.inc.h"
 
diff --git a/instrument/events.inc.h b/instrument/events.inc.h
index e2f4315fb0..d31dec54b8 100644
--- a/instrument/events.inc.h
+++ b/instrument/events.inc.h
@@ -94,3 +94,16 @@ static inline void instr_guest_user_syscall(
         instr_set_state(INSTR_STATE_DISABLE);
     }
 }
+
+static inline void instr_guest_user_syscall_ret(
+    CPUState *vcpu, uint64_t num, uint64_t ret)
+{
+    void (*cb)(QICPU vcpu, uint64_t num, uint64_t ret)
+        = instr_get_event(guest_user_syscall_ret);
+    if (cb) {
+        instr_set_state(INSTR_STATE_ENABLE);
+        QICPU vcpu_ = instr_cpu_to_qicpu(vcpu);
+        (*cb)(vcpu_, num, ret);
+        instr_set_state(INSTR_STATE_DISABLE);
+    }
+}
diff --git a/instrument/load.c b/instrument/load.c
index a76f76e1d1..be13a90286 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -165,6 +165,7 @@ InstrUnloadError instr_unload(const char *id)
     instr_set_event(guest_mem_before_trans, NULL);
     instr_set_event(guest_mem_before_exec, NULL);
     instr_set_event(guest_user_syscall, NULL);
+    instr_set_event(guest_user_syscall_ret, NULL);
 
     instr_cpu_stop_all_end(&info);
     cpu_list_unlock();
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
index 136058af4f..bc4e49bef1 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -157,6 +157,19 @@ void qi_event_set_guest_user_syscall(
                uint64_t arg3, uint64_t arg4, uint64_t arg5, uint64_t arg6,
                uint64_t arg7, uint64_t arg8));
 
+/*
+ * Finish executing a guest system call in syscall emulation mode.
+ *
+ * @num: System call number.
+ * @ret: System call result value.
+ *
+ * Mode: user
+ * Targets: TCG(all)
+ * Time: exec
+ */
+void qi_event_set_guest_user_syscall_ret(
+    void (*fn)(QICPU vcpu, uint64_t num, uint64_t ret));
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index c9f0b9fa56..44b91e3c52 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -12398,6 +12398,7 @@ fail:
 #endif
     if(do_strace)
         print_syscall_ret(num, ret);
+    instr_guest_user_syscall_ret(cpu, num, ret);
     trace_guest_user_syscall_ret(cpu, num, ret);
     return ret;
 efault:
diff --git a/stubs/instrument.c b/stubs/instrument.c
index bb585c69ce..ae581174db 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -69,3 +69,5 @@ void (*instr_event__guest_mem_before_exec)(
 void (*instr_event__guest_user_syscall)(
     QICPU vcpu, uint64_t num, uint64_t arg1, uint64_t arg2, uint64_t arg3,
     uint64_t arg4, uint64_t arg5, uint64_t arg6, uint64_t arg7, uint64_t arg8);
+void (*instr_event__guest_user_syscall_ret)(
+    QICPU vcpu, uint64_t num, uint64_t ret);

[Qemu-devel] [PATCH v6 22/22] instrument: Add API to manipulate guest memory

[Lluís Vilanova]

It includes access to the guest's memory and vCPU registers.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 instrument/Makefile.objs      |    1 
 instrument/qemu-instr/state.h |  104 +++++++++++++++++++++++++++++++++++++++++
 instrument/state.c            |   73 +++++++++++++++++++++++++++++
 3 files changed, 178 insertions(+)
 create mode 100644 instrument/qemu-instr/state.h
 create mode 100644 instrument/state.c

diff --git a/instrument/Makefile.objs b/instrument/Makefile.objs
index d7e6c760c3..ee482bdb45 100644
--- a/instrument/Makefile.objs
+++ b/instrument/Makefile.objs
@@ -5,3 +5,4 @@ target-obj-$(CONFIG_INSTRUMENT) += load.o
 target-obj-$(CONFIG_INSTRUMENT) += qmp.o
 target-obj-$(CONFIG_INSTRUMENT) += control.o
 target-obj-$(CONFIG_INSTRUMENT) += trace.o
+target-obj-$(CONFIG_INSTRUMENT) += state.o
diff --git a/instrument/qemu-instr/state.h b/instrument/qemu-instr/state.h
new file mode 100644
index 0000000000..0ae6255fe5
--- /dev/null
+++ b/instrument/qemu-instr/state.h
@@ -0,0 +1,104 @@
+/*
+ * Interface for accessing guest state.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QI__STATE_H
+#define QI__STATE_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <qemu-instr/types.h>
+
+
+/**
+ * qi_mem_read_virt:
+ * @vcpu: CPU to use for address translation.
+ * @vaddr: Starting virtual address to read from.
+ * @size: Number of bytes to read.
+ * @buf: Buffer to write into.
+ *
+ * Read contents from virtual memory.
+ *
+ * Returns: Whether the range of virtual addresses to read could be translated.
+ *
+ * Warning: Even on error, some of the destination buffer might have been
+ *          modified.
+ *
+ * Precondition: The output buffer has at least "size" bytes.
+ */
+bool qi_mem_read_virt(QICPU vcpu, uint64_t vaddr, size_t size, void *buf);
+
+/**
+ * qi_mem_write_virt:
+ * @vcpu: CPU to use for address translation.
+ * @vaddr: Starting virtual address to write into.
+ * @size: Number of bytes to write.
+ * @buf: Buffer with the contents to write from.
+ *
+ * Write contents into virtual memory.
+ *
+ * Returns: Whether the range of virtual addresses to write could be translated.
+ *
+ * Warning: Even on error, some of the destination memory might have been
+ *          modified.
+ * Precondition: The input buffer has at least "size" bytes.
+ */
+bool qi_mem_write_virt(QICPU vcpu, uint64_t vaddr, size_t size, void *buf);
+
+/**
+ * qi_mem_virt_to_phys:
+ * @vcpu: CPU to use for address translation.
+ * @vaddr: Virtual address to translate.
+ * @paddr: Pointer to output physical address.
+ *
+ * Translate a virtual address into a physical address.
+ *
+ * Returns: Whether the address could be translated.
+ */
+bool qi_mem_virt_to_phys(QICPU vcpu, uint64_t vaddr, uint64_t *paddr);
+
+/**
+ * qi_mem_read_phys:
+ * @paddr: Starting physical address to read from.
+ * @size: Number of bytes to read.
+ * @buf: Buffer to write into.
+ *
+ * Read contents from physical memory.
+ *
+ * Returns: Whether the range of physical addresses is valid.
+ *
+ * Warning: Even on error, some of the destination buffer might have been
+ *          modified.
+ * Precondition: The output buffer has at least "size" bytes.
+ */
+bool qi_mem_read_phys(uint64_t paddr, size_t size, void *buf);
+
+/**
+ * qi_mem_write_phys:
+ * @paddr: Starting physical address to write into.
+ * @size: Number of bytes to write.
+ * @buf: Buffer with the contents to write from.
+ *
+ * Write contents into virtual memory.
+ *
+ * Returns: Whether the range of physical addresses is valid.
+ *
+ * Warning: Even on error, some of the destination memory might have been
+ *          modified.
+ *
+ * Precondition: The input buffer has at least "size" bytes.
+ */
+bool qi_mem_write_phys(uint64_t paddr, size_t size, void *buf);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif  /* QI__STATE_H */
diff --git a/instrument/state.c b/instrument/state.c
new file mode 100644
index 0000000000..e76fd5fbcd
--- /dev/null
+++ b/instrument/state.c
@@ -0,0 +1,73 @@
+/*
+ * Interface for accessing guest state.
+ *
+ * Copyright (C) 2012-2017 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "qemu/compiler.h"
+#include "cpu.h"
+#include "exec/cpu-all.h"
+#include "instrument/control.h"
+#include "instrument/error.h"
+#include "instrument/qemu-instr/state.h"
+
+
+SYM_PUBLIC bool qi_mem_read_virt(QICPU vcpu, uint64_t vaddr,
+                                 size_t size, void *buf)
+{
+    CPUState *vcpu_ = instr_cpu_from_qicpu(vcpu);
+    ERROR_IF_RET(!instr_get_state(), false, "called outside instrumentation");
+    ERROR_IF_RET(!vcpu_, false, "invalid QICPU");
+    return cpu_memory_rw_debug(vcpu_, vaddr, buf, size, 0) == 0;
+}
+
+SYM_PUBLIC bool qi_mem_write_virt(QICPU vcpu, uint64_t vaddr,
+                                  size_t size, void *buf)
+{
+    CPUState *vcpu_ = instr_cpu_from_qicpu(vcpu);
+    ERROR_IF_RET(!instr_get_state(), false, "called outside instrumentation");
+    ERROR_IF_RET(!vcpu_, false, "invalid QICPU");
+    return cpu_memory_rw_debug(vcpu_, vaddr, buf, size, 1) == 0;
+}
+
+SYM_PUBLIC bool qi_mem_virt_to_phys(QICPU vcpu, uint64_t vaddr, uint64_t *paddr)
+{
+    CPUState *vcpu_ = instr_cpu_from_qicpu(vcpu);
+    ERROR_IF_RET(!instr_get_state(), false, "called outside instrumentation");
+    ERROR_IF_RET(!vcpu_, false, "invalid QICPU");
+
+#if defined(CONFIG_USER_ONLY)
+    *paddr = vaddr;
+    return true;
+#else
+    *paddr = cpu_get_phys_page_debug(vcpu_, vaddr);
+    return *paddr != -1;
+#endif
+}
+
+SYM_PUBLIC bool qi_mem_read_phys(uint64_t paddr, size_t size, void *buf)
+{
+    ERROR_IF_RET(!instr_get_state(), false, "called outside instrumentation");
+#if defined(CONFIG_USER_ONLY)
+    return cpu_memory_rw_debug(NULL, paddr, buf, size, 0) == 0;
+#else
+    cpu_physical_memory_read(paddr, buf, size);
+    return true;
+#endif
+}
+
+SYM_PUBLIC bool qi_mem_write_phys(uint64_t paddr, size_t size, void *buf)
+{
+    ERROR_IF_RET(!instr_get_state(), false, "called outside instrumentation");
+#if defined(CONFIG_USER_ONLY)
+    return cpu_memory_rw_debug(NULL, paddr, buf, size, 1) == 0;
+#else
+    cpu_physical_memory_write(paddr, buf, size);
+    return true;
+#endif
+}

Re: [Qemu-devel] [PATCH v6 00/22] instrument: Add basic event instrumentation

[no-reply]

Hi,

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

Subject: [Qemu-devel] [PATCH v6 00/22] instrument: Add basic event instrumentation
Message-id: 150529642278.10902.18234057937634437857.stgit@frigg.lan
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/150529642278.10902.18234057937634437857.stgit@frigg.lan -> patchew/150529642278.10902.18234057937634437857.stgit@frigg.lan
Switched to a new branch 'test'
8963b0695e instrument: Add API to manipulate guest memory
d8552c1e16 instrument: Add event 'guest_user_syscall_ret'
04bda8d5aa instrument: Add event 'guest_user_syscall'
1d8370cd51 instrument: Add event 'guest_mem_before_exec'
df27827de9 instrument: Add event 'guest_mem_before_trans'
4d144f90f0 trace: Introduce a proper structure to describe memory accesses
a4aa7e8863 instrument: Add event 'guest_cpu_reset'
387ca2ea04 instrument: Add event 'guest_cpu_exit'
5ae9ef5c01 exec: Add function to synchronously flush TB on a stopped vCPU
13e99981c9 instrument: Support synchronous modification of vCPU state
722663c82d instrument: Add event 'guest_cpu_enter'
0dc9a4a367 instrument: Track vCPUs
d86f598866 instrument: Add support for tracing events
d4aa1cea7f instrument: Add basic control interface
2d727bb6fb instrument: [hmp] Add library loader
3eb4b46981 instrument: [qapi] Add library loader
5641b387a2 instrument: [softmmu] Add command line library loader
d94ac79e7f instrument: [bsd-user] Add command line library loader
77c31d49ed instrument: [linux-user] Add command line library loader
f47ccffa50 instrument: Add generic library loader
3234817196 instrument: Add configure-time flag
67a448c69e instrument: Add documentation

=== OUTPUT BEGIN ===
Checking PATCH 1/22: instrument: Add documentation...
Checking PATCH 2/22: instrument: Add configure-time flag...
Checking PATCH 3/22: instrument: Add generic library loader...
Checking PATCH 4/22: instrument: [linux-user] Add command line library loader...
Checking PATCH 5/22: instrument: [bsd-user] Add command line library loader...
Checking PATCH 6/22: instrument: [softmmu] Add command line library loader...
Checking PATCH 7/22: instrument: [qapi] Add library loader...
ERROR: externs should be avoided in .c files
#250: FILE: stubs/instrument.c:36:
+void qmp_instr_unload(const char *id, Error **errp);

total: 1 errors, 0 warnings, 200 lines checked

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

Checking PATCH 8/22: instrument: [hmp] Add library loader...
Checking PATCH 9/22: instrument: Add basic control interface...
WARNING: architecture specific defines should be avoided
#52: FILE: include/qemu/compiler.h:119:
+#if defined _WIN32 || defined __CYGWIN__

WARNING: architecture specific defines should be avoided
#53: FILE: include/qemu/compiler.h:120:
+  #ifdef __GNUC__

WARNING: architecture specific defines should be avoided
#59: FILE: include/qemu/compiler.h:126:
+  #if __GNUC__ >= 4

WARNING: architecture specific defines should be avoided
#343: FILE: instrument/qemu-instr/control.h:13:
+#ifdef __cplusplus

WARNING: architecture specific defines should be avoided
#372: FILE: instrument/qemu-instr/control.h:42:
+#ifdef __cplusplus

total: 0 errors, 5 warnings, 309 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 10/22: instrument: Add support for tracing events...
WARNING: architecture specific defines should be avoided
#77: FILE: instrument/qemu-instr/types.h:13:
+#ifdef __cplusplus

WARNING: architecture specific defines should be avoided
#111: FILE: instrument/qemu-instr/types.h:47:
+#ifdef __cplusplus

total: 0 errors, 2 warnings, 225 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 11/22: instrument: Track vCPUs...
Checking PATCH 12/22: instrument: Add event 'guest_cpu_enter'...
Checking PATCH 13/22: instrument: Support synchronous modification of vCPU state...
Checking PATCH 14/22: exec: Add function to synchronously flush TB on a stopped vCPU...
Checking PATCH 15/22: instrument: Add event 'guest_cpu_exit'...
Checking PATCH 16/22: instrument: Add event 'guest_cpu_reset'...
Checking PATCH 17/22: trace: Introduce a proper structure to describe memory accesses...
ERROR: spaces prohibited around that ':' (ctx:WxW)
#244: FILE: trace/mem.h:29:
+            uint8_t size_shift : 2;
                                ^

ERROR: spaces prohibited around that ':' (ctx:VxW)
#245: FILE: trace/mem.h:30:
+            bool    sign_extend: 1;
                                ^

ERROR: spaces prohibited around that ':' (ctx:WxW)
#246: FILE: trace/mem.h:31:
+            uint8_t endianness : 1;
                                ^

ERROR: spaces prohibited around that ':' (ctx:WxW)
#247: FILE: trace/mem.h:32:
+            bool    store      : 1;
                                ^

total: 4 errors, 0 warnings, 227 lines checked

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

Checking PATCH 18/22: instrument: Add event 'guest_mem_before_trans'...
ERROR: spaces prohibited around that ':' (ctx:WxW)
#302: FILE: instrument/qemu-instr/types.h:64:
+            uint8_t size_shift : 2;
                                ^

ERROR: spaces prohibited around that ':' (ctx:VxW)
#303: FILE: instrument/qemu-instr/types.h:65:
+            bool    sign_extend: 1;
                                ^

ERROR: spaces prohibited around that ':' (ctx:WxW)
#304: FILE: instrument/qemu-instr/types.h:66:
+            uint8_t endianness : 1;
                                ^

ERROR: spaces prohibited around that ':' (ctx:WxW)
#305: FILE: instrument/qemu-instr/types.h:67:
+            bool    store      : 1;
                                ^

ERROR: spaces prohibited around that ':' (ctx:WxW)
#430: FILE: trace/control.h:37:
+            uint8_t size_shift : 2;
                                ^

ERROR: spaces prohibited around that ':' (ctx:VxW)
#431: FILE: trace/control.h:38:
+            bool    sign_extend: 1;
                                ^

ERROR: spaces prohibited around that ':' (ctx:WxW)
#432: FILE: trace/control.h:39:
+            uint8_t endianness : 1;
                                ^

ERROR: spaces prohibited around that ':' (ctx:WxW)
#433: FILE: trace/control.h:40:
+            bool    store      : 1;
                                ^

total: 8 errors, 0 warnings, 389 lines checked

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

Checking PATCH 19/22: instrument: Add event 'guest_mem_before_exec'...
ERROR: externs should be avoided in .c files
#339: FILE: stubs/instrument.c:59:
+void helper_instr_guest_mem_before_exec(

total: 1 errors, 0 warnings, 260 lines checked

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

Checking PATCH 20/22: instrument: Add event 'guest_user_syscall'...
Checking PATCH 21/22: instrument: Add event 'guest_user_syscall_ret'...
Checking PATCH 22/22: instrument: Add API to manipulate guest memory...
WARNING: architecture specific defines should be avoided
#41: FILE: instrument/qemu-instr/state.h:13:
+#ifdef __cplusplus

WARNING: architecture specific defines should be avoided
#128: FILE: instrument/qemu-instr/state.h:100:
+#ifdef __cplusplus

total: 0 errors, 2 warnings, 181 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Peter Maydell]

On 13 September 2017 at 10:57, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  MAINTAINERS         |    6 ++
>  docs/instrument.txt |  173 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 179 insertions(+)
>  create mode 100644 docs/instrument.txt
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 36eeb42d19..fb0eaee06a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1486,6 +1486,12 @@ F: scripts/tracetool/
>  F: docs/tracing.txt
>  T: git git://github.com/stefanha/qemu.git tracing
>
> +Event instrumentation
> +M: Lluís Vilanova <vilanova@ac.upc.edu>
> +M: Stefan Hajnoczi <stefanha@redhat.com>
> +S: Maintained
> +F: docs/instrument.txt
> +
>  TPM
>  S: Orphan
>  F: tpm.c
> diff --git a/docs/instrument.txt b/docs/instrument.txt
> new file mode 100644
> index 0000000000..24a0d21fc7
> --- /dev/null
> +++ b/docs/instrument.txt
> @@ -0,0 +1,173 @@
> += Event instrumentation =
> +
> +== Introduction ==
> +
> +Event instrumentation allows users to execute their own host-native code on a
> +set of pre-defined events provided by QEMU. QEMU also exposes other
> +functionality to peek/poke at the guest state (e.g., memory or registers), as
> +well as interacting with tracing events. For those familiar with the term, this
> +provides dynamic binary instrumentation, works on all QEMU-supported
> +architectures, as well as works in both 'user' (standalone application) and
> +'system' (full-system emulation) modes.
> +
> +Look at the headers installed by QEMU on the "qemu-instr" directory for further
> +information beyond this document.
> +
> +
> +== Loading an instrumentation library ==
> +
> +Instrumentation code can be bundled into a dynamic library, which can be later
> +loaded into QEMU:
> +
> +* Using the command-line "-instr" argument.
> +
> +* Using the "instr-load" and "instr-unload" commands in the HMP and QMP
> +  interfaces.
> +
> +
> +== Example ==
> +
> +1. Configure QEMU with event instrumentation:
> +
> +    # instrument guest_cpu_enter and guest_mem_before
> +    mkdir -p /path/to/qemu-build
> +    cd /path/to/qemu-build
> +    /path/to/qemu-source/configure \
> +      --enable-instrument \
> +      --prefix=/path/to/qemu-install

Ideally instrumentation should be cost-free in the case where
we're not using it, so we can default it to enabled.

> +
> +2. Build and install QEMU:
> +
> +    make install
> +
> +3. Create the "Makefile" to build the instrumentation library:
> +
> +    mkdir -p /tmp/my-instrument
> +
> +    cat > /tmp/my-instrument/Makefile <<EOF
> +    QEMU_PATH=/tmp/qemu-install/
> +
> +    CFLAGS += -g
> +    CFLAGS += -O3
> +    CFLAGS += -Werror -Wall
> +    CFLAGS += -I$(QEMU_PATH)/include

Plugins shouldn't have or need access to all of the QEMU source
tree or its include files. We want to be able to provide them
with one header file which defines all they need (and all they
get), under a suitably non-restrictive license like 2-clause-BSD.

> +
> +    all: libtrace-instrument.la
> +
> +    libtrace-instrument.la: instrument.lo
> +            libtool --mode=link --tag=CC $(CC) -module -rpath /usr/local/lib -o $@ $^

-rpath ?

> +
> +    %.lo: %.c
> +            libtool --mode=compile --tag=CC $(CC) $(CFLAGS) -c $^
> +
> +    clean:
> +            $(RM) -f *.o *.so *.lo
> +            $(RM) -Rf .libs
> +    EOF
> +
> +4. Write your instrumentation library:
> +
> +    cat > /tmp/my-instrument/instrument.c <<EOF
> +    #include <stdio.h>
> +    #include <assert.h>
> +
> +    #include <qemu-instr/control.h>         /* manipulate events */
> +    #include <qemu-instr/trace.h>           /* manipulate tracing */
> +
> +    /* the address for the memory access is not known at translation time */
> +    void guest_mem_before_trans(QICPU vcpu_trans, QITCGv_cpu vcpu_exec,
> +                                QITCGv vaddr, QIMemInfo info)
> +    {
> +        printf("%s: %p %p %p %d %d %d %d\n", __func__, vcpu_trans, vcpu_exec, vaddr,
> +               1 << info.size_shift, info.sign_extend, info.endianness, info.store);
> +        if (info.store) {
> +            /* generate at execution time only for memory writes */
> +            qi_event_gen_guest_mem_before_exec(vcpu_exec, vaddr, info);
> +        }
> +    }
> +
> +    /* called when QEMU executes a memory access */
> +    void guest_mem_before_exec(QICPU vcpu, uint64_t vaddr, QIMemInfo info)
> +    {
> +        if (info.store) {
> +            /* if called by TCG code, we'll only get writes (see above) */
> +            printf("%s: %p %lx %d %d %d %d\n", __func__, vcpu, vaddr,
> +                   1 << info.size_shift, info.sign_extend, info.endianness, info.store);
> +        }
> +    }

This looks like it's exposing too much implementation detail.
We should just provide an API for "hook to be called for
memory writes" which gets all the information when it
is called. I don't think we should expose any kind of
"this hook is called at translation time" at all.

I guess the API docs are in doc comments in a header somewhere,
so I'll go look in the other patches.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Lluís Vilanova]

Peter Maydell writes:

> On 13 September 2017 at 10:57, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> MAINTAINERS         |    6 ++
>> docs/instrument.txt |  173 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 179 insertions(+)
>> create mode 100644 docs/instrument.txt
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 36eeb42d19..fb0eaee06a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1486,6 +1486,12 @@ F: scripts/tracetool/
>> F: docs/tracing.txt
>> T: git git://github.com/stefanha/qemu.git tracing
>> 
>> +Event instrumentation
>> +M: Lluís Vilanova <vilanova@ac.upc.edu>
>> +M: Stefan Hajnoczi <stefanha@redhat.com>
>> +S: Maintained
>> +F: docs/instrument.txt
>> +
>> TPM
>> S: Orphan
>> F: tpm.c
>> diff --git a/docs/instrument.txt b/docs/instrument.txt
>> new file mode 100644
>> index 0000000000..24a0d21fc7
>> --- /dev/null
>> +++ b/docs/instrument.txt
>> @@ -0,0 +1,173 @@
>> += Event instrumentation =
>> +
>> +== Introduction ==
>> +
>> +Event instrumentation allows users to execute their own host-native code on a
>> +set of pre-defined events provided by QEMU. QEMU also exposes other
>> +functionality to peek/poke at the guest state (e.g., memory or registers), as
>> +well as interacting with tracing events. For those familiar with the term, this
>> +provides dynamic binary instrumentation, works on all QEMU-supported
>> +architectures, as well as works in both 'user' (standalone application) and
>> +'system' (full-system emulation) modes.
>> +
>> +Look at the headers installed by QEMU on the "qemu-instr" directory for further
>> +information beyond this document.
>> +
>> +
>> +== Loading an instrumentation library ==
>> +
>> +Instrumentation code can be bundled into a dynamic library, which can be later
>> +loaded into QEMU:
>> +
>> +* Using the command-line "-instr" argument.
>> +
>> +* Using the "instr-load" and "instr-unload" commands in the HMP and QMP
>> +  interfaces.
>> +
>> +
>> +== Example ==
>> +
>> +1. Configure QEMU with event instrumentation:
>> +
>> +    # instrument guest_cpu_enter and guest_mem_before
>> +    mkdir -p /path/to/qemu-build
>> +    cd /path/to/qemu-build
>> +    /path/to/qemu-source/configure \
>> +      --enable-instrument \
>> +      --prefix=/path/to/qemu-install

> Ideally instrumentation should be cost-free in the case where
> we're not using it, so we can default it to enabled.

I wasn't sure if everyone would want it enabled by default on the first release,
but I can easily change that.


>> +
>> +2. Build and install QEMU:
>> +
>> +    make install
>> +
>> +3. Create the "Makefile" to build the instrumentation library:
>> +
>> +    mkdir -p /tmp/my-instrument
>> +
>> +    cat > /tmp/my-instrument/Makefile <<EOF
>> +    QEMU_PATH=/tmp/qemu-install/
>> +
>> +    CFLAGS += -g
>> +    CFLAGS += -O3
>> +    CFLAGS += -Werror -Wall
>> +    CFLAGS += -I$(QEMU_PATH)/include

> Plugins shouldn't have or need access to all of the QEMU source
> tree or its include files. We want to be able to provide them
> with one header file which defines all they need (and all they
> get), under a suitably non-restrictive license like 2-clause-BSD.

Variable QEMU_PATH refers to the *installation* path of QEMU.

I can change the API headers to use some other license.


>> +
>> +    all: libtrace-instrument.la
>> +
>> +    libtrace-instrument.la: instrument.lo
>> +            libtool --mode=link --tag=CC $(CC) -module -rpath /usr/local/lib -o $@ $^

> -rpath ?

I couldn't make libtool to generate a .so file without it. I can change the
example to directly use gcc instead of libtool.


>> +
>> +    %.lo: %.c
>> +            libtool --mode=compile --tag=CC $(CC) $(CFLAGS) -c $^
>> +
>> +    clean:
>> +            $(RM) -f *.o *.so *.lo
>> +            $(RM) -Rf .libs
>> +    EOF
>> +
>> +4. Write your instrumentation library:
>> +
>> +    cat > /tmp/my-instrument/instrument.c <<EOF
>> +    #include <stdio.h>
>> +    #include <assert.h>
>> +
>> +    #include <qemu-instr/control.h>         /* manipulate events */
>> +    #include <qemu-instr/trace.h>           /* manipulate tracing */
>> +
>> +    /* the address for the memory access is not known at translation time */
>> +    void guest_mem_before_trans(QICPU vcpu_trans, QITCGv_cpu vcpu_exec,
>> +                                QITCGv vaddr, QIMemInfo info)
>> +    {
>> +        printf("%s: %p %p %p %d %d %d %d\n", __func__, vcpu_trans, vcpu_exec, vaddr,
>> +               1 << info.size_shift, info.sign_extend, info.endianness, info.store);
>> +        if (info.store) {
>> +            /* generate at execution time only for memory writes */
>> +            qi_event_gen_guest_mem_before_exec(vcpu_exec, vaddr, info);
>> +        }
>> +    }
>> +
>> +    /* called when QEMU executes a memory access */
>> +    void guest_mem_before_exec(QICPU vcpu, uint64_t vaddr, QIMemInfo info)
>> +    {
>> +        if (info.store) {
>> +            /* if called by TCG code, we'll only get writes (see above) */
>> +            printf("%s: %p %lx %d %d %d %d\n", __func__, vcpu, vaddr,
>> +                   1 << info.size_shift, info.sign_extend, info.endianness, info.store);
>> +        }
>> +    }

> This looks like it's exposing too much implementation detail.
> We should just provide an API for "hook to be called for
> memory writes" which gets all the information when it
> is called. I don't think we should expose any kind of
> "this hook is called at translation time" at all.

The differentiation between translation-time and execution-time is key to
perform certain analysis efficiently.

The simplest example is probably a BBL trace (e.g., as used by SimPoint
[1]). For each BBL (or, rather, TB) that you translate, you collect what
instructions are in it (addresses, length, opcodes, etc). Then, at execution
time you only need to collect the starting address of each executed BBL/TB,
since its information was already collected before at translation time
(therefore making it more efficient).

[1] https://cseweb.ucsd.edu/~calder/simpoint/


> I guess the API docs are in doc comments in a header somewhere,
> so I'll go look in the other patches.

Yes, you can look at headers in instrument/qemu-instr/.


Thanks,
  Lluis

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Stefan Hajnoczi]

On Wed, Sep 13, 2017 at 12:57:45PM +0300, Lluís Vilanova wrote:
> +    /* mandatory initialization function */
> +    int main(int argc, const char **argv)

Most shared library plugin interfaces I have seen do not use "main()" as
the entry point.  Instead they use a unique name that allows the host
application to differentiate between share library files that are valid
plugins (e.g. "qemu_instr_init") and non-plugin shared libraries.

Stable plugin APIs usually have a versioning or feature detection
scheme.  Versioning is simple: the host application passes a major/minor
version number to the init function.

Of course the dynamic linker already enforces compatibility somewhat: if
a plugin uses a newer API than available in the host application then
there will be a missing symbol/linker error.

So what versioning strategy should we follow?  The simplest would be to
depend 100% on the dynamic linker with no explicit checks inside the
plugin or QEMU.  In that case the API/ABI need to follow some rules like
this (not sure how oudated this information is):
http://plan99.net/~mike/writing-shared-libraries.html

> +    {
> +        int i;
> +        printf("init!\n");
> +        printf("    argc :: %d\n", argc);
> +        for (i = 0; i < argc; i++) {
> +            printf("            -> %s\n", argv[i]);
> +        }
> +    
> +        qi_set_fini(fini, NULL);
> +    
> +        /* instrument and trace events */
> +        QITraceEvent *ev;
> +    
> +        qi_event_set_guest_cpu_enter(guest_cpu_enter);
> +        ev = qi_trace_event_name("guest_cpu_enter");
> +        assert(ev);
> +        qi_trace_event_set_state_dynamic(ev, true);
> +    
> +        qi_event_set_guest_mem_before_trans(guest_mem_before_trans);
> +        ev = qi_trace_event_name("guest_mem_before_trans");
> +        assert(ev);
> +        qi_trace_event_set_state_dynamic(ev, true);
> +    
> +        qi_event_set_guest_mem_before_exec(guest_mem_before_exec);
> +        ev = qi_trace_event_name("guest_mem_before_exec");
> +        assert(ev);
> +        qi_trace_event_set_state_dynamic(ev, true);

Why are trace events being enabled in this example?

I would expect qi_event_set_guest_cpu_enter(guest_cpu_enter) to
immediately enable the callback.  The user shouldn't need to use tracing
to receive callbacks.

qi_event_set_guest_cpu_enter(NULL) should disable the callback.

Stefan

Re: [Qemu-devel] [PATCH v6 03/22] instrument: Add generic library loader

[Stefan Hajnoczi]

On Wed, Sep 13, 2017 at 01:05:49PM +0300, Lluís Vilanova wrote:
> +#include <dlfcn.h>

Is it possible to use glib's portable gmodule API so this works on
Windows too?

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Stefan Hajnoczi]

On Wed, Sep 13, 2017 at 12:57:45PM +0300, Lluís Vilanova wrote:
> +Event instrumentation
> +M: Lluís Vilanova <vilanova@ac.upc.edu>
> +M: Stefan Hajnoczi <stefanha@redhat.com>
> +S: Maintained
> +F: docs/instrument.txt

Thanks for including me but I'm afraid I don't have time to co-maintain
the instrumentation API.

You could maintain it yourself.  Maintainers send signed pull requests
to Peter Maydell to get commits merged into qemu.git/master.

Maybe someone else would like to co-maintain it with you?

Stefan

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Peter Maydell]

On 15 September 2017 at 14:39, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Peter Maydell writes:
>> This looks like it's exposing too much implementation detail.
>> We should just provide an API for "hook to be called for
>> memory writes" which gets all the information when it
>> is called. I don't think we should expose any kind of
>> "this hook is called at translation time" at all.
>
> The differentiation between translation-time and execution-time is key to
> perform certain analysis efficiently.

It's also exposing internal QEMU implementation detail.
What if in future we decide to switch from our current
setup to always interpreting guest instructions as a
first pass with JITting done only in the background for
hot code?

Sticking to instrumentation events that correspond exactly to guest
execution events means they won't break or expose internals.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Lluís Vilanova]

Peter Maydell writes:

> On 15 September 2017 at 14:39, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Peter Maydell writes:
>>> This looks like it's exposing too much implementation detail.
>>> We should just provide an API for "hook to be called for
>>> memory writes" which gets all the information when it
>>> is called. I don't think we should expose any kind of
>>> "this hook is called at translation time" at all.
>> 
>> The differentiation between translation-time and execution-time is key to
>> perform certain analysis efficiently.

> It's also exposing internal QEMU implementation detail.
> What if in future we decide to switch from our current
> setup to always interpreting guest instructions as a
> first pass with JITting done only in the background for
> hot code?

TCI still has a separation of translation-time (translate.c) and execution-time
(interpreting the TCG opcodes), and I don't think that's gonna go away anytime
soon.

Even if it did, I think there still will be a translation/execution separation
easy enough to hook into (even if it's a "fake" one for the cold-path
interpreted instructions).


> Sticking to instrumentation events that correspond exactly to guest
> execution events means they won't break or expose internals.

It also means we won't be able to "conditionally" instrument instructions (e.g.,
based on their opcode, address range, etc.).

Of course we can add the translation/execution differentiation later if we find
it necessary for performance, but I would rather avoid leaving "historical"
instrumentation points behind on the API.

What are the use-cases you're aiming for?


Cheers!
  Lluis

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Peter Maydell]

On 18 September 2017 at 18:09, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Peter Maydell writes:
>> It's also exposing internal QEMU implementation detail.
>> What if in future we decide to switch from our current
>> setup to always interpreting guest instructions as a
>> first pass with JITting done only in the background for
>> hot code?
>
> TCI still has a separation of translation-time (translate.c) and execution-time
> (interpreting the TCG opcodes), and I don't think that's gonna go away anytime
> soon.

I didn't mean TCI, which is nothing like what you'd use for
this if you did it (TCI is slower than just JITting.)

> Even if it did, I think there still will be a translation/execution separation
> easy enough to hook into (even if it's a "fake" one for the cold-path
> interpreted instructions).

But what would it mean? You don't have basic blocks any more.

>> Sticking to instrumentation events that correspond exactly to guest
>> execution events means they won't break or expose internals.
>
> It also means we won't be able to "conditionally" instrument instructions (e.g.,
> based on their opcode, address range, etc.).

You can still do that, it's just less efficient (your
condition-check happens in the callout to the instrumentation
plugin). We can add "filter" options later if we need them
(which I would rather do than have translate-time callbacks).

> Of course we can add the translation/execution differentiation later if we find
> it necessary for performance, but I would rather avoid leaving "historical"
> instrumentation points behind on the API.
>
> What are the use-cases you're aiming for?

* I want to be able to point the small stream of people who come
into qemu-devel asking "how do I trace all my guest's memory
accesses" at a clean API for it.

* I want to be able to have less ugly and confusing tracing
than our current -d output (and perhaps emit tracing in formats
that other analysis tools want as input)

* I want to keep this initial tracing API simple enough that
we can agree on it and get a first working useful version.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Peter Maydell]

On 18 September 2017 at 18:09, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> TCI still has a separation of translation-time (translate.c) and execution-time
> (interpreting the TCG opcodes), and I don't think that's gonna go away anytime
> soon.
>
> Even if it did, I think there still will be a translation/execution separation
> easy enough to hook into (even if it's a "fake" one for the cold-path
> interpreted instructions).

As a slightly more immediate and practical example, I'm currently
implementing the v8M "SG" instruction. This is a somewhat weird
corner-case of an instruction (it's the only instruction you can
execute in non-secure state from a code region that's secure).
I'm implementing it in the exception-handling code path: if we
detect "NS execute from S memory" we throw a QEMU internal exception,
and in the cpu_do_interrupt code we either (a) identify that this
is the SG instruction and execute it or (b) generate the right guest
CPU exception.

That's definitely executing an instruction, and there's no translation
time for it...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Emilio G. Cota]

On Mon, Sep 18, 2017 at 18:42:55 +0100, Peter Maydell wrote:
> On 18 September 2017 at 18:09, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> > It also means we won't be able to "conditionally" instrument instructions (e.g.,
> > based on their opcode, address range, etc.).
> 
> You can still do that, it's just less efficient (your
> condition-check happens in the callout to the instrumentation
> plugin). We can add "filter" options later if we need them
> (which I would rather do than have translate-time callbacks).

My initial reaction to not having translation-time hooks was that it'd
be too slow. However, thinking about it a bit more I suspect it might
not matter much. Other tools such as Pin/DynamoRIO have these hooks,
and in their case it makes sense because one can choose to, instead of
having one callback per executed instruction, get a callback per
executed "trace" (a trace is a chain of TBs). Crucially, these tools do
not go through an intermediate IR, and as a result are about 10X faster
than QEMU.

Therefore, given that QEMU is comparatively slow, we might find that
per-executed-instruction callbacks do not end up slowing things down
significantly.

I like the idea of discussing an API alone, but there is value in having
an implementation to go with it, so that we can explore the perf
trade-offs involved. I was busy with other things the last 10 days or so,
but by the end of this week I hope I'll be able to share some
perf numbers on the per-TB vs. per-instruction callback issue.

Thanks,

		Emilio

Re: [Qemu-devel] [PATCH v6 00/22] instrument: Add basic event instrumentation

[Emilio G. Cota]

On Wed, Sep 13, 2017 at 12:53:43 +0300, Lluís Vilanova wrote:
> The instrumentation code is dynamically loaded as a library into QEMU either
> when it starts or later using its remote control interfaces. The loaded code
> only has access to function explicitly exported through the QI_VPUBLIC macro.
> 
> This series is branch 'devel-instrument' in
> https://code.gso.ac.upc.edu/git/qemu-dbi.

Is this up to date? That tree doesn't build for me with --enable-instrument; I
get the same error I got in v4:

  CC      x86_64-linux-user/instrument/state.o
/data/src/qemu2/instrument/trace.c:13:30: fatal error: qemu-instr/trace.h: No such file or directory
compilation terminated.
/data/src/qemu2/rules.mak:66: recipe for target 'instrument/trace.o' failed

Or maybe I'm doing something wrong?
I'm configuring with `configure --target-list=x86_64-linux-user --enable-instrument'.

Thanks,

		Emilio

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Lluís Vilanova]

First, sorry for the late response; I was away for a few days.


Peter Maydell writes:

> On 18 September 2017 at 18:09, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Peter Maydell writes:
>>> It's also exposing internal QEMU implementation detail.
>>> What if in future we decide to switch from our current
>>> setup to always interpreting guest instructions as a
>>> first pass with JITting done only in the background for
>>> hot code?
>> 
>> TCI still has a separation of translation-time (translate.c) and execution-time
>> (interpreting the TCG opcodes), and I don't think that's gonna go away anytime
>> soon.

> I didn't mean TCI, which is nothing like what you'd use for
> this if you did it (TCI is slower than just JITting.)

My point is that even on the cold path you need to decode a guest instruction
(equivalent to translating) and emulate it on the spot (equivalent to
executing).


>> Even if it did, I think there still will be a translation/execution separation
>> easy enough to hook into (even if it's a "fake" one for the cold-path
>> interpreted instructions).

> But what would it mean? You don't have basic blocks any more.

Every instruction emulated on the spot can be seen as a newly translated block
(of one instruction only), which is executed immediately after.


>>> Sticking to instrumentation events that correspond exactly to guest
>>> execution events means they won't break or expose internals.
>> 
>> It also means we won't be able to "conditionally" instrument instructions (e.g.,
>> based on their opcode, address range, etc.).

> You can still do that, it's just less efficient (your
> condition-check happens in the callout to the instrumentation
> plugin). We can add "filter" options later if we need them
> (which I would rather do than have translate-time callbacks).

Before answering, a short summary of when knowing about translate/execute makes
a difference:

* Record some information only once when an instruction is translated, instead
  of recording it on every executed instruction (e.g., a study of opcode
  distribution, which you can get from a file of per-TB opcodes - generated at
  translation time - and a list of executed TBs - generated at execution time
  -). The translate/execute separation makes this run faster *and* produces much
  smaller files with the recorded info.

  Other typical examples that benefit from this are writing a simulator that
  feeds off a stream of instruction information (a common reason why people want
  to trace memory accesses and information of executed instructions).

* Conditionally instrumenting instructions.

Adding filtering to the instrumentation API would only solve the second point,
but not the first one.

Now, do we need/want to support the first point?


>> Of course we can add the translation/execution differentiation later if we find
>> it necessary for performance, but I would rather avoid leaving "historical"
>> instrumentation points behind on the API.
>> 
>> What are the use-cases you're aiming for?

> * I want to be able to point the small stream of people who come
> into qemu-devel asking "how do I trace all my guest's memory
> accesses" at a clean API for it.

> * I want to be able to have less ugly and confusing tracing
> than our current -d output (and perhaps emit tracing in formats
> that other analysis tools want as input)

> * I want to keep this initial tracing API simple enough that
> we can agree on it and get a first working useful version.

Fair enough.

I know it's not exactly the same we're discussing, but the plot in [1] compares
a few different ways to trace memory accesses on SPEC benchmarks:

* First bar is using a Intel's tool called PIN [2].
* Second is calling into an instrumentation function on every executed memory
  access in QEMU.
* Third is embedding the hot path of writing the memory access info to an array
  into the TCG opcode stream (more or less equivalent to supporting filtering;
  when the array is full, a user's callback is called - cold path -)
* Fourth bar can be ignored.

This was working on a much older version of instrumentation for QEMU, but I can
implement something that does the first use-case point above and some filtering
example (second use-case point) to see what's the performance difference.

[1] https://filetea.me/n3wy9WwyCCZR72E9OWXHArHDw
[2] https://software.intel.com/en-us/articles/pin-a-dynamic-binary-instrumentation-tool


Thanks!
  Lluis

Re: [Qemu-devel] [PATCH v6 00/22] instrument: Add basic event instrumentation

[Lluís Vilanova]

Emilio G Cota writes:

> On Wed, Sep 13, 2017 at 12:53:43 +0300, Lluís Vilanova wrote:
>> The instrumentation code is dynamically loaded as a library into QEMU either
>> when it starts or later using its remote control interfaces. The loaded code
>> only has access to function explicitly exported through the QI_VPUBLIC macro.
>> 
>> This series is branch 'devel-instrument' in
>> https://code.gso.ac.upc.edu/git/qemu-dbi.

> Is this up to date? That tree doesn't build for me with --enable-instrument; I
> get the same error I got in v4:

>   CC      x86_64-linux-user/instrument/state.o
> /data/src/qemu2/instrument/trace.c:13:30: fatal error: qemu-instr/trace.h: No such file or directory
> compilation terminated.
> /data/src/qemu2/rules.mak:66: recipe for target 'instrument/trace.o' failed

> Or maybe I'm doing something wrong?
> I'm configuring with `configure --target-list=x86_64-linux-user --enable-instrument'.

You're doing it right, and I've checked that the branch is properly pushed. Can
you compile with V=1 to show me the failing cmdline?

Thanks,
  Lluis

Re: [Qemu-devel] [PATCH v6 00/22] instrument: Add basic event instrumentation

[Emilio G. Cota]

On Mon, Sep 25, 2017 at 21:07:45 +0300, Lluís Vilanova wrote:
> You're doing it right, and I've checked that the branch is properly pushed. Can
> you compile with V=1 to show me the failing cmdline?

$ make V=1
(cd /data/src/qemu2; printf '#define QEMU_PKGVERSION '; if test -n ""; then printf '""\n'; else if test -d .git; then printf '" ('; git describe --match 'v*' 2>/dev/null | tr -d '\n'; if ! git diff-index --quiet HEAD &>/dev/null; then printf -- '-dirty'; fi; printf ')"\n'; else printf '""\n'; fi; fi) > qemu-version.h.tmp
if ! cmp -s qemu-version.h qemu-version.h.tmp; then mv qemu-version.h.tmp qemu-version.h; else rm qemu-version.h.tmp; fi
make -I/data/src/qemu2/dtc VPATH=/data/src/qemu2/dtc -C dtc V="1" LIBFDT_srcdir=/data/src/qemu2/dtc/libfdt CPPFLAGS="-I/data/src/qemu2/build/dtc -I/data/src/qemu2/dtc -I/data/src/qemu2/dtc/libfdt" CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g  -fvisibility=hidden -I/usr/include/pixman-1 -I/data/src/qemu2/dtc/libfdt -Werror -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-strong -I/usr/include/libpng12 -I/data/src/qemu2/tests" LDFLAGS="-rdynamic -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g " ARFLAGS="rv" CC="cc" AR="ar" LD="ld"  BUILD_DIR=/data/src/qemu2/build libfdt/libfdt.a
make[1]: Entering directory '/data/src/qemu2/build/dtc'
make[1]: 'libfdt/libfdt.a' is up to date.
make[1]: Leaving directory '/data/src/qemu2/build/dtc'
make  BUILD_DIR=/data/src/qemu2/build -C x86_64-linux-user V="1" TARGET_DIR="x86_64-linux-user/" all
make[1]: Entering directory '/data/src/qemu2/build/x86_64-linux-user'
cc -I/data/src/qemu2/build/. -I. -I/data/src/qemu2/tcg -I/data/src/qemu2/tcg/i386 -I/data/src/qemu2/instrument -I/data/src/qemu2/linux-headers -I/data/src/qemu2/build/linux-headers -I. -I/data/src/qemu2 -I/data/src/qemu2/accel/tcg -I/data/src/qemu2/include -fvisibility=hidden -I/usr/include/pixman-1 -I/data/src/qemu2/dtc/libfdt -Werror -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-strong -I/usr/include/libpng12  -I../linux-headers -I.. -I/data/src/qemu2/target/i386 -DNEED_CPU_H -I/data/src/qemu2/include -I/data/src/qemu2/linux-user/x86_64 -I/data/src/qemu2/linux-user/host/x86_64 -I/data/src/qemu2/linux-user -MMD -MP -MT gdbstub-xml.o -MF ./gdbstub-xml.d -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g  -DQEMU_TARGET_BUILD=1  -c -o gdbstub-xml.o gdbstub-xml.c
cc -I/data/src/qemu2/build/instrument -Iinstrument -I/data/src/qemu2/tcg -I/data/src/qemu2/tcg/i386 -I/data/src/qemu2/instrument -I/data/src/qemu2/linux-headers -I/data/src/qemu2/build/linux-headers -I. -I/data/src/qemu2 -I/data/src/qemu2/accel/tcg -I/data/src/qemu2/include -fvisibility=hidden -I/usr/include/pixman-1 -I/data/src/qemu2/dtc/libfdt -Werror -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-strong -I/usr/include/libpng12  -I../linux-headers -I.. -I/data/src/qemu2/target/i386 -DNEED_CPU_H -I/data/src/qemu2/include -I/data/src/qemu2/linux-user/x86_64 -I/data/src/qemu2/linux-user/host/x86_64 -I/data/src/qemu2/linux-user -MMD -MP -MT instrument/trace.o -MF instrument/trace.d -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g  -DQEMU_TARGET_BUILD=1  -c -o instrument/trace.o /data/src/qemu2/instrument/trace.c
/data/src/qemu2/instrument/trace.c:13:30: fatal error: qemu-instr/trace.h: No such file or directory
compilation terminated.
/data/src/qemu2/rules.mak:66: recipe for target 'instrument/trace.o' failed
make[1]: *** [instrument/trace.o] Error 1
make[1]: Leaving directory '/data/src/qemu2/build/x86_64-linux-user'
Makefile:326: recipe for target 'subdir-x86_64-linux-user' failed
make: *** [subdir-x86_64-linux-user] Error 2

Thanks,

		Emilio

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Emilio G. Cota]

On Mon, Sep 25, 2017 at 21:03:39 +0300, Lluís Vilanova wrote:
> I know it's not exactly the same we're discussing, but the plot in [1] compares
> a few different ways to trace memory accesses on SPEC benchmarks:
> 
> * First bar is using a Intel's tool called PIN [2].
> * Second is calling into an instrumentation function on every executed memory
>   access in QEMU.
> * Third is embedding the hot path of writing the memory access info to an array
>   into the TCG opcode stream (more or less equivalent to supporting filtering;
>   when the array is full, a user's callback is called - cold path -)
> * Fourth bar can be ignored.
> 
> This was working on a much older version of instrumentation for QEMU, but I can
> implement something that does the first use-case point above and some filtering
> example (second use-case point) to see what's the performance difference.
> 
> [1] https://filetea.me/n3wy9WwyCCZR72E9OWXHArHDw

Interesting! Unfortunately, this URL gives me a 404.

		E.

Re: [Qemu-devel] [PATCH v6 00/22] instrument: Add basic event instrumentation

[Lluís Vilanova]

Emilio G Cota writes:

> On Mon, Sep 25, 2017 at 21:07:45 +0300, Lluís Vilanova wrote:
>> You're doing it right, and I've checked that the branch is properly pushed. Can
>> you compile with V=1 to show me the failing cmdline?

> $ make V=1
> (cd /data/src/qemu2; printf '#define QEMU_PKGVERSION '; if test -n ""; then
> printf '""\n'; else if test -d .git; then printf '" ('; git describe --match
> 'v*' 2>/dev/null | tr -d '\n'; if ! git diff-index --quiet HEAD &>/dev/null;
> then printf -- '-dirty'; fi; printf ')"\n'; else printf '""\n'; fi; fi) >
> qemu-version.h.tmp
> if ! cmp -s qemu-version.h qemu-version.h.tmp; then mv qemu-version.h.tmp qemu-version.h; else rm qemu-version.h.tmp; fi
> make -I/data/src/qemu2/dtc VPATH=/data/src/qemu2/dtc -C dtc V="1"
> LIBFDT_srcdir=/data/src/qemu2/dtc/libfdt CPPFLAGS="-I/data/src/qemu2/build/dtc
> -I/data/src/qemu2/dtc -I/data/src/qemu2/dtc/libfdt" CFLAGS="-O2
> -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g -fvisibility=hidden
> -I/usr/include/pixman-1 -I/data/src/qemu2/dtc/libfdt -Werror -pthread
> -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fPIE -DPIE
> -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings
> -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels
> -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security
> -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration
> -Wold-style-definition -Wtype-limits -fstack-protector-strong
> -I/usr/include/libpng12 -I/data/src/qemu2/tests" LDFLAGS="-rdynamic
> -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g " ARFLAGS="rv" CC="cc"
> AR="ar" LD="ld" BUILD_DIR=/data/src/qemu2/build libfdt/libfdt.a
> make[1]: Entering directory '/data/src/qemu2/build/dtc'
> make[1]: 'libfdt/libfdt.a' is up to date.
> make[1]: Leaving directory '/data/src/qemu2/build/dtc'
> make  BUILD_DIR=/data/src/qemu2/build -C x86_64-linux-user V="1" TARGET_DIR="x86_64-linux-user/" all
> make[1]: Entering directory '/data/src/qemu2/build/x86_64-linux-user'
> cc -I/data/src/qemu2/build/. -I. -I/data/src/qemu2/tcg
> -I/data/src/qemu2/tcg/i386 -I/data/src/qemu2/instrument
> -I/data/src/qemu2/linux-headers -I/data/src/qemu2/build/linux-headers
> -I. -I/data/src/qemu2 -I/data/src/qemu2/accel/tcg -I/data/src/qemu2/include
> -fvisibility=hidden -I/usr/include/pixman-1 -I/data/src/qemu2/dtc/libfdt -Werror
> -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include
> -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings
> -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels
> -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security
> -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration
> -Wold-style-definition -Wtype-limits -fstack-protector-strong
> -I/usr/include/libpng12 -I../linux-headers -I.. -I/data/src/qemu2/target/i386
> -DNEED_CPU_H -I/data/src/qemu2/include -I/data/src/qemu2/linux-user/x86_64
> -I/data/src/qemu2/linux-user/host/x86_64 -I/data/src/qemu2/linux-user -MMD -MP
> -MT gdbstub-xml.o -MF ./gdbstub-xml.d -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
> -g -DQEMU_TARGET_BUILD=1 -c -o gdbstub-xml.o gdbstub-xml.c
> cc -I/data/src/qemu2/build/instrument -Iinstrument -I/data/src/qemu2/tcg
> -I/data/src/qemu2/tcg/i386 -I/data/src/qemu2/instrument
> -I/data/src/qemu2/linux-headers -I/data/src/qemu2/build/linux-headers
> -I. -I/data/src/qemu2 -I/data/src/qemu2/accel/tcg -I/data/src/qemu2/include
> -fvisibility=hidden -I/usr/include/pixman-1 -I/data/src/qemu2/dtc/libfdt -Werror
> -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include
> -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings
> -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels
> -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security
> -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration
> -Wold-style-definition -Wtype-limits -fstack-protector-strong
> -I/usr/include/libpng12 -I../linux-headers -I.. -I/data/src/qemu2/target/i386
> -DNEED_CPU_H -I/data/src/qemu2/include -I/data/src/qemu2/linux-user/x86_64
> -I/data/src/qemu2/linux-user/host/x86_64 -I/data/src/qemu2/linux-user -MMD -MP
> -MT instrument/trace.o -MF instrument/trace.d -O2 -U_FORTIFY_SOURCE
> -D_FORTIFY_SOURCE=2 -g -DQEMU_TARGET_BUILD=1 -c -o instrument/trace.o
> /data/src/qemu2/instrument/trace.c
> /data/src/qemu2/instrument/trace.c:13:30: fatal error: qemu-instr/trace.h: No such file or directory
> compilation terminated.
> /data/src/qemu2/rules.mak:66: recipe for target 'instrument/trace.o' failed
> make[1]: *** [instrument/trace.o] Error 1
> make[1]: Leaving directory '/data/src/qemu2/build/x86_64-linux-user'
> Makefile:326: recipe for target 'subdir-x86_64-linux-user' failed
> make: *** [subdir-x86_64-linux-user] Error 2

The includes look fine to me... (there's "-I/data/src/qemu2/instrument"). Can
you paste the result of "find /data/src/qemu2/instrument"? You should have a
"/data/src/qemu2/instrument/qemu-instr/trace.h" file there.

In any case, I was planning to drop the tracing API for the next series.


Thanks,
  Lluis

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Lluís Vilanova]

Emilio G Cota writes:

> On Mon, Sep 25, 2017 at 21:03:39 +0300, Lluís Vilanova wrote:
>> I know it's not exactly the same we're discussing, but the plot in [1] compares
>> a few different ways to trace memory accesses on SPEC benchmarks:
>> 
>> * First bar is using a Intel's tool called PIN [2].
>> * Second is calling into an instrumentation function on every executed memory
>> access in QEMU.
>> * Third is embedding the hot path of writing the memory access info to an array
>> into the TCG opcode stream (more or less equivalent to supporting filtering;
>> when the array is full, a user's callback is called - cold path -)
>> * Fourth bar can be ignored.
>> 
>> This was working on a much older version of instrumentation for QEMU, but I can
>> implement something that does the first use-case point above and some filtering
>> example (second use-case point) to see what's the performance difference.
>> 
>> [1] https://filetea.me/n3wy9WwyCCZR72E9OWXHArHDw

> Interesting! Unfortunately, this URL gives me a 404.

Ok, I've uploade it somewhere else:

  https://people.gso.ac.upc.edu/vilanova/mtrace.pdf

There's also another one that simply counts the number of memory accesses, using
the same three approaches:

  https://people.gso.ac.upc.edu/vilanova/mcount.pdf

Cheers,
  Lluis

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Lluís Vilanova]

Lluís Vilanova writes:
[...]
> This was working on a much older version of instrumentation for QEMU, but I can
> implement something that does the first use-case point above and some filtering
> example (second use-case point) to see what's the performance difference.

Ok, so here's some numbers for the discussion (booting Emilio's ARM full system
image that immediately shuts down):

* Without instrumentation

  real	0m10,099s
  user	0m9,876s
  sys	0m0,128s

* Count number of memory access writes, by instrumenting only when they are
  executed

  real	0m15,896s
  user	0m15,752s
  sys	0m0,108s

* Counting same, but the filtering is done at translation time (i.e., not
  generate an execute-time callback if it's not a write)

  real	0m11,084s
  user	0m10,880s
  sys	0m0,112s

  As Peter said, the filtering can be added into the API to take advantage of
  this "speedup", without exposing translation vs execution time callbacks.

* Counting number of executed instructions, by instrumenting the beginning of
  each one of them

  real	0m24,583s
  user	0m24,352s
  sys	0m0,184s

* Counting same, but per-TB numbers are collected at translation-time, and we
  only generate a per-TB execution time callback to add the corresponding number
  of instructions for that TB

  real	0m11,151s
  user	0m10,952s
  sys	0m0,092s

  This really needs to expose translation vs execution time callbacks to take
  advantage of this "speedup".

Cheers,
  Lluis

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Emilio G. Cota]

On Fri, Sep 29, 2017 at 16:16:41 +0300, Lluís Vilanova wrote:
> Lluís Vilanova writes:
> [...]
> > This was working on a much older version of instrumentation for QEMU, but I can
> > implement something that does the first use-case point above and some filtering
> > example (second use-case point) to see what's the performance difference.
> 
> Ok, so here's some numbers for the discussion (booting Emilio's ARM full system
> image that immediately shuts down):
> 
> * Without instrumentation
> 
>   real	0m10,099s
>   user	0m9,876s
>   sys	0m0,128s
> 
> * Count number of memory access writes, by instrumenting only when they are
>   executed
> 
>   real	0m15,896s
>   user	0m15,752s
>   sys	0m0,108s
> 
> * Counting same, but the filtering is done at translation time (i.e., not
>   generate an execute-time callback if it's not a write)
> 
>   real	0m11,084s
>   user	0m10,880s
>   sys	0m0,112s
> 
>   As Peter said, the filtering can be added into the API to take advantage of
>   this "speedup", without exposing translation vs execution time callbacks.

I'm not sure I understand this concept of filtering. Are you saying that in
the first case, all memory accesses are instrumented, and then in the
"access helper" we only call the user's callback if it's a memory write?
And in the second case, we simply just generate a "write helper" instead
of an "access helper". Am I understanding this correctly?

> * Counting number of executed instructions, by instrumenting the beginning of
>   each one of them
> 
>   real	0m24,583s
>   user	0m24,352s
>   sys	0m0,184s
> 
> * Counting same, but per-TB numbers are collected at translation-time, and we
>   only generate a per-TB execution time callback to add the corresponding number
>   of instructions for that TB
> 
>   real	0m11,151s
>   user	0m10,952s
>   sys	0m0,092s
> 
>   This really needs to expose translation vs execution time callbacks to take
>   advantage of this "speedup".

Clearly instrumenting per-TB is a significant net gain. I think we should
definitely allow instrumenters to use this option.

FWIW my experiments so far show similar numbers for instrumenting each
instruction (haven't done the per-tb yet). The difference is that I'm
exposing to instrumenters a copy of the guest instructions (const void *data,
size_t size). These copies are kept around until TB's are flushed.
Luckily there seems to be very little overhead in keeping these around,
apart from the memory overhead -- but in terms of performance, the
necessary allocations do not induce significant overhead.

Thanks,

		Emilio

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Lluís Vilanova]

Emilio G Cota writes:

> On Fri, Sep 29, 2017 at 16:16:41 +0300, Lluís Vilanova wrote:
>> Lluís Vilanova writes:
>> [...]
>> > This was working on a much older version of instrumentation for QEMU, but I can
>> > implement something that does the first use-case point above and some filtering
>> > example (second use-case point) to see what's the performance difference.
>> 
>> Ok, so here's some numbers for the discussion (booting Emilio's ARM full system
>> image that immediately shuts down):
>> 
>> * Without instrumentation
>> 
>> real	0m10,099s
>> user	0m9,876s
>> sys	0m0,128s
>> 
>> * Count number of memory access writes, by instrumenting only when they are
>> executed
>> 
>> real	0m15,896s
>> user	0m15,752s
>> sys	0m0,108s
>> 
>> * Counting same, but the filtering is done at translation time (i.e., not
>> generate an execute-time callback if it's not a write)
>> 
>> real	0m11,084s
>> user	0m10,880s
>> sys	0m0,112s
>> 
>> As Peter said, the filtering can be added into the API to take advantage of
>> this "speedup", without exposing translation vs execution time callbacks.

> I'm not sure I understand this concept of filtering. Are you saying that in
> the first case, all memory accesses are instrumented, and then in the
> "access helper" we only call the user's callback if it's a memory write?
> And in the second case, we simply just generate a "write helper" instead
> of an "access helper". Am I understanding this correctly?

In the previous case (no filtering), the user callback is always called when a
memory access is *executed*, and the user then checks if the access mode is a
write to decide whether to increment a counter.

In this case (with filtering), a user callback is called when a memory access is
*translated*, and if the access mode is a write, the user generates a call to a
second callback that is executed every time a memory access is executed (only
that it is only generated for memory writes, the ones we care about).

Is this clearer?


>> * Counting number of executed instructions, by instrumenting the beginning of
>> each one of them
>> 
>> real	0m24,583s
>> user	0m24,352s
>> sys	0m0,184s
>> 
>> * Counting same, but per-TB numbers are collected at translation-time, and we
>> only generate a per-TB execution time callback to add the corresponding number
>> of instructions for that TB
>> 
>> real	0m11,151s
>> user	0m10,952s
>> sys	0m0,092s
>> 
>> This really needs to expose translation vs execution time callbacks to take
>> advantage of this "speedup".

> Clearly instrumenting per-TB is a significant net gain. I think we should
> definitely allow instrumenters to use this option.

> FWIW my experiments so far show similar numbers for instrumenting each
> instruction (haven't done the per-tb yet). The difference is that I'm
> exposing to instrumenters a copy of the guest instructions (const void *data,
> size_t size). These copies are kept around until TB's are flushed.
> Luckily there seems to be very little overhead in keeping these around,
> apart from the memory overhead -- but in terms of performance, the
> necessary allocations do not induce significant overhead.

To keep this use-case simpler, I added the memory access API I posted in this
series, where instrumenters can read guest memory (more general than passing a
copy of the current instruction).


Cheers,
  Lluis

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Emilio G. Cota]

On Sat, Sep 30, 2017 at 00:46:33 +0300, Lluís Vilanova wrote:
> Emilio G Cota writes:
> > I'm not sure I understand this concept of filtering. Are you saying that in
> > the first case, all memory accesses are instrumented, and then in the
> > "access helper" we only call the user's callback if it's a memory write?
> > And in the second case, we simply just generate a "write helper" instead
> > of an "access helper". Am I understanding this correctly?
> 
> In the previous case (no filtering), the user callback is always called when a
> memory access is *executed*, and the user then checks if the access mode is a
> write to decide whether to increment a counter.
> 
> In this case (with filtering), a user callback is called when a memory access is
> *translated*, and if the access mode is a write, the user generates a call to a
> second callback that is executed every time a memory access is executed (only
> that it is only generated for memory writes, the ones we care about).
> 
> Is this clearer?

I get it now, thanks!

> > FWIW my experiments so far show similar numbers for instrumenting each
> > instruction (haven't done the per-tb yet). The difference is that I'm
> > exposing to instrumenters a copy of the guest instructions (const void *data,
> > size_t size). These copies are kept around until TB's are flushed.
> > Luckily there seems to be very little overhead in keeping these around,
> > apart from the memory overhead -- but in terms of performance, the
> > necessary allocations do not induce significant overhead.
> 
> To keep this use-case simpler, I added the memory access API I posted in this
> series, where instrumenters can read guest memory (more general than passing a
> copy of the current instruction).

I see some potential problems with this:
1. Instrumenters' accesses could generate exceptions. I presume we'd want to avoid
   this, or leave it as a debug-only kind of option.
2. Instrumenters won't know where the end of an instruction (for variable-length
  ISAs) or of a TB is (TB != basic block). For instructions one could have a loop
  where we read byte-by-byte and pass it to the decoder, something similar to
  what we have in the capstone code recently posted to the list (v4). For TBs,
  we really should have a way to delimit the length of the TB. This is further
  complicated if we want instrumentation to be inserted *before* a TB is
  translated.

Some thoughts on the latter problem: if we want a tb_trans_pre callback, like
Pin/DynamoRIO provide, instead of doing two passes (one to delimit the TB and
call the tb_trans_pre callback, to then generate the translated TB), we could:
  - have a tb_trans_pre callback. This callback inserts an exec-time callback
    with a user-defined pointer (let's call it **tb_info). The callback has
    no arguments, perhaps just the pc.
  - have a tb_trans_post callback. This one passes a copy of the guest
    instructions. The instrumenter then can allocate whatever data structure
    to represent the TB (*tb_info), and copies this pointer to **tb_info, so
    that at execution time, we can obtain tb_info _before_ the TB is executed.
    After the callback returns, the copy of the guest instructions can be freed.
  This has two disadvantages:
  - We have an extra dereference to find tb_info
  - If it turns out that the TB should not be instrumented, we have generated
    a callback for nothing.

		Emilio

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Lluís Vilanova]

Emilio G Cota writes:

> On Sat, Sep 30, 2017 at 00:46:33 +0300, Lluís Vilanova wrote:
>> Emilio G Cota writes:
>> > I'm not sure I understand this concept of filtering. Are you saying that in
>> > the first case, all memory accesses are instrumented, and then in the
>> > "access helper" we only call the user's callback if it's a memory write?
>> > And in the second case, we simply just generate a "write helper" instead
>> > of an "access helper". Am I understanding this correctly?
>> 
>> In the previous case (no filtering), the user callback is always called when a
>> memory access is *executed*, and the user then checks if the access mode is a
>> write to decide whether to increment a counter.
>> 
>> In this case (with filtering), a user callback is called when a memory access is
>> *translated*, and if the access mode is a write, the user generates a call to a
>> second callback that is executed every time a memory access is executed (only
>> that it is only generated for memory writes, the ones we care about).
>> 
>> Is this clearer?

> I get it now, thanks!

>> > FWIW my experiments so far show similar numbers for instrumenting each
>> > instruction (haven't done the per-tb yet). The difference is that I'm
>> > exposing to instrumenters a copy of the guest instructions (const void *data,
>> > size_t size). These copies are kept around until TB's are flushed.
>> > Luckily there seems to be very little overhead in keeping these around,
>> > apart from the memory overhead -- but in terms of performance, the
>> > necessary allocations do not induce significant overhead.
>> 
>> To keep this use-case simpler, I added the memory access API I posted in this
>> series, where instrumenters can read guest memory (more general than passing a
>> copy of the current instruction).

> I see some potential problems with this:
> 1. Instrumenters' accesses could generate exceptions. I presume we'd want to avoid
>    this, or leave it as a debug-only kind of option.

The API takes care of telling you if the access could be performed
successfully. If you access the instruction's memory representation at
translation time, it should be able to perform the access, since QEMU's
translation loop just had to do so in order to access that instruction (I should
check what happens in the corner case where another guest CPU changes the page
table, since I'm not sure if the address translation functions I'm using in QEMU
will use the per-vCPU TLB cache or always traverse the page table).


> 2. Instrumenters won't know where the end of an instruction (for variable-length
>   ISAs) or of a TB is (TB != basic block). For instructions one could have a loop
>   where we read byte-by-byte and pass it to the decoder, something similar to
>   what we have in the capstone code recently posted to the list (v4). For TBs,
>   we really should have a way to delimit the length of the TB. This is further
>   complicated if we want instrumentation to be inserted *before* a TB is
>   translated.

> Some thoughts on the latter problem: if we want a tb_trans_pre callback, like
> Pin/DynamoRIO provide, instead of doing two passes (one to delimit the TB and
> call the tb_trans_pre callback, to then generate the translated TB), we could:
>   - have a tb_trans_pre callback. This callback inserts an exec-time callback
>     with a user-defined pointer (let's call it **tb_info). The callback has
>     no arguments, perhaps just the pc.
>   - have a tb_trans_post callback. This one passes a copy of the guest
>     instructions. The instrumenter then can allocate whatever data structure
>     to represent the TB (*tb_info), and copies this pointer to **tb_info, so
>     that at execution time, we can obtain tb_info _before_ the TB is executed.
>     After the callback returns, the copy of the guest instructions can be freed.
>   This has two disadvantages:
>   - We have an extra dereference to find tb_info
>   - If it turns out that the TB should not be instrumented, we have generated
>     a callback for nothing.

That's precisely one of the reasons why I proposed adding instrumentation points
before and after events happen (e.g., instrument right after translating an
instruction, where you know its size).

What you propose is actually a broader issue, how to allow instrumentors to pass
their own data to execution-time functions "after the fact". For this, I
implemented "promises", a kind of generalization of what gen_icount() does (you
pass a value to the execution-time callback that is computed later during
translation-time).


Cheers,
  Lluis

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Emilio G. Cota]

On Thu, Oct 05, 2017 at 02:28:12 +0300, Lluís Vilanova wrote:
> Emilio G Cota writes:
> > I see some potential problems with this:
> > 1. Instrumenters' accesses could generate exceptions. I presume we'd want to avoid
> >    this, or leave it as a debug-only kind of option.
> 
> The API takes care of telling you if the access could be performed
> successfully. If you access the instruction's memory representation at
> translation time, it should be able to perform the access, since QEMU's
> translation loop just had to do so in order to access that instruction (I should
> check what happens in the corner case where another guest CPU changes the page
> table, since I'm not sure if the address translation functions I'm using in QEMU
> will use the per-vCPU TLB cache or always traverse the page table).

That was my concern, I'd rather just perform the read once, that is, the read(s)
done by ops->insn_translate.

> > 2. Instrumenters won't know where the end of an instruction (for variable-length
> >   ISAs) or of a TB is (TB != basic block). For instructions one could have a loop
> >   where we read byte-by-byte and pass it to the decoder, something similar to
> >   what we have in the capstone code recently posted to the list (v4). For TBs,
> >   we really should have a way to delimit the length of the TB. This is further
> >   complicated if we want instrumentation to be inserted *before* a TB is
> >   translated.
> 
> > Some thoughts on the latter problem: if we want a tb_trans_pre callback, like
> > Pin/DynamoRIO provide, instead of doing two passes (one to delimit the TB and
> > call the tb_trans_pre callback, to then generate the translated TB), we could:
> >   - have a tb_trans_pre callback. This callback inserts an exec-time callback
> >     with a user-defined pointer (let's call it **tb_info). The callback has
> >     no arguments, perhaps just the pc.
> >   - have a tb_trans_post callback. This one passes a copy of the guest
> >     instructions. The instrumenter then can allocate whatever data structure
> >     to represent the TB (*tb_info), and copies this pointer to **tb_info, so
> >     that at execution time, we can obtain tb_info _before_ the TB is executed.
> >     After the callback returns, the copy of the guest instructions can be freed.
> >   This has two disadvantages:
> >   - We have an extra dereference to find tb_info
> >   - If it turns out that the TB should not be instrumented, we have generated
> >     a callback for nothing.
> 
> That's precisely one of the reasons why I proposed adding instrumentation points
> before and after events happen (e.g., instrument right after translating an
> instruction, where you know its size).
> 
> What you propose is actually a broader issue, how to allow instrumentors to pass
> their own data to execution-time functions "after the fact". For this, I
> implemented "promises", a kind of generalization of what gen_icount() does (you
> pass a value to the execution-time callback that is computed later during
> translation-time).

I see. I implemented what I suggested above, i.e. tb_trans_cb
(i.e. post_trans) passes an opaque descriptor of the TB (which can
be iterated over insn by insn) and the return value (void *) of this
cb will be passed by tb_exec_cb (i.e. pre_exec).  Perf-wise this
is pretty OK, turns out even if we don't end up caring about the
TB, the additional per-TB helper (which might not end up calling
a callback) does not introduce significant overhead at execution time.

The major hurdle I found is what to do when removing a plugin,
so that we avoid flushing potentially all translated code. What I ended up
doing is adding a per-TB list of "plugin_tb" descriptors, which
track these user pointers so that (1) each plugin gets the right
pointer, and (2) if n_plugins > 1, we still have a single helper
that dispatches the callbacks instead of n_plugin helpers.

If I understand correctly, with promises we directly generate
a callback, which has the promise(s) as one (or more) of its
arguments. This is neat and very flexible. However, it forces us to
retranslate the TB when the plugin is removed (if we're lazy we could
flush all TBs), and if we have several plugins, we end up with one
helper per callback, instead of a single one.
None of this is a huge deal though, I just think is worth considering.

Also, I'm not sure Peter and others would be happy with allowing
plugin code to generate arbitrary callbacks (IIRC arbitrary code
has already been ruled out). So perhaps a more restrictive option
like what I suggested above would be more palatable.

Cheers,

		Emilio

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Lluís Vilanova]

Emilio G Cota writes:

> On Thu, Oct 05, 2017 at 02:28:12 +0300, Lluís Vilanova wrote:
>> Emilio G Cota writes:
>> > I see some potential problems with this:
>> > 1. Instrumenters' accesses could generate exceptions. I presume we'd want to avoid
>> >    this, or leave it as a debug-only kind of option.
>> 
>> The API takes care of telling you if the access could be performed
>> successfully. If you access the instruction's memory representation at
>> translation time, it should be able to perform the access, since QEMU's
>> translation loop just had to do so in order to access that instruction (I should
>> check what happens in the corner case where another guest CPU changes the page
>> table, since I'm not sure if the address translation functions I'm using in QEMU
>> will use the per-vCPU TLB cache or always traverse the page table).

> That was my concern, I'd rather just perform the read once, that is, the read(s)
> done by ops->insn_translate.

If your concern is on performance, that should not be an issue, since you'd be
using the memory peek functions at translation-time. Furthermore, since others
suggested having memory peek anyway, that's a nicer way (to me) to compose APIs
(and is less complex to implement).


>> > 2. Instrumenters won't know where the end of an instruction (for variable-length
>> >   ISAs) or of a TB is (TB != basic block). For instructions one could have a loop
>> >   where we read byte-by-byte and pass it to the decoder, something similar to
>> >   what we have in the capstone code recently posted to the list (v4). For TBs,
>> >   we really should have a way to delimit the length of the TB. This is further
>> >   complicated if we want instrumentation to be inserted *before* a TB is
>> >   translated.
>> 
>> > Some thoughts on the latter problem: if we want a tb_trans_pre callback, like
>> > Pin/DynamoRIO provide, instead of doing two passes (one to delimit the TB and
>> > call the tb_trans_pre callback, to then generate the translated TB), we could:
>> >   - have a tb_trans_pre callback. This callback inserts an exec-time callback
>> >     with a user-defined pointer (let's call it **tb_info). The callback has
>> >     no arguments, perhaps just the pc.
>> >   - have a tb_trans_post callback. This one passes a copy of the guest
>> >     instructions. The instrumenter then can allocate whatever data structure
>> >     to represent the TB (*tb_info), and copies this pointer to **tb_info, so
>> >     that at execution time, we can obtain tb_info _before_ the TB is executed.
>> >     After the callback returns, the copy of the guest instructions can be freed.
>> >   This has two disadvantages:
>> >   - We have an extra dereference to find tb_info
>> >   - If it turns out that the TB should not be instrumented, we have generated
>> >     a callback for nothing.
>> 
>> That's precisely one of the reasons why I proposed adding instrumentation points
>> before and after events happen (e.g., instrument right after translating an
>> instruction, where you know its size).
>> 
>> What you propose is actually a broader issue, how to allow instrumentors to pass
>> their own data to execution-time functions "after the fact". For this, I
>> implemented "promises", a kind of generalization of what gen_icount() does (you
>> pass a value to the execution-time callback that is computed later during
>> translation-time).

> I see. I implemented what I suggested above, i.e. tb_trans_cb
> (i.e. post_trans) passes an opaque descriptor of the TB (which can
> be iterated over insn by insn) and the return value (void *) of this
> cb will be passed by tb_exec_cb (i.e. pre_exec).  Perf-wise this
> is pretty OK, turns out even if we don't end up caring about the
> TB, the additional per-TB helper (which might not end up calling
> a callback) does not introduce significant overhead at execution time.

So you build this structure after translating the whole TB, and the user can
iterate it to check the translated instructions. This is closer to other
existing tools: you iterate the structure and then decide which/how to
instrument instructions, memory accesses, etc within it.

My only concern is that this is much more complex than the simpler API I propose
(you must build the informational structures, generate calls to every possible
instrumentation call, which will be optimized-out by TCG if the user decides not
to use them, and overall pay in performance for any unused functionality),
whereas your approach can be implemented on top of it.


> The major hurdle I found is what to do when removing a plugin,
> so that we avoid flushing potentially all translated code. What I ended up
> doing is adding a per-TB list of "plugin_tb" descriptors, which
> track these user pointers so that (1) each plugin gets the right
> pointer, and (2) if n_plugins > 1, we still have a single helper
> that dispatches the callbacks instead of n_plugin helpers.

I'm not sure it's worth optimising for the multiple plugin case.


> If I understand correctly, with promises we directly generate
> a callback, which has the promise(s) as one (or more) of its
> arguments. This is neat and very flexible. However, it forces us to
> retranslate the TB when the plugin is removed (if we're lazy we could
> flush all TBs), and if we have several plugins, we end up with one
> helper per callback, instead of a single one.
> None of this is a huge deal though, I just think is worth considering.

Yes, that happens seldomly enough that the flush cost is negligible.

> Also, I'm not sure Peter and others would be happy with allowing
> plugin code to generate arbitrary callbacks (IIRC arbitrary code
> has already been ruled out). So perhaps a more restrictive option
> like what I suggested above would be more palatable.

AFAIU, arbitrary access to QEMU's structures was ruled out, but not generating
calls to arbitrary user functions.


Cheers,
  Lluis

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Emilio G. Cota]

On Fri, Oct 06, 2017 at 18:07:16 +0300, Lluís Vilanova wrote:
> Emilio G Cota writes:
> > On Thu, Oct 05, 2017 at 02:28:12 +0300, Lluís Vilanova wrote:
> >> The API takes care of telling you if the access could be performed
> >> successfully. If you access the instruction's memory representation at
> >> translation time, it should be able to perform the access, since QEMU's
> >> translation loop just had to do so in order to access that instruction (I should
> >> check what happens in the corner case where another guest CPU changes the page
> >> table, since I'm not sure if the address translation functions I'm using in QEMU
> >> will use the per-vCPU TLB cache or always traverse the page table).
> 
> > That was my concern, I'd rather just perform the read once, that is, the read(s)
> > done by ops->insn_translate.
> 
> If your concern is on performance, that should not be an issue, since you'd be
> using the memory peek functions at translation-time. Furthermore, since others
> suggested having memory peek anyway, that's a nicer way (to me) to compose APIs
> (and is less complex to implement).

My concern was the same as yours, correctness -- what happens if something
changes between the two reads? Because the two reads should always return
the same thing.

> > I see. I implemented what I suggested above, i.e. tb_trans_cb
> > (i.e. post_trans) passes an opaque descriptor of the TB (which can
> > be iterated over insn by insn) and the return value (void *) of this
> > cb will be passed by tb_exec_cb (i.e. pre_exec).  Perf-wise this
> > is pretty OK, turns out even if we don't end up caring about the
> > TB, the additional per-TB helper (which might not end up calling
> > a callback) does not introduce significant overhead at execution time.
> 
> So you build this structure after translating the whole TB, and the user can
> iterate it to check the translated instructions. This is closer to other
> existing tools: you iterate the structure and then decide which/how to
> instrument instructions, memory accesses, etc within it.

Correct. I suspect they went with this design because it makes sense to
do this preprocessing once, instead of having each plugin do it
themselves. I'm not sure how much we should care about supporting multiple
plugins, but the impression I get from DynamoRIO is that it seems important
to users.

> My only concern is that this is much more complex than the simpler API I propose
> (you must build the informational structures, generate calls to every possible
> instrumentation call, which will be optimized-out by TCG if the user decides not
> to use them, and overall pay in performance for any unused functionality),
> whereas your approach can be implemented on top of it.

It's pretty simple; tr_ops->translate_insn has to copy each insn.
For instance, on aarch64 (disas_a64 is called from tr_translate_insn):

-static void disas_a64_insn(CPUARMState *env, DisasContext *s)
+static void disas_a64_insn(CPUARMState *env, DisasContext *s, struct qemu_plugin_insn *q_insn)
 {
     uint32_t insn;

     insn = arm_ldl_code(env, s->pc, s->sctlr_b);
+    if (q_insn) {
+        qemu_plugin_insn_append(q_insn, &insn, sizeof(insn));
+    }

It takes some memory though (we duplicate the guest code), but perf-wise this
isn't a big deal (an empty callback on every TB execution incurs only a 10-15%
perf slowdown).

I don't understand the part where you say that the instrumentation call can
be optimized out. Is there a special value of a "TCG promise" (at tb_trans_post
time) that removes the previously generated callback (at tb_trans_pre time)?
Otherwise I don't see how selective TB instrumentation can work at tb_trans_pre
time.

Thanks,

		E.

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Lluís Vilanova]

Emilio G Cota writes:

> On Fri, Oct 06, 2017 at 18:07:16 +0300, Lluís Vilanova wrote:
>> Emilio G Cota writes:
>> > On Thu, Oct 05, 2017 at 02:28:12 +0300, Lluís Vilanova wrote:
>> >> The API takes care of telling you if the access could be performed
>> >> successfully. If you access the instruction's memory representation at
>> >> translation time, it should be able to perform the access, since QEMU's
>> >> translation loop just had to do so in order to access that instruction (I should
>> >> check what happens in the corner case where another guest CPU changes the page
>> >> table, since I'm not sure if the address translation functions I'm using in QEMU
>> >> will use the per-vCPU TLB cache or always traverse the page table).
>> 
>> > That was my concern, I'd rather just perform the read once, that is, the read(s)
>> > done by ops->insn_translate.
>> 
>> If your concern is on performance, that should not be an issue, since you'd be
>> using the memory peek functions at translation-time. Furthermore, since others
>> suggested having memory peek anyway, that's a nicer way (to me) to compose APIs
>> (and is less complex to implement).

> My concern was the same as yours, correctness -- what happens if something
> changes between the two reads? Because the two reads should always return
> the same thing.

Thinking about it, shouldn't this always be the same given QEMU's TLB/page table
consistency assurances? Otherwise, QEMU could read bytes from different physical
pages while translating an instruction from the same virtual page.

Therefore, this leads me to believe it is safe to use the memory read operations
during translation to let instrumentation libraries know what exactly they are
dealing with.



>> > I see. I implemented what I suggested above, i.e. tb_trans_cb
>> > (i.e. post_trans) passes an opaque descriptor of the TB (which can
>> > be iterated over insn by insn) and the return value (void *) of this
>> > cb will be passed by tb_exec_cb (i.e. pre_exec).  Perf-wise this
>> > is pretty OK, turns out even if we don't end up caring about the
>> > TB, the additional per-TB helper (which might not end up calling
>> > a callback) does not introduce significant overhead at execution time.
>> 
>> So you build this structure after translating the whole TB, and the user can
>> iterate it to check the translated instructions. This is closer to other
>> existing tools: you iterate the structure and then decide which/how to
>> instrument instructions, memory accesses, etc within it.

> Correct. I suspect they went with this design because it makes sense to
> do this preprocessing once, instead of having each plugin do it
> themselves. I'm not sure how much we should care about supporting multiple
> plugins, but the impression I get from DynamoRIO is that it seems important
> to users.

If that can be built into a "helper" instrumentation library / header that
others can use, I would rather keep this functionality outside QEMU.


>> My only concern is that this is much more complex than the simpler API I propose
>> (you must build the informational structures, generate calls to every possible
>> instrumentation call, which will be optimized-out by TCG if the user decides not
>> to use them, and overall pay in performance for any unused functionality),
>> whereas your approach can be implemented on top of it.

> It's pretty simple; tr_ops->translate_insn has to copy each insn.
> For instance, on aarch64 (disas_a64 is called from tr_translate_insn):

> -static void disas_a64_insn(CPUARMState *env, DisasContext *s)
> +static void disas_a64_insn(CPUARMState *env, DisasContext *s, struct qemu_plugin_insn *q_insn)
>  {
>      uint32_t insn;

>      insn = arm_ldl_code(env, s->pc, s->sctlr_b);
> +    if (q_insn) {
> +        qemu_plugin_insn_append(q_insn, &insn, sizeof(insn));
> +    }

> It takes some memory though (we duplicate the guest code), but perf-wise this
> isn't a big deal (an empty callback on every TB execution incurs only a 10-15%
> perf slowdown).

> I don't understand the part where you say that the instrumentation call can
> be optimized out. Is there a special value of a "TCG promise" (at tb_trans_post
> time) that removes the previously generated callback (at tb_trans_pre time)?
> Otherwise I don't see how selective TB instrumentation can work at tb_trans_pre
> time.

With the approach you propose, the instrumentation library is only called at
translation-time after the whole TB has been generated. If the instrumentor now
decides to instrument each instruction, this means QEMU must inject an
instrumentation call to every instruction while translating the TB *just in
case* the instrumentor will need it later. In case the instrumentor decides some
instructions don't need to be instrumented, now QEMU needs to eliminate them
from the TB before generating the host code (in order to eliminate unnecessary
overheads).

Hope it's clearer now.


Thanks!
  Lluis

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Peter Maydell]

On 15 October 2017 at 17:30, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Thinking about it, shouldn't this always be the same given QEMU's TLB/page table
> consistency assurances?

What TLB/page table consistency assurances? For ARM at least
we will only update (ie flush) the TLB when the guest next
executes a relevant TLB maintenance instruction. So a
misbehaving guest can set things up so the page table
is completely different from what's in QEMU's TLB if it
wants. This all falls in the realms of architecturally
unpredictable behaviour for the guest -- whether you
want the instrumentation to be confused as well is a
different question...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Lluís Vilanova]

Peter Maydell writes:

> On 15 October 2017 at 17:30, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Thinking about it, shouldn't this always be the same given QEMU's TLB/page table
>> consistency assurances?

> What TLB/page table consistency assurances? For ARM at least
> we will only update (ie flush) the TLB when the guest next
> executes a relevant TLB maintenance instruction. So a
> misbehaving guest can set things up so the page table
> is completely different from what's in QEMU's TLB if it
> wants. This all falls in the realms of architecturally
> unpredictable behaviour for the guest -- whether you
> want the instrumentation to be confused as well is a
> different question...

I meant that if the contents of a virtual memory page change while QEMU is
translating an instruction, it must be able to detect that and act accordingly
for correctness.

Having that in mind, the same should hold true when an instrumentor reads a
page's contents during translation (e.g., to gather information on opcodes).


Cheers,
  Lluis

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Peter Maydell]

On 21 October 2017 at 15:05, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Peter Maydell writes:
>
>> On 15 October 2017 at 17:30, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>>> Thinking about it, shouldn't this always be the same given QEMU's TLB/page table
>>> consistency assurances?
>
>> What TLB/page table consistency assurances? For ARM at least
>> we will only update (ie flush) the TLB when the guest next
>> executes a relevant TLB maintenance instruction. So a
>> misbehaving guest can set things up so the page table
>> is completely different from what's in QEMU's TLB if it
>> wants. This all falls in the realms of architecturally
>> unpredictable behaviour for the guest -- whether you
>> want the instrumentation to be confused as well is a
>> different question...
>
> I meant that if the contents of a virtual memory page change while QEMU is
> translating an instruction, it must be able to detect that and act accordingly
> for correctness.

That's an interesting corner case, actually. Traditionally
it simply couldn't happen because we were strictly single
threaded and so if we were translating then we weren't
running guest code. We did need to handle "writes mean we
must invalidate an already produced translation", but not
"invalidate one we're halfway through and haven't put in
our data structures yet". Did we get that right in the MTTCG
design? How does it work?

(Did we produce a summary of the MTTCG design anywhere?
I didn't follow the development in detail as it was going
on, but it would be useful to understand the final result.)

In any case, the only assurance we provide over QEMU as a
whole is that if the guest writes to a physical address then
we don't keep hold of a now-duff translation for that physaddr.
We don't guarantee the same thing for guest changes of
the vaddr-to-physaddr mapping -- instead we let the target
specific code deal with this by invalidating QEMU's TLB
when the guest code does TLB invalidate ops.

> Having that in mind, the same should hold true when an instrumentor reads a
> page's contents during translation (e.g., to gather information on opcodes).

Basically I don't think we actually have very strong
guarantees here, and that's another reason for not
providing instrumentation callbacks at translate time.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 01/22] instrument: Add documentation

[Alex Bennée]

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

> On 21 October 2017 at 15:05, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Peter Maydell writes:
>>
>>> On 15 October 2017 at 17:30, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>>>> Thinking about it, shouldn't this always be the same given QEMU's TLB/page table
>>>> consistency assurances?
>>
>>> What TLB/page table consistency assurances? For ARM at least
>>> we will only update (ie flush) the TLB when the guest next
>>> executes a relevant TLB maintenance instruction. So a
>>> misbehaving guest can set things up so the page table
>>> is completely different from what's in QEMU's TLB if it
>>> wants. This all falls in the realms of architecturally
>>> unpredictable behaviour for the guest -- whether you
>>> want the instrumentation to be confused as well is a
>>> different question...
>>
>> I meant that if the contents of a virtual memory page change while QEMU is
>> translating an instruction, it must be able to detect that and act accordingly
>> for correctness.
>
> That's an interesting corner case, actually. Traditionally
> it simply couldn't happen because we were strictly single
> threaded and so if we were translating then we weren't
> running guest code. We did need to handle "writes mean we
> must invalidate an already produced translation", but not
> "invalidate one we're halfway through and haven't put in
> our data structures yet". Did we get that right in the MTTCG
> design? How does it work?

It's currently protected by locks, as you need to grab tb_lock/mmap_lock
to call:

  void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
                                   int is_cpu_write_access)

So no new blocks can be created until you've complete your invalidation
- or you are serialised until the block currently being translated is
completed. At which point the block will be immediately marked as
invalid and not be called again.

>
> (Did we produce a summary of the MTTCG design anywhere?
> I didn't follow the development in detail as it was going
> on, but it would be useful to understand the final result.)

Sure, it's in:

  docs/devel/multi-thread-tcg.txt

>
> In any case, the only assurance we provide over QEMU as a
> whole is that if the guest writes to a physical address then
> we don't keep hold of a now-duff translation for that physaddr.
> We don't guarantee the same thing for guest changes of
> the vaddr-to-physaddr mapping -- instead we let the target
> specific code deal with this by invalidating QEMU's TLB
> when the guest code does TLB invalidate ops.
>
>> Having that in mind, the same should hold true when an instrumentor reads a
>> page's contents during translation (e.g., to gather information on opcodes).
>
> Basically I don't think we actually have very strong
> guarantees here, and that's another reason for not
> providing instrumentation callbacks at translate time.
>
> thanks
> -- PMM


--
Alex Bennée