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

Leo Yan
2025-12-02T12:15:40Z

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