Re: [PATCH] KVM: arm64: Fix smp_processor_id() call in preemptible context

[Sean Christopherson <seanjc@google.com>]

On Tue, Jun 06, 2023, Oliver Upton wrote:
> On Tue, Jun 06, 2023 at 07:29:16AM -0700, Sean Christopherson wrote:
> > On Tue, Jun 06, 2023, Oliver Upton wrote:
> > > The call from a preemptible context is intentional, so this really
> > > should just be raw_smp_processor_id(). Do you mind if we fix it with the
> > > following?
> > 
> > ...
> > 
> > > Nonetheless, there's no functional requirement for disabling preemption,
> > > as the cpu # is only used to walk the arm_pmus list. Fix it by using
> > > raw_smp_processor_id() instead.
> > 
> > As a partial outsider, that needs an explanation, and the code could really use a
> > comment.  I assume KVM's ABI is that it's userspace's responsibility to ensure that
> > the CPU(s) used for KVM_RUN is compatible with the CPU used for KVM_ARM_VCPU_PMU_V3_CTRL,
> > but neither the original changelog nor the above state that, nor does anything
> > explain what happens if userspace doesn't uphold its side of things.
> 
> See commit 40e54cad4540 ("KVM: arm64: Document default vPMU behavior on
> heterogeneous systems"), which documents the subtlety of vCPU scheduling
> with the 'old' ABI at the callsite of this function. I don't want to
> bleed any details of this crap into user documentation, since the entire
> scheme is irretrievably broken.

I wasn't suggesting adding anything to user documentation, I was suggesting/asking
that the changelog explain the assertion "there's no functional requirement for
disabling preemption".

> See Documentation/virt/kvm/api/devices/vcpu.rst 1.4 for the 'new' ABI
> where userspace explicitly selects a vPMU instance.
> 
> > That stuff might be covered in documentation somewhere, but for someone
> > just looking at git blame, this is all very magical.
> 
> Personally, I find any other fix that involves disabling preemption to be
> quite a lot more 'magical', as there isn't any percpu data we're working
> with in the loop.

Heh, and?  I wasn't suggesting you take Sebastian's fix, nor was I arguing that
disabling preemption is any more or less magical.  I was simply calling out that
for folks that aren't already familiar with the code, it's not obvious why using
an unstable CPU is functionally correct.

Poking around more, it looks like the answer is that kvm_arch_vcpu_load() will
mark the vCPU as being loaded on an unsupported pCPU, which will prevent running
on the target pCPU, and thus presumably prevent creating new perf events on the
incompatible pCPU.

Though following the breadcrumbs from the commit you referenced above, the set of
"supported" CPUs at this point is all possible CPUs in the system.  So unless I'm
missing something, testing that the current, unstable CPU is a "supported" CPU is
unnecessary because running on an impossible CPU is, um, impossible.

I.e. can't you just do?

---
 arch/arm64/kvm/pmu-emul.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 491ca7eb2a4c..b899d580ff73 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -692,29 +692,6 @@ void kvm_host_pmu_init(struct arm_pmu *pmu)
 	mutex_unlock(&arm_pmus_lock);
 }
 
-static struct arm_pmu *kvm_pmu_probe_armpmu(void)
-{
-	struct arm_pmu *tmp, *pmu = NULL;
-	struct arm_pmu_entry *entry;
-	int cpu;
-
-	mutex_lock(&arm_pmus_lock);
-
-	cpu = smp_processor_id();
-	list_for_each_entry(entry, &arm_pmus, entry) {
-		tmp = entry->arm_pmu;
-
-		if (cpumask_test_cpu(cpu, &tmp->supported_cpus)) {
-			pmu = tmp;
-			break;
-		}
-	}
-
-	mutex_unlock(&arm_pmus_lock);
-
-	return pmu;
-}
-
 u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
 {
 	unsigned long *bmap = vcpu->kvm->arch.pmu_filter;
@@ -900,8 +877,14 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		 * upholds the preexisting behavior on heterogeneous systems
 		 * where vCPUs can be scheduled on any core but the guest
 		 * counters could stop working.
+		 *
+		 * In short, the set of "supported" CPUS for the default PMU is
+		 * all possible CPUs in the system, simply grab the first PMU.
 		 */
-		kvm->arch.arm_pmu = kvm_pmu_probe_armpmu();
+		mutex_lock(&arm_pmus_lock);
+		kvm->arch.arm_pmu = list_first_entry_or_null(&arm_pmus);
+		mutex_unlock(&arm_pmus_lock);
+
 		if (!kvm->arch.arm_pmu)
 			return -ENODEV;
 	}

base-commit: 40e54cad454076172cc3e2bca60aa924650a3e4b
-- 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel