Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Oct 2022 22:37:17 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Like Xu <like.xu.linux@gmail.com>, g@google.com
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Jim Mattson <jmattson@google.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Sandipan Das <sandipan.das@amd.com>
Subject: Re: [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg
 leaf 0x80000022
Message-ID: <Y1sIHXX3HEJEXJm+@google.com>
References: <20220919093453.71737-1-likexu@tencent.com>
 <20220919093453.71737-4-likexu@tencent.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220919093453.71737-4-likexu@tencent.com>
Precedence: bulk

On Mon, Sep 19, 2022, Like Xu wrote:
> From: Sandipan Das <sandipan.das@amd.com>
> 
> From: Sandipan Das <sandipan.das@amd.com>

Duplicate "From:"s.

> CPUID leaf 0x80000022 i.e. ExtPerfMonAndDbg advertises some
> new performance monitoring features for AMD processors.

Wrap changelogs closer to ~75 chars.

> 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: Like Xu <likexu@tencent.com>
> Signed-off-by: Like Xu <likexu@tencent.com>
> Signed-off-by: Sandipan Das <sandipan.das@amd.com>
> ---
>  arch/x86/include/asm/perf_event.h |  8 ++++++++
>  arch/x86/kvm/cpuid.c              | 32 ++++++++++++++++++++++++++++++-
>  2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
> index f6fc8dd51ef4..c848f504e467 100644
> --- a/arch/x86/include/asm/perf_event.h
> +++ b/arch/x86/include/asm/perf_event.h
> @@ -214,6 +214,14 @@ union cpuid_0x80000022_ebx {
>  	unsigned int		full;
>  };
>  
> +union cpuid_0x80000022_eax {
> +	struct {
> +		/* Performance Monitoring Version 2 Supported */
> +		unsigned int	perfmon_v2:1;
> +	} split;
> +	unsigned int		full;
> +};

I'm not a fan of perf's unions, but I at least understand the value added for
CPUID entries that are a bunch of multi-bit values.  However, this leaf appears
to be a pure features leaf.  In which case a union just makes life painful.

Please add a CPUID_8000_0022_EAX kvm_only_cpuid_leafs entry (details in link[*]
below) so that KVM can write sane code like

	guest_cpuid_has(X86_FEATURE_AMD_PMU_V2)

and cpuid_entry_override() instead of manually filling in information.

where appropriate.

[*] https://lore.kernel.org/all/Y1AQX3RfM+awULlE@google.com

>  struct x86_pmu_capability {
>  	int		version;
>  	int		num_counters_gp;
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 75dcf7a72605..34ba845c91b7 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1094,7 +1094,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
> @@ -1203,6 +1203,36 @@ 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_eax eax;
> +		union cpuid_0x80000022_ebx ebx;
> +
> +		entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> +		if (!enable_pmu)

Shouldn't

	case 0xa: { /* Architectural Performance Monitoring */

also check enable_pmu instead of X86_FEATURE_ARCH_PERFMON?

> +			break;
> +
> +		if (kvm_pmu_cap.version > 1) {
> +			/* AMD PerfMon is only supported up to V2 in the KVM. */
> +			eax.split.perfmon_v2 = 1;

With a proper CPUID_8000_0022_EAX, this becomes:

		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);

		...

		entry->ebx = ebx.full;