[PATCH v4 0/8] Introduce CET supervisor state support

[PATCH v4 0/8] Introduce CET supervisor state support

[Chao Gao]

==Changelog==
v3->v4:
 - Remove fpu_guest_cfg.
   The fact that only the default_features and default_size fields of
   fpu_guest_cfg are used suggests that a full fpu_guest_cfg may not be
   necessary. Adding two members, "guest_default_xfeatures" and
   "guest_default_size", or even a single "guest_only_xfeatures" member in
   fpu_kernel_cfg, similar to "independent_xfeatures", is more logical. To
   facilitate discussion, implement this approach in this version.
 - Extract the fix for inconsistencies in fpu_guest and post it separately
   (Chang)
 - Rename XFEATURE_MASK_KERNEL_DYNAMIC to XFEATURE_MASK_SUPERVISOR_GUEST as
   tglx noted "this dynamic naming is really bad":

   https://lore.kernel.org/all/87sg1owmth.ffs@nanos.tec.linutronix.de/

 - Rerun performance tests and update the performance claims in the cover-letter
   (Dave)
 - Tighten down the changelogs and drop useless comments (Dave)
 - Reorder the patches to put the CET supervisor state patch before the
   "guest-only" optimization, allowing maintainers to easily adopt or omit the
   optimization.
 - v3: https://lore.kernel.org/kvm/20250307164123.1613414-1-chao.gao@intel.com/

v2->v3:
 - reorder patches to add fpu_guest_cfg first and then introduce dynamic kernel
   feature concept (Dave)
 - Revise changelog for all patches except the first and the last one (Dave)
 - Split up patches that do multiple things into separate patches.
 - collect tags for patch 1
 - v2: https://lore.kernel.org/kvm/20241126101710.62492-1-chao.gao@intel.com/

v1->v2:
 - rebase onto the latest kvm-x86/next
 - Add performance data to the cover-letter
 - v1: https://lore.kernel.org/kvm/73802bff-833c-4233-9a5b-88af0d062c82@intel.com/

==Background==

This series spins off from CET KVM virtualization enabling series [1].
The purpose is to get these preparation work resolved ahead of KVM part
landing. There was a discussion about introducing CET supervisor state
support [2] [3].

CET supervisor state, i.e., IA32_PL{0,1,2}_SSP, are xsave-managed MSRs,
it can be enabled via IA32_XSS[bit 12]. KVM relies on host side CET
supervisor state support to fully enable guest CET MSR contents storage.
The benefits are: 1) No need to manually save/restore the 3 MSRs when
vCPU fpu context is sched in/out. 2) Omit manually swapping the three
MSRs at VM-Exit/VM-Entry for guest/host. 3) Make guest CET user/supervisor
states managed in a consistent manner within host kernel FPU framework.

==Solution==

This series tries to:
1) Fix existing issue regarding enabling guest supervisor states support.
2) Add CET supervisor state support in core kernel.
3) Introduce guest default features and size for guest fpstate setup.

With the preparation work landed, for guest fpstate, we have xstate_bv[12]
== xcomp_bv[12] == 1 and CET supervisor state is saved/reloaded when
xsaves/xrstors executes on guest fpstate.
For non-guest/normal fpstate, we have xstate_bv[12] == xcomp_bv[12] == 0,
then HW can optimize xsaves/xrstors operations.

==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.

Case 2 is preferred over Case 3 because it can save 24B of CET-S space for all
non-vCPU threads with just a one-line change:

+	fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_SUPERVISOR_GUEST

We believe adding guest defaults has its own merits. It improves readability,
decouples host FPUs and guest FPUs, and arguably enhances extensibility.

[1]: https://lore.kernel.org/all/20240219074733.122080-1-weijiang.yang@intel.com/
[2]: https://lore.kernel.org/all/ZM1jV3UPL0AMpVDI@google.com/
[3]: https://lore.kernel.org/all/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: Drop @perm from guest pseudo FPU container
  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

Sean Christopherson (1):
  x86/fpu/xstate: Always preserve non-user xfeatures/flags in
    __state_perm

Yang Weijiang (3):
  x86/fpu/xstate: Add CET supervisor xfeature support
  x86/fpu/xstate: Introduce "guest-only" supervisor xfeature set
  x86/fpu/xstate: Warn if guest-only supervisor states are detected in
    normal fpstate

 arch/x86/include/asm/fpu/types.h  | 58 ++++++++++++++++++++++---------
 arch/x86/include/asm/fpu/xstate.h |  9 +++--
 arch/x86/kernel/fpu/core.c        | 36 ++++++++++++-------
 arch/x86/kernel/fpu/xstate.c      | 42 +++++++++++++++-------
 arch/x86/kernel/fpu/xstate.h      |  2 ++
 5 files changed, 102 insertions(+), 45 deletions(-)

-- 
2.46.1

[PATCH v4 1/8] x86/fpu/xstate: Always preserve non-user xfeatures/flags in __state_perm

[Chao Gao]

From: Sean Christopherson <seanjc@google.com>

When granting userspace or a KVM guest access to an xfeature, preserve the
entity's existing supervisor and software-defined permissions as tracked
by __state_perm, i.e. use __state_perm to track *all* permissions even
though all supported supervisor xfeatures are granted to all FPUs and
FPU_GUEST_PERM_LOCKED disallows changing permissions.

Effectively clobbering supervisor permissions results in inconsistent
behavior, as xstate_get_group_perm() will report supervisor features for
process that do NOT request access to dynamic user xfeatures, whereas any
and all supervisor features will be absent from the set of permissions for
any process that is granted access to one or more dynamic xfeatures (which
right now means AMX).

The inconsistency isn't problematic because fpu_xstate_prctl() already
strips out everything except user xfeatures:

        case ARCH_GET_XCOMP_PERM:
                /*
                 * Lockless snapshot as it can also change right after the
                 * dropping the lock.
                 */
                permitted = xstate_get_host_group_perm();
                permitted &= XFEATURE_MASK_USER_SUPPORTED;
                return put_user(permitted, uptr);

        case ARCH_GET_XCOMP_GUEST_PERM:
                permitted = xstate_get_guest_group_perm();
                permitted &= XFEATURE_MASK_USER_SUPPORTED;
                return put_user(permitted, uptr);

and similarly KVM doesn't apply the __state_perm to supervisor states
(kvm_get_filtered_xcr0() incorporates xstate_get_guest_group_perm()):

        case 0xd: {
                u64 permitted_xcr0 = kvm_get_filtered_xcr0();
                u64 permitted_xss = kvm_caps.supported_xss;

But if KVM in particular were to ever change, dropping supervisor
permissions would result in subtle bugs in KVM's reporting of supported
CPUID settings.  And the above behavior also means that having supervisor
xfeatures in __state_perm is correctly handled by all users.

Dropping supervisor permissions also creates another landmine for KVM.  If
more dynamic user xfeatures are ever added, requesting access to multiple
xfeatures in separate ARCH_REQ_XCOMP_GUEST_PERM calls will result in the
second invocation of __xstate_request_perm() computing the wrong ksize, as
as the mask passed to xstate_calculate_size() would not contain *any*
supervisor features.

Commit 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor states in XSTATE
permissions") fudged around the size issue for userspace FPUs, but for
reasons unknown skipped guest FPUs.  Lack of a fix for KVM "works" only
because KVM doesn't yet support virtualizing features that have supervisor
xfeatures, i.e. as of today, KVM guest FPUs will never need the relevant
xfeatures.

Simply extending the hack-a-fix for guests would temporarily solve the
ksize issue, but wouldn't address the inconsistency issue and would leave
another lurking pitfall for KVM.  KVM support for virtualizing CET will
likely add CET_KERNEL as a guest-only xfeature, i.e. CET_KERNEL will not
be set in xfeatures_mask_supervisor() and would again be dropped when
granting access to dynamic xfeatures.

Note, the existing clobbering behavior is rather subtle.  The @permitted
parameter to __xstate_request_perm() comes from:

	permitted = xstate_get_group_perm(guest);

which is either fpu->guest_perm.__state_perm or fpu->perm.__state_perm,
where __state_perm is initialized to:

        fpu->perm.__state_perm          = fpu_kernel_cfg.default_features;

and copied to the guest side of things:

	/* Same defaults for guests */
	fpu->guest_perm = fpu->perm;

fpu_kernel_cfg.default_features contains everything except the dynamic
xfeatures, i.e. everything except XFEATURE_MASK_XTILE_DATA:

        fpu_kernel_cfg.default_features = fpu_kernel_cfg.max_features;
        fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC;

When __xstate_request_perm() restricts the local "mask" variable to
compute the user state size:

	mask &= XFEATURE_MASK_USER_SUPPORTED;
	usize = xstate_calculate_size(mask, false);

it subtly overwrites the target __state_perm with "mask" containing only
user xfeatures:

	perm = guest ? &fpu->guest_perm : &fpu->perm;
	/* Pairs with the READ_ONCE() in xstate_get_group_perm() */
	WRITE_ONCE(perm->__state_perm, mask);

Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Acked-by: Dave Hansen <dave.hansen@intel.com>
Cc: Maxim Levitsky <mlevitsk@redhat.com>
Cc: Weijiang Yang <weijiang.yang@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Chao Gao <chao.gao@intel.com>
Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: John Allen <john.allen@amd.com>
Cc: kvm@vger.kernel.org
Link: https://lore.kernel.org/all/ZTqgzZl-reO1m01I@google.com
---
 arch/x86/include/asm/fpu/types.h |  8 +++++---
 arch/x86/kernel/fpu/xstate.c     | 18 +++++++++++-------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index de16862bf230..46cc263f9f4f 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -407,9 +407,11 @@ struct fpu_state_perm {
 	/*
 	 * @__state_perm:
 	 *
-	 * This bitmap indicates the permission for state components, which
-	 * are available to a thread group. The permission prctl() sets the
-	 * enabled state bits in thread_group_leader()->thread.fpu.
+	 * This bitmap indicates the permission for state components
+	 * available to a thread group, including both user and supervisor
+	 * components and software-defined bits like FPU_GUEST_PERM_LOCKED.
+	 * The permission prctl() sets the enabled state bits in
+	 * thread_group_leader()->thread.fpu.
 	 *
 	 * All run time operations use the per thread information in the
 	 * currently active fpu.fpstate which contains the xfeature masks
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 6a41d1610d8b..40621ee4d65b 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1606,16 +1606,20 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest)
 	if ((permitted & requested) == requested)
 		return 0;
 
-	/* Calculate the resulting kernel state size */
+	/*
+	 * Calculate the resulting kernel state size.  Note, @permitted also
+	 * contains supervisor xfeatures even though supervisor are always
+	 * permitted for kernel and guest FPUs, and never permitted for user
+	 * FPUs.
+	 */
 	mask = permitted | requested;
-	/* Take supervisor states into account on the host */
-	if (!guest)
-		mask |= xfeatures_mask_supervisor();
 	ksize = xstate_calculate_size(mask, compacted);
 
-	/* Calculate the resulting user state size */
-	mask &= XFEATURE_MASK_USER_SUPPORTED;
-	usize = xstate_calculate_size(mask, false);
+	/*
+	 * Calculate the resulting user state size.  Take care not to clobber
+	 * the supervisor xfeatures in the new mask!
+	 */
+	usize = xstate_calculate_size(mask & XFEATURE_MASK_USER_SUPPORTED, false);
 
 	if (!guest) {
 		ret = validate_sigaltstack(usize);
-- 
2.46.1

Re: [PATCH v4 1/8] x86/fpu/xstate: Always preserve non-user xfeatures/flags in __state_perm

[Chang S. Bae]

On 3/18/2025 8:31 AM, Chao Gao wrote:
> 
> When granting userspace or a KVM guest access to an xfeature, preserve the
> entity's existing supervisor and software-defined permissions as tracked
> by __state_perm, i.e. use __state_perm to track *all* permissions even
> though all supported supervisor xfeatures are granted to all FPUs and
> FPU_GUEST_PERM_LOCKED disallows changing permissions.
> 
> Effectively clobbering supervisor permissions results in inconsistent
> behavior, as xstate_get_group_perm() will report supervisor features for
> process that do NOT request access to dynamic user xfeatures, whereas any
> and all supervisor features will be absent from the set of permissions for
> any process that is granted access to one or more dynamic xfeatures (which
> right now means AMX).
> 
> The inconsistency isn't problematic because fpu_xstate_prctl() already
> strips out everything except user xfeatures:
> 
>          case ARCH_GET_XCOMP_PERM:
>                  /*
>                   * Lockless snapshot as it can also change right after the
>                   * dropping the lock.
>                   */
>                  permitted = xstate_get_host_group_perm();
>                  permitted &= XFEATURE_MASK_USER_SUPPORTED;
>                  return put_user(permitted, uptr);
> 
>          case ARCH_GET_XCOMP_GUEST_PERM:
>                  permitted = xstate_get_guest_group_perm();
>                  permitted &= XFEATURE_MASK_USER_SUPPORTED;
>                  return put_user(permitted, uptr);
> 
> and similarly KVM doesn't apply the __state_perm to supervisor states
> (kvm_get_filtered_xcr0() incorporates xstate_get_guest_group_perm()):
> 
>          case 0xd: {
>                  u64 permitted_xcr0 = kvm_get_filtered_xcr0();
>                  u64 permitted_xss = kvm_caps.supported_xss;
> 
> But if KVM in particular were to ever change, dropping supervisor
> permissions would result in subtle bugs in KVM's reporting of supported
> CPUID settings.  And the above behavior also means that having supervisor
> xfeatures in __state_perm is correctly handled by all users.
> 
> Dropping supervisor permissions also creates another landmine for KVM.  If
> more dynamic user xfeatures are ever added, requesting access to multiple
> xfeatures in separate ARCH_REQ_XCOMP_GUEST_PERM calls will result in the
> second invocation of __xstate_request_perm() computing the wrong ksize, as
> as the mask passed to xstate_calculate_size() would not contain *any*
> supervisor features.
> 
> Commit 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor states in XSTATE
> permissions") fudged around the size issue for userspace FPUs, but for
> reasons unknown skipped guest FPUs.  Lack of a fix for KVM "works" only
> because KVM doesn't yet support virtualizing features that have supervisor
> xfeatures, i.e. as of today, KVM guest FPUs will never need the relevant
> xfeatures.
> 
> Simply extending the hack-a-fix for guests would temporarily solve the
> ksize issue, but wouldn't address the inconsistency issue and would leave
> another lurking pitfall for KVM.  KVM support for virtualizing CET will
> likely add CET_KERNEL as a guest-only xfeature, i.e. CET_KERNEL will not
> be set in xfeatures_mask_supervisor() and would again be dropped when
> granting access to dynamic xfeatures.
> 
> Note, the existing clobbering behavior is rather subtle.  The @permitted
> parameter to __xstate_request_perm() comes from:
> 
> 	permitted = xstate_get_group_perm(guest);
> 
> which is either fpu->guest_perm.__state_perm or fpu->perm.__state_perm,
> where __state_perm is initialized to:
> 
>          fpu->perm.__state_perm          = fpu_kernel_cfg.default_features;
> 
> and copied to the guest side of things:
> 
> 	/* Same defaults for guests */
> 	fpu->guest_perm = fpu->perm;
> 
> fpu_kernel_cfg.default_features contains everything except the dynamic
> xfeatures, i.e. everything except XFEATURE_MASK_XTILE_DATA:
> 
>          fpu_kernel_cfg.default_features = fpu_kernel_cfg.max_features;
>          fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC;
> 
> When __xstate_request_perm() restricts the local "mask" variable to
> compute the user state size:
> 
> 	mask &= XFEATURE_MASK_USER_SUPPORTED;
> 	usize = xstate_calculate_size(mask, false);
> 
> it subtly overwrites the target __state_perm with "mask" containing only
> user xfeatures:
> 
> 	perm = guest ? &fpu->guest_perm : &fpu->perm;
> 	/* Pairs with the READ_ONCE() in xstate_get_group_perm() */
> 	WRITE_ONCE(perm->__state_perm, mask);

This changelog appears to be largely derived from Sean’s previous email. 
I think it can be significantly shortened to focus on the key
points, such as:

x86/fpu/xstate: Preserve non-user bits in permission handling

When granting userspace or a KVM guest access to an xfeature, the task
leader’s perm->__state_perm (host or guest) is overwritten, 
unintentionally discarding non-user bits. Additionally, supervisor state 
permissions are always granted.

The current behavior presents the following issues:

  *  Inconsistencies in permission handling – Supervisor permissions are
     universally granted, and the FPU_GUEST_PERM_LOCKED bit is explicitly
     set to prevent permission changes.

  *  Redundant permission setting – Since supervisor state permissions
     are always granted, the permitted mask already includes them, making
     it unnecessary to set them again.

Ensure that __xstate_request_perm() does not inadvertently drop
supervisor and software-defined permissions. Also, avoid redundantly
granting supervisor state permissions, and document this behavior in the
code comments.

Clarify the presence of non-user feature and flag bits in the field
description.

Re: [PATCH v4 1/8] x86/fpu/xstate: Always preserve non-user xfeatures/flags in __state_perm

[Sean Christopherson]

On Tue, Apr 01, 2025, Chang S. Bae wrote:
> On 3/18/2025 8:31 AM, Chao Gao wrote:
> > 
> > When granting userspace or a KVM guest access to an xfeature, preserve the
> > entity's existing supervisor and software-defined permissions as tracked
> > by __state_perm, i.e. use __state_perm to track *all* permissions even
> > though all supported supervisor xfeatures are granted to all FPUs and
> > FPU_GUEST_PERM_LOCKED disallows changing permissions.
> > 
> > Effectively clobbering supervisor permissions results in inconsistent
> > behavior, as xstate_get_group_perm() will report supervisor features for
> > process that do NOT request access to dynamic user xfeatures, whereas any
> > and all supervisor features will be absent from the set of permissions for
> > any process that is granted access to one or more dynamic xfeatures (which
> > right now means AMX).
> > 
> > The inconsistency isn't problematic because fpu_xstate_prctl() already
> > strips out everything except user xfeatures:
> > 
> >          case ARCH_GET_XCOMP_PERM:
> >                  /*
> >                   * Lockless snapshot as it can also change right after the
> >                   * dropping the lock.
> >                   */
> >                  permitted = xstate_get_host_group_perm();
> >                  permitted &= XFEATURE_MASK_USER_SUPPORTED;
> >                  return put_user(permitted, uptr);
> > 
> >          case ARCH_GET_XCOMP_GUEST_PERM:
> >                  permitted = xstate_get_guest_group_perm();
> >                  permitted &= XFEATURE_MASK_USER_SUPPORTED;
> >                  return put_user(permitted, uptr);
> > 
> > and similarly KVM doesn't apply the __state_perm to supervisor states
> > (kvm_get_filtered_xcr0() incorporates xstate_get_guest_group_perm()):
> > 
> >          case 0xd: {
> >                  u64 permitted_xcr0 = kvm_get_filtered_xcr0();
> >                  u64 permitted_xss = kvm_caps.supported_xss;
> > 
> > But if KVM in particular were to ever change, dropping supervisor
> > permissions would result in subtle bugs in KVM's reporting of supported
> > CPUID settings.  And the above behavior also means that having supervisor
> > xfeatures in __state_perm is correctly handled by all users.
> > 
> > Dropping supervisor permissions also creates another landmine for KVM.  If
> > more dynamic user xfeatures are ever added, requesting access to multiple
> > xfeatures in separate ARCH_REQ_XCOMP_GUEST_PERM calls will result in the
> > second invocation of __xstate_request_perm() computing the wrong ksize, as
> > as the mask passed to xstate_calculate_size() would not contain *any*
> > supervisor features.
> > 
> > Commit 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor states in XSTATE
> > permissions") fudged around the size issue for userspace FPUs, but for
> > reasons unknown skipped guest FPUs.  Lack of a fix for KVM "works" only
> > because KVM doesn't yet support virtualizing features that have supervisor
> > xfeatures, i.e. as of today, KVM guest FPUs will never need the relevant
> > xfeatures.
> > 
> > Simply extending the hack-a-fix for guests would temporarily solve the
> > ksize issue, but wouldn't address the inconsistency issue and would leave
> > another lurking pitfall for KVM.  KVM support for virtualizing CET will
> > likely add CET_KERNEL as a guest-only xfeature, i.e. CET_KERNEL will not
> > be set in xfeatures_mask_supervisor() and would again be dropped when
> > granting access to dynamic xfeatures.
> > 
> > Note, the existing clobbering behavior is rather subtle.  The @permitted
> > parameter to __xstate_request_perm() comes from:
> > 
> > 	permitted = xstate_get_group_perm(guest);
> > 
> > which is either fpu->guest_perm.__state_perm or fpu->perm.__state_perm,
> > where __state_perm is initialized to:
> > 
> >          fpu->perm.__state_perm          = fpu_kernel_cfg.default_features;
> > 
> > and copied to the guest side of things:
> > 
> > 	/* Same defaults for guests */
> > 	fpu->guest_perm = fpu->perm;
> > 
> > fpu_kernel_cfg.default_features contains everything except the dynamic
> > xfeatures, i.e. everything except XFEATURE_MASK_XTILE_DATA:
> > 
> >          fpu_kernel_cfg.default_features = fpu_kernel_cfg.max_features;
> >          fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC;
> > 
> > When __xstate_request_perm() restricts the local "mask" variable to
> > compute the user state size:
> > 
> > 	mask &= XFEATURE_MASK_USER_SUPPORTED;
> > 	usize = xstate_calculate_size(mask, false);
> > 
> > it subtly overwrites the target __state_perm with "mask" containing only
> > user xfeatures:
> > 
> > 	perm = guest ? &fpu->guest_perm : &fpu->perm;
> > 	/* Pairs with the READ_ONCE() in xstate_get_group_perm() */
> > 	WRITE_ONCE(perm->__state_perm, mask);
> 
> This changelog appears to be largely derived from Sean’s previous email.

FWIW, I wrote the changelog.  I'm sure I derived many of the details from my
original mail, but I would rather not redirect future readers to lore to fully
understand the change.

https://lore.kernel.org/all/20231103224402.347278-1-seanjc@google.com

> I think it can be significantly shortened to focus on the key points, such
> as:

I strongly prefer the extremely verbose version.  I wrote the code, and in all
honesty, the below short version doesn't help me understand the full scope of
the change (I have long since paged out the context).  From a KVM perspective,
capturing why this flaw isn't problematic (yet!) is just as important as fixing
the issue.

> x86/fpu/xstate: Preserve non-user bits in permission handling
> 
> When granting userspace or a KVM guest access to an xfeature, the task
> leader’s perm->__state_perm (host or guest) is overwritten, unintentionally
> discarding non-user bits. Additionally, supervisor state permissions are
> always granted.
> 
> The current behavior presents the following issues:
> 
>  *  Inconsistencies in permission handling – Supervisor permissions are
>     universally granted, and the FPU_GUEST_PERM_LOCKED bit is explicitly
>     set to prevent permission changes.
> 
>  *  Redundant permission setting – Since supervisor state permissions
>     are always granted, the permitted mask already includes them, making
>     it unnecessary to set them again.
> 
> Ensure that __xstate_request_perm() does not inadvertently drop
> supervisor and software-defined permissions. Also, avoid redundantly
> granting supervisor state permissions, and document this behavior in the
> code comments.
> 
> Clarify the presence of non-user feature and flag bits in the field
> description.
> 

[PATCH v4 2/8] x86/fpu: Drop @perm from guest pseudo FPU container

[Chao Gao]

Remove @perm from the guest pseudo FPU container. The field is
initialized during allocation and never used later.

Rename fpu_init_guest_permissions() to show that its sole purpose is to
lock down guest permissions.

Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/include/asm/fpu/types.h | 7 -------
 arch/x86/kernel/fpu/core.c       | 7 ++-----
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 46cc263f9f4f..9f9ed406b179 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -526,13 +526,6 @@ struct fpu_guest {
 	 */
 	u64				xfeatures;
 
-	/*
-	 * @perm:			xfeature bitmap of features which are
-	 *				permitted to be enabled for the guest
-	 *				vCPU.
-	 */
-	u64				perm;
-
 	/*
 	 * @xfd_err:			Save the guest value.
 	 */
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 1b734a9ff088..0b695c23bbfb 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -202,7 +202,7 @@ void fpu_reset_from_exception_fixup(void)
 #if IS_ENABLED(CONFIG_KVM)
 static void __fpstate_reset(struct fpstate *fpstate, u64 xfd);
 
-static void fpu_init_guest_permissions(struct fpu_guest *gfpu)
+static void fpu_lock_guest_permissions(struct fpu_guest *gfpu)
 {
 	struct fpu_state_perm *fpuperm;
 	u64 perm;
@@ -218,8 +218,6 @@ static void fpu_init_guest_permissions(struct fpu_guest *gfpu)
 	WRITE_ONCE(fpuperm->__state_perm, perm | FPU_GUEST_PERM_LOCKED);
 
 	spin_unlock_irq(&current->sighand->siglock);
-
-	gfpu->perm = perm & ~FPU_GUEST_PERM_LOCKED;
 }
 
 bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
@@ -240,7 +238,6 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
 
 	gfpu->fpstate		= fpstate;
 	gfpu->xfeatures		= fpu_kernel_cfg.default_features;
-	gfpu->perm		= fpu_kernel_cfg.default_features;
 
 	/*
 	 * KVM sets the FP+SSE bits in the XSAVE header when copying FPU state
@@ -255,7 +252,7 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
 	if (WARN_ON_ONCE(fpu_user_cfg.default_size > gfpu->uabi_size))
 		gfpu->uabi_size = fpu_user_cfg.default_size;
 
-	fpu_init_guest_permissions(gfpu);
+	fpu_lock_guest_permissions(gfpu);
 
 	return true;
 }
-- 
2.46.1

Re: [PATCH v4 2/8] x86/fpu: Drop @perm from guest pseudo FPU container

[Chang S. Bae]

On 3/18/2025 8:31 AM, Chao Gao wrote:
>   
> -static void fpu_init_guest_permissions(struct fpu_guest *gfpu)
> +static void fpu_lock_guest_permissions(struct fpu_guest *gfpu)
>   {
>   	struct fpu_state_perm *fpuperm;
>   	u64 perm;
> @@ -218,8 +218,6 @@ static void fpu_init_guest_permissions(struct fpu_guest *gfpu)
>   	WRITE_ONCE(fpuperm->__state_perm, perm | FPU_GUEST_PERM_LOCKED);
>   
>   	spin_unlock_irq(&current->sighand->siglock);
> -
> -	gfpu->perm = perm & ~FPU_GUEST_PERM_LOCKED;
>   }

With the removal, the function no longer requires a struct fpu_guest 
argument as it now operates solely on the group leader's FPU state.

Thanks,
Chang

Re: [PATCH v4 2/8] x86/fpu: Drop @perm from guest pseudo FPU container

[Chao Gao]

On Tue, Apr 01, 2025 at 10:16:47AM -0700, Chang S. Bae wrote:
>On 3/18/2025 8:31 AM, Chao Gao wrote:
>> -static void fpu_init_guest_permissions(struct fpu_guest *gfpu)
>> +static void fpu_lock_guest_permissions(struct fpu_guest *gfpu)
>>   {
>>   	struct fpu_state_perm *fpuperm;
>>   	u64 perm;
>> @@ -218,8 +218,6 @@ static void fpu_init_guest_permissions(struct fpu_guest *gfpu)
>>   	WRITE_ONCE(fpuperm->__state_perm, perm | FPU_GUEST_PERM_LOCKED);
>>   	spin_unlock_irq(&current->sighand->siglock);
>> -
>> -	gfpu->perm = perm & ~FPU_GUEST_PERM_LOCKED;
>>   }
>
>With the removal, the function no longer requires a struct fpu_guest argument
>as it now operates solely on the group leader's FPU state.

Good catch! I will drop the fpu_guest argument.

>
>Thanks,
>Chang

[PATCH v4 3/8] x86/fpu/xstate: Add CET supervisor xfeature support

[Chao Gao]

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

To support CET virtualization, KVM needs the kernel to save and restore
the CET supervisor xstate in guest FPUs when switching between guest and
host FPUs.

Add CET supervisor xstate support in preparation for the upcoming CET
virtualization in KVM.

Currently, host FPUs do not utilize the CET supervisor xstate. Enabling
this state for host FPUs would lead to a 24-byte waste in the XSAVE buffer
on CET-capable parts.

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>
---
 arch/x86/include/asm/fpu/types.h  | 14 ++++++++++++--
 arch/x86/include/asm/fpu/xstate.h |  6 +++---
 arch/x86/kernel/fpu/xstate.c      |  5 ++++-
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 9f9ed406b179..d555f89db42f 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,
@@ -141,7 +141,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)
@@ -266,6 +266,16 @@ struct cet_user_state {
 	u64 user_ssp;
 };
 
+/*
+ * State component 12 is Control-flow Enforcement supervisor states
+ */
+struct cet_supervisor_state {
+	/* supervisor ssp pointers  */
+	u64 pl0_ssp;
+	u64 pl1_ssp;
+	u64 pl2_ssp;
+};
+
 /*
  * 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 7f39fe7980c5..8990cf381bef 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -47,7 +47,8 @@
 
 /* All currently supported supervisor features */
 #define XFEATURE_MASK_SUPERVISOR_SUPPORTED (XFEATURE_MASK_PASID | \
-					    XFEATURE_MASK_CET_USER)
+					    XFEATURE_MASK_CET_USER | \
+					    XFEATURE_MASK_CET_KERNEL)
 
 /*
  * A supervisor state component may not always contain valuable information,
@@ -74,8 +75,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 40621ee4d65b..14c3a8285f50 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -55,7 +55,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",
 	"unknown xstate feature",
 	"unknown xstate feature",
 	"unknown xstate feature",
@@ -78,6 +78,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,
 };
@@ -340,6 +341,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)
 
 /*
@@ -540,6 +542,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_XTILE_DATA: check_xtile_data_against_struct(sz); return true;
 	default:
 		XSTATE_WARN_ON(1, "No structure for xstate: %d\n", nr);
-- 
2.46.1

Re: [PATCH v4 3/8] x86/fpu/xstate: Add CET supervisor xfeature support

[Chang S. Bae]

On 3/18/2025 8:31 AM, Chao Gao wrote:
> From: Yang Weijiang <weijiang.yang@intel.com>
> 
> To support CET virtualization, KVM needs the kernel to save and restore
> the CET supervisor xstate in guest FPUs when switching between guest and
> host FPUs.
> 
> Add CET supervisor xstate support in preparation for the upcoming CET
> virtualization in KVM.
> 
> Currently, host FPUs do not utilize the CET supervisor xstate. Enabling
> this state for host FPUs would lead to a 24-byte waste in the XSAVE buffer
> on CET-capable parts.
> 
> 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>

Placing this patch immediately after a few mainline fixes looks to 
suggest that supervisor CET state can be enabled as-is, implying that 
the follow-up patches are merely optional optimizations.

In V2, Dave provided feedback [1] when you placed this patch second out 
of six:

  > This series is starting to look backward to me.
  >
  > The normal way you do these things is that you introduce new
  > abstractions and refactor the code. Then you go adding features.
  >
  > For instance, this series should spend a few patches introducing
  > 'fpu_guest_cfg' and using it before ever introducing the concept of a
  > dynamic xfeature.

In V3, you moved this patch further back to position 8 out of 10. Now, 
in this version, you've placed it at position 3 out of 8.

This raises the question of whether you've fully internalized his advice.

If your intent is to save kernel memory, the xstate infrastructure 
should first be properly adjusted. Specifically:

   1. Initialize the VCPU’s default xfeature set and its XSAVE buffer
      size.

   2. Reference them in the two sites:

     (a) for fpu->guest_perm

     (b) at VCPU allocation time.

   3. Introduce a new feature set (you named "guest supervisor state") as
      a placeholder and integrate it into initialization, along with the
      XSAVE sanity check.

With these adjustments in place, you may consider enabling a new 
xfeature, defining it as a guest-supervisor state simply.

Thanks,
Chang

[1] 
https://lore.kernel.org/kvm/d6127d2e-ea95-4e52-b3db-b39203bad935@intel.com/

Re: [PATCH v4 3/8] x86/fpu/xstate: Add CET supervisor xfeature support

[Chao Gao]

On Tue, Apr 01, 2025 at 10:15:50AM -0700, Chang S. Bae wrote:
>On 3/18/2025 8:31 AM, Chao Gao wrote:
>> From: Yang Weijiang <weijiang.yang@intel.com>
>> 
>> To support CET virtualization, KVM needs the kernel to save and restore
>> the CET supervisor xstate in guest FPUs when switching between guest and
>> host FPUs.
>> 
>> Add CET supervisor xstate support in preparation for the upcoming CET
>> virtualization in KVM.
>> 
>> Currently, host FPUs do not utilize the CET supervisor xstate. Enabling
>> this state for host FPUs would lead to a 24-byte waste in the XSAVE buffer
>> on CET-capable parts.
>> 
>> 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>
>
>Placing this patch immediately after a few mainline fixes looks to suggest
>that supervisor CET state can be enabled as-is, implying that the follow-up
>patches are merely optional optimizations.

Yes, this is intentional. I mentioned it in the cover letter:

"""
Reorder the patches to put the CET supervisor state patch before the
"guest-only" optimization, allowing maintainers to easily adopt or omit the
optimization.
"""

>
>In V2, Dave provided feedback [1] when you placed this patch second out of
>six:

In my opinion, he wasn't referring to the patch introducing the CET supervisor
xstate (i.e., this patch). Rather, he requested that the patch making the CET
supervisor xstate a guest-only feature should follow the introduction of
fpu_guest_cfg and the relevant cleanups.

>
> > This series is starting to look backward to me.
> >
> > The normal way you do these things is that you introduce new
> > abstractions and refactor the code. Then you go adding features.
> >
> > For instance, this series should spend a few patches introducing
> > 'fpu_guest_cfg' and using it before ever introducing the concept of a
> > dynamic xfeature.
>
>In V3, you moved this patch further back to position 8 out of 10. Now, in
>this version, you've placed it at position 3 out of 8.
>
>This raises the question of whether you've fully internalized his advice.
>
>If your intent is to save kernel memory, the xstate infrastructure should
>first be properly adjusted. Specifically:
>
>  1. Initialize the VCPU’s default xfeature set and its XSAVE buffer
>     size.
>
>  2. Reference them in the two sites:
>
>    (a) for fpu->guest_perm
>
>    (b) at VCPU allocation time.
>
>  3. Introduce a new feature set (you named "guest supervisor state") as
>     a placeholder and integrate it into initialization, along with the
>     XSAVE sanity check.
>
>With these adjustments in place, you may consider enabling a new xfeature,
>defining it as a guest-supervisor state simply.

I believe you are suggesting that the CET supervisor xstate should be
introduced 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.

This is a valid point, and I have considered it. However, I chose to split them
into two patches because the guest-only aspect is merely an optimization, and
the decision on whether to accept it is still pending. This order and split-up
make it easier for maintainers to make a decision.

Re: [PATCH v4 3/8] x86/fpu/xstate: Add CET supervisor xfeature support

[Dave Hansen]

On 4/1/25 10:15, Chang S. Bae wrote:
> In V3, you moved this patch further back to position 8 out of 10. Now,
> in this version, you've placed it at position 3 out of 8.
> 
> This raises the question of whether you've fully internalized his advice.

Uh huh.

1. Refactor/fix existing code
2. Add new infrastructure
3. Add new feature

Re: [PATCH v4 3/8] x86/fpu/xstate: Add CET supervisor xfeature support

[Chao Gao]

On Wed, Apr 02, 2025 at 02:37:32PM -0700, Dave Hansen wrote:
>On 4/1/25 10:15, Chang S. Bae wrote:
>> In V3, you moved this patch further back to position 8 out of 10. Now,
>> in this version, you've placed it at position 3 out of 8.
>> 
>> This raises the question of whether you've fully internalized his advice.
>
>Uh huh.
>
>1. Refactor/fix existing code
>2. Add new infrastructure
>3. Add new feature

Okay. Then, Chang's interpretation is accurate. I will organize the series
in this order.

Thank you both.

Re: [PATCH v4 3/8] x86/fpu/xstate: Add CET supervisor xfeature support

[Ingo Molnar]

* Dave Hansen <dave.hansen@intel.com> wrote:

> On 4/1/25 10:15, Chang S. Bae wrote:
> > In V3, you moved this patch further back to position 8 out of 10. Now,
> > in this version, you've placed it at position 3 out of 8.
> > 
> > This raises the question of whether you've fully internalized his advice.
> 
> Uh huh.
> 
> 1. Refactor/fix existing code
> 2. Add new infrastructure
> 3. Add new feature

The more detailed version is:

 - fix bugs
 - clean up code
 - refactor code
 - add new infrastructure
 - add new features

Or in general, the patches are basically hierarchy-sorted by the 
'utility/risk' factor to users, while removing net technological debt.

This is why *sometimes* we'll even do cleanups/refactoring before 
fixes: a fix can become lower-risk if it's on top of a cleaner code 
base.

Thanks,

	Ingo

[PATCH v4 4/8] 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, introduce two new members,
guest_default_features and guest_default_size, in fpu_kernel_cfg to clearly
differentiate the default features for host and guest FPUs.

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.

Note that, 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.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/include/asm/fpu/types.h | 20 ++++++++++++++++++++
 arch/x86/kernel/fpu/xstate.c     | 16 +++++++++++-----
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index d555f89db42f..80647c060b32 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -573,6 +573,16 @@ struct fpu_state_config {
 	 */
 	unsigned int		default_size;
 
+	/*
+	 * @guest_default_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		guest_default_size;
+
 	/*
 	 * @max_features:
 	 *
@@ -589,6 +599,16 @@ struct fpu_state_config {
 	 * be requested by user space before usage.
 	 */
 	u64 default_features;
+
+	/*
+	 * @guest_default_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 guest_default_features;
+
 	/*
 	 * @legacy_features:
 	 *
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 14c3a8285f50..1dd6ddba8723 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -673,7 +673,7 @@ static unsigned int __init get_xsave_size_user(void)
 static int __init init_xstate_size(void)
 {
 	/* Recompute the context size for enabled features: */
-	unsigned int user_size, kernel_size, kernel_default_size;
+	unsigned int user_size, kernel_size;
 	bool compacted = cpu_feature_enabled(X86_FEATURE_XCOMPACTED);
 
 	/* Uncompacted user space size */
@@ -692,18 +692,20 @@ static int __init init_xstate_size(void)
 	else
 		kernel_size = user_size;
 
-	kernel_default_size =
-		xstate_calculate_size(fpu_kernel_cfg.default_features, compacted);
-
 	if (!paranoid_xstate_size_valid(kernel_size))
 		return -EINVAL;
 
 	fpu_kernel_cfg.max_size = kernel_size;
 	fpu_user_cfg.max_size = user_size;
 
-	fpu_kernel_cfg.default_size = kernel_default_size;
+	fpu_kernel_cfg.default_size =
+		xstate_calculate_size(fpu_kernel_cfg.default_features, compacted);
+	fpu_kernel_cfg.guest_default_size =
+		xstate_calculate_size(fpu_kernel_cfg.guest_default_features, compacted);
 	fpu_user_cfg.default_size =
 		xstate_calculate_size(fpu_user_cfg.default_features, false);
+	fpu_user_cfg.guest_default_size =
+		xstate_calculate_size(fpu_user_cfg.guest_default_features, false);
 
 	return 0;
 }
@@ -721,8 +723,10 @@ static void __init fpu__init_disable_system_xstate(unsigned int legacy_size)
 	/* Restore the legacy size.*/
 	fpu_kernel_cfg.max_size = legacy_size;
 	fpu_kernel_cfg.default_size = legacy_size;
+	fpu_kernel_cfg.guest_default_size = legacy_size;
 	fpu_user_cfg.max_size = legacy_size;
 	fpu_user_cfg.default_size = legacy_size;
+	fpu_user_cfg.guest_default_size = legacy_size;
 
 	/*
 	 * Prevent enabling the static branch which enables writes to the
@@ -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;
 
 	/* Store it for paranoia check at the end */
 	xfeatures = fpu_kernel_cfg.max_features;
-- 
2.46.1

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

[Chang S. Bae]

[-- 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;

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

[Chao Gao]

On Tue, Apr 01, 2025 at 10:18:07AM -0700, Chang S. Bae wrote:
>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.

I agree that this hunk is unclear, and your version is much better.

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

Your suggestion looks good to me, and I can definitely incorporate the change
in the next version. Thanks a lot, Chang.

[PATCH v4 5/8] 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.

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

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/kernel/fpu/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 0b695c23bbfb..52df97a8a61b 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -542,8 +542,10 @@ 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	= fpu_kernel_cfg.guest_default_features;
+	fpu->guest_perm.__state_size	= fpu_kernel_cfg.guest_default_size;
+	fpu->guest_perm.__user_state_size = fpu_user_cfg.guest_default_size;
 }
 
 static inline void fpu_inherit_perms(struct fpu *dst_fpu)
-- 
2.46.1

[PATCH v4 6/8] 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. Update the
function to use guest defaults instead.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/kernel/fpu/core.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 52df97a8a61b..b00e4032d75f 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -200,7 +200,16 @@ 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		= fpu_kernel_cfg.guest_default_size;
+	fpstate->user_size	= fpu_user_cfg.guest_default_size;
+	fpstate->xfeatures	= fpu_kernel_cfg.guest_default_features;
+	fpstate->user_xfeatures	= fpu_user_cfg.guest_default_features;
+	fpstate->xfd		= xfd;
+}
+
 
 static void fpu_lock_guest_permissions(struct fpu_guest *gfpu)
 {
@@ -225,19 +234,21 @@ 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 = fpu_kernel_cfg.guest_default_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		= fpu_kernel_cfg.guest_default_features;
 
 	/*
 	 * KVM sets the FP+SSE bits in the XSAVE header when copying FPU state
@@ -249,8 +260,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(fpu_user_cfg.guest_default_size > gfpu->uabi_size))
+		gfpu->uabi_size = fpu_user_cfg.guest_default_size;
 
 	fpu_lock_guest_permissions(gfpu);
 
-- 
2.46.1

[PATCH v4 7/8] x86/fpu/xstate: Introduce "guest-only" supervisor xfeature set

[Chao Gao]

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

Define a new XFEATURE_MASK_SUPERVISOR_GUEST mask to specify the features
that are enabled by default in guest FPUs but not in host FPUs.

Add CET_KERNEL as the first guest-only feature to save host FPUs from
allocating XSAVE buffer space for all threads on CET-capable parts.

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>
---
Dropped Dave's Suggested-by as the patch has been changed significantly
---
 arch/x86/include/asm/fpu/types.h  | 9 +++++----
 arch/x86/include/asm/fpu/xstate.h | 3 +++
 arch/x86/kernel/fpu/xstate.c      | 5 ++++-
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 80647c060b32..079f3241e25b 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -568,8 +568,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;
 
@@ -595,8 +596,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 8990cf381bef..69db17476061 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -45,6 +45,9 @@
 /* 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_SUPERVISOR_GUEST	XFEATURE_MASK_CET_KERNEL
+
 /* All currently supported supervisor features */
 #define XFEATURE_MASK_SUPERVISOR_SUPPORTED (XFEATURE_MASK_PASID | \
 					    XFEATURE_MASK_CET_USER | \
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 1dd6ddba8723..b19960215074 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -55,7 +55,7 @@ static const char *xfeature_names[] =
 	"Protection Keys User registers",
 	"PASID state",
 	"Control-flow User registers",
-	"Control-flow Kernel registers",
+	"Control-flow Kernel registers (KVM only)",
 	"unknown xstate feature",
 	"unknown xstate feature",
 	"unknown xstate feature",
@@ -813,6 +813,9 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
 	fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC;
 	fpu_kernel_cfg.guest_default_features = fpu_kernel_cfg.default_features;
 
+	/* Clean out guest-only features from default */
+	fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_SUPERVISOR_GUEST;
+
 	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;
-- 
2.46.1

Re: [PATCH v4 7/8] x86/fpu/xstate: Introduce "guest-only" supervisor xfeature set

[Chang S. Bae]

On 3/18/2025 8:31 AM, Chao Gao wrote:
> 
> Dropped Dave's Suggested-by as the patch has been changed significantly

I think you should provide a clear argument outlining the considerable 
naming options and their trade-offs.

I noticed you referenced Thomas’s feedback in the cover letter (it would 
be clearer to elaborate here rather than using just the above one-liner):

 > Rename XFEATURE_MASK_KERNEL_DYNAMIC to XFEATURE_MASK_SUPERVISOR_GUEST
 > as tglx noted "this dynamic naming is really bad":
 >
 > https://lore.kernel.org/all/87sg1owmth.ffs@nanos.tec.linutronix.de/

While Thomas objected to the "dynamic" naming, have you fully considered 
why he found it problematic? Likewise, have you re-evaluated Dave’s 
original suggestion and his intent? Rather than just quoting feedback, 
you should summarize the key concerns, analyze the pros and cons of 
different naming approaches, and clearly justify your final choice.

Thanks,
Chang

Re: [PATCH v4 7/8] x86/fpu/xstate: Introduce "guest-only" supervisor xfeature set

[Chao Gao]

On Tue, Apr 01, 2025 at 10:16:24AM -0700, Chang S. Bae wrote:
>On 3/18/2025 8:31 AM, Chao Gao wrote:
>> 
>> Dropped Dave's Suggested-by as the patch has been changed significantly
>
>I think you should provide a clear argument outlining the considerable naming
>options and their trade-offs.
>
>I noticed you referenced Thomas’s feedback in the cover letter (it would be
>clearer to elaborate here rather than using just the above one-liner):
>
>> Rename XFEATURE_MASK_KERNEL_DYNAMIC to XFEATURE_MASK_SUPERVISOR_GUEST
>> as tglx noted "this dynamic naming is really bad":
>>
>> https://lore.kernel.org/all/87sg1owmth.ffs@nanos.tec.linutronix.de/
>
>While Thomas objected to the "dynamic" naming, have you fully considered why
>he found it problematic? Likewise, have you re-evaluated Dave’s original
>suggestion and his intent? Rather than just quoting feedback, you should
>summarize the key concerns, analyze the pros and cons of different naming
>approaches, and clearly justify your final choice.

Hi Chang,

The 'dynamic' naming was initially slightly preferred over 'guest-only'. But
later I discovered new evidence suggesting we should be cautious with the
'dynamic' naming, leading me to choose 'guest-only'.

'dynamic' is abstract, while 'guest-only' more clearly conveys the intended
purpose. Using "dynamic" might cause confusion, as it could be associated with
dynamic user features. As you noted in the last version, it is quite confusing
because it doesn't involve permissions and reallocations like dynamic user
features do.

I'm not entirely sure why Thomas found "dynamic" problematic. His comment, made
4 years ago, was about independent features. But I am sure that we shouldn't
reinstate a name that was considered "really bad" without strong justification.

And Dave clearly mentioned he wouldn't oppose the "guest-only" name [1]:

  But I also don't feel strongly about it and I've said my peace.  I won't NAK
  it one way or the other.

Therefore, to be cautious, I chose "guest-only," assuming it is acceptable to
Dave and can prevent Thomas and others from questioning the reinstatement of
dynamic supervisor features.

I can add my thoughts below the --- separator line if the above answers your
questions.

[1]: https://lore.kernel.org/all/893ac578-baaf-4f4f-96ee-e012dfc073a8@intel.com/#t

[PATCH v4 8/8] x86/fpu/xstate: Warn if guest-only supervisor states are detected in normal fpstate

[Chao Gao]

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

guest-only supervisor state bits should be __ONLY__ enabled for guest
fpstate, i.e., never for normal kernel fpstate. WARN_ONCE() if normal
kernel fpstate sees any of these features.

Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/kernel/fpu/xstate.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 1418423bc4c9..f644647c0549 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -208,6 +208,8 @@ static inline void os_xsave(struct fpstate *fpstate)
 	WARN_ON_FPU(!alternatives_patched);
 	xfd_validate_state(fpstate, mask, false);
 
+	WARN_ON_FPU(!fpstate->is_guest && (mask & XFEATURE_MASK_SUPERVISOR_GUEST));
+
 	XSTATE_XSAVE(&fpstate->regs.xsave, lmask, hmask, err);
 
 	/* We should never fault when copying to a kernel buffer: */
-- 
2.46.1

Re: [PATCH v4 8/8] x86/fpu/xstate: Warn if guest-only supervisor states are detected in normal fpstate

[Chang S. Bae]

On 3/18/2025 8:31 AM, Chao Gao wrote:
>   
> +	WARN_ON_FPU(!fpstate->is_guest && (mask & XFEATURE_MASK_SUPERVISOR_GUEST));

Did you check xfeatures_mask_supervisor()? I think you might want to 
introduce a similar wrapper to reference the enabled features 
(max_features) here.

Also, have you audited other code paths to ensure that no additional 
guard like this is needed? Can you summarize your audit process?

Thanks,
Chang

Re: [PATCH v4 8/8] x86/fpu/xstate: Warn if guest-only supervisor states are detected in normal fpstate

[Chao Gao]

On Tue, Apr 01, 2025 at 10:17:08AM -0700, Chang S. Bae wrote:
>On 3/18/2025 8:31 AM, Chao Gao wrote:
>> +	WARN_ON_FPU(!fpstate->is_guest && (mask & XFEATURE_MASK_SUPERVISOR_GUEST));
>
>Did you check xfeatures_mask_supervisor()? I think you might want to
>introduce a similar wrapper to reference the enabled features (max_features)
>here.

Are you suggesting something like this:

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

and do
	WARN_ON_FPU(!fpstate->is_guest && (mask & xfeatures_mask_guest_supervisor()));

?

If so, I don't think it's necessary. The check in the current patch is actually
stricter and could catch more errors than the suggested one.

>
>Also, have you audited other code paths to ensure that no additional guard
>like this is needed? Can you summarize your audit process?

I didn't audit all code paths. I took over this patch and made only very slight
modifications to it. Anyway, thanks for asking this.

The goal is to ensure that guest-only _supervisor_ features are not enabled in
non-guest FPUs.

There are two potential solutions:

a) Check the enabled features during save/restore operations, i.e., when
   executing XSAVES/XRSTORS instructions. This patch adopts this solution, but
   it is partial.

XSAVES/XRSTORS instructions are executed in following places:

  1) os_xrstor_booting()
  2) xsaves()
  3) xrstors()
  4) os_xrstor_safe()
  5) os_xsave()
  6) os_xrstor()
  7) os_xrstor_supervisor()

#2 and #3 are not applicable because they handle independent features only,
which are not associated with guest or non-guest FPUs.

Checks can be applied to #1 and #4-#7, although #1 needs to be refactored first
to accept an fpstate argument.

b) Check the enabled features during initialization and reallocation, i.e.,
whenever fpstate->xfeatures is assigned some value in following functions:

  __fpstate_reset()
  __fpstate_guest_reset()
  fpu__init_system_xstate()
  fpstate_realloc()

We can add a check within these functions or integrate the check into
xstate_init_xcomp_bv(), as it is always called after fpstate->xfeatures is
updated.

IMO, option b) is slightly better because it can catch errors earlier than
option a), allowing the call trace to accurately reflect how the issue arose.

Chang, which option do you prefer, or do you have any other suggestions?

>
>Thanks,
>Chang

Re: [PATCH v4 8/8] x86/fpu/xstate: Warn if guest-only supervisor states are detected in normal fpstate

[Chang S. Bae]

On 4/2/2025 7:30 AM, Chao Gao wrote:
> 
> The goal is to ensure that guest-only _supervisor_ features are not enabled in
> non-guest FPUs.

I think the common XSAVE path matters here. The other XSAVE paths — 
signal delivery and saving the LBR state — already handle supervisor 
states properly. Signal delivery excludes all of supervisor states from 
the RFBM, while LBR state saving triggers a warning if any non-LBR state 
is set in the RFBM. Given this, the guard seems good enough unless 
missing something.

Thanks,
Chang

Re: [PATCH v4 8/8] x86/fpu/xstate: Warn if guest-only supervisor states are detected in normal fpstate

[Dave Hansen]

On 4/3/25 17:02, Chang S. Bae wrote:
> On 4/2/2025 7:30 AM, Chao Gao wrote:
>> The goal is to ensure that guest-only _supervisor_ features are
>> not enabled in non-guest FPUs.
> 
> I think the common XSAVE path matters here. The other XSAVE paths — 
> signal delivery and saving the LBR state — already handle supervisor 
> states properly. Signal delivery excludes all of supervisor states
> from the RFBM, while LBR state saving triggers a warning if any non-
> LBR state is set in the RFBM. Given this, the guard seems good
> enough unless missing something.
This patch is looking more and more optional. There's no broad consensus
on the need for a warning or exactly what it should warn on.

I think it should be dropped.

Re: [PATCH v4 0/8] Introduce CET supervisor state support

[Chang S. Bae]

On 3/18/2025 8:31 AM, Chao Gao wrote:
> 
>   arch/x86/include/asm/fpu/types.h  | 58 ++++++++++++++++++++++---------
>   arch/x86/include/asm/fpu/xstate.h |  9 +++--
>   arch/x86/kernel/fpu/core.c        | 36 ++++++++++++-------
>   arch/x86/kernel/fpu/xstate.c      | 42 +++++++++++++++-------
>   arch/x86/kernel/fpu/xstate.h      |  2 ++
>   5 files changed, 102 insertions(+), 45 deletions(-)

Hi Chao Gao,

I've left a few comments on your patches. It looks like the structure of 
your patch set has been shifting between postings. I’d recommend 
reviewing the build-up of changes more carefully — see my reply on patch3.

Additionally, your approach to the ongoing discussion comes across as 
somewhat reserved. Since you’re presenting a counterpoint to the 
maintainer’s suggestion, I’d encourage you to articulate your reasoning 
more clearly (see my comment on patch 7).

Most of other feedback focuses on refining individual changes. 
Specifically, your upcoming modifications to the initialization sequence 
do not look super clear. My suggestions in patch 4 reflect my 
interpretation, but please consider whether there's a clearer way to 
present the new configuration settings.

Thanks,
Chang

Re: [PATCH v4 0/8] Introduce CET supervisor state support

[Edgecombe, Rick P]

Xin and I were discussing the dependencies we have going here.

For arch reasons, CET supervisor shadow stack was waiting for FRED support. And
for KVM development process reasons KVM FRED support is waiting for KVM CET
support. It’s almost a circle.


It looks like Chao has re-arranged the patches such that the space saving
optimization could be dropped. It would just look like switching to the pretty
trivial patches 1-3 I guess. However, he didn’t actually advocate for that to
happen. But the subtext seems to be that it should be considered?

I think there aren't concerns with the bones of the optimization in the later
patches. It’s just polishing that is needed. But some of the polishing needed
(i.e. the long debated naming of for the guest category of features) is
downstream of the growing complexity of the FPU, which we were planning to
accept. So maybe it’s turning out more costly than we expected?

In any case, at this point I think we need to either double down on polishing
this thing up (by pausing other work) and have a clear “please do this with
these patches" request, or declare failure and argue for the smaller version.

I guess I still lean towards keeping the optimization. But I do think it's worth
considering at this point.

Rick

Re: [PATCH v4 0/8] Introduce CET supervisor state support

[Dave Hansen]

On 4/2/25 14:12, Edgecombe, Rick P wrote:
> In any case, at this point I think we need to either double down on polishing
> this thing up (by pausing other work) and have a clear “please do this with
> these patches" request, or declare failure and argue for the smaller version.
> 
> I guess I still lean towards keeping the optimization. But I do think it's worth
> considering at this point.

I'm not quite feeling the same sense of panic and some need to pause
other work. This is thing is at v4. Between spring break and the merge
window, v4 hasn't gotten many eyeballs, except Chang's (thanks Chang!).

Re: [PATCH v4 0/8] Introduce CET supervisor state support

[Sean Christopherson]

On Wed, Apr 02, 2025, Dave Hansen wrote:
> On 4/2/25 14:12, Edgecombe, Rick P wrote:
> > In any case, at this point I think we need to either double down on polishing
> > this thing up (by pausing other work) and have a clear “please do this with
> > these patches" request, or declare failure and argue for the smaller version.
> > 
> > I guess I still lean towards keeping the optimization. But I do think it's worth
> > considering at this point.
> 
> I'm not quite feeling the same sense of panic and some need to pause
> other work. This is thing is at v4. Between spring break and the merge
> window, v4 hasn't gotten many eyeballs, except Chang's (thanks Chang!).

This particular series is at v4, but KVM CET support was more or less ready to
roll 4 *years* ago.  I agree there's no need to panic, but some sense of urgency
would be nice.