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

From: Ingo Molnar
Date: Thu Jan 26 2017 - 03:14:42 EST

* riel@xxxxxxxxxx <riel@xxxxxxxxxx> wrote:

> From: Rik van Riel <riel@xxxxxxxxxx>
> 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
> 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@xxxxxxxxxx>
> ---
> arch/x86/kernel/fpu/xstate.c | 39 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 38 insertions(+), 1 deletion(-)
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c1508d56ecfb..10b10917af81 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1004,6 +1004,23 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
> }
> /*
> + * Restoring SSE/YMM state requires that MXCSR & MXCSR_MASK are saved.
> + * Those fields are part of the legacy FP state, and only get saved
> + * above if XFEATURES_MASK_FP is set.
> + *
> + * Copy out those fields if we have SSE/YMM but no FP register data.
> + */
> + if ((header.xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)) &&
> + !(header.xfeatures & XFEATURE_MASK_FP)) {
> + size = sizeof(u64);
> + ret = xstate_copyout(offset, size, kbuf, ubuf,
> + &xsave->i387.mxcsr, 0, count);

So this u64 copy copies both i387.mxcsr and i387.mxcsr_mask, which only works
because the two fields are next to each other and there's no hole inbetween in the
structure, right?

That fact should at minimum be commented upon.

> @@ -1053,7 +1071,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,

Also, I clearly wasn't paying enough attention when I merged the commit that
introduced these ptrace conversion bits:

91c3dba7dbc1 ("x86/fpu/xstate: Fix PTRACE frames for XSAVES")


the 'copyin/copyout' nomenclature needlessly departs from what the modern FPU code
uses, which is:


I.e. according to this pattern, the following rename should be done:

copyin_to_xsaves() -> copy_user_to_xstate()
copyout_from_xsaves() -> copy_xstate_to_user()

or, if we want to be pedantic, denote that that the user-space format is ptrace:

copyin_to_xsaves() -> copy_user_ptrace_to_xstate()
copyout_from_xsaves() -> copy_xstate_to_user_ptrace()

(But I'd suggest the shorter, non-pedantic name.)

But there's other problems:


The copy_user_to_xstate() parameter order departs from the regular memcpy()
pattern we try to follow:

int copy_user_to_xstate(const void *kbuf, const void __user *ubuf,
struct xregs_state *xsave);

it should be the other way around:

int copy_user_to_xstate(struct xregs_state *xsave, const void *kbuf, const void __user *ubuf)


But there's worse problems - the 'kbuf' parameter in both APIs, for example in

if (kbuf) {
memcpy(&xfeatures, kbuf + offset, size);
} else {
if (__copy_from_user(&xfeatures, ubuf + offset, size))
return -EFAULT;

WTF: memory copy API semantics dependent on argument presence? Whether it's truly
a 'user' copy depends on whether 'kbuf' is NULL??

This should be split into four APIs:


This decoupling would remove the weird 'kbuf, ubuf, xstate' triple argument
dependence and turn them into regular two-argument memcpy() variant APIs:

copy_xstate_to_user (ubuf, xstate)
copy_xstate_to_kernel (kbuf, xstate)
copy_user_to_xstate (xstate, ubuf)
copy_kernel_to_xstate (xstate, kbuf)

... and would restore the type cleanliness/robustness of these APIs as well.


> /*
> + * SSE/YMM state depends on the MXCSR & MXCSR_MASK fields from the FP
> + * state. If we restored only SSE/YMM state but not FP state, copy
> + * those fields to ensure the SSE/YMM state restore works.
> + */
> + if ((xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)) &&
> + !(xfeatures & XFEATURE_MASK_FP)) {

So this pattern is used twice and it's quite a mouthful. How about introducing
such a helper:

* Weird legacy quirk: indicate whether the MXCSR/MXCSR_MASK part of the FP state
* is used, even though the xfeatures flag lies about it being unused:
static inline bool xfeatures_fp_mxcsr_used(u64 xfeatures)
return 0;

if (xfeatures & XFEATURE_MASK_FP)
return 0;

return 1;



While at it I noticed this code:

u64 mask = ((u64)1 << i);

instead of the ugly type cast, cannot that be written as:

u64 mask = 1ULL << i;

which is shorter and cleaner?

I.e. this code needs some serious love and I'm not surprised it had bugs in it...

But hindsight is 20/20 and I merged it myself and all that, so I'm not really
complaining - but let's not repeat the mistake, ok?