Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1397384 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 02/13] x86/xsaves: Rename xstate_size to kernel_xstate_size to explicitly distinguish xstate size in kernel from user space Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-05-09 23:00 +0200
    Re: [PATCH v5 02/13] x86/xsaves: Rename xstate_size to  kernel_xstate_size to explicitly distinguish xstate size in kernel from user  space Dave Hansen <dave.hansen@linux.intel.com> - 2016-05-10 19:10 +0200
      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> - 2016-05-10 19:30 +0200
        Re: [PATCH v5 02/13] x86/xsaves: Rename xstate_size to  kernel_xstate_size to explicitly distinguish xstate size in kernel from user  space Dave Hansen <dave.hansen@linux.intel.com> - 2016-05-10 19:40 +0200
    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> - 2016-05-10 19:10 +0200

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

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
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.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
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);
+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
 	 * fpu__init_system_xstate().
 	 */
 
@@ -219,15 +219,15 @@ static void __init fpu__init_system_xstate_size_legacy(void)
 		 */
 		setup_clear_cpu_cap(X86_FEATURE_XSAVE);
 		setup_clear_cpu_cap(X86_FEATURE_XSAVEOPT);
-		xstate_size = sizeof(struct swregs_state);
+		kernel_xstate_size = sizeof(struct swregs_state);
 	} else {
 		if (cpu_has_fxsr)
-			xstate_size = sizeof(struct fxregs_state);
+			kernel_xstate_size = sizeof(struct fxregs_state);
 		else
-			xstate_size = sizeof(struct fregs_state);
+			kernel_xstate_size = sizeof(struct fregs_state);
 	}
 
-	user_xstate_size = xstate_size;
+	user_xstate_size = kernel_xstate_size;
 
 	/*
 	 * Quirk: we don't yet handle the XSAVES* instructions
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index ee6d662..0fbf60c 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -261,7 +261,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 	int ia32_fxstate = (buf != buf_fx);
 	struct task_struct *tsk = current;
 	struct fpu *fpu = &tsk->thread.fpu;
-	int state_size = xstate_size;
+	int state_size = kernel_xstate_size;
 	u64 xfeatures = 0;
 	int fx_only = 0;
 
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");
 }
 
-- 
1.9.1

[toc] | [next] | [standalone]

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

On 05/10/2016 10:01 AM, Borislav Petkov wrote:
>> >  	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.

Why?  "kernel_xstate_size" is important to the kernel because it impacts
task_struct size.

But the kernel never actually stores "user_xstate_size" anywhere or
really ever even cares about it except when copying in/out of userspace.
 "user_xstate_size" is also entirely enumerable in userspace with a
single cpuid instruction.

It's nice to dump out interesting data in dmesg, but I'm curious why you
think it's interesting.

[toc] | [prev] | [next] | [standalone]

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

On Tue, May 10, 2016 at 10:08:44AM -0700, Dave Hansen wrote:
> But the kernel never actually stores "user_xstate_size" anywhere or
> really ever even cares about it except when copying in/out of userspace.

Sounds like a reason enough to me.

> "user_xstate_size" is also entirely enumerable in userspace with a
> single cpuid instruction.

So is a lot of other stuff we're dumping in dmesg.

> It's nice to dump out interesting data in dmesg, but I'm curious why you
> think it's interesting.

I think it would be interesting to know what the kernel's idea
is of user_xstate_size. I know, I know, one can follow the code
and figure out what it is but one can say the same about a lot of
other "interesting" data dumped in dmesg. And I'd like to know what
fpu__init_system_xstate_size_legacy() decided. And so I know how many
data is shuffled to/from userspace.

And btw, this message needs more "humanization":

[    0.000000] x86/fpu: xstate_offset[2]:  576, xstate_sizes[2]:  256

That doesn't tell me anything.

Oh and it can be read out from CPUID too.

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

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

On 05/10/2016 10:26 AM, Borislav Petkov wrote:
>> > It's nice to dump out interesting data in dmesg, but I'm curious why you
>> > think it's interesting.
> I think it would be interesting to know what the kernel's idea
> is of user_xstate_size. I know, I know, one can follow the code
> and figure out what it is but one can say the same about a lot of
> other "interesting" data dumped in dmesg. And I'd like to know what
> fpu__init_system_xstate_size_legacy() decided. And so I know how many
> data is shuffled to/from userspace.
> 
> And btw, this message needs more "humanization":
> 
> [    0.000000] x86/fpu: xstate_offset[2]:  576, xstate_sizes[2]:  256
> 
> That doesn't tell me anything.
> 
> Oh and it can be read out from CPUID too.

That all sounds like great stuff to do in a follow-on patchset after
this XSAVES stuff.

[toc] | [prev] | [next] | [standalone]

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

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)
-- 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web