[PATCH v7 0/6] Introduce CET supervisor state support

[PATCH v7 0/6] Introduce CET supervisor state support

[Chao Gao]

Dear maintainers and reviewers,

I kindly request your consideration for merging this series. Most of
patches have received Reviewed-by/Acked-by tags.

Thanks Chang, Rick, Xin, Sean and Dave for their help with this series.

== Changelog ==
v6->v7:
 - Collect reviews from Rick
 - Tweak __fpstate_reset() to handle guest fpstate rather than adding a
   guest-specific reset function (Sean & Dave)
 - Fold xfd initialization into __fpstate_reset() (Sean)
 - v6: https://lore.kernel.org/all/20250506093740.2864458-1-chao.gao@intel.com/

== Background ==

CET defines two register states: CET user, which includes user-mode control
registers, and CET supervisor, which consists of shadow-stack pointers for
privilege levels 0-2.

Current kernel disables shadow stacks in kernel mode, making the CET
supervisor state unused and eliminating the need for context switching.

== Problem ==

To virtualize CET for guests, KVM must accurately emulate hardware
behavior. A key challenge arises because there is no CPUID flag to indicate
that shadow stack is supported only in user mode. Therefore, KVM cannot
assume guests will not enable shadow stacks in kernel mode and must
preserve the CET supervisor state of vCPUs.

== Solution ==

An initial proposal to manually save and restore CET supervisor states
using raw RDMSR/WRMSR in KVM was rejected due to performance concerns and
its impact on KVM's ABI. Instead, leveraging the kernel's FPU
infrastructure for context switching was favored [1].

The main question then became whether to enable the CET supervisor state
globally for all processes or restrict it to vCPU processes. This decision
involves a trade-off between a 24-byte XSTATE buffer waste for all non-vCPU
processes and approximately 100 lines of code complexity in the kernel [2].
The agreed approach is to first try this optimal solution [3], i.e.,
restricting the CET supervisor state to guest FPUs only and eliminating
unnecessary space waste.

Key changes in this series are:

1) Fix existing issue regarding enabling guest supervisor states support.
2) Add default features and size for guest FPUs.
3) Add infrastructure to support guest-only features.
4) Add CET supervisor state as the first guest-only feature.

With the series in place, guest FPUs have xstate_bv[12] == xcomp_bv[12] == 1
and CET supervisor state is saved/reloaded when xsaves/xrstors executes on
guest FPUs. non-guest FPUs have xstate_bv[12] == xcomp_bv[12] == 0, then
CET supervisor state is not saved/restored.

== Performance ==

We measured context-switching performance with the benchmark [4] in following
three cases.

case 1: the baseline. i.e., this series isn't applied
case 2: baseline + this series. CET-S space is allocated for guest fpu only.
case 3: baseline + allocate CET-S space for all tasks. Hardware init
        optimization avoids writing out CET-S space on each XSAVES.

The performance differences in the three cases are very small and fall within the
run-to-run variation.

[1]: https://lore.kernel.org/kvm/ZM1jV3UPL0AMpVDI@google.com/
[2]: https://lore.kernel.org/kvm/1c2fd06e-2e97-4724-80ab-8695aa4334e7@intel.com/
[3]: https://lore.kernel.org/kvm/2597a87b-1248-b8ce-ce60-94074bc67ea4@intel.com/
[4]: https://github.com/antonblanchard/will-it-scale/blob/master/tests/context_switch1.c

Chao Gao (4):
  x86/fpu/xstate: Differentiate default features for host and guest FPUs
  x86/fpu: Initialize guest FPU permissions from guest defaults
  x86/fpu: Initialize guest fpstate and FPU pseudo container from guest
    defaults
  x86/fpu: Remove xfd argument from __fpstate_reset()

Yang Weijiang (2):
  x86/fpu/xstate: Introduce "guest-only" supervisor xfeature set
  x86/fpu/xstate: Add CET supervisor xfeature support as a guest-only
    feature

 arch/x86/include/asm/fpu/types.h  | 49 +++++++++++++++++++++++++++----
 arch/x86/include/asm/fpu/xstate.h |  9 ++++--
 arch/x86/kernel/fpu/core.c        | 49 ++++++++++++++++++++++---------
 arch/x86/kernel/fpu/init.c        |  1 +
 arch/x86/kernel/fpu/xstate.c      | 40 ++++++++++++++++++++-----
 arch/x86/kernel/fpu/xstate.h      |  5 ++++
 6 files changed, 123 insertions(+), 30 deletions(-)

-- 
2.47.1

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

[Chao Gao]

Currently, guest and host FPUs share the same default features. However,
the CET supervisor xstate is the first feature that needs to be enabled
exclusively for guest FPUs. Enabling it for host FPUs leads to a waste of
24 bytes in the XSAVE buffer.

To support "guest-only" features, add a new structure to hold the
default features and sizes for guest FPUs to clearly differentiate them
from those for host FPUs.

Note that,
1) for now, the default features for guest and host FPUs remain the
same. This will change in a follow-up patch once guest permissions, default
xfeatures, and fpstate size are all converted to use the guest defaults.

2) only supervisor features will diverge between guest FPUs and host
FPUs, while user features will remain the same [1][2]. So, the new
vcpu_fpu_config struct does not include default user features and size
for the UABI buffer.

An alternative approach is adding a guest_only_xfeatures member to
fpu_kernel_cfg and adding two helper functions to calculate the guest
default xfeatures and size. However, calculating these defaults at runtime
would introduce unnecessary overhead.

Suggested-by: Chang S. Bae <chang.seok.bae@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Link: https://lore.kernel.org/kvm/aAwdQ759Y6V7SGhv@google.com/ [1]
Link: https://lore.kernel.org/kvm/9ca17e1169805f35168eb722734fbf3579187886.camel@intel.com/ [2]
---
v7: add Rick's Reviewed-by

v6:
Drop vcpu_fpu_config.user_* (Rick)
Reset guest default size when XSAVE is unavaiable or disabled (Chang)

v5:
Add a new vcpu_fpu_config instead of adding new members to
fpu_state_config (Chang)
Extract a helper to set default values (Chang)
---
 arch/x86/include/asm/fpu/types.h | 26 ++++++++++++++++++++++++++
 arch/x86/kernel/fpu/core.c       |  1 +
 arch/x86/kernel/fpu/init.c       |  1 +
 arch/x86/kernel/fpu/xstate.c     | 27 +++++++++++++++++++++------
 4 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 1c94121acd3d..abd193a1a52e 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -551,6 +551,31 @@ struct fpu_guest {
 	struct fpstate			*fpstate;
 };
 
+/*
+ * FPU state configuration data for fpu_guest.
+ * Initialized at boot time. Read only after init.
+ */
+struct vcpu_fpu_config {
+	/*
+	 * @size:
+	 *
+	 * The default size of the register state buffer in guest FPUs.
+	 * Includes all supported features except independent managed
+	 * features and features which have to be requested by user space
+	 * before usage.
+	 */
+	unsigned int size;
+
+	/*
+	 * @features:
+	 *
+	 * The default supported features bitmap in guest FPUs. Does not
+	 * include independent managed features and features which have to
+	 * be requested by user space before usage.
+	 */
+	u64 features;
+};
+
 /*
  * FPU state configuration data. Initialized at boot time. Read only after init.
  */
@@ -606,5 +631,6 @@ struct fpu_state_config {
 
 /* FPU state configuration information */
 extern struct fpu_state_config fpu_kernel_cfg, fpu_user_cfg;
+extern struct vcpu_fpu_config guest_default_cfg;
 
 #endif /* _ASM_X86_FPU_TYPES_H */
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 1cda5b78540b..2cd5e1910ff8 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -36,6 +36,7 @@ DEFINE_PER_CPU(u64, xfd_state);
 /* The FPU state configuration data for kernel and user space */
 struct fpu_state_config	fpu_kernel_cfg __ro_after_init;
 struct fpu_state_config fpu_user_cfg __ro_after_init;
+struct vcpu_fpu_config guest_default_cfg __ro_after_init;
 
 /*
  * Represents the initial FPU state. It's mostly (but not completely) zeroes,
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 6bb3e35c40e2..e19660cdc70c 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -202,6 +202,7 @@ static void __init fpu__init_system_xstate_size_legacy(void)
 	fpu_kernel_cfg.default_size = size;
 	fpu_user_cfg.max_size = size;
 	fpu_user_cfg.default_size = size;
+	guest_default_cfg.size = size;
 }
 
 /*
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 1c8410b68108..f32047e12500 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -742,6 +742,9 @@ 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);
+
 	return 0;
 }
 
@@ -762,6 +765,7 @@ static void __init fpu__init_disable_system_xstate(unsigned int legacy_size)
 	fpu_kernel_cfg.default_size = legacy_size;
 	fpu_user_cfg.max_size = legacy_size;
 	fpu_user_cfg.default_size = legacy_size;
+	guest_default_cfg.size = legacy_size;
 
 	/*
 	 * Prevent enabling the static branch which enables writes to the
@@ -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);
 
 	/* Store it for paranoia check at the end */
 	xfeatures = fpu_kernel_cfg.max_features;
-- 
2.47.1

[PATCH v7 2/6] x86/fpu: Initialize guest FPU permissions from guest defaults

[Chao Gao]

Currently, fpu->guest_perm is copied from fpu->perm, which is derived from
fpu_kernel_cfg.default_features.

Guest defaults were introduced to differentiate the features and sizes of
host and guest FPUs. Copying guest FPU permissions from the host will lead
to inconsistencies between the guest default features and permissions.

Initialize guest FPU permissions from guest defaults instead of host
defaults. This ensures that any changes to guest default features are
automatically reflected in guest permissions, which in turn guarantees
that fpstate_realloc() allocates a correctly sized XSAVE buffer for guest
FPUs.

Suggested-by: Chang S. Bae <chang.seok.bae@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
v6: Drop vcpu_fpu_config.user_* and collect reviews (Rick)
---
 arch/x86/kernel/fpu/core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 2cd5e1910ff8..444e517a8648 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -553,8 +553,14 @@ void fpstate_reset(struct fpu *fpu)
 	fpu->perm.__state_perm		= fpu_kernel_cfg.default_features;
 	fpu->perm.__state_size		= fpu_kernel_cfg.default_size;
 	fpu->perm.__user_state_size	= fpu_user_cfg.default_size;
-	/* Same defaults for guests */
-	fpu->guest_perm = fpu->perm;
+
+	fpu->guest_perm.__state_perm	= guest_default_cfg.features;
+	fpu->guest_perm.__state_size	= guest_default_cfg.size;
+	/*
+	 * User features and sizes remain the same between guest FPUs
+	 * and host FPUs.
+	 */
+	fpu->guest_perm.__user_state_size = fpu_user_cfg.default_size;
 }
 
 static inline void fpu_inherit_perms(struct fpu *dst_fpu)
-- 
2.47.1

[PATCH v7 3/6] x86/fpu: Initialize guest fpstate and FPU pseudo container from guest defaults

[Chao Gao]

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.

Adjust __fpstate_reset() to handle different defaults for host and guest
FPUs. And to distinguish between the types of FPUs, move the initialization
of indicators (is_guest and is_valloc) before the reset.

Suggested-by: Chang S. Bae <chang.seok.bae@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
v7: tweak __fpstate_reset() instead of adding a guest-specific reset
    function (Sean/Dave)
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 | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 444e517a8648..0d501bd25d79 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -236,19 +236,22 @@ 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;
 
+	/* Initialize indicators to reflect properties of the fpstate */
+	fpstate->is_valloc	= true;
+	fpstate->is_guest	= true;
+
 	/* Leave xfd to 0 (the reset value defined by spec) */
 	__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
@@ -535,10 +538,20 @@ void fpstate_init_user(struct fpstate *fpstate)
 
 static void __fpstate_reset(struct fpstate *fpstate, u64 xfd)
 {
-	/* Initialize sizes and feature masks */
-	fpstate->size		= fpu_kernel_cfg.default_size;
+	/*
+	 * 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.
+	 */
+	if (fpstate->is_guest) {
+		fpstate->size		= guest_default_cfg.size;
+		fpstate->xfeatures	= guest_default_cfg.features;
+	} else {
+		fpstate->size		= fpu_kernel_cfg.default_size;
+		fpstate->xfeatures	= fpu_kernel_cfg.default_features;
+	}
+
 	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;
 }
-- 
2.47.1

[PATCH v7 4/6] x86/fpu: Remove xfd argument from __fpstate_reset()

[Chao Gao]

The initial values for fpstate::xfd differ between guest and host fpstates.
Currently, the initial values are passed as an argument to
__fpstate_reset(). But, __fpstate_reset() already assigns different default
features and sizes based on the type of fpstates (i.e., guest or host). So,
handle fpstate::xfd in a similar way to highlight the differences in the
initial xfd value between guest and host fpstates

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
Link: https://lore.kernel.org/all/aBuf7wiiDT0Wflhk@google.com/
---
v7: new.

 arch/x86/kernel/fpu/core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 0d501bd25d79..a3cafed350e0 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -211,7 +211,7 @@ void fpu_reset_from_exception_fixup(void)
 }
 
 #if IS_ENABLED(CONFIG_KVM)
-static void __fpstate_reset(struct fpstate *fpstate, u64 xfd);
+static void __fpstate_reset(struct fpstate *fpstate);
 
 static void fpu_lock_guest_permissions(void)
 {
@@ -246,8 +246,7 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
 	fpstate->is_valloc	= true;
 	fpstate->is_guest	= true;
 
-	/* Leave xfd to 0 (the reset value defined by spec) */
-	__fpstate_reset(fpstate, 0);
+	__fpstate_reset(fpstate);
 	fpstate_init_user(fpstate);
 
 	gfpu->fpstate		= fpstate;
@@ -536,7 +535,7 @@ void fpstate_init_user(struct fpstate *fpstate)
 		fpstate_init_fstate(fpstate);
 }
 
-static void __fpstate_reset(struct fpstate *fpstate, u64 xfd)
+static void __fpstate_reset(struct fpstate *fpstate)
 {
 	/*
 	 * Initialize sizes and feature masks. Supervisor features and
@@ -546,21 +545,23 @@ static void __fpstate_reset(struct fpstate *fpstate, u64 xfd)
 	if (fpstate->is_guest) {
 		fpstate->size		= guest_default_cfg.size;
 		fpstate->xfeatures	= guest_default_cfg.features;
+		/* Leave xfd to 0 (the reset value defined by spec) */
+		fpstate->xfd		= 0;
 	} else {
 		fpstate->size		= fpu_kernel_cfg.default_size;
 		fpstate->xfeatures	= fpu_kernel_cfg.default_features;
+		fpstate->xfd		= init_fpstate.xfd;
 	}
 
 	fpstate->user_size	= fpu_user_cfg.default_size;
 	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);
+	__fpstate_reset(fpu->fpstate);
 
 	/* Initialize the permission related info in fpu */
 	fpu->perm.__state_perm		= fpu_kernel_cfg.default_features;
-- 
2.47.1

[PATCH v7 5/6] x86/fpu/xstate: Introduce "guest-only" supervisor xfeature set

[Chao Gao]

From: Yang Weijiang <weijiang.yang@intel.com>

In preparation for upcoming CET virtualization support, the CET supervisor
state will be added as a "guest-only" feature, since it is required only by
KVM (i.e., guest FPUs). Establish the infrastructure for "guest-only"
features.

Define a new XFEATURE_MASK_GUEST_SUPERVISOR mask to specify features that
are enabled by default in guest FPUs but not in host FPUs. Specifically,
for any bit in this set, permission is granted and XSAVE space is allocated
during vCPU creation. Non-guest FPUs cannot enable guest-only features,
even dynamically, and no XSAVE space will be allocated for them.

The mask is currently empty, but this will be changed by a subsequent
patch.

Co-developed-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
v6: Collect reviews

v5: Explain in detail the reasoning behind the mask name choice below the
"---" separator line.

In previous versions, the mask was named "XFEATURE_MASK_SUPERVISOR_DYNAMIC"
Dave suggested this name [1], but he also noted, "I don't feel strongly about
it and I've said my piece. I won't NAK it one way or the other."

The term "dynamic" was initially preferred because it reflects the impact
on XSAVE buffers—some buffers accommodate dynamic features while others do
not. This naming allows for the introduction of dynamic features that are
not strictly "guest-only", offering flexibility beyond KVM.

However, using "dynamic" has led to confusion [2]. Chang pointed out that
permission granting and buffer allocation are actually static at VCPU
allocation, diverging from the model for user dynamic features. He also
questioned the rationale for introducing a kernel dynamic feature mask
while using it as a guest-only feature mask [3]. Moreover, Thomas remarked
that "the dynamic naming is really bad" [4]. Although his specific concerns
are unclear, we should be cautious about reinstating the "kernel dynamic
feature" naming.

Therefore, in v4, I renamed the mask to "XFEATURE_MASK_SUPERVISOR_GUEST"
and further refined it to "XFEATURE_MASK_GUEST_SUPERVISOR" in this v5.

[1]: https://lore.kernel.org/all/893ac578-baaf-4f4f-96ee-e012dfc073a8@intel.com/#t
[2]: https://lore.kernel.org/kvm/e15d1074-d5ec-431d-86e5-a58bc6297df8@intel.com/
[3]: https://lore.kernel.org/kvm/7bee70fd-b2b9-4466-a694-4bf3486b19c7@intel.com/
[4]: https://lore.kernel.org/all/87sg1owmth.ffs@nanos.tec.linutronix.de/
---
 arch/x86/include/asm/fpu/types.h  |  9 +++++----
 arch/x86/include/asm/fpu/xstate.h |  6 +++++-
 arch/x86/kernel/fpu/xstate.c      | 14 +++++++++++---
 arch/x86/kernel/fpu/xstate.h      |  5 +++++
 4 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index abd193a1a52e..54ba567258d6 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -592,8 +592,9 @@ struct fpu_state_config {
 	 * @default_size:
 	 *
 	 * The default size of the register state buffer. Includes all
-	 * supported features except independent managed features and
-	 * features which have to be requested by user space before usage.
+	 * supported features except independent managed features,
+	 * guest-only features and features which have to be requested by
+	 * user space before usage.
 	 */
 	unsigned int		default_size;
 
@@ -609,8 +610,8 @@ struct fpu_state_config {
 	 * @default_features:
 	 *
 	 * The default supported features bitmap. Does not include
-	 * independent managed features and features which have to
-	 * be requested by user space before usage.
+	 * independent managed features, guest-only features and features
+	 * which have to be requested by user space before usage.
 	 */
 	u64 default_features;
 	/*
diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index b308a76afbb7..a3cd25453f94 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -46,9 +46,13 @@
 /* Features which are dynamically enabled for a process on request */
 #define XFEATURE_MASK_USER_DYNAMIC	XFEATURE_MASK_XTILE_DATA
 
+/* Supervisor features which are enabled only in guest FPUs */
+#define XFEATURE_MASK_GUEST_SUPERVISOR	0
+
 /* All currently supported supervisor features */
 #define XFEATURE_MASK_SUPERVISOR_SUPPORTED (XFEATURE_MASK_PASID | \
-					    XFEATURE_MASK_CET_USER)
+					    XFEATURE_MASK_CET_USER | \
+					    XFEATURE_MASK_GUEST_SUPERVISOR)
 
 /*
  * A supervisor state component may not always contain valuable information,
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index f32047e12500..e77cbfd18094 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -781,14 +781,22 @@ static void __init init_default_features(u64 kernel_max_features, u64 user_max_f
 	u64 kfeatures = kernel_max_features;
 	u64 ufeatures = user_max_features;
 
-	/* Default feature sets should not include dynamic xfeatures. */
-	kfeatures &= ~XFEATURE_MASK_USER_DYNAMIC;
+	/*
+	 * 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;
 
-	guest_default_cfg.features      = kfeatures;
+	/*
+	 * 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.
+	 */
+	guest_default_cfg.features      = kfeatures | xfeatures_mask_guest_supervisor();
 }
 
 /*
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index a0256ef34ecb..5ced1a92e666 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -61,6 +61,11 @@ 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))
-- 
2.47.1

[PATCH v7 6/6] x86/fpu/xstate: Add CET supervisor xfeature support as a guest-only feature

[Chao Gao]

From: Yang Weijiang <weijiang.yang@intel.com>

== Background ==

CET defines two register states: CET user, which includes user-mode control
registers, and CET supervisor, which consists of shadow-stack pointers for
privilege levels 0-2.

Current kernels disable shadow stacks in kernel mode, making the CET
supervisor state unused and eliminating the need for context switching.

== Problem ==

To virtualize CET for guests, KVM must accurately emulate hardware
behavior. A key challenge arises because there is no CPUID flag to indicate
that shadow stack is supported only in user mode. Therefore, KVM cannot
assume guests will not enable shadow stacks in kernel mode and must
preserve the CET supervisor state of vCPUs.

== Solution ==

An initial proposal to manually save and restore CET supervisor states
using raw RDMSR/WRMSR in KVM was rejected due to performance concerns and
its impact on KVM's ABI. Instead, leveraging the kernel's FPU
infrastructure for context switching was favored [1].

The main question then became whether to enable the CET supervisor state
globally for all processes or restrict it to vCPU processes. This decision
involves a trade-off between a 24-byte XSTATE buffer waste for all non-vCPU
processes and approximately 100 lines of code complexity in the kernel [2].
The agreed approach is to first try this optimal solution [3], i.e.,
restricting the CET supervisor state to guest FPUs only and eliminating
unnecessary space waste.

The guest-only xfeature infrastructure has already been added. Now,
introduce CET supervisor xstate support as the first guest-only feature
to prepare for the upcoming CET virtualization in KVM.

Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Link: https://lore.kernel.org/kvm/ZM1jV3UPL0AMpVDI@google.com/ [1]
Link: https://lore.kernel.org/kvm/1c2fd06e-2e97-4724-80ab-8695aa4334e7@intel.com/ [2]
Link: https://lore.kernel.org/kvm/2597a87b-1248-b8ce-ce60-94074bc67ea4@intel.com/ [3]
---
v5:
Introduce CET supervisor xfeature directly as a guest-only feature, rather
than first introducing it in one patch and then converting it to guest-only
in a subsequent patch. (Chang)
Add new features after cleanups/bug fixes (Chang, Dave, Ingo)
Improve the commit message to follow the suggested
background-problem-solution pattern.
---
 arch/x86/include/asm/fpu/types.h  | 14 ++++++++++++--
 arch/x86/include/asm/fpu/xstate.h |  5 ++---
 arch/x86/kernel/fpu/xstate.c      |  5 ++++-
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 54ba567258d6..93e99d2583d6 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -118,7 +118,7 @@ enum xfeature {
 	XFEATURE_PKRU,
 	XFEATURE_PASID,
 	XFEATURE_CET_USER,
-	XFEATURE_CET_KERNEL_UNUSED,
+	XFEATURE_CET_KERNEL,
 	XFEATURE_RSRVD_COMP_13,
 	XFEATURE_RSRVD_COMP_14,
 	XFEATURE_LBR,
@@ -142,7 +142,7 @@ enum xfeature {
 #define XFEATURE_MASK_PKRU		(1 << XFEATURE_PKRU)
 #define XFEATURE_MASK_PASID		(1 << XFEATURE_PASID)
 #define XFEATURE_MASK_CET_USER		(1 << XFEATURE_CET_USER)
-#define XFEATURE_MASK_CET_KERNEL	(1 << XFEATURE_CET_KERNEL_UNUSED)
+#define XFEATURE_MASK_CET_KERNEL	(1 << XFEATURE_CET_KERNEL)
 #define XFEATURE_MASK_LBR		(1 << XFEATURE_LBR)
 #define XFEATURE_MASK_XTILE_CFG		(1 << XFEATURE_XTILE_CFG)
 #define XFEATURE_MASK_XTILE_DATA	(1 << XFEATURE_XTILE_DATA)
@@ -268,6 +268,16 @@ struct cet_user_state {
 	u64 user_ssp;
 };
 
+/*
+ * State component 12 is Control-flow Enforcement supervisor states.
+ * This state includes SSP pointers for privilege levels 0 through 2.
+ */
+struct cet_supervisor_state {
+	u64 pl0_ssp;
+	u64 pl1_ssp;
+	u64 pl2_ssp;
+} __packed;
+
 /*
  * State component 15: Architectural LBR configuration state.
  * The size of Arch LBR state depends on the number of LBRs (lbr_depth).
diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index a3cd25453f94..7a7dc9d56027 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -47,7 +47,7 @@
 #define XFEATURE_MASK_USER_DYNAMIC	XFEATURE_MASK_XTILE_DATA
 
 /* Supervisor features which are enabled only in guest FPUs */
-#define XFEATURE_MASK_GUEST_SUPERVISOR	0
+#define XFEATURE_MASK_GUEST_SUPERVISOR	XFEATURE_MASK_CET_KERNEL
 
 /* All currently supported supervisor features */
 #define XFEATURE_MASK_SUPERVISOR_SUPPORTED (XFEATURE_MASK_PASID | \
@@ -79,8 +79,7 @@
  * Unsupported supervisor features. When a supervisor feature in this mask is
  * supported in the future, move it to the supported supervisor feature mask.
  */
-#define XFEATURE_MASK_SUPERVISOR_UNSUPPORTED (XFEATURE_MASK_PT | \
-					      XFEATURE_MASK_CET_KERNEL)
+#define XFEATURE_MASK_SUPERVISOR_UNSUPPORTED (XFEATURE_MASK_PT)
 
 /* All supervisor states including supported and unsupported states. */
 #define XFEATURE_MASK_SUPERVISOR_ALL (XFEATURE_MASK_SUPERVISOR_SUPPORTED | \
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index e77cbfd18094..549cc8929407 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -56,7 +56,7 @@ static const char *xfeature_names[] =
 	"Protection Keys User registers",
 	"PASID state",
 	"Control-flow User registers",
-	"Control-flow Kernel registers (unused)",
+	"Control-flow Kernel registers (KVM only)",
 	"unknown xstate feature",
 	"unknown xstate feature",
 	"unknown xstate feature",
@@ -80,6 +80,7 @@ static unsigned short xsave_cpuid_features[] __initdata = {
 	[XFEATURE_PKRU]				= X86_FEATURE_OSPKE,
 	[XFEATURE_PASID]			= X86_FEATURE_ENQCMD,
 	[XFEATURE_CET_USER]			= X86_FEATURE_SHSTK,
+	[XFEATURE_CET_KERNEL]			= X86_FEATURE_SHSTK,
 	[XFEATURE_XTILE_CFG]			= X86_FEATURE_AMX_TILE,
 	[XFEATURE_XTILE_DATA]			= X86_FEATURE_AMX_TILE,
 	[XFEATURE_APX]				= X86_FEATURE_APX,
@@ -371,6 +372,7 @@ static __init void os_xrstor_booting(struct xregs_state *xstate)
 	 XFEATURE_MASK_BNDCSR |			\
 	 XFEATURE_MASK_PASID |			\
 	 XFEATURE_MASK_CET_USER |		\
+	 XFEATURE_MASK_CET_KERNEL |		\
 	 XFEATURE_MASK_XTILE |			\
 	 XFEATURE_MASK_APX)
 
@@ -572,6 +574,7 @@ static bool __init check_xstate_against_struct(int nr)
 	case XFEATURE_PASID:	  return XCHECK_SZ(sz, nr, struct ia32_pasid_state);
 	case XFEATURE_XTILE_CFG:  return XCHECK_SZ(sz, nr, struct xtile_cfg);
 	case XFEATURE_CET_USER:	  return XCHECK_SZ(sz, nr, struct cet_user_state);
+	case XFEATURE_CET_KERNEL: return XCHECK_SZ(sz, nr, struct cet_supervisor_state);
 	case XFEATURE_APX:        return XCHECK_SZ(sz, nr, struct apx_state);
 	case XFEATURE_XTILE_DATA: check_xtile_data_against_struct(sz); return true;
 	default:
-- 
2.47.1

Re: [PATCH v7 3/6] x86/fpu: Initialize guest fpstate and FPU pseudo container from guest defaults

[Sean Christopherson]

On Mon, May 12, 2025, Chao Gao wrote:
> @@ -535,10 +538,20 @@ void fpstate_init_user(struct fpstate *fpstate)
>  
>  static void __fpstate_reset(struct fpstate *fpstate, u64 xfd)
>  {
> -	/* Initialize sizes and feature masks */
> -	fpstate->size		= fpu_kernel_cfg.default_size;
> +	/*
> +	 * 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.

Pick of of "identical" or "the same" :-)

And maybe explain why supervisor features can diverge, while the kernel ensures
user features are identical?  Ditto for the XFD divergence.  E.g. I think this
would be accurate (though I may be reading too much into user features):

	/*
	 * Supervisor features (and thus sizes) may diverge between guest FPUs
	 * and host FPUs, as some supervisor features are supported for guests
	 * despite not being utilized by the host.  User features and sizes are
	 * always identical, which allows for common guest and userspace ABI.
	 *
	 * For the host, set XFD to the kernel's desired initialization value.
	 * For guests, set XFD to its architectural RESET value.
	 */

> +	 */
> +	if (fpstate->is_guest) {
> +		fpstate->size		= guest_default_cfg.size;
> +		fpstate->xfeatures	= guest_default_cfg.features;
> +	} else {
> +		fpstate->size		= fpu_kernel_cfg.default_size;
> +		fpstate->xfeatures	= fpu_kernel_cfg.default_features;
> +	}
> +
>  	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;
>  }
> -- 
> 2.47.1
> 

Re: [PATCH v7 3/6] x86/fpu: Initialize guest fpstate and FPU pseudo container from guest defaults

[Chao Gao]

On Mon, May 12, 2025 at 07:13:08AM -0700, Sean Christopherson wrote:
>On Mon, May 12, 2025, Chao Gao wrote:
>> @@ -535,10 +538,20 @@ void fpstate_init_user(struct fpstate *fpstate)
>>  
>>  static void __fpstate_reset(struct fpstate *fpstate, u64 xfd)
>>  {
>> -	/* Initialize sizes and feature masks */
>> -	fpstate->size		= fpu_kernel_cfg.default_size;
>> +	/*
>> +	 * 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.
>
>Pick of of "identical" or "the same" :-)

Sure.

>
>And maybe explain why supervisor features can diverge, while the kernel ensures
>user features are identical?  Ditto for the XFD divergence.  E.g. I think this
>would be accurate (though I may be reading too much into user features):
>
>	/*
>	 * Supervisor features (and thus sizes) may diverge between guest FPUs
>	 * and host FPUs, as some supervisor features are supported for guests
>	 * despite not being utilized by the host.  User features and sizes are
>	 * always identical, which allows for common guest and userspace ABI.
>	 *
>	 * For the host, set XFD to the kernel's desired initialization value.
>	 * For guests, set XFD to its architectural RESET value.
>	 */

Yea, this looks much better.

Re: [PATCH v7 0/6] Introduce CET supervisor state support

[Ingo Molnar]

* Chao Gao <chao.gao@intel.com> wrote:

> Dear maintainers and reviewers,
> 
> I kindly request your consideration for merging this series. Most of 
> patches have received Reviewed-by/Acked-by tags.

I don't see anything objectionable in this series.

The upcoming v6.16 merge window is already quite crowded in terms of 
FPU changes, so I think at this point we are looking at a v6.17 merge, 
done shortly after v6.16-rc1 if everything goes well. Dave, what do you 
think?

Thanks,

	Ingo

Re: [PATCH v7 0/6] Introduce CET supervisor state support

[Uros Bizjak]

On Mon, May 12, 2025 at 10:57 AM Chao Gao <chao.gao@intel.com> wrote:
>
> Dear maintainers and reviewers,
>
> I kindly request your consideration for merging this series. Most of
> patches have received Reviewed-by/Acked-by tags.
>
> Thanks Chang, Rick, Xin, Sean and Dave for their help with this series.
>
> == Changelog ==
> v6->v7:
>  - Collect reviews from Rick
>  - Tweak __fpstate_reset() to handle guest fpstate rather than adding a
>    guest-specific reset function (Sean & Dave)
>  - Fold xfd initialization into __fpstate_reset() (Sean)
>  - v6: https://lore.kernel.org/all/20250506093740.2864458-1-chao.gao@intel.com/
>
> == Background ==
>
> CET defines two register states: CET user, which includes user-mode control
> registers, and CET supervisor, which consists of shadow-stack pointers for
> privilege levels 0-2.
>
> Current kernel disables shadow stacks in kernel mode, making the CET
> supervisor state unused and eliminating the need for context switching.
>
> == Problem ==
>
> To virtualize CET for guests, KVM must accurately emulate hardware
> behavior. A key challenge arises because there is no CPUID flag to indicate
> that shadow stack is supported only in user mode. Therefore, KVM cannot
> assume guests will not enable shadow stacks in kernel mode and must
> preserve the CET supervisor state of vCPUs.
>
> == Solution ==
>
> An initial proposal to manually save and restore CET supervisor states
> using raw RDMSR/WRMSR in KVM was rejected due to performance concerns and
> its impact on KVM's ABI. Instead, leveraging the kernel's FPU
> infrastructure for context switching was favored [1].

Dear Chao,

I wonder if the same approach can be used to optimize switching of
Intel PT configuration context. There was a patch series [1] posted
some time ago that showed substantial reduction of overhead when
switching Intel PT configuration context on VM-Entry/Exit using
XSAVES/XRSTORS instructions:

Manual save(rdmsr):     ~334  cycles
Manual restore(wrmsr):  ~1668 cycles

XSAVES insturction:     ~124  cycles
XRSTORS instruction:    ~378  cycles

[1] https://lore.kernel.org/lkml/1557995114-21629-1-git-send-email-luwei.kang@intel.com/

Best regards,
Uros.

Re: [PATCH v7 0/6] Introduce CET supervisor state support

[Chao Gao]

On Fri, May 16, 2025 at 09:51:50AM +0200, Uros Bizjak wrote:
>On Mon, May 12, 2025 at 10:57 AM Chao Gao <chao.gao@intel.com> wrote:
>>
>> Dear maintainers and reviewers,
>>
>> I kindly request your consideration for merging this series. Most of
>> patches have received Reviewed-by/Acked-by tags.
>>
>> Thanks Chang, Rick, Xin, Sean and Dave for their help with this series.
>>
>> == Changelog ==
>> v6->v7:
>>  - Collect reviews from Rick
>>  - Tweak __fpstate_reset() to handle guest fpstate rather than adding a
>>    guest-specific reset function (Sean & Dave)
>>  - Fold xfd initialization into __fpstate_reset() (Sean)
>>  - v6: https://lore.kernel.org/all/20250506093740.2864458-1-chao.gao@intel.com/
>>
>> == Background ==
>>
>> CET defines two register states: CET user, which includes user-mode control
>> registers, and CET supervisor, which consists of shadow-stack pointers for
>> privilege levels 0-2.
>>
>> Current kernel disables shadow stacks in kernel mode, making the CET
>> supervisor state unused and eliminating the need for context switching.
>>
>> == Problem ==
>>
>> To virtualize CET for guests, KVM must accurately emulate hardware
>> behavior. A key challenge arises because there is no CPUID flag to indicate
>> that shadow stack is supported only in user mode. Therefore, KVM cannot
>> assume guests will not enable shadow stacks in kernel mode and must
>> preserve the CET supervisor state of vCPUs.
>>
>> == Solution ==
>>
>> An initial proposal to manually save and restore CET supervisor states
>> using raw RDMSR/WRMSR in KVM was rejected due to performance concerns and
>> its impact on KVM's ABI. Instead, leveraging the kernel's FPU
>> infrastructure for context switching was favored [1].
>
>Dear Chao,
>
>I wonder if the same approach can be used to optimize switching of
>Intel PT configuration context. There was a patch series [1] posted
>some time ago that showed substantial reduction of overhead when
>switching Intel PT configuration context on VM-Entry/Exit using
>XSAVES/XRSTORS instructions:

No, the guest-only infrastructure utilizes the FPU core to switch states
during context switches, whereas Intel PT state is switched at different
points, i.e., on VM entry/exit.

Switching Intel PT state on VM entry/exit is necessary only for the
"host-guest" mode, which is currently marked as BROKEN. Unless functional
issues are addressed first, there's no point in optimizing its state
switching.

If we ever reinstate support for the "host-guest" mode, I think Intel PT
state probably could be implemented as an independent feature, similar to
LBR state.

Re: [PATCH v7 0/6] Introduce CET supervisor state support

[Dave Hansen]

On 5/16/25 00:51, Uros Bizjak wrote:
> I wonder if the same approach can be used to optimize switching of
> Intel PT configuration context. There was a patch series [1] posted
> some time ago that showed substantial reduction of overhead when
> switching Intel PT configuration context on VM-Entry/Exit using
> XSAVES/XRSTORS instructions:
> 
> Manual save(rdmsr):     ~334  cycles
> Manual restore(wrmsr):  ~1668 cycles
> 
> XSAVES insturction:     ~124  cycles
> XRSTORS instruction:    ~378  cycles

There's nothing stopping us from using XSAVES/XRSTORS for PT,
independent of the kernel FPU infrastructure. RFBM exists for a reason.

There's also WRMSRLIST which we're not using yet either. It's an even
better fit and doesn't have the goofiness that using XSAVE does like the
legacy portion of the save area including the header.

Re: [PATCH v7 0/6] Introduce CET supervisor state support

[Dave Hansen]

On 5/15/25 08:41, Ingo Molnar wrote:
> * Chao Gao <chao.gao@intel.com> wrote:
>> I kindly request your consideration for merging this series. Most of 
>> patches have received Reviewed-by/Acked-by tags.
> I don't see anything objectionable in this series.
> 
> The upcoming v6.16 merge window is already quite crowded in terms of 
> FPU changes, so I think at this point we are looking at a v6.17 merge, 
> done shortly after v6.16-rc1 if everything goes well. Dave, what do you 
> think?

It's getting into shape, but it has a slight shortage of reviews. For
now, it's an all-Intel patch even though I _thought_ AMD had this
feature too. It's also purely for KVM and has some suggested-by's from
Sean, but no KVM acks on it.

I have the feeling Sean would speak up if this was going in a bad
direction for KVM, but I do love to see acks accompanying suggested-by's
to indicate that the suggestion was interpreted properly.

Re: [PATCH v7 0/6] Introduce CET supervisor state support

[Dave Hansen]

On 5/15/25 08:41, Ingo Molnar wrote:
> * Chao Gao <chao.gao@intel.com> wrote:
>> I kindly request your consideration for merging this series. Most of 
>> patches have received Reviewed-by/Acked-by tags.
> I don't see anything objectionable in this series.
> 
> The upcoming v6.16 merge window is already quite crowded in terms of 
> FPU changes, so I think at this point we are looking at a v6.17 merge, 
> done shortly after v6.16-rc1 if everything goes well. Dave, what do you 
> think?

It's getting into shape, but it has a slight shortage of reviews. For
now, it's an all-Intel patch even though I _thought_ AMD had this
feature too. It's also purely for KVM and has some suggested-by's from
Sean, but no KVM acks on it.

Sean is not exactly the quiet type about things, but it always warms me
heart to see an acked-by accompanying a suggested-by because it
indicates that the suggestion was heard and implemented properly.

Re: [PATCH v7 0/6] Introduce CET supervisor state support

[Chao Gao]

On Fri, May 16, 2025 at 08:20:54AM -0700, Dave Hansen wrote:
>On 5/15/25 08:41, Ingo Molnar wrote:
>> * Chao Gao <chao.gao@intel.com> wrote:
>>> I kindly request your consideration for merging this series. Most of 
>>> patches have received Reviewed-by/Acked-by tags.
>> I don't see anything objectionable in this series.
>> 
>> The upcoming v6.16 merge window is already quite crowded in terms of 
>> FPU changes, so I think at this point we are looking at a v6.17 merge, 
>> done shortly after v6.16-rc1 if everything goes well. Dave, what do you 
>> think?
>
>It's getting into shape, but it has a slight shortage of reviews. For
>now, it's an all-Intel patch even though I _thought_ AMD had this
>feature too. It's also purely for KVM and has some suggested-by's from
>Sean, but no KVM acks on it.
>
>Sean is not exactly the quiet type about things, but it always warms me
>heart to see an acked-by accompanying a suggested-by because it
>indicates that the suggestion was heard and implemented properly.

Hi Sean, John,

Based on Dave's feedback, could you please review this series and provide your
reviewed-by/acked-by if appropriate?

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

[Sean Christopherson]

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))

Re: [PATCH v7 0/6] Introduce CET supervisor state support

[Sean Christopherson]

On Wed, May 21, 2025, Chao Gao wrote:
> On Fri, May 16, 2025 at 08:20:54AM -0700, Dave Hansen wrote:
> >On 5/15/25 08:41, Ingo Molnar wrote:
> >> * Chao Gao <chao.gao@intel.com> wrote:
> >>> I kindly request your consideration for merging this series. Most of 
> >>> patches have received Reviewed-by/Acked-by tags.
> >> I don't see anything objectionable in this series.
> >> 
> >> The upcoming v6.16 merge window is already quite crowded in terms of 
> >> FPU changes, so I think at this point we are looking at a v6.17 merge, 
> >> done shortly after v6.16-rc1 if everything goes well. Dave, what do you 
> >> think?
> >
> >It's getting into shape, but it has a slight shortage of reviews. For
> >now, it's an all-Intel patch even though I _thought_ AMD had this
> >feature too. It's also purely for KVM and has some suggested-by's from
> >Sean, but no KVM acks on it.
> >
> >Sean is not exactly the quiet type about things, but it always warms me
> >heart to see an acked-by accompanying a suggested-by because it
> >indicates that the suggestion was heard and implemented properly.
> 
> Hi Sean, John,
> 
> Based on Dave's feedback, could you please review this series and provide your
> reviewed-by/acked-by if appropriate?

The initialization of default features is a bit gnarly and I think can be improved
without too much fuss, but otherwise this looks good.

Re: [PATCH v7 0/6] Introduce CET supervisor state support

[Peter Zijlstra]

On Fri, May 16, 2025 at 08:20:54AM -0700, Dave Hansen wrote:

> It's getting into shape, but it has a slight shortage of reviews. For
> now, it's an all-Intel patch even though I _thought_ AMD had this
> feature too. 

Yeah, AMD should have this. While AMD does not implement IBT, they did
do implement SS, and as such, they should be having this stuff as well.

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

[Chao Gao]

On Wed, May 21, 2025 at 09:49:48AM -0700, Sean Christopherson wrote:
>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;
>  }
>

Indeed, it is odd. And your suggestion looks good to me. Thanks.

I will fix this and the comment issue you pointed out and post a new version.