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

[James Clark]

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: