Re: [PATCH] perf/arm_cspmu: Support 64-bit programmers' model

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unconditional call to `is_cycle_counter_event` in `nvidia_cspmu.c` can lead to a NULL pointer dereference when `has_ext64` is true.
- [High] Unconditional 64-bit MMIO accesses (`writeq`) to EXT64 configuration registers can cause bus faults on systems lacking 64-bit atomic access support.
- [High] The sysfs format definition for 'event' incorrectly truncates to 32 bits (config:0-31) on 32-bit PMUs, breaking the default 'cycles' event parsing.
- [Medium] Writing `0` to `PMCNTEN` fails to clear the counter enables during EXT64 detection.
--

--- Patch [1]: [PATCH] perf/arm_cspmu: Support 64-bit programmers' model ---
commit 3d99db99686e79c818d79a1c86a0485ea652a357
Author: Robin Murphy <[email protected]>

perf/arm_cspmu: Support 64-bit programmers' model

This commit adds support for the FEAT_CSPMU_EXT64 extension, making all
non-counter registers 64-bit. It omits generic fixed cycle counter support
for 64-bit events and updates event configuration fields accordingly.

> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index e6292021f653e..f71a0030625e2 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
[ ... ]
> @@ -173,13 +173,28 @@ arm_cspmu_event_attr_is_visible(struct kobject *kobj,
>  	eattr = container_of(attr, typeof(*eattr), attr.attr);
>  
>  	/* Hide cycle event if not supported */
> -	if (!supports_cycle_counter(cspmu) &&
> +	if ((cspmu->has_ext64 || !supports_cycle_counter(cspmu)) &&
>  	    eattr->id == ARM_CSPMU_EVT_CYCLES_DEFAULT)
>  		return 0;
>  
>  	return attr->mode;
>  }
>  
> +ssize_t arm_cspmu_default_format_show(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct perf_pmu_events_attr *fmt = container_of(attr, typeof(*fmt), attr);
> +	struct arm_cspmu *cspmu = to_arm_cspmu(dev_get_drvdata(dev));
> +	u64 field = cspmu->has_ext64 ? U64_MAX : U32_MAX;
> +	DECLARE_BITMAP(bits, 64) = { BITMAP_FROM_U64(field) };

[Severity: High]
Will this truncate the event format definition on 32-bit PMUs?

Prior to this patch, the format was defined as config:0-32. Since the hardware
cycle counter event uses bit 32, using U32_MAX restricts the bitmap to bits
0-31, which might cause tools like perf to reject the default cycles event.

[ ... ]
> @@ -412,6 +437,17 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
>  		DEFAULT_IMPL_OP(event_attr_is_visible),
>  	};
>  
> +	/*
> +	 * With 64-bit events, since our default "cycles" encoding won't work,
> +	 * and the architecture recommends against implementing it anyway, we
> +	 * choose to effectively ignore FEAT_CSPMU_CCNTR, unless a vendor
> +	 * module really wants to provide its own encoding and ops.
> +	 */
> +	if (cspmu->has_ext64) {
> +		cspmu->impl.ops.is_cycle_counter_event = NULL;
> +		cspmu->impl.ops.set_cc_filter = NULL;
> +	}

[Severity: High]
Could this lead to a NULL pointer dereference in vendor drivers?

Looking at pcie_v2_pmu_validate_event() in nvidia_cspmu.c, it makes an
unconditional call to cspmu->impl.ops.is_cycle_counter_event().
If has_ext64 evaluates to true, opening a perf event on that PMU might trigger
a panic.

[ ... ]
> @@ -797,26 +839,33 @@ static void arm_cspmu_event_update(struct perf_event *event)
>  static inline void arm_cspmu_set_event(struct arm_cspmu *cspmu,
>  					struct hw_perf_event *hwc)
>  {
> -	u32 offset = PMEVTYPER + (4 * hwc->idx);
> -
> -	writel(hwc->config, cspmu->base0 + offset);
> +	if (cspmu->has_ext64)
> +		writeq(hwc->config, cspmu->base0 + PMEVTYPER + (8 * hwc->idx));

[Severity: High]
Does this writeq() need to be guarded by a check for has_atomic_dword?

The driver correctly falls back to lo_hi_writeq() for 64-bit counter accesses
on systems lacking atomic 64-bit support. On systems with 32-bit buses that
don't split 64-bit transactions automatically, this writeq() might generate a
bus fault.

> +	else
> +		writel(hwc->config, cspmu->base0 + PMEVTYPER + (4 * hwc->idx));
>  }
>  
>  static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
>  				    const struct perf_event *event)
>  {
> -	u32 filter = event->attr.config1 & ARM_CSPMU_FILTER_MASK;
> -	u32 filter2 = event->attr.config2 & ARM_CSPMU_FILTER_MASK;
> -	u32 offset = 4 * event->hw.idx;
> +	u64 filter = arm_cspmu_filter(event);
> +	u64 filter2 = arm_cspmu_filter2(event);
> +	int n = event->hw.idx;
>  
> -	writel(filter, cspmu->base0 + PMEVFILTR + offset);
> -	writel(filter2, cspmu->base0 + PMEVFILT2R + offset);
> +	if (cspmu->has_ext64) {
> +		writeq(filter, cspmu->base0 + PMEVFILTR + (8 * n));
> +		writeq(filter2, cspmu->base0 + PMEVFILT2R + (8 * n));

[Severity: High]
Similarly, do these configuration register writes also need to respect
has_atomic_dword to prevent external aborts on 32-bit APB buses?

[ ... ]
> @@ -969,6 +1018,30 @@ static int arm_cspmu_init_mmio(struct arm_cspmu *cspmu)
>  		}
>  	}
>  
> +	/*
> +	 * We can infer FEAT_CSPMU_EXT64 from PMCNTEN, or hope that anything
> +	 * that failed to get that right has at least implemented the optional
> +	 * PMDEVARCH correctly...
> +	 *
> +	 * Note that architecturally, has_ext64 *should* imply has_atomic_dword,
> +	 * but enough implementations have ignored that already that we'll just
> +	 * have to still rely on the firmware flag.
> +	 */
> +	writel(~0U, cspmu->base0 + PMCNTENCLR);
> +	writel(~0U, cspmu->base0 + PMCNTEN);
> +	if (readl(cspmu->base0 + PMCNTENCLR)) {
> +		cspmu->has_ext64 = true;
> +		writel(0, cspmu->base0 + PMCNTEN);

[Severity: Medium]
Is writel(0) sufficient to clear the enable bits here?

Since ARM PMU counter enable registers generally function as set registers
where writing 0 has no effect, would it be better to write ~0U to
PMCNTENCLR instead, to avoid leaving the counters unintentionally enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/df1d5eba590bcb12a3bab401ee47e172c51c9515.1785507788.git.robin.murphy@arm.com?part=1