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

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

On Tue, Jun 06, 2023 at 03:46:09PM +0000, Sean Christopherson wrote:
> 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".

Let's continue this on Marc's reply (don't want to spread context over
multiple threads).

> 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.

That's only the case with the 'new' ABI, where userspace has explicitly
selected a PMU instance. The answer for the 'old' interface is here:

	/*
	 * 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();


> 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.

Careful -- arm_pmu->supported_cpus is different from kvm->arch.supported_cpus.
The former describes a PMU instance in the system and the latter
enforces our scheduling requirements when userspace _explicitly_ sets a
PMU type, i.e. KVM_ARM_VCPU_PMU_V3_SET_PMU.

So it is probable that some of the PMUs in the arm_pmus list *do not*
support the calling CPU.

> I.e. can't you just do?

No, this would break the 'old' ABI, but if we decide to deliberately
break it I prefer your suggestion over using CPU0.

-- 
Thanks,
Oliver