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

From: "Lluís Vilanova" <vilanova@ac.upc.edu>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: qemu-devel@nongnu.org, "Emilio G. Cota" <cota@braap.org>,
	Stefan Hajnoczi <stefanha@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 00/13] instrument: Add basic event instrumentation
Date: Tue, 25 Jul 2017 17:47:08 +0300	[thread overview]
Message-ID: <87wp6wa80j.fsf@frigg.lan> (raw)
In-Reply-To: <20170725131931.GC23343@stefanha-x1.localdomain> (Stefan Hajnoczi's message of "Tue, 25 Jul 2017 14:19:31 +0100")

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 <vilanova@ac.upc.edu>

> 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).

Thanks,
  Lluis