Re: [PATCH v4 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: extout_pulse loads on ETE with single event lacking TRCEXTOUT1
--

commit 5f7a0d60c7147cc2e10191888168dd0008749154
Author: Amir Ayupov <aaupov@fb.com>

coresight: Add pmu_pulse/extout_pulse

This commit adds the built-in pmu_pulse configuration and extout_pulse
feature for ETMv4/ETE to pulse external output 1 on every traced cycle
for the PMU to count as TRCEXTOUT1.

> 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 @@
[ ... ]
> +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 and enable on an ETE implementation that only
supports a single event (event 0 / TRCEXTOUT0), where event 1 and
TRCEXTOUT1 do not exist?

In drivers/hwtracing/coresight/coresight-etm4x-core.c, when TRCIDR0.NUMEVENT
is 0 and resources are present, etm4_init_arch_data() sets caps->nr_event to
1. Then etm4_pmu_counts_extout() checks whether the implemented events have
matching PMU counters:

drivers/hwtracing/coresight/coresight-etm4x-core.c:etm4_pmu_counts_extout() {
	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 etm4_cscfg_register()
advertises CS_CFG_MATCH_CAP_TRCEXTOUT.

When loading the feature, etm4_cfg_load_feature() -> etm4_cfg_map_reg_offset()
maps TRCEVENTCTL0R without checking whether drvdata->caps.nr_event >= 2. By
contrast, event_store() in drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
shows that EVENT1 (bits [15:8] of TRCEVENTCTL0R) is only implemented when
caps->nr_event >= 2:

	case 1:
		/* EVENT0, bits[7:0] */
		sysfs_config->eventctrl0 = val & 0xFF;
		break;
	case 2:
		/* EVENT1, bits[15:8] */
		sysfs_config->eventctrl0 = val & 0xFFFF;
		break;

On an ETE with caps.nr_event == 1, enabling pmu_pulse will succeed and
write 0x0100 to the unimplemented EVENT1 field in TRCEVENTCTL0R while
TRCEXTOUT1 does not exist. Should CS_CFG_MATCH_CAP_TRCEXTOUT or the feature
loading path verify that event 1 (caps.nr_event >= 2 and TRCEXTOUT1) is
available?

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