Re: [PATCHv8 4/5] lib: Add register read/write tracing support

[Sai Prakash Ranjan <quic_saipraka@quicinc.com>]

On 1/17/2022 9:00 PM, Sai Prakash Ranjan wrote:
> Hi Steve,
>
> On 1/17/2022 8:33 PM, Steven Rostedt wrote:
>> On Mon, 17 Jan 2022 09:02:53 +0530
>> Sai Prakash Ranjan <quic_saipraka@quicinc.com> wrote:
>>
>>> diff --git a/include/trace/events/rwmmio.h 
>>> b/include/trace/events/rwmmio.h
>>> new file mode 100644
>>> index 000000000000..798fbe1ac9f9
>>> --- /dev/null
>>> +++ b/include/trace/events/rwmmio.h
>>> @@ -0,0 +1,112 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All 
>>> rights reserved.
>>> + */
>>> +#undef TRACE_SYSTEM
>>> +#define TRACE_SYSTEM rwmmio
>>> +
>>> +#if !defined(_TRACE_RWMMIO_H) || defined(TRACE_HEADER_MULTI_READ)
>>> +#define _TRACE_RWMMIO_H
>>> +
>>> +#include <linux/tracepoint.h>
>>> +
>>> +TRACE_EVENT(rwmmio_write,
>>> +
>>> +    TP_PROTO(unsigned long caller, u64 val, u8 width, volatile void 
>>> __iomem *addr),
>>> +
>>> +    TP_ARGS(caller, val, width, addr),
>>> +
>>> +    TP_STRUCT__entry(
>>> +        __field(u64, caller)
>>> +        __field(u64, val)
>>> +        __field(u64, addr)
>>> +        __field(u8, width)
>>> +    ),
>>> +
>>> +    TP_fast_assign(
>>> +        __entry->caller = caller;
>>> +        __entry->val = val;
>>> +        __entry->addr = (unsigned long)(void *)addr;
>>> +        __entry->width = width;
>>> +    ),
>>> +
>>> +    TP_printk("%pS width=%d val=%#llx addr=%#llx",
>>> +        (void *)(unsigned long)__entry->caller, __entry->width,
>>> +        __entry->val, __entry->addr)
>>> +);
>>> +
>>> +TRACE_EVENT(rwmmio_post_write,
>>> +
>>> +    TP_PROTO(unsigned long caller, u64 val, u8 width, volatile void 
>>> __iomem *addr),
>>> +
>>> +    TP_ARGS(caller, val, width, addr),
>>> +
>>> +    TP_STRUCT__entry(
>>> +        __field(u64, caller)
>>> +        __field(u64, val)
>>> +        __field(u64, addr)
>>> +        __field(u8, width)
>>> +    ),
>>> +
>>> +    TP_fast_assign(
>>> +        __entry->caller = caller;
>>> +        __entry->val = val;
>>> +        __entry->addr = (unsigned long)(void *)addr;
>>> +        __entry->width = width;
>>> +    ),
>>> +
>>> +    TP_printk("%pS width=%d val=%#llx addr=%#llx",
>>> +        (void *)(unsigned long)__entry->caller, __entry->width,
>>> +        __entry->val, __entry->addr)
>>> +);
>>> +
>>> +TRACE_EVENT(rwmmio_read,
>>> +
>>> +    TP_PROTO(unsigned long caller, u8 width, const volatile void 
>>> __iomem *addr),
>>> +
>>> +    TP_ARGS(caller, width, addr),
>>> +
>>> +    TP_STRUCT__entry(
>>> +        __field(u64, caller)
>>> +        __field(u64, addr)
>>> +        __field(u8, width)
>>> +    ),
>>> +
>>> +    TP_fast_assign(
>>> +        __entry->caller = caller;
>>> +        __entry->addr = (unsigned long)(void *)addr;
>>> +        __entry->width = width;
>>> +    ),
>>> +
>>> +    TP_printk("%pS width=%d addr=%#llx",
>>> +         (void *)(unsigned long)__entry->caller, __entry->width, 
>>> __entry->addr)
>>> +);
>>> +
>>> +TRACE_EVENT(rwmmio_post_read,
>>> +
>>> +    TP_PROTO(unsigned long caller, u64 val, u8 width, const 
>>> volatile void __iomem *addr),
>>> +
>>> +    TP_ARGS(caller, val, width, addr),
>>> +
>>> +    TP_STRUCT__entry(
>>> +        __field(u64, caller)
>>> +        __field(u64, val)
>>> +        __field(u64, addr)
>>> +        __field(u8, width)
>>> +    ),
>>> +
>>> +    TP_fast_assign(
>>> +        __entry->caller = caller;
>>> +        __entry->val = val;
>>> +        __entry->addr = (unsigned long)(void *)addr;
>>> +        __entry->width = width;
>>> +    ),
>>> +
>>> +    TP_printk("%pS width=%d val=%#llx addr=%#llx",
>>> +         (void *)(unsigned long)__entry->caller, __entry->width,
>>> +         __entry->val, __entry->addr)
>>> +);
>> The above should be replaced with:
>>
>> DECLARE_EVENT_CLASS(rwmmio_rw_template,
>>
>>     TP_PROTO(unsigned long caller, u64 val, u8 width, volatile void 
>> __iomem *addr),
>>
>>     TP_ARGS(caller, val, width, addr),
>>
>>     TP_STRUCT__entry(
>>         __field(u64, caller)
>>         __field(u64, val)
>>         __field(u64, addr)
>>         __field(u8, width)
>>     ),
>>
>>     TP_fast_assign(
>>         __entry->caller = caller;
>>         __entry->val = val;
>>         __entry->addr = (unsigned long)(void *)addr;
>>         __entry->width = width;
>>     ),
>>
>>     TP_printk("%pS width=%d val=%#llx addr=%#llx",
>>         (void *)(unsigned long)__entry->caller, __entry->width,
>>         __entry->val, __entry->addr)
>> );
>>
>> DEFINE_EVENT(rwmmio_rw_template, rwmmio_write,
>>     TP_PROTO(unsigned long caller, u64 val, u8 width, volatile void 
>> __iomem *addr),
>>     TP_ARGS(caller, val, width, addr)
>> );
>>
>> DEFINE_EVENT(rwmmio_rw_template, rwmmio_post_write,
>>     TP_PROTO(unsigned long caller, u64 val, u8 width, volatile void 
>> __iomem *addr),
>>     TP_ARGS(caller, val, width, addr)
>> );
>>
>> DEFINE_EVENT(rwmmio_rw_template, rwmmio_post_read,
>>     TP_PROTO(unsigned long caller, u64 val, u8 width, volatile void 
>> __iomem *addr),
>>     TP_ARGS(caller, val, width, addr)
>> );
>>
>> It will save around 15k in memory.
>>
>> And since rwmmio_read doesn't have a val field, it can stay a 
>> TRACE_EVENT.
>>
>> TRACE_EVENT(rwmmio_read,
>>
>>     TP_PROTO(unsigned long caller, u8 width, const volatile void 
>> __iomem *addr),
>>
>>     TP_ARGS(caller, width, addr),
>>
>>     TP_STRUCT__entry(
>>         __field(u64, caller)
>>         __field(u64, addr)
>>         __field(u8, width)
>>     ),
>>
>>     TP_fast_assign(
>>         __entry->caller = caller;
>>         __entry->addr = (unsigned long)(void *)addr;
>>         __entry->width = width;
>>     ),
>>
>>     TP_printk("%pS width=%d addr=%#llx",
>>          (void *)(unsigned long)__entry->caller, __entry->width, 
>> __entry->addr)
>> );
>>
>> -- Steve
>
> Thanks for the review, will add these changes in next version.
>
>

Just one note that I need to keep rwmmio_post_read also as TRACE_EVENT 
because
of const qualifier in post_read resulting in compiler warnings,

lib/trace_readwrite.c:43:50: warning: passing argument 4 of 
‘trace_rwmmio_post_read’ discards ‘const’ qualifier from pointer target 
type [-Wdiscarded-qualifiers]

Thanks,
Sai
>>> +
>>> +#endif /* _TRACE_RWMMIO_H */
>>> +
>>> +#include <trace/define_trace.h>
>>> diff --git a/lib/Kconfig b/lib/Kconfig
>>> index c80fde816a7e..ea520c315c0f 100644
>>> --- a/lib/Kconfig
>>> +++ b/lib/Kconfig
>>> @@ -119,6 +119,13 @@ config INDIRECT_IOMEM_FALLBACK
>>>         mmio accesses when the IO memory address is not a registered
>>>         emulated region.
>>>   +config TRACE_MMIO_ACCESS
>>> +    bool "Register read/write tracing"
>>> +    depends on TRACING && ARCH_HAVE_TRACE_MMIO_ACCESS
>>> +    help
>>> +      Create tracepoints for MMIO read/write operations. These 
>>> trace events
>>> +      can be used for logging all MMIO read/write operations.
>>> +
>>>   source "lib/crypto/Kconfig"
>>>     config CRC_CCITT
>>> diff --git a/lib/Makefile b/lib/Makefile
>>> index 300f569c626b..43813b0061cd 100644
>>> --- a/lib/Makefile
>>> +++ b/lib/Makefile
>>> @@ -152,6 +152,8 @@ lib-y += logic_pio.o
>>>     lib-$(CONFIG_INDIRECT_IOMEM) += logic_iomem.o
>>>   +obj-$(CONFIG_TRACE_MMIO_ACCESS) += trace_readwrite.o
>>> +
>>>   obj-$(CONFIG_GENERIC_HWEIGHT) += hweight.o
>>>     obj-$(CONFIG_BTREE) += btree.o
>>> diff --git a/lib/trace_readwrite.c b/lib/trace_readwrite.c
>>> new file mode 100644
>>> index 000000000000..88637038b30c
>>> --- /dev/null
>>> +++ b/lib/trace_readwrite.c
>>> @@ -0,0 +1,47 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Register read and write tracepoints
>>> + *
>>> + * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All 
>>> rights reserved.
>>> + */
>>> +
>>> +#include <linux/ftrace.h>
>>> +#include <linux/module.h>
>>> +#include <asm-generic/io.h>
>>> +
>>> +#define CREATE_TRACE_POINTS
>>> +#include <trace/events/rwmmio.h>
>>> +
>>> +#ifdef CONFIG_TRACE_MMIO_ACCESS
>>> +void log_write_mmio(u64 val, u8 width, volatile void __iomem *addr,
>>> +            unsigned long caller_addr)
>>> +{
>>> +    trace_rwmmio_write(caller_addr, val, width, addr);
>>> +}
>>> +EXPORT_SYMBOL_GPL(log_write_mmio);
>>> +EXPORT_TRACEPOINT_SYMBOL_GPL(rwmmio_write);
>>> +
>>> +void log_post_write_mmio(u64 val, u8 width, volatile void __iomem 
>>> *addr,
>>> +             unsigned long caller_addr)
>>> +{
>>> +    trace_rwmmio_post_write(caller_addr, val, width, addr);
>>> +}
>>> +EXPORT_SYMBOL_GPL(log_post_write_mmio);
>>> +EXPORT_TRACEPOINT_SYMBOL_GPL(rwmmio_post_write);
>>> +
>>> +void log_read_mmio(u8 width, const volatile void __iomem *addr,
>>> +           unsigned long caller_addr)
>>> +{
>>> +    trace_rwmmio_read(caller_addr, width, addr);
>>> +}
>>> +EXPORT_SYMBOL_GPL(log_read_mmio);
>>> +EXPORT_TRACEPOINT_SYMBOL_GPL(rwmmio_read);
>>> +
>>> +void log_post_read_mmio(u64 val, u8 width, const volatile void 
>>> __iomem *addr,
>>> +            unsigned long caller_addr)
>>> +{
>>> +    trace_rwmmio_post_read(caller_addr, val, width, addr);
>>> +}
>>> +EXPORT_SYMBOL_GPL(log_post_read_mmio);
>>> +EXPORT_TRACEPOINT_SYMBOL_GPL(rwmmio_post_read);
>>> +#endif /* CONFIG_TRACE_MMIO_ACCESS */