Re: [PATCH v5 08/13] coresight: Interpret perf config with ATTR_CFG_GET_FLD()

[James Clark <james.clark@linaro.org>]

On 19/11/2025 11:45 am, Mike Leach wrote:
> Hi James
> 
> On Wed, 19 Nov 2025 at 11:26, James Clark <james.clark@linaro.org> wrote:
>>
>>
>>
>> On 19/11/2025 9:32 am, Mike Leach wrote:
>>> Hi James
>>>
>>> On Tue, 18 Nov 2025 at 16:28, James Clark <james.clark@linaro.org> wrote:
>>>>
>>>> The "config:" string construction in format_attr_contextid_show() can be
>>>> removed because it either showed the existing context1 or context2
>>>> formats which have already been generated, so can be called themselves.
>>>>
>>>> The other conversions are straightforward replacements.
>>>>
>>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>>> ---
>>>>    drivers/hwtracing/coresight/coresight-etm-perf.c | 26 +++++++++++++++---------
>>>>    1 file changed, 16 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>>> index 28f1bfc4579f..1b9ae832a576 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>>> @@ -80,12 +80,19 @@ static ssize_t format_attr_contextid_show(struct device *dev,
>>>>                                             struct device_attribute *attr,
>>>>                                             char *page)
>>>>    {
>>>> -       int pid_fmt = ETM_OPT_CTXTID;
>>>> +       /*
>>>> +        * is_kernel_in_hyp_mode() isn't defined in arm32 so avoid calling it
>>>> +        * even though is_visible() prevents this function from being called.
>>>> +        */
>>>> +#if IS_ENABLED(CONFIG_ARM64)
>>>> +       if (is_kernel_in_hyp_mode())
>>>> +               return contextid2_show(dev, attr, page);
>>>>
>>>> -#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
>>>> -       pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ETM_OPT_CTXTID;
>>>> +       return contextid1_show(dev, attr, page);
>>>> +#else
>>>> +       WARN_ONCE(1, "ETM contextid not supported on arm32\n");
>>>> +       return 0;
>>>
>>> Context ID is supported in aarch32 - and traced in etmv3 / ptm and etmv4.
>>>
>>> Mike
>>>
>>
>> Not in Perf mode unless I'm missing something:
>>
>> #define ETM3X_SUPPORTED_OPTIONS (ETMCR_CYC_ACC | \
>>                                   ETMCR_TIMESTAMP_EN | \
>>                                   ETMCR_RETURN_STACK)
>>
>> Only these options are currently accepted. I suppose the comment is
>> describing what the current behavior is, whether that is what we want is
>> another thing.
>>
>> But if we do start supporting context ID in ETMv3 we can update that
>> comment.
>>
> 
> Unlikely - but the comment seems to conflate core architecture and etm
> architecture.
> A core with aarch32 can be traced in etm4 - i.e etm4 supports A64, A32
> and T32 instruction sets.
> If this set gets another version it might be worth saying "not
> ETMv3/PTM" rather than not A32.
> 
> Mike
> 
> 

Oh I see what you mean, yes that would be slightly better. But then to 
make the code match the warning it might also make sense to change 
CONFIG_ARM64 back to CONFIG_CORESIGHT_SOURCE_ETM4X, which Leo suggested 
to change. Maybe I can just delete the warning text, practically this 
warning can never be hit.

It would have been nicer to avoid any conditional compilation or 
warnings and comments, because we're hiding contextid on ETMv3 anyway. 
It's unfortunate that the compiler doesn't allow us to ignore 
is_kernel_in_hyp_mode() being undefined even with short circuit evaluation.

>>>>    #endif
>>>> -       return sprintf(page, "config:%d\n", pid_fmt);
>>>>    }
>>>>
>>>>    static struct device_attribute format_attr_contextid =
>>>> @@ -337,7 +344,7 @@ static bool sinks_compatible(struct coresight_device *a,
>>>>    static void *etm_setup_aux(struct perf_event *event, void **pages,
>>>>                              int nr_pages, bool overwrite)
>>>>    {
>>>> -       u32 id, cfg_hash;
>>>> +       u32 sink_hash, cfg_hash;
>>>>           int cpu = event->cpu;
>>>>           cpumask_t *mask;
>>>>           struct coresight_device *sink = NULL;
>>>> @@ -350,13 +357,12 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>>>>           INIT_WORK(&event_data->work, free_event_data);
>>>>
>>>>           /* First get the selected sink from user space. */
>>>> -       if (event->attr.config2 & GENMASK_ULL(31, 0)) {
>>>> -               id = (u32)event->attr.config2;
>>>> -               sink = user_sink = coresight_get_sink_by_id(id);
>>>> -       }
>>>> +       sink_hash = ATTR_CFG_GET_FLD(&event->attr, sinkid);
>>>> +       if (sink_hash)
>>>> +               sink = user_sink = coresight_get_sink_by_id(sink_hash);
>>>>
>>>>           /* check if user wants a coresight configuration selected */
>>>> -       cfg_hash = (u32)((event->attr.config2 & GENMASK_ULL(63, 32)) >> 32);
>>>> +       cfg_hash = ATTR_CFG_GET_FLD(&event->attr, configid);
>>>>           if (cfg_hash) {
>>>>                   if (cscfg_activate_config(cfg_hash))
>>>>                           goto err;
>>>>
>>>> --
>>>> 2.34.1
>>>>
>>>
>>>
>>
> 
>