Re: [patch 08/41] x86/fpu: Restrict fpstate sanitizing to legacy components

[Andy Lutomirski <luto@kernel.org>]

On 6/11/21 9:15 AM, Thomas Gleixner wrote:
> xstateregs_get() does not longer use fpstate_sanitize_xstate() and the only

s/does not longer use/no longer uses/

		\
> --- a/arch/x86/kernel/fpu/regset.c
> +++ b/arch/x86/kernel/fpu/regset.c
> @@ -11,6 +11,39 @@
>  
>  #include <linux/sched/task_stack.h>
>  
> +/*
> + * When executing XSAVEOPT (or other optimized XSAVE instructions), if

The kernel doesn't use XSAVEOPT any more.  How about:

When executing XSAVES (or other optimized XSAVE instructions)

> + * a processor implementation detects that an FPU state component is still
> + * (or is again) in its initialized state, it may clear the corresponding
> + * bit in the header.xfeatures field, and can skip the writeout of registers
> + * to the corresponding memory layout.

Additionally, copy_xxx_to_xstate() may result in an xsave buffer with a
bit clear in xfeatures but the corresponding state region not containing
the state's init value.

> + *
> + * This means that when the bit is zero, the state component might still
> + * contain some previous - non-initialized register state.

Maybe say what the function does, e.g.:

This function fills in the init values for the X87 and SSE states if the
corresponding xfeatures bits are clear.

> + *
> + * This is required for the legacy regset functions.
> + */
> +static void fpstate_sanitize_legacy(struct fpu *fpu)
> +{
> +	struct fxregs_state *fx = &fpu->state.fxsave;
> +	u64 xfeatures;
> +
> +	if (!use_xsaveopt())
> +		return;

This is confusing, since we never use xsaveopt.  It's also wrong -- see
above.  How about just removing it?

> +
> +	xfeatures = fpu->state.xsave.header.xfeatures;
> +
> +	/* If FP is in init state, reinitialize it */
> +	if (!(xfeatures & XFEATURE_MASK_FP)) {
> +		memset(fx, 0, sizeof(*fx));
> +		fx->cwd = 0x37f;
> +	}
> +
> +	/* If SSE is in init state, clear the storage */
> +	if (!(xfeatures & XFEATURE_MASK_SSE))
> +		memset(fx->xmm_space, 0, sizeof(fx->xmm_space));
> +}
> +
>  

Does this result in the mxcsr_mask and mxcsr fields being correct?
There is a silly number of special cases there.