[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 00/13] instrument: Add basic event instrumentati
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 00/13] instrument: Add basic event instrumentation |
Date: |
Wed, 26 Jul 2017 12:29:15 +0100 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
On Tue, Jul 25, 2017 at 05:47:08PM +0300, Lluís Vilanova wrote:
> Stefan Hajnoczi writes:
>
> > On Mon, Jul 24, 2017 at 08:02:24PM +0300, Lluís Vilanova wrote:
> >> This series adds a basic interface to instrument tracing events and control
> >> their tracing state.
> >>
> >> The instrumentation code is dynamically loaded into QEMU (either when it
> >> starts
> >> or later using its remote control interfaces).
> >>
> >> All events can be instrumented, but the instrumentable events must be
> >> explicitly
> >> specified at configure time.
> >>
> >> Signed-off-by: Lluís Vilanova <address@hidden>
>
> > Hi Lluís,
> > I'm concerned that the shared library interface will be abused to monkey
> > patch code into QEMU far beyond instrumentation use cases and/or avoid
> > the responsibilities of the GPL license.
>
> > Instead I suggest adding a trace backend generates calls to registered
> > "callback" functions:
>
> > $ cat >my-instrumentation.c
> > #include "trace/control.h"
>
> > static void my_cpu_in(unsigned int addr, char size, unsigned int val)
> > {
> > printf("my_cpu_in\n");
> > }
>
> > static void my_init(void)
> > {
> > trace_register_event_callback("cpu_in", my_cpu_in);
> > trace_enable_events("cpu_in");
> > }
> > trace_init(my_init);
>
> > $ ./configure --enable-trace-backends=log,callback && make -j4
>
> > This is still a clean interface that allows instrumentation code to be
> > kept separate from the trace event call sites.
>
> > The instrumentation code gets compiled into QEMU, but that shouldn't be
> > a huge burden since QEMU's Makefiles only recompile changed source
> > files (only the first build is slow).
>
> > Does this alternative sound reasonable to you?
>
> You mean to add a user-provided .c file to QEMU at compile-time? (I'm assuming
> we can keep the "user API" proposed in this series, instead of the one you
> showed).
>
> First, a user might want to provide more than just a .c, so we might have to
> accept a directory that produces a library that is included into QEMU at link
> time (a bit more complicated to do portably).
>
> Second, the user can still do the same actions you want to shield from,
> regardless of whether it's a dynamically loaded library (i.e., access any
> fuction in QEMU).
>
> What I propose to do instead is:
>
> * For the monkey-patch part, we can limit symbol resolution to the
> instrumentation API functions when loading the library (e.g., compile QEMU
> with -fvisibility=hidden).
>
> * For the license part, that is a legal issue that can be handled by the API
> header license, right? (the "public" headers I added are GPL, not
> LGPL). Besides, if only the intended API is available, I'm not sure if that
> matters (e.g., we don't care about the license of a dtrace script, since it
> only has the API provided by QEMU+dtrace).
>
> This would be similar to Linux's module support; only selected functions are
> available to modules, and we could add a license check (e.g.,
> QI_LICENSE("GPL")
> must be on the instrumentation library or it won't be loaded).
Proprietary Linux kernel modules are controversial and some still
consider them license violations - especially when an "open source" shim
module is used to interface between proprietary code and the kernel.
What is the use case for this instrumentation?
Stefan
signature.asc
Description: PGP signature
- [Qemu-devel] [PATCH 13/13] trace: Rename C++-specific names in event arguments, (continued)
- [Qemu-devel] [PATCH 13/13] trace: Rename C++-specific names in event arguments, Lluís Vilanova, 2017/07/24
- Re: [Qemu-devel] [PATCH 00/13] instrument: Add basic event instrumentation, Stefan Hajnoczi, 2017/07/25
- Re: [Qemu-devel] [PATCH 00/13] instrument: Add basic event instrumentation, Stefan Hajnoczi, 2017/07/26
- Re: [Qemu-devel] [PATCH 00/13] instrument: Add basic event instrumentation, Peter Maydell, 2017/07/26
- Re: [Qemu-devel] [PATCH 00/13] instrument: Add basic event instrumentation, Lluís Vilanova, 2017/07/26
Re: [Qemu-devel] [PATCH 00/13] instrument: Add basic event instrumentation, Lluís Vilanova, 2017/07/25