On Fri, Apr 28, 2023 at 01:33:16PM +0100, James Clark wrote:
[...]
ETMv4 uses 'contextid' as well, since the user space needs to know which exception level's PID should be traced, e.g. when CPU runs in EL2 'contextid' is set as ETM_OPT_CTXTID2, the perf tool will set 'contextid2' to tell driver to trace CONTEXTIDR_EL2.
That's still working because it reads the config term in the setup function rather than setting any one bit manually:
if (!perf_cpu_map__empty(cpus)) { evsel__set_config_if_unset(cs_etm_pmu, cs_etm_evsel, "timestamp", 1); evsel__set_config_if_unset(cs_etm_pmu, cs_etm_evsel, "contextid", 1); }
Okay, yeah, this change can set CTXTID or CTXTID2 bit if user doesn't set anything.
We can only verify 'contextid', and set 'contextid1' or 'contextid2' based on CPU running exception level, finally driver knows how to trace PID.
Thanks, Leo
I'm not 100% sure what you mean by this. But previously the validation was looking at both contextid1 and contextid2 options and checking if either were supported if either were set.
I have the following change in mind, it fixes the backwards compatibility issue. And the validation should be exactly the same as it was before. Except for one bug that I found when both bits are requested which I've also fixed here:
From f1b9f56df29dfb4f2a7be25f009c79c86335587a Mon Sep 17 00:00:00 2001 From: James Clark james.clark@arm.com Date: Fri, 28 Apr 2023 10:29:52 +0100 Subject: [PATCH] perf cs-etm: Fix contextid validation
Pre 5.11 kernels don't support 'contextid1' and 'contextid2' so validation would be skipped. By adding an additional check for 'contextid', old kernels will still have validation done even though contextid would either be contextid1 or contextid2.
Additionally now that it's possible to override options, an existing bug in the validation is revealed. 'val' is overwritten by the contextid1 validation, and re-used for contextid2 validation causing it to always fail. '!val || val != 0x4' is the same as 'val != 0x4' because 0 is also != 4, so that expression can be simplified and the temp variable not overwritten.
Fixes: 35c51f83dd1e ("perf cs-etm: Validate options after applying them") Signed-off-by: James Clark james.clark@arm.com
LGTM:
Reviewed-by: Leo Yan <leo.yan@linaro.org
Thanks for the fixing!
tools/perf/arch/arm/util/cs-etm.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index 77cb03e6ff87..9ca040bfb1aa 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -78,9 +78,9 @@ static int cs_etm_validate_context_id(struct auxtrace_record *itr, char path[PATH_MAX]; int err; u32 val;
- u64 contextid =
evsel->core.attr.config &(perf_pmu__format_bits(&cs_etm_pmu->format, "contextid1") |
- u64 contextid = evsel->core.attr.config &
(perf_pmu__format_bits(&cs_etm_pmu->format, "contextid") |perf_pmu__format_bits(&cs_etm_pmu->format, "contextid1") |if (!contextid) @@ -114,8 +114,7 @@ static int cs_etm_validate_context_id(struct auxtrace_record *itr, * 0b00100 Maximum of 32-bit Context ID size. * All other values are reserved. */
val = BMVAL(val, 5, 9);if (!val || val != 0x4) {
if (BMVAL(val, 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;-- 2.34.1