Re: [PATCH v2 07/23] perf: arm_pmuv3: Introduce method to partition the PMU

[Oliver Upton <oliver.upton@linux.dev>]

On Mon, Jun 23, 2025 at 06:26:42PM +0000, Colton Lewis wrote:
> Oliver Upton <oliver.upton@linux.dev> writes:
> 
> > On Fri, Jun 20, 2025 at 10:13:07PM +0000, Colton Lewis wrote:
> > > For PMUv3, the register field MDCR_EL2.HPMN partitiones the PMU
> > > counters into two ranges where counters 0..HPMN-1 are accessible by
> > > EL1 and, if allowed, EL0 while counters HPMN..N are only accessible by
> > > EL2.
> 
> > > Create module parameters partition_pmu and reserved_guest_counters to
> > > reserve a number of counters for the guest. These numbers are set at
> > > boot because the perf subsystem assumes the number of counters will
> > > not change after the PMU is probed.
> 
> > > Introduce the function armv8pmu_partition() to modify the PMU driver's
> > > cntr_mask of available counters to exclude the counters being reserved
> > > for the guest and record reserved_guest_counters as the maximum
> > > allowable value for HPMN.
> 
> > > Due to the difficulty this feature would create for the driver running
> > > at EL1 on the host, partitioning is only allowed in VHE mode. Working
> > > on nVHE mode would require a hypercall for every counter access in the
> > > driver because the counters reserved for the host by HPMN are only
> > > accessible to EL2.
> 
> > > Signed-off-by: Colton Lewis <coltonlewis@google.com>
> > > ---
> > >   arch/arm/include/asm/arm_pmuv3.h   | 10 ++++
> > >   arch/arm64/include/asm/arm_pmuv3.h |  5 ++
> > >   drivers/perf/arm_pmuv3.c           | 95 +++++++++++++++++++++++++++++-
> > >   include/linux/perf/arm_pmu.h       |  1 +
> > >   4 files changed, 109 insertions(+), 2 deletions(-)
> 
> > > diff --git a/arch/arm/include/asm/arm_pmuv3.h
> > > b/arch/arm/include/asm/arm_pmuv3.h
> > > index 2ec0e5e83fc9..9dc43242538c 100644
> > > --- a/arch/arm/include/asm/arm_pmuv3.h
> > > +++ b/arch/arm/include/asm/arm_pmuv3.h
> > > @@ -228,6 +228,11 @@ static inline bool kvm_set_pmuserenr(u64 val)
> 
> > >   static inline void kvm_vcpu_pmu_resync_el0(void) {}
> 
> > > +static inline bool has_vhe(void)
> > > +{
> > > +	return false;
> > > +}
> > > +
> 
> > This has nothing to do with PMUv3, I'm a bit surprised to see you're
> > touching 32-bit ARM. Can you just gate the whole partitioning thing on
> > arm64?
> 
> The PMUv3 driver also has to compile on 32-bit ARM.

Quite aware.

> My first series had the partitioning code in arch/arm64 but you asked me
> to move it to the PMUv3 driver.
> 
> How are you suggesting I square those two requirements?

You should try to structure your predicates in such a way that the
partitioning stuff all resolves to false for 32 bit arm, generally. That
way we can avoid stubbing out silly things like has_vhe() which doesn't
make sense in the context of 32 bit.

> > > +static bool partition_pmu __read_mostly;
> > > +static u8 reserved_guest_counters __read_mostly;
> > > +
> > > +module_param(partition_pmu, bool, 0);
> > > +MODULE_PARM_DESC(partition_pmu,
> > > +		 "Partition the PMU into host and guest VM counters [y/n]");
> > > +
> > > +module_param(reserved_guest_counters, byte, 0);
> > > +MODULE_PARM_DESC(reserved_guest_counters,
> > > +		 "How many counters to reserve for guest VMs [0-$NR_COUNTERS]");
> > > +
> 
> > This is confusing and not what we discussed offline.
> 
> > Please use a single parameter that describes the number of counters used
> > by the *host*. This affects the *host* PMU driver, KVM can discover (and
> > use) the leftovers.
> 
> > If the single module parameter goes unspecified the user did not ask for
> > PMU partitioning.
> 
> I understand what we discussed offline, but I had a dilemma.
> 
> If we do a single module parameter for number of counters used by the
> host, then it defaults to 0 if unset and there is no way to distinguish
> between no partitioning and a request for partitioning reserving 0
> counters to the host which I also thought you requested. Would you be
> happy leaving no way to specify that?

You can make the command line use a signed integer for storage and a
reset value of -1.

-1 would imply default behavior (no partitioning) and a non-negative
value would imply partitioning.

> In any case, I think the usage is more self explainatory if
> partitition=[y/n] is a separate bit.

What would be the user's intent of "partition_pmu=n reserved_guest_counters=$X"?

Thanks,
Oliver