Re: [PATCH v4 00/24] Tracefs support for pKVM

[Vincent Donnefort <vdonnefort@google.com>]

On Wed, May 14, 2025 at 01:38:15PM -0400, Steven Rostedt wrote:
> On Tue,  6 May 2025 17:47:56 +0100
> Vincent Donnefort <vdonnefort@google.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.


Yes, appologies, I've started applying your comments today and I've figured out
I haven't tried building for x86.

I already have fixes locally for what's below.

I probably can send a v5 this week if you wish, unless you prefer to wait a bit
more for more comments?

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