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

From: Andy Lutomirski
Date: Fri Jun 11 2021 - 15:03:43 EST


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.