Re: [RFC PATCH v4 03/27] x86/fpu/xstate: Enable XSAVES system states

[Borislav Petkov <bp@alien8.de>]

On Fri, Sep 21, 2018 at 08:03:27AM -0700, Yu-cheng Yu wrote:
> XSAVES saves both system and user states.  The Linux kernel
> currently does not save/restore any system states.  This patch
> creates the framework for supporting system states.

... and needs a lot more text explaining *why* it is doing that.

> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
>  arch/x86/include/asm/fpu/internal.h |   3 +-
>  arch/x86/include/asm/fpu/xstate.h   |   9 ++-
>  arch/x86/kernel/fpu/core.c          |   7 +-
>  arch/x86/kernel/fpu/init.c          |  10 ---
>  arch/x86/kernel/fpu/xstate.c        | 112 +++++++++++++++++-----------
>  5 files changed, 80 insertions(+), 61 deletions(-)
> 
> diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
> index f1f9bf91a0ab..1f447865db3a 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -45,7 +45,6 @@ extern void fpu__init_cpu_xstate(void);
>  extern void fpu__init_system(struct cpuinfo_x86 *c);
>  extern void fpu__init_check_bugs(void);
>  extern void fpu__resume_cpu(void);
> -extern u64 fpu__get_supported_xfeatures_mask(void);
>  
>  /*
>   * Debugging facility:
> @@ -94,7 +93,7 @@ static inline void fpstate_init_xstate(struct xregs_state *xsave)
>  	 * trigger #GP:
>  	 */
>  	xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
> -			xfeatures_mask_user;
> +			xfeatures_mask_all;
>  }
>  
>  static inline void fpstate_init_fxstate(struct fxregs_state *fx)
> diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
> index 9b382e5157ed..a32dc5f8c963 100644
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -19,10 +19,10 @@
>  #define XSAVE_YMM_SIZE	    256
>  #define XSAVE_YMM_OFFSET    (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
>  
> -/* System features */
> -#define XFEATURE_MASK_SYSTEM (XFEATURE_MASK_PT)

Previous patch renames it, this patch deletes it. Why do we need all
that unnecessary churn?

Also, this patch is trying to do a couple of things at once and
reviewing it is not trivial. Please split the changes logically.

> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 19f8df54c72a..dd2c561c4544 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -51,13 +51,16 @@ static short xsave_cpuid_features[] __initdata = {
>  };
>  
>  /*
> - * Mask of xstate features supported by the CPU and the kernel:
> + * Mask of xstate features supported by the CPU and the kernel.
> + * This is the result from CPUID query, SUPPORTED_XFEATURES_MASK,
> + * and boot_cpu_has().
>   */

This needs to explain what both masks are - user and system. "CPU" and
"kernel" is not "user" and "all".

>  u64 xfeatures_mask_user __read_mostly;
> +u64 xfeatures_mask_all __read_mostly;



> @@ -219,30 +222,31 @@ void fpstate_sanitize_xstate(struct fpu *fpu)
>   */
>  void fpu__init_cpu_xstate(void)
>  {
> -	if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask_user)
> +	if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask_all)
>  		return;
> +
> +	cr4_set_bits(X86_CR4_OSXSAVE);
> +
>  	/*
> -	 * Make it clear that XSAVES system states are not yet
> -	 * implemented should anyone expect it to work by changing
> -	 * bits in XFEATURE_MASK_* macros and XCR0.
> +	 * XCR_XFEATURE_ENABLED_MASK sets the features that are managed
> +	 * by XSAVE{C, OPT} and XRSTOR.  Only XSAVE user states can be
> +	 * set here.
>  	 */
> -	WARN_ONCE((xfeatures_mask_user & XFEATURE_MASK_SYSTEM),
> -		"x86/fpu: XSAVES system states are not yet implemented.\n");
> +	xsetbv(XCR_XFEATURE_ENABLED_MASK,
> +	       xfeatures_mask_user);

No need to break the line here.

Also, you have a couple more places in your patches where you
unnecessarily break lines. Please don't do that, even if it exceeds 80
cols by a couple of chars.

>  
> -	xfeatures_mask_user &= ~XFEATURE_MASK_SYSTEM;
> -
> -	cr4_set_bits(X86_CR4_OSXSAVE);
> -	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask_user);
> +	/*
> +	 * MSR_IA32_XSS sets which XSAVES system states to be managed by
> +	 * XSAVES.  Only XSAVES system states can be set here.
> +	 */
> +	if (boot_cpu_has(X86_FEATURE_XSAVES))
> +		wrmsrl(MSR_IA32_XSS,
> +		       xfeatures_mask_all & ~xfeatures_mask_user);

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.