Re: [PATCH 4/7] perf: cs-etm: Validate options after applying themperf_pmu__format_bits

[Leo Yan]

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.