| Message ID | 20250307164123.1613414-9-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce CET supervisor state support | expand |
On 3/7/25 08:41, Chao Gao wrote: > From: Yang Weijiang <weijiang.yang@intel.com> > > To support CET virtualization, KVM needs the kernel to save/restore CET > supervisor xstate in guest FPUs when switching between guest and host > FPUs. > > Add CET supervisor xstate (i.e., XFEATURE_CET_KERNEL) support. Both the > guest FPU and the kernel FPU will allocate memory for the new xstate. > For the guest FPU, the xstate remains unused until the upcoming CET > virtualization is added to KVM. For the kernel FPU, the xstate is unused > until CET_S is enabled within the kernel. > > Note CET_S may or may not be enabled within the kernel, so always > allocating memory for XFEATURE_CET_KERNEL could potentially waste some > XSAVE buffer space. If necessary, this issue can be addressed by making > XFEATURE_CET_KERNEL a guest-only feature. I feel like these changelogs are long but say very little. This patch *WASTES* resources. Granted, it's only for a single patch, but it's totally not obvious. Could you work on tightening down the changelog, please? > --- 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", This should probably be: > + "Control-flow Kernel registers (KVM only)", or something similar for now. XFEATURE_CET_KERNEL is *VERY* different from all of the other features and it's silly to pretend that it's the same.
On Fri, Mar 07, 2025 at 10:39:47AM -0800, Dave Hansen wrote: >On 3/7/25 08:41, Chao Gao wrote: >> From: Yang Weijiang <weijiang.yang@intel.com> >> >> To support CET virtualization, KVM needs the kernel to save/restore CET >> supervisor xstate in guest FPUs when switching between guest and host >> FPUs. >> >> Add CET supervisor xstate (i.e., XFEATURE_CET_KERNEL) support. Both the >> guest FPU and the kernel FPU will allocate memory for the new xstate. >> For the guest FPU, the xstate remains unused until the upcoming CET >> virtualization is added to KVM. For the kernel FPU, the xstate is unused >> until CET_S is enabled within the kernel. >> >> Note CET_S may or may not be enabled within the kernel, so always >> allocating memory for XFEATURE_CET_KERNEL could potentially waste some >> XSAVE buffer space. If necessary, this issue can be addressed by making >> XFEATURE_CET_KERNEL a guest-only feature. > >I feel like these changelogs are long but say very little. > >This patch *WASTES* resources. Granted, it's only for a single patch, >but it's totally not obvious. > >Could you work on tightening down the changelog, please? ok. will update the changelog to: 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, kernel FPUs will not utilize the CET supervisor xstate, resulting in some wasted XSAVE buffer space (24 Bytes) for all kernel FPUs. > >> --- 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", > >This should probably be: > >> + "Control-flow Kernel registers (KVM only)", > >or something similar for now. XFEATURE_CET_KERNEL is *VERY* different >from all of the other features and it's silly to pretend that it's the same. Agreed. Should "KVM only" tag be added in the next patch, where CET supervisor xstate becomes a guest-only feature?
diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h index d9515d7f65e4..eb034b7ab8c0 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 58325b3b8914..12613ebdbb5d 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, }; @@ -283,6 +284,7 @@ static void __init print_xstate_features(void) print_xstate_feature(XFEATURE_MASK_PKRU); print_xstate_feature(XFEATURE_MASK_PASID); print_xstate_feature(XFEATURE_MASK_CET_USER); + print_xstate_feature(XFEATURE_MASK_CET_KERNEL); print_xstate_feature(XFEATURE_MASK_XTILE_CFG); print_xstate_feature(XFEATURE_MASK_XTILE_DATA); } @@ -352,6 +354,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) /* @@ -552,6 +555,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);