Re: [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state
On Wed, Jan 25, 2017 at 08:57:59PM -0500, r...@redhat.com wrote:
> From: Rik van Riel
>
> 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
> ---
> 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);
> +
> + if (ret)
> + return ret;
> + }
> +
> + /*
>* Fill xsave->i387.sw_reserved value for ptrace frame:
>*/
> offset = offsetof(struct fxregs_state, sw_reserved);
> @@ -1030,6 +1047,7 @@ int copyin_to_xsaves(const void *kbuf, const void
> __user *ubuf,
> int i;
> u64 xfeatures;
> u64 allowed_features;
> + void *dst;
>
> offset = offsetof(struct xregs_state, header);
> size = sizeof(xfeatures);
> @@ -1053,7 +1071,7 @@ int copyin_to_xsaves(const void *kbuf, const void
> __user *ubuf,
> u64 mask = ((u64)1 << i);
>
> if (xfeatures & mask) {
> - void *dst = __raw_xsave_addr(xsave, 1 << i);
> + dst = __raw_xsave_addr(xsave, 1 << i);
>
> offset = xstate_offsets[i];
> size = xstate_sizes[i];
> @@ -1068,6 +1086,25 @@ int copyin_to_xsaves(const void *kbuf, const void
> __user *ubuf,
> }
>
> /*
> + * 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.
> + */
In xstateregs_set(), we enforced the starting pos must be from (0), which in
XSAVE time, was probably for this reason. The real mistake here, I think, is
allowing skipping of xstate[0] and xstate[1]. Both should have been there
even for XSAVES compacted-format. Would it be a simpler fix just making sure
xstate[0] and xstate[1] are copied?
Yu-cheng
Re: [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state
On 01/25/2017 05:57 PM, r...@redhat.com wrote: > /* > + * 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. > + */ Patch looks functionally good to me. One nit on the comment, though: Usually XSAVE "state" refers to the state in the CPU while the "area" refers to the buffer in memory. Probably best to talk about the "legacy FP area", not state. Maybe something like this for the first paragraph: MXCSR and MXCSR_MASK are part of the SSE and YMM states and are saved/restored along with those states. However, they are located in the legacy FP *area* of the XSAVE buffer.
Re: [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state
* r...@redhat.com wrote:
> From: Rik van Riel
>
> 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
> ---
> 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")
1)
the 'copyin/copyout' nomenclature needlessly departs from what the modern FPU
code
uses, which is:
copy_fpregs_to_fpstate()
copy_fpstate_to_sigframe()
copy_fregs_to_user()
copy_fxregs_to_kernel()
copy_fxregs_to_user()
copy_kernel_to_fpregs()
copy_kernel_to_fregs()
copy_kernel_to_fxregs()
copy_kernel_to_xregs()
copy_user_to_fregs()
copy_user_to_fxregs()
copy_user_to_xregs()
copy_xregs_to_kernel()
copy_xregs_to_user()
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:
2)
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)
3)
But there's worse problems - the 'kbuf' parameter in both APIs, for example in
copy_xstate_to_user():
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:
copy_xstate_to_user()
copy_xstate_to_kernel()
copy_user_to_xstate()
copy_kernel_to_xstate()
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.
4)
> /*
> + * 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 fl
[PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state
From: Rik van Riel
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
---
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);
+
+ if (ret)
+ return ret;
+ }
+
+ /*
* Fill xsave->i387.sw_reserved value for ptrace frame:
*/
offset = offsetof(struct fxregs_state, sw_reserved);
@@ -1030,6 +1047,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user
*ubuf,
int i;
u64 xfeatures;
u64 allowed_features;
+ void *dst;
offset = offsetof(struct xregs_state, header);
size = sizeof(xfeatures);
@@ -1053,7 +1071,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user
*ubuf,
u64 mask = ((u64)1 << i);
if (xfeatures & mask) {
- void *dst = __raw_xsave_addr(xsave, 1 << i);
+ dst = __raw_xsave_addr(xsave, 1 << i);
offset = xstate_offsets[i];
size = xstate_sizes[i];
@@ -1068,6 +1086,25 @@ int copyin_to_xsaves(const void *kbuf, const void __user
*ubuf,
}
/*
+* 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)) {
+ offset = offsetof(struct fxregs_state, mxcsr);
+ dst = xsave + offset;
+ size = sizeof(u64);
+
+ if (kbuf) {
+ memcpy(dst, kbuf + offset, size);
+ } else {
+ if (__copy_from_user(dst, ubuf + offset, size))
+ return -EFAULT;
+ }
+ }
+
+ /*
* The state that came in from userspace was user-state only.
* Mask all the user states out of 'xfeatures':
*/
--
2.9.3
