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

[Yu-cheng Yu]

On Wed, Jan 25, 2017 at 08:57:59PM -0500, 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
> 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@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);
> +
> +		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