RE: [patch 4/6] x86/fpu: Add guest support to xfd_enable_feature()

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Sent: Tuesday, December 14, 2021 10:50 AM
> 
> Guest support for dynamically enabling FPU features requires a few

'enabling' -> 'enabled'

> modifications to the enablement function which is currently invoked from
> the #NM handler:
> 
>   1) Use guest permissions and sizes for the update
> 
>   2) Update fpu_guest state accordingly
> 
>   3) Take into account that the enabling can be triggered either from a
>      running guest via XSETBV and MSR_IA32_XFD write emulation and from

'and from' -> 'or from'

>      a guest restore. In the latter case the guests fpstate is not the
>      current tasks active fpstate.
> 
> Split the function and implement the guest mechanics throughout the
> callchain.
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>

[...]
> @@ -1553,6 +1531,13 @@ static int fpstate_realloc(u64 xfeatures
>  	newfps->user_size = usize;
>  	newfps->is_valloc = true;
> 
> +	if (guest_fpu) {
> +		newfps->is_guest = true;
> +		newfps->is_confidential = curfps->is_confidential;
> +		newfps->in_use = curfps->in_use;
> +		guest_fpu->xfeatures |= xfeatures;
> +	}
> +

As you explained guest fpstate is not current active in the restoring 
path, thus it's not correct to always inherit attributes from the 
active one.

Also we want to avoid touching real hardware state if guest_fpstate
!= curfps, e.g.:

	if (test_thread_flag(TIF_NEED_FPU_LOAD))
		fpregs_restore_userregs();

> +	if (guest_fpu) {
> +		curfps = xchg(&guest_fpu->fpstate, newfps);
> +		/* If curfps is active, update the FPU fpstate pointer */
> +		if (fpu->fpstate == curfps)
> +			fpu->fpstate = newfps;
> +	} else {
> +		curfps = xchg(&fpu->fpstate, newfps);
> +	}
> +
> +	xfd_update_state(fpu->fpstate);

and also here.

> @@ -1697,14 +1694,16 @@ int xfd_enable_feature(u64 xfd_err)
>  	spin_lock_irq(&current->sighand->siglock);
> 
>  	/* If not permitted let it die */
> -	if ((xstate_get_host_group_perm() & xfd_event) != xfd_event) {
> +	if ((xstate_get_group_perm(!!guest_fpu) & xfd_event) != xfd_event) {
>  		spin_unlock_irq(&current->sighand->siglock);
>  		return -EPERM;
>  	}
> 
>  	fpu = &current->group_leader->thread.fpu;
> -	ksize = fpu->perm.__state_size;
> -	usize = fpu->perm.__user_state_size;
> +	perm = guest_fpu ? &fpu->guest_perm : &fpu->perm;
> +	ksize = perm->__state_size;
> +	usize = perm->__user_state_size;
> +

Do we want to mention in the commit msg that fpstate 
reallocation size is based on permissions instead of requested 
features? The intuitive thought is that each time a new feature is 
requested this expands the buffer to match the requested feature...

Thanks
Kevin