| Message ID | 20221230162442.3781098-3-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up the supported xfeatures | expand |
On Fri, Dec 30, 2022, Aaron Lewis wrote: > Be a good citizen and don't allow any of the supported AVX-512 > xfeatures[1] to be set if they can't all be set. That way userspace or > a guest doesn't fail if it attempts to set them in XCR0. The form letter shortlog+changelo+code doesn't fit AVX-512. There's only one AVX512 flag, SSE and AVX and are pure prerequisites and exist independently of AVX512. > It's important to note that in order to set any of the AVX-512 > xfeatures, the SSE[bit-1] and AVX[bit-2] must also be set. > > [1] CPUID.(EAX=0DH,ECX=0):EAX.OPMASK[bit-5] > CPUID.(EAX=0DH,ECX=0):EAX.ZMM_Hi256[bit-6] > CPUID.(EAX=0DH,ECX=0):EAX.ZMM_Hi16_ZMM[bit-7] > > Suggested-by: Jim Mattson <jmattson@google.com> > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > --- > arch/x86/kvm/cpuid.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 2431c46d456b4..89ad8cd865173 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -862,6 +862,10 @@ static u64 sanitize_xcr0(u64 xcr0) { > if ((xcr0 & mask) != mask) > xcr0 &= ~mask; > > + mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512; Checking AVX512 is unnecessary. If it's not set, the AND-NOT is a nop. > + if ((xcr0 & mask) != mask) > + xcr0 &= ~XFEATURE_MASK_AVX512; This can be: if (!(xcr0 & XFEATURE_MASK_SSE) || !(xcr0 & XFEATURE_MASK_YMM)) xcr0 &= ~XFEATURE_MASK_AVX512 to better capture the dependency.
On Wed, Jan 04, 2023, Sean Christopherson wrote: > On Fri, Dec 30, 2022, Aaron Lewis wrote: > > Be a good citizen and don't allow any of the supported AVX-512 > > xfeatures[1] to be set if they can't all be set. That way userspace or > > a guest doesn't fail if it attempts to set them in XCR0. > > The form letter shortlog+changelo+code doesn't fit AVX-512. There's only one > AVX512 flag, SSE and AVX and are pure prerequisites and exist independently of > AVX512. Ugh, literacy issues. AVX512 isn't a singular flag. Argh. Can you split this up into two separate patches? One to require all AVX512 features, and one to clear AVX512 if SSE or AVX isn't supported?
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 2431c46d456b4..89ad8cd865173 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -862,6 +862,10 @@ static u64 sanitize_xcr0(u64 xcr0) { if ((xcr0 & mask) != mask) xcr0 &= ~mask; + mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM | XFEATURE_MASK_AVX512; + if ((xcr0 & mask) != mask) + xcr0 &= ~XFEATURE_MASK_AVX512; + return xcr0; }
Be a good citizen and don't allow any of the supported AVX-512 xfeatures[1] to be set if they can't all be set. That way userspace or a guest doesn't fail if it attempts to set them in XCR0. It's important to note that in order to set any of the AVX-512 xfeatures, the SSE[bit-1] and AVX[bit-2] must also be set. [1] CPUID.(EAX=0DH,ECX=0):EAX.OPMASK[bit-5] CPUID.(EAX=0DH,ECX=0):EAX.ZMM_Hi256[bit-6] CPUID.(EAX=0DH,ECX=0):EAX.ZMM_Hi16_ZMM[bit-7] Suggested-by: Jim Mattson <jmattson@google.com> Signed-off-by: Aaron Lewis <aaronlewis@google.com> --- arch/x86/kvm/cpuid.c | 4 ++++ 1 file changed, 4 insertions(+)