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@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
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.
*/
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();
--
2.46.1On 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@intel.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> 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();
>
On 4/30/2025 11:29 AM, Edgecombe, Rick P wrote: > > So let's drop the code but not the idea. Chang what do you think of that? Sure, that makes sense. Thanks for bringing up this case. Staring at the struct guest_fpu fields, some of them appear to match with fields in fpstate. In any case, I agree it would be helpful to consider a follow-up series after this. Thanks, Chang
On Thu, May 01, 2025 at 07:24:09AM -0700, Chang S. Bae wrote: >On 4/30/2025 11:29 AM, Edgecombe, Rick P wrote: >> >> So let's drop the code but not the idea. Chang what do you think of that? >Sure, that makes sense. Thanks for bringing up this case. Yea, I agree with dropping vcpu_fpu_config.user_* for now. > >Staring at the struct guest_fpu fields, some of them appear to match with >fields in fpstate. In any case, I agree it would be helpful to consider a >follow-up series after this.
© 2016 - 2025 Red Hat, Inc.