Re: [PATCH v3] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state

[Ingo Molnar <mingo@kernel.org>]

* Rik van Riel <riel@redhat.com> wrote:

>  /*
> + * Weird legacy quirk: SSE and YMM states store information in the
> + * MXCSR and MXCSR_FLAGS fields of the FP area. That means if the FP
> + * area is marked is unused in the xfeatures header, we need to copy
> + * MXCSR and MXCSR_FLAGS if either SSE or YMM are in use.
> + */
> +static inline bool xfeatures_need_mxcsr_copy(u64 xfeatures)
> +{
> +	if (!(xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)))
> +		return 0;
> +
> +	if (xfeatures & XFEATURE_MASK_FP)
> +		return 0;
> +
> +	return 1;
> +}

Applied to tip:WIP.x86/fpu and will try to get that branch into final shape ASAP, 
thanks Rik!

BTW., a different approach: could we also implement this quirk via setting the 
xfeatures bits accordingly? In particular, we could set FP to 1 if we see that 
XFEATURE_MASK_SSE or XFEATURE_MASK_YMM are set.

I.e. instead of:

	header.xfeatures = xsave->header.xfeatures;

We could do something like:

	header.xfeatures = xfeatures_quirk(xsave->header.xfeatures);

?

xfeatures_quirk() would do the obvious:

static u64 xfeatures_mxcsr_quirk(u64 xfeatures)
{
        if (xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM))
                return xfeatures | XFEATURE_MASK_FP;

	return xfeatures;
}

This means we'd copy the whole FP area, not just the MXCSR* fields, but I think 
overall it's a cleaner and easier to maintain approach - assuming it works and I'm 
missing something.

Such as us then sticking that enabled FP bit into the hardware and it could get 
confused or reject the state if other FP fields have random values?

In any case I've applied your fix with minor edits: I fixed a typo, renamed the 
quirk function which was a bit long, removed marginal linebreaks and twiddled the 
changelog. Edited version is attached below.

BTW., would you be interested in adding your FPU user ABI tests to 
tools/tests/selftests/x86? If there's many tests then I wouldn't mind if it got a 
new, separate subdirectory, under tools/tests/selftests/x86/fpu/ or so.

Thanks,

	Ingo

==========================>
>From 85fb989d3a58cb9c7904bb7dd8264be61e18b185 Mon Sep 17 00:00:00 2001
From: Rik van Riel <riel@redhat.com>
Date: Fri, 10 Feb 2017 08:54:45 -0500
Subject: [PATCH] x86/fpu: Add FPU state copying quirk to handle XRSTOR failure on Intel Skylake CPUs

On Skylake CPUs I noticed that XRSTOR is unable to deal with states
created by copyout_from_xsaves() if the xstate has only SSE/YMM state, and
no FP state. That is, xfeatures had XFEATURE_MASK_SSE set, but not
XFEATURE_MASK_FP.

The reason is that part of the SSE/YMM state lives in the MXCSR and
MXCSR_FLAGS fields of the FP state.

Ensure that whenever we copy SSE or YMM state around, the MXCSR and
MXCSR_FLAGS fields are also copied around.

Signed-off-by: Rik van Riel <riel@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Borislav Petkov <bp@suse.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@intel.com>
Link: http://lkml.kernel.org/r/20170210085445.0f1cc708@annuminas.surriel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/fpu/types.h |  3 +++
 arch/x86/kernel/fpu/xstate.c     | 42 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index d15cbfe0e8c4..ea65ab22e349 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -68,6 +68,9 @@ struct fxregs_state {
 /* Default value for fxregs_state.mxcsr: */
 #define MXCSR_DEFAULT		0x1f80
 
+/* Copy both mxcsr & mxcsr_flags with a single u64 memcpy: */
+#define MXCSR_AND_FLAGS_SIZE sizeof(u64)
+
 /*
  * Software based FPU emulation state. This is arbitrary really,
  * it matches the x87 format to make it easier to understand:
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 772a069f8fbf..2e8938309fac 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -920,6 +920,23 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 #endif /* ! CONFIG_ARCH_HAS_PKEYS */
 
 /*
+ * Weird legacy quirk: SSE and YMM states store information in the
+ * MXCSR and MXCSR_FLAGS fields of the FP area. That means if the FP
+ * area is marked as unused in the xfeatures header, we need to copy
+ * MXCSR and MXCSR_FLAGS if either SSE or YMM are in use.
+ */
+static inline bool xfeatures_mxcsr_quirk(u64 xfeatures)
+{
+	if (!(xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)))
+		return 0;
+
+	if (xfeatures & XFEATURE_MASK_FP)
+		return 0;
+
+	return 1;
+}
+
+/*
  * This is similar to user_regset_copyout(), but will not add offset to
  * the source data pointer or increment pos, count, kbuf, and ubuf.
  */
@@ -987,6 +1004,12 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
 
 	}
 
+	if (xfeatures_mxcsr_quirk(header.xfeatures)) {
+		offset = offsetof(struct fxregs_state, mxcsr);
+		size = MXCSR_AND_FLAGS_SIZE;
+		__copy_xstate_to_kernel(kbuf, &xsave->i387.mxcsr, offset, size, size_total);
+	}
+
 	/*
 	 * Fill xsave->i387.sw_reserved value for ptrace frame:
 	 */
@@ -1069,6 +1092,12 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i
 
 	}
 
+	if (xfeatures_mxcsr_quirk(header.xfeatures)) {
+		offset = offsetof(struct fxregs_state, mxcsr);
+		size = MXCSR_AND_FLAGS_SIZE;
+		__copy_xstate_to_user(ubuf, &xsave->i387.mxcsr, offset, size, size_total);
+	}
+
 	/*
 	 * Fill xsave->i387.sw_reserved value for ptrace frame:
 	 */
@@ -1121,6 +1150,12 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)
 		}
 	}
 
+	if (xfeatures_mxcsr_quirk(xfeatures)) {
+		offset = offsetof(struct fxregs_state, mxcsr);
+		size = MXCSR_AND_FLAGS_SIZE;
+		memcpy(&xsave->i387.mxcsr, kbuf + offset, size);
+	}
+
 	/*
 	 * The state that came in from userspace was user-state only.
 	 * Mask all the user states out of 'xfeatures':
@@ -1176,6 +1211,13 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 		}
 	}
 
+	if (xfeatures_mxcsr_quirk(xfeatures)) {
+		offset = offsetof(struct fxregs_state, mxcsr);
+		size = MXCSR_AND_FLAGS_SIZE;
+		if (__copy_from_user(&xsave->i387.mxcsr, ubuf + offset, size))
+			return -EFAULT;
+	}
+
 	/*
 	 * The state that came in from userspace was user-state only.
 	 * Mask all the user states out of 'xfeatures':