Date:   Wed, 25 Jan 2023 00:16:14 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Like Xu <like.xu.linux@gmail.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Sandipan Das <sandipan.das@amd.com>
Subject: Re: [PATCH v3 7/8] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg
 leaf 0x80000022
Message-ID: <Y9B0zgliB9BMPK1l@google.com>
References: <20221111102645.82001-1-likexu@tencent.com>
 <20221111102645.82001-8-likexu@tencent.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221111102645.82001-8-likexu@tencent.com>
Precedence: bulk

On Fri, Nov 11, 2022, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> CPUID leaf 0x80000022 i.e. ExtPerfMonAndDbg advertises some new
> performance monitoring features for AMD processors.
> 
> Bit 0 of EAX indicates support for Performance Monitoring Version 2
> (PerfMonV2) features. If found to be set during PMU initialization,
> the EBX bits of the same CPUID function can be used to determine
> the number of available PMCs for different PMU types.
> 
> Expose the relevant bits via KVM_GET_SUPPORTED_CPUID so that
> guests can make use of the PerfMonV2 features.
> 
> Co-developed-by: Sandipan Das <sandipan.das@amd.com>
> Signed-off-by: Sandipan Das <sandipan.das@amd.com>
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
>  arch/x86/kvm/cpuid.c   | 30 +++++++++++++++++++++++++++++-
>  arch/x86/kvm/svm/svm.c | 11 ++++++++---
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 6b5912578edd..df551fa66ccc 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1113,7 +1113,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  		entry->edx = 0;
>  		break;
>  	case 0x80000000:
> -		entry->eax = min(entry->eax, 0x80000021);
> +		entry->eax = min(entry->eax, 0x80000022);
>  		/*
>  		 * Serializing LFENCE is reported in a multitude of ways, and
>  		 * NullSegClearsBase is not reported in CPUID on Zen2; help
> @@ -1229,6 +1229,34 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  		if (!static_cpu_has_bug(X86_BUG_NULL_SEG))
>  			entry->eax |= BIT(6);
>  		break;
> +	/* AMD Extended Performance Monitoring and Debug */
> +	case 0x80000022: {
> +		union cpuid_0x80000022_ebx ebx;
> +
> +		entry->ecx = entry->edx = 0;
> +		if (!enable_pmu || !kvm_cpu_cap_has(X86_FEATURE_AMD_PMU_V2)) {
> +			entry->eax = entry->ebx;
> +			break;
> +		}
> +
> +		cpuid_entry_override(entry, CPUID_8000_0022_EAX);
> +
> +		if (kvm_cpu_cap_has(X86_FEATURE_AMD_PMU_V2))
> +			ebx.split.num_core_pmc = min(kvm_pmu_cap.num_counters_gp,
> +						     KVM_AMD_PMC_MAX_GENERIC);

Similar to the previous patch, sanitizing kvm_pmu_cap.num_counters_gp should be
handled during PMU initialization.

> +
> +		if (kvm_cpu_cap_has(X86_FEATURE_PERFCTR_CORE))
> +			ebx.split.num_core_pmc = max_t(unsigned int,
> +						       ebx.split.num_core_pmc,
> +						       AMD64_NUM_COUNTERS_CORE);
> +		else
> +			ebx.split.num_core_pmc = max_t(unsigned int,
> +						       ebx.split.num_core_pmc,
> +						       AMD64_NUM_COUNTERS);

Again like the previous patch, I would expect this to be an if-elif-else sequence.

> +
> +		entry->ebx = ebx.full;
> +		break;
> +	}
>  	/*Add support for Centaur's CPUID instruction*/
>  	case 0xC0000000:
>  		/*Just support up to 0xC0000004 now*/
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 527f18d8cc44..127983ab8307 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4914,9 +4914,14 @@ static __init void svm_set_cpu_caps(void)
>  	    boot_cpu_has(X86_FEATURE_AMD_SSBD))
>  		kvm_cpu_cap_set(X86_FEATURE_VIRT_SSBD);
>  
> -	/* AMD PMU PERFCTR_CORE CPUID */
> -	if (enable_pmu && boot_cpu_has(X86_FEATURE_PERFCTR_CORE))
> -		kvm_cpu_cap_set(X86_FEATURE_PERFCTR_CORE);

Another existing wart.  This existing code can be:

	if (enable_pmu)
		kvm_cpu_cap_check_and_set(X86_FEATURE_PERFCTR_CORE)

Can you add a patch to do that minor cleanup?  Then this patch can yield:

	if (enable_pmu) {
		kvm_cpu_cap_check_and_set(X86_FEATURE_PERFCTR_CORE);
		kvm_cpu_cap_check_and_set(X86_FEATURE_PERFMON_V2);
	}

KVM's reverse-CPUID magic should Just Work for PERFMON_V2 even though it's scattered.
		

> +	if (enable_pmu) {
> +		/* AMD PMU PERFCTR_CORE CPUID */
> +		if (boot_cpu_has(X86_FEATURE_PERFCTR_CORE))
> +			kvm_cpu_cap_set(X86_FEATURE_PERFCTR_CORE);
> +		/* KVM only support AMD PerfMon V2 */
> +		if (kvm_pmu_cap.version > 1)
> +			kvm_cpu_cap_set(X86_FEATURE_AMD_PMU_V2);
> +	}
>  
>  	/* CPUID 0x8000001F (SME/SEV features) */
>  	sev_set_cpu_caps();
> -- 
> 2.38.1
>