Re: [PATCH v5] arch/x86: port I/O tracing on x86

[Greg KH]

On Sat, Oct 07, 2023 at 11:56:53AM -0600, Dan Raymond wrote:
> Add support for port I/O tracing on x86.  Memory mapped I/O tracing is
> available on x86 via CONFIG_MMIOTRACE but that relies on page faults
> so it doesn't work with port I/O.  This feature uses tracepoints in a
> similar manner as CONFIG_TRACE_MMIO_ACCESS.
> 
> Signed-off-by: Dan Raymond <raymod2@xxxxxxxxx>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

This is now outside of my subsystems to review, sorry.  It's going to
have to go through the x86 tree, so you are going to have to convince
them that this is something that actually matters and is needed by
people, as maintaining it over time is going to add to their workload.

Note, you are keeping tracing from working in a few areas that might not
be good:

> --- a/arch/x86/boot/Makefile
> +++ b/arch/x86/boot/Makefile
> @@ -70,6 +70,7 @@ KBUILD_CFLAGS	:= $(REALMODE_CFLAGS) -D_SETUP
>  KBUILD_AFLAGS	:= $(KBUILD_CFLAGS) -D__ASSEMBLY__
>  KBUILD_CFLAGS	+= $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
>  KBUILD_CFLAGS	+= -fno-asynchronous-unwind-tables
> +KBUILD_CFLAGS	+= -DDISABLE_TRACEPOINTS
>  GCOV_PROFILE := n
>  UBSAN_SANITIZE := n
>  
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 35ce1a64068b..c368bcc008eb 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -51,6 +51,7 @@ KBUILD_CFLAGS += -D__DISABLE_EXPORTS
>  # Disable relocation relaxation in case the link is not PIE.
>  KBUILD_CFLAGS += $(call as-option,-Wa$(comma)-mrelax-relocations=no)
>  KBUILD_CFLAGS += -include $(srctree)/include/linux/hidden.h
> +KBUILD_CFLAGS += -DDISABLE_TRACEPOINTS
>  
>  # sev.c indirectly inludes inat-table.h which is generated during
>  # compilation and stored in $(objtree). Add the directory to the includes so

Now I know why you did that for this patch (i.e. so early boot doesn't
get the printk mess), but that kind of defeats the use of tracepoints at
all for this part of the kernel, is that ok?  Are any existing
tracepoints now removed?

Some other random comments:

> --- a/arch/x86/include/asm/cmpxchg_32.h
> +++ b/arch/x86/include/asm/cmpxchg_32.h
> @@ -37,13 +37,16 @@ static inline void set_64bit(volatile u64 *ptr, u64 value)
>  
>  #ifdef CONFIG_X86_CMPXCHG64
>  #define arch_cmpxchg64(ptr, o, n)					\
> -	((__typeof__(*(ptr)))__cmpxchg64((ptr), (unsigned long long)(o), \
> +	((__typeof__(*(ptr)))__cmpxchg64((unsigned long long *)(ptr),	\
> +					 (unsigned long long)(o),	\
>  					 (unsigned long long)(n)))
> -#define arch_cmpxchg64_local(ptr, o, n)					\
> -	((__typeof__(*(ptr)))__cmpxchg64_local((ptr), (unsigned long long)(o), \
> +#define arch_cmpxchg64_local(ptr, o, n)						\
> +	((__typeof__(*(ptr)))__cmpxchg64_local((unsigned long long *)(ptr),	\
> +					       (unsigned long long)(o),		\
>  					       (unsigned long long)(n)))
> -#define arch_try_cmpxchg64(ptr, po, n)					\
> -	__try_cmpxchg64((ptr), (unsigned long long *)(po), \
> +#define arch_try_cmpxchg64(ptr, po, n)			\
> +	__try_cmpxchg64((unsigned long long *)(ptr),	\
> +			(unsigned long long *)(po),	\
>  			(unsigned long long)(n))
>  #endif
>  

Why are these needed to be changed at all?  What code changes with it,
and it's not mentioned in the changelog, so why is it required?

> diff --git a/arch/x86/include/asm/shared/io.h b/arch/x86/include/asm/shared/io.h
> index c0ef921c0586..82664956ce41 100644
> --- a/arch/x86/include/asm/shared/io.h
> +++ b/arch/x86/include/asm/shared/io.h
> @@ -2,13 +2,20 @@
>  #ifndef _ASM_X86_SHARED_IO_H
>  #define _ASM_X86_SHARED_IO_H
>  
> +#include <linux/tracepoint-defs.h>
> +#include <linux/trace_portio.h>
>  #include <linux/types.h>
>  
> +DECLARE_TRACEPOINT(portio_write);
> +DECLARE_TRACEPOINT(portio_read);
> +
>  #define BUILDIO(bwl, bw, type)						\
>  static inline void __out##bwl(type value, u16 port)			\
>  {									\
>  	asm volatile("out" #bwl " %" #bw "0, %w1"			\
>  		     : : "a"(value), "Nd"(port));			\
> +	if (tracepoint_enabled(portio_write))				\
> +		do_trace_portio_write(value, port, #bwl[0]);		\

Your level of indirection here seems deep, why doesn't
do_trace_portio_write() belong in a .h file here and do the
tracepoint_enabled() check?

Is this a by-product of the tracing macros that require this deeper
callchain to happen?

>  }									\
>  									\
>  static inline type __in##bwl(u16 port)					\
> @@ -16,6 +23,8 @@ static inline type __in##bwl(u16 port)					\
>  	type value;							\
>  	asm volatile("in" #bwl " %w1, %" #bw "0"			\
>  		     : "=a"(value) : "Nd"(port));			\
> +	if (tracepoint_enabled(portio_read))				\
> +		do_trace_portio_read(value, port, #bwl[0]);		\
>  	return value;							\
>  }
>  
> diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
> index f76747862bd2..254f223c025d 100644
> --- a/arch/x86/lib/Makefile
> +++ b/arch/x86/lib/Makefile
> @@ -40,6 +40,7 @@ $(obj)/inat.o: $(obj)/inat-tables.c
>  clean-files := inat-tables.c
>  
>  obj-$(CONFIG_SMP) += msr-smp.o cache-smp.o
> +obj-$(CONFIG_TRACEPOINTS) += trace_portio.o

So you are always enabling these if any CONFIG_TRACEPOINTS is enabled?
That seems brave.

> --- a/arch/x86/realmode/rm/Makefile
> +++ b/arch/x86/realmode/rm/Makefile
> @@ -75,5 +75,6 @@ KBUILD_CFLAGS	:= $(REALMODE_CFLAGS) -D_SETUP -D_WAKEUP \
>  		   -I$(srctree)/arch/x86/boot
>  KBUILD_AFLAGS	:= $(KBUILD_CFLAGS) -D__ASSEMBLY__
>  KBUILD_CFLAGS	+= -fno-asynchronous-unwind-tables
> +KBUILD_CFLAGS	+= -DDISABLE_TRACEPOINTS

Again, you prevent any tracepoints from this code chunk, is that going
to be ok going forward?

>  GCOV_PROFILE := n
>  UBSAN_SANITIZE := n
> diff --git a/include/linux/trace_portio.h b/include/linux/trace_portio.h
> new file mode 100644
> index 000000000000..2324d62e6c9e
> --- /dev/null
> +++ b/include/linux/trace_portio.h
> @@ -0,0 +1,6 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <linux/types.h>
> +
> +extern void do_trace_portio_read(u32 value, u16 port, char width);
> +extern void do_trace_portio_write(u32 value, u16 port, char width);

Nit, "extern" isn't needed in .h files anymore.  Not a big deal, just
for other work you do going forward.

> diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> index e7c2276be33e..bfe70e17b2aa 100644
> --- a/include/linux/tracepoint-defs.h
> +++ b/include/linux/tracepoint-defs.h
> @@ -80,7 +80,7 @@ struct bpf_raw_event_map {
>  #define DECLARE_TRACEPOINT(tp) \
>  	extern struct tracepoint __tracepoint_##tp
>  
> -#ifdef CONFIG_TRACEPOINTS
> +#if defined(CONFIG_TRACEPOINTS) && !defined(DISABLE_TRACEPOINTS)

Why this global change?

Anyway, it's up to the x86 maintainers now, good luck!

But personally, I don't see the real need for this at all.  It's a
debugging thing for what exactly?  Who needs this?  Who will use it?
When will they use it?  And why?

thanks,

greg k-h