Re: [PATCH v2 06/12] perf: arm_pmu: Remove event index to counter remapping
From: Marc Zyngier <maz@kernel.org>
Date: 2024-06-27 11:05:26
Also in:
kvmarm, linux-perf-users, lkml
On Wed, 26 Jun 2024 23:32:30 +0100, "Rob Herring (Arm)" [off-list ref] wrote:
Xscale and Armv6 PMUs defined the cycle counter at 0 and event counters starting at 1 and had 1:1 event index to counter numbering. On Armv7 and later, this changed the cycle counter to 31 and event counters start at 0. The drivers for Armv7 and PMUv3 kept the old event index numbering and introduced an event index to counter conversion. The conversion uses masking to convert from event index to a counter number. This operation relies on having at most 32 counters so that the cycle counter index 0 can be transformed to counter number 31. Armv9.4 adds support for an additional fixed function counter (instructions) which increases possible counters to more than 32, and the conversion won't work anymore as a simple subtract and mask. The primary reason for the translation (other than history) seems to be to have a contiguous mask of counters 0-N. Keeping that would result in more complicated index to counter conversions. Instead, store a mask of available counters rather than just number of events. That provides more information in addition to the number of events. No (intended) functional changes. Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
[...]
quoted hunk ↗ jump to hunk
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h index b3b34f6670cf..e5d6d204beab 100644 --- a/include/linux/perf/arm_pmu.h +++ b/include/linux/perf/arm_pmu.h@@ -96,7 +96,7 @@ struct arm_pmu { void (*stop)(struct arm_pmu *); void (*reset)(void *); int (*map_event)(struct perf_event *event); - int num_events; + DECLARE_BITMAP(cntr_mask, ARMPMU_MAX_HWEVENTS);
I'm slightly worried by this, as this size is never used, let alone checked by the individual drivers. I can perfectly picture some new (non-architectural) PMU driver having more counters than that, and blindly setting bits outside of the allowed range. One way to make it a bit safer would be to add a helper replacing the various bitmap_set() calls, and enforcing that we never overflow this bitmap. Thanks, M. -- Without deviation from the norm, progress is not possible.