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

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

[Dan Raymond]

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@gmail.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
V1 -> V2:
  - create header file for prototypes to silence new compiler warning
  - reduce CPU overhead to 2 instructions (no branching) when tracing disabled
  - fix imprecise IP logging by retrieving the IP off the stack instead of using
    compile time labels

V2 -> V3:
  - restore missing semicolon

 arch/x86/include/asm/shared/io.h | 25 ++++++++++++++++
 arch/x86/lib/Makefile            |  1 +
 arch/x86/lib/trace_portio.c      | 21 ++++++++++++++
 include/linux/trace_portio.h     |  6 ++++
 include/trace/events/portio.h    | 49 ++++++++++++++++++++++++++++++++
 5 files changed, 102 insertions(+)
 create mode 100644 arch/x86/lib/trace_portio.c
 create mode 100644 include/linux/trace_portio.h
 create mode 100644 include/trace/events/portio.h

diff --git a/arch/x86/include/asm/shared/io.h b/arch/x86/include/asm/shared/io.h
index c0ef921c0586..9e5dce1cb62d 100644
--- a/arch/x86/include/asm/shared/io.h
+++ b/arch/x86/include/asm/shared/io.h
@@ -2,13 +2,36 @@
 #ifndef _ASM_X86_SHARED_IO_H
 #define _ASM_X86_SHARED_IO_H
 
+#include <linux/trace_portio.h>
 #include <linux/types.h>
 
+/*
+ * We don't want the tracing logic included in the early boot modules (under
+ * arch/x86/boot) so we check for their include guards here.  If we don't do
+ * this we will get compiler errors.  These checks are not present in
+ * arch/x86/include/asm/msr.h which contains similar tracing logic.  That is
+ * possible only because none of the msr inline functions are instantiated in
+ * the early boot modules.  If that changes this issue will need to be addressed
+ * there as well.  Therefore it might be better to handle this centrally in
+ * tracepoint-defs.h.
+ */
+
+#if defined(CONFIG_TRACEPOINTS) && !defined(BOOT_COMPRESSED_MISC_H) && !defined(BOOT_BOOT_H)
+#include <linux/tracepoint-defs.h>
+DECLARE_TRACEPOINT(portio_write);
+DECLARE_TRACEPOINT(portio_read);
+#define _tracepoint_enabled(tracepoint) tracepoint_enabled(tracepoint)
+#else
+#define _tracepoint_enabled(tracepoint) false
+#endif
+
 #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]);		\
 }									\
 									\
 static inline type __in##bwl(u16 port)					\
@@ -16,6 +39,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
 
 lib-y := delay.o misc.o cmdline.o cpu.o
 lib-y += usercopy_$(BITS).o usercopy.o getuser.o putuser.o
diff --git a/arch/x86/lib/trace_portio.c b/arch/x86/lib/trace_portio.c
new file mode 100644
index 000000000000..c048dffcfe05
--- /dev/null
+++ b/arch/x86/lib/trace_portio.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/instruction_pointer.h>
+#include <linux/trace_portio.h>
+
+#define CREATE_TRACE_POINTS
+#include <trace/events/portio.h>
+
+void do_trace_portio_read(u32 value, u16 port, char width)
+{
+	trace_portio_read(value, port, width, _RET_IP_);
+}
+EXPORT_SYMBOL_GPL(do_trace_portio_read);
+EXPORT_TRACEPOINT_SYMBOL_GPL(portio_read);
+
+void do_trace_portio_write(u32 value, u16 port, char width)
+{
+	trace_portio_write(value, port, width, _RET_IP_);
+}
+EXPORT_SYMBOL_GPL(do_trace_portio_write);
+EXPORT_TRACEPOINT_SYMBOL_GPL(portio_write);
diff --git a/include/linux/trace_portio.h b/include/linux/trace_portio.h
new file mode 100644
index 000000000000..013418d3d2ae
--- /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);
diff --git a/include/trace/events/portio.h b/include/trace/events/portio.h
new file mode 100644
index 000000000000..3591a75a475e
--- /dev/null
+++ b/include/trace/events/portio.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM portio
+
+#if !defined(_TRACE_PORTIO_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_PORTIO_H
+
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(portio_class,
+	TP_PROTO(u32 value, u16 port, char width, long ip_addr),
+
+	TP_ARGS(value, port, width, ip_addr),
+
+	TP_STRUCT__entry(
+		__field(u32, value)
+		__field(u16, port)
+		__field(char, width)
+		__field(long, ip_addr)
+	),
+
+	TP_fast_assign(
+		__entry->value = value;
+		__entry->port = port;
+		__entry->width = width;
+		__entry->ip_addr = ip_addr;
+	),
+
+	TP_printk("port=0x%04x value=0x%0*x %pS",
+		__entry->port,
+		__entry->width == 'b' ? 2 :
+		__entry->width == 'w' ? 4 : 8,
+		__entry->value, (void *)__entry->ip_addr)
+);
+
+DEFINE_EVENT(portio_class, portio_read,
+	TP_PROTO(u32 value, u16 port, char width, long ip_addr),
+	TP_ARGS(value, port, width, ip_addr)
+);
+
+DEFINE_EVENT(portio_class, portio_write,
+	TP_PROTO(u32 value, u16 port, char width, long ip_addr),
+	TP_ARGS(value, port, width, ip_addr)
+);
+
+#endif /* _TRACE_PORTIO_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

base-commit: be8b93b5cc7d533eb8c9b0590cdac055ecafe13a
-- 
2.25.1

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

[Greg KH]

On Fri, Sep 29, 2023 at 01:15:41PM -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@gmail.com>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> V1 -> V2:
>   - create header file for prototypes to silence new compiler warning
>   - reduce CPU overhead to 2 instructions (no branching) when tracing disabled
>   - fix imprecise IP logging by retrieving the IP off the stack instead of using
>     compile time labels
> 
> V2 -> V3:
>   - restore missing semicolon
> 
>  arch/x86/include/asm/shared/io.h | 25 ++++++++++++++++
>  arch/x86/lib/Makefile            |  1 +
>  arch/x86/lib/trace_portio.c      | 21 ++++++++++++++
>  include/linux/trace_portio.h     |  6 ++++
>  include/trace/events/portio.h    | 49 ++++++++++++++++++++++++++++++++
>  5 files changed, 102 insertions(+)
>  create mode 100644 arch/x86/lib/trace_portio.c
>  create mode 100644 include/linux/trace_portio.h
>  create mode 100644 include/trace/events/portio.h
> 
> diff --git a/arch/x86/include/asm/shared/io.h b/arch/x86/include/asm/shared/io.h
> index c0ef921c0586..9e5dce1cb62d 100644
> --- a/arch/x86/include/asm/shared/io.h
> +++ b/arch/x86/include/asm/shared/io.h
> @@ -2,13 +2,36 @@
>  #ifndef _ASM_X86_SHARED_IO_H
>  #define _ASM_X86_SHARED_IO_H
>  
> +#include <linux/trace_portio.h>
>  #include <linux/types.h>
>  
> +/*
> + * We don't want the tracing logic included in the early boot modules (under
> + * arch/x86/boot) so we check for their include guards here.  If we don't do
> + * this we will get compiler errors.  These checks are not present in
> + * arch/x86/include/asm/msr.h which contains similar tracing logic.  That is
> + * possible only because none of the msr inline functions are instantiated in
> + * the early boot modules.  If that changes this issue will need to be addressed
> + * there as well.  Therefore it might be better to handle this centrally in
> + * tracepoint-defs.h.
> + */
> +
> +#if defined(CONFIG_TRACEPOINTS) && !defined(BOOT_COMPRESSED_MISC_H) && !defined(BOOT_BOOT_H)

I see what you are doing here in trying to see if a .h file has been
included already, but now you are making an assumption on both the .h
file ordering, and the #ifdef guard for those .h files, which are
something that we almost never remember or even consider when dealing
with .h files files.


> +#include <linux/tracepoint-defs.h>
> +DECLARE_TRACEPOINT(portio_write);
> +DECLARE_TRACEPOINT(portio_read);
> +#define _tracepoint_enabled(tracepoint) tracepoint_enabled(tracepoint)
> +#else
> +#define _tracepoint_enabled(tracepoint) false
> +#endif
> +
>  #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]);		\
>  }									\

Who wants/needs port tracing these days?  What types of systems still
rely on that for their primary form of I/O other than some old-school
serial ports?

The MMIO tracing was added because there are crazy out-of-tree SoC
devices out there that "insisted" that they need to hook into the mmio
access path, so they added traceing there under the auspicious of trying
to log all mmio accesses so that they could then override the access
path in the tracehook to do who-knows-what other things.  Hopefully you
are not wanting to do the same thing here as well?

And have you addressed all of Peter's previous review comments?



>  									\
>  static inline type __in##bwl(u16 port)					\
> @@ -16,6 +39,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 this is always enabled?  No separate config option?  Why not?

>  lib-y := delay.o misc.o cmdline.o cpu.o
>  lib-y += usercopy_$(BITS).o usercopy.o getuser.o putuser.o
> diff --git a/arch/x86/lib/trace_portio.c b/arch/x86/lib/trace_portio.c
> new file mode 100644
> index 000000000000..c048dffcfe05
> --- /dev/null
> +++ b/arch/x86/lib/trace_portio.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/instruction_pointer.h>
> +#include <linux/trace_portio.h>
> +
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/portio.h>
> +
> +void do_trace_portio_read(u32 value, u16 port, char width)
> +{
> +	trace_portio_read(value, port, width, _RET_IP_);
> +}
> +EXPORT_SYMBOL_GPL(do_trace_portio_read);
> +EXPORT_TRACEPOINT_SYMBOL_GPL(portio_read);
> +
> +void do_trace_portio_write(u32 value, u16 port, char width)
> +{
> +	trace_portio_write(value, port, width, _RET_IP_);
> +}
> +EXPORT_SYMBOL_GPL(do_trace_portio_write);
> +EXPORT_TRACEPOINT_SYMBOL_GPL(portio_write);
> diff --git a/include/linux/trace_portio.h b/include/linux/trace_portio.h
> new file mode 100644
> index 000000000000..013418d3d2ae
> --- /dev/null
> +++ b/include/linux/trace_portio.h
> @@ -0,0 +1,6 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */

Why "+"?  (I have to ask).

> +
> +#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);
> diff --git a/include/trace/events/portio.h b/include/trace/events/portio.h
> new file mode 100644
> index 000000000000..3591a75a475e
> --- /dev/null
> +++ b/include/trace/events/portio.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */

This is -only, which is fine, but your patch doesn't seem to be uniform
here for new files being added in the same patch, right?  So documenting
this somewhere (i.e. in the changelog), is essential.

> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM portio
> +
> +#if !defined(_TRACE_PORTIO_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_PORTIO_H
> +
> +#include <linux/tracepoint.h>
> +
> +DECLARE_EVENT_CLASS(portio_class,
> +	TP_PROTO(u32 value, u16 port, char width, long ip_addr),

Memory locations are stored in "unsigned long" not "long", right?

> +
> +	TP_ARGS(value, port, width, ip_addr),
> +
> +	TP_STRUCT__entry(
> +		__field(u32, value)
> +		__field(u16, port)
> +		__field(char, width)
> +		__field(long, ip_addr)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->value = value;
> +		__entry->port = port;
> +		__entry->width = width;
> +		__entry->ip_addr = ip_addr;
> +	),
> +
> +	TP_printk("port=0x%04x value=0x%0*x %pS",
> +		__entry->port,
> +		__entry->width == 'b' ? 2 :
> +		__entry->width == 'w' ? 4 : 8,
> +		__entry->value, (void *)__entry->ip_addr)

Logging kernel memory locations, why?  Where is this format documented?

thanks,

greg k-h

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

[Dan Raymond]

On 10/3/2023 6:50 AM, Greg KH wrote:
>> +/*
>> + * We don't want the tracing logic included in the early boot modules (under
>> + * arch/x86/boot) so we check for their include guards here.  If we don't do
>> + * this we will get compiler errors.  These checks are not present in
>> + * arch/x86/include/asm/msr.h which contains similar tracing logic.  That is
>> + * possible only because none of the msr inline functions are instantiated in
>> + * the early boot modules.  If that changes this issue will need to be addressed
>> + * there as well.  Therefore it might be better to handle this centrally in
>> + * tracepoint-defs.h.
>> + */
>> +
>> +#if defined(CONFIG_TRACEPOINTS) && !defined(BOOT_COMPRESSED_MISC_H) && !defined(BOOT_BOOT_H)
> 
> I see what you are doing here in trying to see if a .h file has been
> included already, but now you are making an assumption on both the .h
> file ordering, and the #ifdef guard for those .h files, which are
> something that we almost never remember or even consider when dealing
> with .h files files.

With one exception io.h is included from boot.h or misc.h which is where
the include guards are defined:

# find arch/x86/boot -type f -print0 | xargs -0 grep "#include.*[^a-z]io\.h"
arch/x86/boot/boot.h:#include "io.h"
arch/x86/boot/compressed/misc.h:#include "../io.h"
arch/x86/boot/compressed/tdx.c:#include "../io.h"
arch/x86/boot/io.h:#include <asm/shared/io.h>

I agree this is fragile but the problem is not confined to this patch.
If I add a call to rdmsr() or wrmsr() in arch/x86/boot/compressed/misc.c
I get the same compiler error.  It has something to do with the inline
assembly inside arch/x86/include/asm/jump_label.h.

I've copied Steven Rostedt who is the maintainer of tracefs to see if he
has any comment.  I just noticed arch/x86/boot/msr.h and I see that it
redefines rdmsr() and wrmsr() and omits the tracepoints.  A comment there
explains:

/*
 * The kernel proper already defines rdmsr()/wrmsr(), but they are not for the
 * boot kernel since they rely on tracepoint/exception handling infrastructure
 * that's not available here.
 */

We could do something similar for inb()/outb() and redefine them in
arch/x86/boot/io.h instead of including <asm/shared/io.h> there.

> Who wants/needs port tracing these days?  What types of systems still
> rely on that for their primary form of I/O other than some old-school
> serial ports?
> 
> The MMIO tracing was added because there are crazy out-of-tree SoC
> devices out there that "insisted" that they need to hook into the mmio
> access path, so they added traceing there under the auspicious of trying
> to log all mmio accesses so that they could then override the access
> path in the tracehook to do who-knows-what other things.  Hopefully you
> are not wanting to do the same thing here as well?
> 
> And have you addressed all of Peter's previous review comments?

The author of the CONFIG_TRACE_MMIO_ACCESS feature (Sai Prakash Ranjan)
stated that he uses it for debugging unclocked register accesses on ARM.
Andy Shevchenko mentioned that he has used MMIO tracing for tracing
memory-mapped UARTs on x86.  I was motivated to create this feature to
debug UART problems in an environment that runs Linux on a Celeron-based
SBC which uses port-mapped UARTs.

This feature can also be used for other things besides UARTs.  Things
like the PIT and interrupt controller use port-mapped I/O.  I work on
an embedded system that uses an ISA card cage to connect peripherals
to our SBC and all communication over the ISA bus uses port I/O.

Yes, I believe I have addressed all of Peter's review comments:

1) cannot use early_console->write()

   That is not true.  Everything that includes boot.h (ie.
   'arch/x86/boot/early_serial_console.c') will not change due to the
   '#ifdef BOOT_BOOT_H' guard.  Also tracing will not be compiled in
   unless 'CONFIG_TRACEPOINTS' is true.  Finally, tracing will be
   disabled at boot until the user mounts the tracefs and explicitly
   enables 'portio' events.  When tracing is disabled a nop
   instruction is used in lieu of a jmp instruction to avoid
   executing any trace code.

2) tracing relies on RCU which might not be alive yet and might
   itself be the problem

   RCU is not needed unless/until tracing is enabled.  I also
   demonstrated that port I/O tracing works correctly during boot by
   testing this patch with "trace_event=portio" on the command line.

>>  obj-$(CONFIG_SMP) += msr-smp.o cache-smp.o
>> +obj-$(CONFIG_TRACEPOINTS) += trace_portio.o
> 
> So this is always enabled?  No separate config option?  Why not?

The CONFIG_TRACE_MMIO_ACCESS feature has a separate config option
but it incurs a significant amount of overhead.  For example
mem32_serial_out() comprises 4 instructions (12 bytes) when
CONFIG_TRACE_MMIO_ACCESS is false.  It comprises 33 instructions
(101 bytes) when CONFIG_TRACE_MMIO_ACCESS is true and all of those
instructions are executed (including two function calls) whether
tracing is enabled or not.

This feature incurs much less overhead.  For example io_serial_out()
comprises 4 instructions (6 bytes) when CONFIG_TRACEPOINTS is false.
It comprises 14 instructions (34 bytes) when CONFIG_TRACEPOINTS is
true but only two of the extra instructions are executed when
tracing is disabled (and neither of them are branches).  The
extra instructions for tracing are marked with *** below:

c104e740 <io_serial_out>:
c104e740:  01 c2                 add    edx,eax
c104e742:  88 c8                 mov    al,cl
c104e744:  0f b7 d2          *** movzx  edx,dx                           ***
c104e747:  ee                    out    dx,al
c104e748:  3e 8d 74 26 00    *** lea    esi,ds:[esi+eiz*1+0x0]           ***
c104e74d:  c3                    ret
    
c104e74e:  66 90             *** xchg   ax,ax                            ***
c104e750:  55                *** push   ebp                              ***
c104e751:  0f b6 c1          *** movzx  eax,cl                           ***
c104e754:  89 e5             *** mov    ebp,esp                          ***
c104e756:  b9 62 00 00 00    *** mov    ecx,0x62                         ***
c104e75b:  e8 80 1e 33 00    *** call   c13805e0 <do_trace_portio_write> ***
c104e760:  5d                *** pop    ebp                              ***
c104e761:  c3                *** ret                                     ***

This is similar to the tracing overhead of rdmsr()/wrmsr() which
does not have a separate config option.

>> +/* SPDX-License-Identifier: GPL-2.0+ */
> 
> Why "+"?  (I have to ask).
> 
>> +/* SPDX-License-Identifier: GPL-2.0-only */
> 
> This is -only, which is fine, but your patch doesn't seem to be uniform
> here for new files being added in the same patch, right?  So documenting
> this somewhere (i.e. in the changelog), is essential.

I did not intend to be inconsistent.  I can make them both GPL-2.0-only if
that is preferred.

>> +DECLARE_EVENT_CLASS(portio_class,
>> +	TP_PROTO(u32 value, u16 port, char width, long ip_addr),
> 
> Memory locations are stored in "unsigned long" not "long", right?

It doesn't matter since "unsigned long" is the same size as "long".
I should use "void *" here but that will require a cast at the call
sites because for some reason _RET_IP_ is casting the pointer to
"unsigned long".

>> +	TP_printk("port=0x%04x value=0x%0*x %pS",
>> +		__entry->port,
>> +		__entry->width == 'b' ? 2 :
>> +		__entry->width == 'w' ? 4 : 8,
>> +		__entry->value, (void *)__entry->ip_addr)
> 
> Logging kernel memory locations, why?  Where is this format documented?

I borrowed this technique from the CONFIG_TRACE_MMIO_ACCESS feature.
It indicates from where the port access originated which can be
useful.  For example, it will reveal problems caused by two sections
of code accessing the same resource simultaneously.  It can also be
used for trace filtering or triggering.  The "%pS" format is
documented here:

   https://www.kernel.org/doc/Documentation/printk-formats.txt

Here is an example of the output:

   <idle>-0       [000] d.s2.   244.534473: portio_read: port=0xeb12 value=0x0080 uhci_check_ports+0x7c/0x2a0
   <idle>-0       [000] d.s2.   244.534474: portio_read: port=0xeb10 value=0x0080 uhci_hub_status_data+0x2dc/0x4b0
   <idle>-0       [000] d.s2.   244.534475: portio_read: port=0xeb12 value=0x0080 uhci_hub_status_data+0x2dc/0x4b0
   my_app-90      [000] d..1.   244.545706: portio_write: port=0x02f9 value=0x07 io_serial_out+0x2d/0x30
   my_app-90      [000] d.h2.   244.545720: portio_read: port=0x02fa value=0xc2 io_serial_in+0x36/0x40
   my_app-90      [000] d.h3.   244.545722: portio_read: port=0x02fd value=0x60 io_serial_in+0x36/0x40
   my_app-90      [000] d.h3.   244.545725: portio_read: port=0x02fe value=0x00 io_serial_in+0x36/0x40
   my_app-90      [000] d.h3.   244.545727: portio_write: port=0x02f8 value=0xc9 io_serial_out+0x2d/0x30
   my_app-90      [000] d.h3.   244.545732: portio_write: port=0x02f9 value=0x05 io_serial_out+0x2d/0x30
   my_app-90      [000] d.h2.   244.545734: portio_read: port=0x02fa value=0xc1 io_serial_in+0x36/0x40
   <idle>-0       [000] d.s2.   244.590459: portio_read: port=0x0408 value=0x008e2dfc acpi_pm_read_verified+0x9a/0xb0
   <idle>-0       [000] d.s2.   244.590463: portio_read: port=0x0408 value=0x008e2e19 acpi_pm_read_verified+0x82/0xb0
   <idle>-0       [000] d.s2.   244.590464: portio_read: port=0x0408 value=0x008e2e1e acpi_pm_read_verified+0x6d/0xb0

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

[Steven Rostedt]

On Wed, 4 Oct 2023 16:54:20 -0600
Dan Raymond <raymod2@gmail.com> wrote:

> With one exception io.h is included from boot.h or misc.h which is where
> the include guards are defined:
> 
> # find arch/x86/boot -type f -print0 | xargs -0 grep "#include.*[^a-z]io\.h"
> arch/x86/boot/boot.h:#include "io.h"
> arch/x86/boot/compressed/misc.h:#include "../io.h"
> arch/x86/boot/compressed/tdx.c:#include "../io.h"
> arch/x86/boot/io.h:#include <asm/shared/io.h>
> 
> I agree this is fragile but the problem is not confined to this patch.
> If I add a call to rdmsr() or wrmsr() in arch/x86/boot/compressed/misc.c
> I get the same compiler error.  It has something to do with the inline
> assembly inside arch/x86/include/asm/jump_label.h.

Doesn't arch/x86/boot/* code create an image that is separate from the core
vmlinux? That is, that code doesn't implement jump label logic nor sections.

> 
> I've copied Steven Rostedt who is the maintainer of tracefs to see if he
> has any comment.  I just noticed arch/x86/boot/msr.h and I see that it
> redefines rdmsr() and wrmsr() and omits the tracepoints.  A comment there
> explains:
> 
> /*
>  * The kernel proper already defines rdmsr()/wrmsr(), but they are not for the
>  * boot kernel since they rely on tracepoint/exception handling infrastructure
>  * that's not available here.
>  */
> 
> We could do something similar for inb()/outb() and redefine them in
> arch/x86/boot/io.h instead of including <asm/shared/io.h> there.

That would be a saner approach.

-- Steve

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

[Dan Raymond]

On 10/4/2023 5:50 PM, Steven Rostedt wrote:
>> I've copied Steven Rostedt who is the maintainer of tracefs to see if he
>> has any comment.  I just noticed arch/x86/boot/msr.h and I see that it
>> redefines rdmsr() and wrmsr() and omits the tracepoints.  A comment there
>> explains:
>>
>> /*
>>  * The kernel proper already defines rdmsr()/wrmsr(), but they are not for the
>>  * boot kernel since they rely on tracepoint/exception handling infrastructure
>>  * that's not available here.
>>  */
>>
>> We could do something similar for inb()/outb() and redefine them in
>> arch/x86/boot/io.h instead of including <asm/shared/io.h> there.
> 
> That would be a saner approach.
> 
> -- Steve

I tried this but it is problematic because there are include chains that
define inb()/outb() without including arch/x86/boot/io.h at all.  For example:

  arch/x86/boot/compressed/misc.c ->
  arch/x86/boot/compressed/misc.h ->
  include/linux/acpi.h ->
  include/acpi/acpi_io.h ->
  include/linux/io.h ->
  arch/x86/include/asm/io.h ->
  arch/x86/include/asm/shared/io.h

What we need is to disable tracepoints altogether in arch/x86/boot/* so I
added -DDISABLE_TRACEPOINTS to the relevant Makefiles and I added a check for
that symbol in tracepoint-defs.h.  I will submit a v4 version of my patch
with these changes shortly.

This resolves the problem with <asm/msr.h> as well.  After applying the v4
patch I was able to call rdmsr()/wrmsr() from arch/x86/boot/misc.c.
Theoretically we can now remove arch/x86/boot/msr.h but I had trouble with
that due to compiler warnings and errors.  The include files in arch/x86/boot
are a mess.  Maybe this can be cleaned up in another patch.

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

[Dan Raymond]

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@gmail.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
V1 -> V2:
  - create header file for prototypes to silence new compiler warning
  - reduce CPU overhead to 2 instructions (no branching) when tracing disabled
  - fix imprecise IP logging by retrieving the IP off the stack instead of using
    compile time labels

V2 -> V3:
  - restore missing semicolon

V3 -> V4:
  - make GPL licenses consistent
  - change pointer arguments from (long) to (void *)
  - eliminate include guard checks and use -DDISABLE_TRACEPOINTS instead to
    disable tracepoints in arch/x86/boot/*
  - fix compiler warnings due to signed/unsigned mismatch in arch_cmpxchg64()

 arch/x86/boot/Makefile            |  1 +
 arch/x86/boot/compressed/Makefile |  1 +
 arch/x86/include/asm/cmpxchg_32.h | 13 ++++----
 arch/x86/include/asm/shared/io.h  |  9 ++++++
 arch/x86/lib/Makefile             |  1 +
 arch/x86/lib/trace_portio.c       | 21 +++++++++++++
 include/linux/trace_portio.h      |  6 ++++
 include/linux/tracepoint-defs.h   |  2 +-
 include/trace/events/portio.h     | 49 +++++++++++++++++++++++++++++++
 9 files changed, 97 insertions(+), 6 deletions(-)
 create mode 100644 arch/x86/lib/trace_portio.c
 create mode 100644 include/linux/trace_portio.h
 create mode 100644 include/trace/events/portio.h

diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
index ffec8bb01ba8..6cec531be03b 100644
--- 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
diff --git a/arch/x86/include/asm/cmpxchg_32.h b/arch/x86/include/asm/cmpxchg_32.h
index 215f5a65790f..054a2906eefd 100644
--- 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
 
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]);		\
 }									\
 									\
 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
 
 lib-y := delay.o misc.o cmdline.o cpu.o
 lib-y += usercopy_$(BITS).o usercopy.o getuser.o putuser.o
diff --git a/arch/x86/lib/trace_portio.c b/arch/x86/lib/trace_portio.c
new file mode 100644
index 000000000000..b5f62dfd85a0
--- /dev/null
+++ b/arch/x86/lib/trace_portio.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/instruction_pointer.h>
+#include <linux/trace_portio.h>
+
+#define CREATE_TRACE_POINTS
+#include <trace/events/portio.h>
+
+void do_trace_portio_read(u32 value, u16 port, char width)
+{
+	trace_portio_read(value, port, width, (void *)_RET_IP_);
+}
+EXPORT_SYMBOL_GPL(do_trace_portio_read);
+EXPORT_TRACEPOINT_SYMBOL_GPL(portio_read);
+
+void do_trace_portio_write(u32 value, u16 port, char width)
+{
+	trace_portio_write(value, port, width, (void *)_RET_IP_);
+}
+EXPORT_SYMBOL_GPL(do_trace_portio_write);
+EXPORT_TRACEPOINT_SYMBOL_GPL(portio_write);
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);
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)
 # define tracepoint_enabled(tp) \
 	static_key_false(&(__tracepoint_##tp).key)
 #else
diff --git a/include/trace/events/portio.h b/include/trace/events/portio.h
new file mode 100644
index 000000000000..7bac3ed197a8
--- /dev/null
+++ b/include/trace/events/portio.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM portio
+
+#if !defined(_TRACE_PORTIO_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_PORTIO_H
+
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(portio_class,
+	TP_PROTO(u32 value, u16 port, char width, void *ip_addr),
+
+	TP_ARGS(value, port, width, ip_addr),
+
+	TP_STRUCT__entry(
+		__field(u32, value)
+		__field(u16, port)
+		__field(char, width)
+		__field(void *, ip_addr)
+	),
+
+	TP_fast_assign(
+		__entry->value = value;
+		__entry->port = port;
+		__entry->width = width;
+		__entry->ip_addr = ip_addr;
+	),
+
+	TP_printk("port=0x%04x value=0x%0*x %pS",
+		__entry->port,
+		__entry->width == 'b' ? 2 :
+		__entry->width == 'w' ? 4 : 8,
+		__entry->value, __entry->ip_addr)
+);
+
+DEFINE_EVENT(portio_class, portio_read,
+	TP_PROTO(u32 value, u16 port, char width, void *ip_addr),
+	TP_ARGS(value, port, width, ip_addr)
+);
+
+DEFINE_EVENT(portio_class, portio_write,
+	TP_PROTO(u32 value, u16 port, char width, void *ip_addr),
+	TP_ARGS(value, port, width, ip_addr)
+);
+
+#endif /* _TRACE_PORTIO_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

base-commit: be8b93b5cc7d533eb8c9b0590cdac055ecafe13a
-- 
2.25.1

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

[kernel test robot]

Hi Dan,

kernel test robot noticed the following build errors:

[auto build test ERROR on be8b93b5cc7d533eb8c9b0590cdac055ecafe13a]

url:    https://github.com/intel-lab-lkp/linux/commits/Dan-Raymond/arch-x86-port-I-O-tracing-on-x86/20231007-053424
base:   be8b93b5cc7d533eb8c9b0590cdac055ecafe13a
patch link:    https://lore.kernel.org/r/80b84be0-a0ad-d1a9-607a-a87c6cf509e0%40gmail.com
patch subject: [PATCH v4] arch/x86: port I/O tracing on x86
config: x86_64-randconfig-001-20231007 (https://download.01.org/0day-ci/archive/20231007/202310071410.19ffOGGp-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231007/202310071410.19ffOGGp-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310071410.19ffOGGp-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: arch/x86/realmode/rm/wakemain.o: in function `__inb':
   arch/x86/include/asm/shared/io.h:31: undefined reference to `do_trace_portio_read'
   ld: arch/x86/realmode/rm/wakemain.o: in function `__outw':
   arch/x86/include/asm/shared/io.h:32: undefined reference to `do_trace_portio_write'
   ld: arch/x86/realmode/rm/wakemain.o: in function `__outb':
   arch/x86/include/asm/shared/io.h:31: undefined reference to `do_trace_portio_write'
>> ld: arch/x86/realmode/rm/wakemain.o:(__jump_table+0x8): undefined reference to `__tracepoint_portio_read'
>> ld: arch/x86/realmode/rm/wakemain.o:(__jump_table+0x14): undefined reference to `__tracepoint_portio_write'
   ld: arch/x86/realmode/rm/wakemain.o:(__jump_table+0x20): undefined reference to `__tracepoint_portio_write'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Dan Raymond]

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@gmail.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
V1 -> V2:
  - create header file for prototypes to silence new compiler warning
  - reduce CPU overhead to 2 instructions (no branching) when tracing disabled
  - fix imprecise IP logging by retrieving the IP off the stack instead of using
    compile time labels

V2 -> V3:
  - restore missing semicolon

V3 -> V4:
  - make GPL licenses consistent
  - change pointer arguments from (long) to (void *)
  - eliminate include guard checks and use -DDISABLE_TRACEPOINTS instead to
    disable tracepoints in arch/x86/boot/*
  - fix compiler warnings due to signed/unsigned mismatch in arch_cmpxchg64()

V4 -> V5:
  - add -DDISABLE_TRACEPOINTS to arch/x86/realmode/rm/Makefile

 arch/x86/boot/Makefile            |  1 +
 arch/x86/boot/compressed/Makefile |  1 +
 arch/x86/include/asm/cmpxchg_32.h | 13 ++++----
 arch/x86/include/asm/shared/io.h  |  9 ++++++
 arch/x86/lib/Makefile             |  1 +
 arch/x86/lib/trace_portio.c       | 21 +++++++++++++
 arch/x86/realmode/rm/Makefile     |  1 +
 include/linux/trace_portio.h      |  6 ++++
 include/linux/tracepoint-defs.h   |  2 +-
 include/trace/events/portio.h     | 49 +++++++++++++++++++++++++++++++
 10 files changed, 98 insertions(+), 6 deletions(-)
 create mode 100644 arch/x86/lib/trace_portio.c
 create mode 100644 include/linux/trace_portio.h
 create mode 100644 include/trace/events/portio.h

diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
index ffec8bb01ba8..6cec531be03b 100644
--- 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
diff --git a/arch/x86/include/asm/cmpxchg_32.h b/arch/x86/include/asm/cmpxchg_32.h
index 215f5a65790f..054a2906eefd 100644
--- 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
 
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]);		\
 }									\
 									\
 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
 
 lib-y := delay.o misc.o cmdline.o cpu.o
 lib-y += usercopy_$(BITS).o usercopy.o getuser.o putuser.o
diff --git a/arch/x86/lib/trace_portio.c b/arch/x86/lib/trace_portio.c
new file mode 100644
index 000000000000..b5f62dfd85a0
--- /dev/null
+++ b/arch/x86/lib/trace_portio.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/instruction_pointer.h>
+#include <linux/trace_portio.h>
+
+#define CREATE_TRACE_POINTS
+#include <trace/events/portio.h>
+
+void do_trace_portio_read(u32 value, u16 port, char width)
+{
+	trace_portio_read(value, port, width, (void *)_RET_IP_);
+}
+EXPORT_SYMBOL_GPL(do_trace_portio_read);
+EXPORT_TRACEPOINT_SYMBOL_GPL(portio_read);
+
+void do_trace_portio_write(u32 value, u16 port, char width)
+{
+	trace_portio_write(value, port, width, (void *)_RET_IP_);
+}
+EXPORT_SYMBOL_GPL(do_trace_portio_write);
+EXPORT_TRACEPOINT_SYMBOL_GPL(portio_write);
diff --git a/arch/x86/realmode/rm/Makefile b/arch/x86/realmode/rm/Makefile
index 83f1b6a56449..660d3e1ecc03 100644
--- 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
 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);
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)
 # define tracepoint_enabled(tp) \
 	static_key_false(&(__tracepoint_##tp).key)
 #else
diff --git a/include/trace/events/portio.h b/include/trace/events/portio.h
new file mode 100644
index 000000000000..7bac3ed197a8
--- /dev/null
+++ b/include/trace/events/portio.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM portio
+
+#if !defined(_TRACE_PORTIO_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_PORTIO_H
+
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(portio_class,
+	TP_PROTO(u32 value, u16 port, char width, void *ip_addr),
+
+	TP_ARGS(value, port, width, ip_addr),
+
+	TP_STRUCT__entry(
+		__field(u32, value)
+		__field(u16, port)
+		__field(char, width)
+		__field(void *, ip_addr)
+	),
+
+	TP_fast_assign(
+		__entry->value = value;
+		__entry->port = port;
+		__entry->width = width;
+		__entry->ip_addr = ip_addr;
+	),
+
+	TP_printk("port=0x%04x value=0x%0*x %pS",
+		__entry->port,
+		__entry->width == 'b' ? 2 :
+		__entry->width == 'w' ? 4 : 8,
+		__entry->value, __entry->ip_addr)
+);
+
+DEFINE_EVENT(portio_class, portio_read,
+	TP_PROTO(u32 value, u16 port, char width, void *ip_addr),
+	TP_ARGS(value, port, width, ip_addr)
+);
+
+DEFINE_EVENT(portio_class, portio_write,
+	TP_PROTO(u32 value, u16 port, char width, void *ip_addr),
+	TP_ARGS(value, port, width, ip_addr)
+);
+
+#endif /* _TRACE_PORTIO_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

base-commit: be8b93b5cc7d533eb8c9b0590cdac055ecafe13a
-- 
2.25.1

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

[Dan Raymond]

On 10/7/2023 11:56 AM, 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@gmail.com>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> V1 -> V2:
>   - create header file for prototypes to silence new compiler warning
>   - reduce CPU overhead to 2 instructions (no branching) when tracing disabled
>   - fix imprecise IP logging by retrieving the IP off the stack instead of using
>     compile time labels
> 
> V2 -> V3:
>   - restore missing semicolon
> 
> V3 -> V4:
>   - make GPL licenses consistent
>   - change pointer arguments from (long) to (void *)
>   - eliminate include guard checks and use -DDISABLE_TRACEPOINTS instead to
>     disable tracepoints in arch/x86/boot/*
>   - fix compiler warnings due to signed/unsigned mismatch in arch_cmpxchg64()
> 
> V4 -> V5:
>   - add -DDISABLE_TRACEPOINTS to arch/x86/realmode/rm/Makefile

Can I get reviews on this please?

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

[Greg KH]

On Wed, Oct 11, 2023 at 02:22:14PM -0600, Dan Raymond wrote:
> On 10/7/2023 11:56 AM, 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@gmail.com>
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > V1 -> V2:
> >   - create header file for prototypes to silence new compiler warning
> >   - reduce CPU overhead to 2 instructions (no branching) when tracing disabled
> >   - fix imprecise IP logging by retrieving the IP off the stack instead of using
> >     compile time labels
> > 
> > V2 -> V3:
> >   - restore missing semicolon
> > 
> > V3 -> V4:
> >   - make GPL licenses consistent
> >   - change pointer arguments from (long) to (void *)
> >   - eliminate include guard checks and use -DDISABLE_TRACEPOINTS instead to
> >     disable tracepoints in arch/x86/boot/*
> >   - fix compiler warnings due to signed/unsigned mismatch in arch_cmpxchg64()
> > 
> > V4 -> V5:
> >   - add -DDISABLE_TRACEPOINTS to arch/x86/realmode/rm/Makefile
> 
> Can I get reviews on this please?

You sent it 3 days ago, please be patient, there is no need to demand
work from others so quickly, most of us are totally swamped.  If after 2
weeks or so with no review, then you can ask again.

In the meantime, to help us out, please do some patch review yourself on
the various mailing lists (the tty list can always use help.)  To ask
for work from others without helping out is not always good...

thanks,

greg k-h

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@gmail.com>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

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

[Steven Rostedt]

On Sat, 21 Oct 2023 18:00:38 +0200
Greg KH <gregkh@linuxfoundation.org> 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@gmail.com>
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>  

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

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

[Dan Raymond]

On 10/21/2023 10:00 AM, Greg KH wrote:> 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.

Tracing is not needed, per se, but it can be incredibly useful for
debugging certain types of problems.  I think the utility of tracing is
well accepted by the community.  

Quoting Peter Zijlstra:

   "...tracepoints in general are useful"

Quoting Andy Shevchenko:

   "...you may trace any IO on some architectures (at least x86), it's
   called mmiotracer (I have used it like 5 years ago or so to trace UART)"

Similar trace functionality is already present in the kernel today
(CONFIG_MMIOTRACE and CONFIG_TRACE_MMIO_ACCESS) and it does anecdotally
get used.  However, as mentioned in the patch description, those tracing
features don't work with port-based I/O hence the need for this patch.
I don't see how this is going to create a maintenance burden.  It adds
two lines of code to a macro that rarely if ever changes.

>> --- a/arch/x86/boot/Makefile
>> +++ b/arch/x86/boot/Makefile
>
> Note, you are keeping tracing from working in a few areas that might not
> be good...
>> 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 of the kernel sources (arch/x86/boot/* and arch/x86/realmode/*) are
not part of the kernel proper and they don't have the infrastructure to
support tracepoints.  When these sources include header files that
reference tracepoints it causes compiler errors.  I previously worked
around this issue with include guard checks but you objected to that:

   "I see what you are doing here in trying to see if a .h file has been
   included already, but now you are making an assumption on both the .h
   file ordering, and the #ifdef guard for those .h files, which are
   something that we almost never remember or even consider when dealing
   with .h files files."

Therefore I implemented a more reliable mechanism to disable tracepoints.
I explained this earlier in the thread:

   "What we need is to disable tracepoints altogether in arch/x86/boot/*
   so I added -DDISABLE_TRACEPOINTS to the relevant Makefiles and I added
   a check for that symbol in tracepoint-defs.h.  I will submit a v4
   version of my patch with these changes shortly.

   This resolves the problem with <asm/msr.h> as well.  After applying
   the v4 patch I was able to call rdmsr()/wrmsr() from
   arch/x86/boot/misc.c.  Theoretically we can now remove
   arch/x86/boot/msr.h but I had trouble with that due to compiler
   warnings and errors.  The include files in arch/x86/boot are a mess.
   Maybe this can be cleaned up in another patch."

>> --- a/arch/x86/include/asm/cmpxchg_32.h
>> +++ b/arch/x86/include/asm/cmpxchg_32.h
> 
> 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?

I did mention these changes in the changelog:

   "fix compiler warnings due to signed/unsigned mismatch in arch_cmpxchg64()"

These warnings appear to be a latent defect which was triggered by the
include file changes in this patch.  I assume that introducing compiler
warnings (even indirectly) is not allowed so I fixed them on this patch.

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

Please reference Documentation/trace/tracepoints.rst:

   "If you require calling a tracepoint from a header file, it is not
   recommended to call one directly or to use the
   trace_<tracepoint>_enabled() function call, as tracepoints in header
   files can have side effects..."

The tracepoint_enabled() macro is very efficient and causes only one
instruction of overhead (a nop) when tracing is disabled.  I verified
this by disassembling vmlinux.

>> +obj-$(CONFIG_TRACEPOINTS) += trace_portio.o
> 
> So you are always enabling these if any CONFIG_TRACEPOINTS is enabled?
> That seems brave.

This doesn't enable the tracepoints.  It just adds support for portio
tracepoints to the kernel image.  The tracepoints are disabled by default
and must be explicitly enabled by the user at runtime.  The overhead is
very modest when portio tracing is disabled (as I mentioned above).

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

I addressed this question above.

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

Noted.

>> -#ifdef CONFIG_TRACEPOINTS
>> +#if defined(CONFIG_TRACEPOINTS) && !defined(DISABLE_TRACEPOINTS)
> 
> Why this global change?

I addressed this question above.  This is how we prevent tracepoint
logic in header files from causing compiler errors in source files that
aren't part of the kernel proper.

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

This comment confuses me.  As you know I originally submitted a patch
that added I/O tracing just to the 8250 serial driver.  The patch was
titled "create debugfs interface for UART register tracing".  You said
this at the time:

   "Anyway, again, cool feature, I like it, but if you can tie it into
   the existing trace framework better (either by using that entirely
   which might be best), or at the least, putting your hook into the
   data path with it, that would be best."

My original patch went through a few revisions before Andy Shevchenko
suggested I should add portio tracing instead in a manner similar to
how CONFIG_TRACE_MMIO_ACCESS works.  You agreed.  Hence I created this
patch.

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

[Dan Raymond]

On 10/21/2023 2:15 PM, Steven Rostedt wrote:

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

As I mentioned to Greg, this fix is needed to avoid compiler warnings
triggered by this patch.  If I submitted this separately it would have
to be merged first.  Isn't it easier to combine them since this is
not a functional change (it just makes a cast explicit)?

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

It's only needed to suppress compiler errors when building arch/x86/boot/*
and arch/x86/realmode/*.  Those source files include various x86 headers
such as <asm/msr.h> and <asm/shared/io.h>.  Those x86 headers include
<linux/tracepoint-defs.h> which references static_key_false() in
<linux/jump_label.h>.  DISABLE_TRACEPOINTS eliminates that reference and
hence suppresses the compiler error.

I didn't intend for this macro to be used by developers adding new
tracepoints so I didn't document it as such.  As far as creating a
separate patch: again this is a requirement for this patch and it doesn't
cause any functional changes so can't we combine them?

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

[Steven Rostedt]

On Mon, 23 Oct 2023 15:29:53 -0600
Dan Raymond <raymod2@gmail.com> wrote:

> On 10/21/2023 2:15 PM, Steven Rostedt wrote:
> 
> >> 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?  

BTW, trimming is good, but it's still better to leave the code that the
comment was talking about. I had to go back to my sent folder to figure it
out.

Adding back here for reference:


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

> > 
> > Agreed, if this has issues, it probably should be a separate patch.  
> 
> As I mentioned to Greg, this fix is needed to avoid compiler warnings
> triggered by this patch.  If I submitted this separately it would have
> to be merged first.  Isn't it easier to combine them since this is
> not a functional change (it just makes a cast explicit)?

That's what patch series are for. You make a series of changes where one is
dependent on the other. But each commit should only do one thing. If you
need to fix something to do your one thing, that fix should be a separate
patch, but make it part of a series.

Just a "cast explicit" that's in generic code should be a separate change
with a change log explaining why it is needed, and why it didn't work
without it. Single commits can be just "no functional change".

> 
> >>> -#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.  
> 
> It's only needed to suppress compiler errors when building arch/x86/boot/*
> and arch/x86/realmode/*.  Those source files include various x86 headers
> such as <asm/msr.h> and <asm/shared/io.h>.  Those x86 headers include
> <linux/tracepoint-defs.h> which references static_key_false() in
> <linux/jump_label.h>.  DISABLE_TRACEPOINTS eliminates that reference and
> hence suppresses the compiler error.
> 
> I didn't intend for this macro to be used by developers adding new
> tracepoints so I didn't document it as such.  As far as creating a
> separate patch: again this is a requirement for this patch and it doesn't
> cause any functional changes so can't we combine them?

This is touching generic code, and as such, it *will* be used by others. If
generic code needs something like "DISABLE_TRACEPOINTS" for your use case,
it may be needed for someone else's.

For the same reason above, it really needs to be a separate patch with a
change log explaining why it is needed.

If this should only be used by code that is not part of the kernel proper,
then we should probably call it something else.

#ifdef PRE_LINUX_CODE  ?

Or something that more explicitly states that this is included in code
that's not part of the Linux proper. By saying "DISABLE_TRACEPOINTS" it
will look like this is the way to disable it for other use cases. Maybe we
want that, or perhaps we don't.

Either case, it should still be separate with a detailed explanation, for
when another developer sees this code, a git blame will give all the
explanation necessary for why it exists.

-- Steve

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

[Greg KH]

On Mon, Oct 23, 2023 at 02:28:04PM -0600, Dan Raymond wrote:
> > 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?
> 
> This comment confuses me.  As you know I originally submitted a patch
> that added I/O tracing just to the 8250 serial driver.  The patch was
> titled "create debugfs interface for UART register tracing".  You said
> this at the time:
> 
>    "Anyway, again, cool feature, I like it, but if you can tie it into
>    the existing trace framework better (either by using that entirely
>    which might be best), or at the least, putting your hook into the
>    data path with it, that would be best."

Remember some of us, like myself, get on average 1000+ emails a day that
they need to file/delete/review, so what I wrote yesterday I usually
can't remember, let alone weeks ago :)

> My original patch went through a few revisions before Andy Shevchenko
> suggested I should add portio tracing instead in a manner similar to
> how CONFIG_TRACE_MMIO_ACCESS works.  You agreed.  Hence I created this
> patch.

That's great, but it turns out that the x86 maintainers don't like this,
so perhaps that's not going to work out well.

I still think the original idea of using tracepoints for serial data
would be best, but hey, I don't need this feature :)

thanks,

greg k-h