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

[Liang, Kan]

On 7/20/2020 1:33 PM, Cyrill Gorcunov wrote:
On Mon, Jul 20, 2020 at 06:50:51AM -0700, kan.li...@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.


For now, I don't have more use of 
get_xsaves_size_no_dynamic()/get_xsaves_size(). I don't know if anyone 
else will use them later.

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!


Personally, I prefer to keep the current patch because I like the name 
get_xsaves_size_no_dynamic(), which explicitly tells the dynamic 
features are excluded.

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


Thanks for the review.

Kan