Re: [PATCH v4 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 5/18/2022 8:41 AM, Dave Hansen wrote:
> 
> This is a pretty minor nit, but:
> 
> X86_FEATURE_XFD depends on X86_FEATURE_XGETBV1
> 
> and
> 
> X86_FEATURE_AMX_TILE depends on X86_FEATURE_XFD
> 
> via cpu_deps[].  So there is an implicit dependency all the way from AMX
> to XGETBV1.  It's also not patently obvious what X86_FEATURE_XGETBV1 has
> to do with the rest of the if().
> 
> Would this make more logical sense to folks?
> 
> 	/* Note: AMX_TILE being enabled implies XGETBV1 support */
> 	if (cpu_feature_enabled(X86_FEATURE_AMX_TILE) &&
> 	    (xfeatures_in_use() & XFEATURE_MASK_XTILE)) {
> 		tile_release();
> 		fpregs_deactivate(&current->thread.fpu);
> 	}

With the note, I guess people will have no problem with AMX->XGETBV1. 
But I would leave this question to others who can tell.

> 
> That also has a nice side effect that non-AMX systems will get to use a
> static branch and can also skip over the XGETBV1 entirely.

Yes, but FWIW, as it is non-architectural, the function should be 
consumed only by drivers for AMX systems.

> 
> The downside is that there's no explicit XGETBV1 check before calling
> xfeatures_in_use().  But, I don't really expect the AMX->XGETBV1
> dependency to go away either.

Yes, as long as AMX is wanted as a dynamic feature I think.

Thanks,
Chang