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

From: Stefan Hajnoczi <stefanha@redhat.com>
To: Stefan Hajnoczi <stefanha@gmail.com>,
	qemu-devel@nongnu.org, "Emilio G. Cota" <cota@braap.org>
Subject: Re: [Qemu-devel] [PATCH 00/13] instrument: Add basic event instrumentation
Date: Wed, 26 Jul 2017 12:29:15 +0100	[thread overview]
Message-ID: <20170726112915.GF18489@stefanha-x1.localdomain> (raw)
In-Reply-To: <87wp6wa80j.fsf@frigg.lan>

[-- Attachment #1: Type: text/plain, Size: 3456 bytes --]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]