Re: [PATCH] x86/fpu/xstate: Fix an xstate size check warning

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Mon, Jul 20, 2020 at 06:50:51AM -0700, kan.liang@linux.intel.com wrote:
...
>  static unsigned int __init get_xsave_size(void)
>  {
>  	unsigned int eax, ebx, ecx, edx;
> @@ -710,7 +741,7 @@ static int __init init_xstate_size(void)
>  	xsave_size = get_xsave_size();
>  
>  	if (boot_cpu_has(X86_FEATURE_XSAVES))
> -		possible_xstate_size = get_xsaves_size();
> +		possible_xstate_size = get_xsaves_size_no_dynamic();
>  	else
>  		possible_xstate_size = xsave_size;

Hi! Maybe we could enhance get_xsaves_size instead ? The get_xsaves_size is
static and __init function (thus not a hot path) used once as far as I see.
Say

static unsigned int __init get_xsaves_size(void)
{
	u64 mask = xfeatures_mask_dynamic();
	unsigned int eax, ebx, ecx, edx;

	/*
	 * In case if dynamic features are present make
	 * sure they are not accounted in the result since
	 * the buffer should be allocated separately from
	 * task->fpu.
	 */
	if (mask)
		wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor());

	/*
	 * - CPUID function 0DH, sub-function 1:
	 *    EBX enumerates the size (in bytes) required by
	 *    the XSAVES instruction for an XSAVE area
	 *    containing all the state components
	 *    corresponding to bits currently set in
	 *    XCR0 | IA32_XSS.
	 */
	cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);

	if (mask)
		wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor() | mask);

	return ebx;
}

but if you expect more use of get_xsaves_size_no_dynamic() and
get_xsaves_size() in future then sure, we need a separate function.

The benefit from such extension is that when you read get_xsaves_size
you'll notice the dependency on dynamic features immediaely.

Though I'm fine with current patch as well, up to you. Thanks for the patch!

Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>