[PATCH 1/2] x86, fpu: use kvm definition of compaction bit and make it generic

[PATCH 1/2] x86, fpu: use kvm definition of compaction bit and make it generic

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

We will use this in non-KVM code in the next patch.

Note that we are also renaming the macro.  It is not really
an 'XSTATE' like would go in XSTATE_BV, or be used as part
of the "instruction mask".  It goes in XCOMP_BV, so make
that explicit in the name.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: x86@kernel.org
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andy Lutomirski <luto@amacapital.net>
---

 b/arch/x86/include/asm/xsave.h |    2 ++
 b/arch/x86/kvm/x86.c           |    7 +++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff -puN arch/x86/include/asm/xsave.h~x86-fpu-take-kvm-def-and-make-generic arch/x86/include/asm/xsave.h
--- a/arch/x86/include/asm/xsave.h~x86-fpu-take-kvm-def-and-make-generic	2015-04-28 09:15:28.503683137 -0700
+++ b/arch/x86/include/asm/xsave.h	2015-04-28 09:15:28.522683994 -0700
@@ -38,6 +38,8 @@
 /* All currently supported features */
 #define XCNTXT_MASK	(XSTATE_LAZY | XSTATE_EAGER)
 
+#define XCOMP_BV_COMPACTION_ENABLED (1ULL << 63)
+
 #ifdef CONFIG_X86_64
 #define REX_PREFIX	"0x48, "
 #else
diff -puN arch/x86/kvm/x86.c~x86-fpu-take-kvm-def-and-make-generic arch/x86/kvm/x86.c
--- a/arch/x86/kvm/x86.c~x86-fpu-take-kvm-def-and-make-generic	2015-04-28 09:15:28.508683362 -0700
+++ b/arch/x86/kvm/x86.c	2015-04-28 09:15:28.534684535 -0700
@@ -3169,8 +3169,6 @@ static int kvm_vcpu_ioctl_x86_set_debugr
 	return 0;
 }
 
-#define XSTATE_COMPACTION_ENABLED (1ULL << 63)
-
 static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu)
 {
 	struct xsave_struct *xsave = &vcpu->arch.guest_fpu.state->xsave;
@@ -3222,7 +3220,8 @@ static void load_xsave(struct kvm_vcpu *
 	/* Set XSTATE_BV and possibly XCOMP_BV.  */
 	xsave->xsave_hdr.xstate_bv = xstate_bv;
 	if (cpu_has_xsaves)
-		xsave->xsave_hdr.xcomp_bv = host_xcr0 | XSTATE_COMPACTION_ENABLED;
+		xsave->xsave_hdr.xcomp_bv =
+			host_xcr0 | XCOMP_BV_COMPACTION_ENABLED;
 
 	/*
 	 * Copy each region from the non-compacted offset to the
@@ -6992,7 +6991,7 @@ int fx_init(struct kvm_vcpu *vcpu)
 	fpu_finit(&vcpu->arch.guest_fpu);
 	if (cpu_has_xsaves)
 		vcpu->arch.guest_fpu.state->xsave.xsave_hdr.xcomp_bv =
-			host_xcr0 | XSTATE_COMPACTION_ENABLED;
+			host_xcr0 | XCOMP_BV_COMPACTION_ENABLED;
 
 	/*
 	 * Ensure guest xcr0 is valid for loading
_

[PATCH 2/2] x86, fpu: fix boot time GPF with eagerfpu=off

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

I'm seeing some widespread failures to boot if I boot with
eagerfpu=off.  This only happens on hardware that supports
'xsaves', which is not widely available, so it is not
seen widely in practice.

The failure comes from init hitting a GPF very early in its
execution.  The GPF comes from a 'xrstors' operation that
the kernel performs which kills init.

The problem is essentially that "fpu_finit()" does not have
any knowledge about how to initialize an FPU buffer
(tsk->fpu->state) which will be restored with xsaves.  This
does not hit the use_eager_fpu() xsaves code because it
always does a __save_fpu() to initialize the FPU state from
the the process doing the fork()'s *registers* instead of
the 'init_xstate_buf' via init_fpu().

The code calls fx_finit(&fpu->state->fxsave) on the FPU state
to initialize the 'fxsave' style FPU buffer and then soon
after does an 'xrstors'-style restore.  That promptly GPF's.

As far as I can tell init_fpu() is only called when a task
has !used_math().  That never happens in the eagerfpu=on
case.

We never noticed this problem because this commit:

e7d820a5: x86, xsave: Support eager-only xsave features, add MPX support

effectively disabled lazy FPU mode on all hardware that
supports xsaves.  I think that commit itself is wrong
and there is no fundamental reason we can not support
lazy FPU mode with the MPX registers being xsave'd.  But,
that's another patch.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: x86@kernel.org
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andy Lutomirski <luto@amacapital.net>
---

 b/arch/x86/kernel/i387.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff -puN arch/x86/kernel/i387.c~x86-fpu-fix-boot-time-gpf-with-eagerfpuoff arch/x86/kernel/i387.c
--- a/arch/x86/kernel/i387.c~x86-fpu-fix-boot-time-gpf-with-eagerfpuoff	2015-04-28 09:51:55.634326943 -0700
+++ b/arch/x86/kernel/i387.c	2015-04-28 09:53:23.065270248 -0700
@@ -19,6 +19,7 @@
 #include <asm/i387.h>
 #include <asm/fpu-internal.h>
 #include <asm/user.h>
+#include <asm/xcr.h>
 
 static DEFINE_PER_CPU(bool, in_kernel_fpu);
 
@@ -218,6 +219,41 @@ void fpu_init(void)
 	eager_fpu_init();
 }
 
+/*
+ * This will take the 'xsave' portion of a 'thread_xstate'
+ * and get it in to a state where an 'xrstor' or 'xrstors'
+ * of the buffer will result in all of the xsave state
+ * components being set to their initialized state.
+ *
+ * We initialize the minimal set of fields in the header
+ * and rely on the CPU to initialize the registers
+ * themselves.
+ */
+static void xsave_fpu_buf_init(struct xsave_hdr_struct *xhdr)
+{
+	/*
+	 * This will cause the xrstor[s] to initialize
+	 * all the xsave state components.
+	 */
+	xhdr->xstate_bv = 0;
+	/*
+	 * xrstors will GPF if this bit is not set
+	 */
+	if (boot_cpu_has(X86_FEATURE_XSAVEC))
+		xhdr->xcomp_bv = XCOMP_BV_COMPACTION_ENABLED;
+	else
+		xhdr->xcomp_bv = 0;
+	/*
+	 * SDM Says we will GPF:
+	 * "If bytes 63:16 of the XSAVE header are not all zero."
+	 * If this reserved size ever changes, we will need to
+	 * update this code to look at and conditionally
+	 * initialize the new fields, just like ->xcomp_bv above.
+	 */
+	BUILD_BUG_ON(sizeof(xhdr->reserved) != 48);
+	memset(xhdr->reserved, 0, sizeof(xhdr->reserved));
+}
+
 void fpu_finit(struct fpu *fpu)
 {
 	if (!cpu_has_fpu) {
@@ -225,6 +261,15 @@ void fpu_finit(struct fpu *fpu)
 		return;
 	}
 
+	/*
+	 * The xsave code does not require the entire buffer
+	 * to be zeroed, only a chunk of the header.
+	 */
+	if (cpu_has_xsave) {
+		xsave_fpu_buf_init(&fpu->state->xsave.xsave_hdr);
+		return;
+	}
+
 	memset(fpu->state, 0, xstate_size);
 
 	if (cpu_has_fxsr) {
_