Messages in this thread | | | Date | Fri, 15 Jan 2021 15:44:16 -0700 | From | Mathieu Poirier <> | Subject | Re: [PATCH v1 2/7] perf cs_etm: Use pid tracing explicitly instead of contextid |
| |
On Sat, Jan 09, 2021 at 03:44:30PM +0800, Leo Yan wrote: > From: Suzuki K Poulose <suzuki.poulose@arm.com> > > If the kernel is running at EL2, the pid of the task is exposed > via VMID instead of the CONTEXTID. Add support for this in the > perf tool. > > By default the perf tool requests contextid and timestamp for > task bound events. Instead of hard coding contextid, switch > to "pid" config exposed by the kernel. While at it, define new > independent macros (rather than using the "config" bits) for > requesting the "pid" and "timestamp" for cs_etm_set_option(), > since the PID config is now dynamic depending on the kernel > exception level. > > Cc: Mike Leach <mike.leach@linaro.org> > Cc: Mathieu Poirier <mathieu.poirier@linaro.org> > Cc: Al Grant <al.grant@arm.com> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > Signed-off-by: Leo Yan <leo.yan@linaro.org> > --- > tools/include/linux/coresight-pmu.h | 11 +++-- > tools/perf/arch/arm/util/cs-etm.c | 68 ++++++++++++++++++++++------- > 2 files changed, 59 insertions(+), 20 deletions(-) > > diff --git a/tools/include/linux/coresight-pmu.h b/tools/include/linux/coresight-pmu.h > index b0e35eec6499..927c6285ce5d 100644 > --- a/tools/include/linux/coresight-pmu.h > +++ b/tools/include/linux/coresight-pmu.h > @@ -11,16 +11,19 @@ > #define CORESIGHT_ETM_PMU_SEED 0x10 > > /* ETMv3.5/PTM's ETMCR config bit */ > -#define ETM_OPT_CYCACC 12 > -#define ETM_OPT_CTXTID 14 > -#define ETM_OPT_TS 28 > -#define ETM_OPT_RETSTK 29 > +#define ETM_OPT_CYCACC 12 > +#define ETM_OPT_CTXTID 14 > +#define ETM_OPT_CTXTID_IN_VMID 15 > +#define ETM_OPT_TS 28 > +#define ETM_OPT_RETSTK 29 > > /* ETMv4 CONFIGR programming bits for the ETM OPTs */ > #define ETM4_CFG_BIT_CYCACC 4 > #define ETM4_CFG_BIT_CTXTID 6 > +#define ETM4_CFG_BIT_VMID 7 > #define ETM4_CFG_BIT_TS 11 > #define ETM4_CFG_BIT_RETSTK 12 > +#define ETM4_CFG_BIT_VMID_OPT 15
Wasn't this done in the previous patch?
> > static inline int coresight_get_trace_id(int cpu) > { > diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c > index cad7bf783413..fad7b6e13ccc 100644 > --- a/tools/perf/arch/arm/util/cs-etm.c > +++ b/tools/perf/arch/arm/util/cs-etm.c > @@ -59,14 +59,15 @@ static const char *metadata_etmv4_ro[CS_ETMV4_PRIV_MAX] = { > > static bool cs_etm_is_etmv4(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_set_pid(struct auxtrace_record *itr, > + struct evsel *evsel, int cpu) > { > struct cs_etm_recording *ptr; > struct perf_pmu *cs_etm_pmu; > char path[PATH_MAX]; > int err = -EINVAL; > u32 val; > + u64 pid_fmt; > > ptr = container_of(itr, struct cs_etm_recording, itr); > cs_etm_pmu = ptr->cs_etm_pmu; > @@ -86,21 +87,50 @@ static int cs_etm_set_context_id(struct auxtrace_record *itr, > goto out; > } > > + pid_fmt = perf_pmu__format_bits(&cs_etm_pmu->format, "pid");
If we go with my suggestion from patch 1 this would become "contextid".
I think what follows below is all good.
> /* > - * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID tracing > - * is supported: > - * 0b00000 Context ID tracing is not supported. > - * 0b00100 Maximum of 32-bit Context ID size. > - * All other values are reserved. > + * If the kernel doesn't support the "pid" format (older kernel), > + * fall back to using the CTXTID. > */ > - val = BMVAL(val, 5, 9); > - if (!val || val != 0x4) { > + if (!pid_fmt) > + pid_fmt = 1ULL << ETM_OPT_CTXTID; > + > + switch (pid_fmt) { > + case (1ULL << ETM_OPT_CTXTID): > + /* > + * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID > + * tracing is supported: > + * 0b00000 Context ID tracing is not supported. > + * 0b00100 Maximum of 32-bit Context ID size. > + * All other values are reserved. > + */ > + val = BMVAL(val, 5, 9); > + if (!val || val != 0x4) { > + err = -EINVAL; > + goto out; > + } > + break; > + case (1ULL << ETM_OPT_CTXTID_IN_VMID): > + /* > + * TRCIDR2.VMIDOPT[30:29] != 0 and > + * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual contextid) > + * We can't support CONTEXTIDR in VMID if the size of the > + * 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) { > + err = -EINVAL; > + goto out; > + } > + break; > + default: > err = -EINVAL; > goto out; > } > > + > /* All good, let the kernel know */ > - evsel->core.attr.config |= (1 << ETM_OPT_CTXTID); > + evsel->core.attr.config |= pid_fmt; > err = 0; > > out: > @@ -156,6 +186,10 @@ static int cs_etm_set_timestamp(struct auxtrace_record *itr, > return err; > } > > +#define ETM_SET_OPT_PID (1 << 0) > +#define ETM_SET_OPT_TS (1 << 1) > +#define ETM_SET_OPT_MASK (ETM_SET_OPT_PID | ETM_SET_OPT_TS) > +
I don't think we need this as part of the current patchset. Back in the days I thought it would be a good idea to use the ETMv3.5/PTM configuration bits but in hindsight it wasn't. That's what happens when your crystal ball isn't working.
That being said we can start treating the perf_event_attr::config bits arbitrarily. Someone pointed out that it might get confusing but I don't think it is anything that can't be overcomed with good in-line documentation.
> static int cs_etm_set_option(struct auxtrace_record *itr, > struct evsel *evsel, u32 option) > { > @@ -169,17 +203,17 @@ static int cs_etm_set_option(struct auxtrace_record *itr, > !cpu_map__has(online_cpus, i)) > continue; > > - if (option & ETM_OPT_CTXTID) { > - err = cs_etm_set_context_id(itr, evsel, i); > + if (option & ETM_SET_OPT_PID) { > + err = cs_etm_set_pid(itr, evsel, i); > if (err) > goto out; > } > - if (option & ETM_OPT_TS) { > + if (option & ETM_SET_OPT_TS) { > err = cs_etm_set_timestamp(itr, evsel, i); > if (err) > goto out; > } > - if (option & ~(ETM_OPT_CTXTID | ETM_OPT_TS)) > + if (option & ~(ETM_SET_OPT_MASK)) > /* Nothing else is currently supported */ > goto out; > } > @@ -406,7 +440,7 @@ static int cs_etm_recording_options(struct auxtrace_record *itr, > evsel__set_sample_bit(cs_etm_evsel, CPU); > > err = cs_etm_set_option(itr, cs_etm_evsel, > - ETM_OPT_CTXTID | ETM_OPT_TS); > + ETM_SET_OPT_PID | ETM_SET_OPT_TS); > if (err) > goto out; > } > @@ -485,7 +519,9 @@ static u64 cs_etmv4_get_config(struct auxtrace_record *itr) > 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_CTXTID_IN_VMID)) > + config |= BIT(ETM4_CFG_BIT_VMID) | > + BIT(ETM4_CFG_BIT_VMID_OPT); > return config; > } > > -- > 2.25.1 >
| |