[PATCH v5 00/12] x86/xsaves: Fix XSAVES issues

[PATCH v5 00/12] x86/xsaves: Fix XSAVES issues

[Yu-cheng Yu]

XSAVES is a kernel-mode instruction. It offers a compacted format and
memory-write optimization. These patches fix issues in the first
implementation. They are intended for discussion and getting feedback
before actually getting applied.

Changes since Version 4:

Patch 4 - Introduce a new check to copy registers directly to signal
          frame: Simplify the check for compacted format.

Patch 7 - Fix PTRACE frames for XSAVES: If PTRACE attempts to set
          any disabled xstate, return failure; using_compacted_format()
          is used when appropriate.

New additions:

Patch 10 - Fix __fp_restore_sig() for xsaves: it was doing direct
           copying from user mode standard format to kernel mode,
           which could be in compacted format. Fix it by restoring
           directly to registers.

Patch 11 - Add WARN_ON_FPU() when a disabled xstate component is
           requested for a compacted format.

Patch 12 - Fix fpstate_init() for XSAVES: xcomp_bv[63] must be set
           for XSAVES.

Yu-cheng Yu (13):
  x86/xsaves: Define and use user_xstate_size for xstate size in signal
    context
  x86/xsaves: Rename xstate_size to kernel_xstate_size to explicitly
    distinguish xstate size in kernel from user space
  x86/xsaves: Keep init_fpstate.xsave.header.xfeatures as zero for init
    optimization
  x86/xsaves: Introduce a new check that allows correct xstates copy
    from kernel to user directly
  x86/xsaves: Align xstate components according to CPUID
  x86/xsaves: Supervisor state component offset
  x86/xsaves: Fix PTRACE frames for XSAVES
  x86/xsaves: Fix XSTATE component offset print out
  x86/xsaves: Fix xstate_offsets, xstate_sizes for non-extended states
  x86/xsaves: Fix __fpu_restore_sig() for XSAVES
  x86/xsaves: Add WARN_ON_FPU() when a disabled xstate component offset
    is requested for a compacted format
  x86/xsaves: Fix fpstate_init() for XSAVES
  x86/xsaves: Re-enable XSAVES

 arch/x86/include/asm/fpu/types.h  |   1 +
 arch/x86/include/asm/fpu/xstate.h |  10 +-
 arch/x86/include/asm/processor.h  |   3 +-
 arch/x86/kernel/fpu/core.c        |   9 +-
 arch/x86/kernel/fpu/init.c        |  32 +--
 arch/x86/kernel/fpu/regset.c      |  56 ++++--
 arch/x86/kernel/fpu/signal.c      |  42 +++-
 arch/x86/kernel/fpu/xstate.c      | 408 ++++++++++++++++++++++++++++++--------
 8 files changed, 420 insertions(+), 141 deletions(-)

-- 
1.9.1

[PATCH v5 01/13] x86/xsaves: Define and use user_xstate_size for xstate size in signal context

[Yu-cheng Yu]

If "xsaves" is enabled, kernel always uses compacted format of xsave area.
But user space still uses standard format of xsave area. Thus, xstate size
in kernel's xsave area is smaller than xstate size in user's xsave area.
The xstate in signal frame should be in standard format for user's signal
handler to access.

In no "xsaves" case, xsave area in both user space and kernel space are in
standard format. Therefore, user's and kernel's xstate sizes are equal.

In "xsaves" case, xsave area in user space is in standard format while
xsave area in kernel space is in compacted format. Therefore, kernel's
xstate size is smaller than user's xstate size.

So here is the problem: currently kernel assumes its own xstate size is
signal frame's xstate size. This is not a problem in no "xsaves" case.
It is an issue in "xsaves" case because kernel's xstate size is smaller
than user's xstate size. In fpu__alloc_mathframe(), a smaller fpstate
buffer is allocated for the standard format xstate in signal frame.
Then kernel saves only part of xstate registers into this smaller
user's fpstate buffer and user will see part of the xstate registers in
signal context. Similar issue happens after returning from signal handler:
kernel will only restore part of xstate registers from user's fpstate
buffer in signal frame.

This patch defines and uses user_xstate_size for xstate size in signal
frame. It's read from returned value in ebx from CPUID leaf 0x0D subleaf
0x0. This is maximum size required by enabled states in XCR0 and may be
different from ecx when states at the end of the xsave area are not
enabled. This value indicates the size required for XSAVE to save all
supported user states in legacy/standard format.

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/fpu/xstate.h |  1 -
 arch/x86/include/asm/processor.h  |  1 +
 arch/x86/kernel/fpu/init.c        |  5 ++-
 arch/x86/kernel/fpu/signal.c      | 26 ++++++++++----
 arch/x86/kernel/fpu/xstate.c      | 71 ++++++++++++++++++++++++---------------
 5 files changed, 67 insertions(+), 37 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 38951b0..16df2c4 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -39,7 +39,6 @@
 #define REX_PREFIX
 #endif
 
-extern unsigned int xstate_size;
 extern u64 xfeatures_mask;
 extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
 
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 9264476..132b4ca 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -368,6 +368,7 @@ DECLARE_PER_CPU(struct irq_stack *, softirq_stack);
 #endif	/* X86_64 */
 
 extern unsigned int xstate_size;
+extern unsigned int user_xstate_size;
 
 struct perf_event;
 
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 54c86ff..7ea80c2 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -195,7 +195,7 @@ static void __init fpu__init_task_struct_size(void)
 }
 
 /*
- * Set up the xstate_size based on the legacy FPU context size.
+ * Set up the user and kernel xstate_size 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.
@@ -226,6 +226,9 @@ static void __init fpu__init_system_xstate_size_legacy(void)
 		else
 			xstate_size = sizeof(struct fregs_state);
 	}
+
+	user_xstate_size = xstate_size;
+
 	/*
 	 * Quirk: we don't yet handle the XSAVES* instructions
 	 * correctly, as we don't correctly convert between
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 31c6a60..ee6d662 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -31,7 +31,7 @@ static inline int check_for_xstate(struct fxregs_state __user *buf,
 	/* Check for the first magic field and other error scenarios. */
 	if (fx_sw->magic1 != FP_XSTATE_MAGIC1 ||
 	    fx_sw->xstate_size < min_xstate_size ||
-	    fx_sw->xstate_size > xstate_size ||
+	    fx_sw->xstate_size > user_xstate_size ||
 	    fx_sw->xstate_size > fx_sw->extended_size)
 		return -1;
 
@@ -88,7 +88,7 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame)
 	if (!use_xsave())
 		return err;
 
-	err |= __put_user(FP_XSTATE_MAGIC2, (__u32 *)(buf + xstate_size));
+	err |= __put_user(FP_XSTATE_MAGIC2, (__u32 *)(buf + user_xstate_size));
 
 	/*
 	 * Read the xfeatures which we copied (directly from the cpu or
@@ -125,7 +125,7 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
 	else
 		err = copy_fregs_to_user((struct fregs_state __user *) buf);
 
-	if (unlikely(err) && __clear_user(buf, xstate_size))
+	if (unlikely(err) && __clear_user(buf, user_xstate_size))
 		err = -EFAULT;
 	return err;
 }
@@ -175,8 +175,19 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 		if (ia32_fxstate)
 			copy_fxregs_to_kernel(&tsk->thread.fpu);
 	} else {
+		/*
+		 * It is a *bug* if kernel uses compacted-format for xsave
+		 * area and we copy it out directly to a signal frame. It
+		 * should have been handled above by saving the registers
+		 * directly.
+		 */
+		if (boot_cpu_has(X86_FEATURE_XSAVES)) {
+			WARN_ONCE(1, "x86/fpu: saving compacted-format xsave area to a signal frame!\n");
+			return -1;
+		}
+
 		fpstate_sanitize_xstate(&tsk->thread.fpu);
-		if (__copy_to_user(buf_fx, xsave, xstate_size))
+		if (__copy_to_user(buf_fx, xsave, user_xstate_size))
 			return -1;
 	}
 
@@ -341,7 +352,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 
 static inline int xstate_sigframe_size(void)
 {
-	return use_xsave() ? xstate_size + FP_XSTATE_MAGIC2_SIZE : xstate_size;
+	return use_xsave() ? user_xstate_size + FP_XSTATE_MAGIC2_SIZE :
+			user_xstate_size;
 }
 
 /*
@@ -385,12 +397,12 @@ fpu__alloc_mathframe(unsigned long sp, int ia32_frame,
  */
 void fpu__init_prepare_fx_sw_frame(void)
 {
-	int size = xstate_size + FP_XSTATE_MAGIC2_SIZE;
+	int size = user_xstate_size + FP_XSTATE_MAGIC2_SIZE;
 
 	fx_sw_reserved.magic1 = FP_XSTATE_MAGIC1;
 	fx_sw_reserved.extended_size = size;
 	fx_sw_reserved.xfeatures = xfeatures_mask;
-	fx_sw_reserved.xstate_size = xstate_size;
+	fx_sw_reserved.xstate_size = user_xstate_size;
 
 	if (config_enabled(CONFIG_IA32_EMULATION) ||
 	    config_enabled(CONFIG_X86_32)) {
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index b48ef35..d8aa7d2 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -43,6 +43,8 @@ static unsigned int xstate_offsets[XFEATURE_MAX] = { [ 0 ... XFEATURE_MAX - 1] =
 static unsigned int xstate_sizes[XFEATURE_MAX]   = { [ 0 ... XFEATURE_MAX - 1] = -1};
 static unsigned int xstate_comp_offsets[sizeof(xfeatures_mask)*8];
 
+unsigned int user_xstate_size;
+
 /*
  * Clear all of the X86_FEATURE_* bits that are unavailable
  * when the CPU has no XSAVE support.
@@ -171,7 +173,7 @@ void fpstate_sanitize_xstate(struct fpu *fpu)
 	 */
 	while (xfeatures) {
 		if (xfeatures & 0x1) {
-			int offset = xstate_offsets[feature_bit];
+			int offset = xstate_comp_offsets[feature_bit];
 			int size = xstate_sizes[feature_bit];
 
 			memcpy((void *)fx + offset,
@@ -533,8 +535,9 @@ static void do_extra_xstate_size_checks(void)
 	XSTATE_WARN_ON(paranoid_xstate_size != xstate_size);
 }
 
+
 /*
- * Calculate total size of enabled xstates in XCR0/xfeatures_mask.
+ * Get total size of enabled xstates in XCR0/xfeatures_mask.
  *
  * Note the SDM's wording here.  "sub-function 0" only enumerates
  * the size of the *user* states.  If we use it to size a buffer
@@ -544,34 +547,33 @@ static void do_extra_xstate_size_checks(void)
  * Note that we do not currently set any bits on IA32_XSS so
  * 'XCR0 | IA32_XSS == XCR0' for now.
  */
-static unsigned int __init calculate_xstate_size(void)
+static unsigned int __init get_xsaves_size(void)
 {
 	unsigned int eax, ebx, ecx, edx;
-	unsigned int calculated_xstate_size;
+	/*
+	 * - 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);
+	return ebx;
+}
 
-	if (!cpu_has_xsaves) {
-		/*
-		 * - CPUID function 0DH, sub-function 0:
-		 *    EBX enumerates the size (in bytes) required by
-		 *    the XSAVE instruction for an XSAVE area
-		 *    containing all the *user* state components
-		 *    corresponding to bits currently set in XCR0.
-		 */
-		cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
-		calculated_xstate_size = ebx;
-	} else {
-		/*
-		 * - 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);
-		calculated_xstate_size = ebx;
-	}
-	return calculated_xstate_size;
+static unsigned int __init get_xsave_size(void)
+{
+	unsigned int eax, ebx, ecx, edx;
+	/*
+	 * - CPUID function 0DH, sub-function 0:
+	 *    EBX enumerates the size (in bytes) required by
+	 *    the XSAVE instruction for an XSAVE area
+	 *    containing all the *user* state components
+	 *    corresponding to bits currently set in XCR0.
+	 */
+	cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
+	return ebx;
 }
 
 /*
@@ -591,7 +593,15 @@ static bool is_supported_xstate_size(unsigned int test_xstate_size)
 static int init_xstate_size(void)
 {
 	/* Recompute the context size for enabled features: */
-	unsigned int possible_xstate_size = calculate_xstate_size();
+	unsigned int possible_xstate_size;
+	unsigned int xsave_size;
+
+	xsave_size = get_xsave_size();
+
+	if (cpu_has_xsaves)
+		possible_xstate_size = get_xsaves_size();
+	else
+		possible_xstate_size = xsave_size;
 
 	/* Ensure we have the space to store all enabled: */
 	if (!is_supported_xstate_size(possible_xstate_size))
@@ -603,6 +613,11 @@ static int init_xstate_size(void)
 	 */
 	xstate_size = possible_xstate_size;
 	do_extra_xstate_size_checks();
+
+	/*
+	 * User space is always in standard format.
+	 */
+	user_xstate_size = xsave_size;
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH v5 01/13] x86/xsaves: Define and use user_xstate_size for xstate size in signal context

[Borislav Petkov]

On Mon, May 09, 2016 at 01:45:58PM -0700, Yu-cheng Yu wrote:
> If "xsaves" is enabled, kernel always uses compacted format of xsave area.
> But user space still uses standard format of xsave area. Thus, xstate size
> in kernel's xsave area is smaller than xstate size in user's xsave area.
> The xstate in signal frame should be in standard format for user's signal
> handler to access.
> 
> In no "xsaves" case, xsave area in both user space and kernel space are in
> standard format. Therefore, user's and kernel's xstate sizes are equal.
> 
> In "xsaves" case, xsave area in user space is in standard format while
> xsave area in kernel space is in compacted format. Therefore, kernel's
> xstate size is smaller than user's xstate size.

So this repeats what the first paragraph said.

> So here is the problem: currently kernel assumes its own xstate size is
> signal frame's xstate size. This is not a problem in no "xsaves" case.

							 ^
							the

This whole text is missing a bunch of "the"s...

> It is an issue in "xsaves" case because kernel's xstate size is smaller
		   ^			 ^
		 the			the

and so on...


> than user's xstate size. In fpu__alloc_mathframe(), a smaller fpstate
> buffer is allocated for the standard format xstate in signal frame.
> Then kernel saves only part of xstate registers into this smaller
> user's fpstate buffer and user will see part of the xstate registers in
> signal context. Similar issue happens after returning from signal handler:
> kernel will only restore part of xstate registers from user's fpstate
> buffer in signal frame.
> 
> This patch defines and uses user_xstate_size for xstate size in signal
> frame. It's read from returned value in ebx from CPUID leaf 0x0D subleaf
> 0x0. This is maximum size required by enabled states in XCR0 and may be
> different from ecx when states at the end of the xsave area are not
> enabled. This value indicates the size required for XSAVE to save all
> supported user states in legacy/standard format.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>

That SOB chain needs clarification: if Fenghua is the author, the patch
should contain his From: at the top. If it is based on an earlier patch
from him, commit message should say:

Based on an earlier patch from Fenghua... without the SOB.

> Reviewed-by: Dave Hansen <dave.hansen@intel.com>
> ---
>  arch/x86/include/asm/fpu/xstate.h |  1 -
>  arch/x86/include/asm/processor.h  |  1 +
>  arch/x86/kernel/fpu/init.c        |  5 ++-
>  arch/x86/kernel/fpu/signal.c      | 26 ++++++++++----
>  arch/x86/kernel/fpu/xstate.c      | 71 ++++++++++++++++++++++++---------------
>  5 files changed, 67 insertions(+), 37 deletions(-)
> 
> diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
> index 38951b0..16df2c4 100644
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -39,7 +39,6 @@
>  #define REX_PREFIX
>  #endif
>  
> -extern unsigned int xstate_size;
>  extern u64 xfeatures_mask;
>  extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
>  
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 9264476..132b4ca 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -368,6 +368,7 @@ DECLARE_PER_CPU(struct irq_stack *, softirq_stack);
>  #endif	/* X86_64 */
>  
>  extern unsigned int xstate_size;
> +extern unsigned int user_xstate_size;

If this is going to be exported, let's prefix it pls:

fpu_user_xstate_size

or

xstate_user_state_size

or somesuch.

And let's add a comment over its definition what exactly it represents.
I.e., the aspect about the signal frame...

...

> @@ -591,7 +593,15 @@ static bool is_supported_xstate_size(unsigned int test_xstate_size)
>  static int init_xstate_size(void)
>  {
>  	/* Recompute the context size for enabled features: */
> -	unsigned int possible_xstate_size = calculate_xstate_size();
> +	unsigned int possible_xstate_size;
> +	unsigned int xsave_size;
> +
> +	xsave_size = get_xsave_size();
> +
> +	if (cpu_has_xsaves)

	if (boot_cpu_has(X86_FEATURE_XSAVES))

> +		possible_xstate_size = get_xsaves_size();
> +	else
> +		possible_xstate_size = xsave_size;
>  
>  	/* Ensure we have the space to store all enabled: */
>  	if (!is_supported_xstate_size(possible_xstate_size))

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v5 01/13] x86/xsaves: Define and use user_xstate_size for xstate size in signal context

[Yu-cheng Yu]

On Tue, May 10, 2016 at 01:04:41PM +0200, Borislav Petkov wrote:
> On Mon, May 09, 2016 at 01:45:58PM -0700, Yu-cheng Yu wrote:
> > If "xsaves" is enabled, kernel always uses compacted format of xsave area.
> > But user space still uses standard format of xsave area. Thus, xstate size
> > in kernel's xsave area is smaller than xstate size in user's xsave area.
> > The xstate in signal frame should be in standard format for user's signal
> > handler to access.
> > 
> > In no "xsaves" case, xsave area in both user space and kernel space are in
> > standard format. Therefore, user's and kernel's xstate sizes are equal.
> > 
> > In "xsaves" case, xsave area in user space is in standard format while
> > xsave area in kernel space is in compacted format. Therefore, kernel's
> > xstate size is smaller than user's xstate size.
> 
> So this repeats what the first paragraph said.
> 
> > So here is the problem: currently kernel assumes its own xstate size is
> > signal frame's xstate size. This is not a problem in no "xsaves" case.
> 
> 							 ^
> 							the
> 
> This whole text is missing a bunch of "the"s...
> 
> > It is an issue in "xsaves" case because kernel's xstate size is smaller
> 		   ^			 ^
> 		 the			the
> 
> and so on...
> 
> 
> > than user's xstate size. In fpu__alloc_mathframe(), a smaller fpstate
> > buffer is allocated for the standard format xstate in signal frame.
> > Then kernel saves only part of xstate registers into this smaller
> > user's fpstate buffer and user will see part of the xstate registers in
> > signal context. Similar issue happens after returning from signal handler:
> > kernel will only restore part of xstate registers from user's fpstate
> > buffer in signal frame.
> > 
> > This patch defines and uses user_xstate_size for xstate size in signal
> > frame. It's read from returned value in ebx from CPUID leaf 0x0D subleaf
> > 0x0. This is maximum size required by enabled states in XCR0 and may be
> > different from ecx when states at the end of the xsave area are not
> > enabled. This value indicates the size required for XSAVE to save all
> > supported user states in legacy/standard format.
> > 
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> 
> That SOB chain needs clarification: if Fenghua is the author, the patch
> should contain his From: at the top. If it is based on an earlier patch
> from him, commit message should say:
> 
> Based on an earlier patch from Fenghua... without the SOB.
> 
> > Reviewed-by: Dave Hansen <dave.hansen@intel.com>
> > ---
> >  arch/x86/include/asm/fpu/xstate.h |  1 -
> >  arch/x86/include/asm/processor.h  |  1 +
> >  arch/x86/kernel/fpu/init.c        |  5 ++-
> >  arch/x86/kernel/fpu/signal.c      | 26 ++++++++++----
> >  arch/x86/kernel/fpu/xstate.c      | 71 ++++++++++++++++++++++++---------------
> >  5 files changed, 67 insertions(+), 37 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
> > index 38951b0..16df2c4 100644
> > --- a/arch/x86/include/asm/fpu/xstate.h
> > +++ b/arch/x86/include/asm/fpu/xstate.h
> > @@ -39,7 +39,6 @@
> >  #define REX_PREFIX
> >  #endif
> >  
> > -extern unsigned int xstate_size;
> >  extern u64 xfeatures_mask;
> >  extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
> >  
> > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> > index 9264476..132b4ca 100644
> > --- a/arch/x86/include/asm/processor.h
> > +++ b/arch/x86/include/asm/processor.h
> > @@ -368,6 +368,7 @@ DECLARE_PER_CPU(struct irq_stack *, softirq_stack);
> >  #endif	/* X86_64 */
> >  
> >  extern unsigned int xstate_size;
> > +extern unsigned int user_xstate_size;
> 
> If this is going to be exported, let's prefix it pls:
> 
> fpu_user_xstate_size
> 
> or
> 
> xstate_user_state_size
> 
> or somesuch.
> 
> And let's add a comment over its definition what exactly it represents.
> I.e., the aspect about the signal frame...
> 
> ...
> 
> > @@ -591,7 +593,15 @@ static bool is_supported_xstate_size(unsigned int test_xstate_size)
> >  static int init_xstate_size(void)
> >  {
> >  	/* Recompute the context size for enabled features: */
> > -	unsigned int possible_xstate_size = calculate_xstate_size();
> > +	unsigned int possible_xstate_size;
> > +	unsigned int xsave_size;
> > +
> > +	xsave_size = get_xsave_size();
> > +
> > +	if (cpu_has_xsaves)
> 
> 	if (boot_cpu_has(X86_FEATURE_XSAVES))
> 
> > +		possible_xstate_size = get_xsaves_size();
> > +	else
> > +		possible_xstate_size = xsave_size;
> >  
> >  	/* Ensure we have the space to store all enabled: */
> >  	if (!is_supported_xstate_size(possible_xstate_size))
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> -- 

This is actually a patch of Fenghua's, but I re-based it.
I will fix it.

Yu-cheng

Re: [PATCH v5 01/13] x86/xsaves: Define and use user_xstate_size for xstate size in signal context

[Borislav Petkov]

On Tue, May 10, 2016 at 08:59:43AM -0700, Yu-cheng Yu wrote:
> This is actually a patch of Fenghua's, but I re-based it.
> I will fix it.

Then please take a look at Documentation/SubmittingPatches too.

Also, please snip the mail text you're quoting if you're not going to
refer to it. Like I just did.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v5 01/13] x86/xsaves: Define and use user_xstate_size for xstate size in signal context

[Yu-cheng Yu]

On Tue, May 10, 2016 at 06:29:00PM +0200, Borislav Petkov wrote:
> Also, please snip the mail text you're quoting if you're not going to
> refer to it. Like I just did.

Ok :-)

[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]

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

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]

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

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]

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.

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]

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

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]

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.

[PATCH v5 03/13] x86/xsaves: Keep init_fpstate.xsave.header.xfeatures as zero for init optimization

[Yu-cheng Yu]

Keep init_fpstate.xsave.header.xfeatures as zero for init optimization.
This is important for init optimization that is implemented in processor.
If a bit corresponding to an xstate in xstate_bv is 0, it means the
xstate is in init status and will not be read from memory to the processor
during XRSTOR/XRSTORS instruction. This largely impacts context switch
performance.

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/kernel/fpu/xstate.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 20c6631..170c164 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -324,13 +324,11 @@ static void __init setup_init_fpu_buf(void)
 	setup_xstate_features();
 	print_xstate_features();
 
-	if (cpu_has_xsaves) {
+	if (cpu_has_xsaves)
 		init_fpstate.xsave.header.xcomp_bv = (u64)1 << 63 | xfeatures_mask;
-		init_fpstate.xsave.header.xfeatures = xfeatures_mask;
-	}
 
 	/*
-	 * Init all the features state with header_bv being 0x0
+	 * Init all the features state with header.xfeatures being 0x0
 	 */
 	copy_kernel_to_xregs_booting(&init_fpstate.xsave);
 
-- 
1.9.1

[PATCH v5 04/13] x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly

[Yu-cheng Yu]

XSAVES is a kernel instruction and uses a compacted format. When working
with user space, the kernel should provide standard-format, non-supervisor
state data. We cannot do __copy_to_user() from a compacted- format kernel
xstate area to a signal frame.

Dave Hansen proposes this method to simplify copy xstate directly to user.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/include/asm/fpu/xstate.h | 1 +
 arch/x86/kernel/fpu/signal.c      | 3 ++-
 arch/x86/kernel/fpu/xstate.c      | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 16df2c4..d812cf3 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -47,5 +47,6 @@ extern void update_regset_xstate_info(unsigned int size, u64 xstate_mask);
 void fpu__xstate_clear_all_cpu_caps(void);
 void *get_xsave_addr(struct xregs_state *xsave, int xstate);
 const void *get_xsave_field_ptr(int xstate_field);
+int using_compacted_format(void);
 
 #endif
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 0fbf60c..d7fdd8c 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -8,6 +8,7 @@
 #include <asm/fpu/internal.h>
 #include <asm/fpu/signal.h>
 #include <asm/fpu/regset.h>
+#include <asm/fpu/xstate.h>
 
 #include <asm/sigframe.h>
 
@@ -167,7 +168,7 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 			sizeof(struct user_i387_ia32_struct), NULL,
 			(struct _fpstate_32 __user *) buf) ? -1 : 1;
 
-	if (fpregs_active()) {
+	if (fpregs_active() || using_compacted_format()) {
 		/* Save the live register state to the user directly. */
 		if (copy_fpregs_to_sigframe(buf_fx))
 			return -1;
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 170c164..2b59bd7 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -415,7 +415,7 @@ static int xfeature_size(int xfeature_nr)
  * that it is obvious which aspect of 'XSAVES' is being handled
  * by the calling code.
  */
-static int using_compacted_format(void)
+int using_compacted_format(void)
 {
 	return cpu_has_xsaves;
 }
-- 
1.9.1

Re: [PATCH v5 04/13] x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly

[Dave Hansen]

On 05/09/2016 01:46 PM, Yu-cheng Yu wrote:
> XSAVES is a kernel instruction and uses a compacted format. When working
> with user space, the kernel should provide standard-format, non-supervisor
> state data. We cannot do __copy_to_user() from a compacted- format kernel
> xstate area to a signal frame.

Looks good now:

Reviewed-by: Dave Hansen <dave.hansen@intel.com>

[PATCH v5 05/13] x86/xsaves: Align xstate components according to CPUID

[Yu-cheng Yu]

CPUID function 0x0d, sub function (i, i > 1) returns in ecx[1] the
alignment requirement of component i when the compacted format is used.

If ecx[1] is 0, component i is located immediately following the preceding
component. If ecx[1] is 1, component i is located on the next 64-byte
boundary following the preceding component.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@intel.com>
---
 arch/x86/kernel/fpu/xstate.c | 60 +++++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 2b59bd7..76ca265 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -265,6 +265,33 @@ static void __init print_xstate_features(void)
 }
 
 /*
+ * This check is important because it is easy to get XSTATE_*
+ * confused with XSTATE_BIT_*.
+ */
+#define CHECK_XFEATURE(nr) do {		\
+	WARN_ON(nr < FIRST_EXTENDED_XFEATURE);	\
+	WARN_ON(nr >= XFEATURE_MAX);	\
+} while (0)
+
+/*
+ * We could cache this like xstate_size[], but we only use
+ * it here, so it would be a waste of space.
+ */
+static int xfeature_is_aligned(int xfeature_nr)
+{
+	u32 eax, ebx, ecx, edx;
+
+	CHECK_XFEATURE(xfeature_nr);
+	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
+	/*
+	 * The value returned by ECX[1] indicates the alignment
+	 * of state component i when the compacted format
+	 * of the extended region of an XSAVE area is used
+	 */
+	return !!(ecx & 2);
+}
+
+/*
  * This function sets up offsets and sizes of all extended states in
  * xsave area. This supports both standard format and compacted format
  * of the xsave aread.
@@ -301,10 +328,14 @@ static void __init setup_xstate_comp(void)
 		else
 			xstate_comp_sizes[i] = 0;
 
-		if (i > FIRST_EXTENDED_XFEATURE)
+		if (i > FIRST_EXTENDED_XFEATURE) {
 			xstate_comp_offsets[i] = xstate_comp_offsets[i-1]
 					+ xstate_comp_sizes[i-1];
 
+			if (xfeature_is_aligned(i))
+				xstate_comp_offsets[i] =
+					ALIGN(xstate_comp_offsets[i], 64);
+		}
 	}
 }
 
@@ -361,33 +392,6 @@ static int xfeature_is_user(int xfeature_nr)
 }
 */
 
-/*
- * This check is important because it is easy to get XSTATE_*
- * confused with XSTATE_BIT_*.
- */
-#define CHECK_XFEATURE(nr) do {		\
-	WARN_ON(nr < FIRST_EXTENDED_XFEATURE);	\
-	WARN_ON(nr >= XFEATURE_MAX);	\
-} while (0)
-
-/*
- * We could cache this like xstate_size[], but we only use
- * it here, so it would be a waste of space.
- */
-static int xfeature_is_aligned(int xfeature_nr)
-{
-	u32 eax, ebx, ecx, edx;
-
-	CHECK_XFEATURE(xfeature_nr);
-	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
-	/*
-	 * The value returned by ECX[1] indicates the alignment
-	 * of state component i when the compacted format
-	 * of the extended region of an XSAVE area is used
-	 */
-	return !!(ecx & 2);
-}
-
 static int xfeature_uncompacted_offset(int xfeature_nr)
 {
 	u32 eax, ebx, ecx, edx;
-- 
1.9.1

[PATCH v5 06/13] x86/xsaves: Supervisor state component offset

[Yu-cheng Yu]

CPUID function 0x0d, sub function (i, i > 1) returns in ebx the offset of
xstate component i. Zero is returned for a supervisor state. A supervisor
state can only be saved by XSAVES and XSAVES uses a compacted format.
There is no fixed offset for a supervisor state. This patch checks and
makes sure a supervisor state offset is not recorded or mis-used. This has
no effect in practice as we currently use no supervisor states, but it
would be good to fix.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@intel.com>
---
 arch/x86/include/asm/fpu/types.h  |  1 +
 arch/x86/include/asm/fpu/xstate.h |  3 ++
 arch/x86/kernel/fpu/xstate.c      | 62 ++++++++++++++++++++++++---------------
 3 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 36b90bb..12dd648 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -122,6 +122,7 @@ enum xfeature {
 #define XFEATURE_MASK_OPMASK		(1 << XFEATURE_OPMASK)
 #define XFEATURE_MASK_ZMM_Hi256		(1 << XFEATURE_ZMM_Hi256)
 #define XFEATURE_MASK_Hi16_ZMM		(1 << XFEATURE_Hi16_ZMM)
+#define XFEATURE_MASK_PT		(1 << XFEATURE_PT_UNIMPLEMENTED_SO_FAR)
 #define XFEATURE_MASK_PKRU		(1 << XFEATURE_PKRU)
 
 #define XFEATURE_MASK_FPSSE		(XFEATURE_MASK_FP | XFEATURE_MASK_SSE)
diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index d812cf3..92f376c 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -18,6 +18,9 @@
 #define XSAVE_YMM_SIZE	    256
 #define XSAVE_YMM_OFFSET    (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
 
+/* Supervisor features */
+#define XFEATURE_MASK_SUPERVISOR (XFEATURE_MASK_PT)
+
 /* Supported features which support lazy state saving */
 #define XFEATURE_MASK_LAZY	(XFEATURE_MASK_FP | \
 				 XFEATURE_MASK_SSE | \
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 76ca265..d21556f 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -107,6 +107,27 @@ int cpu_has_xfeatures(u64 xfeatures_needed, const char **feature_name)
 }
 EXPORT_SYMBOL_GPL(cpu_has_xfeatures);
 
+static int xfeature_is_supervisor(int xfeature_nr)
+{
+	/*
+	 * We currently do not support supervisor states, but if
+	 * we did, we could find out like this.
+	 *
+	 * SDM says: If state component i is a user state component,
+	 * ECX[0] return 0; if state component i is a supervisor
+	 * state component, ECX[0] returns 1.
+	 */
+	u32 eax, ebx, ecx, edx;
+
+	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
+	return !!(ecx & 1);
+}
+
+static int xfeature_is_user(int xfeature_nr)
+{
+	return !xfeature_is_supervisor(xfeature_nr);
+}
+
 /*
  * When executing XSAVEOPT (or other optimized XSAVE instructions), if
  * a processor implementation detects that an FPU state component is still
@@ -225,7 +246,14 @@ static void __init setup_xstate_features(void)
 			continue;
 
 		cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
-		xstate_offsets[i] = ebx;
+
+		/*
+		 * If an xfeature is supervisor state, the offset
+		 * in ebx is invalid. We leave it to -1.
+		 */
+		if (xfeature_is_user(i))
+			xstate_offsets[i] = ebx;
+
 		xstate_sizes[i] = eax;
 		/*
 		 * In our xstate size checks, we assume that the
@@ -370,32 +398,20 @@ static void __init setup_init_fpu_buf(void)
 	copy_xregs_to_kernel_booting(&init_fpstate.xsave);
 }
 
-static int xfeature_is_supervisor(int xfeature_nr)
-{
-	/*
-	 * We currently do not support supervisor states, but if
-	 * we did, we could find out like this.
-	 *
-	 * SDM says: If state component i is a user state component,
-	 * ECX[0] return 0; if state component i is a supervisor
-	 * state component, ECX[0] returns 1.
-	u32 eax, ebx, ecx, edx;
-	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx;
-	return !!(ecx & 1);
-	*/
-	return 0;
-}
-/*
-static int xfeature_is_user(int xfeature_nr)
-{
-	return !xfeature_is_supervisor(xfeature_nr);
-}
-*/
-
 static int xfeature_uncompacted_offset(int xfeature_nr)
 {
 	u32 eax, ebx, ecx, edx;
 
+	/*
+	 * Only XSAVES supports supervisor states and it uses compacted
+	 * format. Checking a supervisor state's uncompacted offset is
+	 * an error.
+	 */
+	if (XFEATURE_MASK_SUPERVISOR & (1 << xfeature_nr)) {
+		WARN_ONCE(1, "No fixed offset for xstate %d\n", xfeature_nr);
+		return -1;
+	}
+
 	CHECK_XFEATURE(xfeature_nr);
 	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
 	return ebx;
-- 
1.9.1

[PATCH v5 07/13] x86/xsaves: Fix PTRACE frames for XSAVES

[Yu-cheng Yu]

XSAVES uses compacted format and is a kernel instruction. The kernel
should use standard-format, non-supervisor state data for PTRACE.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/include/asm/fpu/xstate.h |   5 +-
 arch/x86/kernel/fpu/regset.c      |  56 +++++++++----
 arch/x86/kernel/fpu/xstate.c      | 167 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 212 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 92f376c..ae55a43 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -51,5 +51,8 @@ void fpu__xstate_clear_all_cpu_caps(void);
 void *get_xsave_addr(struct xregs_state *xsave, int xstate);
 const void *get_xsave_field_ptr(int xstate_field);
 int using_compacted_format(void);
-
+int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
+			void __user *ubuf, struct xregs_state *xsave);
+int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
+		     struct xregs_state *xsave);
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index 8bd1c00..46bc63c 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -4,6 +4,7 @@
 #include <asm/fpu/internal.h>
 #include <asm/fpu/signal.h>
 #include <asm/fpu/regset.h>
+#include <asm/fpu/xstate.h>
 
 /*
  * The xstateregs_active() routine is the same as the regset_fpregs_active() routine,
@@ -82,21 +83,30 @@ int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
 	if (!cpu_has_xsave)
 		return -ENODEV;
 
+	xsave = &fpu->state.xsave;
+
 	fpu__activate_fpstate_read(fpu);
 
-	xsave = &fpu->state.xsave;
+	if (using_compacted_format()) {
+		ret = copyout_from_xsaves(pos, count, kbuf, ubuf, xsave);
+	} else {
+		fpstate_sanitize_xstate(fpu);
+
+		/*
+		 * Copy the 48 bytes defined by the software into the xsave
+		 * area in the thread struct, so that we can copy the whole
+		 * area to user using one user_regset_copyout().
+		 */
+		memcpy(&xsave->i387.sw_reserved,
+			xstate_fx_sw_bytes, sizeof(xstate_fx_sw_bytes));
+
+		/*
+		 * Copy the xstate memory layout.
+		 */
+		ret = user_regset_copyout(&pos,
+					  &count, &kbuf, &ubuf, xsave, 0, -1);
+	}
 
-	/*
-	 * Copy the 48bytes defined by the software first into the xstate
-	 * memory layout in the thread struct, so that we can copy the entire
-	 * xstateregs to the user using one user_regset_copyout().
-	 */
-	memcpy(&xsave->i387.sw_reserved,
-		xstate_fx_sw_bytes, sizeof(xstate_fx_sw_bytes));
-	/*
-	 * Copy the xstate memory layout.
-	 */
-	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
 	return ret;
 }
 
@@ -111,11 +121,29 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 	if (!cpu_has_xsave)
 		return -ENODEV;
 
-	fpu__activate_fpstate_write(fpu);
+	/*
+	 * A whole standard-format XSAVE buffer is needed.
+	 */
+	if ((pos != 0) || (count < user_xstate_size))
+		return -EFAULT;
 
 	xsave = &fpu->state.xsave;
 
-	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
+	fpu__activate_fpstate_write(fpu);
+
+	if (boot_cpu_has(X86_FEATURE_XSAVES))
+		ret = copyin_to_xsaves(kbuf, ubuf, xsave);
+	else
+		ret = user_regset_copyin(&pos,
+					 &count, &kbuf, &ubuf, xsave, 0, -1);
+
+	/*
+	 * In case of failure, mark all states as init.
+	 */
+
+	if (ret)
+		fpstate_init(&fpu->state);
+
 	/*
 	 * mxcsr reserved bits must be masked to zero for security reasons.
 	 */
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index d21556f..0ddbc94 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -11,6 +11,7 @@
 #include <asm/fpu/internal.h>
 #include <asm/fpu/signal.h>
 #include <asm/fpu/regset.h>
+#include <asm/fpu/xstate.h>
 
 #include <asm/tlbflush.h>
 
@@ -692,7 +693,13 @@ void __init fpu__init_system_xstate(void)
 		return;
 	}
 
-	update_regset_xstate_info(kernel_xstate_size, xfeatures_mask);
+	/*
+	 * Update info used for ptrace frames; use standard-format size and no
+	 * supervisor xstates.
+	 */
+	update_regset_xstate_info(user_xstate_size,
+		xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR);
+
 	fpu__init_prepare_fx_sw_frame();
 	setup_init_fpu_buf();
 	setup_xstate_comp();
@@ -728,6 +735,7 @@ void *__raw_xsave_addr(struct xregs_state *xsave, int xstate_feature_mask)
 
 	return (void *)xsave + xstate_comp_offsets[feature_nr];
 }
+
 /*
  * Given the xsave area and a state inside, this function returns the
  * address of the state.
@@ -963,3 +971,160 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 
 	return 0;
 }
+
+/*
+ * This is similar to user_regset_copyout(), but will not add offset to
+ * the source data pointer or increment pos, count, kbuf, and ubuf.
+ */
+static inline int xstate_copyout(unsigned int pos, unsigned int count,
+				 void *kbuf, void __user *ubuf,
+				 const void *data, const int start_pos,
+				 const int end_pos)
+{
+	if ((count == 0) || (pos < start_pos))
+		return 0;
+
+	if (end_pos < 0 || pos < end_pos) {
+		unsigned int copy =
+			(end_pos < 0 ? count : min(count, end_pos - pos));
+
+		if (kbuf)
+			memcpy(kbuf + pos, data, copy);
+		else if (__copy_to_user(ubuf + pos, data, copy))
+			return -EFAULT;
+	}
+	return 0;
+}
+
+/*
+ * Convert from kernel XSAVES compacted format to standard format and copy
+ * to a ptrace buffer. It supports partial copy but pos always starts from
+ * zero. This is called from xstateregs_get() and there we check the cpu
+ * has XSAVES.
+ */
+int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
+			void __user *ubuf, struct xregs_state *xsave)
+{
+	unsigned int offset, size;
+	int ret, i;
+	struct xstate_header header;
+
+	/*
+	 * Currently copy_regset_to_user() starts from pos 0.
+	 */
+	if (unlikely(pos != 0))
+		return -EFAULT;
+
+	/*
+	 * The destination is a ptrace buffer; we put in only user xstates.
+	 */
+	memset(&header, 0, sizeof(header));
+	header.xfeatures = xsave->header.xfeatures;
+	header.xfeatures &= ~XFEATURE_MASK_SUPERVISOR;
+
+	/*
+	 * Copy xregs_state->header.
+	 */
+	offset = offsetof(struct xregs_state, header);
+	size = sizeof(header);
+
+	ret = xstate_copyout(offset, size, kbuf, ubuf, &header, 0, count);
+
+	if (ret)
+		return ret;
+
+	for (i = 0; i < XFEATURE_MAX; i++) {
+		/*
+		 * Copy only in-use xstates.
+		 */
+		if ((header.xfeatures >> i) & 1) {
+			void *src = __raw_xsave_addr(xsave, 1 << i);
+
+			offset = xstate_offsets[i];
+			size = xstate_sizes[i];
+
+			ret = xstate_copyout(offset, size, kbuf, ubuf, src, 0,
+					     count);
+
+			if (ret)
+				return ret;
+
+			if (offset + size >= count)
+				break;
+		}
+	}
+
+	/*
+	 * Fill xsave->i387.sw_reserved value for ptrace frame.
+	 */
+	offset = offsetof(struct fxregs_state, sw_reserved);
+	size = sizeof(xstate_fx_sw_bytes);
+
+	ret = xstate_copyout(offset, size, kbuf, ubuf, xstate_fx_sw_bytes, 0,
+			     count);
+
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * Convert from a ptrace standard-format buffer to kernel XSAVES format
+ * and copy to the target thread. This is called from xstateregs_set() and
+ * there we check the cpu has XSAVES and a whole standard-sized buffer
+ * exists.
+ */
+int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
+		     struct xregs_state *xsave)
+{
+	unsigned int offset, size;
+	int i;
+	u64 xfeatures;
+	u64 allowed_features;
+
+	offset = offsetof(struct xregs_state, header);
+	size = sizeof(xfeatures);
+
+	if (kbuf)
+		memcpy(&xfeatures, kbuf + offset, size);
+	else if (__copy_from_user(&xfeatures, ubuf + offset, size))
+		return -EFAULT;
+
+	/*
+	 * Reject if the user sets any forbidden features.
+	 */
+	allowed_features = xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR;
+
+	if (xfeatures & ~allowed_features)
+		return -EINVAL;
+
+	for (i = 0; i < XFEATURE_MAX; i++) {
+		u64 mask = ((u64)1 << i);
+
+		if (xfeatures & mask) {
+			void *dst = __raw_xsave_addr(xsave, 1 << i);
+
+			offset = xstate_offsets[i];
+			size = xstate_sizes[i];
+
+			if (kbuf)
+				memcpy(dst, kbuf + offset, size);
+			else if (__copy_from_user(dst, ubuf + offset, size))
+				return -EFAULT;
+		}
+	}
+
+	/*
+	 * The state that came in from userspace was user-state only.
+	 * Mask all the user states out of 'xfeatures'.
+	 */
+	xsave->header.xfeatures &= XFEATURE_MASK_SUPERVISOR;
+
+	/*
+	 * Add back in the features that came in from userspace.
+	 */
+	xsave->header.xfeatures |= xfeatures;
+
+	return 0;
+}
-- 
1.9.1

[PATCH v5 08/13] x86/xsaves: Fix XSTATE component offset print out

[Yu-cheng Yu]

Component offset print out was incorrect for XSAVES. Correct it and move
to a separate function.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@intel.com>
---
 arch/x86/kernel/fpu/xstate.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 0ddbc94..e74a6b6 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -264,8 +264,6 @@ static void __init setup_xstate_features(void)
 		WARN_ONCE(last_good_offset > xstate_offsets[i],
 			"x86/fpu: misordered xstate at %d\n", last_good_offset);
 		last_good_offset = xstate_offsets[i];
-
-		printk(KERN_INFO "x86/fpu: xstate_offset[%d]: %4d, xstate_sizes[%d]: %4d\n", i, ebx, i, eax);
 	}
 }
 
@@ -369,6 +367,21 @@ static void __init setup_xstate_comp(void)
 }
 
 /*
+ * Print out xstate component offsets and sizes
+ */
+static void __init print_xstate_offset_size(void)
+{
+	int i;
+
+	for (i = FIRST_EXTENDED_XFEATURE; i < XFEATURE_MAX; i++) {
+		if (!xfeature_enabled(i))
+			continue;
+		pr_info("x86/fpu: xstate_offset[%d]: %4d, xstate_sizes[%d]: %4d\n",
+			 i, xstate_comp_offsets[i], i, xstate_sizes[i]);
+	}
+}
+
+/*
  * setup the xstate image representing the init state
  */
 static void __init setup_init_fpu_buf(void)
@@ -703,6 +716,7 @@ void __init fpu__init_system_xstate(void)
 	fpu__init_prepare_fx_sw_frame();
 	setup_init_fpu_buf();
 	setup_xstate_comp();
+	print_xstate_offset_size();
 
 	pr_info("x86/fpu: Enabled xstate features 0x%llx, context size is %d bytes, using '%s' format.\n",
 		xfeatures_mask,
-- 
1.9.1

[PATCH v5 09/13] x86/xsaves: Fix xstate_offsets, xstate_sizes for non-extended states

[Yu-cheng Yu]

The arrays xstate_offsets[] and xstate_sizes[] record XSAVE standard-
format offsets and sizes. Values for non-extended state components
fpu and xmm's were not initialized or used. Ptrace format conversion
needs them. Fix it.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@intel.com>
---
 arch/x86/kernel/fpu/xstate.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index e74a6b6..350814c 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -241,6 +241,15 @@ static void __init setup_xstate_features(void)
 	/* start at the beginnning of the "extended state" */
 	unsigned int last_good_offset = offsetof(struct xregs_state,
 						 extended_state_area);
+	/*
+	 * The FP xstates and SSE xstates are legacy states. They are always
+	 * in the fixed offsets in the xsave area in either compacted form
+	 * or standard form.
+	 */
+	xstate_offsets[0] = 0;
+	xstate_sizes[0] = offsetof(struct fxregs_state, xmm_space);
+	xstate_offsets[1] = xstate_sizes[0];
+	xstate_sizes[1] = FIELD_SIZEOF(struct fxregs_state, xmm_space);
 
 	for (i = FIRST_EXTENDED_XFEATURE; i < XFEATURE_MAX; i++) {
 		if (!xfeature_enabled(i))
-- 
1.9.1

[PATCH v5 10/13] x86/xsaves: Fix __fpu_restore_sig() for XSAVES

[Yu-cheng Yu]

When the kernel is using XSAVES compacted format, we cannot do
__copy_from_user() from a signal frame, which has standard-format data.
Fix it by using copyin_to_xsaves().

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/kernel/fpu/signal.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index d7fdd8c..9c2ff42 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -320,8 +320,15 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		 */
 		fpu__drop(fpu);
 
-		if (__copy_from_user(&fpu->state.xsave, buf_fx, state_size) ||
-		    __copy_from_user(&env, buf, sizeof(env))) {
+		if (using_compacted_format()) {
+			err = copyin_to_xsaves(NULL, buf_fx,
+					       &fpu->state.xsave);
+		} else {
+			err = __copy_from_user(&fpu->state.xsave,
+					       buf_fx, state_size);
+		}
+
+		if (err || __copy_from_user(&env, buf, sizeof(env))) {
 			fpstate_init(&fpu->state);
 			err = -1;
 		} else {
-- 
1.9.1

Re: [PATCH v5 10/13] x86/xsaves: Fix __fpu_restore_sig() for XSAVES

[Dave Hansen]

On 05/09/2016 01:46 PM, Yu-cheng Yu wrote:
> When the kernel is using XSAVES compacted format, we cannot do
> __copy_from_user() from a signal frame, which has standard-format data.
> Fix it by using copyin_to_xsaves().
... which converts between formats and filters out all supervisor state,
which we do not want to allow userspace to write.

Reviewed-by: Dave Hansen <dave.hansen@intel.com>

[PATCH v5 11/13] x86/xsaves: Add WARN_ON_FPU() when a disabled xstate component offset is requested for a compacted format

[Yu-cheng Yu]

Add a warning in case a disabled (not existing) xstate component offset
is requested.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/kernel/fpu/xstate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 350814c..2e6dbfe 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -756,6 +756,7 @@ void *__raw_xsave_addr(struct xregs_state *xsave, int xstate_feature_mask)
 {
 	int feature_nr = fls64(xstate_feature_mask) - 1;
 
+	WARN_ON_FPU(using_compacted_format() && !xfeature_enabled(feature_nr));
 	return (void *)xsave + xstate_comp_offsets[feature_nr];
 }
 
-- 
1.9.1

Re: [PATCH v5 11/13] x86/xsaves: Add WARN_ON_FPU() when a disabled xstate component offset is requested for a compacted format

[Dave Hansen]

On 05/09/2016 01:46 PM, Yu-cheng Yu wrote:
> Add a warning in case a disabled (not existing) xstate component offset
> is requested.
...
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 350814c..2e6dbfe 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -756,6 +756,7 @@ void *__raw_xsave_addr(struct xregs_state *xsave, int xstate_feature_mask)
>  {
>  	int feature_nr = fls64(xstate_feature_mask) - 1;
>  
> +	WARN_ON_FPU(using_compacted_format() && !xfeature_enabled(feature_nr));
>  	return (void *)xsave + xstate_comp_offsets[feature_nr];
>  }

Why the using_compacted_format()?  Shouldn't this be an error, regardless.

Also, what is xstate_comp_offsets[feature_nr] in this case?  Isn't it
-1?  Should we be returning NULL along with WARN()ing?

Re: [PATCH v5 11/13] x86/xsaves: Add WARN_ON_FPU() when a disabled xstate component offset is requested for a compacted format

[Yu-cheng Yu]

On Mon, May 09, 2016 at 04:31:18PM -0700, Dave Hansen wrote:
> On 05/09/2016 01:46 PM, Yu-cheng Yu wrote:
> > Add a warning in case a disabled (not existing) xstate component offset
> > is requested.
> ...
> > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> > index 350814c..2e6dbfe 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -756,6 +756,7 @@ void *__raw_xsave_addr(struct xregs_state *xsave, int xstate_feature_mask)
> >  {
> >  	int feature_nr = fls64(xstate_feature_mask) - 1;
> >  
> > +	WARN_ON_FPU(using_compacted_format() && !xfeature_enabled(feature_nr));
> >  	return (void *)xsave + xstate_comp_offsets[feature_nr];
> >  }
> 
> Why the using_compacted_format()?  Shouldn't this be an error, regardless.

If the kernel is not using compacted format, I can get a component offset, no? 

> 
> Also, what is xstate_comp_offsets[feature_nr] in this case?  Isn't it
> -1?  Should we be returning NULL along with WARN()ing?

NULL makes sense. I will change it.

Re: [PATCH v5 11/13] x86/xsaves: Add WARN_ON_FPU() when a disabled xstate component offset is requested for a compacted format

[Dave Hansen]

On 05/09/2016 04:44 PM, Yu-cheng Yu wrote:
> On Mon, May 09, 2016 at 04:31:18PM -0700, Dave Hansen wrote:
>> On 05/09/2016 01:46 PM, Yu-cheng Yu wrote:
>>> Add a warning in case a disabled (not existing) xstate component offset
>>> is requested.
>> ...
>>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>>> index 350814c..2e6dbfe 100644
>>> --- a/arch/x86/kernel/fpu/xstate.c
>>> +++ b/arch/x86/kernel/fpu/xstate.c
>>> @@ -756,6 +756,7 @@ void *__raw_xsave_addr(struct xregs_state *xsave, int xstate_feature_mask)
>>>  {
>>>  	int feature_nr = fls64(xstate_feature_mask) - 1;
>>>  
>>> +	WARN_ON_FPU(using_compacted_format() && !xfeature_enabled(feature_nr));
>>>  	return (void *)xsave + xstate_comp_offsets[feature_nr];
>>>  }
>>
>> Why the using_compacted_format()?  Shouldn't this be an error, regardless.
> 
> If the kernel is not using compacted format, I can get a component offset, no? 

You can get it, but why would you?  Let's say you were trying to get the
MPX contents.  You'd either be guaranteed to be getting 0's or
uninitialized garbage (if we didn't zero it carefully).

The garbage could be kernel data (if we didn't zero carefully).  So it
just seems dangerous to allow this for no apparent benefit.

[PATCH v5 12/13] x86/xsaves: Fix fpstate_init() for XSAVES

[Yu-cheng Yu]

When XSAVES is used, xsave.header.xcomp_bv[63] must be set.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/kernel/fpu/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 41ab106..25e2605 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -224,6 +224,9 @@ void fpstate_init(union fpregs_state *state)
 
 	memset(state, 0, kernel_xstate_size);
 
+	if (cpu_has_xsaves)
+		state->xsave.header.xcomp_bv = (u64)1 << 63;
+
 	if (cpu_has_fxsr)
 		fpstate_init_fxstate(&state->fxsave);
 	else
-- 
1.9.1

Re: [PATCH v5 12/13] x86/xsaves: Fix fpstate_init() for XSAVES

[Dave Hansen]

On 05/09/2016 01:46 PM, Yu-cheng Yu wrote:
> When XSAVES is used, xsave.header.xcomp_bv[63] must be set.

So, that's not strictly correct.  XSAVES can write to a completely empty
(0'd) memory buffer.  It's only XRSTORS that _needs_ bit 63 set.  The
instruction reference is pretty clear on this point.

Oh, and if you decided to do this for some reason, please have mercy and
go make a macro for 1<<63.

Also, I don't think the kernel ever checks for this bit.  So are we
really calling XRSTORS on otherwise uninitialized xsave buffers?

Re: [PATCH v5 12/13] x86/xsaves: Fix fpstate_init() for XSAVES

[Yu-cheng Yu]

On Mon, May 09, 2016 at 04:41:31PM -0700, Dave Hansen wrote:
> On 05/09/2016 01:46 PM, Yu-cheng Yu wrote:
> > When XSAVES is used, xsave.header.xcomp_bv[63] must be set.
> 
> So, that's not strictly correct.  XSAVES can write to a completely empty
> (0'd) memory buffer.  It's only XRSTORS that _needs_ bit 63 set.  The
> instruction reference is pretty clear on this point.

You mean the comments?  I will change it to XRSTORS.

> 
> Oh, and if you decided to do this for some reason, please have mercy and
> go make a macro for 1<<63.
> 
> Also, I don't think the kernel ever checks for this bit.  So are we
> really calling XRSTORS on otherwise uninitialized xsave buffers?

Right now if we do fpstate_init(), without this patch, it will 
trigger a warning from copy_kernel_to_fxregs() when the task is
scheduled.

Re: [PATCH v5 12/13] x86/xsaves: Fix fpstate_init() for XSAVES

[Dave Hansen]

On 05/09/2016 04:50 PM, Yu-cheng Yu wrote:
>> > 
>> > Oh, and if you decided to do this for some reason, please have mercy and
>> > go make a macro for 1<<63.
>> > 
>> > Also, I don't think the kernel ever checks for this bit.  So are we
>> > really calling XRSTORS on otherwise uninitialized xsave buffers?
> Right now if we do fpstate_init(), without this patch, it will 
> trigger a warning from copy_kernel_to_fxregs() when the task is
> scheduled.

Please include this in the patch description.  It's a pretty important
piece of justification.  Did you mean "copy_kernel_to_xregs()" without
the "f"?

I think we should probably also have an _explicit_ FPU_WARN_ON() in the
XRSTORS path for this.  *Both* the booting and regular ones, btw...

[PATCH v5 13/13] x86/xsaves: Re-enable XSAVES

[Yu-cheng Yu]

We did not handle XSAVES instructions correctly. There were issues in
converting between standard and compacted format when interfacing with
user-space. These issues have been corrected.

Add a WARN_ONCE() to make it clear that XSAVES supervisor states are not
yet implemented.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/kernel/fpu/init.c   | 15 ---------------
 arch/x86/kernel/fpu/xstate.c | 10 ++++++++++
 2 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 549ff59..cdd45ac 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -228,21 +228,6 @@ static void __init fpu__init_system_xstate_size_legacy(void)
 	}
 
 	user_xstate_size = kernel_xstate_size;
-
-	/*
-	 * Quirk: we don't yet handle the XSAVES* instructions
-	 * correctly, as we don't correctly convert between
-	 * standard and compacted format when interfacing
-	 * with user-space - so disable it for now.
-	 *
-	 * The difference is small: with recent CPUs the
-	 * compacted format is only marginally smaller than
-	 * the standard FPU state format.
-	 *
-	 * ( This is easy to backport while we are fixing
-	 *   XSAVES* support. )
-	 */
-	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
 }
 
 /*
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 2e6dbfe..80c6d87 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -217,6 +217,16 @@ void fpu__init_cpu_xstate(void)
 	if (!cpu_has_xsave || !xfeatures_mask)
 		return;
 
+	/*
+	 * Make it clear that XSAVES supervisor states are not yet
+	 * implemented should anyone expect it to work by changing
+	 * bits in XFEATURE_MASK_* macros and XCR0.
+	 */
+	WARN_ONCE((xfeatures_mask & XFEATURE_MASK_SUPERVISOR),
+		"x86/fpu: XSAVES supervisor states are not yet implemented.\n");
+
+	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;
+
 	cr4_set_bits(X86_CR4_OSXSAVE);
 	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
 }
-- 
1.9.1