Re: [PATCH 4/6] x86/fpu/xstate: Introduce XSAVES supervisor states

[Borislav Petkov]

On Wed, Sep 25, 2019 at 08:10:20AM -0700, Yu-cheng Yu wrote:
> Enable XSAVES supervisor states by setting MSR_IA32_XSS bits according to
> CPUID enumeration results.  Also revise comments at various places.
> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>

Same issue as before.

> Reviewed-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
> ---
>  arch/x86/kernel/fpu/xstate.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index b7f2808dd3f4..a183c319d808 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -233,13 +233,14 @@ void fpu__init_cpu_xstate(void)
>  	 * states can be set here.
>  	 */
>  	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask_user());
> +
> +	/*
> +	 * MSR_IA32_XSS sets supervisor states managed by XSAVES.
> +	 */
> +	if (boot_cpu_has(X86_FEATURE_XSAVES))
> +		wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor());
>  }
>  
> -/*
> - * Note that in the future we will likely need a pair of
> - * functions here: one for user xstates and the other for
> - * system xstates.  For now, they are the same.
> - */
>  static int xfeature_enabled(enum xfeature xfeature)
>  {
>  	return !!(xfeatures_mask_all & (1UL << xfeature));
> @@ -623,9 +624,6 @@ static void do_extra_xstate_size_checks(void)
>   * the size of the *user* states.  If we use it to size a buffer
>   * that we use 'XSAVES' on, we could potentially overflow the
>   * buffer because 'XSAVES' saves system states too.
> - *
> - * Note that we do not currently set any bits on IA32_XSS so
> - * 'XCR0 | IA32_XSS == XCR0' for now.
>   */
>  static unsigned int __init get_xsaves_size(void)
>  {
> @@ -748,7 +746,11 @@ void __init fpu__init_system_xstate(void)
>  	cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
>  	xfeatures_mask_all = eax + ((u64)edx << 32);
>  
> -	/* Place supervisor features in xfeatures_mask_all here */
> +	/*
> +	 * Find supervisor xstates supported by the processor.
> +	 */
> +	cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
> +	xfeatures_mask_all |= ecx + ((u64)edx << 32);
>  
>  	if ((xfeatures_mask_user() & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) {
>  		/*
> @@ -788,9 +790,10 @@ void __init fpu__init_system_xstate(void)
>  	setup_xstate_comp();
>  	print_xstate_offset_size();
>  
> -	pr_info("x86/fpu: Enabled xstate features 0x%llx: user 0x%llx, context size is %d bytes, using '%s' format.\n",
> +	pr_info("x86/fpu: Enabled xstate features 0x%llx: user 0x%llx and supervisor %llx, context size is %d bytes, using '%s' format.\n",
>  		xfeatures_mask_all,
>  		xfeatures_mask_user(),
> +		xfeatures_mask_supervisor(),

So if you're dumping both user and supervisor, then you don't need to
dump _all anymore. Do it this way:

	pr_info("x86/fpu: Enabled xstate features: (U: 0x%llx, S: 0x%llx), context size: %d bytes, using '%s' format.\n",
  		xfeatures_mask_user(),
 		xfeatures_mask_supervisor(),

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette