On Tue, May 21, 2024 at 12:41:54PM -0700, Sean Christopherson wrote:
> 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.
>
I see, if a 'small part' of a version cannot be advertised, it will block the
virtualization of all subsequent versions. If this special 'small part' is
really introduced later, I believe this will belong to the rare case and be
enumerated in the sub-leaf of CPUID leaf 24H.
> 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.
Yes, I can’t agree more.
> 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.
>
For applications, it seems no significant advantage to the version number.
Maybe applications can batch operations based on the version number and the
supported vector length.
> > 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.1 just adds the related CPUIDs for software pre-enabling, i.e. AVX10.1
has no VMX capability, Embedded rounding and Suppress All Exceptions (SAE)
control, which will be introduced in AVX10.2.
> > 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?
Per the spec, AVX10.2 will have a VMX capability, i.e. in the future, KVM may
have to do something before advertising AVX10.2. But now there are CPUs that
support AVX10.1, so AVX10.1 should to be advertised to guest firstly.
