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;
#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.1Hi 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
> #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
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
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.
>> #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
>>
>
>
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
> >> #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
> >>
> >
> >
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
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
>>>>
>>>
>>>
>>
>
>
On Wed, Nov 19, 2025 at 12:00:30PM +0000, James Clark wrote:
[...]
> ... 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.
Armv8 CPUs can runs in aarch32 mode, strictly speaking, we should also
can run ETMv4 driver in aarch32 mode as well. Then CONFIG_ARM64 is the
right choice, this can remind us that `is_kernel_in_hyp_mode()` is
always stick to aarch64 mode.
static ssize_t format_attr_contextid_show(struct device *dev,
struct device_attribute *attr,
char *page)
{
#if IS_ENABLED(CONFIG_ARM64)
if (is_kernel_in_hyp_mode())
return contextid2_show(dev, attr, page);
#endif
return contextid1_show(dev, attr, page);
}
Thanks,
Leo
On 19/11/2025 12:36 pm, Leo Yan wrote:
> On Wed, Nov 19, 2025 at 12:00:30PM +0000, James Clark wrote:
>
> [...]
>
>> ... 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.
>
> Armv8 CPUs can runs in aarch32 mode, strictly speaking, we should also
> can run ETMv4 driver in aarch32 mode as well. Then CONFIG_ARM64 is the
> right choice, this can remind us that `is_kernel_in_hyp_mode()` is
> always stick to aarch64 mode.
>
For the avoidance of confusion, in this case CONFIG_ARM64 and
CONFIG_CORESIGHT_SOURCE_ETM4X are 100% equivalent and there's no
functional difference. Yes maybe 32 bit userspace can be traced from
ETMv4, but that's not really related with how this code is compiled.
> static ssize_t format_attr_contextid_show(struct device *dev,
> struct device_attribute *attr,
> char *page)
> {
> #if IS_ENABLED(CONFIG_ARM64)
> if (is_kernel_in_hyp_mode())
> return contextid2_show(dev, attr, page);
> #endif
>
> return contextid1_show(dev, attr, page);
Not having an #else implies that the contextid1_show() part is valid
when !CONFIG_ARM64, but that isn't right. That's why I had the WARN_ON
because it's dead code.
Personally I would drop the is_visible(). It makes sense for dynamically
hidden things, but these are all compile time. IMO it's cleaner to just
not include them to begin with, rather than include and then hide them.
Then the extra condition in format_attr_contextid_show() isn't needed
because the function doesn't exist:
GEN_PMU_FORMAT_ATTR(cycacc);
GEN_PMU_FORMAT_ATTR(timestamp);
GEN_PMU_FORMAT_ATTR(retstack);
GEN_PMU_FORMAT_ATTR(sinkid);
#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
/* contextid1 enables tracing CONTEXTIDR_EL1 for ETMv4 */
GEN_PMU_FORMAT_ATTR(contextid1);
/* contextid2 enables tracing CONTEXTIDR_EL2 for ETMv4 */
GEN_PMU_FORMAT_ATTR(contextid2);
GEN_PMU_FORMAT_ATTR(branch_broadcast);
/* preset - if sink ID is used as a configuration selector */
GEN_PMU_FORMAT_ATTR(preset);
/* config ID - set if a system configuration is selected */
GEN_PMU_FORMAT_ATTR(configid);
GEN_PMU_FORMAT_ATTR(cc_threshold);
/*
* contextid always traces the "PID". The PID is in CONTEXTIDR_EL1
* when the kernel is running at EL1; when the kernel is at EL2,
* the PID is in CONTEXTIDR_EL2.
*/
static ssize_t format_attr_contextid_show(struct device *dev,
struct device_attribute *attr,
char *page)
{
if (is_kernel_in_hyp_mode())
return contextid2_show(dev, attr, page);
return contextid1_show(dev, attr, page);
}
static struct device_attribute format_attr_contextid =
__ATTR(contextid, 0444, format_attr_contextid_show, NULL);
#endif
/*
* ETMv3 only uses the first 3 attributes for programming itself (see
* ETM3X_SUPPORTED_OPTIONS). Sink ID is also supported for selecting a
* sink in both, but not used for configuring the ETM. The remaining
* attributes are ETMv4 specific.
*/
static struct attribute *etm_config_formats_attr[] = {
&format_attr_cycacc.attr,
&format_attr_timestamp.attr,
&format_attr_retstack.attr,
&format_attr_sinkid.attr,
#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
&format_attr_contextid.attr,
&format_attr_contextid1.attr,
&format_attr_contextid2.attr,
&format_attr_preset.attr,
&format_attr_configid.attr,
&format_attr_branch_broadcast.attr,
&format_attr_cc_threshold.attr,
#endif
NULL,
};
> }
>
> Thanks,
> Leo
On Wed, Nov 19, 2025 at 01:55:15PM +0000, James Clark wrote:
[...]
> > static ssize_t format_attr_contextid_show(struct device *dev,
> > struct device_attribute *attr,
> > char *page)
> > {
> > #if IS_ENABLED(CONFIG_ARM64)
> > if (is_kernel_in_hyp_mode())
> > return contextid2_show(dev, attr, page);
> > #endif
> >
> > return contextid1_show(dev, attr, page);
>
> Not having an #else implies that the contextid1_show() part is valid when
> !CONFIG_ARM64, but that isn't right. That's why I had the WARN_ON because
> it's dead code.
Based on ETMv3/v4 spec, would contextid1 always be valid ? (Though we do
not support context ID for ETMv3 yet).
> Personally I would drop the is_visible(). It makes sense for dynamically
> hidden things, but these are all compile time. IMO it's cleaner to just not
> include them to begin with, rather than include and then hide them. Then the
> extra condition in format_attr_contextid_show() isn't needed because the
> function doesn't exist:
This is fine for me, though in general I think the dynamic approach is
readable and extendable than the compile-time approach.
Thanks,
Leo
On 19/11/2025 2:37 pm, Leo Yan wrote:
> On Wed, Nov 19, 2025 at 01:55:15PM +0000, James Clark wrote:
>
> [...]
>
>>> static ssize_t format_attr_contextid_show(struct device *dev,
>>> struct device_attribute *attr,
>>> char *page)
>>> {
>>> #if IS_ENABLED(CONFIG_ARM64)
>>> if (is_kernel_in_hyp_mode())
>>> return contextid2_show(dev, attr, page);
>>> #endif
>>>
>>> return contextid1_show(dev, attr, page);
>>
>> Not having an #else implies that the contextid1_show() part is valid when
>> !CONFIG_ARM64, but that isn't right. That's why I had the WARN_ON because
>> it's dead code.
>
> Based on ETMv3/v4 spec, would contextid1 always be valid ? (Though we do
> not support context ID for ETMv3 yet).
>
It's not currently supported for ETMv3 in perf mode, which is the
relevant thing here. So format_attr_contextid_show() never gets called
for ETMv3 (equivalent to !CONFIG_ARM64).
Based on the spec it may be supported, but that's a different discussion
and I doubt anyone wants it so it's unlikely to be added.
>> Personally I would drop the is_visible(). It makes sense for dynamically
>> hidden things, but these are all compile time. IMO it's cleaner to just not
>> include them to begin with, rather than include and then hide them. Then the
>> extra condition in format_attr_contextid_show() isn't needed because the
>> function doesn't exist:
>
> This is fine for me, though in general I think the dynamic approach is
> readable and extendable than the compile-time approach.
>
> Thanks,
> Leo
I agree in a perfect world, but it seems to have caused confusion and
wasn't that clean because is_kernel_in_hyp_mode() only exists for arm64.
I'll send a new version without it.
© 2016 - 2025 Red Hat, Inc.