diff mbox series

[PATCHv11,6/6] asm-generic/io: Add logging support for MMIO accessors

Message ID 3de35c9f4a3a070d197bab499acefc709a6f5336.1645772606.git.quic_saipraka@quicinc.com
State Superseded
Headers show
Series lib/rwmmio/arm64: Add support to trace register reads/writes | expand

Commit Message

Sai Prakash Ranjan April 28, 2022, 3:30 a.m. UTC 
Add logging support for MMIO high level accessors such as read{b,w,l,q}
and their relaxed versions to aid in debugging unexpected crashes/hangs
caused by the corresponding MMIO operation. Also add a generic flag
(__DISABLE_TRACE_MMIO__) which is used to disable MMIO tracing in nVHE KVM
and if required can be used to disable MMIO tracing for specific drivers.

Signed-off-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>
---
 arch/arm64/kvm/hyp/nvhe/Makefile |  7 ++-
 include/asm-generic/io.h         | 82 ++++++++++++++++++++++++++++++--
 2 files changed, 84 insertions(+), 5 deletions(-)

Comments

Sai Prakash Ranjan April 28, 2022, 7:44 a.m. UTC | #1 
On 4/28/2022 1:05 PM, Greg KH wrote:
> On Thu, Apr 28, 2022 at 12:59:13PM +0530, Sai Prakash Ranjan wrote:
>> On 4/28/2022 11:21 AM, Greg KH wrote:
>>> On Thu, Apr 28, 2022 at 09:00:13AM +0530, Sai Prakash Ranjan wrote:
>>>> Add logging support for MMIO high level accessors such as read{b,w,l,q}
>>>> and their relaxed versions to aid in debugging unexpected crashes/hangs
>>>> caused by the corresponding MMIO operation. Also add a generic flag
>>>> (__DISABLE_TRACE_MMIO__) which is used to disable MMIO tracing in nVHE KVM
>>>> and if required can be used to disable MMIO tracing for specific drivers.
>>> This "add a build flag to a Makefile to change how a driver operates"
>>> feels very wrong to me given that this is now a totally new way to
>>> control how a driver works at build time.  That's not anything we have
>>> done before for drivers and if added, is only going to create much added
>>> complexity.
>> Not exactly, there are already many such build flags being used currently across kernel.
>>
>> Example: "-D__KVM_NVHE_HYPERVISOR__, D__KVM_VHE_HYPERVISOR__,
> That's crazy KVM stuff, don't extrapoloate that to all kernel drivers
> please.

Ok :)

>> -D__NO_FORTIFY, -D__DISABLE_EXPORTS, -DDISABLE_BRANCH_PROFILING".
> Those are compiler flags that affect gcc, not kernel code functionality.
>
>> It gives us even more flexibility to disable feature for multiple files under a directory
>> rather than individually cluttering each file, look at "-D__KVM_NVHE_HYPERVISOR__"
>> for files under "arch/arm64/kvm/hyp/nvhe/".
> Again, crazy KVM stuff, do not want that for all drivers in the kernel.
>
>>> How about requiring that the #define be in the .c files, and not in the
>>> Makefile, as Makefile changes are much much harder to notice and review
>>> over time.
>> How is this cleaner, lets say we have many such drivers like all drivers under drivers/serial,
>> so we go and add #define for each of them? That looks more spread out than having all
>> such information under one file (Makefile).
> If you have to go and add this to each and every driver, that is a HUGE
> hint that this feature is not a good one and that no one should be using
> it in the first place, right?
>
> Again, this is a new way to modify driver functionality that is outside
> of the driver itself, which is not something that I would like to see
> added without a whole lot of discussion and planning.  To throw it in as
> part of a kvm change is not a nice way to hide such a thing.

Sure, will make the change as you suggested.

>> And I didn't understand how is it harder to track changes to makefile? Makefile is  part
>> of the driver directory and any changes to makefile is visible to the corresponding maintainers.
>> Do you mean something else?
> I mean that we review driver changes all the time, and code is
> self-contained and the functionality is only affected right now by
> Kconfig options, and what is in the .c files itself.  You are adding a
> new way to change functionality by adding a Makefile configuration
> option as well.  That is a major functional change for how we do our
> configuration logic in Linux.

Ah ok, you mean like that. Sure I don't have any strong opinion, so will move it
to the driver file.

>
>>> Also, I see that this "disable the trace" feature has already been asked
>>> for for 2 other drivers in the Android kernel tree, why not include
>>> those changes here as well?  That kind of shows that this new feature is
>>> limited in that driver authors are already wanting it disabled, even
>>> before it is accepted.
>> That can be done later on top of this series right? This series mainly deals with adding
>> initial support for such tracing, there could be numerous drivers who might or might
>> not want the feature which can be added onto later. We can't actually identify all
>> the driver requirements upfront. As an example, we have already used the flag to
>> disable tracing for nVHE KVM, so we know how to use the flag.
> Again, make it explicit in the driver file itself that it is doing this,
> not in the Makefile, and I will not have any objections.

Ok, for kernel drivers I will make the define at the top of the .c driver file and include
those 2 driver changes in the series.

>>> Because of that, who _will_ be using this feature?
>>>
>> Every driver except those two or few more, and it is not a bug or anything, they just want to disable it
>> to limit the logs in case of example UART driver since the reads/writes are very frequent in those cases
>> and the logs are not necessarily useful for them.
> I have a feeling that lots more drivers will want this disabled due to
> the noise it will cause.  The fact that we already have 2 requests to
> change this _before_ the code is even merged is proof of that.
>
> thanks,
>
> greg k-h

Thanks,
Sai
Greg KH April 28, 2022, 8:20 a.m. UTC | #2 
On Thu, Apr 28, 2022 at 01:14:59PM +0530, Sai Prakash Ranjan wrote:
> > > > Also, I see that this "disable the trace" feature has already been asked
> > > > for for 2 other drivers in the Android kernel tree, why not include
> > > > those changes here as well?  That kind of shows that this new feature is
> > > > limited in that driver authors are already wanting it disabled, even
> > > > before it is accepted.
> > > That can be done later on top of this series right? This series mainly deals with adding
> > > initial support for such tracing, there could be numerous drivers who might or might
> > > not want the feature which can be added onto later. We can't actually identify all
> > > the driver requirements upfront. As an example, we have already used the flag to
> > > disable tracing for nVHE KVM, so we know how to use the flag.
> > Again, make it explicit in the driver file itself that it is doing this,
> > not in the Makefile, and I will not have any objections.
> 
> Ok, for kernel drivers I will make the define at the top of the .c driver file and include
> those 2 driver changes in the series.

Thank you, that is a much better way forward.

greg k-h
diff mbox series

Patch

diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
index 24b2c2425b38..228d1f8921c3 100644
--- a/arch/arm64/kvm/hyp/nvhe/Makefile
+++ b/arch/arm64/kvm/hyp/nvhe/Makefile
@@ -4,7 +4,12 @@ 
 #
 
 asflags-y := -D__KVM_NVHE_HYPERVISOR__ -D__DISABLE_EXPORTS
-ccflags-y := -D__KVM_NVHE_HYPERVISOR__ -D__DISABLE_EXPORTS
+
+# Tracepoint and MMIO logging symbols should not be visible at nVHE KVM as
+# there is no way to execute them and any such MMIO access from nVHE KVM
+# will explode instantly (Words of Marc Zyngier). So introduce a generic flag
+# __DISABLE_TRACE_MMIO__ to disable MMIO tracing for nVHE KVM.
+ccflags-y := -D__KVM_NVHE_HYPERVISOR__ -D__DISABLE_EXPORTS -D__DISABLE_TRACE_MMIO__
 
 hostprogs := gen-hyprel
 HOST_EXTRACFLAGS += -I$(objtree)/include
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 7ce93aaf69f8..99090722cb4b 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -10,6 +10,7 @@ 
 #include <asm/page.h> /* I/O is all done through memory accesses */
 #include <linux/string.h> /* for memset() and memcpy() */
 #include <linux/types.h>
+#include <linux/instruction_pointer.h>
 
 #ifdef CONFIG_GENERIC_IOMAP
 #include <asm-generic/iomap.h>
@@ -61,6 +62,35 @@ 
 #define __io_par(v)     __io_ar(v)
 #endif
 
+#if IS_ENABLED(CONFIG_TRACE_MMIO_ACCESS) && !(defined(__DISABLE_TRACE_MMIO__))
+#include <linux/tracepoint-defs.h>
+
+DECLARE_TRACEPOINT(rwmmio_write);
+DECLARE_TRACEPOINT(rwmmio_post_write);
+DECLARE_TRACEPOINT(rwmmio_read);
+DECLARE_TRACEPOINT(rwmmio_post_read);
+
+void log_write_mmio(u64 val, u8 width, volatile void __iomem *addr,
+		    unsigned long caller_addr);
+void log_post_write_mmio(u64 val, u8 width, volatile void __iomem *addr,
+			 unsigned long caller_addr);
+void log_read_mmio(u8 width, const volatile void __iomem *addr,
+		   unsigned long caller_addr);
+void log_post_read_mmio(u64 val, u8 width, const volatile void __iomem *addr,
+			unsigned long caller_addr);
+
+#else
+
+static inline void log_write_mmio(u64 val, u8 width, volatile void __iomem *addr,
+				  unsigned long caller_addr) {}
+static inline void log_post_write_mmio(u64 val, u8 width, volatile void __iomem *addr,
+				       unsigned long caller_addr) {}
+static inline void log_read_mmio(u8 width, const volatile void __iomem *addr,
+				 unsigned long caller_addr) {}
+static inline void log_post_read_mmio(u64 val, u8 width, const volatile void __iomem *addr,
+				      unsigned long caller_addr) {}
+
+#endif /* CONFIG_TRACE_MMIO_ACCESS */
 
 /*
  * __raw_{read,write}{b,w,l,q}() access memory in native endianness.
@@ -149,9 +179,11 @@  static inline u8 readb(const volatile void __iomem *addr)
 {
 	u8 val;
 
+	log_read_mmio(8, addr, _THIS_IP_);
 	__io_br();
 	val = __raw_readb(addr);
 	__io_ar(val);
+	log_post_read_mmio(val, 8, addr, _THIS_IP_);
 	return val;
 }
 #endif
@@ -162,9 +194,11 @@  static inline u16 readw(const volatile void __iomem *addr)
 {
 	u16 val;
 
+	log_read_mmio(16, addr, _THIS_IP_);
 	__io_br();
 	val = __le16_to_cpu((__le16 __force)__raw_readw(addr));
 	__io_ar(val);
+	log_post_read_mmio(val, 16, addr, _THIS_IP_);
 	return val;
 }
 #endif
@@ -175,9 +209,11 @@  static inline u32 readl(const volatile void __iomem *addr)
 {
 	u32 val;
 
+	log_read_mmio(32, addr, _THIS_IP_);
 	__io_br();
 	val = __le32_to_cpu((__le32 __force)__raw_readl(addr));
 	__io_ar(val);
+	log_post_read_mmio(val, 32, addr, _THIS_IP_);
 	return val;
 }
 #endif
@@ -189,9 +225,11 @@  static inline u64 readq(const volatile void __iomem *addr)
 {
 	u64 val;
 
+	log_read_mmio(64, addr, _THIS_IP_);
 	__io_br();
 	val = __le64_to_cpu(__raw_readq(addr));
 	__io_ar(val);
+	log_post_read_mmio(val, 64, addr, _THIS_IP_);
 	return val;
 }
 #endif
@@ -201,9 +239,11 @@  static inline u64 readq(const volatile void __iomem *addr)
 #define writeb writeb
 static inline void writeb(u8 value, volatile void __iomem *addr)
 {
+	log_write_mmio(value, 8, addr, _THIS_IP_);
 	__io_bw();
 	__raw_writeb(value, addr);
 	__io_aw();
+	log_post_write_mmio(value, 8, addr, _THIS_IP_);
 }
 #endif
 
@@ -211,9 +251,11 @@  static inline void writeb(u8 value, volatile void __iomem *addr)
 #define writew writew
 static inline void writew(u16 value, volatile void __iomem *addr)
 {
+	log_write_mmio(value, 16, addr, _THIS_IP_);
 	__io_bw();
 	__raw_writew((u16 __force)cpu_to_le16(value), addr);
 	__io_aw();
+	log_post_write_mmio(value, 16, addr, _THIS_IP_);
 }
 #endif
 
@@ -221,9 +263,11 @@  static inline void writew(u16 value, volatile void __iomem *addr)
 #define writel writel
 static inline void writel(u32 value, volatile void __iomem *addr)
 {
+	log_write_mmio(value, 32, addr, _THIS_IP_);
 	__io_bw();
 	__raw_writel((u32 __force)__cpu_to_le32(value), addr);
 	__io_aw();
+	log_post_write_mmio(value, 32, addr, _THIS_IP_);
 }
 #endif
 
@@ -232,9 +276,11 @@  static inline void writel(u32 value, volatile void __iomem *addr)
 #define writeq writeq
 static inline void writeq(u64 value, volatile void __iomem *addr)
 {
+	log_write_mmio(value, 64, addr, _THIS_IP_);
 	__io_bw();
 	__raw_writeq(__cpu_to_le64(value), addr);
 	__io_aw();
+	log_post_write_mmio(value, 64, addr, _THIS_IP_);
 }
 #endif
 #endif /* CONFIG_64BIT */
@@ -248,7 +294,12 @@  static inline void writeq(u64 value, volatile void __iomem *addr)
 #define readb_relaxed readb_relaxed
 static inline u8 readb_relaxed(const volatile void __iomem *addr)
 {
-	return __raw_readb(addr);
+	u8 val;
+
+	log_read_mmio(8, addr, _THIS_IP_);
+	val = __raw_readb(addr);
+	log_post_read_mmio(val, 8, addr, _THIS_IP_);
+	return val;
 }
 #endif
 
@@ -256,7 +307,12 @@  static inline u8 readb_relaxed(const volatile void __iomem *addr)
 #define readw_relaxed readw_relaxed
 static inline u16 readw_relaxed(const volatile void __iomem *addr)
 {
-	return __le16_to_cpu(__raw_readw(addr));
+	u16 val;
+
+	log_read_mmio(16, addr, _THIS_IP_);
+	val = __le16_to_cpu(__raw_readw(addr));
+	log_post_read_mmio(val, 16, addr, _THIS_IP_);
+	return val;
 }
 #endif
 
@@ -264,7 +320,12 @@  static inline u16 readw_relaxed(const volatile void __iomem *addr)
 #define readl_relaxed readl_relaxed
 static inline u32 readl_relaxed(const volatile void __iomem *addr)
 {
-	return __le32_to_cpu(__raw_readl(addr));
+	u32 val;
+
+	log_read_mmio(32, addr, _THIS_IP_);
+	val = __le32_to_cpu(__raw_readl(addr));
+	log_post_read_mmio(val, 32, addr, _THIS_IP_);
+	return val;
 }
 #endif
 
@@ -272,7 +333,12 @@  static inline u32 readl_relaxed(const volatile void __iomem *addr)
 #define readq_relaxed readq_relaxed
 static inline u64 readq_relaxed(const volatile void __iomem *addr)
 {
-	return __le64_to_cpu(__raw_readq(addr));
+	u64 val;
+
+	log_read_mmio(64, addr, _THIS_IP_);
+	val = __le64_to_cpu(__raw_readq(addr));
+	log_post_read_mmio(val, 64, addr, _THIS_IP_);
+	return val;
 }
 #endif
 
@@ -280,7 +346,9 @@  static inline u64 readq_relaxed(const volatile void __iomem *addr)
 #define writeb_relaxed writeb_relaxed
 static inline void writeb_relaxed(u8 value, volatile void __iomem *addr)
 {
+	log_write_mmio(value, 8, addr, _THIS_IP_);
 	__raw_writeb(value, addr);
+	log_post_write_mmio(value, 8, addr, _THIS_IP_);
 }
 #endif
 
@@ -288,7 +356,9 @@  static inline void writeb_relaxed(u8 value, volatile void __iomem *addr)
 #define writew_relaxed writew_relaxed
 static inline void writew_relaxed(u16 value, volatile void __iomem *addr)
 {
+	log_write_mmio(value, 16, addr, _THIS_IP_);
 	__raw_writew(cpu_to_le16(value), addr);
+	log_post_write_mmio(value, 16, addr, _THIS_IP_);
 }
 #endif
 
@@ -296,7 +366,9 @@  static inline void writew_relaxed(u16 value, volatile void __iomem *addr)
 #define writel_relaxed writel_relaxed
 static inline void writel_relaxed(u32 value, volatile void __iomem *addr)
 {
+	log_write_mmio(value, 32, addr, _THIS_IP_);
 	__raw_writel(__cpu_to_le32(value), addr);
+	log_post_write_mmio(value, 32, addr, _THIS_IP_);
 }
 #endif
 
@@ -304,7 +376,9 @@  static inline void writel_relaxed(u32 value, volatile void __iomem *addr)
 #define writeq_relaxed writeq_relaxed
 static inline void writeq_relaxed(u64 value, volatile void __iomem *addr)
 {
+	log_write_mmio(value, 64, addr, _THIS_IP_);
 	__raw_writeq(__cpu_to_le64(value), addr);
+	log_post_write_mmio(value, 64, addr, _THIS_IP_);
 }
 #endif