Re: [PATCH v2] KVM: x86/pmu: Disable all vPMU features support on Intel hybrid CPUs

[Sean Christopherson]

On Thu, Feb 02, 2023, Like Xu wrote:
> On 1/2/2023 12:02 am, Sean Christopherson wrote:
> > > diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> > > index 79988dafb15b..6a3995657e1e 100644
> > > --- a/arch/x86/kvm/pmu.h
> > > +++ b/arch/x86/kvm/pmu.h
> > > @@ -166,9 +166,11 @@ static inline void kvm_init_pmu_capability(const struct kvm_pmu_ops *pmu_ops)
> > >   	 /*
> > >   	  * For Intel, only support guest architectural pmu
> > > -	  * on a host with architectural pmu.
> > > +	  * on a non-hybrid host with architectural pmu.
> > >   	  */
> > > -	if ((is_intel && !kvm_pmu_cap.version) || !kvm_pmu_cap.num_counters_gp)
> > > +	if (!kvm_pmu_cap.num_counters_gp ||
> > > +	    (is_intel && (!kvm_pmu_cap.version ||
> > > +			  boot_cpu_has(X86_FEATURE_HYBRID_CPU))))
> > 
> > Why do this here instead of in perf_get_x86_pmu_capability()[*]?  The issue isn't
> > restricted to Intel CPUs, it just so happens that Intel is the only x86 vendor
> > that has shipped hybrid CPUs/PMUs.  Similarly, it's entirely possible to create a
> > hybrid CPU with a fully homogeneous PMU.  IMO KVM should rely on the PMU's is_hybrid()
> > and not the generic X86_FEATURE_HYBRID_CPU flag.
> > 
> > [*] https://lore.kernel.org/all/20230120004051.2043777-1-seanjc@xxxxxxxxxx
> 
> As of today, other x86 vendors do not have hybrid core products in their
> road maps. Before implementing the virtual hybrid vCPU model, there is
> no practical value in talking about homogeneous PMU on hybrid vCPU
> at the present stage.

Why not?  I assume Intel put a fair bit of effort into ensuring feature parity
between P and E cores.  Other than time, money, and effort, I don't see any
reason why Intel can't do the same for the PMU.

> The perf interface only provides host PMU capabilities and the logic for
> choosing to disable (or enable) vPMU based on perf input should be left
> in the KVM part so that subsequent development work can add most code
> to the just KVM, which is very helpful for downstream users to upgrade
> loadable KVM module rather than the entire core kernel.
> 
> My experience interacting with the perf subsystem has taught me that
> perf change required from KVM should be made as small as possible.

I don't disagree, but I don't think that's relevant in this case.  Perf doesn't
provide the necessary bits for KVM to virtualize a hybrid PMU, so unless KVM is
somehow able to get away with enumerating a very stripped down vPMU, additional
modifications to perf_get_x86_pmu_capability() will be required.

What I care more about though is this ugliness in perf_get_x86_pmu_capability():

	/*
	 * KVM doesn't support the hybrid PMU yet.
	 * Return the common value in global x86_pmu,
	 * which available for all cores.
	 */
	cap->num_counters_gp	= x86_pmu.num_counters;

I really don't want to leave that comment lying around as it's flat out wrong in
that it obviously doesn't address the other differences beyond the number of
counters.  And since there are dependencies on perf, my preference is to disable
PMU enumeration in perf specifically so that whoever takes on vPMU enabling is
forced to consider the perf side of things, and get buy in from the perf folks.