Re: [PATCH] fpu: xstate: Keep xfd_state always in-sync with IA32_XFD MSR

[Dave Hansen]

On 5/11/23 08:28, Adamos Ttofari wrote:
> @@ -179,8 +179,14 @@ void fpu__init_cpu_xstate(void)
>  	 * key as that does not work on the boot CPU. This also ensures
>  	 * that any stale state is wiped out from XFD.
>  	 */
> -	if (cpu_feature_enabled(X86_FEATURE_XFD))
> -		wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
> +	if (cpu_feature_enabled(X86_FEATURE_XFD)) {
> +		u64 xfd;
> +
> +		rdmsrl(MSR_IA32_XFD, xfd);
> +		__this_cpu_write(xfd_state, xfd);
> +
> +		xfd_update_state(&init_fpstate);
> +	}

The above comment didn't _quite_ make it into the context, so I'll paste
it here for your convenience:

>         /*
>          * Must happen after CR4 setup and before xsetbv() to allow KVM
>          * lazy passthrough.  Write independent of the dynamic state static
>          * key as that does not work on the boot CPU. This also ensures
>          * that any stale state is wiped out from XFD.
>          */

Translating there, "the dynamic state static key" means
'__fpu_state_size_dynamic' which is used here:

> static __always_inline __pure bool fpu_state_size_dynamic(void)
> {
>         return static_branch_unlikely(&__fpu_state_size_dynamic);
> }

You might recognize fpu_state_size_dynamic() from the first line of
xfd_update_state(), the function that you added to the above hunk.

Which brings me to ask what the point of calling xfd_update_state() is
in the first place if you're getting away with it not working on the
boot CPU.

Why not just short-circuit the (non-working) xfd_update_state() and do
this directly:

	wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
        __this_cpu_write(xfd_state, init_fpstate.xfd);

I don't think you even need to *READ* the MSR.  You're going to blow it
away anyway.

>  	/*
>  	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
> @@ -915,7 +921,7 @@ void fpu__resume_cpu(void)
>  	}
>  
>  	if (fpu_state_size_dynamic())
> -		wrmsrl(MSR_IA32_XFD, current->thread.fpu.fpstate->xfd);
> +		xfd_update_state(&init_fpstate);
>  }
>  
>  /*