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

[Lluís Vilanova]

Stefan Hajnoczi writes:

> 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?

As I said, we can require instrumentation libraries to be GPL (and nothing
else), so no proprietary code is possible without a license violation (then
we're on the same field as if someone ships a modified QEMU binary, which
technically is just as doable).

As for use-cases, see Peter's email and my response.


Cheers,
  Lluis