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

[Like Xu <like.xu.linux@gmail.com>]

On 1/2/2023 12:02 am, Sean Christopherson wrote:
> On Tue, Jan 31, 2023, Like Xu wrote:
>> From: Like Xu <likexu@tencent.com>
>>
>> Disable KVM support for virtualizing PMUs on hosts with hybrid PMUs until
>> KVM gains a sane way to enumeration the hybrid vPMU to userspace and/or
>> gains a mechanism to let userspace opt-in to the dangers of exposing a
>> hybrid vPMU to KVM guests.
>>
>> Virtualizing a hybrid PMU, or at least part of a hybrid PMU, is possible,
>> but it requires userspace to pin vCPUs to pCPUs to prevent migrating a
>> vCPU between a big core and a little core, requires the VMM to accurately
>> enumerate the topology to the guest (if exposing a hybrid CPU to the
>> guest), and also requires the VMM to accurately enumerate the vPMU
>> capabilities to the guest.
>>
>> The last point is especially problematic, as KVM doesn't control which
>> pCPU it runs on when enumerating KVM's vPMU capabilities to userspace.
>> For now, simply disable vPMU support on hybrid CPUs to avoid inducing
>> seemingly random #GPs in guests.
>>
>> Reported-by: Jianfeng Gao <jianfeng.gao@intel.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Like Xu <likexu@tencent.com>
>> ---
>> v1: https://lore.kernel.org/all/20230120004051.2043777-1-seanjc@google.com/
>>   arch/x86/kvm/pmu.h | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> 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@google.com

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.

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 assume that Peterz's timely "Acked-by" also implies his preference.

Thanks,
Like Xu