On Thu, 2025-04-10 at 15:24 +0800, Chao Gao wrote:
> fpu_alloc_guest_fpstate() currently uses host defaults to initialize guest
> fpstate and pseudo containers. Guest defaults were introduced to
> differentiate the features and sizes of host and guest FPUs. Switch to
> using guest defaults instead.
>
> Additionally, incorporate the initialization of indicators (is_valloc and
> is_guest) into the newly added guest-specific reset function to centralize
> the resetting of guest fpstate.
>
> Suggested-by: Chang S. Bae <chang.seok.bae@xxxxxxxxx>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> v5: init is_valloc/is_guest in the guest-specific reset function (Chang)
> ---
> arch/x86/kernel/fpu/core.c | 28 ++++++++++++++++++++--------
> 1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index e23e435b85c4..f5593f6009a4 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -201,7 +201,20 @@ void fpu_reset_from_exception_fixup(void)
> }
>
> #if IS_ENABLED(CONFIG_KVM)
> -static void __fpstate_reset(struct fpstate *fpstate, u64 xfd);
> +static void __guest_fpstate_reset(struct fpstate *fpstate, u64 xfd)
> +{
> + /* Initialize sizes and feature masks */
> + fpstate->size = guest_default_cfg.size;
> + fpstate->user_size = guest_default_cfg.user_size;
> + fpstate->xfeatures = guest_default_cfg.features;
> + fpstate->user_xfeatures = guest_default_cfg.user_features;
> + fpstate->xfd = xfd;
> +
> + /* Initialize indicators to reflect properties of the fpstate */
> + fpstate->is_valloc = true;
> + fpstate->is_guest = true;
> +}
> +
>
> static void fpu_lock_guest_permissions(void)
> {
> @@ -226,19 +239,18 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
> struct fpstate *fpstate;
> unsigned int size;
>
> - size = fpu_kernel_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64);
> + size = guest_default_cfg.size + ALIGN(offsetof(struct fpstate, regs), 64);
> +
> fpstate = vzalloc(size);
> if (!fpstate)
> return false;
>
> /* Leave xfd to 0 (the reset value defined by spec) */
> - __fpstate_reset(fpstate, 0);
> + __guest_fpstate_reset(fpstate, 0);
> fpstate_init_user(fpstate);
> - fpstate->is_valloc = true;
> - fpstate->is_guest = true;
>
> gfpu->fpstate = fpstate;
> - gfpu->xfeatures = fpu_kernel_cfg.default_features;
> + gfpu->xfeatures = guest_default_cfg.features;
>
> /*
> * KVM sets the FP+SSE bits in the XSAVE header when copying FPU state
> @@ -250,8 +262,8 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
> * all features that can expand the uABI size must be opt-in.
> */
The above comment is enlightening to the debate about whether guest needs a
separate user size and 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.
*/
The KVM FPU user xsave behavior *is* different, just not in the way than we have
been discussing. So the below code responds to mismatch between
fpu_user_cfg.default_size and KVM's ABI.
The fix that added it, d187ba531230 ("x86/fpu: KVM: Set the base guest FPU uABI
size to sizeof(struct kvm_xsave)"), seems like quick fix that could have instead
been fixed more properly by something like proposed in this series.
I propose we drop it from this series and follow up with a proper cleanup. It
deserves more than currently done here. For example in the below hunk it's now
comparing guest_default_cfg.user_size which is a guest only thing. I also wonder
if we really need gfpu->uabi_size.
So let's drop the code but not the idea. Chang what do you think of that?
> 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;
> + if (WARN_ON_ONCE(guest_default_cfg.user_size > gfpu->uabi_size))
> + gfpu->uabi_size = guest_default_cfg.user_size;
>
> fpu_lock_guest_permissions();
>
