Re: [PATCH] KVM: x86: Advertise AVX10.1 CPUID to userspace

[Tao Su <tao1.su@linux.intel.com>]

On Mon, May 20, 2024 at 07:43:50AM -0700, Sean Christopherson wrote:
> On Mon, May 20, 2024, Tao Su wrote:
> > @@ -1162,6 +1162,22 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
> >  			break;
> >  		}
> >  		break;
> > +	case 0x24: {
> > +		u8 avx10_version;
> > +		u32 vector_support;
> > +
> > +		if (!kvm_cpu_cap_has(X86_FEATURE_AVX10)) {
> > +			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> > +			break;
> > +		}
> > +		avx10_version = min(entry->ebx & 0xff, 1);
> 
> Taking the min() of '1' and anything else is pointless.  Per the spec, the version
> can never be 0.
> 
>   CPUID.(EAX=24H, ECX=00H):EBX[bits 7:0]  Reports the Intel AVX10 Converged Vector ISA version. Integer (≥ 1)
> 
> And it's probably too late, but why on earth is there an AVX10 version number?
> Version numbers are _awful_ for virtualization; see the constant vPMU problems
> that arise from bundling things under a single version number..  Y'all carved out
> room for sub-leafs, i.e. there's a ton of room for "discrete feature bits", so
> why oh why is there a version number?
> 

Per the spec, AVX10 wants to reduce the number of CPUID feature flags required
to be checked, which may simplify application development. Application only
needs to check the version number that can know whether hardware supports an
instruction. There's indeed a sub-leaf for enumerating discrete CPUID feature
bits, but the sub-leaf is only in the rare case.

AVX10.2 (version number == 2) is the initial and fully-featured version of
AVX10, we may need to advertise AVX10.2 in the future. Is keeping min() more
flexible to control the advertised version number? E.g.

    avx10_version = min(entry->ebx & 0xff, 2);

can advertise AVX10.2 to userspace.

> > +		vector_support = entry->ebx & GENMASK(18, 16);
> 
> Please add proper defines somewhere, this this can be something like:
> 
> 		/* EBX[7:0] hold the AVX10 version; KVM supports version '1'. */
> 		entry->eax = 0;
> 		entry->ebx = (entry->ebx & AVX10_VECTOR_SIZES_MASK) | 1;
> 		entry->ecx = 0;
> 		entry->edx = 0;
> 

Yes, its readability will be better.

> Or perhaps we should have feature bits for the vector sizes, because that's really
> what they are.  Mixing feature bits in with a version number makes for painful
> code, but there's nothing KVM can do about that.  With proper features, this then
> becomes something like:
> 
> 		entry->eax = 0;
> 		cpuid_entry_override(entry, CPUID_24_0_EBX);
> 		/* EBX[7:0] hold the AVX10 version; KVM supports version '1'. */
> 		entry->ebx |= 1;
> 		entry->ecx = 0;
> 		entry->edx = 0;

Agree, I will introduce the feature bits for the vector sizes, thanks!