Re: [PATCH v7 1/6] x86/fpu/xstate: Differentiate default features for host and guest FPUs

[Sean Christopherson <seanjc@google.com>]

On Mon, May 12, 2025, Chao Gao wrote:
> @@ -772,6 +776,21 @@ static void __init fpu__init_disable_system_xstate(unsigned int legacy_size)
>  	fpstate_reset(x86_task_fpu(current));
>  }
>  
> +static void __init init_default_features(u64 kernel_max_features, u64 user_max_features)
> +{
> +	u64 kfeatures = kernel_max_features;
> +	u64 ufeatures = user_max_features;
> +
> +	/* Default feature sets should not include dynamic xfeatures. */
> +	kfeatures &= ~XFEATURE_MASK_USER_DYNAMIC;
> +	ufeatures &= ~XFEATURE_MASK_USER_DYNAMIC;
> +
> +	fpu_kernel_cfg.default_features = kfeatures;
> +	fpu_user_cfg.default_features   = ufeatures;
> +
> +	guest_default_cfg.features      = kfeatures;
> +}
> +
>  /*
>   * Enable and initialize the xsave feature.
>   * Called once per system bootup.
> @@ -854,12 +873,8 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
>  	fpu_user_cfg.max_features = fpu_kernel_cfg.max_features;
>  	fpu_user_cfg.max_features &= XFEATURE_MASK_USER_SUPPORTED;
>  
> -	/* Clean out dynamic features from default */
> -	fpu_kernel_cfg.default_features = fpu_kernel_cfg.max_features;
> -	fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC;
> -
> -	fpu_user_cfg.default_features = fpu_user_cfg.max_features;
> -	fpu_user_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC;
> +	/* Now, given maximum feature set, determine default values */
> +	init_default_features(fpu_kernel_cfg.max_features, fpu_user_cfg.max_features);

Passing in max_features is rather odd.  I assume the intent is to capture the
dependencies, but that falls apart by the end of series as the guest features
are initialized as:

	guest_default_cfg.features      = kfeatures | xfeatures_mask_guest_supervisor();

where xfeatures_mask_guest_supervisor() sneakily consumes fpu_kernel_cfg.max_features,
the very field this patch deliberately avoids consuming directly.

  static inline u64 xfeatures_mask_guest_supervisor(void)
  {
	return fpu_kernel_cfg.max_features & XFEATURE_MASK_GUEST_SUPERVISOR;
  }

Rather than providing a helper to initialize the defaults, what if we provide
helpers to provide the default *masks*?  Then the dependencies on max_features
are super clear.

E.g. spread over multiple patches (completely untested)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index be1cdfa9b00d..e52c7517df5f 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -780,27 +780,22 @@ static void __init fpu__init_disable_system_xstate(unsigned int legacy_size)
        fpstate_reset(x86_task_fpu(current));
 }
 
-static void __init init_default_features(u64 kernel_max_features, u64 user_max_features)
+static u64 __init host_default_mask(void)
 {
-       u64 kfeatures = kernel_max_features;
-       u64 ufeatures = user_max_features;
-
        /*
-        * Default feature sets should not include dynamic and guest-only
-        * xfeatures at all.
+        * Exclude dynamic features (require userspace opt-in) and features
+        * that are supported only for KVM guests.
         */
-       kfeatures &= ~(XFEATURE_MASK_USER_DYNAMIC | XFEATURE_MASK_GUEST_SUPERVISOR);
-       ufeatures &= ~XFEATURE_MASK_USER_DYNAMIC;
-
-       fpu_kernel_cfg.default_features = kfeatures;
-       fpu_user_cfg.default_features   = ufeatures;
+       return ~((u64)XFEATURE_MASK_USER_DYNAMIC | XFEATURE_MASK_GUEST_SUPERVISOR);
+}
 
+static u64 __init guest_default_mask(void)
+{
        /*
-        * Ensure VCPU FPU container only reserves a space for guest-only
-        * xfeatures. This distinction can save kernel memory by
-        * maintaining a necessary amount of XSAVE buffer.
+        * Exclude dynamic features, which require userspace opt-in even for
+        * KVM guests.
         */
-       guest_default_cfg.features      = kfeatures | xfeatures_mask_guest_supervisor();
+       return ~(u64)XFEATURE_MASK_USER_DYNAMIC;
 }
 
 /*
@@ -886,7 +881,9 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
        fpu_user_cfg.max_features &= XFEATURE_MASK_USER_SUPPORTED;
 
        /* Now, given maximum feature set, determine default values */
-       init_default_features(fpu_kernel_cfg.max_features, fpu_user_cfg.max_features);
+       fpu_kernel_cfg.default_features = fpu_kernel_cfg.max_features & host_default_mask();
+       fpu_user_cfg.default_features = fpu_user_cfg.max_features & host_default_mask();
+       guest_default_cfg.features = fpu_kernel_cfg.max_features & guest_default_mask();
 
        /* Store it for paranoia check at the end */
        xfeatures = fpu_kernel_cfg.max_features;
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 9e496391b5f0..52ce19289989 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -62,11 +62,6 @@ static inline u64 xfeatures_mask_supervisor(void)
        return fpu_kernel_cfg.max_features & XFEATURE_MASK_SUPERVISOR_SUPPORTED;
 }
 
-static inline u64 xfeatures_mask_guest_supervisor(void)
-{
-       return fpu_kernel_cfg.max_features & XFEATURE_MASK_GUEST_SUPERVISOR;
-}
-
 static inline u64 xfeatures_mask_independent(void)
 {
        if (!cpu_feature_enabled(X86_FEATURE_ARCH_LBR))