[PATCH 0/2] KVM: arm64: Fix for probing default PMU

[PATCH 0/2] KVM: arm64: Fix for probing default PMU

[Oliver Upton]

Follow-up to the discussion in [1].

KVM uses a junk perf event to probe for a default PMU instance to use
for the guest, which depends on perf finding the right PMU instance
to use for the event. Looks like perf is going to rely strictly on
the specified PMU type, breaking the entire scheme.

KVM now maintains a list of valid PMU instances that can be selected by
userspace, we can simply walk that to probe for a default PMU.

Applies to 6.4-rc3 and tested on -next.

[1] https://lore.kernel.org/all/20230524214133.GA2359762@dev-arch.thelio-3990X/

Oliver Upton (2):
  KVM: arm64: Iterate arm_pmus list to probe for default PMU
  KVM: arm64: Document default vPMU behavior on heterogeneous systems

 arch/arm64/kvm/pmu-emul.c | 58 ++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 35 deletions(-)


base-commit: 44c026a73be8038f03dbdeef028b642880cf1511
-- 
2.41.0.rc0.172.g3f132b7071-goog

[PATCH 1/2] KVM: arm64: Iterate arm_pmus list to probe for default PMU

[Oliver Upton]

To date KVM has relied on using a perf event to probe the core PMU at
the time of vPMU initialization. Behind the scenes perf_event_init()
would iteratively walk the PMUs of the system and return the PMU that
could handle the event. However, an upcoming change in perf core will
drop the iterative walk, thereby breaking the fragile dance we do on the
KVM side.

Avoid the problem altogether by iterating over the list of supported
PMUs maintained in KVM, returning the core PMU that matches the CPU
we were called on.

Tested-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/pmu-emul.c | 46 ++++++++++-----------------------------
 1 file changed, 12 insertions(+), 34 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 45727d50d18d..5deddc49e745 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -694,45 +694,23 @@ void kvm_host_pmu_init(struct arm_pmu *pmu)
 
 static struct arm_pmu *kvm_pmu_probe_armpmu(void)
 {
-	struct perf_event_attr attr = { };
-	struct perf_event *event;
-	struct arm_pmu *pmu = NULL;
-
-	/*
-	 * Create a dummy event that only counts user cycles. As we'll never
-	 * leave this function with the event being live, it will never
-	 * count anything. But it allows us to probe some of the PMU
-	 * details. Yes, this is terrible.
-	 */
-	attr.type = PERF_TYPE_RAW;
-	attr.size = sizeof(attr);
-	attr.pinned = 1;
-	attr.disabled = 0;
-	attr.exclude_user = 0;
-	attr.exclude_kernel = 1;
-	attr.exclude_hv = 1;
-	attr.exclude_host = 1;
-	attr.config = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
-	attr.sample_period = GENMASK(63, 0);
+	struct arm_pmu *tmp, *pmu = NULL;
+	struct arm_pmu_entry *entry;
+	int cpu;
 
-	event = perf_event_create_kernel_counter(&attr, -1, current,
-						 kvm_pmu_perf_overflow, &attr);
+	mutex_lock(&arm_pmus_lock);
 
-	if (IS_ERR(event)) {
-		pr_err_once("kvm: pmu event creation failed %ld\n",
-			    PTR_ERR(event));
-		return NULL;
-	}
+	cpu = smp_processor_id();
+	list_for_each_entry(entry, &arm_pmus, entry) {
+		tmp = entry->arm_pmu;
 
-	if (event->pmu) {
-		pmu = to_arm_pmu(event->pmu);
-		if (pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_NI ||
-		    pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
-			pmu = NULL;
+		if (cpumask_test_cpu(cpu, &tmp->supported_cpus)) {
+			pmu = tmp;
+			break;
+		}
 	}
 
-	perf_event_disable(event);
-	perf_event_release_kernel(event);
+	mutex_unlock(&arm_pmus_lock);
 
 	return pmu;
 }
-- 
2.41.0.rc0.172.g3f132b7071-goog

[PATCH 2/2] KVM: arm64: Document default vPMU behavior on heterogeneous systems

[Oliver Upton]

KVM maintains a mask of supported CPUs when a vPMU type is explicitly
selected by userspace and is used to reject any attempt to run the vCPU
on an unsupported CPU. This is great, but we're still beholden to the
default behavior where vCPUs can be scheduled anywhere and guest
counters may silently stop working.

Avoid confusing the next poor sod to look at this code and document the
intended behavior.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/pmu-emul.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 5deddc49e745..491ca7eb2a4c 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -890,7 +890,17 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		return -EBUSY;
 
 	if (!kvm->arch.arm_pmu) {
-		/* No PMU set, get the default one */
+		/*
+		 * No PMU set, get the default one.
+		 *
+		 * The observant among you will notice that the supported_cpus
+		 * mask does not get updated for the default PMU even though it
+		 * is quite possible the selected instance supports only a
+		 * subset of cores in the system. This is intentional, and
+		 * upholds the preexisting behavior on heterogeneous systems
+		 * where vCPUs can be scheduled on any core but the guest
+		 * counters could stop working.
+		 */
 		kvm->arch.arm_pmu = kvm_pmu_probe_armpmu();
 		if (!kvm->arch.arm_pmu)
 			return -ENODEV;
-- 
2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH 1/2] KVM: arm64: Iterate arm_pmus list to probe for default PMU

[Oliver Upton]

On Thu, May 25, 2023 at 09:27:21PM +0000, Oliver Upton wrote:
> To date KVM has relied on using a perf event to probe the core PMU at
> the time of vPMU initialization. Behind the scenes perf_event_init()

Oops:

s/perf_event_init/perf_init_event/

> would iteratively walk the PMUs of the system and return the PMU that
> could handle the event. However, an upcoming change in perf core will
> drop the iterative walk, thereby breaking the fragile dance we do on the
> KVM side.
> 
> Avoid the problem altogether by iterating over the list of supported
> PMUs maintained in KVM, returning the core PMU that matches the CPU
> we were called on.
> 
> Tested-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/pmu-emul.c | 46 ++++++++++-----------------------------
>  1 file changed, 12 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 45727d50d18d..5deddc49e745 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -694,45 +694,23 @@ void kvm_host_pmu_init(struct arm_pmu *pmu)
>  
>  static struct arm_pmu *kvm_pmu_probe_armpmu(void)
>  {
> -	struct perf_event_attr attr = { };
> -	struct perf_event *event;
> -	struct arm_pmu *pmu = NULL;
> -
> -	/*
> -	 * Create a dummy event that only counts user cycles. As we'll never
> -	 * leave this function with the event being live, it will never
> -	 * count anything. But it allows us to probe some of the PMU
> -	 * details. Yes, this is terrible.
> -	 */
> -	attr.type = PERF_TYPE_RAW;
> -	attr.size = sizeof(attr);
> -	attr.pinned = 1;
> -	attr.disabled = 0;
> -	attr.exclude_user = 0;
> -	attr.exclude_kernel = 1;
> -	attr.exclude_hv = 1;
> -	attr.exclude_host = 1;
> -	attr.config = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
> -	attr.sample_period = GENMASK(63, 0);
> +	struct arm_pmu *tmp, *pmu = NULL;
> +	struct arm_pmu_entry *entry;
> +	int cpu;
>  
> -	event = perf_event_create_kernel_counter(&attr, -1, current,
> -						 kvm_pmu_perf_overflow, &attr);
> +	mutex_lock(&arm_pmus_lock);
>  
> -	if (IS_ERR(event)) {
> -		pr_err_once("kvm: pmu event creation failed %ld\n",
> -			    PTR_ERR(event));
> -		return NULL;
> -	}
> +	cpu = smp_processor_id();
> +	list_for_each_entry(entry, &arm_pmus, entry) {
> +		tmp = entry->arm_pmu;
>  
> -	if (event->pmu) {
> -		pmu = to_arm_pmu(event->pmu);
> -		if (pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_NI ||
> -		    pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
> -			pmu = NULL;
> +		if (cpumask_test_cpu(cpu, &tmp->supported_cpus)) {
> +			pmu = tmp;
> +			break;
> +		}
>  	}
>  
> -	perf_event_disable(event);
> -	perf_event_release_kernel(event);
> +	mutex_unlock(&arm_pmus_lock);
>  
>  	return pmu;
>  }
> -- 
> 2.41.0.rc0.172.g3f132b7071-goog
> 

-- 
Thanks,
Oliver

Re: [PATCH 0/2] KVM: arm64: Fix for probing default PMU

[Marc Zyngier]

On Thu, 25 May 2023 21:27:20 +0000, Oliver Upton wrote:
> Follow-up to the discussion in [1].
> 
> KVM uses a junk perf event to probe for a default PMU instance to use
> for the guest, which depends on perf finding the right PMU instance
> to use for the event. Looks like perf is going to rely strictly on
> the specified PMU type, breaking the entire scheme.
> 
> [...]

Applied to fixes, thanks!

[1/2] KVM: arm64: Iterate arm_pmus list to probe for default PMU
      commit: 1c913a1c35aa61cf280173b2bcc133c3953c38fc
[2/2] KVM: arm64: Document default vPMU behavior on heterogeneous systems
      commit: 40e54cad454076172cc3e2bca60aa924650a3e4b

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.