Re: [PATCH v3 7/7] coresight: Add pmu_pulse/extout_pulse

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] coresight: pmu_pulse loads on 1-event ETE lacking event 1 and TRCEXTOUT1
--

commit 75adf8bc69eef9902a1a675969ecaf268f815f11
Author: Amir Ayupov <aaupov@fb.com>

coresight: Add pmu_pulse/extout_pulse

This commit adds a built-in pmu_pulse configuration and extout_pulse feature
for ETMv4/ETE trace units that program event 1 in TRCEVENTCTL0R to select the
always-true resource and pulse external output 1 (TRCEXTOUT1) on every traced
cycle.

> diff --git a/drivers/hwtracing/coresight/coresight-cfg-pmu-pulse.c b/drivers/hwtracing/coresight/coresight-cfg-pmu-pulse.c
> new file mode 100644
> index 0000000000000..61472154dd02a
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cfg-pmu-pulse.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Pulse ETM external output 1 on every traced cycle for a PMU event to count.
> + * Based on coresight-cfg-afdo.c
> + */
> +
> +#include "coresight-config.h"
> +
> +/* ETMv4 includes and features */
> +#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)
> +#include "coresight-etm4x-cfg.h"
> +
> +/*
> + * extout_pulse feature: event 1 selects the always-true resource, so ETM
> + * external output 1 pulses on every cycle while the trace unit is enabled. A
> + * PMU event counting that output (TRCEXTOUT1), for example with
> + * aux-action=pause, then fires after as many traced cycles as its sample
> + * period. Event 0 is avoided because it is the trace unit trigger, and the
> + * TRBE driver treats a detected trigger as fatal.
> + */
> +static struct cscfg_regval_desc extout_pulse_regs[] = {
> +	/* event 1: resource selector 1 (always true), drives external output 1 */
> +	{
> +		.type = CS_CFG_REG_TYPE_STD | CS_CFG_REG_TYPE_VAL_MASK,
> +		.offset = TRCEVENTCTL0R,
> +		.val32 = 0x0100,
> +		.mask32 = 0xff00,
> +	},
> +	/* end of regs */
> +};
> +
> +struct cscfg_feature_desc extout_pulse_etm4x = {
> +	.name = "extout_pulse",
> +	.description = "Pulse external output 1 on every traced cycle.\n",
> +	.match_flags = CS_CFG_MATCH_CLASS_SRC_ETM4 | CS_CFG_MATCH_CAP_TRCEXTOUT,
> +	.nr_regs = ARRAY_SIZE(extout_pulse_regs),
> +	.regs_desc = extout_pulse_regs,
> +};

[Severity: Medium]
Can extout_pulse_etm4x load on a 1-event ETE that only implements event 0 and
TRCEXTOUT0, where event 1 (TRCEVENTCTL0R[15:8]) and TRCEXTOUT1 do not exist?

In drivers/hwtracing/coresight/coresight-etm4x-core.c, on an ETE where
TRCIDR4.NUMRSPAIR > 0 and TRCIDR0.NUMEVENT == 0, etm4_init_arch_data() sets
drvdata->caps.nr_event = 1. When etm4_cscfg_register() checks
etm4_pmu_counts_extout(), it only verifies the events up to caps.nr_event:

static bool etm4_pmu_counts_extout(struct etmv4_drvdata *drvdata)
{
	int n;

	if (!etm4x_is_ete(drvdata) || !drvdata->caps.nr_event)
		return false;

	for (n = 0; n < drvdata->caps.nr_event; n++) {
		if (!pmuv3_event_implemented(ARMV8_PMUV3_PERFCTR_TRCEXTOUT0 + n))
			return false;
	}

	return true;
}

When drvdata->caps.nr_event == 1 and ARMV8_PMUV3_PERFCTR_TRCEXTOUT0 is
implemented, etm4_pmu_counts_extout() returns true and sets
CS_CFG_MATCH_CAP_TRCEXTOUT without checking TRCEXTOUT1.

During feature loading in drivers/hwtracing/coresight/coresight-etm4x-cfg.c,
etm4_cfg_load_feature() -> etm4_cfg_map_reg_offset() maps TRCEVENTCTL0R
unconditionally via CHECKREG(TRCEVENTCTL0R, eventctrl0) without checking
whether drvdata->caps.nr_event covers the event fields masked by the feature.

Should etm4_cfg_load_feature() or the capability check verify that
drvdata->caps.nr_event >= 2 before allowing extout_pulse_etm4x to load and
program TRCEVENTCTL0R[15:8]?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792907332.git.aaupov@fb.com?part=7