| Message ID | 20230424134748.228137-5-james.clark@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf: cs-etm: Fixes around timestamped and timeless decoding | expand |
Hi James, On Mon, Apr 24, 2023 at 02:47:44PM +0100, James Clark wrote: > Currently the cs_etm_set_option() function both validates and applies > the config options. Because it's only called when they are added > automatically, there are some paths where the user can apply the option > on the command line and skip the validation. By moving it to the end it > covers both cases. > > Also, options don't need to be re-applied anyway, Perf handles parsing > and applying the config terms automatically. > > Signed-off-by: James Clark <james.clark@arm.com> > --- > tools/perf/arch/arm/util/cs-etm.c | 152 +++++++++++++----------------- > 1 file changed, 68 insertions(+), 84 deletions(-) > > diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c > index f9b9ebf7fffc..af0a2400c655 100644 > --- a/tools/perf/arch/arm/util/cs-etm.c > +++ b/tools/perf/arch/arm/util/cs-etm.c > @@ -69,21 +69,29 @@ static const char * const metadata_ete_ro[] = { > static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu); > static bool cs_etm_is_ete(struct auxtrace_record *itr, int cpu); > > -static int cs_etm_set_context_id(struct auxtrace_record *itr, > - struct evsel *evsel, int cpu) > +static int cs_etm_validate_context_id(struct auxtrace_record *itr, > + struct evsel *evsel, int cpu) > { > - struct cs_etm_recording *ptr; > - struct perf_pmu *cs_etm_pmu; > + struct cs_etm_recording *ptr = > + container_of(itr, struct cs_etm_recording, itr); > + struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu; > char path[PATH_MAX]; > - int err = -EINVAL; > + int err; > u32 val; > - u64 contextid; > + u64 contextid = > + evsel->core.attr.config & > + (perf_pmu__format_bits(&cs_etm_pmu->format, "contextid1") | > + perf_pmu__format_bits(&cs_etm_pmu->format, "contextid2")); Seems to me, this would break backward compability. The old kernel (before 5.11) doesn't provide 'contextid1' and 'contextid2', so we always check the entry 'contextid' rather than 'contextid1' and 'contextid2'. With this change, if a kernel doesn't contain 'contextid1' and 'contextid2' formats, will perf tool never trace for contexid? Thanks, Leo > > - ptr = container_of(itr, struct cs_etm_recording, itr); > - cs_etm_pmu = ptr->cs_etm_pmu; > + if (!contextid) > + return 0; > > - if (!cs_etm_is_etmv4(itr, cpu)) > - goto out; > + /* Not supported in etmv3 */ > + if (!cs_etm_is_etmv4(itr, cpu)) { > + pr_err("%s: contextid not supported in ETMv3, disable with %s/contextid=0/\n", > + CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME); > + return -EINVAL; > + } > > /* Get a handle on TRCIDR2 */ > snprintf(path, PATH_MAX, "cpu%d/%s", > @@ -92,27 +100,13 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr, > > /* There was a problem reading the file, bailing out */ > if (err != 1) { > - pr_err("%s: can't read file %s\n", > - CORESIGHT_ETM_PMU_NAME, path); > - goto out; > + pr_err("%s: can't read file %s\n", CORESIGHT_ETM_PMU_NAME, > + path); > + return err; > } > > - /* User has configured for PID tracing, respects it. */ > - contextid = evsel->core.attr.config & > - (BIT(ETM_OPT_CTXTID) | BIT(ETM_OPT_CTXTID2)); > - > - /* > - * If user doesn't configure the contextid format, parse PMU format and > - * enable PID tracing according to the "contextid" format bits: > - * > - * If bit ETM_OPT_CTXTID is set, trace CONTEXTIDR_EL1; > - * If bit ETM_OPT_CTXTID2 is set, trace CONTEXTIDR_EL2. > - */ > - if (!contextid) > - contextid = perf_pmu__format_bits(&cs_etm_pmu->format, > - "contextid"); > - > - if (contextid & BIT(ETM_OPT_CTXTID)) { > + if (contextid & > + perf_pmu__format_bits(&cs_etm_pmu->format, "contextid1")) { > /* > * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID > * tracing is supported: > @@ -122,14 +116,14 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr, > */ > val = BMVAL(val, 5, 9); > if (!val || val != 0x4) { > - pr_err("%s: CONTEXTIDR_EL1 isn't supported\n", > - CORESIGHT_ETM_PMU_NAME); > - err = -EINVAL; > - goto out; > + 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 & BIT(ETM_OPT_CTXTID2)) { > + if (contextid & > + perf_pmu__format_bits(&cs_etm_pmu->format, "contextid2")) { > /* > * TRCIDR2.VMIDOPT[30:29] != 0 and > * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual contextid) > @@ -138,35 +132,34 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr, > * Any value of VMIDSIZE >= 4 (i.e, > 32bit) is fine for us. > */ > if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) { > - pr_err("%s: CONTEXTIDR_EL2 isn't supported\n", > - CORESIGHT_ETM_PMU_NAME); > - err = -EINVAL; > - goto out; > + pr_err("%s: CONTEXTIDR_EL2 isn't supported, disable with %s/contextid2=0/\n", > + CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME); > + return -EINVAL; > } > } > > - /* All good, let the kernel know */ > - evsel->core.attr.config |= contextid; > - err = 0; > - > -out: > - return err; > + return 0; > } > > -static int cs_etm_set_timestamp(struct auxtrace_record *itr, > - struct evsel *evsel, int cpu) > +static int cs_etm_validate_timestamp(struct auxtrace_record *itr, > + struct evsel *evsel, int cpu) > { > - struct cs_etm_recording *ptr; > - struct perf_pmu *cs_etm_pmu; > + struct cs_etm_recording *ptr = > + container_of(itr, struct cs_etm_recording, itr); > + struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu; > char path[PATH_MAX]; > - int err = -EINVAL; > + int err; > u32 val; > > - ptr = container_of(itr, struct cs_etm_recording, itr); > - cs_etm_pmu = ptr->cs_etm_pmu; > + if (!(evsel->core.attr.config & > + perf_pmu__format_bits(&cs_etm_pmu->format, "timestamp"))) > + return 0; > > - if (!cs_etm_is_etmv4(itr, cpu)) > - goto out; > + if (!cs_etm_is_etmv4(itr, cpu)) { > + pr_err("%s: timestamp not supported in ETMv3, disable with %s/timestamp=0/\n", > + CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME); > + return -EINVAL; > + } > > /* Get a handle on TRCIRD0 */ > snprintf(path, PATH_MAX, "cpu%d/%s", > @@ -177,7 +170,7 @@ static int cs_etm_set_timestamp(struct auxtrace_record *itr, > if (err != 1) { > pr_err("%s: can't read file %s\n", > CORESIGHT_ETM_PMU_NAME, path); > - goto out; > + return err; > } > > /* > @@ -189,24 +182,21 @@ static int cs_etm_set_timestamp(struct auxtrace_record *itr, > */ > val &= GENMASK(28, 24); > if (!val) { > - err = -EINVAL; > - goto out; > + return -EINVAL; > } > > - /* All good, let the kernel know */ > - evsel->core.attr.config |= (1 << ETM_OPT_TS); > - err = 0; > - > -out: > - return err; > + return 0; > } > > -#define ETM_SET_OPT_CTXTID (1 << 0) > -#define ETM_SET_OPT_TS (1 << 1) > -#define ETM_SET_OPT_MASK (ETM_SET_OPT_CTXTID | ETM_SET_OPT_TS) > - > -static int cs_etm_set_option(struct auxtrace_record *itr, > - struct evsel *evsel, u32 option) > +/* > + * Check whether the requested timestamp and contextid options should be > + * available on all requested CPUs and if not, tell the user how to override. > + * The kernel will silently disable any unavailable options so a warning here > + * first is better. In theory the kernel could still disable the option for > + * some other reason so this is best effort only. > + */ > +static int cs_etm_validate_config(struct auxtrace_record *itr, > + struct evsel *evsel) > { > int i, err = -EINVAL; > struct perf_cpu_map *event_cpus = evsel->evlist->core.user_requested_cpus; > @@ -220,18 +210,11 @@ static int cs_etm_set_option(struct auxtrace_record *itr, > !perf_cpu_map__has(online_cpus, cpu)) > continue; > > - if (option & BIT(ETM_OPT_CTXTID)) { > - err = cs_etm_set_context_id(itr, evsel, i); > - if (err) > - goto out; > - } > - if (option & BIT(ETM_OPT_TS)) { > - err = cs_etm_set_timestamp(itr, evsel, i); > - if (err) > - goto out; > - } > - if (option & ~(BIT(ETM_OPT_CTXTID) | BIT(ETM_OPT_TS))) > - /* Nothing else is currently supported */ > + err = cs_etm_validate_context_id(itr, evsel, i); > + if (err) > + goto out; > + err = cs_etm_validate_timestamp(itr, evsel, i); > + if (err) > goto out; > } > > @@ -447,10 +430,10 @@ static int cs_etm_recording_options(struct auxtrace_record *itr, > * when a context switch happened. > */ > if (!perf_cpu_map__empty(cpus)) { > - err = cs_etm_set_option(itr, cs_etm_evsel, > - BIT(ETM_OPT_CTXTID) | BIT(ETM_OPT_TS)); > - if (err) > - goto out; > + cs_etm_evsel->core.attr.config |= > + perf_pmu__format_bits(&cs_etm_pmu->format, "timestamp"); > + cs_etm_evsel->core.attr.config |= > + perf_pmu__format_bits(&cs_etm_pmu->format, "contextid"); > } > > /* Add dummy event to keep tracking */ > @@ -466,6 +449,7 @@ static int cs_etm_recording_options(struct auxtrace_record *itr, > if (!perf_cpu_map__empty(cpus)) > evsel__set_sample_bit(evsel, TIME); > > + err = cs_etm_validate_config(itr, cs_etm_evsel); > out: > return err; > } > -- > 2.34.1 >
On 27/04/2023 16:12, Leo Yan wrote: > Hi James, > > On Mon, Apr 24, 2023 at 02:47:44PM +0100, James Clark wrote: >> Currently the cs_etm_set_option() function both validates and applies >> the config options. Because it's only called when they are added >> automatically, there are some paths where the user can apply the option >> on the command line and skip the validation. By moving it to the end it >> covers both cases. >> >> Also, options don't need to be re-applied anyway, Perf handles parsing >> and applying the config terms automatically. >> >> Signed-off-by: James Clark <james.clark@arm.com> >> --- >> tools/perf/arch/arm/util/cs-etm.c | 152 +++++++++++++----------------- >> 1 file changed, 68 insertions(+), 84 deletions(-) >> >> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c >> index f9b9ebf7fffc..af0a2400c655 100644 >> --- a/tools/perf/arch/arm/util/cs-etm.c >> +++ b/tools/perf/arch/arm/util/cs-etm.c >> @@ -69,21 +69,29 @@ static const char * const metadata_ete_ro[] = { >> static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu); >> static bool cs_etm_is_ete(struct auxtrace_record *itr, int cpu); >> >> -static int cs_etm_set_context_id(struct auxtrace_record *itr, >> - struct evsel *evsel, int cpu) >> +static int cs_etm_validate_context_id(struct auxtrace_record *itr, >> + struct evsel *evsel, int cpu) >> { >> - struct cs_etm_recording *ptr; >> - struct perf_pmu *cs_etm_pmu; >> + struct cs_etm_recording *ptr = >> + container_of(itr, struct cs_etm_recording, itr); >> + struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu; >> char path[PATH_MAX]; >> - int err = -EINVAL; >> + int err; >> u32 val; >> - u64 contextid; >> + u64 contextid = >> + evsel->core.attr.config & >> + (perf_pmu__format_bits(&cs_etm_pmu->format, "contextid1") | >> + perf_pmu__format_bits(&cs_etm_pmu->format, "contextid2")); > > Seems to me, this would break backward compability. > > The old kernel (before 5.11) doesn't provide 'contextid1' and > 'contextid2', so we always check the entry 'contextid' rather than > 'contextid1' and 'contextid2'. > > With this change, if a kernel doesn't contain 'contextid1' and > 'contextid2' formats, will perf tool never trace for contexid? > No because I changed to to be purely validation, so the format flags would still be applied. But yes I think you are right there is a small issue. Now validation of 'contextid' isn't done on pre 5.11 kernels. But that only checks for ETMv3 anyway. Validation of 'contextid1' and 'contextid2' isn't a problem, because if the kernel doesn't support them they can't be applied on the command line anyway. I can fix it by checking for 'contextid' and ETMv3 first and then doing 'contextid1' and 'contextid2' after. > Thanks, > Leo >
On Thu, Apr 27, 2023 at 04:52:06PM +0100, James Clark wrote: [...] > >> -static int cs_etm_set_context_id(struct auxtrace_record *itr, > >> - struct evsel *evsel, int cpu) > >> +static int cs_etm_validate_context_id(struct auxtrace_record *itr, > >> + struct evsel *evsel, int cpu) > >> { > >> - struct cs_etm_recording *ptr; > >> - struct perf_pmu *cs_etm_pmu; > >> + struct cs_etm_recording *ptr = > >> + container_of(itr, struct cs_etm_recording, itr); > >> + struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu; > >> char path[PATH_MAX]; > >> - int err = -EINVAL; > >> + int err; > >> u32 val; > >> - u64 contextid; > >> + u64 contextid = > >> + evsel->core.attr.config & > >> + (perf_pmu__format_bits(&cs_etm_pmu->format, "contextid1") | > >> + perf_pmu__format_bits(&cs_etm_pmu->format, "contextid2")); > > > > Seems to me, this would break backward compability. > > > > The old kernel (before 5.11) doesn't provide 'contextid1' and > > 'contextid2', so we always check the entry 'contextid' rather than > > 'contextid1' and 'contextid2'. > > > > With this change, if a kernel doesn't contain 'contextid1' and > > 'contextid2' formats, will perf tool never trace for contexid? > > > > No because I changed to to be purely validation, so the format flags > would still be applied. But yes I think you are right there is a small > issue. > > Now validation of 'contextid' isn't done on pre 5.11 kernels. But that > only checks for ETMv3 anyway. IIUC, 'contextid' is not only used for ETMv3. Just quotes the comments from drivers/hwtracing/coresight/coresight-etm-perf.c: 73 /* 74 * contextid always traces the "PID". The PID is in CONTEXTIDR_EL1 75 * when the kernel is running at EL1; when the kernel is at EL2, 76 * the PID is in CONTEXTIDR_EL2. 77 */ 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. 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 > Validation of 'contextid1' and > 'contextid2' isn't a problem, because if the kernel doesn't support them > they can't be applied on the command line anyway. > > I can fix it by checking for 'contextid' and ETMv3 first and then doing > 'contextid1' and 'contextid2' after.
On 27/04/2023 23:10, Leo Yan wrote: > On Thu, Apr 27, 2023 at 04:52:06PM +0100, James Clark wrote: > > [...] > >>>> -static int cs_etm_set_context_id(struct auxtrace_record *itr, >>>> - struct evsel *evsel, int cpu) >>>> +static int cs_etm_validate_context_id(struct auxtrace_record *itr, >>>> + struct evsel *evsel, int cpu) >>>> { >>>> - struct cs_etm_recording *ptr; >>>> - struct perf_pmu *cs_etm_pmu; >>>> + struct cs_etm_recording *ptr = >>>> + container_of(itr, struct cs_etm_recording, itr); >>>> + struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu; >>>> char path[PATH_MAX]; >>>> - int err = -EINVAL; >>>> + int err; >>>> u32 val; >>>> - u64 contextid; >>>> + u64 contextid = >>>> + evsel->core.attr.config & >>>> + (perf_pmu__format_bits(&cs_etm_pmu->format, "contextid1") | >>>> + perf_pmu__format_bits(&cs_etm_pmu->format, "contextid2")); >>> >>> Seems to me, this would break backward compability. >>> >>> The old kernel (before 5.11) doesn't provide 'contextid1' and >>> 'contextid2', so we always check the entry 'contextid' rather than >>> 'contextid1' and 'contextid2'. >>> >>> With this change, if a kernel doesn't contain 'contextid1' and >>> 'contextid2' formats, will perf tool never trace for contexid? >>> >> >> No because I changed to to be purely validation, so the format flags >> would still be applied. But yes I think you are right there is a small >> issue. >> >> Now validation of 'contextid' isn't done on pre 5.11 kernels. But that >> only checks for ETMv3 anyway. > > IIUC, 'contextid' is not only used for ETMv3. Just quotes the comments > from drivers/hwtracing/coresight/coresight-etm-perf.c: > I meant that the validation only looks for the presence of ETMv3 and shows a warning in that scenario, so that was the only thing not working any more. Not that it's only used for ETMv3. > 73 /* > 74 * contextid always traces the "PID". The PID is in CONTEXTIDR_EL1 > 75 * when the kernel is running at EL1; when the kernel is at EL2, > 76 * the PID is in CONTEXTIDR_EL2. > 77 */ > > 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); } > 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> --- 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") | perf_pmu__format_bits(&cs_etm_pmu->format, "contextid2")); 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;
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") | > perf_pmu__format_bits(&cs_etm_pmu->format, "contextid2")); > > 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
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index f9b9ebf7fffc..af0a2400c655 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -69,21 +69,29 @@ static const char * const metadata_ete_ro[] = { static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu); static bool cs_etm_is_ete(struct auxtrace_record *itr, int cpu); -static int cs_etm_set_context_id(struct auxtrace_record *itr, - struct evsel *evsel, int cpu) +static int cs_etm_validate_context_id(struct auxtrace_record *itr, + struct evsel *evsel, int cpu) { - struct cs_etm_recording *ptr; - struct perf_pmu *cs_etm_pmu; + struct cs_etm_recording *ptr = + container_of(itr, struct cs_etm_recording, itr); + struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu; char path[PATH_MAX]; - int err = -EINVAL; + int err; u32 val; - u64 contextid; + u64 contextid = + evsel->core.attr.config & + (perf_pmu__format_bits(&cs_etm_pmu->format, "contextid1") | + perf_pmu__format_bits(&cs_etm_pmu->format, "contextid2")); - ptr = container_of(itr, struct cs_etm_recording, itr); - cs_etm_pmu = ptr->cs_etm_pmu; + if (!contextid) + return 0; - if (!cs_etm_is_etmv4(itr, cpu)) - goto out; + /* Not supported in etmv3 */ + if (!cs_etm_is_etmv4(itr, cpu)) { + pr_err("%s: contextid not supported in ETMv3, disable with %s/contextid=0/\n", + CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME); + return -EINVAL; + } /* Get a handle on TRCIDR2 */ snprintf(path, PATH_MAX, "cpu%d/%s", @@ -92,27 +100,13 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr, /* There was a problem reading the file, bailing out */ if (err != 1) { - pr_err("%s: can't read file %s\n", - CORESIGHT_ETM_PMU_NAME, path); - goto out; + pr_err("%s: can't read file %s\n", CORESIGHT_ETM_PMU_NAME, + path); + return err; } - /* User has configured for PID tracing, respects it. */ - contextid = evsel->core.attr.config & - (BIT(ETM_OPT_CTXTID) | BIT(ETM_OPT_CTXTID2)); - - /* - * If user doesn't configure the contextid format, parse PMU format and - * enable PID tracing according to the "contextid" format bits: - * - * If bit ETM_OPT_CTXTID is set, trace CONTEXTIDR_EL1; - * If bit ETM_OPT_CTXTID2 is set, trace CONTEXTIDR_EL2. - */ - if (!contextid) - contextid = perf_pmu__format_bits(&cs_etm_pmu->format, - "contextid"); - - if (contextid & BIT(ETM_OPT_CTXTID)) { + if (contextid & + perf_pmu__format_bits(&cs_etm_pmu->format, "contextid1")) { /* * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID * tracing is supported: @@ -122,14 +116,14 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr, */ val = BMVAL(val, 5, 9); if (!val || val != 0x4) { - pr_err("%s: CONTEXTIDR_EL1 isn't supported\n", - CORESIGHT_ETM_PMU_NAME); - err = -EINVAL; - goto out; + 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 & BIT(ETM_OPT_CTXTID2)) { + if (contextid & + perf_pmu__format_bits(&cs_etm_pmu->format, "contextid2")) { /* * TRCIDR2.VMIDOPT[30:29] != 0 and * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual contextid) @@ -138,35 +132,34 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr, * Any value of VMIDSIZE >= 4 (i.e, > 32bit) is fine for us. */ if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) { - pr_err("%s: CONTEXTIDR_EL2 isn't supported\n", - CORESIGHT_ETM_PMU_NAME); - err = -EINVAL; - goto out; + pr_err("%s: CONTEXTIDR_EL2 isn't supported, disable with %s/contextid2=0/\n", + CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME); + return -EINVAL; } } - /* All good, let the kernel know */ - evsel->core.attr.config |= contextid; - err = 0; - -out: - return err; + return 0; } -static int cs_etm_set_timestamp(struct auxtrace_record *itr, - struct evsel *evsel, int cpu) +static int cs_etm_validate_timestamp(struct auxtrace_record *itr, + struct evsel *evsel, int cpu) { - struct cs_etm_recording *ptr; - struct perf_pmu *cs_etm_pmu; + struct cs_etm_recording *ptr = + container_of(itr, struct cs_etm_recording, itr); + struct perf_pmu *cs_etm_pmu = ptr->cs_etm_pmu; char path[PATH_MAX]; - int err = -EINVAL; + int err; u32 val; - ptr = container_of(itr, struct cs_etm_recording, itr); - cs_etm_pmu = ptr->cs_etm_pmu; + if (!(evsel->core.attr.config & + perf_pmu__format_bits(&cs_etm_pmu->format, "timestamp"))) + return 0; - if (!cs_etm_is_etmv4(itr, cpu)) - goto out; + if (!cs_etm_is_etmv4(itr, cpu)) { + pr_err("%s: timestamp not supported in ETMv3, disable with %s/timestamp=0/\n", + CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME); + return -EINVAL; + } /* Get a handle on TRCIRD0 */ snprintf(path, PATH_MAX, "cpu%d/%s", @@ -177,7 +170,7 @@ static int cs_etm_set_timestamp(struct auxtrace_record *itr, if (err != 1) { pr_err("%s: can't read file %s\n", CORESIGHT_ETM_PMU_NAME, path); - goto out; + return err; } /* @@ -189,24 +182,21 @@ static int cs_etm_set_timestamp(struct auxtrace_record *itr, */ val &= GENMASK(28, 24); if (!val) { - err = -EINVAL; - goto out; + return -EINVAL; } - /* All good, let the kernel know */ - evsel->core.attr.config |= (1 << ETM_OPT_TS); - err = 0; - -out: - return err; + return 0; } -#define ETM_SET_OPT_CTXTID (1 << 0) -#define ETM_SET_OPT_TS (1 << 1) -#define ETM_SET_OPT_MASK (ETM_SET_OPT_CTXTID | ETM_SET_OPT_TS) - -static int cs_etm_set_option(struct auxtrace_record *itr, - struct evsel *evsel, u32 option) +/* + * Check whether the requested timestamp and contextid options should be + * available on all requested CPUs and if not, tell the user how to override. + * The kernel will silently disable any unavailable options so a warning here + * first is better. In theory the kernel could still disable the option for + * some other reason so this is best effort only. + */ +static int cs_etm_validate_config(struct auxtrace_record *itr, + struct evsel *evsel) { int i, err = -EINVAL; struct perf_cpu_map *event_cpus = evsel->evlist->core.user_requested_cpus; @@ -220,18 +210,11 @@ static int cs_etm_set_option(struct auxtrace_record *itr, !perf_cpu_map__has(online_cpus, cpu)) continue; - if (option & BIT(ETM_OPT_CTXTID)) { - err = cs_etm_set_context_id(itr, evsel, i); - if (err) - goto out; - } - if (option & BIT(ETM_OPT_TS)) { - err = cs_etm_set_timestamp(itr, evsel, i); - if (err) - goto out; - } - if (option & ~(BIT(ETM_OPT_CTXTID) | BIT(ETM_OPT_TS))) - /* Nothing else is currently supported */ + err = cs_etm_validate_context_id(itr, evsel, i); + if (err) + goto out; + err = cs_etm_validate_timestamp(itr, evsel, i); + if (err) goto out; } @@ -447,10 +430,10 @@ static int cs_etm_recording_options(struct auxtrace_record *itr, * when a context switch happened. */ if (!perf_cpu_map__empty(cpus)) { - err = cs_etm_set_option(itr, cs_etm_evsel, - BIT(ETM_OPT_CTXTID) | BIT(ETM_OPT_TS)); - if (err) - goto out; + cs_etm_evsel->core.attr.config |= + perf_pmu__format_bits(&cs_etm_pmu->format, "timestamp"); + cs_etm_evsel->core.attr.config |= + perf_pmu__format_bits(&cs_etm_pmu->format, "contextid"); } /* Add dummy event to keep tracking */ @@ -466,6 +449,7 @@ static int cs_etm_recording_options(struct auxtrace_record *itr, if (!perf_cpu_map__empty(cpus)) evsel__set_sample_bit(evsel, TIME); + err = cs_etm_validate_config(itr, cs_etm_evsel); out: return err; }
Currently the cs_etm_set_option() function both validates and applies the config options. Because it's only called when they are added automatically, there are some paths where the user can apply the option on the command line and skip the validation. By moving it to the end it covers both cases. Also, options don't need to be re-applied anyway, Perf handles parsing and applying the config terms automatically. Signed-off-by: James Clark <james.clark@arm.com> --- tools/perf/arch/arm/util/cs-etm.c | 152 +++++++++++++----------------- 1 file changed, 68 insertions(+), 84 deletions(-)