Message-ID: <20250514133815.78bc2599@gandalf.local.home>
Date: Wed, 14 May 2025 13:38:15 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Vincent Donnefort <vdonnefort@...gle.com>
Cc: mhiramat@...nel.org, mathieu.desnoyers@...icios.com,
 linux-trace-kernel@...r.kernel.org, maz@...nel.org, oliver.upton@...ux.dev,
 joey.gouly@....com, suzuki.poulose@....com, yuzenghui@...wei.com,
 kvmarm@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 jstultz@...gle.com, qperret@...gle.com, will@...nel.org,
 kernel-team@...roid.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 00/24] Tracefs support for pKVM

On Tue,  6 May 2025 17:47:56 +0100
Vincent Donnefort <vdonnefort@...gle.com> wrote:

> The growing set of features supported by the hypervisor in protected
> mode necessitates debugging and profiling tools. Tracefs is the
> ideal candidate for this task:
> 
>   * It is simple to use and to script.
> 
>   * It is supported by various tools, from the trace-cmd CLI to the
>     Android web-based perfetto.
> 
>   * The ring-buffer, where are stored trace events consists of linked
>     pages, making it an ideal structure for sharing between kernel and
>     hypervisor.
> 
> This series first introduces a new generic way of creating remote events and
> remote buffers. Then it adds support to the pKVM hypervisor.
> 
> 1. ring-buffer
> --------------
> 
> To setup the per-cpu ring-buffers, a new interface is created:
> 
>   ring_buffer_remote:	Describes what the kernel needs to know about the
> 			remote writer, that is, the set of pages forming the
> 			ring-buffer and a callback for the reader/head
> 			swapping (enables consuming read)
> 
>   ring_buffer_remote():	Creates a read-only ring-buffer from a
> 			ring_buffer_remote.
> 
> To keep the internals of `struct ring_buffer` in sync with the remote,
> the meta-page is used. It was originally introduced to enable user-space
> mapping of the ring-buffer [1]. In this case, the kernel is not the
> producer anymore but the reader. The function to read that meta-page is:
> 
>   ring_buffer_poll_remote():
> 			Update `struct ring_buffer` based on the remote
> 			meta-page. Wake-up readers if necessary.
> 
> The kernel has to poll the meta-page to be notified of newly written
> events.
> 
> 2. Tracefs
> ----------
> 
> This series introduce a new trace_remote that does the link between
> tracefs and the remote ring-buffer.
> 
> The interface is found in the remotes/ directory at the root of the
> tracefs mount point. Each remote is like an instance and you'll find
> there a subset of the regular Tracefs user-space interface:
> 
>   remotes/test/
>      buffer_size_kb
>      trace_clock
>      trace_pipe
>      trace
>      per_cpu/
>              cpuX/
>                  trace
>                  trace_pipe
>      events/
> 
>             test/
>                 selftest/
>                           enable
>                           id
> 
> Behind the scenes, kernel/trace/trace_remote.c creates this tracefs
> hierarchy without relying on kernel/trace/trace.c. This is due to
> fundamental differences:
> 
>   * Remote tracing doesn't support trace_array's system-specific
>     features (snapshots, tracers, etc.).
> 
>   * Logged event formats differ (e.g., no PID for remote events).
> 
>   * Buffer operations require specific remote interactions.
> 
> 3. Simple Ring-Buffer
> ---------------------
> 
> As the current ring-buffer.c implementation has too many dependencies to
> be used directly by the pKVM hypervisor. A new simple implementation is
> created and can be found in kernel/trace/simple-ring-buffer.c.
> 
> This implementation is write-only and is used by both the pKVM
> hypervisor and a trace_remote test module.
> 
> 4. Events
> ---------
> 
> A new REMOTE_EVENT() macro is added to simplify the creation of events
> on the kernel side. As remote tracing buffer are read only, only the
> event structure and a way of printing must be declared. The prototype of
> the macro is very similar to the well-known TRACE_EVENT()
> 
>  REMOTE_EVENT(my_event, id,
>      RE_STRUCT(
>          re_field(u64, foobar)
>      ),
>      RE_PRINTK("foobar=%lld", __entry->foobar)
>      )
>   )
> 
> 5. pKVM
> -------
> 
> The pKVM support simply creates a "hypervisor" trace_remote on the
> kernel side and inherits from simple-ring-buffer.c on the hypervisor
> side.
> 
> A new event macro is created HYP_EVENT() that is under the hood re-using
> REMOTE_EVENT() (defined in the previous paragaph) as well as generate
> hypervisor specific struct and trace_<event>() functions.
> 
> 5. Limitations:
> ---------------
> 
> Non-consuming reading of the buffer isn't supported (i.e. cat trace ->
> -EPERM) due to current the lack of support in the ring-buffer meta-page.
> 
> [1] https://tracingsummit.org/ts/2022/hypervisortracing/
> [2] https://lore.kernel.org/all/20240510140435.3550353-1-vdonnefort@google.com/
> 

BTW,  I tried to build this series and it fails.

  CALL    /work/git/test-linux.git/scripts/checksyscalls.sh
  CC      kernel/trace/simple_ring_buffer.o
In file included from ./arch/x86/include/generated/asm/rwonce.h:1,
                 from /work/git/test-linux.git/include/linux/compiler.h:390,
                 from /work/git/test-linux.git/arch/x86/include/asm/atomic.h:5,
                 from /work/git/test-linux.git/include/linux/atomic.h:7,
                 from /work/git/test-linux.git/kernel/trace/simple_ring_buffer.c:7:
/work/git/test-linux.git/kernel/trace/simple_ring_buffer.c: In function ‘simple_rb_move_tail’:
/work/git/test-linux.git/include/asm-generic/rwonce.h:55:37: error: assignment to ‘struct list_head *’ from ‘long unsigned int’ makes pointer from integer without a cast [-Wint-conversion]
   55 |         *(volatile typeof(x) *)&(x) = (val);                            \
      |                                     ^
/work/git/test-linux.git/include/asm-generic/rwonce.h:61:9: note: in expansion of macro ‘__WRITE_ONCE’
   61 |         __WRITE_ONCE(x, val);                                           \
      |         ^~~~~~~~~~~~
/work/git/test-linux.git/arch/x86/include/asm/barrier.h:63:9: note: in expansion of macro ‘WRITE_ONCE’
   63 |         WRITE_ONCE(*p, v);                                              \
      |         ^~~~~~~~~~
/work/git/test-linux.git/include/asm-generic/barrier.h:172:55: note: in expansion of macro ‘__smp_store_release’
  172 | #define smp_store_release(p, v) do { kcsan_release(); __smp_store_release(p, v); } while (0)
      |                                                       ^~~~~~~~~~~~~~~~~~~
/work/git/test-linux.git/kernel/trace/simple_ring_buffer.c:129:17: note: in expansion of macro ‘smp_store_release’
  129 |                 smp_store_release(&new_tail->list.next,
      |                 ^~~~~~~~~~~~~~~~~
make[5]: *** [/work/git/test-linux.git/scripts/Makefile.build:203: kernel/trace/simple_ring_buffer.o] Error 1
make[4]: *** [/work/git/test-linux.git/scripts/Makefile.build:461: kernel/trace] Error 2
make[3]: *** [/work/git/test-linux.git/scripts/Makefile.build:461: kernel] Error 2
make[2]: *** [/work/git/test-linux.git/Makefile:2004: .] Error 2
make[1]: *** [/work/git/test-linux.git/Makefile:248: __sub-make] Error 2
make[1]: Leaving directory '/work/build/trace/nobackup/debiantesting-x86-64'

Even when I fixed this, it then failed with the building of the sample module.

I think you need something like:

obj-$(CONFIG_TRACE_REMOTE_TEST) += remote_test_mod.o

remote_test_mod-y := simple_ring_buffer.o remote_test.o trace_remote.o

If the module needs more than one object file. Then the module should be
called something that doesn't have a .c file and use that name with ".o" to
add all the objects.

I think this could work, but this still had issues with functions not exported.

-- Steve

Powered by blists - more mailing lists