Re: [PATCH v4 12/12] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022

[Like Xu]

On 7/4/2023 9:50 am, Sean Christopherson wrote:
> On Tue, Feb 14, 2023, Like Xu wrote:
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index f4a4691b4f4e..2472fa8746c2 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -4916,6 +4916,12 @@ static __init void svm_set_cpu_caps(void)
> >   		} else {
> >   			/* AMD PMU PERFCTR_CORE CPUID */
> >   			kvm_cpu_cap_check_and_set(X86_FEATURE_PERFCTR_CORE);
> > +			/*
> > +			 * KVM only supports AMD PerfMon V2, even if it supports V3+.
>
> Ha!  A perfect example of why I strongly prefer that changelogs and comments avoid
> pronouns.  The above "it" reads as:
>
> 			 * KVM only supports AMD PerfMon V2, even if KVM supports V3+.
>
> which is clearly nonsensical.

I get your point. Thanks.

> > +			 * For PerfMon V3+, it's unsafe to expect V2 bit is set or cleared.
>
> If it's unsafe to assume anything v3+ implying v2 support, then it's definitely
> not safe to assume that KVM can blindly set v2 without future changes.  I don't
> see any reason not to do
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index bd324962bb7e..1192f605ad47 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -756,6 +756,10 @@ void kvm_set_cpu_caps(void)
>                  F(NULL_SEL_CLR_BASE) | F(AUTOIBRS) | 0 /* PrefetchCtlMsr */
>          );
>   
> +       kvm_cpu_cap_mask(CPUID_8000_0022_EAX,
> +               F(PERFMON_V2)
> +       );
> +
>          /*
>           * Synthesize "LFENCE is serializing" into the AMD-defined entry in
>           * KVM's supported CPUID if the feature is reported as supported by the
>
>
> and then this code can be:
>
> 			if (kvm_pmu_cap.version != 2)
> 				kvm_cpu_cap_clear(X86_FEATURE_PERFMON_V2);
>
> Ah, but presumably the
>
> 		if (kvm_pmu_cap.num_counters_gp < AMD64_NUM_COUNTERS_CORE)
>
> path also needs to clear PERFMON_V2.  I think I'd still vote to grab host CPUID
> and clear here (instead of setting).

Looks good to me.

> What is the relationship between PERFCTR_CORE and PERFMON_V2?  E.g. if v2 depends
> on having PERFCTR_CORE, then we can do:

Yes, the PERFCTR_CORE bit will always be set if the v2 bit is set.

> 	if (enable_pmu) {
> 		if (kvm_pmu_cap.num_counters_gp < AMD64_NUM_COUNTERS_CORE)
> 			kvm_pmu_cap.num_counters_gp = AMD64_NUM_COUNTERS;
> 		else
> 			kvm_cpu_cap_check_and_set(X86_FEATURE_PERFCTR_CORE);
>
> 		if (kvm_pmu_cap.version != 2 ||
> 		    !kvm_cpu_cap_has(X86_FEATURE_PERFCTR_CORE))
> 			kvm_cpu_cap_clear(X86_FEATURE_PERFMON_V2);