Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 20 Jul 2020 20:33:13 +0300
From:   Cyrill Gorcunov <gorcunov@gmail.com>
To:     kan.liang@linux.intel.com
Cc:     peterz@infradead.org, mingo@redhat.com, acme@kernel.org,
        tglx@linutronix.de, bp@alien8.de, x86@kernel.org,
        linux-kernel@vger.kernel.org, dave.hansen@intel.com,
        yu-cheng.yu@intel.com, bigeasy@linutronix.de, hpa@zytor.com,
        eranian@google.com, ak@linux.intel.com, chang.seok.bae@intel.com
Subject: Re: [PATCH] x86/fpu/xstate: Fix an xstate size check warning
Message-ID: <20200720173313.GA4221@grain>
References: <1595253051-75374-1-git-send-email-kan.liang@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1595253051-75374-1-git-send-email-kan.liang@linux.intel.com>
User-Agent: Mutt/1.14.5 (2020-06-23)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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>