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.1On 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/
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
* 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
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.
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.
© 2016 - 2025 Red Hat, Inc.