linux-kernel - Re: [PATCH 6/7] perf cs-etm: Don't hard code config attribute when configuring the event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <857538e7-cac2-4871-9711-4c434420df8a@linaro.org>
Date: Thu, 4 Dec 2025 14:08:59 +0000
From: James Clark <james.clark@...aro.org>
To: Leo Yan <leo.yan@....com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
 Adrian Hunter <adrian.hunter@...el.com>,
 Suzuki K Poulose <suzuki.poulose@....com>, Mike Leach
 <mike.leach@...aro.org>, John Garry <john.g.garry@...cle.com>,
 Will Deacon <will@...nel.org>, Leo Yan <leo.yan@...ux.dev>,
 linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
 coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 6/7] perf cs-etm: Don't hard code config attribute when
 configuring the event



On 02/12/2025 12:15 pm, Leo Yan wrote:
> On Mon, Dec 01, 2025 at 04:41:09PM +0000, Coresight ML wrote:
> 
> [...]
> 
>> @@ -103,13 +103,20 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
>>   				      struct perf_cpu cpu)
>>   {
>>   	int err;
>> -	__u64 val;
>> -	u64 contextid = evsel->core.attr.config &
>> -		(perf_pmu__format_bits(cs_etm_pmu, "contextid") |
>> -		 perf_pmu__format_bits(cs_etm_pmu, "contextid1") |
>> -		 perf_pmu__format_bits(cs_etm_pmu, "contextid2"));
>> +	u64 ctxt, ctxt1, ctxt2;
>> +	__u64 trcidr2;
>>   
>> -	if (!contextid)
>> +	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid", &ctxt);
>> +	if (err)
>> +		return err;
>> +	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid1", &ctxt1);
>> +	if (err)
>> +		return err;
>> +	err = evsel__get_config_val(cs_etm_pmu, evsel, "contextid2", &ctxt2);
>> +	if (err)
>> +		return err;
> 
> Seems to me, this is not right.  The current code checks any context
> ID setting but it can tolerate if missing "contexid[*]" format.
> 
> After calling evsel__get_config_val(), if any "contextid[*]" format is
> missed, it returns error and will diretly bail out.  As a result,
> cs_etm_validate_context_id() will always return error.
> 

Missed by the driver or the user? evsel__get_config_val() only returns 
an error when "contextid" isn't published by the driver, not when the 
user doesn't supply one of those options. The actual user supplied value 
is in the out param, not the return value.

Having said that, this does make it an error if the driver did stop 
publishing one, which might be too inflexible and is a new behavior. I 
can change to it ignore the errors instead.

>> +
>> +	if (!ctxt && !ctxt1 && !ctxt2)
>>   		return 0;
>>   
>>   	/* Not supported in etmv3 */
>> @@ -120,12 +127,11 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
>>   	}
>>   
>>   	/* Get a handle on TRCIDR2 */
>> -	err = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR2], &val);
>> +	err = cs_etm_get_ro(cs_etm_pmu, cpu, metadata_etmv4_ro[CS_ETMV4_TRCIDR2], &trcidr2);
>>   	if (err)
>>   		return err;
>>   
>> -	if (contextid &
>> -	    perf_pmu__format_bits(cs_etm_pmu, "contextid1")) {
>> +	if (ctxt1) {
>>   		/*
>>   		 * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID
>>   		 * tracing is supported:
>> @@ -133,15 +139,14 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
>>   		 *  0b00100 Maximum of 32-bit Context ID size.
>>   		 *  All other values are reserved.
>>   		 */
>> -		if (BMVAL(val, 5, 9) != 0x4) {
>> +		if (BMVAL(trcidr2, 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;
>>   		}
>>   	}
>>   
>> -	if (contextid &
>> -	    perf_pmu__format_bits(cs_etm_pmu, "contextid2")) {
>> +	if (ctxt2) {
>>   		/*
>>   		 * TRCIDR2.VMIDOPT[30:29] != 0 and
>>   		 * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual contextid)
>> @@ -149,7 +154,7 @@ static int cs_etm_validate_context_id(struct perf_pmu *cs_etm_pmu, struct evsel
>>   		 * 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) {
>> +		if (!BMVAL(trcidr2, 29, 30) || BMVAL(trcidr2, 10, 14) < 4) {
>>   			pr_err("%s: CONTEXTIDR_EL2 isn't supported, disable with %s/contextid2=0/\n",
>>   			       CORESIGHT_ETM_PMU_NAME, CORESIGHT_ETM_PMU_NAME);
>>   			return -EINVAL;
>> @@ -163,10 +168,14 @@ static int cs_etm_validate_timestamp(struct perf_pmu *cs_etm_pmu, struct evsel *
>>   				     struct perf_cpu cpu)
>>   {
>>   	int err;
>> -	__u64 val;
>> +	u64 val;
>> +	__u64 trcidr0;
>>   
>> -	if (!(evsel->core.attr.config &
>> -	      perf_pmu__format_bits(cs_etm_pmu, "timestamp")))
>> +	err = evsel__get_config_val(cs_etm_pmu, evsel, "timestamp", &val);
>> +	if (err)
>> +		return err;
>> +
>> +	if (!val)
>>   		return 0;
> 
> Similiar issue here.  The current code returns 0 if not find "timestamp"
> format.  With this change, it returns error instead.
> 
> Thanks,
> Leo