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

[Like Xu]

On 28/10/2022 6:37 am, Sean Christopherson wrote:
> On Mon, Sep 19, 2022, Like Xu wrote:
> > From: Sandipan Das <sandipan.das@xxxxxxx>
> >
> > From: Sandipan Das <sandipan.das@xxxxxxx>
>
> 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@xxxxxxxxxxx>
> > Signed-off-by: Like Xu <likexu@xxxxxxxxxxx>
> > Signed-off-by: Sandipan Das <sandipan.das@xxxxxxx>
> > ---
> >   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@xxxxxxxxxx

When someone is selling syntactic sugar in the kernel space, extra attention
needs to be paid to runtime performance (union) and memory footprint 
(reverse_cpuid).

Applied for this case, while the cpuid_0x80000022_eax will be used again
in the perf core since the other new AMD PMU features are pacing at the door.

> >   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?

Applied as a separate patch, though KVM will have zero-padded kvm_pmu_cap to do
subsequent assignments when !enable_pmu but that doesn't hurt.

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

Then in this code block, we will have:

	/* AMD PerfMon is only supported up to V2 in the KVM. */
	entry->eax |= BIT(0);

to cover AMD Perfmon V3+, any better move ?

> 		entry->ebx = ebx.full;