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

[James Clark <james.clark@arm.com>]

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
>