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

From: Stefan Hajnoczi <stefanha@gmail.com>
To: "Lluís Vilanova" <vilanova@ac.upc.edu>
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 14:19:31 +0100	[thread overview]
Message-ID: <20170725131931.GC23343@stefanha-x1.localdomain> (raw)
In-Reply-To: <150091574424.30739.4131793221953168474.stgit@frigg.lan>

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

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?

Stefan

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