| Message ID | cc7fba3b-9da2-b9eb-95c8-7336e1cd4449@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] arch/x86: port I/O tracing on x86 | expand |
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?
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
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.
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?
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
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>
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 base-commit: be8b93b5cc7d533eb8c9b0590cdac055ecafe13a