Introduce CET supervisor state support
v6 cover Introduce CET supervisor state support
Dear maintainers and reviewers,
I kindly request your consideration for merging this series. The changes between v5 (posted one month ago) and v6 are minimal, and most of the patches have received Reviewed-by/Acked-by tags.
Thanks Chang, Rick, Xin and Sean for their help with this series.
== Changelog ==v5->v6:
- Collect reviews from Chang and Rick
- Reset guest default size to FPU legacy size when XSAVE is not available or not enabled (Chang)
- Drop guest default user features and size, as they will not differ from host FPUs (Rick)
- v5: https://lore.kernel.org/kvm/[email protected]/
== 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.
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/[email protected]/
[2]: https://lore.kernel.org/kvm/[email protected]/
[3]: https://lore.kernel.org/kvm/[email protected]/
[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 (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 | 64 +++++++++++++++++++++++-------- arch/x86/include/asm/fpu/xstate.h | 9 +++-- arch/x86/kernel/fpu/core.c | 46 +++++++++++++++------- arch/x86/kernel/fpu/init.c | 1 + arch/x86/kernel/fpu/xstate.c | 58 +++++++++++++++++++++------- arch/x86/kernel/fpu/xstate.h | 5 +++ 6 files changed, 137 insertions(+), 46 deletions(-)
base-commit: 960bc2bcba5987a82530b9756e1f602a894cffa4
Signature · 1 line
2.47.1
PATCH v6 1/7 x86/fpu/xstate: Always preserve non-user xfeatures/flags in __state_perm
From: Sean Christopherson <[email protected]>
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);
--- arch/x86/include/asm/fpu/types.h | 8 +++++--- arch/x86/kernel/fpu/xstate.c | 18 +++++++++++------- 2 files changed, 16 insertions(+), 10 deletions(-)
arch/x86/include/asm/fpu/types.h +5 -3
@@ -416,9 +416,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
arch/x86/kernel/fpu/xstate.c +11 -7
@@ -1656,16 +1656,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);
Signature · 1 line
2.47.1
[tip: x86/fpu] x86/fpu/xstate: Always preserve non-user xfeatures/flags in __state_perm
The following commit has been merged into the x86/fpu branch of tip:
The following commit has been merged into the x86/fpu branch of tip:
Commit-ID: d8414603b29f25191fcceafb72e74b67ac2e92b1 Gitweb: https://git.kernel.org/tip/d8414603b29f25191fcceafb72e74b67ac2e92b1 Author: Sean Christopherson <[email protected]> AuthorDate: Tue, 06 May 2025 17:36:06 +08:00 Committer: Ingo Molnar <[email protected]> CommitterDate: Tue, 06 May 2025 11:42:04 +02:00
x86/fpu/xstate: Always preserve non-user xfeatures/flags in __state_perm
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);
--- arch/x86/include/asm/fpu/types.h | 8 +++++--- arch/x86/kernel/fpu/xstate.c | 18 +++++++++++------- 2 files changed, 16 insertions(+), 10 deletions(-)
arch/x86/include/asm/fpu/types.h +5 -3
@@ -416,9 +416,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
arch/x86/kernel/fpu/xstate.c +11 -7
@@ -1656,16 +1656,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);
PATCH v6 2/7 x86/fpu: Drop @perm from guest pseudo FPU container
In reply to Chao GaoRemove @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.
---
v6: add Chang's Reviewed-by
v5: drop the useless fpu_guest argument (Chang)
--- arch/x86/include/asm/fpu/types.h | 7 ------- arch/x86/kernel/fpu/core.c | 7 ++----- 2 files changed, 2 insertions(+), 12 deletions(-)
arch/x86/include/asm/fpu/types.h +0 -7
@@ -535,13 +535,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. */
arch/x86/kernel/fpu/core.c +2 -5
@@ -212,7 +212,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 (void ) {struct fpu_state_perm *fpuperm;u64 perm;
@@ -228,8 +228,6 @@ static void fpu_init_guest_permissions(struct fpu_guest *gfpu)WRITE_ONCE (fpuperm->__state_perm, perm |FPU_GUEST_PERM_LOCKED );spin_unlock_irq (¤t->sighand->siglock); - - gfpu->perm = perm & ~FPU_GUEST_PERM_LOCKED ; }bool fpu_alloc_guest_fpstate (struct fpu_guest *gfpu)
@@ -250,7 +248,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
@@ -265,7 +262,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 ();return true; }
Signature · 1 line
2.47.1
[tip: x86/fpu] x86/fpu: Drop @perm from guest pseudo FPU container
The following commit has been merged into the x86/fpu branch of tip:
The following commit has been merged into the x86/fpu branch of tip:
Commit-ID: 32d5fa804dc9bd7cf6651a1378ba616d332e7444 Gitweb: https://git.kernel.org/tip/32d5fa804dc9bd7cf6651a1378ba616d332e7444 Author: Chao Gao <[email protected]> AuthorDate: Tue, 06 May 2025 17:36:07 +08:00 Committer: Ingo Molnar <[email protected]> CommitterDate: Tue, 06 May 2025 11:52:22 +02:00
x86/fpu: Drop @perm from guest pseudo FPU container
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.
--- arch/x86/include/asm/fpu/types.h | 7 ------- arch/x86/kernel/fpu/core.c | 7 ++----- 2 files changed, 2 insertions(+), 12 deletions(-)
arch/x86/include/asm/fpu/types.h +0 -7
@@ -536,13 +536,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. */ u64 xfd_err;
arch/x86/kernel/fpu/core.c +2 -5
@@ -212,7 +212,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 (void ) {struct fpu_state_perm *fpuperm;u64 perm;
@@ -228,8 +228,6 @@ static void fpu_init_guest_permissions(struct fpu_guest *gfpu)WRITE_ONCE (fpuperm->__state_perm, perm |FPU_GUEST_PERM_LOCKED );spin_unlock_irq (¤t->sighand->siglock); - - gfpu->perm = perm & ~FPU_GUEST_PERM_LOCKED ; }bool fpu_alloc_guest_fpstate (struct fpu_guest *gfpu)
@@ -250,7 +248,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
@@ -265,7 +262,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 ();return true; }
PATCH v6 3/7 x86/fpu/xstate: Differentiate default features for host and guest FPUs
In reply to Chao GaoCurrently, 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.
---
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(-)
arch/x86/include/asm/fpu/types.h +26 -0
@@ -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 */
arch/x86/kernel/fpu/core.c +1 -0
@@ -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,
arch/x86/kernel/fpu/init.c +1 -0
@@ -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; }/*
arch/x86/kernel/fpu/xstate.c +21 -6
@@ -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;
Signature · 1 line
2.47.1
Re: [PATCH v6 3/7] x86/fpu/xstate: Differentiate default features for host and guest FPUs
Quoted 19 lines · 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_configstruct does not include default user features and size for the UABI buffer.
Quoted 7 lines
An alternative approach is adding a
guest_only_xfeaturesmember tofpu_kernel_cfgand 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 <[email protected]>Signed-off-by Chao Gao <[email protected]>
PATCH v6 4/7 x86/fpu: Initialize guest FPU permissions from guest defaults
In reply to Chao GaoCurrently, 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.
---
v6: Drop vcpu_fpu_config.user_* and collect reviews (Rick)
--- arch/x86/kernel/fpu/core.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
arch/x86/kernel/fpu/core.c +8 -2
@@ -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)
Signature · 1 line
2.47.1
PATCH v6 5/7 x86/fpu: Initialize guest fpstate and FPU pseudo container from guest defaults
In reply to Chao Gaofpu_alloc_guest_fpstate() currently uses host defaults to initialize guest fpstate and pseudo containers. Guest defaults were introduced to differentiate the features and sizes of host and guest FPUs. Switch to using guest defaults instead.
Additionally, incorporate the initialization of indicators (is_valloc and is_guest) into the newly added guest-specific reset function to centralize the resetting of guest fpstate.
---
v6: Drop vcpu_fpu_config.user_* (Rick)
v5: init is_valloc/is_guest in the guest-specific reset function (Chang)
--- arch/x86/kernel/fpu/core.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)
arch/x86/kernel/fpu/core.c +22 -6
@@ -211,7 +211,24 @@ void fpu_reset_from_exception_fixup(void) }#if IS_ENABLED (CONFIG_KVM ) -static void __fpstate_reset (struct fpstate *fpstate,u64 xfd); +static void __guest_fpstate_reset (struct fpstate *fpstate,u64 xfd) +{ +/* +* Initialize sizes and feature masks. Supervisor features and +* sizes may diverge between guest FPUs and host FPUs, whereas +* user features and sizes remain the same. +*/ + fpstate->size = guest_default_cfg.size; + fpstate->xfeatures = guest_default_cfg.features; + fpstate->user_size = fpu_user_cfg.default_size; + fpstate->user_xfeatures = fpu_user_cfg.default_features; + fpstate->xfd = xfd; + +/* Initialize indicators to reflect properties of the fpstate */ + fpstate->is_valloc = true; + fpstate->is_guest = true; +} +static void fpu_lock_guest_permissions (void ) {
@@ -236,19 +253,18 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)struct fpstate *fpstate;unsigned int size; - size = fpu_kernel_cfg.default_size +ALIGN (offsetof(struct fpstate , regs),64 ); + size = guest_default_cfg.size +ALIGN (offsetof(struct fpstate , regs),64 ); + fpstate =vzalloc (size);if (!fpstate)return false;/* Leave xfd to 0 (the reset value defined by spec) */ -__fpstate_reset (fpstate,0 ); +__guest_fpstate_reset (fpstate,0 );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
Signature · 1 line
2.47.1
Sean Christopherson
reviewerRe: [PATCH v6 5/7] x86/fpu: Initialize guest fpstate and FPU pseudo container from guest defaults
Quoted 35 lines · 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.Additionally, incorporate the initialization of indicators (
is_vallocandis_guest) into the newly added guest-specific reset function to centralize the resetting of guest fpstate.Suggested-by Chang S. Bae <[email protected]>Signed-off-by Chao Gao <[email protected]>---
v6: Drop vcpu_fpu_config.user_* (Rick)
v5: init is_valloc/is_guest in the guest-specific reset function (Chang)--- arch/x86/kernel/fpu/core.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)arch/x86/kernel/fpu/core.c +8
-1@@ -211,7 +211,24 @@ void fpu_reset_from_exception_fixup(void) }#if IS_ENABLED (CONFIG_KVM ) -static void __fpstate_reset (struct fpstate *fpstate,u64 xfd); +static void __guest_fpstate_reset (struct fpstate *fpstate,u64 xfd) +{ +/* +* Initialize sizes and feature masks. Supervisor features and +* sizes may diverge between guest FPUs and host FPUs, whereas +* user features and sizes remain the same. +*/ + fpstate->size = guest_default_cfg.size;
arch/x86/kernel/fpu/core.c void fpu_reset_from_exception_fixup(void) · line 231
Quoted 10 lines
arch/x86/kernel/fpu/core.c +10
-0+ fpstate->xfeatures = guest_default_cfg.features; + fpstate->user_size = fpu_user_cfg.default_size; + fpstate->user_xfeatures = fpu_user_cfg.default_features; + fpstate->xfd = xfd; + +/* Initialize indicators to reflect properties of the fpstate */ + fpstate->is_valloc = true; + fpstate->is_guest = true; +} +
Extra newline.
Quoted 7 lines
arch/x86/kernel/fpu/core.c +0
-0static void fpu_lock_guest_permissions (void ) {@@ -236,19 +253,18 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)struct fpstate *fpstate;unsigned int size;
arch/x86/kernel/fpu/core.c bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu) · line 263
Quoted 10 lines
arch/x86/kernel/fpu/core.c +3
-2- size = fpu_kernel_cfg.default_size +ALIGN (offsetof(struct fpstate , regs),64 ); + size = guest_default_cfg.size +ALIGN (offsetof(struct fpstate , regs),64 ); + fpstate =vzalloc (size);if (!fpstate)return false;/* Leave xfd to 0 (the reset value defined by spec) */ -__fpstate_reset (fpstate,0 ); +__guest_fpstate_reset (fpstate,0 );
Given that there is a single caller for each of __fpstate_reset() and __guest_fpstate_reset(), keeping the helpers does more harm than good IMO. Passing in '0' and setting xfd in __guest_fpstate_reset() is especially pointless.
E.g.
arch/x86/kernel/fpu/core.c +21 -32
@@ -212,25 +212,6 @@ void fpu_reset_from_exception_fixup(void) }#if IS_ENABLED (CONFIG_KVM ) -static void __guest_fpstate_reset (struct fpstate *fpstate,u64 xfd) -{ -/* -* Initialize sizes and feature masks. Supervisor features and -* sizes may diverge between guest FPUs and host FPUs, whereas -* user features and sizes remain the same. -*/ - fpstate->size = guest_default_cfg.size; - fpstate->xfeatures = guest_default_cfg.features; - fpstate->user_size = fpu_user_cfg.default_size; - fpstate->user_xfeatures = fpu_user_cfg.default_features; - fpstate->xfd = xfd; - -/* Initialize indicators to reflect properties of the fpstate */ - fpstate->is_valloc = true; - fpstate->is_guest = true; -} - -static void fpu_lock_guest_permissions (void ) {struct fpu_state_perm *fpuperm;
@@ -260,8 +241,20 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)if (!fpstate)return false; -/* Leave xfd to 0 (the reset value defined by spec) */ -__guest_fpstate_reset (fpstate,0 ); +/* Initialize indicators to reflect properties of the fpstate */ + fpstate->is_valloc = true; + fpstate->is_guest = true; + +/* +* Initialize sizes and feature masks. Supervisor features and sizes +* may diverge between guest FPUs and host FPUs, whereas user features +* and sizes are always identical the same. +*/ + fpstate->size = guest_default_cfg.size; + fpstate->xfeatures = guest_default_cfg.features; + fpstate->user_size = fpu_user_cfg.default_size; + fpstate->user_xfeatures = fpu_user_cfg.default_features; +fpstate_init_user (fpstate); gfpu->fpstate = fpstate;
@@ -550,21 +543,17 @@ void fpstate_init_user(struct fpstate *fpstate)fpstate_init_fstate (fpstate); } -static void __fpstate_reset (struct fpstate *fpstate,u64 xfd) -{ -/* Initialize sizes and feature masks */ - fpstate->size = fpu_kernel_cfg.default_size; - fpstate->user_size = fpu_user_cfg.default_size; - fpstate->xfeatures = fpu_kernel_cfg.default_features; - fpstate->user_xfeatures = fpu_user_cfg.default_features; - fpstate->xfd = xfd; -} -void fpstate_reset (struct fpu *fpu) {/* Set the fpstate pointer to the default fpstate */ fpu->fpstate = &fpu->__fpstate; -__fpstate_reset (fpu->fpstate, init_fpstate.xfd); + +/* Initialize sizes and feature masks */ + fpu->fpstate->size = fpu_kernel_cfg.default_size; + fpu->fpstate->user_size = fpu_user_cfg.default_size; + fpu->fpstate->xfeatures = fpu_kernel_cfg.default_features; + fpu->fpstate->user_xfeatures = fpu_user_cfg.default_features; + fpu->fpstate->xfd = init_fpstate.xfd;/* Initialize the permission related info in fpu */ fpu->perm.__state_perm = fpu_kernel_cfg.default_features;
Re: [PATCH v6 5/7] x86/fpu: Initialize guest fpstate and FPU pseudo container from guest defaults
Quoted 13 lines
Quoted 13 lines
static void
fpu_lock_guest_permissions(void)
{(quoted diff) +2
-1@@ -236,19 +253,18 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu) struct fpstate *fpstate; unsigned int size; - size = fpu_kernel_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64); + size = guest_default_cfg.size + ALIGN(offsetof(struct fpstate, regs), 64); + fpstate = vzalloc(size); if (!fpstate) return false;
Quoted 6 lines
Review bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu) · line 263
Quoted 3 lines
(quoted diff) +1
-1/* Leave xfd to 0 (the reset value defined by spec) */ - __fpstate_reset(fpstate, 0); + __guest_fpstate_reset(fpstate, 0);Given that there is a single caller for each of
__fpstate_reset()and__guest_fpstate_reset(), keeping the helpers does more harm than good IMO.
Seems Dave strongly dislikes inlining the helpers
https://lore.kernel.org/kvm/[email protected]/
Quoted 1 line
Passing in '0' and setting xfd in
__guest_fpstate_reset()is especially pointless.
Agreed.
Giving Dave's feedback, how about tweaking __fpstate_reset() for guest FPUs:
From 13eddc8a0ecbd9e2a59dc1caacc7dbf198277a24 Mon Sep 17 00:00:00 2001
From: Chao Gao <[email protected]>
Date: Fri, 31 May 2024 02:03:30 -0700
Subject: [PATCH] x86/fpu: Initialize guest fpstate and FPU pseudo container
from guest defaults
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.
--- arch/x86/kernel/fpu/core.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)
arch/x86/kernel/fpu/core.c +20 -7
@@ -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; }
Signature · 1 line
2.47.1
Sean Christopherson
reviewerRe: [PATCH v6 5/7] x86/fpu: Initialize guest fpstate and FPU pseudo container from guest defaults
Quoted 59 lines · Chao Gao
From: Chao Gao <[email protected]>
Date: Fri, 31 May 2024 02:03:30 -0700
Subject: [PATCH] x86/fpu: Initialize guest fpstate and FPU pseudo container
from guest defaults
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_guestandis_valloc) before the reset.Suggested-by Chang S. Bae <[email protected]>Signed-off-by Chao Gao <[email protected]>--- arch/x86/kernel/fpu/core.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)arch/x86/kernel/fpu/core.c +9
-6@@ -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
arch/x86/kernel/fpu/core.c void fpstate_init_user(struct fpstate *fpstate) · line 552
Quoted 10 lines
arch/x86/kernel/fpu/core.c +10
-0+* 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; + }
Nice! I like this idea.
arch/x86/kernel/fpu/core.c void fpstate_init_user(struct fpstate *fpstate) · line 556
Quoted 5 lines
arch/x86/kernel/fpu/core.c +1
-1+ 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;
And then a follow-up patch (or same patch?) to do this?
if (fpstate->is_guest) {
fpstate->size = guest_default_cfg.size;
fpstate->xfeatures = guest_default_cfg.features;
fpstate->xfd = 0;
} else {
fpstate->size = fpu_kernel_cfg.default_size;
fpstate->xfeatures = fpu_kernel_cfg.default_features;
fpstate->xfd = init_fpstate.xfd;
}Quoted 3 lines
arch/x86/kernel/fpu/core.c +0
-0}Signature · 1 line
2.47.1
Re: [PATCH v6 5/7] x86/fpu: Initialize guest fpstate and FPU pseudo container from guest defaults
Quoted 11 lines · Sean Christopherson
And then a follow-up patch (or same patch?) to do this?
if (fpstate->is_guest) { fpstate->size = guest_default_cfg.size; fpstate->xfeatures = guest_default_cfg.features; fpstate->xfd = 0; } else { fpstate->size = fpu_kernel_cfg.default_size; fpstate->xfeatures = fpu_kernel_cfg.default_features; fpstate->xfd = init_fpstate.xfd; }
That looks nice because it clearly highlights how guests and bare-metal fpstates are different. I like it too.
PATCH 5/8 x86/fpu: Initialize guest fpstate and FPU pseudo container from guest defaults
In reply to Chao Gaofpu_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.
---
v6a: 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)
Note: this quick revision is just intended to ensure that the feedback has been properly addressed.
arch/x86/kernel/fpu/core.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)
arch/x86/kernel/fpu/core.c +20 -7
@@ -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; }
Signature · 1 line
2.47.1
PATCH 6/8 x86/fpu: Remove xfd argument from __fpstate_reset()
In reply to Chao GaoThe 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
---
v6a: new.
Note: this quick revision is just intended to ensure that the feedback has been properly addressed.
arch/x86/kernel/fpu/core.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
arch/x86/kernel/fpu/core.c +7 -6
@@ -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;
Signature · 1 line
2.47.1
Sean Christopherson
reviewerRe: [PATCH v6a 6/8] x86/fpu: Remove xfd argument from __fpstate_reset()
Quoted 7 lines · Chao Gao
The initial values for
fpstate::xfddiffer 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, handlefpstate::xfdin a similar way to highlight the differences in the initial xfd value between guest and host fpstates
Quoted 8 lines
Suggested-by Sean Christopherson <[email protected]>Signed-off-by Chao Gao <[email protected]>---
v6a: new.Note: this quick revision is just intended to ensure that the feedback has been properly addressed.
Both patches LGTM.
PATCH v6 6/7 x86/fpu/xstate: Introduce "guest-only" supervisor xfeature set
In reply to Chao GaoFrom: Yang Weijiang <[email protected]>
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.
---
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/[email protected]/#t
[2]: https://lore.kernel.org/kvm/[email protected]/
[3]: https://lore.kernel.org/kvm/[email protected]/
[4]: https://lore.kernel.org/all/[email protected]/
--- 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(-)
arch/x86/include/asm/fpu/types.h +5 -4
@@ -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;/*
arch/x86/include/asm/fpu/xstate.h +5 -1
@@ -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,
arch/x86/kernel/fpu/xstate.c +11 -3
@@ -781,14 +781,22 @@ static void __init init_default_features(u64 kernel_max_features, u64 user_max_fu64 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 (); }/*
arch/x86/kernel/fpu/xstate.h +5 -0
@@ -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 ))
Signature · 1 line
2.47.1
PATCH v6 7/7 x86/fpu/xstate: Add CET supervisor xfeature support as a guest-only feature
In reply to Chao GaoFrom: Yang Weijiang <[email protected]>
== 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.
---
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(-)
arch/x86/include/asm/fpu/types.h +12 -2
@@ -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).
arch/x86/include/asm/fpu/xstate.h +2 -3
@@ -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 | \
arch/x86/kernel/fpu/xstate.c +4 -1
@@ -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 :
Signature · 1 line
2.47.1