Re: [PATCH v6 5/7] x86/fpu: Initialize guest fpstate and FPU pseudo container from guest defaults

[Sean Christopherson <seanjc@google.com>]

On Tue, May 06, 2025, 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>
> ---
> v6: Drop vcpu_fpu_config.user_* (Rick)
> v5: init is_valloc/is_guest in the guest-specific reset function (Chang)
> ---
>  arch/x86/kernel/fpu/core.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 444e517a8648..78a9c809dfad 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -211,7 +211,24 @@ 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. Supervisor features and
> +	 * sizes may diverge between guest FPUs and host FPUs, whereas
> +	 * user features and sizes remain the same.
> +	 */
> +	fpstate->size		= guest_default_cfg.size;
> +	fpstate->xfeatures	= guest_default_cfg.features;
> +	fpstate->user_size	= fpu_user_cfg.default_size;
> +	fpstate->user_xfeatures	= fpu_user_cfg.default_features;
> +	fpstate->xfd		= xfd;
> +
> +	/* Initialize indicators to reflect properties of the fpstate */
> +	fpstate->is_valloc	= true;
> +	fpstate->is_guest	= true;
> +}
> +

Extra newline.
>  
>  static void fpu_lock_guest_permissions(void)
>  {
> @@ -236,19 +253,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);

Given that there is a single caller for each of __fpstate_reset() and
__guest_fpstate_reset(), keeping the helpers does more harm than good IMO.
Passing in '0' and setting xfd in __guest_fpstate_reset() is especially pointless.

E.g.

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index d5f3af2ba758..87d6ee87ff55 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -212,25 +212,6 @@ void fpu_reset_from_exception_fixup(void)
 }
 
 #if IS_ENABLED(CONFIG_KVM)
-static void __guest_fpstate_reset(struct fpstate *fpstate, u64 xfd)
-{
-       /*
-        * Initialize sizes and feature masks. Supervisor features and
-        * sizes may diverge between guest FPUs and host FPUs, whereas
-        * user features and sizes remain the same.
-        */
-       fpstate->size           = guest_default_cfg.size;
-       fpstate->xfeatures      = guest_default_cfg.features;
-       fpstate->user_size      = fpu_user_cfg.default_size;
-       fpstate->user_xfeatures = fpu_user_cfg.default_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)
 {
        struct fpu_state_perm *fpuperm;
@@ -260,8 +241,20 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
        if (!fpstate)
                return false;
 
-       /* Leave xfd to 0 (the reset value defined by spec) */
-       __guest_fpstate_reset(fpstate, 0);
+       /* Initialize indicators to reflect properties of the fpstate */
+       fpstate->is_valloc      = true;
+       fpstate->is_guest       = true;
+
+       /*
+        * Initialize sizes and feature masks. Supervisor features and sizes
+        * may diverge between guest FPUs and host FPUs, whereas user features
+        * and sizes are always identical the same.
+        */
+       fpstate->size           = guest_default_cfg.size;
+       fpstate->xfeatures      = guest_default_cfg.features;
+       fpstate->user_size      = fpu_user_cfg.default_size;
+       fpstate->user_xfeatures = fpu_user_cfg.default_features;
+
        fpstate_init_user(fpstate);
 
        gfpu->fpstate           = fpstate;
@@ -550,21 +543,17 @@ void fpstate_init_user(struct fpstate *fpstate)
                fpstate_init_fstate(fpstate);
 }
 
-static void __fpstate_reset(struct fpstate *fpstate, u64 xfd)
-{
-       /* Initialize sizes and feature masks */
-       fpstate->size           = fpu_kernel_cfg.default_size;
-       fpstate->user_size      = fpu_user_cfg.default_size;
-       fpstate->xfeatures      = fpu_kernel_cfg.default_features;
-       fpstate->user_xfeatures = fpu_user_cfg.default_features;
-       fpstate->xfd            = xfd;
-}
-
 void fpstate_reset(struct fpu *fpu)
 {
        /* Set the fpstate pointer to the default fpstate */
        fpu->fpstate = &fpu->__fpstate;
-       __fpstate_reset(fpu->fpstate, init_fpstate.xfd);
+
+       /* Initialize sizes and feature masks */
+       fpu->fpstate->size              = fpu_kernel_cfg.default_size;
+       fpu->fpstate->user_size         = fpu_user_cfg.default_size;
+       fpu->fpstate->xfeatures         = fpu_kernel_cfg.default_features;
+       fpu->fpstate->user_xfeatures    = fpu_user_cfg.default_features;
+       fpu->fpstate->xfd               = init_fpstate.xfd;
 
        /* Initialize the permission related info in fpu */
        fpu->perm.__state_perm          = fpu_kernel_cfg.default_features;