* [Qemu-devel] [PATCH v4 01/20] instrument: Add documentation
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
@ 2017-09-06 17:26 ` Lluís Vilanova
2017-09-06 17:30 ` [Qemu-devel] [PATCH v4 02/20] instrument: Add configure-time flag Lluís Vilanova
` (21 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 17:26 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Lluís Vilanova
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
MAINTAINERS | 6 ++
docs/instrument.txt | 174 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 180 insertions(+)
create mode 100644 docs/instrument.txt
diff --git a/MAINTAINERS b/MAINTAINERS
index ccee28b12d..edb313c632 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1480,6 +1480,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
+
Checkpatch
S: Odd Fixes
F: scripts/checkpatch.pl
diff --git a/docs/instrument.txt b/docs/instrument.txt
new file mode 100644
index 0000000000..c43ca9c6d0
--- /dev/null
+++ b/docs/instrument.txt
@@ -0,0 +1,174 @@
+= 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 */
+ #include <qemu-instr/visibility.h> /* symbol visibility */
+
+ /* 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 */
+ QI_VPUBLIC 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
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 02/20] instrument: Add configure-time flag
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
2017-09-06 17:26 ` [Qemu-devel] [PATCH v4 01/20] instrument: Add documentation Lluís Vilanova
@ 2017-09-06 17:30 ` Lluís Vilanova
2017-09-06 17:34 ` [Qemu-devel] [PATCH v4 03/20] instrument: Add generic library loader Lluís Vilanova
` (20 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 17:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
configure | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/configure b/configure
index dd73cce62f..80dcc91c98 100755
--- a/configure
+++ b/configure
@@ -352,6 +352,7 @@ pie=""
qom_cast_debug="yes"
trace_backends="log"
trace_file="trace"
+instrument="no"
spice=""
rbd=""
smartcard=""
@@ -891,6 +892,8 @@ for opt do
;;
--with-trace-file=*) trace_file="$optarg"
;;
+ --enable-instrument) instrument="yes"
+ ;;
--enable-gprof) gprof="yes"
;;
--enable-gcov) gcov="yes"
@@ -1441,6 +1444,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
@@ -5371,6 +5375,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"
@@ -6028,6 +6033,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
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 03/20] instrument: Add generic library loader
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
2017-09-06 17:26 ` [Qemu-devel] [PATCH v4 01/20] instrument: Add documentation Lluís Vilanova
2017-09-06 17:30 ` [Qemu-devel] [PATCH v4 02/20] instrument: Add configure-time flag Lluís Vilanova
@ 2017-09-06 17:34 ` Lluís Vilanova
2017-09-06 21:20 ` Emilio G. Cota
2017-09-06 17:38 ` [Qemu-devel] [PATCH v4 04/20] instrument: [linux-user] Add command line " Lluís Vilanova
` (19 subsequent siblings)
22 siblings, 1 reply; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 17:34 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Lluís Vilanova
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
MAINTAINERS | 1
Makefile.objs | 4 +
configure | 2 +
instrument/Makefile.objs | 4 +
instrument/cmdline.c | 124 ++++++++++++++++++++++++++++++++
instrument/cmdline.h | 49 +++++++++++++
instrument/load.c | 176 ++++++++++++++++++++++++++++++++++++++++++++++
instrument/load.h | 83 ++++++++++++++++++++++
8 files changed, 443 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
diff --git a/MAINTAINERS b/MAINTAINERS
index edb313c632..edd2c49078 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1485,6 +1485,7 @@ M: Lluís Vilanova <vilanova@ac.upc.edu>
M: Stefan Hajnoczi <stefanha@redhat.com>
S: Maintained
F: docs/instrument.txt
+F: instrument/
Checkpatch
S: Odd Fixes
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 80dcc91c98..05bd7b1950 100755
--- a/configure
+++ b/configure
@@ -6034,6 +6034,8 @@ fi
echo "CONFIG_TRACE_FILE=$trace_file" >> $config_host_mak
if test "$instrument" = "yes"; then
+ LDFLAGS="-rdynamic $LDFLAGS" # limit symbols available to clients
+ 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..5ea5c77245
--- /dev/null
+++ b/instrument/Makefile.objs
@@ -0,0 +1,4 @@
+# -*- mode: makefile -*-
+
+target-obj-y += cmdline.o
+target-obj-$(CONFIG_INSTRUMENT) += load.o
diff --git a/instrument/cmdline.c b/instrument/cmdline.c
new file mode 100644
index 0000000000..ec87f96c72
--- /dev/null
+++ b/instrument/cmdline.c
@@ -0,0 +1,124 @@
+/*
+ * 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;
+ int64_t handle;
+
+ if (path == NULL) {
+ return;
+ }
+
+ if (atexit(instr_fini) != 0) {
+ fprintf(stderr, "error: atexit: %s\n", strerror(errno));
+ abort();
+ }
+
+ err = instr_load(path, argc, argv, &handle);
+ switch (err) {
+ case INSTR_LOAD_OK:
+ return;
+ case INSTR_LOAD_TOO_MANY:
+ error_report("instrument: tried to load too many libraries");
+ 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..e6ea08c3e3
--- /dev/null
+++ b/instrument/cmdline.h
@@ -0,0 +1,49 @@
+/*
+ * 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
+
+
+/**
+ * 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..a57401102a
--- /dev/null
+++ b/instrument/load.c
@@ -0,0 +1,176 @@
+/*
+ * 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 int64_t InstrHandleID;
+
+typedef struct InstrHandle
+{
+ InstrHandleID id;
+ void *dlhandle;
+ QSLIST_ENTRY(InstrHandle) list;
+} InstrHandle;
+
+
+static InstrHandleID handle_last_id;
+static QSLIST_HEAD(, InstrHandle) handles = QSLIST_HEAD_INITIALIZER(handles);
+static QemuMutex instr_lock;
+
+
+static InstrHandle *handle_get(void)
+{
+ InstrHandle *res = g_malloc0(sizeof(InstrHandle));
+ res->id = handle_last_id++;
+ QSLIST_INSERT_HEAD(&handles, res, list);
+ return res;
+}
+
+static bool handle_put(InstrHandleID id)
+{
+ InstrHandle *prev = NULL;
+ InstrHandle *handle;
+ QSLIST_FOREACH(handle, &handles, list) {
+ if (handle->id == id) {
+ break;
+ }
+ prev = handle;
+ }
+ if (handle == NULL) {
+ return false;
+ } else {
+ if (prev == NULL) {
+ QSLIST_REMOVE_HEAD(&handles, list);
+ } else {
+ QSLIST_REMOVE_AFTER(prev, list);
+ }
+ g_free(handle);
+ return true;
+ }
+}
+
+static InstrHandle *handle_find(InstrHandleID id)
+{
+ InstrHandle *handle;
+ QSLIST_FOREACH(handle, &handles, list) {
+ if (handle->id == id) {
+ return handle;
+ }
+ }
+ return NULL;
+}
+
+InstrLoadError instr_load(const char * path, int argc, const char ** argv,
+ int64_t *handle_id)
+{
+ InstrLoadError res;
+ InstrHandle * handle;
+ int (*main_cb)(int, const char **);
+ int main_res;
+
+ qemu_rec_mutex_lock(&instr_lock);
+
+ *handle_id = -1;
+
+ if (!QSLIST_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_get();
+ 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;
+ }
+
+ *handle_id = handle->id;
+ res = INSTR_LOAD_OK;
+ goto out;
+
+err:
+ handle_put(handle->id);
+out:
+ qemu_rec_mutex_unlock(&instr_lock);
+ return res;
+}
+
+InstrUnloadError instr_unload(int64_t handle_id)
+{
+ InstrLoadError res;
+
+ qemu_rec_mutex_lock(&instr_lock);
+
+ InstrHandle *handle = handle_find(handle_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_put(handle->id);
+
+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 = QSLIST_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..2ddb2c6c19
--- /dev/null
+++ b/instrument/load.h
@@ -0,0 +1,83 @@
+/*
+ * 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 "qemu/queue.h"
+#include "qemu/thread.h"
+
+
+/**
+ * InstrLoadError:
+ * @INSTR_LOAD_OK: Correctly loaded.
+ * @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_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.
+ * @handle: Instrumentation library handle (undefined in case of error).
+ *
+ * Load a dynamic trace instrumentation library.
+ *
+ * Returns: Whether the library could be loaded.
+ */
+InstrLoadError instr_load(const char * path, int argc, const char ** argv,
+ int64_t *handle);
+
+/**
+ * instr_unload:
+ * @handle: Instrumentation library handle returned by instr_load().
+ *
+ * Unload the given instrumentation library.
+ *
+ * Returns: Whether the library could be unloaded.
+ */
+InstrUnloadError instr_unload(int64_t handle);
+
+/**
+ * instr_unload_all:
+ *
+ * Unload all instrumentation libraries.
+ *
+ * Returns: Whether any library could not be unloaded.
+ */
+InstrUnloadError instr_unload_all(void);
+
+#endif /* INSTRUMENT_LOAD_H */
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v4 03/20] instrument: Add generic library loader
2017-09-06 17:34 ` [Qemu-devel] [PATCH v4 03/20] instrument: Add generic library loader Lluís Vilanova
@ 2017-09-06 21:20 ` Emilio G. Cota
2017-09-10 17:41 ` Lluís Vilanova
0 siblings, 1 reply; 39+ messages in thread
From: Emilio G. Cota @ 2017-09-06 21:20 UTC (permalink / raw)
To: Lluís Vilanova; +Cc: qemu-devel, Eric Blake, Stefan Hajnoczi
On Wed, Sep 06, 2017 at 20:34:48 +0300, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
(snip)
> diff --git a/configure b/configure
> index 80dcc91c98..05bd7b1950 100755
> --- a/configure
> +++ b/configure
> @@ -6034,6 +6034,8 @@ fi
> echo "CONFIG_TRACE_FILE=$trace_file" >> $config_host_mak
>
> if test "$instrument" = "yes"; then
> + LDFLAGS="-rdynamic $LDFLAGS" # limit symbols available to clients
-rdynamic exports all objects to the instrumenters, not the other way around.
You can see it with nm(1).
BTW I'm using ebf447b5b from your qemu-dbi repo. Is that the same as this
patchset? It doesn't compile with --enable-instrument:
$ make
CC x86_64-linux-user/instrument/trace.o
/data/src/qemu2/instrument/trace.c:12: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
Makefile:326: recipe for target 'subdir-x86_64-linux-user' failed
make: *** [subdir-x86_64-linux-user] Error 2
Emilio
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v4 03/20] instrument: Add generic library loader
2017-09-06 21:20 ` Emilio G. Cota
@ 2017-09-10 17:41 ` Lluís Vilanova
0 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-10 17:41 UTC (permalink / raw)
To: Emilio G. Cota; +Cc: qemu-devel, Eric Blake, Stefan Hajnoczi
Emilio G Cota writes:
> On Wed, Sep 06, 2017 at 20:34:48 +0300, Lluís Vilanova wrote:
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
> (snip)
>> diff --git a/configure b/configure
>> index 80dcc91c98..05bd7b1950 100755
>> --- a/configure
>> +++ b/configure
>> @@ -6034,6 +6034,8 @@ fi
>> echo "CONFIG_TRACE_FILE=$trace_file" >> $config_host_mak
>>
>> if test "$instrument" = "yes"; then
>> + LDFLAGS="-rdynamic $LDFLAGS" # limit symbols available to clients
> -rdynamic exports all objects to the instrumenters, not the other way around.
> You can see it with nm(1).
I'll send a minor fix for this in next version.
> BTW I'm using ebf447b5b from your qemu-dbi repo. Is that the same as this
> patchset? It doesn't compile with --enable-instrument:
> $ make
> CC x86_64-linux-user/instrument/trace.o
> /data/src/qemu2/instrument/trace.c:12: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
> Makefile:326: recipe for target 'subdir-x86_64-linux-user' failed
> make: *** [subdir-x86_64-linux-user] Error 2
No, I started using a separate branch to keep the old patches around in the
meantime.
> Emilio
Lluis
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v4 04/20] instrument: [linux-user] Add command line library loader
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (2 preceding siblings ...)
2017-09-06 17:34 ` [Qemu-devel] [PATCH v4 03/20] instrument: Add generic library loader Lluís Vilanova
@ 2017-09-06 17:38 ` Lluís Vilanova
2017-09-06 17:42 ` [Qemu-devel] [PATCH v4 05/20] instrument: [bsd-user] " Lluís Vilanova
` (18 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 17:38 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Riku Voipio, Laurent Vivier
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
linux-user/main.c | 19 +++++++++++++++++++
linux-user/syscall.c | 4 ++++
2 files changed, 23 insertions(+)
diff --git a/linux-user/main.c b/linux-user/main.c
index 03666ef657..25253bc28a 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,7 @@ int main(int argc, char **argv, char **envp)
srand(time(NULL));
qemu_add_opts(&qemu_trace_opts);
+ qemu_add_opts(&qemu_instr_opts);
optind = parse_args(argc, argv);
@@ -4265,6 +4282,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
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 05/20] instrument: [bsd-user] Add command line library loader
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (3 preceding siblings ...)
2017-09-06 17:38 ` [Qemu-devel] [PATCH v4 04/20] instrument: [linux-user] Add command line " Lluís Vilanova
@ 2017-09-06 17:42 ` Lluís Vilanova
2017-09-06 17:46 ` [Qemu-devel] [PATCH v4 06/20] instrument: [softmmu] " Lluís Vilanova
` (17 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 17:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
bsd-user/main.c | 15 +++++++++++++++
bsd-user/syscall.c | 5 +++++
2 files changed, 20 insertions(+)
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 8a6706a1c8..2cf75290cd 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,7 @@ int main(int argc, char **argv)
cpu_model = NULL;
qemu_add_opts(&qemu_trace_opts);
+ qemu_add_opts(&qemu_instr_opts);
optind = 1;
for (;;) {
@@ -843,6 +853,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 +885,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 */
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 06/20] instrument: [softmmu] Add command line library loader
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (4 preceding siblings ...)
2017-09-06 17:42 ` [Qemu-devel] [PATCH v4 05/20] instrument: [bsd-user] " Lluís Vilanova
@ 2017-09-06 17:46 ` Lluís Vilanova
2017-09-06 17:50 ` [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add " Lluís Vilanova
` (16 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 17:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Paolo Bonzini
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 8e247cc2a2..10dfea3ec7 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"
@@ -3035,6 +3036,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);
@@ -3062,6 +3066,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);
@@ -4003,6 +4010,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);
@@ -4190,6 +4203,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) {
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add library loader
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (5 preceding siblings ...)
2017-09-06 17:46 ` [Qemu-devel] [PATCH v4 06/20] instrument: [softmmu] " Lluís Vilanova
@ 2017-09-06 17:50 ` Lluís Vilanova
2017-09-07 8:43 ` Markus Armbruster
2017-09-06 17:55 ` [Qemu-devel] [PATCH v4 08/20] instrument: [hmp] " Lluís Vilanova
` (15 subsequent siblings)
22 siblings, 1 reply; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 17:50 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Lluís Vilanova,
Markus Armbruster
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
instrument/Makefile.objs | 1
instrument/load.h | 4 ++
instrument/qmp.c | 88 ++++++++++++++++++++++++++++++++++++++++++
qapi-schema.json | 3 +
qapi/instrument.json | 96 ++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 192 insertions(+)
create mode 100644 instrument/qmp.c
create mode 100644 qapi/instrument.json
diff --git a/instrument/Makefile.objs b/instrument/Makefile.objs
index 5ea5c77245..13a8f60431 100644
--- a/instrument/Makefile.objs
+++ b/instrument/Makefile.objs
@@ -2,3 +2,4 @@
target-obj-y += cmdline.o
target-obj-$(CONFIG_INSTRUMENT) += load.o
+target-obj-y += qmp.o
diff --git a/instrument/load.h b/instrument/load.h
index 2ddb2c6c19..f8a02e6849 100644
--- a/instrument/load.h
+++ b/instrument/load.h
@@ -25,6 +25,8 @@
* @INSTR_LOAD_DLERROR: Error with libdl (see dlerror).
*
* Error codes for instr_load().
+ *
+ * NOTE: Keep in sync with QAPI's #InstrLoadCode.
*/
typedef enum {
INSTR_LOAD_OK,
@@ -40,6 +42,8 @@ typedef enum {
* @INSTR_UNLOAD_DLERROR: Error with libdl (see dlerror).
*
* Error codes for instr_unload().
+ *
+ * NOTE: Keep in sync with QAPI's #InstrUnloadCode.
*/
typedef enum {
INSTR_UNLOAD_OK,
diff --git a/instrument/qmp.c b/instrument/qmp.c
new file mode 100644
index 0000000000..c36960c12f
--- /dev/null
+++ b/instrument/qmp.c
@@ -0,0 +1,88 @@
+/*
+ * 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 "qemu-common.h"
+#include "qapi/qmp/qerror.h"
+#include "qmp-commands.h"
+
+#include <dlfcn.h>
+
+#include "instrument/load.h"
+
+
+
+InstrLoadResult *qmp_instr_load(const char * path,
+ bool have_args, strList * args,
+ Error **errp)
+{
+ InstrLoadResult *res = g_malloc0(sizeof(*res));
+
+#if defined(CONFIG_INSTRUMENT)
+ int argc = 0;
+ const char **argv = 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;
+ }
+
+ InstrLoadError code = instr_load(path, argc, argv, &res->handle);
+ switch (code) {
+ case INSTR_LOAD_OK:
+ res->code = INSTR_LOAD_CODE_OK;
+ res->has_handle = true;
+ break;
+ case INSTR_LOAD_TOO_MANY:
+ res->code = INSTR_LOAD_CODE_TOO_MANY;
+ break;
+ case INSTR_LOAD_ERROR:
+ res->code = INSTR_LOAD_CODE_ERROR;
+ break;
+ case INSTR_LOAD_DLERROR:
+ res->has_msg = true;
+ res->msg = dlerror();
+ res->code = INSTR_LOAD_CODE_DLERROR;
+ break;
+ }
+#else
+ res->code = INSTR_LOAD_CODE_UNAVAILABLE;
+#endif
+
+ return res;
+}
+
+InstrUnloadResult *qmp_instr_unload(int64_t handle, Error **errp)
+{
+ InstrUnloadResult *res = g_malloc0(sizeof(*res));
+
+#if defined(CONFIG_INSTRUMENT)
+ InstrUnloadError code = instr_unload(handle);
+ switch (code) {
+ case INSTR_UNLOAD_OK:
+ res->code = INSTR_UNLOAD_CODE_OK;
+ break;
+ case INSTR_UNLOAD_INVALID:
+ res->code = INSTR_UNLOAD_CODE_INVALID;
+ break;
+ case INSTR_UNLOAD_DLERROR:
+ res->has_msg = true;
+ res->msg = dlerror();
+ break;
+ res->code = INSTR_UNLOAD_CODE_DLERROR;
+ }
+#else
+ res->code = INSTR_UNLOAD_CODE_UNAVAILABLE;
+#endif
+
+ return res;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index 802ea53d00..5e343be9ff 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -90,6 +90,9 @@
# QAPI introspection
{ 'include': 'qapi/introspect.json' }
+# Instrumentation commands
+{ 'include': 'qapi/instrument.json' }
+
##
# = QMP commands
##
diff --git a/qapi/instrument.json b/qapi/instrument.json
new file mode 100644
index 0000000000..ea63fae309
--- /dev/null
+++ b/qapi/instrument.json
@@ -0,0 +1,96 @@
+# *-*- Mode: Python -*-*
+#
+# QAPI 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.
+
+##
+# @InstrLoadCode:
+#
+# Result code of an 'instr-load' command.
+#
+# @ok: Correctly loaded.
+# @too-many: Tried to load too many instrumentation libraries.
+# @error: The library's main() function returned a non-zero value.
+# @dlerror: Error with libdl (see 'msg').
+# @unavailable: Service not available.
+#
+# Since: 2.11
+##
+{ 'enum': 'InstrLoadCode',
+ 'data': [ 'ok', 'too-many', 'error', 'dlerror', 'unavailable' ] }
+
+##
+# @InstrLoadResult:
+#
+# Result of an 'instr-load' command.
+#
+# @code: Result code.
+# @msg: Additional error message (for human consumption only; present only in
+# case of error).
+# @handle: Instrumentation library identifier (present only if successful).
+#
+# Since: 2.11
+##
+{ 'struct': 'InstrLoadResult',
+ 'data': { 'code': 'InstrLoadCode', '*msg': 'str', '*handle': 'int' } }
+
+##
+# @instr-load:
+#
+# Load an instrumentation library.
+#
+# @path: path to the dynamic instrumentation library
+# @args: arguments to the dynamic instrumentation library
+#
+# Since: 2.11
+##
+{ 'command': 'instr-load',
+ 'data': { 'path': 'str', '*args': ['str'] },
+ 'returns': 'InstrLoadResult' }
+
+
+##
+# @InstrUnloadCode:
+#
+# Result code of an 'instr-unload' command.
+#
+# @ok: Correctly unloaded.
+# @invalid: Invalid handle.
+# @dlerror: Error with libdl (see 'msg').
+# @unavailable: Service not available.
+#
+# Since: 2.11
+##
+{ 'enum': 'InstrUnloadCode',
+ 'data': [ 'ok', 'invalid', 'dlerror', 'unavailable' ] }
+
+##
+# @InstrUnloadResult:
+#
+# Result of an 'instr-unload' command.
+#
+# @code: Result code.
+# @msg: Additional error message (for human consumption only; present only in
+# case of error).
+#
+# Since: 2.11
+##
+{ 'struct': 'InstrUnloadResult',
+ 'data': { 'code': 'InstrUnloadCode', '*msg': 'str' } }
+
+##
+# @instr-unload:
+#
+# Unload an instrumentation library.
+#
+# @handle: Instrumentation library identifier (see #InstrLoadResult).
+#
+# Since: 2.11
+##
+{ 'command': 'instr-unload',
+ 'data': { 'handle': 'int' },
+ 'returns': 'InstrUnloadResult' }
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add library loader
2017-09-06 17:50 ` [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add " Lluís Vilanova
@ 2017-09-07 8:43 ` Markus Armbruster
2017-09-10 21:39 ` Lluís Vilanova
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2017-09-07 8:43 UTC (permalink / raw)
To: Lluís Vilanova
Cc: qemu-devel, Emilio G. Cota, Stefan Hajnoczi, Eric Blake,
Marc-André Lureau
I missed prior versions of this series. Please cc: qapi-schema
maintainers on all non-trivial schema patches.
scripts/get_maintainer.pl points to them for this patch.
Marc-André, semantic conflict with your QAPI conditionals series. Just
a heads-up, there's nothing for you to do about it right now.
Lluís Vilanova <vilanova@ac.upc.edu> writes:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> instrument/Makefile.objs | 1
> instrument/load.h | 4 ++
> instrument/qmp.c | 88 ++++++++++++++++++++++++++++++++++++++++++
> qapi-schema.json | 3 +
> qapi/instrument.json | 96 ++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 192 insertions(+)
> create mode 100644 instrument/qmp.c
> create mode 100644 qapi/instrument.json
Missing: update of MAINTAINERS for qapi/instrument.json.
> diff --git a/instrument/Makefile.objs b/instrument/Makefile.objs
> index 5ea5c77245..13a8f60431 100644
> --- a/instrument/Makefile.objs
> +++ b/instrument/Makefile.objs
> @@ -2,3 +2,4 @@
>
> target-obj-y += cmdline.o
> target-obj-$(CONFIG_INSTRUMENT) += load.o
> +target-obj-y += qmp.o
> diff --git a/instrument/load.h b/instrument/load.h
> index 2ddb2c6c19..f8a02e6849 100644
> --- a/instrument/load.h
> +++ b/instrument/load.h
> @@ -25,6 +25,8 @@
> * @INSTR_LOAD_DLERROR: Error with libdl (see dlerror).
> *
> * Error codes for instr_load().
> + *
> + * NOTE: Keep in sync with QAPI's #InstrLoadCode.
> */
> typedef enum {
> INSTR_LOAD_OK,
> @@ -40,6 +42,8 @@ typedef enum {
> * @INSTR_UNLOAD_DLERROR: Error with libdl (see dlerror).
> *
> * Error codes for instr_unload().
> + *
> + * NOTE: Keep in sync with QAPI's #InstrUnloadCode.
> */
> typedef enum {
> INSTR_UNLOAD_OK,
Any particular reason why you can't simply use the QAPI-generated enum
types?
> diff --git a/instrument/qmp.c b/instrument/qmp.c
> new file mode 100644
> index 0000000000..c36960c12f
> --- /dev/null
> +++ b/instrument/qmp.c
> @@ -0,0 +1,88 @@
> +/*
> + * 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 "qemu-common.h"
> +#include "qapi/qmp/qerror.h"
> +#include "qmp-commands.h"
> +
> +#include <dlfcn.h>
System headers go right after "qemu/osdep.h".
> +
> +#include "instrument/load.h"
> +
> +
> +
Fewer blank lines would do.
> +InstrLoadResult *qmp_instr_load(const char * path,
> + bool have_args, strList * args,
checkpatch ERROR: "foo * bar" should be "foo *bar"
Please feed it all your patches, and carefully consider which of its
complaints you should address.
> + Error **errp)
> +{
> + InstrLoadResult *res = g_malloc0(sizeof(*res));
> +
> +#if defined(CONFIG_INSTRUMENT)
> + int argc = 0;
> + const char **argv = 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;
> + }
> +
> + InstrLoadError code = instr_load(path, argc, argv, &res->handle);
> + switch (code) {
> + case INSTR_LOAD_OK:
> + res->code = INSTR_LOAD_CODE_OK;
> + res->has_handle = true;
> + break;
> + case INSTR_LOAD_TOO_MANY:
> + res->code = INSTR_LOAD_CODE_TOO_MANY;
> + break;
> + case INSTR_LOAD_ERROR:
> + res->code = INSTR_LOAD_CODE_ERROR;
> + break;
> + case INSTR_LOAD_DLERROR:
> + res->has_msg = true;
> + res->msg = dlerror();
> + res->code = INSTR_LOAD_CODE_DLERROR;
> + break;
> + }
> +#else
> + res->code = INSTR_LOAD_CODE_UNAVAILABLE;
> +#endif
> +
> + return res;
> +}
> +
> +InstrUnloadResult *qmp_instr_unload(int64_t handle, Error **errp)
> +{
> + InstrUnloadResult *res = g_malloc0(sizeof(*res));
> +
> +#if defined(CONFIG_INSTRUMENT)
> + InstrUnloadError code = instr_unload(handle);
> + switch (code) {
> + case INSTR_UNLOAD_OK:
> + res->code = INSTR_UNLOAD_CODE_OK;
> + break;
> + case INSTR_UNLOAD_INVALID:
> + res->code = INSTR_UNLOAD_CODE_INVALID;
> + break;
> + case INSTR_UNLOAD_DLERROR:
> + res->has_msg = true;
> + res->msg = dlerror();
> + break;
> + res->code = INSTR_UNLOAD_CODE_DLERROR;
> + }
> +#else
> + res->code = INSTR_UNLOAD_CODE_UNAVAILABLE;
> +#endif
> +
> + return res;
> +}
You're inventing your own "this feature isn't compiled in" protocol. We
already got too many of them. Let's stick to one of the existing ones,
namely the one that's got some visibility in introspection. Bonus:
turns the semantic conflict with Marc-André's work into a textual
conflict. Here's how:
* Add your commands to qmp_unregister_commands_hack() in monitor.c,
roughly like this:
#ifndef CONFIG_INSTRUMENT
qmp_unregister_command(&qmp_commands, "instr-load");
qmp_unregister_command(&qmp_commands, "instr-unload");
#endif
* Compile qmp.c only when CONFIG_INSTRUMENT, just like the other .c
files there, except for cmdline.c. Drop the ifdeffery there.
* Add stubbed out command handlers to stubs/instrument.c, roughly like
this:
InstrLoadResult *qmp_instr_load(const char *path,
bool have_args, strList *args,
Error **errp)
{
error_setg(errp, QERR_UNSUPPORTED);
return NULL;
}
Same for qmp_instr_unload().
These stubs must exist for the link to succeed, but they won't be
called. They'll go away when Marc-André's work lands.
* In the next patch, make the HMP command conditional on
CONFIG_INSTRUMENT, just like trace-file is conditional on
CONFIG_TRACE_SIMPLE.
Questions?
You're also inventing your own "command failed" protocol. I'll explain
in my review of instrument.json.
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 802ea53d00..5e343be9ff 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -90,6 +90,9 @@
> # QAPI introspection
> { 'include': 'qapi/introspect.json' }
>
> +# Instrumentation commands
> +{ 'include': 'qapi/instrument.json' }
> +
> ##
> # = QMP commands
> ##
> diff --git a/qapi/instrument.json b/qapi/instrument.json
> new file mode 100644
> index 0000000000..ea63fae309
> --- /dev/null
> +++ b/qapi/instrument.json
> @@ -0,0 +1,96 @@
> +# *-*- Mode: Python -*-*
> +#
> +# QAPI 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.
Make the title a doc comment, like this:
# *-*- 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.
##
The ## around the title make it go into generated
docs/interop/qemu-qmp-ref.* documentation.
> +
> +##
> +# @InstrLoadCode:
> +#
> +# Result code of an 'instr-load' command.
> +#
> +# @ok: Correctly loaded.
> +# @too-many: Tried to load too many instrumentation libraries.
> +# @error: The library's main() function returned a non-zero value.
> +# @dlerror: Error with libdl (see 'msg').
> +# @unavailable: Service not available.
> +#
> +# Since: 2.11
> +##
> +{ 'enum': 'InstrLoadCode',
> + 'data': [ 'ok', 'too-many', 'error', 'dlerror', 'unavailable' ] }
> +
> +##
> +# @InstrLoadResult:
> +#
> +# Result of an 'instr-load' command.
> +#
> +# @code: Result code.
> +# @msg: Additional error message (for human consumption only; present only in
> +# case of error).
> +# @handle: Instrumentation library identifier (present only if successful).
> +#
> +# Since: 2.11
> +##
> +{ 'struct': 'InstrLoadResult',
> + 'data': { 'code': 'InstrLoadCode', '*msg': 'str', '*handle': 'int' } }
> +
> +##
> +# @instr-load:
> +#
> +# Load an instrumentation library.
> +#
> +# @path: path to the dynamic instrumentation library
> +# @args: arguments to the dynamic instrumentation library
> +#
> +# Since: 2.11
> +##
> +{ 'command': 'instr-load',
> + 'data': { 'path': 'str', '*args': ['str'] },
> + 'returns': 'InstrLoadResult' }
No :)
If the command fails, it must send an error response instead success
response. Yours always sends a success response.
As far as I can tell, instr-load returns a handle on success, always,
and nothing else. Therefore:
{ 'struct': 'InstrLoadResult',
'data': { 'handle': 'int' } }
On error, qmp_instr_load() should set a suitable error with error_setg()
and return NULL.
QAPI enum type InstrLoadCode goes away.
On returning a handle: we commonly let the user specify an ID string
instead. For instance, device_add doesn't return a handle you can pass
to device_del. Instead, it takes a string-valued 'id' parameter. Other
commands, such as device_del, can refer to the device by that ID. Is
there any particular reason why you can't stick to this convention for
instrumentation?
> +
> +
> +##
> +# @InstrUnloadCode:
> +#
> +# Result code of an 'instr-unload' command.
> +#
> +# @ok: Correctly unloaded.
> +# @invalid: Invalid handle.
> +# @dlerror: Error with libdl (see 'msg').
> +# @unavailable: Service not available.
> +#
> +# Since: 2.11
> +##
> +{ 'enum': 'InstrUnloadCode',
> + 'data': [ 'ok', 'invalid', 'dlerror', 'unavailable' ] }
> +
> +##
> +# @InstrUnloadResult:
> +#
> +# Result of an 'instr-unload' command.
> +#
> +# @code: Result code.
> +# @msg: Additional error message (for human consumption only; present only in
> +# case of error).
> +#
> +# Since: 2.11
> +##
> +{ 'struct': 'InstrUnloadResult',
> + 'data': { 'code': 'InstrUnloadCode', '*msg': 'str' } }
> +
> +##
> +# @instr-unload:
> +#
> +# Unload an instrumentation library.
> +#
> +# @handle: Instrumentation library identifier (see #InstrLoadResult).
> +#
> +# Since: 2.11
> +##
> +{ 'command': 'instr-unload',
> + 'data': { 'handle': 'int' },
> + 'returns': 'InstrUnloadResult' }
Likewise. instr-unload returns nothing on success. Both QAPI types go
away.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add library loader
2017-09-07 8:43 ` Markus Armbruster
@ 2017-09-10 21:39 ` Lluís Vilanova
0 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-10 21:39 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Emilio G. Cota, Stefan Hajnoczi, Eric Blake,
Marc-André Lureau
Markus Armbruster writes:
> I missed prior versions of this series. Please cc: qapi-schema
> maintainers on all non-trivial schema patches.
> scripts/get_maintainer.pl points to them for this patch.
> Marc-André, semantic conflict with your QAPI conditionals series. Just
> a heads-up, there's nothing for you to do about it right now.
> Lluís Vilanova <vilanova@ac.upc.edu> writes:
[...]
>> diff --git a/instrument/load.h b/instrument/load.h
>> index 2ddb2c6c19..f8a02e6849 100644
>> --- a/instrument/load.h
>> +++ b/instrument/load.h
>> @@ -25,6 +25,8 @@
>> * @INSTR_LOAD_DLERROR: Error with libdl (see dlerror).
>> *
>> * Error codes for instr_load().
>> + *
>> + * NOTE: Keep in sync with QAPI's #InstrLoadCode.
>> */
>> typedef enum {
>> INSTR_LOAD_OK,
>> @@ -40,6 +42,8 @@ typedef enum {
>> * @INSTR_UNLOAD_DLERROR: Error with libdl (see dlerror).
>> *
>> * Error codes for instr_unload().
>> + *
>> + * NOTE: Keep in sync with QAPI's #InstrUnloadCode.
>> */
>> typedef enum {
>> INSTR_UNLOAD_OK,
> Any particular reason why you can't simply use the QAPI-generated enum
> types?
Silly me, just missed that "optimization" :) But after reading your later
comments, I'll keep these and remove the QAPI-generated enums.
[...]
>> diff --git a/instrument/qmp.c b/instrument/qmp.c
>> new file mode 100644
>> index 0000000000..c36960c12f
>> --- /dev/null
>> +++ b/instrument/qmp.c
>> @@ -0,0 +1,88 @@
>> +/*
>> + * 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 "qemu-common.h"
>> +#include "qapi/qmp/qerror.h"
>> +#include "qmp-commands.h"
>> +
>> +#include <dlfcn.h>
> System headers go right after "qemu/osdep.h".
>> +
>> +#include "instrument/load.h"
>> +
>> +
>> +
> Fewer blank lines would do.
>> +InstrLoadResult *qmp_instr_load(const char * path,
>> + bool have_args, strList * args,
> checkpatch ERROR: "foo * bar" should be "foo *bar"
> Please feed it all your patches, and carefully consider which of its
> complaints you should address.
>> + Error **errp)
>> +{
>> + InstrLoadResult *res = g_malloc0(sizeof(*res));
>> +
>> +#if defined(CONFIG_INSTRUMENT)
>> + int argc = 0;
>> + const char **argv = 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;
>> + }
>> +
>> + InstrLoadError code = instr_load(path, argc, argv, &res->handle);
>> + switch (code) {
>> + case INSTR_LOAD_OK:
>> + res->code = INSTR_LOAD_CODE_OK;
>> + res->has_handle = true;
>> + break;
>> + case INSTR_LOAD_TOO_MANY:
>> + res->code = INSTR_LOAD_CODE_TOO_MANY;
>> + break;
>> + case INSTR_LOAD_ERROR:
>> + res->code = INSTR_LOAD_CODE_ERROR;
>> + break;
>> + case INSTR_LOAD_DLERROR:
>> + res->has_msg = true;
>> + res->msg = dlerror();
>> + res->code = INSTR_LOAD_CODE_DLERROR;
>> + break;
>> + }
>> +#else
>> + res->code = INSTR_LOAD_CODE_UNAVAILABLE;
>> +#endif
>> +
>> + return res;
>> +}
>> +
>> +InstrUnloadResult *qmp_instr_unload(int64_t handle, Error **errp)
>> +{
>> + InstrUnloadResult *res = g_malloc0(sizeof(*res));
>> +
>> +#if defined(CONFIG_INSTRUMENT)
>> + InstrUnloadError code = instr_unload(handle);
>> + switch (code) {
>> + case INSTR_UNLOAD_OK:
>> + res->code = INSTR_UNLOAD_CODE_OK;
>> + break;
>> + case INSTR_UNLOAD_INVALID:
>> + res->code = INSTR_UNLOAD_CODE_INVALID;
>> + break;
>> + case INSTR_UNLOAD_DLERROR:
>> + res->has_msg = true;
>> + res->msg = dlerror();
>> + break;
>> + res->code = INSTR_UNLOAD_CODE_DLERROR;
>> + }
>> +#else
>> + res->code = INSTR_UNLOAD_CODE_UNAVAILABLE;
>> +#endif
>> +
>> + return res;
>> +}
> You're inventing your own "this feature isn't compiled in" protocol. We
> already got too many of them. Let's stick to one of the existing ones,
> namely the one that's got some visibility in introspection. Bonus:
> turns the semantic conflict with Marc-André's work into a textual
> conflict. Here's how:
> * Add your commands to qmp_unregister_commands_hack() in monitor.c,
> roughly like this:
> #ifndef CONFIG_INSTRUMENT
> qmp_unregister_command(&qmp_commands, "instr-load");
> qmp_unregister_command(&qmp_commands, "instr-unload");
> #endif
> * Compile qmp.c only when CONFIG_INSTRUMENT, just like the other .c
> files there, except for cmdline.c. Drop the ifdeffery there.
> * Add stubbed out command handlers to stubs/instrument.c, roughly like
> this:
> InstrLoadResult *qmp_instr_load(const char *path,
> bool have_args, strList *args,
> Error **errp)
> {
> error_setg(errp, QERR_UNSUPPORTED);
> return NULL;
> }
> Same for qmp_instr_unload().
> These stubs must exist for the link to succeed, but they won't be
> called. They'll go away when Marc-André's work lands.
> * In the next patch, make the HMP command conditional on
> CONFIG_INSTRUMENT, just like trace-file is conditional on
> CONFIG_TRACE_SIMPLE.
> Questions?
Crystal clear, applied.
> You're also inventing your own "command failed" protocol. I'll explain
> in my review of instrument.json.
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 802ea53d00..5e343be9ff 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -90,6 +90,9 @@
>> # QAPI introspection
>> { 'include': 'qapi/introspect.json' }
>>
>> +# Instrumentation commands
>> +{ 'include': 'qapi/instrument.json' }
>> +
>> ##
>> # = QMP commands
>> ##
>> diff --git a/qapi/instrument.json b/qapi/instrument.json
>> new file mode 100644
>> index 0000000000..ea63fae309
>> --- /dev/null
>> +++ b/qapi/instrument.json
>> @@ -0,0 +1,96 @@
>> +# *-*- Mode: Python -*-*
>> +#
>> +# QAPI 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.
> Make the title a doc comment, like this:
> # *-*- 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.
> ##
> The ## around the title make it go into generated
> docs/interop/qemu-qmp-ref.* documentation.
>> +
>> +##
>> +# @InstrLoadCode:
>> +#
>> +# Result code of an 'instr-load' command.
>> +#
>> +# @ok: Correctly loaded.
>> +# @too-many: Tried to load too many instrumentation libraries.
>> +# @error: The library's main() function returned a non-zero value.
>> +# @dlerror: Error with libdl (see 'msg').
>> +# @unavailable: Service not available.
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'enum': 'InstrLoadCode',
>> + 'data': [ 'ok', 'too-many', 'error', 'dlerror', 'unavailable' ] }
>> +
>> +##
>> +# @InstrLoadResult:
>> +#
>> +# Result of an 'instr-load' command.
>> +#
>> +# @code: Result code.
>> +# @msg: Additional error message (for human consumption only; present only in
>> +# case of error).
>> +# @handle: Instrumentation library identifier (present only if successful).
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'struct': 'InstrLoadResult',
>> + 'data': { 'code': 'InstrLoadCode', '*msg': 'str', '*handle': 'int' } }
>> +
>> +##
>> +# @instr-load:
>> +#
>> +# Load an instrumentation library.
>> +#
>> +# @path: path to the dynamic instrumentation library
>> +# @args: arguments to the dynamic instrumentation library
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'command': 'instr-load',
>> + 'data': { 'path': 'str', '*args': ['str'] },
>> + 'returns': 'InstrLoadResult' }
> No :)
> If the command fails, it must send an error response instead success
> response. Yours always sends a success response.
> As far as I can tell, instr-load returns a handle on success, always,
> and nothing else. Therefore:
> { 'struct': 'InstrLoadResult',
> 'data': { 'handle': 'int' } }
> On error, qmp_instr_load() should set a suitable error with error_setg()
> and return NULL.
> QAPI enum type InstrLoadCode goes away.
> On returning a handle: we commonly let the user specify an ID string
> instead. For instance, device_add doesn't return a handle you can pass
> to device_del. Instead, it takes a string-valued 'id' parameter. Other
> commands, such as device_del, can refer to the device by that ID. Is
> there any particular reason why you can't stick to this convention for
> instrumentation?
Done too :)
>> +
>> +
>> +##
>> +# @InstrUnloadCode:
>> +#
>> +# Result code of an 'instr-unload' command.
>> +#
>> +# @ok: Correctly unloaded.
>> +# @invalid: Invalid handle.
>> +# @dlerror: Error with libdl (see 'msg').
>> +# @unavailable: Service not available.
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'enum': 'InstrUnloadCode',
>> + 'data': [ 'ok', 'invalid', 'dlerror', 'unavailable' ] }
>> +
>> +##
>> +# @InstrUnloadResult:
>> +#
>> +# Result of an 'instr-unload' command.
>> +#
>> +# @code: Result code.
>> +# @msg: Additional error message (for human consumption only; present only in
>> +# case of error).
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'struct': 'InstrUnloadResult',
>> + 'data': { 'code': 'InstrUnloadCode', '*msg': 'str' } }
>> +
>> +##
>> +# @instr-unload:
>> +#
>> +# Unload an instrumentation library.
>> +#
>> +# @handle: Instrumentation library identifier (see #InstrLoadResult).
>> +#
>> +# Since: 2.11
>> +##
>> +{ 'command': 'instr-unload',
>> + 'data': { 'handle': 'int' },
>> + 'returns': 'InstrUnloadResult' }
> Likewise. instr-unload returns nothing on success. Both QAPI types go
> away.
Thanks a lot!
Lluis
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v4 08/20] instrument: [hmp] Add library loader
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (6 preceding siblings ...)
2017-09-06 17:50 ` [Qemu-devel] [PATCH v4 07/20] instrument: [qapi] Add " Lluís Vilanova
@ 2017-09-06 17:55 ` Lluís Vilanova
2017-09-07 8:51 ` Markus Armbruster
2017-09-06 17:59 ` [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control interface Lluís Vilanova
` (14 subsequent siblings)
22 siblings, 1 reply; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 17:55 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi,
Dr. David Alan Gilbert, Markus Armbruster
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
hmp-commands.hx | 28 ++++++++++++++++++++++++++
monitor.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 88 insertions(+)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 1941e19932..703d7262f5 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1858,6 +1858,34 @@ ETEXI
.sub_table = info_cmds,
},
+ {
+ .name = "instr-load",
+ .args_type = "path:F,args:s?",
+ .params = "path [arg]",
+ .help = "load an instrumentation library",
+ .cmd = hmp_instr_load,
+ },
+
+STEXI
+@item instr-load @var{path} [args=value[,...]]
+@findex instr-load
+Load an instrumentation library.
+ETEXI
+
+ {
+ .name = "instr-unload",
+ .args_type = "handle:i",
+ .params = "handle",
+ .help = "unload an instrumentation library",
+ .cmd = hmp_instr_unload,
+ },
+
+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 e0f880107f..8a7684f860 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2319,6 +2319,66 @@ int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp)
return fd;
}
+static void hmp_instr_load(Monitor *mon, const QDict *qdict)
+{
+ const char *path = qdict_get_str(qdict, "path");
+ const char *str = qdict_get_try_str(qdict, "args");
+ strList args;
+ args.value = (str == NULL) ? NULL : (char *)str;
+ args.next = NULL;
+ InstrLoadResult *res = qmp_instr_load(path, args.value != NULL,
+ args.value != NULL ? &args : NULL,
+ NULL);
+ switch (res->code) {
+ case INSTR_LOAD_CODE_OK:
+ monitor_printf(mon, "Handle: %"PRId64"\n", res->handle);
+ monitor_printf(mon, "OK\n");
+ break;
+ case INSTR_LOAD_CODE_TOO_MANY:
+ monitor_printf(mon, "Too many instrumentation libraries already loaded\n");
+ break;
+ case INSTR_LOAD_CODE_ERROR:
+ monitor_printf(mon, "Instrumentation library returned a non-zero value during initialization");
+ break;
+ case INSTR_LOAD_CODE_DLERROR:
+ monitor_printf(mon, "Error loading library: %s\n", res->msg);
+ break;
+ case INSTR_LOAD_CODE_UNAVAILABLE:
+ monitor_printf(mon, "Service not available\n");
+ break;
+ default:
+ fprintf(stderr, "Unknown instrumentation load code: %d", res->code);
+ exit(1);
+ break;
+ }
+ qapi_free_InstrLoadResult(res);
+}
+
+static void hmp_instr_unload(Monitor *mon, const QDict *qdict)
+{
+ int64_t handle = qdict_get_int(qdict, "handle");
+ InstrUnloadResult *res = qmp_instr_unload(handle, NULL);
+ switch (res->code) {
+ case INSTR_UNLOAD_CODE_OK:
+ monitor_printf(mon, "OK\n");
+ break;
+ case INSTR_UNLOAD_CODE_INVALID:
+ monitor_printf(mon, "Invalid handle\n");
+ break;
+ case INSTR_UNLOAD_CODE_DLERROR:
+ monitor_printf(mon, "Error unloading library: %s\n", res->msg);
+ break;
+ case INSTR_UNLOAD_CODE_UNAVAILABLE:
+ monitor_printf(mon, "Service not available\n");
+ break;
+ default:
+ fprintf(stderr, "Unknown instrumentation unload code: %d", res->code);
+ exit(1);
+ break;
+ }
+ qapi_free_InstrUnloadResult(res);
+}
+
/* Please update hmp-commands.hx when adding or changing commands */
static mon_cmd_t info_cmds[] = {
#include "hmp-commands-info.h"
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v4 08/20] instrument: [hmp] Add library loader
2017-09-06 17:55 ` [Qemu-devel] [PATCH v4 08/20] instrument: [hmp] " Lluís Vilanova
@ 2017-09-07 8:51 ` Markus Armbruster
2017-09-10 22:07 ` Lluís Vilanova
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2017-09-07 8:51 UTC (permalink / raw)
To: Lluís Vilanova
Cc: qemu-devel, Emilio G. Cota, Dr. David Alan Gilbert,
Stefan Hajnoczi, Eric Blake
Lluís Vilanova <vilanova@ac.upc.edu> writes:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> hmp-commands.hx | 28 ++++++++++++++++++++++++++
> monitor.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 88 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 1941e19932..703d7262f5 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1858,6 +1858,34 @@ ETEXI
> .sub_table = info_cmds,
> },
>
> + {
> + .name = "instr-load",
> + .args_type = "path:F,args:s?",
> + .params = "path [arg]",
> + .help = "load an instrumentation library",
> + .cmd = hmp_instr_load,
> + },
> +
> +STEXI
> +@item instr-load @var{path} [args=value[,...]]
> +@findex instr-load
> +Load an instrumentation library.
> +ETEXI
> +
> + {
> + .name = "instr-unload",
> + .args_type = "handle:i",
> + .params = "handle",
> + .help = "unload an instrumentation library",
> + .cmd = hmp_instr_unload,
> + },
> +
> +STEXI
> +@item instr-unload
> +@findex instr-unload
> +Unload an instrumentation library.
> +ETEXI
> +
> STEXI
> @end table
> ETEXI
Want #ifdef CONFIG_INSTRUMENT, see my review of the previous patch.
See also my remark there on returning handles vs. passing in IDs.
> diff --git a/monitor.c b/monitor.c
> index e0f880107f..8a7684f860 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2319,6 +2319,66 @@ int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp)
> return fd;
> }
>
> +static void hmp_instr_load(Monitor *mon, const QDict *qdict)
> +{
> + const char *path = qdict_get_str(qdict, "path");
> + const char *str = qdict_get_try_str(qdict, "args");
> + strList args;
Blank line between last declaration and first statement, please.
> + args.value = (str == NULL) ? NULL : (char *)str;
What's wrong with
args.value = (char *)str;
?
> + args.next = NULL;
> + InstrLoadResult *res = qmp_instr_load(path, args.value != NULL,
> + args.value != NULL ? &args : NULL,
> + NULL);
> + switch (res->code) {
> + case INSTR_LOAD_CODE_OK:
> + monitor_printf(mon, "Handle: %"PRId64"\n", res->handle);
> + monitor_printf(mon, "OK\n");
> + break;
> + case INSTR_LOAD_CODE_TOO_MANY:
> + monitor_printf(mon, "Too many instrumentation libraries already loaded\n");
> + break;
> + case INSTR_LOAD_CODE_ERROR:
> + monitor_printf(mon, "Instrumentation library returned a non-zero value during initialization");
> + break;
> + case INSTR_LOAD_CODE_DLERROR:
> + monitor_printf(mon, "Error loading library: %s\n", res->msg);
> + break;
> + case INSTR_LOAD_CODE_UNAVAILABLE:
> + monitor_printf(mon, "Service not available\n");
> + break;
> + default:
> + fprintf(stderr, "Unknown instrumentation load code: %d", res->code);
> + exit(1);
Impossible conditions should be assertion failures, but it's a moot point
because...
> + break;
> + }
> + qapi_free_InstrLoadResult(res);
> +}
With qmp_instr_load() fixed to set an error on failure, this becomes
something like
InstrLoadResult *res = qmp_instr_load(path, args.value != NULL,
args.value != NULL ? &args : NULL,
&err);
if (err) {
error_report_err(err);
} else {
monitor_printf(mon, "Handle: %"PRId64"\n", res->handle);
monitor_printf(mon, "OK\n");
}
qapi_free_InstrLoadResult(res);
> +
> +static void hmp_instr_unload(Monitor *mon, const QDict *qdict)
> +{
> + int64_t handle = qdict_get_int(qdict, "handle");
> + InstrUnloadResult *res = qmp_instr_unload(handle, NULL);
> + switch (res->code) {
> + case INSTR_UNLOAD_CODE_OK:
> + monitor_printf(mon, "OK\n");
> + break;
> + case INSTR_UNLOAD_CODE_INVALID:
> + monitor_printf(mon, "Invalid handle\n");
> + break;
> + case INSTR_UNLOAD_CODE_DLERROR:
> + monitor_printf(mon, "Error unloading library: %s\n", res->msg);
> + break;
> + case INSTR_UNLOAD_CODE_UNAVAILABLE:
> + monitor_printf(mon, "Service not available\n");
> + break;
> + default:
> + fprintf(stderr, "Unknown instrumentation unload code: %d", res->code);
> + exit(1);
> + break;
> + }
> + qapi_free_InstrUnloadResult(res);
> +}
> +
> /* Please update hmp-commands.hx when adding or changing commands */
> static mon_cmd_t info_cmds[] = {
> #include "hmp-commands-info.h"
Likewise.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v4 08/20] instrument: [hmp] Add library loader
2017-09-07 8:51 ` Markus Armbruster
@ 2017-09-10 22:07 ` Lluís Vilanova
0 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-10 22:07 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Emilio G. Cota, Dr. David Alan Gilbert,
Stefan Hajnoczi, Eric Blake
Markus Armbruster writes:
> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> hmp-commands.hx | 28 ++++++++++++++++++++++++++
>> monitor.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 88 insertions(+)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 1941e19932..703d7262f5 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1858,6 +1858,34 @@ ETEXI
>> .sub_table = info_cmds,
>> },
>>
>> + {
>> + .name = "instr-load",
>> + .args_type = "path:F,args:s?",
>> + .params = "path [arg]",
>> + .help = "load an instrumentation library",
>> + .cmd = hmp_instr_load,
>> + },
>> +
>> +STEXI
>> +@item instr-load @var{path} [args=value[,...]]
>> +@findex instr-load
>> +Load an instrumentation library.
>> +ETEXI
>> +
>> + {
>> + .name = "instr-unload",
>> + .args_type = "handle:i",
>> + .params = "handle",
>> + .help = "unload an instrumentation library",
>> + .cmd = hmp_instr_unload,
>> + },
>> +
>> +STEXI
>> +@item instr-unload
>> +@findex instr-unload
>> +Unload an instrumentation library.
>> +ETEXI
>> +
>> STEXI
>> @end table
>> ETEXI
> Want #ifdef CONFIG_INSTRUMENT, see my review of the previous patch.
> See also my remark there on returning handles vs. passing in IDs.
>> diff --git a/monitor.c b/monitor.c
>> index e0f880107f..8a7684f860 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2319,6 +2319,66 @@ int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp)
>> return fd;
>> }
>>
>> +static void hmp_instr_load(Monitor *mon, const QDict *qdict)
>> +{
>> + const char *path = qdict_get_str(qdict, "path");
>> + const char *str = qdict_get_try_str(qdict, "args");
>> + strList args;
> Blank line between last declaration and first statement, please.
>> + args.value = (str == NULL) ? NULL : (char *)str;
> What's wrong with
> args.value = (char *)str;
> ?
>> + args.next = NULL;
>> + InstrLoadResult *res = qmp_instr_load(path, args.value != NULL,
>> + args.value != NULL ? &args : NULL,
>> + NULL);
>> + switch (res->code) {
>> + case INSTR_LOAD_CODE_OK:
>> + monitor_printf(mon, "Handle: %"PRId64"\n", res->handle);
>> + monitor_printf(mon, "OK\n");
>> + break;
>> + case INSTR_LOAD_CODE_TOO_MANY:
>> + monitor_printf(mon, "Too many instrumentation libraries already loaded\n");
>> + break;
>> + case INSTR_LOAD_CODE_ERROR:
>> + monitor_printf(mon, "Instrumentation library returned a non-zero value during initialization");
>> + break;
>> + case INSTR_LOAD_CODE_DLERROR:
>> + monitor_printf(mon, "Error loading library: %s\n", res->msg);
>> + break;
>> + case INSTR_LOAD_CODE_UNAVAILABLE:
>> + monitor_printf(mon, "Service not available\n");
>> + break;
>> + default:
>> + fprintf(stderr, "Unknown instrumentation load code: %d", res->code);
>> + exit(1);
> Impossible conditions should be assertion failures, but it's a moot point
> because...
>> + break;
>> + }
>> + qapi_free_InstrLoadResult(res);
>> +}
> With qmp_instr_load() fixed to set an error on failure, this becomes
> something like
> InstrLoadResult *res = qmp_instr_load(path, args.value != NULL,
> args.value != NULL ? &args : NULL,
> &err);
> if (err) {
> error_report_err(err);
> } else {
> monitor_printf(mon, "Handle: %"PRId64"\n", res->handle);
> monitor_printf(mon, "OK\n");
> }
> qapi_free_InstrLoadResult(res);
>> +
>> +static void hmp_instr_unload(Monitor *mon, const QDict *qdict)
>> +{
>> + int64_t handle = qdict_get_int(qdict, "handle");
>> + InstrUnloadResult *res = qmp_instr_unload(handle, NULL);
>> + switch (res->code) {
>> + case INSTR_UNLOAD_CODE_OK:
>> + monitor_printf(mon, "OK\n");
>> + break;
>> + case INSTR_UNLOAD_CODE_INVALID:
>> + monitor_printf(mon, "Invalid handle\n");
>> + break;
>> + case INSTR_UNLOAD_CODE_DLERROR:
>> + monitor_printf(mon, "Error unloading library: %s\n", res->msg);
>> + break;
>> + case INSTR_UNLOAD_CODE_UNAVAILABLE:
>> + monitor_printf(mon, "Service not available\n");
>> + break;
>> + default:
>> + fprintf(stderr, "Unknown instrumentation unload code: %d", res->code);
>> + exit(1);
>> + break;
>> + }
>> + qapi_free_InstrUnloadResult(res);
>> +}
>> +
>> /* Please update hmp-commands.hx when adding or changing commands */
>> static mon_cmd_t info_cmds[] = {
>> #include "hmp-commands-info.h"
> Likewise.
Done, thanks!
Lluis
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control interface
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (7 preceding siblings ...)
2017-09-06 17:55 ` [Qemu-devel] [PATCH v4 08/20] instrument: [hmp] " Lluís Vilanova
@ 2017-09-06 17:59 ` Lluís Vilanova
2017-09-06 21:23 ` Emilio G. Cota
2017-09-06 21:57 ` Emilio G. Cota
2017-09-06 18:03 ` [Qemu-devel] [PATCH v4 10/20] instrument: Add support for tracing events Lluís Vilanova
` (13 subsequent siblings)
22 siblings, 2 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 17:59 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Lluís Vilanova,
Paolo Bonzini
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
Makefile | 5 +++
configure | 1 +
instrument/Makefile.objs | 2 +
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 | 43 +++++++++++++++++++++++++++
instrument/qemu-instr/visibility.h | 58 ++++++++++++++++++++++++++++++++++++
stubs/Makefile.objs | 1 +
stubs/instrument.c | 13 ++++++++
14 files changed, 309 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
create mode 100644 instrument/qemu-instr/visibility.h
create mode 100644 stubs/instrument.c
diff --git a/Makefile b/Makefile
index 81447b1f08..6171661458 100644
--- a/Makefile
+++ b/Makefile
@@ -589,6 +589,11 @@ 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/"
+ $(INSTALL_DATA) $(SRC_PATH)/instrument/qemu-instr/visibility.h "$(DESTDIR)$(includedir)/qemu-instr/"
+endif
install-datadir:
$(INSTALL_DIR) "$(DESTDIR)$(qemu_datadir)"
diff --git a/configure b/configure
index 05bd7b1950..3673fc9058 100755
--- a/configure
+++ b/configure
@@ -6038,6 +6038,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/instrument/Makefile.objs b/instrument/Makefile.objs
index 13a8f60431..9b7e1c03aa 100644
--- a/instrument/Makefile.objs
+++ b/instrument/Makefile.objs
@@ -3,3 +3,5 @@
target-obj-y += cmdline.o
target-obj-$(CONFIG_INSTRUMENT) += load.o
target-obj-y += qmp.o
+
+target-obj-$(CONFIG_INSTRUMENT) += control.o
diff --git a/instrument/control.c b/instrument/control.c
new file mode 100644
index 0000000000..2c2781beeb
--- /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 "instrument/qemu-instr/visibility.h"
+
+__thread InstrState instr_cur_state;
+
+
+qi_fini_fn instr_event__fini_fn;
+void *instr_event__fini_data;
+
+QI_VPUBLIC 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 a57401102a..e180f03429 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"
@@ -105,8 +107,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;
@@ -136,6 +141,14 @@ InstrUnloadError instr_unload(int64_t handle_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..f6e289daa0
--- /dev/null
+++ b/instrument/qemu-instr/control.h
@@ -0,0 +1,43 @@
+/*
+ * 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.
+ */
+void qi_set_fini(qi_fini_fn fn, void *data);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* QI__CONTROL_H */
diff --git a/instrument/qemu-instr/visibility.h b/instrument/qemu-instr/visibility.h
new file mode 100644
index 0000000000..305dddf7d8
--- /dev/null
+++ b/instrument/qemu-instr/visibility.h
@@ -0,0 +1,58 @@
+/*
+ * Macros for symbol visibility.
+ *
+ * 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 of QEMU.
+ */
+
+#ifndef QI__VISIBILITY_H
+#define QI__VISIBILITY_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * SECTION:visibility
+ * @section_id: qi-visibility
+ * @title: Symbol visibility
+ *
+ * This code is taken from http://gcc.gnu.org/wiki/Visibility.
+ */
+
+/**
+ * QI_VPUBLIC:
+ *
+ * Make an element public to user's instrumentation code.
+ */
+
+/**
+ * QI_VLOCAL:
+ *
+ * Make an element not visible to user's instrumentation code.
+ */
+
+#if defined _WIN32 || defined __CYGWIN__
+ #ifdef __GNUC__
+ #define QI_VPUBLIC __attribute__ ((dllimport))
+ #else
+ #define QI_VPUBLIC __declspec(dllimport)
+ #endif
+ #define QI_VLOCAL
+#else
+ #if __GNUC__ >= 4
+ #define QI_VPUBLIC __attribute__ ((visibility ("default")))
+ #define QI_VLOCAL __attribute__ ((visibility ("hidden")))
+ #else
+ #define QI_VPUBLIC
+ #define QI_VLOCAL
+ #endif
+#endif
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* QI__VISIBILITY_H */
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index e69c217aff..3aaec2d9dd 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..6731710fd5
--- /dev/null
+++ b/stubs/instrument.c
@@ -0,0 +1,13 @@
+/*
+ * 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/control.h"
+
+
+__thread InstrState instr_cur_state;
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control interface
2017-09-06 17:59 ` [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control interface Lluís Vilanova
@ 2017-09-06 21:23 ` Emilio G. Cota
2017-09-10 22:15 ` Lluís Vilanova
2017-09-06 21:57 ` Emilio G. Cota
1 sibling, 1 reply; 39+ messages in thread
From: Emilio G. Cota @ 2017-09-06 21:23 UTC (permalink / raw)
To: Lluís Vilanova
Cc: qemu-devel, Eric Blake, Stefan Hajnoczi, Paolo Bonzini
On Wed, Sep 06, 2017 at 20:59:02 +0300, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> Makefile | 5 +++
> configure | 1 +
> instrument/Makefile.objs | 2 +
> 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 +++++++
Am I the only one who finds this control vs. events division confusing?
Also, do we need all these many files, even for the public API?
And why the .inc's?
Thanks,
E.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control interface
2017-09-06 21:23 ` Emilio G. Cota
@ 2017-09-10 22:15 ` Lluís Vilanova
0 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-10 22:15 UTC (permalink / raw)
To: Emilio G. Cota; +Cc: qemu-devel, Eric Blake, Stefan Hajnoczi, Paolo Bonzini
Emilio G Cota writes:
> On Wed, Sep 06, 2017 at 20:59:02 +0300, Lluís Vilanova wrote:
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> Makefile | 5 +++
>> configure | 1 +
>> instrument/Makefile.objs | 2 +
>> 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 +++++++
> Am I the only one who finds this control vs. events division confusing?
Control is only for controlling instrumentation, and the header is used mainly
inside the instrumentation directory. Wheread the events header is later going
to be included in every file that needs to trigger an instrumentation event.
> Also, do we need all these many files, even for the public API?
The only other header, error.h, is later used in many other files.
> And why the .inc's?
To keep tidy headers with documentation, and the implementation details stashed
away on a separate file (like in the case of trace/).
> Thanks,
> E.
Cheers,
Lluis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control interface
2017-09-06 17:59 ` [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control interface Lluís Vilanova
2017-09-06 21:23 ` Emilio G. Cota
@ 2017-09-06 21:57 ` Emilio G. Cota
2017-09-10 23:28 ` Lluís Vilanova
1 sibling, 1 reply; 39+ messages in thread
From: Emilio G. Cota @ 2017-09-06 21:57 UTC (permalink / raw)
To: Lluís Vilanova
Cc: qemu-devel, Eric Blake, Stefan Hajnoczi, Paolo Bonzini
On Wed, Sep 06, 2017 at 20:59:02 +0300, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
(snip)
> +QI_VPUBLIC 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);
> +}
Why are these QI_VPUBLIC attributes here? Those are useful for DSO's, not
for executables --by using -rdynamic, all non-static symbols in the
executable are already visible.
> diff --git a/instrument/control.h b/instrument/control.h
> new file mode 100644
> index 0000000000..f2b085f69b
> --- /dev/null
> +++ b/instrument/control.h
(snip)
> + * 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;
I find this unnecessarily ugly for the little gain we get, i.e. asserts against
calling API code from QEMU.. seems unlikely to me (although admittedly I think
the qemu-internal API is unnecessarily complex/verbose, so maybe
you're better off with these checks).
(snip)
> +/**
> + * 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)
This isn't enough to decide whether to call instrumentation, especially for
TCG. We need TB's to know what to call, and update that mask with async
work, just like we do with tracing. Check out my alternative patchset.
Also, a single function pointer cannot work for more than one plugin. But
I see you have an XXX when there's more than one plugin, so it's OK for now.
I used RCU lists for this, which at least gives you a time in the future
at which things become visible/invisible by other threads -- this is important
when unloading an instrumenter, since you don't want to clear important stuff
(e.g. dlclose) before you're sure no further callbacks to it are possible.
[no, the atomic_acquire/release isn't enough!]
(snip)
> diff --git a/instrument/load.c b/instrument/load.c
> index a57401102a..e180f03429 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"
> @@ -105,8 +107,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;
> @@ -136,6 +141,14 @@ InstrUnloadError instr_unload(int64_t handle_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);
> +
Is fini really that useful? Doesn't the tool just die with QEMU once QEMU exits?
At the end of the day, the tool could register its own atexit hook.
E.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control interface
2017-09-06 21:57 ` Emilio G. Cota
@ 2017-09-10 23:28 ` Lluís Vilanova
0 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-10 23:28 UTC (permalink / raw)
To: Emilio G. Cota; +Cc: qemu-devel, Eric Blake, Stefan Hajnoczi, Paolo Bonzini
Emilio G Cota writes:
> On Wed, Sep 06, 2017 at 20:59:02 +0300, Lluís Vilanova wrote:
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
> (snip)
>> +QI_VPUBLIC 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);
>> +}
> Why are these QI_VPUBLIC attributes here? Those are useful for DSO's, not
> for executables --by using -rdynamic, all non-static symbols in the
> executable are already visible.
That's because I'm using -rdynamic, -fvisibility=hidden and these attributes all
together to limit what symbols are available to dlopen()'ed libs (new series
version will have the visibility compiler flag I forgot to send).
>> diff --git a/instrument/control.h b/instrument/control.h
>> new file mode 100644
>> index 0000000000..f2b085f69b
>> --- /dev/null
>> +++ b/instrument/control.h
> (snip)
>> + * 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;
> I find this unnecessarily ugly for the little gain we get, i.e. asserts against
> calling API code from QEMU.. seems unlikely to me (although admittedly I think
> the qemu-internal API is unnecessarily complex/verbose, so maybe
> you're better off with these checks).
It ensures only QEMU's threads now calling a library function are allowed to use
the public API. In a later patch, it also ensures that TCG-generating functions
(e.g., qi_event_gen_guest_mem_before_exec) can only be called from
translation-time library functions.
> (snip)
>> +/**
>> + * 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)
> This isn't enough to decide whether to call instrumentation, especially for
> TCG. We need TB's to know what to call, and update that mask with async
> work, just like we do with tracing. Check out my alternative patchset.
What do you mean by TB's need to know what to call?
> Also, a single function pointer cannot work for more than one plugin. But
> I see you have an XXX when there's more than one plugin, so it's OK for now.
> I used RCU lists for this, which at least gives you a time in the future
> at which things become visible/invisible by other threads -- this is important
> when unloading an instrumenter, since you don't want to clear important stuff
> (e.g. dlclose) before you're sure no further callbacks to it are possible.
> [no, the atomic_acquire/release isn't enough!]
In principle, this together with the API checks above and kicking all vCPUs to
flush TBs before unloading a library (I'm still looking at last bit) should
ensure libs can be safely unloaded. This should work for guest code events, but
I still need to look at other events like vCPU hotplug (is it asynchronous?), or
other events we might want triggered outside the vCPU loops.
Flushing all TBs is also on my todo to support immediate event availability when
loading a library and setting a translation-time callback.
Also, I'm not sure if we should support the complexity and performance penalty
of more than one library at a time. Right now, the QAPI commands expose support
for more than one just for future-proofing (as suggested by Richard Henderson,
if I remember correctly).
> (snip)
>> diff --git a/instrument/load.c b/instrument/load.c
>> index a57401102a..e180f03429 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"
>> @@ -105,8 +107,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;
>> @@ -136,6 +141,14 @@ InstrUnloadError instr_unload(int64_t handle_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);
>> +
> Is fini really that useful? Doesn't the tool just die with QEMU once QEMU exits?
The lib can be unloaded at any time (in softmmu mode).
> At the end of the day, the tool could register its own atexit hook.
I'm aware of that, but:
* Passing a pointer when registering the callback avoids global variables.
* I thought that qi_set_fini() would be more discoverable than
dlclose()+atexit().
Thanks!
Lluis
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v4 10/20] instrument: Add support for tracing events
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (8 preceding siblings ...)
2017-09-06 17:59 ` [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control interface Lluís Vilanova
@ 2017-09-06 18:03 ` Lluís Vilanova
2017-09-06 18:07 ` [Qemu-devel] [PATCH v4 11/20] instrument: Track vCPUs Lluís Vilanova
` (12 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 18:03 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, 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 6171661458..6f4de75f79 100644
--- a/Makefile
+++ b/Makefile
@@ -592,6 +592,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/"
$(INSTALL_DATA) $(SRC_PATH)/instrument/qemu-instr/visibility.h "$(DESTDIR)$(includedir)/qemu-instr/"
endif
diff --git a/instrument/Makefile.objs b/instrument/Makefile.objs
index 9b7e1c03aa..8258dbfa79 100644
--- a/instrument/Makefile.objs
+++ b/instrument/Makefile.objs
@@ -5,3 +5,4 @@ target-obj-$(CONFIG_INSTRUMENT) += load.o
target-obj-y += 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..a73fcdf50a
--- /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-instr/trace.h"
+#include "qemu-instr/visibility.h"
+#include "trace/control.h"
+
+
+QI_VPUBLIC
+QITraceEvent *qi_trace_event_name(const char *name)
+{
+ ERROR_IF_RET(!name, NULL, "must provide a name");
+ return (QITraceEvent *)trace_event_name(name);
+}
+
+QI_VPUBLIC
+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);
+}
+
+QI_VPUBLIC
+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_);
+}
+
+
+QI_VPUBLIC
+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_);
+}
+
+QI_VPUBLIC
+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_);
+}
+
+
+QI_VPUBLIC
+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_);
+}
+
+QI_VPUBLIC
+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_);
+}
+
+QI_VPUBLIC
+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_);
+}
+
+QI_VPUBLIC
+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_);
+}
+
+QI_VPUBLIC
+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_);
+}
+
+QI_VPUBLIC
+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);
+}
+
+QI_VPUBLIC
+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;
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 11/20] instrument: Track vCPUs
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (9 preceding siblings ...)
2017-09-06 18:03 ` [Qemu-devel] [PATCH v4 10/20] instrument: Add support for tracing events Lluís Vilanova
@ 2017-09-06 18:07 ` Lluís Vilanova
2017-09-06 21:36 ` Emilio G. Cota
2017-09-06 18:11 ` [Qemu-devel] [PATCH v4 12/20] instrument: Add event 'guest_cpu_enter' Lluís Vilanova
` (11 subsequent siblings)
22 siblings, 1 reply; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 18:07 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, 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 | 22 ++++++++++++++++++++++
instrument/control.h | 32 ++++++++++++++++++++++++++++++++
instrument/control.inc.h | 23 +++++++++++++++++++++++
4 files changed, 86 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 2c2781beeb..83453ea561 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -13,10 +13,32 @@
#include "instrument/load.h"
#include "instrument/qemu-instr/control.h"
#include "instrument/qemu-instr/visibility.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..0c37692465 100644
--- a/instrument/control.h
+++ b/instrument/control.h
@@ -10,6 +10,38 @@
#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.
+ */
+void instr_cpu_add(CPUState *vcpu);
+
+/**
+ * instr_cpu_remove:
+ *
+ * Make @vcpu unavailable to instrumentation clients.
+ */
+void instr_cpu_remove(CPUState *vcpu);
+
+/**
+ * instr_cpu_get:
+ *
+ * Get the #CPUState corresponding to the given #QICPU.
+ */
+static inline CPUState *instr_cpu_get(QICPU vcpu);
+
+/**
+ * instr_cpu_set:
+ *
+ * Get the #QICPU corresponding to the given #CPUState.
+ */
+static inline QICPU instr_cpu_set(CPUState *vcpu);
+
/**
* InstrState:
diff --git a/instrument/control.inc.h b/instrument/control.inc.h
index 0f649f4caa..18ae6a34cc 100644
--- a/instrument/control.inc.h
+++ b/instrument/control.inc.h
@@ -7,9 +7,12 @@
* 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 __thread InstrState instr_cur_state;
@@ -23,3 +26,23 @@ static inline InstrState instr_get_state(void)
{
return atomic_load_acquire(&instr_cur_state);
}
+
+
+extern unsigned int instr_cpus_count;
+extern CPUState **instr_cpus;
+
+static inline CPUState *instr_cpu_get(QICPU vcpu)
+{
+ unsigned int idx = (uintptr_t)vcpu;
+ if (idx >= instr_cpus_count) {
+ return NULL;
+ } else {
+ return instr_cpus[idx];
+ }
+}
+
+static inline QICPU instr_cpu_set(CPUState *vcpu)
+{
+ uintptr_t idx = vcpu->cpu_index;
+ return (QICPU )idx;
+}
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v4 11/20] instrument: Track vCPUs
2017-09-06 18:07 ` [Qemu-devel] [PATCH v4 11/20] instrument: Track vCPUs Lluís Vilanova
@ 2017-09-06 21:36 ` Emilio G. Cota
0 siblings, 0 replies; 39+ messages in thread
From: Emilio G. Cota @ 2017-09-06 21:36 UTC (permalink / raw)
To: Lluís Vilanova; +Cc: qemu-devel, Eric Blake, Stefan Hajnoczi
On Wed, Sep 06, 2017 at 21:07:06 +0300, Lluís Vilanova wrote:
> 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>
(snip)
> diff --git a/instrument/control.c b/instrument/control.c
> index 2c2781beeb..83453ea561 100644
> --- a/instrument/control.c
> +++ b/instrument/control.c
> @@ -13,10 +13,32 @@
> #include "instrument/load.h"
> #include "instrument/qemu-instr/control.h"
> #include "instrument/qemu-instr/visibility.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;
> +}
instr_cpus_count and instr_cpus are both modified with cpu_list_lock.
However, other readers do not acquire this same lock. This gets messy
when you try to implement something like "vcpu_for_each", which is
very useful when you load an instrumentation tool once the program
is running (otherwise the tool cannot know for sure what the vCPUs
are). This vcpu_for_each would also have to take cpu_list_lock, for
otherwise it'd miss new threads being added. As you can imagine this
gets messy pretty quickly, given that you have your own lock here.
I went with having my own hash table (a list would be fine too),
protected with the same "instr_lock" you have (i.e. the recursive mutex).
An unrelated comment: your usage of get/set/put confuses me. I'd expect those
to work on refcounted items; instead you use them for instance to hide a cast
(cpu_set) or to create/destroy (e.g. handle_get/put).
E.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v4 12/20] instrument: Add event 'guest_cpu_enter'
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (10 preceding siblings ...)
2017-09-06 18:07 ` [Qemu-devel] [PATCH v4 11/20] instrument: Track vCPUs Lluís Vilanova
@ 2017-09-06 18:11 ` Lluís Vilanova
2017-09-06 18:15 ` [Qemu-devel] [PATCH v4 13/20] instrument: Add event 'guest_cpu_exit' Lluís Vilanova
` (10 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 18:11 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Lluís Vilanova,
Paolo Bonzini
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 | 1 +
instrument/qemu-instr/control.h | 44 +++++++++++++++++++++++++++++++++++++++
stubs/instrument.c | 1 +
trace/control-target.c | 2 ++
7 files changed, 73 insertions(+)
diff --git a/instrument/control.c b/instrument/control.c
index 83453ea561..ed0d537b88 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -48,3 +48,12 @@ QI_VPUBLIC 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);
+
+QI_VPUBLIC 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..238cce9855 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_set(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 e180f03429..0fe878afa8 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -148,6 +148,7 @@ InstrUnloadError instr_unload(int64_t handle_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 f6e289daa0..26d8aa976d 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>
/**
@@ -36,6 +37,49 @@ 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.
+ *
+ * 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 6731710fd5..658a872e1c 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -11,3 +11,4 @@
__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 0056da6a46..e47361cef8 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"
@@ -147,5 +148,6 @@ void trace_init_vcpu(CPUState *vcpu)
}
}
}
+ instr_guest_cpu_enter(vcpu);
trace_guest_cpu_enter(vcpu);
}
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 13/20] instrument: Add event 'guest_cpu_exit'
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (11 preceding siblings ...)
2017-09-06 18:11 ` [Qemu-devel] [PATCH v4 12/20] instrument: Add event 'guest_cpu_enter' Lluís Vilanova
@ 2017-09-06 18:15 ` Lluís Vilanova
2017-09-06 18:19 ` [Qemu-devel] [PATCH v4 14/20] instrument: Add event 'guest_cpu_reset' Lluís Vilanova
` (9 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 18:15 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Lluís Vilanova,
Paolo Bonzini
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 +++++++++
stubs/instrument.c | 1 +
trace/control.c | 4 +++-
7 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/instrument/control.c b/instrument/control.c
index ed0d537b88..09ba682483 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -57,3 +57,12 @@ QI_VPUBLIC 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);
+
+QI_VPUBLIC 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 238cce9855..bcbf0cb32a 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_set(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 0fe878afa8..63b7d564ec 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -149,6 +149,7 @@ InstrUnloadError instr_unload(int64_t handle_id)
instr_set_event(fini_fn, NULL);
instr_set_event(guest_cpu_enter, NULL);
+ instr_set_event(guest_cpu_exit, 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 26d8aa976d..c37a380ab0 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -80,6 +80,15 @@ 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.
+ *
+ * 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 658a872e1c..ea8df6d467 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -12,3 +12,4 @@
__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);
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 14/20] instrument: Add event 'guest_cpu_reset'
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (12 preceding siblings ...)
2017-09-06 18:15 ` [Qemu-devel] [PATCH v4 13/20] instrument: Add event 'guest_cpu_exit' Lluís Vilanova
@ 2017-09-06 18:19 ` Lluís Vilanova
2017-09-06 18:23 ` [Qemu-devel] [PATCH v4 15/20] trace: Introduce a proper structure to describe memory accesses Lluís Vilanova
` (8 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 18:19 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Lluís Vilanova,
Paolo Bonzini
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 09ba682483..3cec1028e5 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -66,3 +66,12 @@ QI_VPUBLIC 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);
+
+QI_VPUBLIC 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 bcbf0cb32a..2f2cd324aa 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_set(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 63b7d564ec..d9310d1979 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -150,6 +150,7 @@ InstrUnloadError instr_unload(int64_t handle_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);
/* this should never fail */
if (dlclose(handle->dlhandle) < 0) {
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
index c37a380ab0..238ea63301 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -89,6 +89,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 4f38db0dac..d09cf583eb 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"
@@ -271,6 +272,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 ea8df6d467..74935975da 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -13,3 +13,4 @@
__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);
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 15/20] trace: Introduce a proper structure to describe memory accesses
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (13 preceding siblings ...)
2017-09-06 18:19 ` [Qemu-devel] [PATCH v4 14/20] instrument: Add event 'guest_cpu_reset' Lluís Vilanova
@ 2017-09-06 18:23 ` Lluís Vilanova
2017-09-06 18:27 ` [Qemu-devel] [PATCH v4 16/20] instrument: Add event 'guest_mem_before_trans' Lluís Vilanova
` (7 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 18:23 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Paolo Bonzini,
Peter Crosthwaite, Richard Henderson
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 87f673ef49..234e300ede 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -2664,22 +2664,26 @@ static void gen_ldst_i64(TCGOpcode opc, TCGv_i64 val, TCGv addr,
void tcg_gen_qemu_ld_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
{
+ TraceMemInfo meminfo;
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;
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;
+
if (TCG_TARGET_REG_BITS == 32 && (memop & MO_SIZE) < MO_64) {
tcg_gen_qemu_ld_i32(TCGV_LOW(val), addr, idx, memop);
if (memop & MO_SIGN) {
@@ -2691,21 +2695,23 @@ 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;
+
if (TCG_TARGET_REG_BITS == 32 && (memop & MO_SIZE) < MO_64) {
tcg_gen_qemu_st_i32(TCGV_LOW(val), addr, idx, memop);
return;
}
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"
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 16/20] instrument: Add event 'guest_mem_before_trans'
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (14 preceding siblings ...)
2017-09-06 18:23 ` [Qemu-devel] [PATCH v4 15/20] trace: Introduce a proper structure to describe memory accesses Lluís Vilanova
@ 2017-09-06 18:27 ` Lluís Vilanova
2017-09-06 18:31 ` [Qemu-devel] [PATCH v4 17/20] instrument: Add event 'guest_mem_before_exec' Lluís Vilanova
` (6 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 18:27 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Lluís Vilanova,
Paolo Bonzini, Richard Henderson
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
Makefile.target | 1 +
instrument/control.c | 13 +++++++-
instrument/control.h | 36 +++++++++++++++++++++-
instrument/control.inc.h | 16 +++++++---
instrument/events.h | 21 +++++++++++++
instrument/events.inc.h | 19 ++++++++++++
instrument/load.c | 1 +
instrument/qemu-instr/control.h | 15 +++++++++
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, 210 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 3cec1028e5..3c3875dc99 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -16,7 +16,7 @@
#include "qom/cpu.h"
-__thread InstrState instr_cur_state;
+__thread InstrInfo instr_cur_info;
unsigned int instr_cpus_count;
@@ -75,3 +75,14 @@ QI_VPUBLIC 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);
+
+QI_VPUBLIC 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");
+ instr_set_event(guest_mem_before_trans, fn);
+}
diff --git a/instrument/control.h b/instrument/control.h
index 0c37692465..d9e3dd3da6 100644
--- a/instrument/control.h
+++ b/instrument/control.h
@@ -56,12 +56,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:
@@ -70,6 +79,29 @@ static inline void instr_set_state(InstrState state);
*/
static inline InstrState instr_get_state(void);
+/**
+ * instr_tcg_set:
+ * @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_set(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 18ae6a34cc..e8224319e0 100644
--- a/instrument/control.inc.h
+++ b/instrument/control.inc.h
@@ -15,16 +15,18 @@
#include <stdint.h>
-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);
}
@@ -46,3 +48,9 @@ static inline QICPU instr_cpu_set(CPUState *vcpu)
uintptr_t idx = vcpu->cpu_index;
return (QICPU )idx;
}
+
+
+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 2f2cd324aa..2cb17049f7 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,21 @@ 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_set(vcpu_trans);
+ QITCGv_cpu vcpu_exec_ = instr_tcg_set(iinfo, 0, vcpu_exec);
+ QITCGv vaddr_ = instr_tcg_set(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 d9310d1979..d5612af452 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -151,6 +151,7 @@ InstrUnloadError instr_unload(int64_t handle_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);
/* this should never fail */
if (dlclose(handle->dlhandle) < 0) {
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
index 238ea63301..af4fda138e 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -98,6 +98,21 @@ 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 74935975da..5e0d5150b5 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -10,7 +10,9 @@
#include "instrument/control.h"
-__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 234e300ede..40de61872e 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 "trace-tcg.h"
@@ -2667,6 +2668,7 @@ void tcg_gen_qemu_ld_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
TraceMemInfo meminfo;
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);
}
@@ -2676,6 +2678,7 @@ void tcg_gen_qemu_st_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
TraceMemInfo meminfo;
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);
}
@@ -2696,6 +2699,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);
}
@@ -2711,6 +2715,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:
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 17/20] instrument: Add event 'guest_mem_before_exec'
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (15 preceding siblings ...)
2017-09-06 18:27 ` [Qemu-devel] [PATCH v4 16/20] instrument: Add event 'guest_mem_before_trans' Lluís Vilanova
@ 2017-09-06 18:31 ` Lluís Vilanova
2017-09-06 18:35 ` [Qemu-devel] [PATCH v4 18/20] instrument: Add event 'guest_user_syscall' Lluís Vilanova
` (5 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 18:31 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Paolo Bonzini,
Peter Crosthwaite, Richard Henderson, 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 | 35 +++++++++++++++++++++++++++++
instrument/control.h | 15 ++++++++++++
instrument/events.h | 5 ++++
instrument/events.inc.h | 18 ++++++++++++++-
instrument/helpers.h | 1 +
instrument/load.c | 1 +
instrument/qemu-instr/control.h | 21 +++++++++++++++++
stubs/instrument.c | 2 ++
13 files changed, 108 insertions(+), 1 deletion(-)
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 3c3875dc99..f39e81d7c7 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -14,6 +14,8 @@
#include "instrument/qemu-instr/control.h"
#include "instrument/qemu-instr/visibility.h"
#include "qom/cpu.h"
+#include "exec/helper-proto.h"
+#include "exec/helper-gen.h"
__thread InstrInfo instr_cur_info;
@@ -86,3 +88,36 @@ QI_VPUBLIC void qi_event_set_guest_mem_before_trans(
ERROR_IF(!instr_get_state(), "called outside instrumentation");
instr_set_event(guest_mem_before_trans, fn);
}
+
+
+QI_VPUBLIC 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");
+ InstrInfo *iinfo = &instr_cur_info;
+ TCGv_env vcpu_ = instr_tcg_get(iinfo, vcpu);
+ TCGv vaddr_ = instr_tcg_get(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);
+
+QI_VPUBLIC 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");
+ instr_set_event(guest_mem_before_exec, fn);
+}
diff --git a/instrument/control.h b/instrument/control.h
index d9e3dd3da6..19c9c9fb98 100644
--- a/instrument/control.h
+++ b/instrument/control.h
@@ -54,6 +54,7 @@ static inline QICPU instr_cpu_set(CPUState *vcpu);
typedef enum {
INSTR_STATE_DISABLE,
INSTR_STATE_ENABLE,
+ INSTR_STATE_ENABLE_TCG,
} InstrState;
#define INSTR_MAX_TCG_REGS 16
@@ -102,6 +103,20 @@ static inline InstrState instr_get_state(void);
*/
static inline void instr_tcg_count(InstrInfo *info, unsigned int count);
+/**
+ * instr_tcg_get:
+ * @info: Pointer to #InstrInfo.
+ * @arg: QITCG register.
+ *
+ * Get a suitable TCGv* from a QITCGv* value.
+ */
+#define instr_tcg_get(info, arg) \
+ ({ \
+ unsigned int idx = (uintptr_t)arg; \
+ ERROR_IF(info->max <= idx, "invalid QITCGv register"); \
+ info->tcg_regs[idx]; \
+ })
+
#include "instrument/control.inc.h"
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 2cb17049f7..d7a3065ac1 100644
--- a/instrument/events.inc.h
+++ b/instrument/events.inc.h
@@ -50,7 +50,7 @@ static inline void instr_guest_mem_before_trans(
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);
+ InstrInfo *iinfo = instr_set_state(INSTR_STATE_ENABLE_TCG);
QICPU vcpu_trans_ = instr_cpu_set(vcpu_trans);
QITCGv_cpu vcpu_exec_ = instr_tcg_set(iinfo, 0, vcpu_exec);
QITCGv vaddr_ = instr_tcg_set(iinfo, 1, vaddr);
@@ -61,3 +61,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_set(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..957cdc0993
--- /dev/null
+++ b/instrument/helpers.h
@@ -0,0 +1 @@
+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 d5612af452..1df660d5d1 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -152,6 +152,7 @@ InstrUnloadError instr_unload(int64_t handle_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);
/* this should never fail */
if (dlclose(handle->dlhandle) < 0) {
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
index af4fda138e..4fa99a968d 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -113,6 +113,27 @@ void qi_event_set_guest_cpu_reset(void (*fn)(QICPU vcpu));
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 5e0d5150b5..c6c279c85e 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -16,3 +16,5 @@ 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);
+void (*instr_event__guest_mem_before_exec)(
+ QICPU vcpu_trans, QITCGv_cpu vcpu_exec, QITCGv vaddr, QIMemInfo info);
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 18/20] instrument: Add event 'guest_user_syscall'
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (16 preceding siblings ...)
2017-09-06 18:31 ` [Qemu-devel] [PATCH v4 17/20] instrument: Add event 'guest_mem_before_exec' Lluís Vilanova
@ 2017-09-06 18:35 ` Lluís Vilanova
2017-09-06 18:39 ` [Qemu-devel] [PATCH v4 19/20] instrument: Add event 'guest_user_syscall_ret' Lluís Vilanova
` (4 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 18:35 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Lluís Vilanova,
Riku Voipio, Laurent Vivier, Paolo Bonzini
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
bsd-user/syscall.c | 3 +++
instrument/control.c | 14 ++++++++++++++
instrument/events.h | 7 +++++++
instrument/events.inc.h | 16 ++++++++++++++++
instrument/load.c | 1 +
instrument/qemu-instr/control.h | 15 +++++++++++++++
linux-user/syscall.c | 1 +
stubs/instrument.c | 3 +++
8 files changed, 60 insertions(+)
diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
index 3230f722f3..43f8887529 100644
--- a/bsd-user/syscall.c
+++ b/bsd-user/syscall.c
@@ -324,6 +324,7 @@ 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 +424,7 @@ 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 +501,7 @@ 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 f39e81d7c7..7e84dadf24 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -121,3 +121,17 @@ QI_VPUBLIC void qi_event_set_guest_mem_before_exec(
ERROR_IF(!instr_get_state(), "called outside instrumentation");
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);
+
+QI_VPUBLIC 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");
+ 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 d7a3065ac1..9c64497533 100644
--- a/instrument/events.inc.h
+++ b/instrument/events.inc.h
@@ -77,3 +77,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_set(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 1df660d5d1..d977049082 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -153,6 +153,7 @@ InstrUnloadError instr_unload(int64_t handle_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);
/* this should never fail */
if (dlclose(handle->dlhandle) < 0) {
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
index 4fa99a968d..cba8ade54e 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -134,6 +134,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..c0c33d4a75 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7723,6 +7723,7 @@ 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 c6c279c85e..dbd8b1438d 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -18,3 +18,6 @@ void (*instr_event__guest_mem_before_trans)(
QICPU vcpu_trans, QITCGv_cpu vcpu_exec, QITCGv vaddr, QIMemInfo info);
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);
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 19/20] instrument: Add event 'guest_user_syscall_ret'
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (17 preceding siblings ...)
2017-09-06 18:35 ` [Qemu-devel] [PATCH v4 18/20] instrument: Add event 'guest_user_syscall' Lluís Vilanova
@ 2017-09-06 18:39 ` Lluís Vilanova
2017-09-06 18:43 ` [Qemu-devel] [PATCH v4 20/20] instrument: Add API to manipulate guest memory Lluís Vilanova
` (3 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 18:39 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, Lluís Vilanova,
Riku Voipio, Laurent Vivier, Paolo Bonzini
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
bsd-user/syscall.c | 3 +++
instrument/control.c | 11 +++++++++++
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, 49 insertions(+)
diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
index 43f8887529..dcfb31f436 100644
--- a/bsd-user/syscall.c
+++ b/bsd-user/syscall.c
@@ -406,6 +406,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:
@@ -483,6 +484,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:
@@ -560,6 +562,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 7e84dadf24..f8f368efb0 100644
--- a/instrument/control.c
+++ b/instrument/control.c
@@ -135,3 +135,14 @@ QI_VPUBLIC void qi_event_set_guest_user_syscall(
ERROR_IF(!instr_get_state(), "called outside instrumentation");
instr_set_event(guest_user_syscall, fn);
}
+
+
+void (*instr_event__guest_user_syscall_ret)(
+ QICPU vcpu, uint64_t num, uint64_t ret);
+
+QI_VPUBLIC 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");
+ 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 9c64497533..ca1d789cea 100644
--- a/instrument/events.inc.h
+++ b/instrument/events.inc.h
@@ -93,3 +93,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_set(vcpu);
+ (*cb)(vcpu_, num, ret);
+ instr_set_state(INSTR_STATE_DISABLE);
+ }
+}
diff --git a/instrument/load.c b/instrument/load.c
index d977049082..decd357105 100644
--- a/instrument/load.c
+++ b/instrument/load.c
@@ -154,6 +154,7 @@ InstrUnloadError instr_unload(int64_t handle_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);
/* this should never fail */
if (dlclose(handle->dlhandle) < 0) {
diff --git a/instrument/qemu-instr/control.h b/instrument/qemu-instr/control.h
index cba8ade54e..fd83c86c2b 100644
--- a/instrument/qemu-instr/control.h
+++ b/instrument/qemu-instr/control.h
@@ -149,6 +149,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 c0c33d4a75..0f86b6935d 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -12397,6 +12397,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 dbd8b1438d..7c171dcee0 100644
--- a/stubs/instrument.c
+++ b/stubs/instrument.c
@@ -21,3 +21,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);
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v4 20/20] instrument: Add API to manipulate guest memory
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (18 preceding siblings ...)
2017-09-06 18:39 ` [Qemu-devel] [PATCH v4 19/20] instrument: Add event 'guest_user_syscall_ret' Lluís Vilanova
@ 2017-09-06 18:43 ` Lluís Vilanova
2017-09-06 20:59 ` [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Emilio G. Cota
` (2 subsequent siblings)
22 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-06 18:43 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Emilio G. Cota, Stefan Hajnoczi, 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 | 72 ++++++++++++++++++++++++++++
3 files changed, 177 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 8258dbfa79..c9bc4e75f4 100644
--- a/instrument/Makefile.objs
+++ b/instrument/Makefile.objs
@@ -6,3 +6,4 @@ target-obj-y += 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..4bf99b4882
--- /dev/null
+++ b/instrument/state.c
@@ -0,0 +1,72 @@
+/*
+ * 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 "cpu.h"
+#include "exec/cpu-all.h"
+#include "instrument/control.h"
+#include "instrument/error.h"
+#include "instrument/qemu-instr/state.h"
+#include "instrument/qemu-instr/visibility.h"
+
+
+QI_VPUBLIC bool qi_mem_read_virt(QICPU vcpu, uint64_t vaddr,
+ size_t size, void *buf)
+{
+ CPUState *vcpu_ = instr_cpu_get(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;
+}
+
+QI_VPUBLIC bool qi_mem_write_virt(QICPU vcpu, uint64_t vaddr,
+ size_t size, void *buf)
+{
+ CPUState *vcpu_ = instr_cpu_get(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;
+}
+
+QI_VPUBLIC bool qi_mem_virt_to_phys(QICPU vcpu, uint64_t vaddr, uint64_t *paddr)
+{
+ CPUState *vcpu_ = instr_cpu_get(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
+}
+
+QI_VPUBLIC 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
+}
+
+QI_VPUBLIC 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
+}
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (19 preceding siblings ...)
2017-09-06 18:43 ` [Qemu-devel] [PATCH v4 20/20] instrument: Add API to manipulate guest memory Lluís Vilanova
@ 2017-09-06 20:59 ` Emilio G. Cota
2017-09-10 17:40 ` Lluís Vilanova
2017-09-10 23:31 ` Lluís Vilanova
2017-09-07 7:45 ` Markus Armbruster
2017-09-07 10:58 ` Markus Armbruster
22 siblings, 2 replies; 39+ messages in thread
From: Emilio G. Cota @ 2017-09-06 20:59 UTC (permalink / raw)
To: Lluís Vilanova; +Cc: qemu-devel, Eric Blake, Stefan Hajnoczi
On Wed, Sep 06, 2017 at 20:22:41 +0300, Lluís Vilanova wrote:
> This series adds an API to add instrumentation events.
>
> It also provides additional APIs for:
> * Controlling tracing events
hmm didn't Stefan say that tracing should be decoupled from this?
> * Peek/poke guest memory
>
> There's still missing APIs for (can be added in later series?):
> * Provide something like tracing's per-vCPU trace states (i.e., so that each
> vCPU can have different instrumentation code). 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).
As I said in the description of my alternative implementation [*], I don't see
much value in having per-vCPU events, as most instrumenters just care about
the guest application/system. I can only think of cases where the instrumenter
is only interested in a guest process (in system-mode), but that'd be ugly
anyway (would need to change both QEMU and the guest kernel to track the pid).
If the need ever arises, we could add __vcpu(cpu_index) registration functions.
[*] https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01446.html
E.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation
2017-09-06 20:59 ` [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Emilio G. Cota
@ 2017-09-10 17:40 ` Lluís Vilanova
2017-09-10 23:31 ` Lluís Vilanova
1 sibling, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-10 17:40 UTC (permalink / raw)
To: Emilio G. Cota; +Cc: qemu-devel, Eric Blake, Stefan Hajnoczi
Emilio G Cota writes:
> On Wed, Sep 06, 2017 at 20:22:41 +0300, Lluís Vilanova wrote:
>> This series adds an API to add instrumentation events.
>>
>> It also provides additional APIs for:
>> * Controlling tracing events
> hmm didn't Stefan say that tracing should be decoupled from this?
>> * Peek/poke guest memory
>>
>> There's still missing APIs for (can be added in later series?):
>> * Provide something like tracing's per-vCPU trace states (i.e., so that each
>> vCPU can have different instrumentation code). 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).
> As I said in the description of my alternative implementation [*], I don't see
> much value in having per-vCPU events, as most instrumenters just care about
> the guest application/system. I can only think of cases where the instrumenter
> is only interested in a guest process (in system-mode), but that'd be ugly
> anyway (would need to change both QEMU and the guest kernel to track the pid).
Yes, that's exactly what I was using it for (track guest processes in softmmu
mode).
> If the need ever arises, we could add __vcpu(cpu_index) registration functions.
You still need the functionality I cited if you're using
"qi_event_gen_guest_mem_before_exec" or similar.
> [*] https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01446.html
> E.
Cheers,
Lluis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation
2017-09-06 20:59 ` [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Emilio G. Cota
2017-09-10 17:40 ` Lluís Vilanova
@ 2017-09-10 23:31 ` Lluís Vilanova
1 sibling, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-10 23:31 UTC (permalink / raw)
To: Emilio G. Cota; +Cc: qemu-devel, Eric Blake, Stefan Hajnoczi
Emilio G Cota writes:
> On Wed, Sep 06, 2017 at 20:22:41 +0300, Lluís Vilanova wrote:
>> This series adds an API to add instrumentation events.
>>
>> It also provides additional APIs for:
>> * Controlling tracing events
> hmm didn't Stefan say that tracing should be decoupled from this?
I understood decoupling instr from the tracing infrastructure (since tracing
events are defined as not stable, and instr must be stable by definition).
>> * Peek/poke guest memory
>>
>> There's still missing APIs for (can be added in later series?):
>> * Provide something like tracing's per-vCPU trace states (i.e., so that each
>> vCPU can have different instrumentation code). 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).
> As I said in the description of my alternative implementation [*], I don't see
> much value in having per-vCPU events, as most instrumenters just care about
> the guest application/system. I can only think of cases where the instrumenter
> is only interested in a guest process (in system-mode), but that'd be ugly
> anyway (would need to change both QEMU and the guest kernel to track the pid).
> If the need ever arises, we could add __vcpu(cpu_index) registration functions.
> [*] https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01446.html
Sorry, your series slipped my radar. I'll take a look at it.
Thanks,
Lluis
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (20 preceding siblings ...)
2017-09-06 20:59 ` [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Emilio G. Cota
@ 2017-09-07 7:45 ` Markus Armbruster
2017-09-07 10:58 ` Markus Armbruster
22 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2017-09-07 7:45 UTC (permalink / raw)
To: Lluís Vilanova; +Cc: qemu-devel, Emilio G. Cota, Stefan Hajnoczi
Lluís Vilanova <vilanova@ac.upc.edu> writes:
> This series adds an API to add instrumentation events.
>
> It also provides additional APIs for:
> * Controlling tracing events
> * Peek/poke guest memory
>
> There's still missing APIs for (can be added in later series?):
> * Provide something like tracing's per-vCPU trace states (i.e., so that each
> vCPU can have different instrumentation code). 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).
> * Peek/poke guest registers
>
> The instrumentation code is dynamically loaded as a library into QEMU either
> when it starts or later using its remote control interfaces.
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
Doesn't apply to current master, and appears to have whitespace errors.
Please rebase and use scripts/checkpatch.pl to tidy up.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation
2017-09-06 17:22 [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation Lluís Vilanova
` (21 preceding siblings ...)
2017-09-07 7:45 ` Markus Armbruster
@ 2017-09-07 10:58 ` Markus Armbruster
2017-09-07 14:21 ` Emilio G. Cota
22 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2017-09-07 10:58 UTC (permalink / raw)
To: Lluís Vilanova; +Cc: qemu-devel, Emilio G. Cota, Stefan Hajnoczi
Lluís Vilanova <vilanova@ac.upc.edu> writes:
> This series adds an API to add instrumentation events.
>
> It also provides additional APIs for:
> * Controlling tracing events
> * Peek/poke guest memory
>
> There's still missing APIs for (can be added in later series?):
> * Provide something like tracing's per-vCPU trace states (i.e., so that each
> vCPU can have different instrumentation code). 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).
> * Peek/poke guest registers
>
> The instrumentation code is dynamically loaded as a library into QEMU either
> when it starts or later using its remote control interfaces.
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
Taking a step back.
This looks like a way to dynamically load arbitrary code. What
interfaces can this code use? Your cover letter should answer this.
As long as the answer is "everything the dynamic linker is willing to
resolve", this series heading nowhere. We can talk about an interface
for plugins, but "anything goes" is not on the menu.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation
2017-09-07 10:58 ` Markus Armbruster
@ 2017-09-07 14:21 ` Emilio G. Cota
2017-09-10 17:34 ` Lluís Vilanova
0 siblings, 1 reply; 39+ messages in thread
From: Emilio G. Cota @ 2017-09-07 14:21 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Lluís Vilanova, qemu-devel, Stefan Hajnoczi
On Thu, Sep 07, 2017 at 12:58:05 +0200, Markus Armbruster wrote:
> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>
> > This series adds an API to add instrumentation events.
> >
> > It also provides additional APIs for:
> > * Controlling tracing events
> > * Peek/poke guest memory
> >
> > There's still missing APIs for (can be added in later series?):
> > * Provide something like tracing's per-vCPU trace states (i.e., so that each
> > vCPU can have different instrumentation code). 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).
> > * Peek/poke guest registers
> >
> > The instrumentation code is dynamically loaded as a library into QEMU either
> > when it starts or later using its remote control interfaces.
> >
> > Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>
> Taking a step back.
>
> This looks like a way to dynamically load arbitrary code. What
> interfaces can this code use? Your cover letter should answer this.
>
> As long as the answer is "everything the dynamic linker is willing to
> resolve", this series heading nowhere. We can talk about an interface
> for plugins, but "anything goes" is not on the menu.
A simple solution to this is to only export the API symbols by passing
--dynamic-file to the linker -- see patch 2 of the following series for an
example (ELF-only, although I'm pretty sure this can be achieved on Windows
as well):
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01446.html
Thanks,
Emilio
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v4 00/20] instrument: Add basic event instrumentation
2017-09-07 14:21 ` Emilio G. Cota
@ 2017-09-10 17:34 ` Lluís Vilanova
0 siblings, 0 replies; 39+ messages in thread
From: Lluís Vilanova @ 2017-09-10 17:34 UTC (permalink / raw)
To: Emilio G. Cota; +Cc: Markus Armbruster, qemu-devel, Stefan Hajnoczi
Emilio G Cota writes:
> On Thu, Sep 07, 2017 at 12:58:05 +0200, Markus Armbruster wrote:
>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>
>> > This series adds an API to add instrumentation events.
>> >
>> > It also provides additional APIs for:
>> > * Controlling tracing events
>> > * Peek/poke guest memory
>> >
>> > There's still missing APIs for (can be added in later series?):
>> > * Provide something like tracing's per-vCPU trace states (i.e., so that each
>> > vCPU can have different instrumentation code). 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).
>> > * Peek/poke guest registers
>> >
>> > The instrumentation code is dynamically loaded as a library into QEMU either
>> > when it starts or later using its remote control interfaces.
>> >
>> > Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>
>> Taking a step back.
>>
>> This looks like a way to dynamically load arbitrary code. What
>> interfaces can this code use? Your cover letter should answer this.
>>
>> As long as the answer is "everything the dynamic linker is willing to
>> resolve", this series heading nowhere. We can talk about an interface
>> for plugins, but "anything goes" is not on the menu.
> A simple solution to this is to only export the API symbols by passing
> --dynamic-file to the linker -- see patch 2 of the following series for an
> example (ELF-only, although I'm pretty sure this can be achieved on Windows
> as well):
> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01446.html
Sorry, I adapted the instr series in a rush and missed your series. I'm now
preparing a new version that fixes this without the flags you mention (missed to
add a line in this series).
Thanks,
Lluis
^ permalink raw reply [flat|nested] 39+ messages in thread