Re: [PATCH v10 13/28] x86/fpu/xstate: Use feature disable (XFD) to protect dynamic user state

["Bae, Chang Seok" <chang.seok.bae@intel.com>]

On Oct 1, 2021, at 13:20, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, Oct 01 2021 at 17:02, Thomas Gleixner wrote:
>> On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:
>>> +/**
>>> + * xfd_switch - Switches the MSR IA32_XFD context if needed.
>>> + * @prev:	The previous task's struct fpu pointer
>>> + * @next:	The next task's struct fpu pointer
>>> + */
>>> +static inline void xfd_switch(struct fpu *prev, struct fpu *next)
>>> +{
>>> +	u64 prev_xfd_mask, next_xfd_mask;
>>> +
>>> +	if (!cpu_feature_enabled(X86_FEATURE_XFD) || !xfeatures_mask_user_dynamic)
>>> +		return;
>> 
>> This is context switch, so this wants to be a static key which is turned
>> on during init when the CPU supports XFD and user dynamic features are
>> available.

I see. When there is a static key, use it only since this is context switch.

>>> +
>>> +	prev_xfd_mask = prev->state_mask & xfeatures_mask_user_dynamic;
>>> +	next_xfd_mask = next->state_mask & xfeatures_mask_user_dynamic;
>>> +
>>> +	if (unlikely(prev_xfd_mask != next_xfd_mask))
>>> +		wrmsrl_safe(MSR_IA32_XFD, xfeatures_mask_user_dynamic ^ next_xfd_mask);
>>> +}
>>> +
>>> /*
>>>  * Delay loading of the complete FPU state until the return to userland.
>>>  * PKRU is handled separately.
>>>  */
>>> -static inline void switch_fpu_finish(struct fpu *new_fpu)
>>> +static inline void switch_fpu_finish(struct fpu *old_fpu, struct fpu *new_fpu)
>>> {
>>> -	if (cpu_feature_enabled(X86_FEATURE_FPU))
>>> +	if (cpu_feature_enabled(X86_FEATURE_FPU)) {
>>> 		set_thread_flag(TIF_NEED_FPU_LOAD);
>>> +		xfd_switch(old_fpu, new_fpu);
>> 
>> Why has this to be done on context switch? Zero explanation provided.
>> 
>> Why can't this be done in exit_to_user() where the FPU state restore is
>> handled?

Looking at the changelog of the patch to delay XSTATE [1] load:

    This gives the kernel the potential to skip loading FPU state for tasks
    that stay in kernel mode, or for tasks that end up with repeated
    invocations of kernel_fpu_begin() & kernel_fpu_end().

But I think XFD state is different from XSTATE. There is no use case for
XFD-enabled features in kernel mode. So, XFD state was considered to be
switched under switch_to() just like other user states. E.g. user FSBASE is
switched here as kernel does not use it. But user GSBASE is loaded at
returning to userspace. Potentially, it is also beneficial as XFD-armed states
will hold INIT-state [3]:

    If XSAVE, XSAVEC, XSAVEOPT, or XSAVES is saving the state component i, the
    instruction does not generate #NM when XCR0[i] = IA32_XFD[i] = 1; instead,
    it saves bit i of XSTATE_BV field of the XSAVE header as 0 (indicating
    that the state component is in its initialized state).

I wish I could make sure the above point before following this:

> DEFINE_PER_CPU(xfd_state);
> 
> update_xfd(fpu)
> {
> 	if (__this_cpu_read(xfd_state) != fpu->xfd_state) {
> 		wrmsrl(XFD, fpu->xfd_state);
> 		__this_cpu_write(xfd_state, fpu->xfd_state);
> 	}
> }
> 
> fpregs_restore_userregs()
> {
> 	if (!fpregs_state_valid(fpu, cpu)) {
>        	if (static_branch_unlikely(xfd_switching_enabled))
>                	update_xfd(fpu);
> 	...
> 	}
> }
> 
> Hmm?

Yes, I thought some similar things in this [2].

Thanks,
Chang

[1] https://lore.kernel.org/all/20190403164156.19645-24-bigeasy@linutronix.de/
[2] https://lore.kernel.org/lkml/20210825155413.19673-28-chang.seok.bae@intel.com/
[3] 3.2.6 Extended Feature Disable (XFD), Intel Architecture Instruction Set Extension Programming Reference May 2021, https://software.intel.com/content/dam/develop/external/us/en/documents-tps/architecture-instruction-set-extensions-programming-reference.pdf