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,
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;
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)
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; }
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-4d... 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.
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-4d... 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);
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; }
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; } }
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
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.
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);
}
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
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
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
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.
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.
Signed-off-by: James Clark james.clark@linaro.org
Reviewed-by: Leo Yan leo.yan@arm.com
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
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
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
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
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
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); }
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.
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
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
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
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
-
James Clark -
Leo Yan -
Mike Leach