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

[Sean Christopherson <seanjc@google.com>]

On Tue, May 21, 2024, Tao Su wrote:
> 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.

I get that, but it royally hoses virtualization.  Bundling multiple features
under a single flag is annoying, e.g. it makes it impossible to selectively
advertise features, but I can appreciate that there are situations where having
one feature but not another is nonsensical.

Incrementing version numbers are a whole other level of bad though.  E.g. if
AVX10.2 has a feature that shouldn't be enumerated to guests for whatever reason,
then KVM can't enumerate any "later" features either, because the only way to hide
the problematic AVX10.2 feature is to set the version to AVX10.1 or lower.

FWIW, unlike the PMU, which is a bit of a disaster due to version numbers, I don't
expect AVX to be problematic in practice.  E.g. most AVX features are just passed
through and don't have virtualization controls.  I just think it's a terrible
tradeoff.  E.g. if features really need to be bundled together, I don't see how
application development is meaningfully more difficult if enumeration is done
via a multi-purpose CPUID flag, versus a version number.

> 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

So what's AVX10.1?

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

I'm not worried about flexibility at this point, as much as I'm worried about
having sensible code.  E.g. if we know AVX10.2 is coming (or already here?), why
not set KVM's supported min version to 2 from the get-go?