Re: [PATCH v2 06/12] perf: arm_pmu: Remove event index to counter remapping

On Mon, Jul 01, 2024 at 02:52:16PM +0100, Will Deacon wrote:

On Thu, Jun 27, 2024 at 12:05:23PM +0100, Marc Zyngier wrote:

quoted

On Wed, 26 Jun 2024 23:32:30 +0100,
"Rob Herring (Arm)" [off-list ref] wrote:

quoted

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

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.

I tend to agree.

It's the same size as other bitmaps and arrays in struct arm_pmu, e.g.
the first two fields:

| struct pmu_hw_events {
|         /*  
|          * The events that are active on the PMU for the given index.
|          */
|         struct perf_event       *events[ARMPMU_MAX_HWEVENTS];
| 
|         /*  
|          * A 1 bit for an index indicates that the counter is being used for
|          * an event. A 0 means that the counter can be used.
|          */
|         DECLARE_BITMAP(used_mask, ARMPMU_MAX_HWEVENTS);

... so IMO it's fine as-is, since anything not bound by
ARMPMU_MAX_HWEVENTS would already be wrong today.

quoted

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.

Or perhaps wd could leave the 'num_events' field intact and allocate the
new bitmap dynamically?

I don't think we should allocate the bitmap dynamically, since then we'd
have to do likewise for all the other fields sized by
ARMPMU_MAX_HWEVENTS.

I'm not averse to a check when setting bits in the new cntr_mask (which
I guess would WARN() and not set the bit), but as above I think it's
fine as-is.

Mark.