by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH 9/9] perf: arm_pmuv3: Add support for Armv9.4 PMU instruction counter

On Mon, Jun 10, 2024 at 5:55 AM Mark Rutland <[email protected]> wrote:
>
> On Fri, Jun 07, 2024 at 02:31:34PM -0600, Rob Herring (Arm) wrote:
> > Armv9.4/8.9 PMU adds optional support for a fixed instruction counter
> > similar to the fixed cycle counter. Support for the feature is indicated
> > in the ID_AA64DFR1_EL1 register PMICNTR field. The counter is not
> > accessible in AArch32.
> >
> > Existing userspace using direct counter access won't know how to handle
> > the fixed instruction counter, so we have to avoid using the counter
> > when user access is requested.
> >
> > Signed-off-by: Rob Herring (Arm) <[email protected]>
> > ---
> > arch/arm/include/asm/arm_pmuv3.h | 21 +++++++++++++++++++++
> > arch/arm64/include/asm/arm_pmuv3.h | 29 +++++++++++++++++++++++++++++
> > arch/arm64/kvm/pmu.c | 8 ++++++--
> > arch/arm64/tools/sysreg | 25 +++++++++++++++++++++++++
> > drivers/perf/arm_pmuv3.c | 28 ++++++++++++++++++++++++++--
> > include/linux/perf/arm_pmu.h | 8 ++++++--
> > include/linux/perf/arm_pmuv3.h | 4 +++-
> > 7 files changed, 116 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/arm_pmuv3.h b/arch/arm/include/asm/arm_pmuv3.h
> > index ac2cf37b57e3..b836537ddfbf 100644
> > --- a/arch/arm/include/asm/arm_pmuv3.h
> > +++ b/arch/arm/include/asm/arm_pmuv3.h
> > @@ -10,6 +10,7 @@
> > #include <asm/cputype.h>
> >
> > #define ARMV8_PMU_CYCLE_IDX 31
> > +#define ARMV8_PMU_INSTR_IDX 32 /* Not accessible from AArch32 */
>
> As with ARMV8_PMU_CYCLE_IDX, I reckon this should live in
> <linux/perf/arm_pmuv3.h> (with the comment above) so that we don't need
> separate definitions for arm & arm64.

Sure. My only reasoning was to not pull in everything defined in
<linux/perf/arm_pmuv3.h> where it is not needed (KVM).

Of course, the normal pattern here is that linux/foo.h includes
asm/foo.h. We should probably do that here because the arm64 version
of arm_pmuv3.h depends on defines (e.g. PMEVN_SWITCH) in
perf/arm_pmuv3.h, but there is no explicit include. I'll add doing
that into the series.

> [...]
>
> > diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
> > index 231817a379b5..8ab6e09871de 100644
> > --- a/arch/arm64/tools/sysreg
> > +++ b/arch/arm64/tools/sysreg
> > @@ -2029,6 +2029,31 @@ Sysreg FAR_EL1 3 0 6 0 0
> > Field 63:0 ADDR
> > EndSysreg
> >
> > +Sysreg PMICNTR_EL0 3 3 9 4 0
> > +Field 63:0 ICNT
> > +EndSysreg
>
> LGTM per ARM DDI 0487K.a, section D23.5.15, pages 8989 to 8992.
>
> > +
> > +Sysreg PMICFILTR_EL0 3 3 9 6 0
> > +Res0 63:59
> > +Field 58 SYNC
> > +Field 57:56 VS
>
> The 'VS' field doesn't seem to be in the ARM ARM (ARM DDI 0487K.a); is
> that defined in a supplement?

It is in the A-profile Arch Registers doc:
https://developer.arm.com/documentation/ddi0601/2024-03/AArch64-Registers/PMICFILTR-EL0--Performance-Monitors-Instruction-Counter-Filter-Register?lang=en

> > +Res0 55:32
> > +Field 31 P
> > +Field 30 U
> > +Field 29 NSK
> > +Field 28 NSU
> > +Field 27 NSH
> > +Field 26 M
> > +Res0 25
> > +Field 24 SH
> > +Field 23 T
> > +Field 22 RLK
> > +Field 21 RLU
> > +Field 20 RLH
> > +Res0 19:16
> > +Field 15:0 evtCount
> > +EndSysreg
>
> Aside from 'VS', this LGTM per ARM DDI 0487K.a, section D23.5.14, pages
> 8981 to 8988.
>
> One important thing to note is that this doesn't have the threshold
> controls (TC, TE, TH); so if threshold events make sense for instruction
> events, we cannot place those in the dedicated isntruction counter.

Talking to James C, he thinks there could be. Counting cycles which
retired more than 1 instruction was what he came up with.

Also, looks like we need to handle thresholds for the cycle counter as
well. Either reject thresholds for cycles events or don't place cycles
on the fixed cycle counter if thresholds are used. While thresholds
with cycles doesn't make much sense, James prefers the latter as he
did do that for testing.

Rob

2024-06-10 16:54:56

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH 3/9] perf: arm_pmu: Remove event index to counter remapping

On Mon, Jun 10, 2024 at 4:44 AM Mark Rutland <[email protected]> wrote:
>
> On Fri, Jun 07, 2024 at 02:31:28PM -0600, Rob Herring (Arm) 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.

[...]

> > @@ -783,7 +767,7 @@ static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu)
> > struct pmu_hw_events *cpuc = this_cpu_ptr(cpu_pmu->hw_events);
> >
> > /* Clear any unused counters to avoid leaking their contents */
> > - for_each_clear_bit(i, cpuc->used_mask, cpu_pmu->num_events) {
> > + for_each_clear_bit(i, cpuc->used_mask, ARMPMU_MAX_HWEVENTS) {
> > if (i == ARMV8_IDX_CYCLE_COUNTER)
> > write_pmccntr(0);
> > else
>
> IIUC this will now hit all unimplemented counters; e.g. for N counters the body
> will run for counters N..31, and the else case has:
>
> armv8pmu_write_evcntr(i, 0);
>
> ... where the resulting write to PMEVCNTR<n>_EL0 for unimplemented
> counters is CONSTRAINED UNPREDICTABLE and might be UNDEFINED.
>
> We can fix that with for_each_andnot_bit(), e.g.

Good catch. Fixed.

>
> for_each_andnot_bit(i, cpu_pmu->cntr_mask, cpuc->used_mask,
> ARMPMU_MAX_HWEVENTS) {
> if (i == ARMV8_IDX_CYCLE_COUNTER)
> write_pmccntr(0);
> else
> armv8pmu_write_evcntr(i, 0);
> }
>
> [...]
>
> > @@ -905,7 +889,7 @@ static int armv8pmu_get_single_idx(struct pmu_hw_events *cpuc,
> > {
> > int idx;
> >
> > - for (idx = ARMV8_IDX_COUNTER0; idx < cpu_pmu->num_events; idx++) {
> > + for_each_set_bit(idx, cpu_pmu->cntr_mask, 31) {
> > if (!test_and_set_bit(idx, cpuc->used_mask))
> > return idx;
> > }
> > @@ -921,7 +905,9 @@ static int armv8pmu_get_chain_idx(struct pmu_hw_events *cpuc,
> > * Chaining requires two consecutive event counters, where
> > * the lower idx must be even.
> > */
> > - for (idx = ARMV8_IDX_COUNTER0 + 1; idx < cpu_pmu->num_events; idx += 2) {
> > + for_each_set_bit(idx, cpu_pmu->cntr_mask, 31) {
> > + if (!(idx & 0x1))
> > + continue;
> > if (!test_and_set_bit(idx, cpuc->used_mask)) {
> > /* Check if the preceding even counter is available */
> > if (!test_and_set_bit(idx - 1, cpuc->used_mask))
>
> It would be nice to replace those instances of '31' with something
> indicating that this was only covering the generic/programmable
> counters, but I wasn't able to come up with a nice mnemonic for that.
> The best I could think of was:
>
> #define ARMV8_MAX_NR_GENERIC_COUNTERS 31
>
> Maybe it makes sense to define that along with ARMV8_IDX_CYCLE_COUNTER.

I've got nothing better. :) I think there's a few other spots that can use this.

[...]

> > /* Read the nb of CNTx counters supported from PMNC */
> > - *nb_cnt = (armv7_pmnc_read() >> ARMV7_PMNC_N_SHIFT) & ARMV7_PMNC_N_MASK;
> > + nb_cnt = (armv7_pmnc_read() >> ARMV7_PMNC_N_SHIFT) & ARMV7_PMNC_N_MASK;
> > + bitmap_set(cpu_pmu->cntr_mask, 0, nb_cnt);
> >
> > /* Add the CPU cycles counter */
> > - *nb_cnt += 1;
> > + bitmap_set(cpu_pmu->cntr_mask, ARMV7_IDX_CYCLE_COUNTER, 1);
>
> This can be:
>
> set_bit(cpu_pmu->cntr_mask, ARMV7_IDX_CYCLE_COUNTER);
>
> ... and likewise for the PMUv3 version.

Indeed. The documentation in bitmap.h is not clear that greater than 1
unsigned long # of bits works given it says there set_bit() is just
"*addr |= bit". I guess I don't use bitops enough...

Rob