Re: [PATCH] crypto: x86/aes-ni: fix AVX detection

[Maxim Levitsky]

On Wed, 2021-11-03 at 07:43 -0700, Dave Hansen wrote:
> On 11/3/21 5:46 AM, Maxim Levitsky wrote:
> > Fix two semi-theoretical issues that are present.
> > 
> > 1. AVX is assumed to be present when AVX2 is present.
> >  That can be false in a VM.
> >  This can be considered a hypervisor bug,
> >  but the kernel should not crash in this case if this is possible.
> 
> The kernel shouldn't crash in this case.  We've got a software
> dependency which should disable AVX2 if AVX is off:
> 
> static const struct cpuid_dep cpuid_deps[] = {
> ...
>         { X86_FEATURE_AVX2,                     X86_FEATURE_AVX,      },



Looks like this table is only used when someone calls setup_clear_cpu_cap/clear_cpu_cap
which is used in all kind of places to disable CPU features that fails various conditions
(like VMX/SMX when MSR_IA32_FEAT_CTL suddently faults, or to disable RDRAND when
it fails built-in selfcheck and such, and it is used when user disables features on
kernel cmd line like the 'noxsave'

Also we do have the 'filter_cpuid_features' which disables some CPUID features,
which depend on whole CPUID leaves to be there.
This is the only precendent of the kernel coping with a bogus CPUID given by the
hypervisor, and it is even mentioned in a comment near this code.


filter_cpuid_features can be extended to filter known bogus CPUID depedencies,
like case when AVX2 supported and AVX not supported in CPUID.
If you agree, then it seems the best case to deal with this issue.

> 
> 
> > 2. YMM state can be soft disabled in XCR0.
> > 
> > Fix both issues by using 'cpu_has_xfeatures(XFEATURE_MASK_YMM')
> > to check for usable AVX support.
> 
> There's another table to ensure that this doesn't happen:
> 
> > static unsigned short xsave_cpuid_features[] __initdata = {
> >         [XFEATURE_FP]                           = X86_FEATURE_FPU,
> >         [XFEATURE_SSE]                          = X86_FEATURE_XMM,
> >         [XFEATURE_YMM]                          = X86_FEATURE_AVX,
> 
> So, if XFEATURE_YMM isn't supported, X86_FEATURE_AVX should be cleared.

I afraid that it is the other way around - this table makes sure that
we disable 'XFEATURE_YMM' in the xsave header when we (the kernel) uses
the xsave instruction, if the X86_FEATURE_AVX is not supported.

However, I haven't found a way to disable selected xfeatures, but only the
'noxsave/noxsaves/etc' which disable the whole feature in cpuid,
and that indeed does trigger the disablement via 'cpuid_deps' table.

> 
> But, that's all how it's _supposed_ to work.  It's quite possible we've
> got bugs somewhere, so if you're hitting an issue in practice please let
> us know.
> 
> If this did end up confusing you and Paulo, that's not great either.
> Any patches that make these dependency tables easier to find or grok
> would be appreciated too.
> 


Sorry for very late reply, I haven't gotten to work on this bug for a while,
Best regards,
	Maxim Levitsky