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

Groups > linux.kernel > #1578063 > unrolled thread

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

Started byRik van Riel <riel@redhat.com>
First post2017-02-10 01:00 +0100
Last post2017-02-10 02:30 +0100
Articles 9 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Rik van Riel <riel@redhat.com> - 2017-02-10 01:00 +0100
    Re: [PATCH v2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Borislav Petkov <bp@suse.de> - 2017-02-10 01:10 +0100
      Re: [PATCH v2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Rik van Riel <riel@redhat.com> - 2017-02-10 02:20 +0100
        Re: [PATCH v2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Ingo Molnar <mingo@kernel.org> - 2017-02-10 09:10 +0100
          [PATCH v3] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Rik van Riel <riel@redhat.com> - 2017-02-10 15:30 +0100
            Re: [PATCH v3] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Ingo Molnar <mingo@kernel.org> - 2017-02-11 11:10 +0100
    Re: [PATCH v2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Rik van Riel <riel@redhat.com> - 2017-02-10 02:10 +0100
      Re: [PATCH v2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Yu-cheng Yu <yu-cheng.yu@intel.com> - 2017-02-10 19:30 +0100
    Re: [PATCH v2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Yu-cheng Yu <yu-cheng.yu@intel.com> - 2017-02-10 02:30 +0100

#1578063 — [PATCH v2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state

FromRik van Riel <riel@redhat.com>
Date2017-02-10 01:00 +0100
Subject[PATCH v2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state
Message-ID<t96N4-4gQ-23@gated-at.bofh.it>
On Skylake CPUs I noticed that XRSTOR is unable to deal with xsave areas
created by copyout_from_xsaves if the xstate has only SSE/YMM state, but
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 area.

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>
---
 arch/x86/kernel/fpu/xstate.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 772a069f8fbf..97d485157564 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_need_mxcsr_copy(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,13 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
 
 	}
 
+	if (xfeatures_need_mxcsr_copy(header.xfeatures)) {
+		offset = offsetof(struct fxregs_state, mxcsr);
+		size = sizeof(u64); // copy mxcsr & mxcsr_flags
+		__copy_xstate_to_kernel(kbuf, &xsave->i387.mxcsr, offset,
+					size, size_total);
+	}
+
 	/*
 	 * Fill xsave->i387.sw_reserved value for ptrace frame:
 	 */
@@ -1069,6 +1093,13 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i
 
 	}
 
+	if (xfeatures_need_mxcsr_copy(header.xfeatures)) {
+		offset = offsetof(struct fxregs_state, mxcsr);
+		size = sizeof(u64); // copy mxcsr & mxcsr_flags
+		__copy_xstate_to_user(ubuf, &xsave->i387.mxcsr, offset,
+					size, size_total);
+	}
+
 	/*
 	 * Fill xsave->i387.sw_reserved value for ptrace frame:
 	 */
@@ -1121,6 +1152,12 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)
 		}
 	}
 
+	if (xfeatures_need_mxcsr_copy(xfeatures)) {
+		offset = offsetof(struct fxregs_state, mxcsr);
+		size = sizeof(u64); // copy mxcsr & mxcsr_flags
+		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 +1213,13 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 		}
 	}
 
+	if (xfeatures_need_mxcsr_copy(xfeatures)) {
+		offset = offsetof(struct fxregs_state, mxcsr);
+		size = sizeof(u64); // copy mxcsr & mxcsr_flags
+		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':

[toc] | [next] | [standalone]

#1578065

FromBorislav Petkov <bp@suse.de>
Date2017-02-10 01:10 +0100
Message-ID<t96WJ-4z7-5@gated-at.bofh.it>
In reply to#1578063
On Thu, Feb 09, 2017 at 06:43:47PM -0500, Rik van Riel wrote:
> On Skylake CPUs I noticed that XRSTOR is unable to deal with xsave areas
> created by copyout_from_xsaves if the xstate has only SSE/YMM state, but
> 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 area.
> 
> 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>
> ---
>  arch/x86/kernel/fpu/xstate.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)

...

> @@ -987,6 +1004,13 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
>  
>  	}
>  
> +	if (xfeatures_need_mxcsr_copy(header.xfeatures)) {
> +		offset = offsetof(struct fxregs_state, mxcsr);
> +		size = sizeof(u64); // copy mxcsr & mxcsr_flags
				    ^^^^^^^^^^^^^^^^^^^^^^^^^^^

We don't do // comments, do we?

And side-line comments are always impairing the readability of the code
unless it is a struct's members or asm or so ...

-- 
Regards/Gruss,
    Boris.

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

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

#1578080

FromRik van Riel <riel@redhat.com>
Date2017-02-10 02:20 +0100
Message-ID<t982u-5bz-17@gated-at.bofh.it>
In reply to#1578065
On Fri, 2017-02-10 at 01:02 +0100, Borislav Petkov wrote:
> On Thu, Feb 09, 2017 at 06:43:47PM -0500, Rik van Riel wrote:
> > On Skylake CPUs I noticed that XRSTOR is unable to deal with xsave
> > areas
> > created by copyout_from_xsaves if the xstate has only SSE/YMM
> > state, but
> > 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 area.
> > 
> > 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>
> > ---
> >  arch/x86/kernel/fpu/xstate.c | 44
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> 
> ...
> 
> > @@ -987,6 +1004,13 @@ int copy_xstate_to_kernel(void *kbuf, struct
> > xregs_state *xsave, unsigned int of
> >  
> >  	}
> >  
> > +	if (xfeatures_need_mxcsr_copy(header.xfeatures)) {
> > +		offset = offsetof(struct fxregs_state, mxcsr);
> > +		size = sizeof(u64); // copy mxcsr & mxcsr_flags
> 
> 				    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> We don't do // comments, do we?
> 
> And side-line comments are always impairing the readability of the
> code
> unless it is a struct's members or asm or so ...

Good point. OTOH, I don't really want to add an extra line
to each of these blocks of code, either...

Ingo, how would you like me to do these comments?

Or should I have a magic #define with comment somewhere,
like this?

/* Copy both mxcsr & mxcsr_flags */
#define MXCSR_AND_FLAGS_SIZE sizeof(u64)

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

#1578265

FromIngo Molnar <mingo@kernel.org>
Date2017-02-10 09:10 +0100
Message-ID<t9erf-Q0-1@gated-at.bofh.it>
In reply to#1578080
* Rik van Riel <riel@redhat.com> wrote:

> On Fri, 2017-02-10 at 01:02 +0100, Borislav Petkov wrote:
> > On Thu, Feb 09, 2017 at 06:43:47PM -0500, Rik van Riel wrote:
> > > On Skylake CPUs I noticed that XRSTOR is unable to deal with xsave
> > > areas
> > > created by copyout_from_xsaves if the xstate has only SSE/YMM
> > > state, but
> > > 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 area.
> > > 
> > > 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>
> > > ---
> > >  arch/x86/kernel/fpu/xstate.c | 44
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 44 insertions(+)
> > 
> > ...
> > 
> > > @@ -987,6 +1004,13 @@ int copy_xstate_to_kernel(void *kbuf, struct
> > > xregs_state *xsave, unsigned int of
> > >  
> > >  	}
> > >  
> > > +	if (xfeatures_need_mxcsr_copy(header.xfeatures)) {
> > > +		offset = offsetof(struct fxregs_state, mxcsr);
> > > +		size = sizeof(u64); // copy mxcsr & mxcsr_flags
> > 
> > 				    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > We don't do // comments, do we?
> > 
> > And side-line comments are always impairing the readability of the
> > code
> > unless it is a struct's members or asm or so ...
> 
> Good point. OTOH, I don't really want to add an extra line
> to each of these blocks of code, either...
> 
> Ingo, how would you like me to do these comments?
> 
> Or should I have a magic #define with comment somewhere,
> like this?
> 
> /* Copy both mxcsr & mxcsr_flags */
> #define MXCSR_AND_FLAGS_SIZE sizeof(u64)

Yeah, that define would make it pretty clear what's going on. Please make it a bit 
more vebose:

  /* Copy both mxcsr & mxcsr_flags with a single u64 memcpy: */
  #define MXCSR_AND_FLAGS_SIZE sizeof(u64)

As for same-line comments, it can be the usual comment form:

		size = sizeof(u64); /* Copy mxcsr & mxcsr_flags */

But MXCSR_AND_FLAGS_SIZE is more expressive.

BTW., you can also use a separate comment line in such cases:

		/* Copy mxcsr & mxcsr_flags in one u64 step: */
		size = sizeof(u64);

... as readability is more important than brevity.

It's the C++ comment style that is frowned upon, as it looks weird in Linux kernel 
code.

Thanks,

	Ingo

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

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

FromRik van Riel <riel@redhat.com>
Date2017-02-10 15:30 +0100
Subject[PATCH v3] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state
Message-ID<t9kn0-4wD-13@gated-at.bofh.it>
In reply to#1578265
On Fri, 10 Feb 2017 09:00:54 +0100
Ingo Molnar <mingo@kernel.org> wrote:

> > /* Copy both mxcsr & mxcsr_flags */
> > #define MXCSR_AND_FLAGS_SIZE sizeof(u64)  
> 
> Yeah, that define would make it pretty clear what's going on. Please make it a bit 
> more vebose:
> 
>   /* Copy both mxcsr & mxcsr_flags with a single u64 memcpy: */
>   #define MXCSR_AND_FLAGS_SIZE sizeof(u64)

OK, here it is :)

---8<---
Subject: x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state

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>
---
 arch/x86/include/asm/fpu/types.h |  3 +++
 arch/x86/kernel/fpu/xstate.c     | 44 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 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..992a7f8f4988 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 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;
+}
+
+/*
  * 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,13 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
 
 	}
 
+	if (xfeatures_need_mxcsr_copy(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 +1093,13 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i
 
 	}
 
+	if (xfeatures_need_mxcsr_copy(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 +1152,12 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)
 		}
 	}
 
+	if (xfeatures_need_mxcsr_copy(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 +1213,13 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 		}
 	}
 
+	if (xfeatures_need_mxcsr_copy(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':

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

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

FromIngo Molnar <mingo@kernel.org>
Date2017-02-11 11:10 +0100
SubjectRe: [PATCH v3] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state
Message-ID<t9CMV-7PR-11@gated-at.bofh.it>
In reply to#1578500
* 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':

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

#1578077

FromRik van Riel <riel@redhat.com>
Date2017-02-10 02:10 +0100
Message-ID<t97SN-58a-1@gated-at.bofh.it>
In reply to#1578063
On Thu, 2017-02-09 at 16:45 -0800, Yu-cheng Yu wrote:
> On Thu, Feb 09, 2017 at 06:43:47PM -0500, Rik van Riel 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 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_need_mxcsr_copy(u64 xfeatures)
> > +{
> > +	if (!(xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)))
> > +		return 0;
> > +
> > +	if (xfeatures & XFEATURE_MASK_FP)
> > +		return 0;
> > +
> > +	return 1;
> > +}
> 
> Would you consider separating MXCSR & MXCSR_FLAGS from
> XFEATURE_MASK_FP.  Here we assume if both xstate[0] and xstate[1]
> are being copied, then there is no need to copy MXCSR/MXCSR_FLAGS
> again.  What if only xstate[0] is copied and MXCSR/MXCSR_FLAGS
> is invalid?

Surely then the CPU would ignore the contents of
MXCSR/MXCSR_FLAGS because the SSE and YMM bits in
the xfeatures header are clear?

What am I missing?

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

#1578685

FromYu-cheng Yu <yu-cheng.yu@intel.com>
Date2017-02-10 19:30 +0100
Message-ID<t9o7f-73R-9@gated-at.bofh.it>
In reply to#1578077
On Thu, Feb 09, 2017 at 08:00:11PM -0500, Rik van Riel wrote:
> On Thu, 2017-02-09 at 16:45 -0800, Yu-cheng Yu wrote:
> > On Thu, Feb 09, 2017 at 06:43:47PM -0500, Rik van Riel 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 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_need_mxcsr_copy(u64 xfeatures)
> > > +{
> > > +	if (!(xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)))
> > > +		return 0;
> > > +
> > > +	if (xfeatures & XFEATURE_MASK_FP)
> > > +		return 0;
> > > +
> > > +	return 1;
> > > +}
> > 
> > Would you consider separating MXCSR & MXCSR_FLAGS from
> > XFEATURE_MASK_FP.  Here we assume if both xstate[0] and xstate[1]
> > are being copied, then there is no need to copy MXCSR/MXCSR_FLAGS
> > again.  What if only xstate[0] is copied and MXCSR/MXCSR_FLAGS
> > is invalid?
> 
> Surely then the CPU would ignore the contents of
> MXCSR/MXCSR_FLAGS because the SSE and YMM bits in
> the xfeatures header are clear?
> 
> What am I missing?

For example in copy_user_to_xstate(), is it possible that only
xstate[0] is copied in, but in fact the task already has a valid
xstate[1] and MXCSR is overwritten?  We can think about this
as a separate patch.

Thanks,
Yu-cheng

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

#1578092

FromYu-cheng Yu <yu-cheng.yu@intel.com>
Date2017-02-10 02:30 +0100
Message-ID<t97SN-58a-3@gated-at.bofh.it>
In reply to#1578063
On Thu, Feb 09, 2017 at 06:43:47PM -0500, Rik van Riel 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 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_need_mxcsr_copy(u64 xfeatures)
> +{
> +	if (!(xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)))
> +		return 0;
> +
> +	if (xfeatures & XFEATURE_MASK_FP)
> +		return 0;
> +
> +	return 1;
> +}

Would you consider separating MXCSR & MXCSR_FLAGS from
XFEATURE_MASK_FP.  Here we assume if both xstate[0] and xstate[1]
are being copied, then there is no need to copy MXCSR/MXCSR_FLAGS
again.  What if only xstate[0] is copied and MXCSR/MXCSR_FLAGS
is invalid?

Yu-cheng

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web