Re: [RFC PATCH 2/8] x86/fpu/xstate: Fix guest fpstate allocation size calculation

[Sean Christopherson]

On Wed, Sep 13, 2023, Yang Weijiang wrote:
> Fix guest xsave area allocation size from fpu_user_cfg.default_size to
> fpu_kernel_cfg.default_size so that the xsave area size is consistent
> with fpstate->size set in __fpstate_reset().
> 
> With the fix, guest fpstate size is sufficient for KVM supported guest
> xfeatures.
> 
> Fixes: 69f6ed1d14c6 ("x86/fpu: Provide infrastructure for KVM FPU cleanup");
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> ---
>  arch/x86/kernel/fpu/core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index a86d37052a64..a42d8ad26ce6 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -220,7 +220,9 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
>  	struct fpstate *fpstate;
>  	unsigned int size;
>  
> -	size = fpu_user_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64);
> +	size = fpu_kernel_cfg.default_size +
> +	       ALIGN(offsetof(struct fpstate, regs), 64);

This looks sketchy and incomplete.  I haven't looked at the gory details of
fpu_user_cfg vs. fpu_kernel_cfg, but the rest of this function uses fpu_user_cfg,
including a check on fpu_user_cfg.default_size.  That makes me think that changing
just the allocation size isn't quite right.

	/* Leave xfd to 0 (the reset value defined by spec) */
	__fpstate_reset(fpstate, 0);
	fpstate_init_user(fpstate);
	fpstate->is_valloc	= true;
	fpstate->is_guest	= true;

	gfpu->fpstate		= fpstate;
	gfpu->xfeatures		= fpu_user_cfg.default_features;
	gfpu->perm		= fpu_user_cfg.default_features;

	/*
	 * KVM sets the FP+SSE bits in the XSAVE header when copying FPU state
	 * to userspace, even when XSAVE is unsupported, so that restoring FPU
	 * state on a different CPU that does support XSAVE can cleanly load
	 * the incoming state using its natural XSAVE.  In other words, KVM's
	 * uABI size may be larger than this host's default size.  Conversely,
	 * the default size should never be larger than KVM's base uABI size;
	 * all features that can expand the uABI size must be opt-in.
	 */
	gfpu->uabi_size		= sizeof(struct kvm_xsave);
	if (WARN_ON_ONCE(fpu_user_cfg.default_size > gfpu->uabi_size))
		gfpu->uabi_size = fpu_user_cfg.default_size;