| Message ID | 150472074219.24907.5510718414753398145.stgit@frigg.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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
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
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;
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