[PATCH 0/7] perf cs-etm/arm-spe: Remove hard coded config fields

[PATCH 0/7] perf cs-etm/arm-spe: Remove hard coded config fields

[James Clark]

The specific config field that an event format attribute is in is
consistently hard coded, even though the API is supposed to be that the
driver publishes the config field name. To stop this pattern from being
copy pasted and causing problems in the future, replace them all with
calls to a new helper that returns the value that a user set.

The existing evsel__set_config_if_unset() also has a similar problem
that it hard codes attr.config, so fix that too.

There are no functional changes here because all the fields touched are
in attr.config and not config1 or config2 etc. Although this may not be
the case for new fields.

Signed-off-by: James Clark <james.clark@linaro.org>
---
James Clark (7):
      perf tools: Track all user changed config bits
      perf tools: apply evsel__set_config_if_unset() to all config fields
      perf cs-etm: Make a helper to find the Coresight evsel
      perf cs-etm: Don't use hard coded config bits when setting up ETMCR
      perf cs-etm: Don't use hard coded config bits when setting up TRCCONFIGR
      perf cs-etm: Don't hard code config attribute when configuring the event
      perf arm-spe: Don't hard code config attribute

 tools/perf/arch/arm/util/cs-etm.c    | 202 ++++++++++++++++++++---------------
 tools/perf/arch/arm64/util/arm-spe.c |  15 +--
 tools/perf/util/evsel.c              |   6 +-
 tools/perf/util/evsel.h              |   2 +
 tools/perf/util/evsel_config.h       |   6 +-
 tools/perf/util/parse-events.c       |  89 +++++++--------
 tools/perf/util/pmu.c                |  93 +++++++++++++---
 7 files changed, 251 insertions(+), 162 deletions(-)
---
base-commit: 834ebb5678d75d844f5d4f44ede78724d8c96630
change-id: 20251112-james-perf-config-bits-bee7106f0f00

Best regards,
-- 
James Clark <james.clark@linaro.org>

[PATCH 1/7] perf tools: Track all user changed config bits

[James Clark]

Currently we only track which bits were set by the user in attr->config.
But all configN fields should be treated equally as they can all have
default and user overridden values.

Track them all by making get_config_chgs() generic and calling it once
for each config value.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/perf/util/evsel.c        |  6 ++-
 tools/perf/util/evsel_config.h |  6 ++-
 tools/perf/util/parse-events.c | 89 +++++++++++++++++-------------------------
 tools/perf/util/pmu.c          |  2 +-
 4 files changed, 47 insertions(+), 56 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index aee42666e882..40d68f346007 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1238,7 +1238,11 @@ static void evsel__apply_config_terms(struct evsel *evsel,
 		case EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE:
 			/* Already applied by auxtrace */
 			break;
-		case EVSEL__CONFIG_TERM_CFG_CHG:
+		case EVSEL__CONFIG_TERM_USR_CHG_CONFIG:
+		case EVSEL__CONFIG_TERM_USR_CHG_CONFIG1:
+		case EVSEL__CONFIG_TERM_USR_CHG_CONFIG2:
+		case EVSEL__CONFIG_TERM_USR_CHG_CONFIG3:
+		case EVSEL__CONFIG_TERM_USR_CHG_CONFIG4:
 			break;
 		case EVSEL__CONFIG_TERM_RATIO_TO_PREV:
 			rtp_buf = term->val.str;
diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
index bcd3a978f0c4..b38190dc5a40 100644
--- a/tools/perf/util/evsel_config.h
+++ b/tools/perf/util/evsel_config.h
@@ -27,7 +27,11 @@ enum evsel_term_type {
 	EVSEL__CONFIG_TERM_AUX_OUTPUT,
 	EVSEL__CONFIG_TERM_AUX_ACTION,
 	EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE,
-	EVSEL__CONFIG_TERM_CFG_CHG,
+	EVSEL__CONFIG_TERM_USR_CHG_CONFIG,
+	EVSEL__CONFIG_TERM_USR_CHG_CONFIG1,
+	EVSEL__CONFIG_TERM_USR_CHG_CONFIG2,
+	EVSEL__CONFIG_TERM_USR_CHG_CONFIG3,
+	EVSEL__CONFIG_TERM_USR_CHG_CONFIG4,
 	EVSEL__CONFIG_TERM_RATIO_TO_PREV,
 };
 
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 17c1c36a7bf9..f1cc6e96a371 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1235,63 +1235,46 @@ do {								\
 	return 0;
 }
 
+#define ADD_CONFIG_CHG(format_type, term_type, new_term)		\
+{									\
+	struct parse_events_term *term;					\
+	u64 bits = 0;							\
+	int type;							\
+									\
+	list_for_each_entry(term, &head_config->terms, list) {		\
+		if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER) {  \
+			type = perf_pmu__format_type(pmu, term->config);\
+			if (type != format_type)			\
+				continue;				\
+			bits |= perf_pmu__format_bits(pmu, term->config); \
+		} else if (term->type_term == term_type) {		\
+			bits = ~(u64)0;					\
+		}							\
+	}								\
+									\
+	if (bits)							\
+		ADD_CONFIG_TERM_VAL(new_term, cfg_chg, bits, false);	\
+	return 0;							\
+}
+
 /*
- * Add EVSEL__CONFIG_TERM_CFG_CHG where cfg_chg will have a bit set for
- * each bit of attr->config that the user has changed.
+ * Add EVSEL__CONFIG_TERM_USR_CFG_CONFIGn where cfg_chg will have a bit set for
+ * each bit of attr->configN that the user has changed.
  */
-static int get_config_chgs(struct perf_pmu *pmu, struct parse_events_terms *head_config,
+static int get_config_chgs(struct perf_pmu *pmu,
+			   struct parse_events_terms *head_config,
 			   struct list_head *head_terms)
 {
-	struct parse_events_term *term;
-	u64 bits = 0;
-	int type;
-
-	list_for_each_entry(term, &head_config->terms, list) {
-		switch (term->type_term) {
-		case PARSE_EVENTS__TERM_TYPE_USER:
-			type = perf_pmu__format_type(pmu, term->config);
-			if (type != PERF_PMU_FORMAT_VALUE_CONFIG)
-				continue;
-			bits |= perf_pmu__format_bits(pmu, term->config);
-			break;
-		case PARSE_EVENTS__TERM_TYPE_CONFIG:
-			bits = ~(u64)0;
-			break;
-		case PARSE_EVENTS__TERM_TYPE_CONFIG1:
-		case PARSE_EVENTS__TERM_TYPE_CONFIG2:
-		case PARSE_EVENTS__TERM_TYPE_CONFIG3:
-		case PARSE_EVENTS__TERM_TYPE_CONFIG4:
-		case PARSE_EVENTS__TERM_TYPE_LEGACY_HARDWARE_CONFIG:
-		case PARSE_EVENTS__TERM_TYPE_LEGACY_CACHE_CONFIG:
-		case PARSE_EVENTS__TERM_TYPE_NAME:
-		case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
-		case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
-		case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
-		case PARSE_EVENTS__TERM_TYPE_TIME:
-		case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
-		case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
-		case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
-		case PARSE_EVENTS__TERM_TYPE_INHERIT:
-		case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
-		case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS:
-		case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
-		case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
-		case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
-		case PARSE_EVENTS__TERM_TYPE_PERCORE:
-		case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT:
-		case PARSE_EVENTS__TERM_TYPE_AUX_ACTION:
-		case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
-		case PARSE_EVENTS__TERM_TYPE_METRIC_ID:
-		case PARSE_EVENTS__TERM_TYPE_RAW:
-		case PARSE_EVENTS__TERM_TYPE_CPU:
-		case PARSE_EVENTS__TERM_TYPE_RATIO_TO_PREV:
-		default:
-			break;
-		}
-	}
-
-	if (bits)
-		ADD_CONFIG_TERM_VAL(CFG_CHG, cfg_chg, bits, false);
+	ADD_CONFIG_CHG(PERF_PMU_FORMAT_VALUE_CONFIG,
+		       PARSE_EVENTS__TERM_TYPE_CONFIG, USR_CHG_CONFIG);
+	ADD_CONFIG_CHG(PERF_PMU_FORMAT_VALUE_CONFIG1,
+		       PARSE_EVENTS__TERM_TYPE_CONFIG1, USR_CHG_CONFIG1);
+	ADD_CONFIG_CHG(PERF_PMU_FORMAT_VALUE_CONFIG2,
+		       PARSE_EVENTS__TERM_TYPE_CONFIG2, USR_CHG_CONFIG2);
+	ADD_CONFIG_CHG(PERF_PMU_FORMAT_VALUE_CONFIG3,
+		       PARSE_EVENTS__TERM_TYPE_CONFIG3, USR_CHG_CONFIG3);
+	ADD_CONFIG_CHG(PERF_PMU_FORMAT_VALUE_CONFIG4,
+		       PARSE_EVENTS__TERM_TYPE_CONFIG4, USR_CHG_CONFIG4);
 
 #undef ADD_CONFIG_TERM
 	return 0;
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 1b7c712d8f99..7a7db31be70d 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1384,7 +1384,7 @@ void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel,
 				const char *config_name, u64 val)
 {
 	u64 user_bits = 0, bits;
-	struct evsel_config_term *term = evsel__get_config_term(evsel, CFG_CHG);
+	struct evsel_config_term *term = evsel__get_config_term(evsel, USR_CHG_CONFIG);
 
 	if (term)
 		user_bits = term->val.cfg_chg;

-- 
2.34.1

Re: [PATCH 1/7] perf tools: Track all user changed config bits

[Leo Yan]

On Mon, Dec 01, 2025 at 04:41:04PM +0000, Coresight ML wrote:

[...]

> +#define ADD_CONFIG_CHG(format_type, term_type, new_term)		\
> +{									\
> +	struct parse_events_term *term;					\
> +	u64 bits = 0;							\
> +	int type;							\
> +									\
> +	list_for_each_entry(term, &head_config->terms, list) {		\
> +		if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER) {  \
> +			type = perf_pmu__format_type(pmu, term->config);\
> +			if (type != format_type)			\
> +				continue;				\
> +			bits |= perf_pmu__format_bits(pmu, term->config); \
> +		} else if (term->type_term == term_type) {		\
> +			bits = ~(u64)0;					\
> +		}							\
> +	}								\
> +									\
> +	if (bits)							\
> +		ADD_CONFIG_TERM_VAL(new_term, cfg_chg, bits, false);	\
> +	return 0;							\

Nitpick: "return 0" is not needed here.  Otherwise:

Reviewed-by: Leo Yan <leo.yan@arm.com>

Re: [PATCH 1/7] perf tools: Track all user changed config bits

[James Clark]

On 02/12/2025 10:15 am, Leo Yan wrote:
> On Mon, Dec 01, 2025 at 04:41:04PM +0000, Coresight ML wrote:
> 
> [...]
> 
>> +#define ADD_CONFIG_CHG(format_type, term_type, new_term)		\
>> +{									\
>> +	struct parse_events_term *term;					\
>> +	u64 bits = 0;							\
>> +	int type;							\
>> +									\
>> +	list_for_each_entry(term, &head_config->terms, list) {		\
>> +		if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER) {  \
>> +			type = perf_pmu__format_type(pmu, term->config);\
>> +			if (type != format_type)			\
>> +				continue;				\
>> +			bits |= perf_pmu__format_bits(pmu, term->config); \
>> +		} else if (term->type_term == term_type) {		\
>> +			bits = ~(u64)0;					\
>> +		}							\
>> +	}								\
>> +									\
>> +	if (bits)							\
>> +		ADD_CONFIG_TERM_VAL(new_term, cfg_chg, bits, false);	\
>> +	return 0;							\
> 
> Nitpick: "return 0" is not needed here.  Otherwise:
> 
> Reviewed-by: Leo Yan <leo.yan@arm.com>

I think it's worse than not needed, it makes it stop collecting the 
changes after the first one.

It was annoying that this had to be a macro instead of a function 
because ADD_CONFIG_TERM_VAL constructs the #define name of the first 
argument.

It's probably worth putting in some effort to make ADD_CONFIG_CHG() a 
function to avoid problems like this and maybe add a test.

Re: [PATCH 1/7] perf tools: Track all user changed config bits

[Leo Yan]

On Tue, Dec 02, 2025 at 10:40:49AM +0000, James Clark wrote:
> 
> 
> On 02/12/2025 10:15 am, Leo Yan wrote:
> > On Mon, Dec 01, 2025 at 04:41:04PM +0000, Coresight ML wrote:
> > 
> > [...]
> > 
> > > +#define ADD_CONFIG_CHG(format_type, term_type, new_term)		\
> > > +{									\
> > > +	struct parse_events_term *term;					\
> > > +	u64 bits = 0;							\
> > > +	int type;							\
> > > +									\
> > > +	list_for_each_entry(term, &head_config->terms, list) {		\
> > > +		if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER) {  \
> > > +			type = perf_pmu__format_type(pmu, term->config);\
> > > +			if (type != format_type)			\
> > > +				continue;				\
> > > +			bits |= perf_pmu__format_bits(pmu, term->config); \
> > > +		} else if (term->type_term == term_type) {		\
> > > +			bits = ~(u64)0;					\
> > > +		}							\
> > > +	}								\
> > > +									\
> > > +	if (bits)							\
> > > +		ADD_CONFIG_TERM_VAL(new_term, cfg_chg, bits, false);	\
> > > +	return 0;							\
> > 
> > Nitpick: "return 0" is not needed here.  Otherwise:
> > 
> > Reviewed-by: Leo Yan <leo.yan@arm.com>
> 
> I think it's worse than not needed, it makes it stop collecting the changes
> after the first one.

Just curious how this can happen.

  foo()
  {
      {
          chunk 1;
      }

      {
          chunk 2;
      }
  }

Seem to me, if without "return 0" in chunk 1, it still can continue to
run chunk 2, no?

> It was annoying that this had to be a macro instead of a function because
> ADD_CONFIG_TERM_VAL constructs the #define name of the first argument.
> 
> It's probably worth putting in some effort to make ADD_CONFIG_CHG() a
> function to avoid problems like this and maybe add a test.

This would be fine for me.

Thanks,
Leo

Re: [PATCH 1/7] perf tools: Track all user changed config bits

[Leo Yan]

On Tue, Dec 02, 2025 at 11:21:47AM +0000, Coresight ML wrote:
> On Tue, Dec 02, 2025 at 10:40:49AM +0000, James Clark wrote:
> > 
> > 
> > On 02/12/2025 10:15 am, Leo Yan wrote:
> > > On Mon, Dec 01, 2025 at 04:41:04PM +0000, Coresight ML wrote:
> > > 
> > > [...]
> > > 
> > > > +#define ADD_CONFIG_CHG(format_type, term_type, new_term)		\
> > > > +{									\
> > > > +	struct parse_events_term *term;					\
> > > > +	u64 bits = 0;							\
> > > > +	int type;							\
> > > > +									\
> > > > +	list_for_each_entry(term, &head_config->terms, list) {		\
> > > > +		if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER) {  \
> > > > +			type = perf_pmu__format_type(pmu, term->config);\
> > > > +			if (type != format_type)			\
> > > > +				continue;				\
> > > > +			bits |= perf_pmu__format_bits(pmu, term->config); \
> > > > +		} else if (term->type_term == term_type) {		\
> > > > +			bits = ~(u64)0;					\
> > > > +		}							\
> > > > +	}								\
> > > > +									\
> > > > +	if (bits)							\
> > > > +		ADD_CONFIG_TERM_VAL(new_term, cfg_chg, bits, false);	\
> > > > +	return 0;							\
> > > 
> > > Nitpick: "return 0" is not needed here.  Otherwise:
> > > 
> > > Reviewed-by: Leo Yan <leo.yan@arm.com>
> > 
> > I think it's worse than not needed, it makes it stop collecting the changes
> > after the first one.
> 
> Just curious how this can happen.
> 
>   foo()
>   {
>       {
>           chunk 1;
>       }
> 
>       {
>           chunk 2;
>       }
>   }
> 
> Seem to me, if without "return 0" in chunk 1, it still can continue to
> run chunk 2, no?

Please ignore my question above.  We need to iterate from config to
config4, if return 0 when handling config, then never add other
configs?

Thanks,
Leo

[PATCH 2/7] perf tools: apply evsel__set_config_if_unset() to all config fields

[James Clark]

Misleadingly, evsel__set_config_if_unset() only works with the config
field and not config1, config2, etc. This is fine at the moment because
all users of it happen to operate on bits that are in that config field.
Fix it before there are any new users of the function which operate on
bits in different config fields.

In theory it's also possible for a driver to move an existing bit to
another config field and this fixes that scenario too, although this
hasn't happened yet either.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/perf/util/pmu.c | 60 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 7a7db31be70d..c8968cddc0a9 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/list.h>
+#include <linux/bitfield.h>
 #include <linux/compiler.h>
 #include <linux/string.h>
 #include <linux/zalloc.h>
@@ -1371,6 +1372,18 @@ bool evsel__is_aux_event(const struct evsel *evsel)
 	return pmu && pmu->auxtrace;
 }
 
+static struct perf_pmu_format *
+pmu_find_format(const struct list_head *formats, const char *name)
+{
+	struct perf_pmu_format *format;
+
+	list_for_each_entry(format, formats, list)
+		if (!strcmp(format->name, name))
+			return format;
+
+	return NULL;
+}
+
 /*
  * Set @config_name to @val as long as the user hasn't already set or cleared it
  * by passing a config term on the command line.
@@ -1379,12 +1392,39 @@ bool evsel__is_aux_event(const struct evsel *evsel)
  * the bit pattern. It is shifted into position by this function, so to set
  * something to true, pass 1 for val rather than a pre shifted value.
  */
-#define field_prep(_mask, _val) (((_val) << (ffsll(_mask) - 1)) & (_mask))
 void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel,
 				const char *config_name, u64 val)
 {
 	u64 user_bits = 0, bits;
-	struct evsel_config_term *term = evsel__get_config_term(evsel, USR_CHG_CONFIG);
+	struct evsel_config_term *term;
+	struct perf_pmu_format *format = pmu_find_format(&pmu->format, config_name);
+	__u64 *vp;
+
+	switch (format->value) {
+	case PERF_PMU_FORMAT_VALUE_CONFIG:
+		term = evsel__get_config_term(evsel, USR_CHG_CONFIG);
+		vp = &evsel->core.attr.config;
+		break;
+	case PERF_PMU_FORMAT_VALUE_CONFIG1:
+		term = evsel__get_config_term(evsel, USR_CHG_CONFIG1);
+		vp = &evsel->core.attr.config1;
+		break;
+	case PERF_PMU_FORMAT_VALUE_CONFIG2:
+		term = evsel__get_config_term(evsel, USR_CHG_CONFIG2);
+		vp = &evsel->core.attr.config2;
+		break;
+	case PERF_PMU_FORMAT_VALUE_CONFIG3:
+		term = evsel__get_config_term(evsel, USR_CHG_CONFIG3);
+		vp = &evsel->core.attr.config3;
+		break;
+	case PERF_PMU_FORMAT_VALUE_CONFIG4:
+		term = evsel__get_config_term(evsel, USR_CHG_CONFIG4);
+		vp = &evsel->core.attr.config4;
+		break;
+	default:
+		pr_err("Unknown format value: %d\n", format->value);
+		return;
+	}
 
 	if (term)
 		user_bits = term->val.cfg_chg;
@@ -1396,20 +1436,8 @@ void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel,
 		return;
 
 	/* Otherwise replace it */
-	evsel->core.attr.config &= ~bits;
-	evsel->core.attr.config |= field_prep(bits, val);
-}
-
-static struct perf_pmu_format *
-pmu_find_format(const struct list_head *formats, const char *name)
-{
-	struct perf_pmu_format *format;
-
-	list_for_each_entry(format, formats, list)
-		if (!strcmp(format->name, name))
-			return format;
-
-	return NULL;
+	*vp &= ~bits;
+	*vp |= FIELD_PREP(bits, val);
 }
 
 __u64 perf_pmu__format_bits(struct perf_pmu *pmu, const char *name)

-- 
2.34.1

Re: [PATCH 2/7] perf tools: apply evsel__set_config_if_unset() to all config fields

[Leo Yan]

On Mon, Dec 01, 2025 at 04:41:05PM +0000, Coresight ML wrote:

[...]

>  void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel,
>  				const char *config_name, u64 val)
>  {
>  	u64 user_bits = 0, bits;
> -	struct evsel_config_term *term = evsel__get_config_term(evsel, USR_CHG_CONFIG);
> +	struct evsel_config_term *term;
> +	struct perf_pmu_format *format = pmu_find_format(&pmu->format, config_name);
> +	__u64 *vp;

Should check (format == NULL) here and bail out for a unknown config name?

> +
> +	switch (format->value) {
> +	case PERF_PMU_FORMAT_VALUE_CONFIG:
> +		term = evsel__get_config_term(evsel, USR_CHG_CONFIG);
> +		vp = &evsel->core.attr.config;
> +		break;
> +	case PERF_PMU_FORMAT_VALUE_CONFIG1:
> +		term = evsel__get_config_term(evsel, USR_CHG_CONFIG1);
> +		vp = &evsel->core.attr.config1;
> +		break;
> +	case PERF_PMU_FORMAT_VALUE_CONFIG2:
> +		term = evsel__get_config_term(evsel, USR_CHG_CONFIG2);
> +		vp = &evsel->core.attr.config2;
> +		break;
> +	case PERF_PMU_FORMAT_VALUE_CONFIG3:
> +		term = evsel__get_config_term(evsel, USR_CHG_CONFIG3);
> +		vp = &evsel->core.attr.config3;
> +		break;
> +	case PERF_PMU_FORMAT_VALUE_CONFIG4:
> +		term = evsel__get_config_term(evsel, USR_CHG_CONFIG4);
> +		vp = &evsel->core.attr.config4;
> +		break;
> +	default:
> +		pr_err("Unknown format value: %d\n", format->value);
> +		return;
> +	}
>  
>  	if (term)
>  		user_bits = term->val.cfg_chg;
> @@ -1396,20 +1436,8 @@ void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel,
>  		return;
>  
>  	/* Otherwise replace it */
> -	evsel->core.attr.config &= ~bits;
> -	evsel->core.attr.config |= field_prep(bits, val);
> -}
> -
> -static struct perf_pmu_format *
> -pmu_find_format(const struct list_head *formats, const char *name)
> -{
> -	struct perf_pmu_format *format;
> -
> -	list_for_each_entry(format, formats, list)
> -		if (!strcmp(format->name, name))
> -			return format;
> -
> -	return NULL;
> +	*vp &= ~bits;
> +	*vp |= FIELD_PREP(bits, val);
>  }

Re: [PATCH 2/7] perf tools: apply evsel__set_config_if_unset() to all config fields

[James Clark]

On 02/12/2025 11:14 am, Leo Yan wrote:
> On Mon, Dec 01, 2025 at 04:41:05PM +0000, Coresight ML wrote:
> 
> [...]
> 
>>   void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel,
>>   				const char *config_name, u64 val)
>>   {
>>   	u64 user_bits = 0, bits;
>> -	struct evsel_config_term *term = evsel__get_config_term(evsel, USR_CHG_CONFIG);
>> +	struct evsel_config_term *term;
>> +	struct perf_pmu_format *format = pmu_find_format(&pmu->format, config_name);
>> +	__u64 *vp;
> 
> Should check (format == NULL) here and bail out for a unknown config name?
> 

Good point, yes I'll return without any errors. It will be slightly 
inconsistent with the other config functions that return an error if 
it's not found, but it's consistent with the existing function which is 
a nop in that case.

>> +
>> +	switch (format->value) {
>> +	case PERF_PMU_FORMAT_VALUE_CONFIG:
>> +		term = evsel__get_config_term(evsel, USR_CHG_CONFIG);
>> +		vp = &evsel->core.attr.config;
>> +		break;
>> +	case PERF_PMU_FORMAT_VALUE_CONFIG1:
>> +		term = evsel__get_config_term(evsel, USR_CHG_CONFIG1);
>> +		vp = &evsel->core.attr.config1;
>> +		break;
>> +	case PERF_PMU_FORMAT_VALUE_CONFIG2:
>> +		term = evsel__get_config_term(evsel, USR_CHG_CONFIG2);
>> +		vp = &evsel->core.attr.config2;
>> +		break;
>> +	case PERF_PMU_FORMAT_VALUE_CONFIG3:
>> +		term = evsel__get_config_term(evsel, USR_CHG_CONFIG3);
>> +		vp = &evsel->core.attr.config3;
>> +		break;
>> +	case PERF_PMU_FORMAT_VALUE_CONFIG4:
>> +		term = evsel__get_config_term(evsel, USR_CHG_CONFIG4);
>> +		vp = &evsel->core.attr.config4;
>> +		break;
>> +	default:
>> +		pr_err("Unknown format value: %d\n", format->value);
>> +		return;
>> +	}
>>   
>>   	if (term)
>>   		user_bits = term->val.cfg_chg;
>> @@ -1396,20 +1436,8 @@ void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel,
>>   		return;
>>   
>>   	/* Otherwise replace it */
>> -	evsel->core.attr.config &= ~bits;
>> -	evsel->core.attr.config |= field_prep(bits, val);
>> -}
>> -
>> -static struct perf_pmu_format *
>> -pmu_find_format(const struct list_head *formats, const char *name)
>> -{
>> -	struct perf_pmu_format *format;
>> -
>> -	list_for_each_entry(format, formats, list)
>> -		if (!strcmp(format->name, name))
>> -			return format;
>> -
>> -	return NULL;
>> +	*vp &= ~bits;
>> +	*vp |= FIELD_PREP(bits, val);
>>   }

[PATCH 3/7] perf cs-etm: Make a helper to find the Coresight evsel

[James Clark]

This pattern occurs a few times and we'll add another one later, so add
a helper function for it.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/perf/arch/arm/util/cs-etm.c | 50 +++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index ea891d12f8f4..22c6272e8c36 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -302,6 +302,19 @@ static int cs_etm_set_sink_attr(struct perf_pmu *pmu,
 	return 0;
 }
 
+static struct evsel *cs_etm_get_evsel(struct evlist *evlist,
+				      struct perf_pmu *cs_etm_pmu)
+{
+	struct evsel *evsel;
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (evsel->core.attr.type == cs_etm_pmu->type)
+			return evsel;
+	}
+
+	return NULL;
+}
+
 static int cs_etm_recording_options(struct auxtrace_record *itr,
 				    struct evlist *evlist,
 				    struct record_opts *opts)
@@ -476,29 +489,21 @@ static int cs_etm_recording_options(struct auxtrace_record *itr,
 
 static u64 cs_etm_get_config(struct auxtrace_record *itr)
 {
-	u64 config = 0;
 	struct cs_etm_recording *ptr =
 			container_of(itr, struct cs_etm_recording, itr);
 	struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
 	struct evlist *evlist = ptr->evlist;
-	struct evsel *evsel;
+	struct evsel *evsel = cs_etm_get_evsel(evlist, cs_etm_pmu);
 
-	evlist__for_each_entry(evlist, evsel) {
-		if (evsel->core.attr.type == cs_etm_pmu->type) {
-			/*
-			 * Variable perf_event_attr::config is assigned to
-			 * ETMv3/PTM.  The bit fields have been made to match
-			 * the ETMv3.5 ETRMCR register specification.  See the
-			 * PMU_FORMAT_ATTR() declarations in
-			 * drivers/hwtracing/coresight/coresight-perf.c for
-			 * details.
-			 */
-			config = evsel->core.attr.config;
-			break;
-		}
-	}
-
-	return config;
+	/*
+	 * Variable perf_event_attr::config is assigned to
+	 * ETMv3/PTM.  The bit fields have been made to match
+	 * the ETMv3.5 ETRMCR register specification.  See the
+	 * PMU_FORMAT_ATTR() declarations in
+	 * drivers/hwtracing/coresight/coresight-perf.c for
+	 * details.
+	 */
+	return evsel ? evsel->core.attr.config : 0;
 }
 
 #ifndef BIT
@@ -832,12 +837,11 @@ static int cs_etm_snapshot_start(struct auxtrace_record *itr)
 {
 	struct cs_etm_recording *ptr =
 			container_of(itr, struct cs_etm_recording, itr);
-	struct evsel *evsel;
+	struct evsel *evsel = cs_etm_get_evsel(ptr->evlist, ptr->cs_etm_pmu);
+
+	if (evsel)
+		return evsel__disable(evsel);
 
-	evlist__for_each_entry(ptr->evlist, evsel) {
-		if (evsel->core.attr.type == ptr->cs_etm_pmu->type)
-			return evsel__disable(evsel);
-	}
 	return -EINVAL;
 }
 

-- 
2.34.1

Re: [PATCH 3/7] perf cs-etm: Make a helper to find the Coresight evsel

[Leo Yan]

On Mon, Dec 01, 2025 at 04:41:06PM +0000, Coresight ML wrote:
> This pattern occurs a few times and we'll add another one later, so add
> a helper function for it.
> 
> Signed-off-by: James Clark <james.clark@linaro.org>

Reviewed-by: Leo Yan <leo.yan@arm.com>

[PATCH 4/7] perf cs-etm: Don't use hard coded config bits when setting up ETMCR

[James Clark]

Perf only looks at attr.config when determining what was programmed into
ETMCR. These bits could theoretically be in any of the config fields.
Add a generic helper to find the value of any named format field in any
config field and then use it to get the attributes relevant to ETMCR.

The kernel will also stop publishing the ETMCR register bits in a header
[1] so preempt that by defining them here.

[1]: https://lore.kernel.org/linux-arm-kernel/20251128-james-cs-syncfreq-v8-10-4d319764cc58@linaro.org/
Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/perf/arch/arm/util/cs-etm.c | 35 ++++++++++++++++++++++++++++++++++-
 tools/perf/util/evsel.h           |  2 ++
 tools/perf/util/pmu.c             | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index 22c6272e8c36..414cafb21c98 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -68,6 +68,12 @@ static const char * const metadata_ete_ro[] = {
 
 enum cs_etm_version { CS_NOT_PRESENT, CS_ETMV3, CS_ETMV4, CS_ETE };
 
+
+/* ETMv3 ETMCR register bits */
+#define ETMCR_CYC_ACC		BIT(12)
+#define ETMCR_TIMESTAMP_EN	BIT(28)
+#define ETMCR_RETURN_STACK	BIT(29)
+
 static bool cs_etm_is_ete(struct perf_pmu *cs_etm_pmu, struct perf_cpu cpu);
 static int cs_etm_get_ro(struct perf_pmu *pmu, struct perf_cpu cpu, const char *path, __u64 *val);
 static bool cs_etm_pmu_path_exists(struct perf_pmu *pmu, struct perf_cpu cpu, const char *path);
@@ -487,6 +493,33 @@ static int cs_etm_recording_options(struct auxtrace_record *itr,
 	return err;
 }
 
+static u64 cs_etm_guess_etmcr(struct auxtrace_record *itr)
+{
+	struct cs_etm_recording *ptr =
+		container_of(itr, struct cs_etm_recording, itr);
+	struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
+	struct evsel *evsel = cs_etm_get_evsel(ptr->evlist, cs_etm_pmu);
+	u64 etmcr = 0;
+	u64 val;
+
+	if (!evsel)
+		return 0;
+
+	/*
+	 * Roughly guess what the kernel programmed into ETMCR based on
+	 * what options the event was opened with. This doesn't have to be
+	 * complete or 100% accurate, not all bits used by OpenCSD anyway.
+	 */
+	if (!evsel__get_config_val(cs_etm_pmu, evsel, "cycacc", &val) && val)
+		etmcr |= ETMCR_CYC_ACC;
+	if (!evsel__get_config_val(cs_etm_pmu, evsel, "timestamp", &val) && val)
+		etmcr |= ETMCR_TIMESTAMP_EN;
+	if (!evsel__get_config_val(cs_etm_pmu, evsel, "retstack", &val) && val)
+		etmcr |= ETMCR_RETURN_STACK;
+
+	return etmcr;
+}
+
 static u64 cs_etm_get_config(struct auxtrace_record *itr)
 {
 	struct cs_etm_recording *ptr =
@@ -746,7 +779,7 @@ static void cs_etm_get_metadata(struct perf_cpu cpu, u32 *offset,
 	case CS_ETMV3:
 		magic = __perf_cs_etmv3_magic;
 		/* Get configuration register */
-		info->priv[*offset + CS_ETM_ETMCR] = cs_etm_get_config(itr);
+		info->priv[*offset + CS_ETM_ETMCR] = cs_etm_guess_etmcr(itr);
 		/* traceID set to legacy value in case new perf running on old system */
 		info->priv[*offset + CS_ETM_ETMTRACEIDR] = cs_etm_get_legacy_trace_id(cpu);
 		/* Get read-only information from sysFS */
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 3ae4ac8f9a37..1c567cc70a82 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -574,6 +574,8 @@ void evsel__uniquify_counter(struct evsel *counter);
 	((((src) >> (pos)) & ((1ull << (size)) - 1)) << (63 - ((pos) + (size) - 1)))
 
 u64 evsel__bitfield_swap_branch_flags(u64 value);
+int evsel__get_config_val(struct perf_pmu *pmu, struct evsel *evsel,
+			  const char *config_name, u64 *val);
 void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel,
 				const char *config_name, u64 val);
 
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index c8968cddc0a9..5501b0230097 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1384,6 +1384,39 @@ pmu_find_format(const struct list_head *formats, const char *name)
 	return NULL;
 }
 
+int evsel__get_config_val(struct perf_pmu *pmu, struct evsel *evsel,
+			  const char *config_name, u64 *val)
+{
+	struct perf_pmu_format *format = pmu_find_format(&pmu->format, config_name);
+	u64 bits = perf_pmu__format_bits(pmu, config_name);
+
+	if (!format || !bits) {
+		pr_err("Unknown/empty format name: %s\n", config_name);
+		return -EINVAL;
+	}
+
+	switch (format->value) {
+	case PERF_PMU_FORMAT_VALUE_CONFIG:
+		*val = FIELD_GET(bits, evsel->core.attr.config);
+		return 0;
+	case PERF_PMU_FORMAT_VALUE_CONFIG1:
+		*val = FIELD_GET(bits, evsel->core.attr.config1);
+		return 0;
+	case PERF_PMU_FORMAT_VALUE_CONFIG2:
+		*val = FIELD_GET(bits, evsel->core.attr.config2);
+		return 0;
+	case PERF_PMU_FORMAT_VALUE_CONFIG3:
+		*val = FIELD_GET(bits, evsel->core.attr.config3);
+		return 0;
+	case PERF_PMU_FORMAT_VALUE_CONFIG4:
+		*val = FIELD_GET(bits, evsel->core.attr.config4);
+		return 0;
+	default:
+		pr_err("Unknown format value: %d\n", format->value);
+		return -EINVAL;
+	}
+}
+
 /*
  * Set @config_name to @val as long as the user hasn't already set or cleared it
  * by passing a config term on the command line.

-- 
2.34.1

Re: [PATCH 4/7] perf cs-etm: Don't use hard coded config bits when setting up ETMCR

[Leo Yan]

On Mon, Dec 01, 2025 at 04:41:07PM +0000, Coresight ML wrote:

[...]

> @@ -746,7 +779,7 @@ static void cs_etm_get_metadata(struct perf_cpu cpu, u32 *offset,
>  	case CS_ETMV3:
>  		magic = __perf_cs_etmv3_magic;
>  		/* Get configuration register */
> -		info->priv[*offset + CS_ETM_ETMCR] = cs_etm_get_config(itr);
> +		info->priv[*offset + CS_ETM_ETMCR] = cs_etm_guess_etmcr(itr);

I still think cs_etm_get_config() is better than cs_etm_guess_etmcr().

For ETMv3, we directly pass CONFIG to the kernel, and after validation
in the dirver, then the value will be set to ETMCR.  If we already know
the config value is consistent between user space and kernel, why
introduce a redundant "guess" operation here?

Thanks,
Leo

Re: [PATCH 4/7] perf cs-etm: Don't use hard coded config bits when setting up ETMCR

[James Clark]

On 02/12/2025 11:43 am, Leo Yan wrote:
> On Mon, Dec 01, 2025 at 04:41:07PM +0000, Coresight ML wrote:
> 
> [...]
> 
>> @@ -746,7 +779,7 @@ static void cs_etm_get_metadata(struct perf_cpu cpu, u32 *offset,
>>   	case CS_ETMV3:
>>   		magic = __perf_cs_etmv3_magic;
>>   		/* Get configuration register */
>> -		info->priv[*offset + CS_ETM_ETMCR] = cs_etm_get_config(itr);
>> +		info->priv[*offset + CS_ETM_ETMCR] = cs_etm_guess_etmcr(itr);
> 
> I still think cs_etm_get_config() is better than cs_etm_guess_etmcr().
> 
> For ETMv3, we directly pass CONFIG to the kernel, and after validation
> in the dirver, then the value will be set to ETMCR.  If we already know
> the config value is consistent between user space and kernel, why
> introduce a redundant "guess" operation here?
> 
> Thanks,
> Leo

Because userspace doesn't always come up with the same value as the 
driver. For example right now in ETM3, ETMCR_RETURN_STACK isn't set 
depending on certain conditions that userspace doesn't know about.  ETM4 
has the same for TRCCONFIGR_RS and maybe some others. In the future, 
other versions of the driver could do different things as long as we 
don't break decoding.

I didn't want the function name to imply it was doing something it 
wasn't as that confused me a little bit. It's definitely not "getting" 
the value. Maybe "guess" isn't the best it could be, but it's not far off.

Re: [PATCH 4/7] perf cs-etm: Don't use hard coded config bits when setting up ETMCR

[James Clark]

On 02/12/2025 11:53 am, James Clark wrote:
> 
> 
> On 02/12/2025 11:43 am, Leo Yan wrote:
>> On Mon, Dec 01, 2025 at 04:41:07PM +0000, Coresight ML wrote:
>>
>> [...]
>>
>>> @@ -746,7 +779,7 @@ static void cs_etm_get_metadata(struct perf_cpu 
>>> cpu, u32 *offset,
>>>       case CS_ETMV3:
>>>           magic = __perf_cs_etmv3_magic;
>>>           /* Get configuration register */
>>> -        info->priv[*offset + CS_ETM_ETMCR] = cs_etm_get_config(itr);
>>> +        info->priv[*offset + CS_ETM_ETMCR] = cs_etm_guess_etmcr(itr);
>>
>> I still think cs_etm_get_config() is better than cs_etm_guess_etmcr().
>>
>> For ETMv3, we directly pass CONFIG to the kernel, and after validation
>> in the dirver, then the value will be set to ETMCR.  If we already know
>> the config value is consistent between user space and kernel, why

One other note is that since moving the timestamp field, this is no 
longer true either. The value in attr.config isn't directly put into ETMCR.

>> introduce a redundant "guess" operation here?
>>
>> Thanks,
>> Leo
> 
> Because userspace doesn't always come up with the same value as the 
> driver. For example right now in ETM3, ETMCR_RETURN_STACK isn't set 
> depending on certain conditions that userspace doesn't know about.  ETM4 
> has the same for TRCCONFIGR_RS and maybe some others. In the future, 
> other versions of the driver could do different things as long as we 
> don't break decoding.
> 
> I didn't want the function name to imply it was doing something it 
> wasn't as that confused me a little bit. It's definitely not "getting" 
> the value. Maybe "guess" isn't the best it could be, but it's not far off.
> 

Re: [PATCH 4/7] perf cs-etm: Don't use hard coded config bits when setting up ETMCR

[Mike Leach]

Hi,

On Thu, 4 Dec 2025 at 10:55, James Clark <james.clark@linaro.org> wrote:
>
>
>
> On 02/12/2025 11:53 am, James Clark wrote:
> >
> >
> > On 02/12/2025 11:43 am, Leo Yan wrote:
> >> On Mon, Dec 01, 2025 at 04:41:07PM +0000, Coresight ML wrote:
> >>
> >> [...]
> >>
> >>> @@ -746,7 +779,7 @@ static void cs_etm_get_metadata(struct perf_cpu
> >>> cpu, u32 *offset,
> >>>       case CS_ETMV3:
> >>>           magic = __perf_cs_etmv3_magic;
> >>>           /* Get configuration register */
> >>> -        info->priv[*offset + CS_ETM_ETMCR] = cs_etm_get_config(itr);
> >>> +        info->priv[*offset + CS_ETM_ETMCR] = cs_etm_guess_etmcr(itr);
> >>
> >> I still think cs_etm_get_config() is better than cs_etm_guess_etmcr().
> >>
> >> For ETMv3, we directly pass CONFIG to the kernel, and after validation
> >> in the dirver, then the value will be set to ETMCR.  If we already know
> >> the config value is consistent between user space and kernel, why
>
> One other note is that since moving the timestamp field, this is no
> longer true either. The value in attr.config isn't directly put into ETMCR.
>
> >> introduce a redundant "guess" operation here?
> >>
> >> Thanks,
> >> Leo
> >
> > Because userspace doesn't always come up with the same value as the
> > driver. For example right now in ETM3, ETMCR_RETURN_STACK isn't set
> > depending on certain conditions that userspace doesn't know about.  ETM4
> > has the same for TRCCONFIGR_RS and maybe some others. In the future,
> > other versions of the driver could do different things as long as we
> > don't break decoding.
> >
> > I didn't want the function name to imply it was doing something it
> > wasn't as that confused me a little bit. It's definitely not "getting"
> > the value. Maybe "guess" isn't the best it could be, but it's not far off.
> >
>

Perhaps cs_etm_synth_etmcr()? We cannot read it directly as it has not
been set at the time of creating these headers. (unlike the sets of
static read only IDR regs that we do read).

When in perf mode the only configuration bits set in the ConfigR for
either ETM3 or 4 are those generated or implied by parameters on the
perf command line.
This info has to pass from perf to the driver somehow. Evidently many
years ago, when only ETMv3/PTM existed the easy way was perf.config ==
etm.configr, now that is no longer feasible.
As long as perf and the drivers interpret the command line attributes
in the same way - all is well.

As James says, the actual configr can differ from the synth one - the
key is the bits that control the trace format - e.g. cyclecounts,
rather than trace filtering e.g. userspace/kernel that affects the
drivers configr but not the synthesized value in perf.
Decode cares about format, not about filtering. Additionally some
things - like return-stack are implementation dependent - optional on
PTM, not at all on ETMv3. If the trace unit does not support it then
the drivers ignore this. the only effect on the trace output is less
compression if retstack cannot be used.

Generally decode needs to know about things that affect format and
function, rather than filtering.

Mike
-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Re: [PATCH 4/7] perf cs-etm: Don't use hard coded config bits when setting up ETMCR

[James Clark]

On 04/12/2025 1:45 pm, Mike Leach wrote:
> Hi,
> 
> On Thu, 4 Dec 2025 at 10:55, James Clark <james.clark@linaro.org> wrote:
>>
>>
>>
>> On 02/12/2025 11:53 am, James Clark wrote:
>>>
>>>
>>> On 02/12/2025 11:43 am, Leo Yan wrote:
>>>> On Mon, Dec 01, 2025 at 04:41:07PM +0000, Coresight ML wrote:
>>>>
>>>> [...]
>>>>
>>>>> @@ -746,7 +779,7 @@ static void cs_etm_get_metadata(struct perf_cpu
>>>>> cpu, u32 *offset,
>>>>>        case CS_ETMV3:
>>>>>            magic = __perf_cs_etmv3_magic;
>>>>>            /* Get configuration register */
>>>>> -        info->priv[*offset + CS_ETM_ETMCR] = cs_etm_get_config(itr);
>>>>> +        info->priv[*offset + CS_ETM_ETMCR] = cs_etm_guess_etmcr(itr);
>>>>
>>>> I still think cs_etm_get_config() is better than cs_etm_guess_etmcr().
>>>>
>>>> For ETMv3, we directly pass CONFIG to the kernel, and after validation
>>>> in the dirver, then the value will be set to ETMCR.  If we already know
>>>> the config value is consistent between user space and kernel, why
>>
>> One other note is that since moving the timestamp field, this is no
>> longer true either. The value in attr.config isn't directly put into ETMCR.
>>
>>>> introduce a redundant "guess" operation here?
>>>>
>>>> Thanks,
>>>> Leo
>>>
>>> Because userspace doesn't always come up with the same value as the
>>> driver. For example right now in ETM3, ETMCR_RETURN_STACK isn't set
>>> depending on certain conditions that userspace doesn't know about.  ETM4
>>> has the same for TRCCONFIGR_RS and maybe some others. In the future,
>>> other versions of the driver could do different things as long as we
>>> don't break decoding.
>>>
>>> I didn't want the function name to imply it was doing something it
>>> wasn't as that confused me a little bit. It's definitely not "getting"
>>> the value. Maybe "guess" isn't the best it could be, but it's not far off.
>>>
>>
> 
> Perhaps cs_etm_synth_etmcr()? We cannot read it directly as it has not

synth is a good name, I can use that.

> been set at the time of creating these headers. (unlike the sets of
> static read only IDR regs that we do read).
> 
> When in perf mode the only configuration bits set in the ConfigR for
> either ETM3 or 4 are those generated or implied by parameters on the
> perf command line.
> This info has to pass from perf to the driver somehow. Evidently many
> years ago, when only ETMv3/PTM existed the easy way was perf.config ==
> etm.configr, now that is no longer feasible.
> As long as perf and the drivers interpret the command line attributes
> in the same way - all is well.
> 
> As James says, the actual configr can differ from the synth one - the
> key is the bits that control the trace format - e.g. cyclecounts,
> rather than trace filtering e.g. userspace/kernel that affects the
> drivers configr but not the synthesized value in perf.
> Decode cares about format, not about filtering. Additionally some
> things - like return-stack are implementation dependent - optional on
> PTM, not at all on ETMv3. If the trace unit does not support it then
> the drivers ignore this. the only effect on the trace output is less
> compression if retstack cannot be used.
> 
> Generally decode needs to know about things that affect format and
> function, rather than filtering.
> 
> Mike

[PATCH 5/7] perf cs-etm: Don't use hard coded config bits when setting up TRCCONFIGR

[James Clark]

Perf only looks at attr.config when determining what was programmed into
TRCCONFIGR. These bits could theoretically be in any of the config
fields. Use the evsel__get_config_val() helper so it's agnostic to
which config field they are in.

The kernel will also stop publishing the TRCCONFIGR register bits in a
header [1] so preempt that by defining them here.

[1]: https://lore.kernel.org/linux-arm-kernel/20251128-james-cs-syncfreq-v8-10-4d319764cc58@linaro.org/
Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/perf/arch/arm/util/cs-etm.c | 79 +++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 45 deletions(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index 414cafb21c98..51ca8fa66ef8 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -68,6 +68,14 @@ static const char * const metadata_ete_ro[] = {
 
 enum cs_etm_version { CS_NOT_PRESENT, CS_ETMV3, CS_ETMV4, CS_ETE };
 
+/* ETMv4 CONFIGR register bits */
+#define TRCCONFIGR_BB		BIT(3)
+#define TRCCONFIGR_CCI		BIT(4)
+#define TRCCONFIGR_CID		BIT(6)
+#define TRCCONFIGR_VMID		BIT(7)
+#define TRCCONFIGR_TS		BIT(11)
+#define TRCCONFIGR_RS		BIT(12)
+#define TRCCONFIGR_VMIDOPT	BIT(15)
 
 /* ETMv3 ETMCR register bits */
 #define ETMCR_CYC_ACC		BIT(12)
@@ -520,56 +528,37 @@ static u64 cs_etm_guess_etmcr(struct auxtrace_record *itr)
 	return etmcr;
 }
 
-static u64 cs_etm_get_config(struct auxtrace_record *itr)
+static u64 cs_etmv4_guess_trcconfigr(struct auxtrace_record *itr)
 {
+	u64 trcconfigr = 0;
 	struct cs_etm_recording *ptr =
-			container_of(itr, struct cs_etm_recording, itr);
+		container_of(itr, struct cs_etm_recording, itr);
 	struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
-	struct evlist *evlist = ptr->evlist;
-	struct evsel *evsel = cs_etm_get_evsel(evlist, cs_etm_pmu);
-
-	/*
-	 * Variable perf_event_attr::config is assigned to
-	 * ETMv3/PTM.  The bit fields have been made to match
-	 * the ETMv3.5 ETRMCR register specification.  See the
-	 * PMU_FORMAT_ATTR() declarations in
-	 * drivers/hwtracing/coresight/coresight-perf.c for
-	 * details.
-	 */
-	return evsel ? evsel->core.attr.config : 0;
-}
-
-#ifndef BIT
-#define BIT(N) (1UL << (N))
-#endif
+	struct evsel *evsel = cs_etm_get_evsel(ptr->evlist, cs_etm_pmu);
+	u64 val;
 
-static u64 cs_etmv4_get_config(struct auxtrace_record *itr)
-{
-	u64 config = 0;
-	u64 config_opts = 0;
+	if (!evsel)
+		return 0;
 
 	/*
-	 * The perf event variable config bits represent both
-	 * the command line options and register programming
-	 * bits in ETMv3/PTM. For ETMv4 we must remap options
-	 * to real bits
+	 * Roughly guess what the kernel programmed into TRCCONFIGR based on
+	 * what options the event was opened with. This doesn't have to be
+	 * complete or 100% accurate, not all bits used by OpenCSD anyway.
 	 */
-	config_opts = cs_etm_get_config(itr);
-	if (config_opts & BIT(ETM_OPT_CYCACC))
-		config |= BIT(ETM4_CFG_BIT_CYCACC);
-	if (config_opts & BIT(ETM_OPT_CTXTID))
-		config |= BIT(ETM4_CFG_BIT_CTXTID);
-	if (config_opts & BIT(ETM_OPT_TS))
-		config |= BIT(ETM4_CFG_BIT_TS);
-	if (config_opts & BIT(ETM_OPT_RETSTK))
-		config |= BIT(ETM4_CFG_BIT_RETSTK);
-	if (config_opts & BIT(ETM_OPT_CTXTID2))
-		config |= BIT(ETM4_CFG_BIT_VMID) |
-			  BIT(ETM4_CFG_BIT_VMID_OPT);
-	if (config_opts & BIT(ETM_OPT_BRANCH_BROADCAST))
-		config |= BIT(ETM4_CFG_BIT_BB);
-
-	return config;
+	if (!evsel__get_config_val(cs_etm_pmu, evsel, "cycacc", &val) && val)
+		trcconfigr |= TRCCONFIGR_CCI;
+	if (!evsel__get_config_val(cs_etm_pmu, evsel, "contextid1", &val) && val)
+		trcconfigr |= TRCCONFIGR_CID;
+	if (!evsel__get_config_val(cs_etm_pmu, evsel, "timestamp", &val) && val)
+		trcconfigr |= TRCCONFIGR_TS;
+	if (!evsel__get_config_val(cs_etm_pmu, evsel, "retstack", &val) && val)
+		trcconfigr |= TRCCONFIGR_RS;
+	if (!evsel__get_config_val(cs_etm_pmu, evsel, "contextid2", &val) && val)
+		trcconfigr |= TRCCONFIGR_VMID | TRCCONFIGR_VMIDOPT;
+	if (!evsel__get_config_val(cs_etm_pmu, evsel, "branch_broadcast", &val) && val)
+		trcconfigr |= TRCCONFIGR_BB;
+
+	return trcconfigr;
 }
 
 static size_t
@@ -691,7 +680,7 @@ static void cs_etm_save_etmv4_header(__u64 data[], struct auxtrace_record *itr,
 	struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
 
 	/* Get trace configuration register */
-	data[CS_ETMV4_TRCCONFIGR] = cs_etmv4_get_config(itr);
+	data[CS_ETMV4_TRCCONFIGR] = cs_etmv4_guess_trcconfigr(itr);
 	/* traceID set to legacy version, in case new perf running on older system */
 	data[CS_ETMV4_TRCTRACEIDR] = cs_etm_get_legacy_trace_id(cpu);
 
@@ -723,7 +712,7 @@ static void cs_etm_save_ete_header(__u64 data[], struct auxtrace_record *itr, st
 	struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu;
 
 	/* Get trace configuration register */
-	data[CS_ETE_TRCCONFIGR] = cs_etmv4_get_config(itr);
+	data[CS_ETE_TRCCONFIGR] = cs_etmv4_guess_trcconfigr(itr);
 	/* traceID set to legacy version, in case new perf running on older system */
 	data[CS_ETE_TRCTRACEIDR] = cs_etm_get_legacy_trace_id(cpu);
 

-- 
2.34.1

Re: [PATCH 5/7] perf cs-etm: Don't use hard coded config bits when setting up TRCCONFIGR

[Leo Yan]

On Mon, Dec 01, 2025 at 04:41:08PM +0000, Coresight ML wrote:
> Perf only looks at attr.config when determining what was programmed into
> TRCCONFIGR. These bits could theoretically be in any of the config
> fields. Use the evsel__get_config_val() helper so it's agnostic to
> which config field they are in.
> 
> The kernel will also stop publishing the TRCCONFIGR register bits in a
> header [1] so preempt that by defining them here.
> 
> [1]: https://lore.kernel.org/linux-arm-kernel/20251128-james-cs-syncfreq-v8-10-4d319764cc58@linaro.org/
> Signed-off-by: James Clark <james.clark@linaro.org>

Reviewed-by: Leo Yan <leo.yan@arm.com>

[PATCH 6/7] perf cs-etm: Don't hard code config attribute when configuring the event

[James Clark]

These instances of hard coded config attributes are used for configuring
and validating the event options. Use the config attribute that's
published by the driver by replacing the open coded operations with
evsel__get_config_val() and evsel__set_config_if_unset().

Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/perf/arch/arm/util/cs-etm.c | 66 ++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 29 deletions(-)

diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index 51ca8fa66ef8..52e304584095 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -103,13 +103,20 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
 				      struct perf_cpu cpu)
 {
 	int err;
-	__u64 val;
-	u64 contextid = evsel->core.attr.config &
-		(perf_pmu__format_bits(cs_etm_pmu, "contextid") |
-		 perf_pmu__format_bits(cs_etm_pmu, "contextid1") |
-		 perf_pmu__format_bits(cs_etm_pmu, "contextid2"));
+	u64 ctxt, ctxt1, ctxt2;
+	__u64 trcidr2;
 
-	if (!contextid)
+	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid", &ctxt);
+	if (err)
+		return err;
+	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid1", &ctxt1);
+	if (err)
+		return err;
+	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid2", &ctxt2);
+	if (err)
+		return err;
+
+	if (!ctxt && !ctxt1 && !ctxt2)
 		return 0;
 
 	/* Not supported in etmv3 */
@@ -120,12 +127,11 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
 	}
 
 	/* Get a handle on TRCIDR2 */
-	err = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR2], &val);
+	err = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR2], &trcidr2);
 	if (err)
 		return err;
 
-	if (contextid &
-	    perf_pmu__format_bits(cs_etm_pmu, "contextid1")) {
+	if (ctxt1) {
 		/*
 		 * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID
 		 * tracing is supported:
@@ -133,15 +139,14 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
 		 *  0b00100 Maximum of 32-bit Context ID size.
 		 *  All other values are reserved.
 		 */
-		if (BMVAL(val, 5, 9) != 0x4) {
+		if (BMVAL(trcidr2, 5, 9) != 0x4) {
 			pr_err("%s: CONTEXTIDR_EL1 isn't supported, disable with %s/contextid1=0/\n",
 			       CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME);
 			return -EINVAL;
 		}
 	}
 
-	if (contextid &
-	    perf_pmu__format_bits(cs_etm_pmu, "contextid2")) {
+	if (ctxt2) {
 		/*
 		 * TRCIDR2.VMIDOPT[30:29] != 0 and
 		 * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual contextid)
@@ -149,7 +154,7 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
 		 * virtual context id is < 32bit.
 		 * Any value of VMIDSIZE >= 4 (i.e, > 32bit) is fine for us.
 		 */
-		if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) {
+		if (!BMVAL(trcidr2, 29, 30) || BMVAL(trcidr2, 10, 14) < 4) {
 			pr_err("%s: CONTEXTIDR_EL2 isn't supported, disable with %s/contextid2=0/\n",
 			       CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME);
 			return -EINVAL;
@@ -163,10 +168,14 @@ static int cs_etm_validate_timestamp(struct perf_pmu *cs_etm_pmu, struct evsel *
 				     struct perf_cpu cpu)
 {
 	int err;
-	__u64 val;
+	u64 val;
+	__u64 trcidr0;
 
-	if (!(evsel->core.attr.config &
-	      perf_pmu__format_bits(cs_etm_pmu, "timestamp")))
+	err = evsel__get_config_val(cs_etm_pmu, evsel, "timestamp", &val);
+	if (err)
+		return err;
+
+	if (!val)
 		return 0;
 
 	if (cs_etm_get_version(cs_etm_pmu, cpu) == CS_ETMV3) {
@@ -176,7 +185,7 @@ static int cs_etm_validate_timestamp(struct perf_pmu *cs_etm_pmu, struct evsel *
 	}
 
 	/* Get a handle on TRCIRD0 */
-	err = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR0], &val);
+	err = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR0], &trcidr0);
 	if (err)
 		return err;
 
@@ -187,10 +196,9 @@ static int cs_etm_validate_timestamp(struct perf_pmu *cs_etm_pmu, struct evsel *
 	 *  0b00110 Implementation supports a maximum timestamp of 48bits.
 	 *  0b01000 Implementation supports a maximum timestamp of 64bits.
 	 */
-	val &= GENMASK(28, 24);
-	if (!val) {
+	trcidr0 &= GENMASK(28, 24);
+	if (!trcidr0)
 		return -EINVAL;
-	}
 
 	return 0;
 }
@@ -273,16 +281,20 @@ static int cs_etm_parse_snapshot_options(struct auxtrace_record *itr,
 	return 0;
 }
 
+/*
+ * The sink name format "@sink_name" is used, lookup the sink by name to convert
+ * to "sinkid=sink_hash" format.
+ *
+ * If the user has already manually provided a hash then "sinkid" isn't
+ * overwritten. If neither are provided then the driver will pick the best sink.
+ */
 static int cs_etm_set_sink_attr(struct perf_pmu *pmu,
 				struct evsel *evsel)
 {
 	char msg[BUFSIZ], path[PATH_MAX], *sink;
 	struct evsel_config_term *term;
-	int ret = -EINVAL;
 	u32 hash;
-
-	if (evsel->core.attr.config2 & GENMASK(31, 0))
-		return 0;
+	int ret;
 
 	list_for_each_entry(term, &evsel->config_terms, list) {
 		if (term->type != EVSEL__CONFIG_TERM_DRV_CFG)
@@ -305,14 +317,10 @@ static int cs_etm_set_sink_attr(struct perf_pmu *pmu,
 			return ret;
 		}
 
-		evsel->core.attr.config2 |= hash;
+		evsel__set_config_if_unset(pmu, evsel, "sinkid", hash);
 		return 0;
 	}
 
-	/*
-	 * No sink was provided on the command line - allow the CoreSight
-	 * system to look for a default
-	 */
 	return 0;
 }
 

-- 
2.34.1

Re: [PATCH 6/7] perf cs-etm: Don't hard code config attribute when configuring the event

[Leo Yan]

On Mon, Dec 01, 2025 at 04:41:09PM +0000, Coresight ML wrote:

[...]

> @@ -103,13 +103,20 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
>  				      struct perf_cpu cpu)
>  {
>  	int err;
> -	__u64 val;
> -	u64 contextid = evsel->core.attr.config &
> -		(perf_pmu__format_bits(cs_etm_pmu, "contextid") |
> -		 perf_pmu__format_bits(cs_etm_pmu, "contextid1") |
> -		 perf_pmu__format_bits(cs_etm_pmu, "contextid2"));
> +	u64 ctxt, ctxt1, ctxt2;
> +	__u64 trcidr2;
>  
> -	if (!contextid)
> +	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid", &ctxt);
> +	if (err)
> +		return err;
> +	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid1", &ctxt1);
> +	if (err)
> +		return err;
> +	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid2", &ctxt2);
> +	if (err)
> +		return err;

Seems to me, this is not right.  The current code checks any context
ID setting but it can tolerate if missing "contexid[*]" format.

After calling evsel__get_config_val(), if any "contextid[*]" format is
missed, it returns error and will diretly bail out.  As a result,
cs_etm_validate_context_id() will always return error.

> +
> +	if (!ctxt && !ctxt1 && !ctxt2)
>  		return 0;
>  
>  	/* Not supported in etmv3 */
> @@ -120,12 +127,11 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
>  	}
>  
>  	/* Get a handle on TRCIDR2 */
> -	err = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR2], &val);
> +	err = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR2], &trcidr2);
>  	if (err)
>  		return err;
>  
> -	if (contextid &
> -	    perf_pmu__format_bits(cs_etm_pmu, "contextid1")) {
> +	if (ctxt1) {
>  		/*
>  		 * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID
>  		 * tracing is supported:
> @@ -133,15 +139,14 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
>  		 *  0b00100 Maximum of 32-bit Context ID size.
>  		 *  All other values are reserved.
>  		 */
> -		if (BMVAL(val, 5, 9) != 0x4) {
> +		if (BMVAL(trcidr2, 5, 9) != 0x4) {
>  			pr_err("%s: CONTEXTIDR_EL1 isn't supported, disable with %s/contextid1=0/\n",
>  			       CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME);
>  			return -EINVAL;
>  		}
>  	}
>  
> -	if (contextid &
> -	    perf_pmu__format_bits(cs_etm_pmu, "contextid2")) {
> +	if (ctxt2) {
>  		/*
>  		 * TRCIDR2.VMIDOPT[30:29] != 0 and
>  		 * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual contextid)
> @@ -149,7 +154,7 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
>  		 * virtual context id is < 32bit.
>  		 * Any value of VMIDSIZE >= 4 (i.e, > 32bit) is fine for us.
>  		 */
> -		if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) {
> +		if (!BMVAL(trcidr2, 29, 30) || BMVAL(trcidr2, 10, 14) < 4) {
>  			pr_err("%s: CONTEXTIDR_EL2 isn't supported, disable with %s/contextid2=0/\n",
>  			       CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME);
>  			return -EINVAL;
> @@ -163,10 +168,14 @@ static int cs_etm_validate_timestamp(struct perf_pmu *cs_etm_pmu, struct evsel *
>  				     struct perf_cpu cpu)
>  {
>  	int err;
> -	__u64 val;
> +	u64 val;
> +	__u64 trcidr0;
>  
> -	if (!(evsel->core.attr.config &
> -	      perf_pmu__format_bits(cs_etm_pmu, "timestamp")))
> +	err = evsel__get_config_val(cs_etm_pmu, evsel, "timestamp", &val);
> +	if (err)
> +		return err;
> +
> +	if (!val)
>  		return 0;

Similiar issue here.  The current code returns 0 if not find "timestamp"
format.  With this change, it returns error instead.

Thanks,
Leo

Re: [PATCH 6/7] perf cs-etm: Don't hard code config attribute when configuring the event

[James Clark]

On 02/12/2025 12:15 pm, Leo Yan wrote:
> On Mon, Dec 01, 2025 at 04:41:09PM +0000, Coresight ML wrote:
> 
> [...]
> 
>> @@ -103,13 +103,20 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
>>   				      struct perf_cpu cpu)
>>   {
>>   	int err;
>> -	__u64 val;
>> -	u64 contextid = evsel->core.attr.config &
>> -		(perf_pmu__format_bits(cs_etm_pmu, "contextid") |
>> -		 perf_pmu__format_bits(cs_etm_pmu, "contextid1") |
>> -		 perf_pmu__format_bits(cs_etm_pmu, "contextid2"));
>> +	u64 ctxt, ctxt1, ctxt2;
>> +	__u64 trcidr2;
>>   
>> -	if (!contextid)
>> +	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid", &ctxt);
>> +	if (err)
>> +		return err;
>> +	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid1", &ctxt1);
>> +	if (err)
>> +		return err;
>> +	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid2", &ctxt2);
>> +	if (err)
>> +		return err;
> 
> Seems to me, this is not right.  The current code checks any context
> ID setting but it can tolerate if missing "contexid[*]" format.
> 
> After calling evsel__get_config_val(), if any "contextid[*]" format is
> missed, it returns error and will diretly bail out.  As a result,
> cs_etm_validate_context_id() will always return error.
> 

Missed by the driver or the user? evsel__get_config_val() only returns 
an error when "contextid" isn't published by the driver, not when the 
user doesn't supply one of those options. The actual user supplied value 
is in the out param, not the return value.

Having said that, this does make it an error if the driver did stop 
publishing one, which might be too inflexible and is a new behavior. I 
can change to it ignore the errors instead.

>> +
>> +	if (!ctxt && !ctxt1 && !ctxt2)
>>   		return 0;
>>   
>>   	/* Not supported in etmv3 */
>> @@ -120,12 +127,11 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
>>   	}
>>   
>>   	/* Get a handle on TRCIDR2 */
>> -	err = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR2], &val);
>> +	err = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR2], &trcidr2);
>>   	if (err)
>>   		return err;
>>   
>> -	if (contextid &
>> -	    perf_pmu__format_bits(cs_etm_pmu, "contextid1")) {
>> +	if (ctxt1) {
>>   		/*
>>   		 * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID
>>   		 * tracing is supported:
>> @@ -133,15 +139,14 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
>>   		 *  0b00100 Maximum of 32-bit Context ID size.
>>   		 *  All other values are reserved.
>>   		 */
>> -		if (BMVAL(val, 5, 9) != 0x4) {
>> +		if (BMVAL(trcidr2, 5, 9) != 0x4) {
>>   			pr_err("%s: CONTEXTIDR_EL1 isn't supported, disable with %s/contextid1=0/\n",
>>   			       CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME);
>>   			return -EINVAL;
>>   		}
>>   	}
>>   
>> -	if (contextid &
>> -	    perf_pmu__format_bits(cs_etm_pmu, "contextid2")) {
>> +	if (ctxt2) {
>>   		/*
>>   		 * TRCIDR2.VMIDOPT[30:29] != 0 and
>>   		 * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual contextid)
>> @@ -149,7 +154,7 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
>>   		 * virtual context id is < 32bit.
>>   		 * Any value of VMIDSIZE >= 4 (i.e, > 32bit) is fine for us.
>>   		 */
>> -		if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) {
>> +		if (!BMVAL(trcidr2, 29, 30) || BMVAL(trcidr2, 10, 14) < 4) {
>>   			pr_err("%s: CONTEXTIDR_EL2 isn't supported, disable with %s/contextid2=0/\n",
>>   			       CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME);
>>   			return -EINVAL;
>> @@ -163,10 +168,14 @@ static int cs_etm_validate_timestamp(struct perf_pmu *cs_etm_pmu, struct evsel *
>>   				     struct perf_cpu cpu)
>>   {
>>   	int err;
>> -	__u64 val;
>> +	u64 val;
>> +	__u64 trcidr0;
>>   
>> -	if (!(evsel->core.attr.config &
>> -	      perf_pmu__format_bits(cs_etm_pmu, "timestamp")))
>> +	err = evsel__get_config_val(cs_etm_pmu, evsel, "timestamp", &val);
>> +	if (err)
>> +		return err;
>> +
>> +	if (!val)
>>   		return 0;
> 
> Similiar issue here.  The current code returns 0 if not find "timestamp"
> format.  With this change, it returns error instead.
> 
> Thanks,
> Leo

Re: [PATCH 6/7] perf cs-etm: Don't hard code config attribute when configuring the event

[Leo Yan]

On Thu, Dec 04, 2025 at 02:08:59PM +0000, James Clark wrote:

[...]

> > > +	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid", &ctxt);
> > > +	if (err)
> > > +		return err;
> > > +	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid1", &ctxt1);
> > > +	if (err)
> > > +		return err;
> > > +	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid2", &ctxt2);
> > > +	if (err)
> > > +		return err;
> > 
> > Seems to me, this is not right.  The current code checks any context
> > ID setting but it can tolerate if missing "contexid[*]" format.
> > 
> > After calling evsel__get_config_val(), if any "contextid[*]" format is
> > missed, it returns error and will diretly bail out.  As a result,
> > cs_etm_validate_context_id() will always return error.
> > 
> 
> Missed by the driver or the user? evsel__get_config_val() only returns an
> error when "contextid" isn't published by the driver, not when the user
> doesn't supply one of those options. The actual user supplied value is in
> the out param, not the return value.

My question is for missing format from user.  Thanks for reminding the
error is only for driver's publishing.

> Having said that, this does make it an error if the driver did stop
> publishing one, which might be too inflexible and is a new behavior. I can
> change to it ignore the errors instead.

Yeah, this will likely break nVHE / pKVM cases.

Thanks,
Leo

[PATCH 7/7] perf arm-spe: Don't hard code config attribute

[James Clark]

Use the config attribute that's published by the driver instead of
hard coding "attr.config".

Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/perf/arch/arm64/util/arm-spe.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
index d5ec1408d0ae..6c3dc97fde30 100644
--- a/tools/perf/arch/arm64/util/arm-spe.c
+++ b/tools/perf/arch/arm64/util/arm-spe.c
@@ -256,7 +256,7 @@ static __u64 arm_spe_pmu__sample_period(const struct perf_pmu *arm_spe_pmu)
 
 static void arm_spe_setup_evsel(struct evsel *evsel, struct perf_cpu_map *cpus)
 {
-	u64 bit;
+	u64 pa_enable_bit;
 
 	evsel->core.attr.freq = 0;
 	evsel->core.attr.sample_period = arm_spe_pmu__sample_period(evsel->pmu);
@@ -288,9 +288,10 @@ static void arm_spe_setup_evsel(struct evsel *evsel, struct perf_cpu_map *cpus)
 	 * inform that the resulting output's SPE samples contain physical addresses
 	 * where applicable.
 	 */
-	bit = perf_pmu__format_bits(evsel->pmu, "pa_enable");
-	if (evsel->core.attr.config & bit)
-		evsel__set_sample_bit(evsel, PHYS_ADDR);
+
+	if (!evsel__get_config_val(evsel->pmu, evsel, "pa_enable", &pa_enable_bit))
+		if (pa_enable_bit)
+			evsel__set_sample_bit(evsel, PHYS_ADDR);
 }
 
 static int arm_spe_setup_aux_buffer(struct record_opts *opts)
@@ -397,6 +398,7 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 	struct perf_cpu_map *cpus = evlist->core.user_requested_cpus;
 	bool discard = false;
 	int err;
+	u64 discard_bit;
 
 	sper->evlist = evlist;
 
@@ -425,9 +427,8 @@ static int arm_spe_recording_options(struct auxtrace_record *itr,
 	evlist__for_each_entry_safe(evlist, tmp, evsel) {
 		if (evsel__is_aux_event(evsel)) {
 			arm_spe_setup_evsel(evsel, cpus);
-			if (evsel->core.attr.config &
-			    perf_pmu__format_bits(evsel->pmu, "discard"))
-				discard = true;
+			if (!evsel__get_config_val(evsel->pmu, evsel, "discard", &discard_bit))
+				discard = !!discard_bit;
 		}
 	}
 

-- 
2.34.1

Re: [PATCH 7/7] perf arm-spe: Don't hard code config attribute

[Leo Yan]

On Mon, Dec 01, 2025 at 04:41:10PM +0000, Coresight ML wrote:
> Use the config attribute that's published by the driver instead of
> hard coding "attr.config".
> 
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  tools/perf/arch/arm64/util/arm-spe.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
> index d5ec1408d0ae..6c3dc97fde30 100644
> --- a/tools/perf/arch/arm64/util/arm-spe.c
> +++ b/tools/perf/arch/arm64/util/arm-spe.c
> @@ -256,7 +256,7 @@ static __u64 arm_spe_pmu__sample_period(const struct perf_pmu *arm_spe_pmu)
>  
>  static void arm_spe_setup_evsel(struct evsel *evsel, struct perf_cpu_map *cpus)
>  {
> -	u64 bit;
> +	u64 pa_enable_bit;
>  
>  	evsel->core.attr.freq = 0;
>  	evsel->core.attr.sample_period = arm_spe_pmu__sample_period(evsel->pmu);
> @@ -288,9 +288,10 @@ static void arm_spe_setup_evsel(struct evsel *evsel, struct perf_cpu_map *cpus)
>  	 * inform that the resulting output's SPE samples contain physical addresses
>  	 * where applicable.
>  	 */
> -	bit = perf_pmu__format_bits(evsel->pmu, "pa_enable");
> -	if (evsel->core.attr.config & bit)
> -		evsel__set_sample_bit(evsel, PHYS_ADDR);
> +
> +	if (!evsel__get_config_val(evsel->pmu, evsel, "pa_enable", &pa_enable_bit))
> +		if (pa_enable_bit)
> +			evsel__set_sample_bit(evsel, PHYS_ADDR);

Hmm... I am a bit concerned for the evsel__get_config_val() usage
throughout the series.

evsel__get_config_val() returns a whole config value rather than the
bit field specified by the format name.  If other bits (but not the
"pa_enable" bit) are set in the same config set, would it wrongly set
the PHYS_ADDR sample bit?

Seems to me, for reading specific format, perf_pmu__format_bits() is
more suitable than evsel__get_config_val().

Thanks,
Leo

Re: [PATCH 7/7] perf arm-spe: Don't hard code config attribute

[Leo Yan]

On Tue, Dec 02, 2025 at 12:28:35PM +0000, Coresight ML wrote:
> On Mon, Dec 01, 2025 at 04:41:10PM +0000, Coresight ML wrote:
> > Use the config attribute that's published by the driver instead of
> > hard coding "attr.config".
> > 
> > Signed-off-by: James Clark <james.clark@linaro.org>
> > ---
> >  tools/perf/arch/arm64/util/arm-spe.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
> > index d5ec1408d0ae..6c3dc97fde30 100644
> > --- a/tools/perf/arch/arm64/util/arm-spe.c
> > +++ b/tools/perf/arch/arm64/util/arm-spe.c
> > @@ -256,7 +256,7 @@ static __u64 arm_spe_pmu__sample_period(const struct perf_pmu *arm_spe_pmu)
> >  
> >  static void arm_spe_setup_evsel(struct evsel *evsel, struct perf_cpu_map *cpus)
> >  {
> > -	u64 bit;
> > +	u64 pa_enable_bit;
> >  
> >  	evsel->core.attr.freq = 0;
> >  	evsel->core.attr.sample_period = arm_spe_pmu__sample_period(evsel->pmu);
> > @@ -288,9 +288,10 @@ static void arm_spe_setup_evsel(struct evsel *evsel, struct perf_cpu_map *cpus)
> >  	 * inform that the resulting output's SPE samples contain physical addresses
> >  	 * where applicable.
> >  	 */
> > -	bit = perf_pmu__format_bits(evsel->pmu, "pa_enable");
> > -	if (evsel->core.attr.config & bit)
> > -		evsel__set_sample_bit(evsel, PHYS_ADDR);
> > +
> > +	if (!evsel__get_config_val(evsel->pmu, evsel, "pa_enable", &pa_enable_bit))
> > +		if (pa_enable_bit)
> > +			evsel__set_sample_bit(evsel, PHYS_ADDR);
> 
> Hmm... I am a bit concerned for the evsel__get_config_val() usage
> throughout the series.
> 
> evsel__get_config_val() returns a whole config value rather than the
> bit field specified by the format name.  If other bits (but not the
> "pa_enable" bit) are set in the same config set, would it wrongly set
> the PHYS_ADDR sample bit?

I saw you used FIELD_GET() to read specific bit field in
evsel__get_config_val().  This is not concerned anymore.

Sorry for noise.

> Seems to me, for reading specific format, perf_pmu__format_bits() is
> more suitable than evsel__get_config_val().

For me, this comment is still valid, given perf_pmu__format_bits()
will always exist.

Thanks,
Leo

Re: [PATCH 7/7] perf arm-spe: Don't hard code config attribute

[James Clark]

On 02/12/2025 12:42 pm, Leo Yan wrote:
> On Tue, Dec 02, 2025 at 12:28:35PM +0000, Coresight ML wrote:
>> On Mon, Dec 01, 2025 at 04:41:10PM +0000, Coresight ML wrote:
>>> Use the config attribute that's published by the driver instead of
>>> hard coding "attr.config".
>>>
>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>> ---
>>>   tools/perf/arch/arm64/util/arm-spe.c | 15 ++++++++-------
>>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
>>> index d5ec1408d0ae..6c3dc97fde30 100644
>>> --- a/tools/perf/arch/arm64/util/arm-spe.c
>>> +++ b/tools/perf/arch/arm64/util/arm-spe.c
>>> @@ -256,7 +256,7 @@ static __u64 arm_spe_pmu__sample_period(const struct perf_pmu *arm_spe_pmu)
>>>   
>>>   static void arm_spe_setup_evsel(struct evsel *evsel, struct perf_cpu_map *cpus)
>>>   {
>>> -	u64 bit;
>>> +	u64 pa_enable_bit;
>>>   
>>>   	evsel->core.attr.freq = 0;
>>>   	evsel->core.attr.sample_period = arm_spe_pmu__sample_period(evsel->pmu);
>>> @@ -288,9 +288,10 @@ static void arm_spe_setup_evsel(struct evsel *evsel, struct perf_cpu_map *cpus)
>>>   	 * inform that the resulting output's SPE samples contain physical addresses
>>>   	 * where applicable.
>>>   	 */
>>> -	bit = perf_pmu__format_bits(evsel->pmu, "pa_enable");
>>> -	if (evsel->core.attr.config & bit)
>>> -		evsel__set_sample_bit(evsel, PHYS_ADDR);
>>> +
>>> +	if (!evsel__get_config_val(evsel->pmu, evsel, "pa_enable", &pa_enable_bit))
>>> +		if (pa_enable_bit)
>>> +			evsel__set_sample_bit(evsel, PHYS_ADDR);
>>
>> Hmm... I am a bit concerned for the evsel__get_config_val() usage
>> throughout the series.
>>
>> evsel__get_config_val() returns a whole config value rather than the
>> bit field specified by the format name.  If other bits (but not the
>> "pa_enable" bit) are set in the same config set, would it wrongly set
>> the PHYS_ADDR sample bit?
> 
> I saw you used FIELD_GET() to read specific bit field in
> evsel__get_config_val().  This is not concerned anymore.
> 
> Sorry for noise.
> 
>> Seems to me, for reading specific format, perf_pmu__format_bits() is
>> more suitable than evsel__get_config_val().
> 
> For me, this comment is still valid, given perf_pmu__format_bits()
> will always exist.
> 

perf_pmu__format_bits() only returns the mask for the format attribute 
that's published by the kernel. It never changes depending on what 
options the user provides and doesn't even say which config field that 
mask applies to. It's fairly useless on its own and should probably be 
made private to pmu.c if we manage to refactor its uses out of 
intel-pt.c. Although they're a bit more involved there than here so I 
won't touch it now.

evsel__get_config_val() returns the value that the user provided for a 
named attribute which is what we need to do here.

> Thanks,
> Leo