Re: [PATCH v2 1/2] x86/fpu: Add a helper to prepare AMX state for low-power CPU idle

["Chang S. Bae" <chang.seok.bae@intel.com>]

On 3/9/2022 4:24 PM, Dave Hansen wrote:
> 
> I assume that splat is because 0day found a CPU which doesn't support
> XGETBV1.  Since fpu_state_size_dynamic() only ever returns true on
> XGETBV1 systems so it works as a proxy for checking XGETBV1 support.
> 
> Right? >
> If so, then fpu_state_size_dynamic() is a *bit* of an oblique way to
> check for XGETBV1 support.
> > Why don't we do a good old:
> 
> 	cpu_feature_enabled(X86_FEATURE_XGETBV1)
> 
> check?

Agreed, checking XGETBV1 support is the reason for this, so this looks 
to be straightforward here.

> 
> Also, did we get the asm constraints wrong on xgetbv()?  Surely we
> shouldn't be allowing the compiler to reorder it.  Do we need a "memory"
> constraint?

I think this is a good point. Perhaps x{get|set}bv() may follow this 
change [1] to prevent any reordering.

BTW, now I'm suspicious of this JMP as patched at runtime with 
fpu_state_size_dynamic():

   22:   eb 01                    jmp    0x25
   24:   c3                       retq
   25:   b9 01 00 00 00           mov    $0x1,%ecx
   2a:*  0f 01 d0                 xgetbv           <-- trapping instruction

Still, the question is, if so, why it was patched on non-XFD systems. 
Let me analyze the case a bit further with 0day folks.

Thanks,
Chang

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=aa5cacdc29d76a005cbbee018a47faa6e724dd2d