Re: [PATCH v4 4/8] x86/fpu/xstate: Differentiate default features for host and guest FPUs

["Chang S. Bae" <chang.seok.bae@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

On 3/18/2025 8:31 AM, Chao Gao wrote:
> 
> @@ -807,9 +811,11 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
>   	/* 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_kernel_cfg.guest_default_features = fpu_kernel_cfg.default_features;
>   
>   	fpu_user_cfg.default_features = fpu_user_cfg.max_features;
>   	fpu_user_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC;
> +	fpu_user_cfg.guest_default_features = fpu_user_cfg.default_features;

And you'll add up this on patch7:

   + /* Clean out guest-only features from default */
   + fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_SUPERVISOR_GUEST;


I'm not sure this overall hunk is entirely clear.


Taking a step back, we currently define three types of xfeature sets:

   1. 'default_features'     in a task-inlined buffer
   2. 'max_features'         in an extended buffer
   3. 'independent_features' in a separate buffer (only for LBR)

The VCPU fpstate has so far followed (1) and (2). Now, since we're 
introducing divergence at (1), you've named it guest_default_features:

   'default_features' < 'guest_default_features' < 'max_features'

I don’t see a strong reason against introducing this new field, as 
'guest' already implies the VCPU state. However, rather than directly 
modifying or extending struct fpu_state_config — which may not align 
well with VCPU FPU properties — a dedicated struct could provide a 
cleaner and more structured alternative:

   struct vcpu_fpu_config {
     unsigned int size;
     unsigned int user_size;
     u64 features;
     u64 user_features;
   } guest_default_cfg;

This struct would make VCPU-specific state handling clearer:

   (1) Guest permission setup:

        /* Set the guest default permission */
        fpu->guest_perm.__state_perm      = guest_default_cfg.features;
        fpu->guest_perm.__state_size      = guest_default_cfg.size;
        fpu->guest_perm.__user_state_size = guest_default_cfg.user_size;

   (2) VCPU allocation time:

        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;

These assignments considerably make the code more readable.

With that, going back to the default settings, perhaps refactoring it 
could be an option to improve clarity in distinguishing guest vs. host 
settings.

See the attached diff file. I thought this restructuring could make the 
logic more explicit and highlight the differences between guest and host 
settings.

Thanks,
Chang

[-- Attachment #2: guest_default.patch --]
[-- Type: text/plain, Size: 2350 bytes --]

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 40621ee4d65b..d2f7ce45d4de 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -702,6 +702,11 @@ static int __init init_xstate_size(void)
 	fpu_user_cfg.default_size =
 		xstate_calculate_size(fpu_user_cfg.default_features, false);
 
+	guest_default_cfg.size =
+		xstate_calculate_size(guest_default_cfg.features, compacted);
+	guest_default_cfg.user_size =
+		xstate_calculate_size(guest_default_cfg.user_features, false);
+
 	return 0;
 }
 
@@ -730,6 +735,30 @@ static void __init fpu__init_disable_system_xstate(unsigned int legacy_size)
 	fpstate_reset(&current->thread.fpu);
 }
 
+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 and guest-only
+	 * xfeatures at all:
+	 */
+	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;
+
+	/*
+	 * Ensure VCPU FPU container only reserves a space for
+	 * guest-exclusive xfeatures. This distinction can save kernel
+	 * memory by maintaining a necessary amount of XSAVE buffer.
+	 */
+	guest_default_cfg.features      = kfeatures | xfeatures_mask_guest_supervisor();
+	guest_default_cfg.user_features = ufeatures;
+}
+
 /*
  * Enable and initialize the xsave feature.
  * Called once per system bootup.
@@ -801,12 +830,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, determin default values: */
+	init_default_features(fpu_kernel_cfg.max_features, fpu_user_cfg.max_features);
 
 	/* Store it for paranoia check at the end */
 	xfeatures = fpu_kernel_cfg.max_features;