On Sat, 21 Oct 2023 18:00:38 +0200
Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> 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>
BTW, It's usually common practice to send a new revision of a patch as
a new thread and not as a reply to another thread. When I go look for
patches (besides in patchwork), I only look at top level threads. If I
see a patch that has comments against it for a new revision, I might
ignore the rest of the thread to wait for the new revision. If the new
revision is a reply to the existing thread, it may never be seen.
>
> 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?
I believe everything in arch/x86/boot is not part of the kernel proper
and not having tracepoints there is fine.
>
> 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?
Agreed, if this has issues, it probably should be a separate patch.
>
> > 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?
Yeah. tracepoints cannot be in header files as the macros cause a lot
of side effects. If you need to have them in a header file, then you
need to do this little trick above. That is, use tracepoint_enabled(tp)
and then call a function that will call the tracepoint in a C file.
>
> > } \
> > \
> > 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?
I don't know this code very well, but "realmode" sounds like code
that's not going to be "normal" ;-)
>
> > 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?
Yeah, DISABLE_TRACEPOINTS does not currently exist. If this is to be a
new way to disable TRACEPOINTS it needs a separate patch and be able to
disable tracepoints everywhere (maybe include/trace/*.h files also need
to be modified?), and also be documented somewhere in Documentation/trace.
-- Steve
