Messages in this thread | | | Date | Mon, 22 Nov 2021 20:29:05 +0530 | Subject | Re: [PATCHv4 2/2] arm64/io: Add a header for mmio access instrumentation | From | Sai Prakash Ranjan <> |
| |
On 11/22/2021 8:00 PM, Arnd Bergmann wrote: > On Mon, Nov 22, 2021 at 3:19 PM Sai Prakash Ranjan > <quic_saipraka@quicinc.com> wrote: >> On 11/22/2021 7:29 PM, Arnd Bergmann wrote: >>> I think this would be a lot less confusing to readers, as it is implemented >>> exactly in the place that has the normal definition, and it can also have >>> somewhat more logical semantics by only instrumenting the >>> normal/relaxed/ioport accessors but not the __raw_* versions that >>> are meant to be little more than a pointer dereference. >> But how is this different from logic in atomic-instrumented.h which also >> has asm-generic version? >> Initial review few years back mentioned about having something similar >> to atomic instrumentation >> and hence it was implemented with the similar approach keeping >> instrumentation out of arch specific details. > This is only a cosmetic difference. I usually prefer fewer indirections, > and I like the way that include/asm-generic/io.h only has all the > normal 'static inline' definitions spelled out, and calling the __raw_* > versions. Your version adds an extra layer with the arch_raw_readl(), > which I'd prefer to avoid.
I'm ok with your preference as long as we have some way to log these MMIO accesses.
>> And if we do move this instrumentation to asm-generic/io.h, how will >> that be executed since >> the arch specifc read{b,w,l,q} overrides this generic version? > As I understand it, your version also requires architecture specific > changes, so that would be the same: it only works for architectures > that get the definition of readl()/readl_relaxed()/inl()/... from > include/asm-generic/io.h and only override the __raw version. Arnd
Sorry, I didn't get this part, so I am trying this on ARM64:
arm64/include/asm/io.h has read{b,l,w,q} defined. include/asm-generic/io.h has below: #ifndef readl #define readl readl static inline u32 readl(const volatile void __iomem *addr)
and we include asm-generic/io.h in arm64/include/asm/io.h at the end after the definitions for arm64 mmio accesors. So arch implementation here overrides generic ones as I see it, am I missing something? I even confirmed this with some trace_printk to generic and arch specific definitions of readl and I see arch specific ones being called.
Thanks, Sai
| |