RE: [patch 5/6] x86/fpu: Provide fpu_update_guest_xcr0/xfd()

["Wang, Wei W" <wei.w.wang@intel.com>]

On Tuesday, December 14, 2021 10:50 AM, Thomas Gleixner wrote:
> KVM can require fpstate expansion due to updates to XCR0 and to the XFD
> MSR. In both cases it is required to check whether:
> 
>   - the requested values are correct or permitted
> 
>   - the resulting xfeature mask which is relevant for XSAVES is a subset of
>     the guests fpstate xfeature mask for which the register buffer is sized.
> 
>     If the feature mask does not fit into the guests fpstate then
>     reallocation is required.
> 
> Provide a common update function which utilizes the existing XFD
> enablement mechanics and two wrapper functions, one for XCR0 and one
> for XFD.
> 
> These wrappers have to be invoked from XSETBV emulation and the XFD
> MSR write emulation.
> 
> XCR0 modification can only proceed when fpu_update_guest_xcr0() returns
> success.
> 
> XFD modification is done by the FPU core code as it requires to update the
> software state as well.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> New version to handle the restore case and XCR0 updates correctly.
> ---
>  arch/x86/include/asm/fpu/api.h |   57
> +++++++++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/fpu/core.c     |   57
> +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 114 insertions(+)
> 
> --- a/arch/x86/include/asm/fpu/api.h
> +++ b/arch/x86/include/asm/fpu/api.h
> @@ -136,6 +136,63 @@ extern void fpstate_clear_xstate_compone
> extern bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu);  extern void
> fpu_free_guest_fpstate(struct fpu_guest *gfpu);  extern int
> fpu_swap_kvm_fpstate(struct fpu_guest *gfpu, bool enter_guest);
> +extern int __fpu_update_guest_features(struct fpu_guest *guest_fpu, u64
> +xcr0, u64 xfd);
> +
> +/**
> + * fpu_update_guest_xcr0 - Update guest XCR0 from XSETBV emulation
> + * @guest_fpu:	Pointer to the guest FPU container
> + * @xcr0:	Requested guest XCR0
> + *
> + * Has to be invoked before making the guest XCR0 update effective. The
> + * function validates that the requested features are permitted and
> +ensures
> + * that @guest_fpu->fpstate is properly sized taking
> +@guest_fpu->fpstate->xfd
> + * into account.
> + *
> + * If @guest_fpu->fpstate is not the current tasks active fpstate then
> +the
> + * caller has to ensure that @guest_fpu->fpstate cannot be concurrently
> +in
> + * use, i.e. the guest restore case.
> + *
> + * Return:
> + * 0		- Success
> + * -EPERM	- Feature(s) not permitted
> + * -EFAULT	- Resizing of fpstate failed
> + */
> +static inline int fpu_update_guest_xcr0(struct fpu_guest *guest_fpu,
> +u64 xcr0) {
> +	return __fpu_update_guest_features(guest_fpu, xcr0,
> +guest_fpu->fpstate->xfd); }
> +
> +/**
> + * fpu_update_guest_xfd - Update guest XFD from MSR write emulation
> + * @guest_fpu:	Pointer to the guest FPU container
> + * @xcr0:	Current guest XCR0
> + * @xfd:	Requested XFD value
> + *
> + * Has to be invoked to make the guest XFD update effective. The
> +function
> + * validates the XFD value and ensures that @guest_fpu->fpstate is
> +properly
> + * sized by taking @xcr0 into account.
> + *
> + * The caller must not modify @guest_fpu->fpstate->xfd or the XFD MSR
> + * directly.
> + *
> + * If @guest_fpu->fpstate is not the current tasks active fpstate then
> +the
> + * caller has to ensure that @guest_fpu->fpstate cannot be concurrently
> +in
> + * use, i.e. the guest restore case.
> + *
> + * On success the buffer size is valid, @guest_fpu->fpstate.xfd == @xfd
> +and
> + * if the guest fpstate is active then MSR_IA32_XFD == @xfd.
> + *
> + * On failure the previous state is retained.
> + *
> + * Return:
> + * 0		- Success
> + * -ENOTSUPP	- XFD value not supported
> + * -EFAULT	- Resizing of fpstate failed
> + */
> +static inline int fpu_update_guest_xfd(struct fpu_guest *guest_fpu, u64
> +xcr0, u64 xfd) {
> +	return __fpu_update_guest_features(guest_fpu, xcr0, xfd); }
> 
>  extern void fpu_copy_guest_fpstate_to_uabi(struct fpu_guest *gfpu, void
> *buf, unsigned int size, u32 pkru);  extern int
> fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf,
> u64 xcr0, u32 *vpkru);
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -261,6 +261,63 @@ void fpu_free_guest_fpstate(struct fpu_g  }
> EXPORT_SYMBOL_GPL(fpu_free_guest_fpstate);
> 
> +/**
> + * __fpu_update_guest_features - Validate and enable guest XCR0 and XFD
> updates
> + * @guest_fpu:	Pointer to the guest FPU container
> + * @xcr0:	Guest XCR0
> + * @xfd:	Guest XFD
> + *
> + * Note: @xcr0 and @xfd must either be the already validated values or
> +the
> + * requested values (guest emulation or host write on restore).
> + *
> + * Do not invoke directly. Use the provided wrappers
> +fpu_validate_guest_xcr0()
> + * and fpu_update_guest_xfd() instead.
> + *
> + * Return: 0 on success, error code otherwise  */ int
> +__fpu_update_guest_features(struct fpu_guest *guest_fpu, u64 xcr0, u64
> +xfd) {

I think there would be one issue for the "host write on restore" case.
The current QEMU based host restore uses the following sequence:
1) restore xsave
2) restore xcr0
3) restore XFD MSR

At the time of "1) restore xsave", KVM already needs fpstate expansion before restoring the xsave data.
So the 2 APIs here might not be usable for this usage.
Our current solution to fpstate expansion at KVM_SET_XSAVE (i.e. step 1) above) is:

kvm_load_guest_fpu(vcpu);
guest_fpu->realloc_request = realloc_request;
kvm_put_guest_fpu(vcpu);

"realloc_request" above is generated from the "xstate_header" received from userspace.

Thanks,
Wei