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

Groups > linux.kernel > #1346039 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/9] x86/xsaves: Fix XSAVES known issues Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-02-29 18:50 +0100
    [PATCH v3 4/9] x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-02-29 18:50 +0100
    [PATCH v3 3/9] x86/xsaves: Keep init_fpstate.xsave.header.xfeatures as zero for init optimization Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-02-29 18:50 +0100
    [PATCH v3 5/9] x86/xsaves: Align xstate components according to CPUID Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-02-29 18:50 +0100

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

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

[toc] | [next] | [standalone]

#1346041 — [PATCH v3 4/9] x86/xsaves: Introduce a new check that allows correct xstates copy from kernel to user directly

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

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

#1346042 — [PATCH v3 3/9] x86/xsaves: Keep init_fpstate.xsave.header.xfeatures as zero for init optimization

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

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

#1346043 — [PATCH v3 5/9] x86/xsaves: Align xstate components according to CPUID

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web