Message-ID: <gsntpleu9uvx.fsf@coltonlewis-kvm.c.googlers.com>
Date: Mon, 23 Jun 2025 18:26:42 +0000
From: Colton Lewis <coltonlewis@...gle.com>
To: Oliver Upton <oliver.upton@...ux.dev>
Cc: kvm@...r.kernel.org, pbonzini@...hat.com, corbet@....net, 
	linux@...linux.org.uk, catalin.marinas@....com, will@...nel.org, 
	maz@...nel.org, joey.gouly@....com, suzuki.poulose@....com, 
	yuzenghui@...wei.com, mark.rutland@....com, shuah@...nel.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev, 
	linux-perf-users@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v2 07/23] perf: arm_pmuv3: Introduce method to partition
 the PMU

Oliver Upton <oliver.upton@...ux.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@...gle.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.

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?

>> +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?

In any case, I think the usage is more self explainatory if
partitition=[y/n] is a separate bit. The other parameter for guest
reservation is then based on a consideration of what an unset parameter
should mean and I decided it's a more sane default if partition=y
[other-param]=0/unset gives 0 counters to the guest.

It does affect the host, but by default the host owns everything. The
only people who will be tweaking these parameters are going to be
concerned with how many counters the guest gets and I think the
parameters should reflect that intent.

>> +/**
>> + * armv8pmu_reservation_is_valid() - Determine if reservation is allowed
>> + * @guest_counters: Number of host counters to reserve
>> + *
>> + * Determine if the number of host counters in the argument is
>> + * allowed. It is allowed if it will produce a valid value for
>> + * register field MDCR_EL2.HPMN.
>> + *
>> + * Return: True if reservation allowed, false otherwise
>> + */
>> +static bool armv8pmu_reservation_is_valid(u8 guest_counters)
>> +{
>> +	return guest_counters <= armv8pmu_pmcr_n_read();
>> +}
>> +
>> +/**
>> + * armv8pmu_partition_supported() - Determine if partitioning is  
>> possible
>> + *
>> + * Partitioning is only supported in VHE mode (with PMUv3, assumed
>> + * since we are in the PMUv3 driver)
>> + *
>> + * Return: True if partitioning is possible, false otherwise
>> + */
>> +static bool armv8pmu_partition_supported(void)
>> +{
>> +	return has_vhe();
>> +}
>> +
>> +/**
>> + * armv8pmu_partition() - Partition the PMU
>> + * @pmu: Pointer to pmu being partitioned
>> + * @guest_counters: Number of host counters to reserve
>> + *
>> + * Partition the given PMU by taking a number of host counters to
>> + * reserve and, if it is a valid reservation, recording the
>> + * corresponding HPMN value in the hpmn field of the PMU and clearing
>> + * the guest-reserved counters from the counter mask.
>> + *
>> + * Passing 0 for @guest_counters has the effect of disabling  
>> partitioning.
>> + *
>> + * Return: 0 on success, -ERROR otherwise
>> + */
>> +static int armv8pmu_partition(struct arm_pmu *pmu, u8 guest_counters)
>> +{
>> +	u8 nr_counters;
>> +	u8 hpmn;
>> +
>> +	if (!armv8pmu_reservation_is_valid(guest_counters))
>> +		return -EINVAL;
>> +
>> +	nr_counters = armv8pmu_pmcr_n_read();
>> +	hpmn = guest_counters;
>> +
>> +	pmu->hpmn_max = hpmn;

> I'm not sure the host driver needs this for anything, KVM just needs to
> know what's potentially in use by the host.

>> +	/* Inform host driver of available counters */

> ... said the driver to itself :)

I can delete that comment now :)

Powered by blists - more mailing lists