Re: [PATCH v5 02/13] x86/xsaves: Rename xstate_size to kernel_xstate_size to explicitly distinguish xstate size in kernel from user space

[Borislav Petkov <bp@suse.de>]

On Mon, May 09, 2016 at 01:45:59PM -0700, Yu-cheng Yu wrote:
> User space uses standard format xsave area. fpstate in signal frame should
> have standard format size.
> 
> To explicitly distinguish between xstate size in kernel space and the one
> in user space, we rename xstate_size to kernel_xstate_size. This patch is

Let's call it

xstate_kernel_size

or

fpu_xstate_kernel_size

and thus keep all the FPU-related variables and functions in the same
namespace.

> not fixing a bug. It just makes kernel code more clear.
> 
> So we define the xsave area sizes in two global variables:
> 
> kernel_xstate_size (previous xstate_size): the xsave area size used in
> xsave area allocated in kernel
> user_xstate_size: the xsave area size used in xsave area used by user.
> 
> In no "xsaves" case, xsave area in both user space and kernel space are in
> standard format. Therefore, kernel_xstate_size and user_xstate_size are
> equal.
> 
> In "xsaves" case, xsave area in user space is in standard format while
> xsave area in kernel space is in compact format. Therefore, kernel's
> xstate size is less than user's xstate size.

Those last two paragraphs look like a good candidates for comments above
that new variable.

> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>

This SOB chain needs fixing too.

> Reviewed-by: Dave Hansen <dave.hansen@intel.com>
> ---
>  arch/x86/include/asm/processor.h |  2 +-
>  arch/x86/kernel/fpu/core.c       |  6 +++---
>  arch/x86/kernel/fpu/init.c       | 18 +++++++++---------
>  arch/x86/kernel/fpu/signal.c     |  2 +-
>  arch/x86/kernel/fpu/xstate.c     |  8 ++++----
>  5 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 132b4ca..db7f0f9 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -367,7 +367,7 @@ DECLARE_PER_CPU(struct irq_stack *, hardirq_stack);
>  DECLARE_PER_CPU(struct irq_stack *, softirq_stack);
>  #endif	/* X86_64 */
>  
> -extern unsigned int xstate_size;
> +extern unsigned int kernel_xstate_size;
>  extern unsigned int user_xstate_size;
>  
>  struct perf_event;
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 8e37cc8..41ab106 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -222,7 +222,7 @@ void fpstate_init(union fpregs_state *state)
>  		return;
>  	}
>  
> -	memset(state, 0, xstate_size);
> +	memset(state, 0, kernel_xstate_size);
>  
>  	if (cpu_has_fxsr)
>  		fpstate_init_fxstate(&state->fxsave);
> @@ -247,7 +247,7 @@ int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu)
>  	 * leak into the child task:
>  	 */
>  	if (use_eager_fpu())
> -		memset(&dst_fpu->state.xsave, 0, xstate_size);
> +		memset(&dst_fpu->state.xsave, 0, kernel_xstate_size);
>  
>  	/*
>  	 * Save current FPU registers directly into the child
> @@ -266,7 +266,7 @@ int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu)
>  	 */
>  	preempt_disable();
>  	if (!copy_fpregs_to_fpstate(dst_fpu)) {
> -		memcpy(&src_fpu->state, &dst_fpu->state, xstate_size);
> +		memcpy(&src_fpu->state, &dst_fpu->state, kernel_xstate_size);
>  
>  		if (use_eager_fpu())
>  			copy_kernel_to_fpregs(&src_fpu->state);
> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
> index 7ea80c2..549ff59 100644
> --- a/arch/x86/kernel/fpu/init.c
> +++ b/arch/x86/kernel/fpu/init.c
> @@ -145,8 +145,8 @@ static void __init fpu__init_system_generic(void)
>   * This is inherent to the XSAVE architecture which puts all state
>   * components into a single, continuous memory block:
>   */
> -unsigned int xstate_size;
> -EXPORT_SYMBOL_GPL(xstate_size);

/*
 * Needs a comment here explaining what it is exactly.
 */

> +unsigned int kernel_xstate_size;
> +EXPORT_SYMBOL_GPL(kernel_xstate_size);
>  
>  /* Get alignment of the TYPE. */
>  #define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
> @@ -178,7 +178,7 @@ static void __init fpu__init_task_struct_size(void)
>  	 * Add back the dynamically-calculated register state
>  	 * size.
>  	 */
> -	task_size += xstate_size;
> +	task_size += kernel_xstate_size;
>  
>  	/*
>  	 * We dynamically size 'struct fpu', so we require that
> @@ -195,7 +195,7 @@ static void __init fpu__init_task_struct_size(void)
>  }
>  
>  /*
> - * Set up the user and kernel xstate_size based on the legacy FPU context size.
> + * Set up the user and kernel xstate sizes based on the legacy FPU context size.
>   *
>   * We set this up first, and later it will be overwritten by
>   * fpu__init_system_xstate() if the CPU knows about xstates.
> @@ -208,7 +208,7 @@ static void __init fpu__init_system_xstate_size_legacy(void)
>  	on_boot_cpu = 0;
>  
>  	/*
> -	 * Note that xstate_size might be overwriten later during
> +	 * Note that xstate sizes might be overwriten later during

s/overwriten/overwritten/

>  	 * fpu__init_system_xstate().
>  	 */

...

> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index d8aa7d2..20c6631 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -532,7 +532,7 @@ static void do_extra_xstate_size_checks(void)
>  		 */
>  		paranoid_xstate_size += xfeature_size(i);
>  	}
> -	XSTATE_WARN_ON(paranoid_xstate_size != xstate_size);
> +	XSTATE_WARN_ON(paranoid_xstate_size != kernel_xstate_size);
>  }
>  
>  
> @@ -611,7 +611,7 @@ static int init_xstate_size(void)
>  	 * The size is OK, we are definitely going to use xsave,
>  	 * make it known to the world that we need more space.
>  	 */
> -	xstate_size = possible_xstate_size;
> +	kernel_xstate_size = possible_xstate_size;
>  	do_extra_xstate_size_checks();
>  
>  	/*
> @@ -674,14 +674,14 @@ void __init fpu__init_system_xstate(void)
>  		return;
>  	}
>  
> -	update_regset_xstate_info(xstate_size, xfeatures_mask);
> +	update_regset_xstate_info(kernel_xstate_size, xfeatures_mask);
>  	fpu__init_prepare_fx_sw_frame();
>  	setup_init_fpu_buf();
>  	setup_xstate_comp();
>  
>  	pr_info("x86/fpu: Enabled xstate features 0x%llx, context size is %d bytes, using '%s' format.\n",
>  		xfeatures_mask,
> -		xstate_size,
> +		kernel_xstate_size,
>  		cpu_has_xsaves ? "compacted" : "standard");

I think we should dump user_xstate_size in the compacted case since it
is != kernel_xstate_size.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--