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

[Leo Yan <leo.yan@arm.com>]

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.

> +
> +	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