[PATCH v3 0/9] x86/xsaves: Fix XSAVES known issues

[PATCH v3 0/9] x86/xsaves: Fix XSAVES known issues

[Yu-cheng Yu]

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

Version 3 adds a WARN_ONCE() in patch 9 to make it clear XSAVES supervisor
states are not yet supported.

Version 2 fixes a mistake in handling supervisor states of ptrace function
copyin_to_xsaves() and some coding style/naming issues in the first version.
It also limits XSAVES only to 64-bit kernel in patch 9.

Patch 1, 2, and 4 are for converting between kernel-mode xstate area and
signal frames.

Patch 3 fixes optimization issues introduced by XSAVES to the buffer
init_fpstate.

Patch 5 and 6 are related to xstate component offsets.

Patch 7 is for converting between kernel-mode xstate area and ptrace
frames.

Patch 8 fixes xstate area print out.

Patch 9 re-enables XSAVES.

Yu-cheng Yu (9):
  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: Re-enable XSAVES

 arch/x86/include/asm/fpu/types.h  |   2 +
 arch/x86/include/asm/fpu/xstate.h |   8 +-
 arch/x86/include/asm/processor.h  |   3 +-
 arch/x86/kernel/fpu/core.c        |   6 +-
 arch/x86/kernel/fpu/init.c        |  35 ++--
 arch/x86/kernel/fpu/regset.c      |  56 ++++--
 arch/x86/kernel/fpu/signal.c      |  69 ++++++-
 arch/x86/kernel/fpu/xstate.c      | 400 +++++++++++++++++++++++++++++---------
 8 files changed, 443 insertions(+), 136 deletions(-)

-- 
1.9.1

[PATCH v3 1/9] 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 af30fde..c6667f2 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 20c11d1..01e127e 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -366,6 +366,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 6d9f0a7..4ac2561 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -193,7 +193,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.
@@ -224,6 +224,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 d425cda5..9d41c0f 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -35,6 +35,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.
@@ -159,7 +161,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,
@@ -518,8 +520,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
@@ -529,34 +532,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;
 }
 
 /*
@@ -576,7 +578,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))
@@ -588,6 +598,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

[PATCH v3 2/9] 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 01e127e..68a5fa4 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -365,7 +365,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 d25097c..faa00c0 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -214,7 +214,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);
@@ -238,7 +238,7 @@ static void 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
@@ -258,7 +258,7 @@ static void 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);
 		fpregs_deactivate(src_fpu);
 	}
 	preempt_enable();
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 4ac2561..b5952d5 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -143,8 +143,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)
@@ -176,7 +176,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
@@ -193,7 +193,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.
@@ -206,7 +206,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().
 	 */
 
@@ -217,15 +217,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 9d41c0f..b8d6d98 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -517,7 +517,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);
 }
 
 
@@ -596,7 +596,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();
 
 	/*
@@ -659,14 +659,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

[PATCH v3 3/9] 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 b8d6d98..25e11a1 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -311,13 +311,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 v3 4/9] 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.

Note that the path to copy_fpstate_to_sigframe() does currently check if
the thread has used FPU, but add a WARN_ONCE() there to detect any
potential mis-use.

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/kernel/fpu/signal.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 0fbf60c..09945f1 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -130,6 +130,45 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
 	return err;
 }
 
+static int may_copy_fpregs_to_sigframe(void)
+{
+	/*
+	 * In signal handling path, the kernel already checks if
+	 * FPU instructions have been used before it calls
+	 * copy_fpstate_to_sigframe(). We check this here again
+	 * to detect any potential mis-use and saving invalid
+	 * register values directly to a signal frame.
+	 */
+	WARN_ONCE(!current->thread.fpu.fpstate_active,
+		  "direct FPU save with no math use\n");
+
+	/*
+	 * In the case that we are using a compacted kernel
+	 * xsave area, we can not copy the thread.fpu.state
+	 * directly to userspace and *must* save it from the
+	 * registers directly.
+	 */
+	if (boot_cpu_has(X86_FEATURE_XSAVES))
+		return 1;
+
+	/*
+	 * fpregs_active() means "Can I use the FPU hardware
+	 * without taking a device-not-available exception?" This
+	 * means that saving the registers directly will be
+	 * cheaper than copying their contents out of
+	 * thread.fpu.state.
+	 *
+	 * Note that fpregs_active() is inherently racy and may
+	 * become false at any time.  If this race happens, we
+	 * will take a harmless device-not-available exception
+	 * when we attempt the FPU save instruction.
+	 */
+	if (fpregs_active())
+		return 1;
+
+	return 0;
+}
+
 /*
  * Save the fpu, extended register state to the user signal frame.
  *
@@ -167,7 +206,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 (may_copy_fpregs_to_sigframe()) {
 		/* Save the live register state to the user directly. */
 		if (copy_fpregs_to_sigframe(buf_fx))
 			return -1;
-- 
1.9.1

[PATCH v3 5/9] 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>
---
 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 25e11a1..6e42b87 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -252,6 +252,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.
@@ -288,10 +315,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);
+		}
 	}
 }
 
@@ -348,33 +379,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 v3 6/9] 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>
---
 arch/x86/include/asm/fpu/types.h  |  2 ++
 arch/x86/include/asm/fpu/xstate.h |  3 ++
 arch/x86/kernel/fpu/xstate.c      | 62 ++++++++++++++++++++++++---------------
 3 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 1c6f6ac..11466cf 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -108,6 +108,7 @@ enum xfeature {
 	XFEATURE_OPMASK,
 	XFEATURE_ZMM_Hi256,
 	XFEATURE_Hi16_ZMM,
+	XFEATURE_PT,
 
 	XFEATURE_MAX,
 };
@@ -120,6 +121,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)
 
 #define XFEATURE_MASK_FPSSE		(XFEATURE_MASK_FP | XFEATURE_MASK_SSE)
 #define XFEATURE_MASK_AVX512		(XFEATURE_MASK_OPMASK \
diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index c6667f2..b4f5d94 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 6e42b87..aaab0d3 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -95,6 +95,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
@@ -213,7 +234,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
@@ -357,32 +385,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 v3 7/9] 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 |   4 +
 arch/x86/kernel/fpu/regset.c      |  56 +++++++++---
 arch/x86/kernel/fpu/xstate.c      | 173 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 217 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index b4f5d94..a5cd808 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -50,5 +50,9 @@ 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 copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
+			void __user *ubuf, const 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 0bc3490..61fe8e9 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 (boot_cpu_has(X86_FEATURE_XSAVES)) {
+		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 aaab0d3..b9d4d59 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -677,7 +677,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();
@@ -701,6 +707,15 @@ void fpu__resume_cpu(void)
 }
 
 /*
+ * Get an xstate component address for either kernel or user mode.
+ */
+static void *get_xsave_addr_no_check(const struct xregs_state *xsave,
+				     int feature_nr)
+{
+	return (void *)xsave + xstate_comp_offsets[feature_nr];
+}
+
+/*
  * Given the xsave area and a state inside, this function returns the
  * address of the state.
  *
@@ -748,7 +763,7 @@ void *get_xsave_addr(struct xregs_state *xsave, int xstate_feature)
 	if (!(xsave->header.xfeatures & xstate_feature))
 		return NULL;
 
-	return (void *)xsave + xstate_comp_offsets[feature_nr];
+	return get_xsave_addr_no_check(xsave, feature_nr);
 }
 EXPORT_SYMBOL_GPL(get_xsave_addr);
 
@@ -783,3 +798,157 @@ const void *get_xsave_field_ptr(int xsave_state)
 
 	return get_xsave_addr(&fpu->state.xsave, xsave_state);
 }
+
+/*
+ * 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, const 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) && xfeature_enabled(i)) {
+			void *src = get_xsave_addr_no_check(xsave, 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;
+
+	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 tries to set any supervisor xstates.
+	 */
+	if (xfeatures & XFEATURE_MASK_SUPERVISOR)
+		return -EINVAL;
+
+	for (i = 0; i < XFEATURE_MAX; i++) {
+		u64 mask = ((u64)1 << i);
+
+		if ((xfeatures & mask) && xfeature_enabled(i)) {
+			void *dst = get_xsave_addr_no_check(xsave, 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 v3 8/9] 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>
---
 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 b9d4d59..2e80d6f 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -251,8 +251,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);
 	}
 }
 
@@ -355,6 +353,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)
@@ -687,6 +700,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 v3 9/9] 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   | 16 ++++------------
 arch/x86/kernel/fpu/xstate.c |  8 ++++++++
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index b5952d5..21e0d52 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -228,19 +228,11 @@ 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. )
+	 * Most recent CPUs supporting XSAVES can run 64-bit mode.
+	 * Enable XSAVES for 64-bit.
 	 */
-	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
+	if (!config_enabled(CONFIG_X86_64))
+		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
 }
 
 /*
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 2e80d6f..cb2a484 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -204,6 +204,14 @@ 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");
+
 	cr4_set_bits(X86_CR4_OSXSAVE);
 	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
 }
-- 
1.9.1

Re: [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES

[Dave Hansen]

On 02/29/2016 09:42 AM, Yu-cheng Yu wrote:
>  	/*
> -	 * 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. )
> +	 * Most recent CPUs supporting XSAVES can run 64-bit mode.
> +	 * Enable XSAVES for 64-bit.
>  	 */
> -	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
> +	if (!config_enabled(CONFIG_X86_64))
> +		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
>  }

I think we need a much better explanation of this for posterity.  Why
are we not supporting this now, and what would someone have to do in the
future in order to enable it?

>  /*
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 2e80d6f..cb2a484 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -204,6 +204,14 @@ 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");
> +
>  	cr4_set_bits(X86_CR4_OSXSAVE);
>  	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
>  }

Let's also do a:

	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;

Otherwise, we have a broken system at the moment.

Re: [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES

[Yu-cheng Yu]

On Tue, Mar 01, 2016 at 03:56:12PM -0800, Dave Hansen wrote:
> On 02/29/2016 09:42 AM, Yu-cheng Yu wrote:
> >  	/*
> > -	 * 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. )
> > +	 * Most recent CPUs supporting XSAVES can run 64-bit mode.
> > +	 * Enable XSAVES for 64-bit.
> >  	 */
> > -	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
> > +	if (!config_enabled(CONFIG_X86_64))
> > +		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
> >  }
> 
> I think we need a much better explanation of this for posterity.  Why
> are we not supporting this now, and what would someone have to do in the
> future in order to enable it?
> 
If anyone is using this newer feature, then that user is most likely using
a 64-bit capable processor and a 64-bit kernel. The intention here is to
take out the complexity and any potential of error. If the user removes 
the restriction and builds a private kernel, it should work but we have
not checked all possible combinations. I will put these in the comments.

> >  /*
> > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> > index 2e80d6f..cb2a484 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -204,6 +204,14 @@ 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");
> > +
> >  	cr4_set_bits(X86_CR4_OSXSAVE);
> >  	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
> >  }
> 
> Let's also do a:
> 
> 	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;
> 
> Otherwise, we have a broken system at the moment.
> 
Currently, if anyone sets any supervisor state in xfeatures_mask, the
kernel prints out the warning then goes into a protection fault.
That is a very strong indication to the user. Do we want to mute it? 

Yu-cheng

Re: [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES

[Dave Hansen]

On 03/01/2016 04:34 PM, Yu-cheng Yu wrote:
> On Tue, Mar 01, 2016 at 03:56:12PM -0800, Dave Hansen wrote:
>> On 02/29/2016 09:42 AM, Yu-cheng Yu wrote:
>>> -	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
>>> +	if (!config_enabled(CONFIG_X86_64))
>>> +		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
>>>  }
>>
>> I think we need a much better explanation of this for posterity.  Why
>> are we not supporting this now, and what would someone have to do in the
>> future in order to enable it?
>>
> If anyone is using this newer feature, then that user is most likely using
> a 64-bit capable processor and a 64-bit kernel. The intention here is to
> take out the complexity and any potential of error. If the user removes 
> the restriction and builds a private kernel, it should work but we have
> not checked all possible combinations. I will put these in the comments.

A user can go download a 32-bit version of Ubuntu or Debian and install
it on a 64-bit processor today.  It's a very easy mistake to make when
downloading the install CD.

In any case, I don't have a _problem_ with leaving i386 in the dust
here.  I just want us to be very explicit about what we are doing.

>>> +	/*
>>> +	 * 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");
>>> +
>>>  	cr4_set_bits(X86_CR4_OSXSAVE);
>>>  	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
>>>  }
>>
>> Let's also do a:
>>
>> 	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;
>>
>> Otherwise, we have a broken system at the moment.
>>
> Currently, if anyone sets any supervisor state in xfeatures_mask, the
> kernel prints out the warning then goes into a protection fault.
> That is a very strong indication to the user. Do we want to mute it? 

By "goes into a protection fault", do you mean that it doesn't boot?

I'd just rather we put the kernel in a known-safe configuration (masking
supervisor state out of xfeatures_mask) rather than rely on the general
protection fault continuing to be generated by whatever is generating it.

Re: [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES

[Yu-cheng Yu]

On Tue, Mar 01, 2016 at 04:45:41PM -0800, Dave Hansen wrote:
> >>> +	WARN_ONCE((xfeatures_mask & XFEATURE_MASK_SUPERVISOR),
> >>> +		"x86/fpu: XSAVES supervisor states are not yet implemented.\n");
> >>> +
> >>>  	cr4_set_bits(X86_CR4_OSXSAVE);
> >>>  	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
> >>>  }
> >>
> >> Let's also do a:
> >>
> >> 	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;
> >>
> >> Otherwise, we have a broken system at the moment.
> >>
> > Currently, if anyone sets any supervisor state in xfeatures_mask, the
> > kernel prints out the warning then goes into a protection fault.
> > That is a very strong indication to the user. Do we want to mute it? 
> 
> By "goes into a protection fault", do you mean that it doesn't boot?
> 
> I'd just rather we put the kernel in a known-safe configuration (masking
> supervisor state out of xfeatures_mask) rather than rely on the general
> protection fault continuing to be generated by whatever is generating it.
> 
Ok.

Yu-cheng

Re: [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES

[H. Peter Anvin]

On March 1, 2016 4:45:41 PM PST, Dave Hansen <dave.hansen@linux.intel.com> wrote:
>On 03/01/2016 04:34 PM, Yu-cheng Yu wrote:
>> On Tue, Mar 01, 2016 at 03:56:12PM -0800, Dave Hansen wrote:
>>> On 02/29/2016 09:42 AM, Yu-cheng Yu wrote:
>>>> -	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
>>>> +	if (!config_enabled(CONFIG_X86_64))
>>>> +		setup_clear_cpu_cap(X86_FEATURE_XSAVES);
>>>>  }
>>>
>>> I think we need a much better explanation of this for posterity. 
>Why
>>> are we not supporting this now, and what would someone have to do in
>the
>>> future in order to enable it?
>>>
>> If anyone is using this newer feature, then that user is most likely
>using
>> a 64-bit capable processor and a 64-bit kernel. The intention here is
>to
>> take out the complexity and any potential of error. If the user
>removes 
>> the restriction and builds a private kernel, it should work but we
>have
>> not checked all possible combinations. I will put these in the
>comments.
>
>A user can go download a 32-bit version of Ubuntu or Debian and install
>it on a 64-bit processor today.  It's a very easy mistake to make when
>downloading the install CD.
>
>In any case, I don't have a _problem_ with leaving i386 in the dust
>here.  I just want us to be very explicit about what we are doing.
>
>>>> +	/*
>>>> +	 * 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");
>>>> +
>>>>  	cr4_set_bits(X86_CR4_OSXSAVE);
>>>>  	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
>>>>  }
>>>
>>> Let's also do a:
>>>
>>> 	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;
>>>
>>> Otherwise, we have a broken system at the moment.
>>>
>> Currently, if anyone sets any supervisor state in xfeatures_mask, the
>> kernel prints out the warning then goes into a protection fault.
>> That is a very strong indication to the user. Do we want to mute it? 
>
>By "goes into a protection fault", do you mean that it doesn't boot?
>
>I'd just rather we put the kernel in a known-safe configuration
>(masking
>supervisor state out of xfeatures_mask) rather than rely on the general
>protection fault continuing to be generated by whatever is generating
>it.

Differences between i386 and x86-64 generally add problems, so unless this requires significant 32-bit-specific code we should not exclude i386 just because.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: [PATCH v3 9/9] x86/xsaves: Re-enable XSAVES

[Yu-cheng Yu]

On Tue, Mar 01, 2016 at 04:53:53PM -0800, H. Peter Anvin wrote:
> Differences between i386 and x86-64 generally add problems, so unless this requires significant 32-bit-specific code we should not exclude i386 just because.

I have not seen any issues with 32-bit code, but will do some tests.
Thanks.

Yu-cheng