| Message ID | 20231221140239.4349-5-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable CET Virtualization | expand |
On Thu, 2023-12-21 at 09:02 -0500, Yang Weijiang wrote: > Define a new XFEATURE_MASK_KERNEL_DYNAMIC mask to specify the features > that can be optionally enabled by kernel components. This is similar to > XFEATURE_MASK_USER_DYNAMIC in that it contains optional xfeatures that > can allows the FPU buffer to be dynamically sized. The difference is that > the KERNEL variant contains supervisor features and will be enabled by > kernel components that need them, and not directly by the user. Currently > it's used by KVM to configure guest dedicated fpstate for calculating > the xfeature and fpstate storage size etc. > > The kernel dynamic xfeatures now only contain XFEATURE_CET_KERNEL, which > is supported by host as they're enabled in kernel XSS MSR setting but > relevant CPU feature, i.e., supervisor shadow stack, is not enabled in > host kernel therefore it can be omitted for normal fpstate by default. > > Remove the kernel dynamic feature from fpu_kernel_cfg.default_features > so that the bits in xstate_bv and xcomp_bv are cleared and xsaves/xrstors > can be optimized by HW for normal fpstate. > > Suggested-by: Dave Hansen <dave.hansen@intel.com> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > --- > arch/x86/include/asm/fpu/xstate.h | 5 ++++- > arch/x86/kernel/fpu/xstate.c | 1 + > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h > index 3b4a038d3c57..a212d3851429 100644 > --- a/arch/x86/include/asm/fpu/xstate.h > +++ b/arch/x86/include/asm/fpu/xstate.h > @@ -46,9 +46,12 @@ > #define XFEATURE_MASK_USER_RESTORE \ > (XFEATURE_MASK_USER_SUPPORTED & ~XFEATURE_MASK_PKRU) > > -/* Features which are dynamically enabled for a process on request */ > +/* Features which are dynamically enabled per userspace request */ > #define XFEATURE_MASK_USER_DYNAMIC XFEATURE_MASK_XTILE_DATA > > +/* Features which are dynamically enabled per kernel side request */ > +#define XFEATURE_MASK_KERNEL_DYNAMIC 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 03e166a87d61..ca4b83c142eb 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -824,6 +824,7 @@ 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.default_features &= ~XFEATURE_MASK_KERNEL_DYNAMIC; > > fpu_user_cfg.default_features = fpu_user_cfg.max_features; > fpu_user_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC; I still think that we should consider adding XFEATURE_MASK_CET_KERNEL to XFEATURE_MASK_INDEPENDENT or at least have a good conversation on why this doesn't make sense, but I also don't intend to fight over this, as long as the code works. Best regards, Maxim Levitsky
On 1/3/2024 6:25 AM, Maxim Levitsky wrote: > On Thu, 2023-12-21 at 09:02 -0500, Yang Weijiang wrote: >> Define a new XFEATURE_MASK_KERNEL_DYNAMIC mask to specify the features >> that can be optionally enabled by kernel components. This is similar to >> XFEATURE_MASK_USER_DYNAMIC in that it contains optional xfeatures that >> can allows the FPU buffer to be dynamically sized. The difference is that >> the KERNEL variant contains supervisor features and will be enabled by >> kernel components that need them, and not directly by the user. Currently >> it's used by KVM to configure guest dedicated fpstate for calculating >> the xfeature and fpstate storage size etc. >> >> The kernel dynamic xfeatures now only contain XFEATURE_CET_KERNEL, which >> is supported by host as they're enabled in kernel XSS MSR setting but >> relevant CPU feature, i.e., supervisor shadow stack, is not enabled in >> host kernel therefore it can be omitted for normal fpstate by default. >> >> Remove the kernel dynamic feature from fpu_kernel_cfg.default_features >> so that the bits in xstate_bv and xcomp_bv are cleared and xsaves/xrstors >> can be optimized by HW for normal fpstate. >> >> Suggested-by: Dave Hansen <dave.hansen@intel.com> >> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> >> --- >> arch/x86/include/asm/fpu/xstate.h | 5 ++++- >> arch/x86/kernel/fpu/xstate.c | 1 + >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h >> index 3b4a038d3c57..a212d3851429 100644 >> --- a/arch/x86/include/asm/fpu/xstate.h >> +++ b/arch/x86/include/asm/fpu/xstate.h >> @@ -46,9 +46,12 @@ >> #define XFEATURE_MASK_USER_RESTORE \ >> (XFEATURE_MASK_USER_SUPPORTED & ~XFEATURE_MASK_PKRU) >> >> -/* Features which are dynamically enabled for a process on request */ >> +/* Features which are dynamically enabled per userspace request */ >> #define XFEATURE_MASK_USER_DYNAMIC XFEATURE_MASK_XTILE_DATA >> >> +/* Features which are dynamically enabled per kernel side request */ >> +#define XFEATURE_MASK_KERNEL_DYNAMIC 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 03e166a87d61..ca4b83c142eb 100644 >> --- a/arch/x86/kernel/fpu/xstate.c >> +++ b/arch/x86/kernel/fpu/xstate.c >> @@ -824,6 +824,7 @@ 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.default_features &= ~XFEATURE_MASK_KERNEL_DYNAMIC; >> >> fpu_user_cfg.default_features = fpu_user_cfg.max_features; >> fpu_user_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC; > > I still think that we should consider adding XFEATURE_MASK_CET_KERNEL to > XFEATURE_MASK_INDEPENDENT or at least have a good conversation on why this doesn't make sense, Hi, Maxim, Thanks for continuously adding feedback on this series! Appreciated! I think the discussion is not closed at this point but need maintainers to indicate the preferred approach, so far I'm following previous alignment that reached in community discussion, but it's still open for discussion. IMHO, folding XFEATURE_MASK_CET_KERNEL into XFEATURE_MASK_INDEPENDENT isn't necessarily cheap, we may have to touch more code that works pretty fine these days. In terms of KVM part, currently after VM-exit, guest arch-lbr MSRs are not saved/restored unless vCPU thread is preempted and host kernel arch-lbr save/restore code will handle the MSRs. But for guest CET supervisor xstate, host kernel doesn't have similar mechanism to handle CET supervisor MSRs, so require relatively "eager" handling after VM-exit. If we mix two different flavors in XFEATURE_MASK_INDEPENDENT, it would make it harder to handle guest xstates. Note, arch-lbr support for guest hasn't been upstreamed yet, it's based on my previous upstream solution. Maybe I missed something but it looks like true for the two guest features. > but I also don't intend to fight over this, as long as the code works. > > Best regards, > Maxim Levitsky >
On Wed, 2024-01-03 at 00:25 +0200, Maxim Levitsky wrote: > I still think that we should consider adding XFEATURE_MASK_CET_KERNEL > to > XFEATURE_MASK_INDEPENDENT or at least have a good conversation on why > this doesn't make sense, > but I also don't intend to fight over this, as long as the code > works. Hi, Using XFEATURE_MASK_INDEPENDENT would be pretty close to what we initially discussed when this series resumed: https://lore.kernel.org/lkml/20230803042732.88515-10-weijiang.yang@intel.com/ Except that it used manual MSR operations instead of xsaves. But the gist is the same I think - the state is managed manually by KVM. A XFEATURE_MASK_INDEPENDENT solution seems reasonable to me. I kind of liked the that the MSR version didn't complicate the overly complex FPU code. But there was an idea to give XFEATURE_MASK_KERNEL_DYNAMIC a try, to see if it turned out easy. I think it turned out "ok" complexity wise. So it doesn't make it clear win one way or the other for me. I guess it might be slightly more efficient as in this patch because it gets to use the lazy FPU stuff. It won't need to save/restore if the exit is handled within KVM, or the kernel switches to a kernel thread and back. I think that tilts it in favor of XFEATURE_MASK_KERNEL_DYNAMIC. Rick
On Thu, 2023-12-21 at 09:02 -0500, Yang Weijiang wrote: > Define a new XFEATURE_MASK_KERNEL_DYNAMIC mask to specify the > features > that can be optionally enabled by kernel components. This is similar > to > XFEATURE_MASK_USER_DYNAMIC in that it contains optional xfeatures > that > can allows the FPU buffer to be dynamically sized. The difference is > that > the KERNEL variant contains supervisor features and will be enabled > by > kernel components that need them, and not directly by the user. > Currently > it's used by KVM to configure guest dedicated fpstate for calculating > the xfeature and fpstate storage size etc. > > The kernel dynamic xfeatures now only contain XFEATURE_CET_KERNEL, > which > is supported by host as they're enabled in kernel XSS MSR setting but > relevant CPU feature, i.e., supervisor shadow stack, is not enabled > in > host kernel therefore it can be omitted for normal fpstate by > default. > > Remove the kernel dynamic feature from > fpu_kernel_cfg.default_features > so that the bits in xstate_bv and xcomp_bv are cleared and > xsaves/xrstors > can be optimized by HW for normal fpstate. > > Suggested-by: Dave Hansen <dave.hansen@intel.com> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h index 3b4a038d3c57..a212d3851429 100644 --- a/arch/x86/include/asm/fpu/xstate.h +++ b/arch/x86/include/asm/fpu/xstate.h @@ -46,9 +46,12 @@ #define XFEATURE_MASK_USER_RESTORE \ (XFEATURE_MASK_USER_SUPPORTED & ~XFEATURE_MASK_PKRU) -/* Features which are dynamically enabled for a process on request */ +/* Features which are dynamically enabled per userspace request */ #define XFEATURE_MASK_USER_DYNAMIC XFEATURE_MASK_XTILE_DATA +/* Features which are dynamically enabled per kernel side request */ +#define XFEATURE_MASK_KERNEL_DYNAMIC 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 03e166a87d61..ca4b83c142eb 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -824,6 +824,7 @@ 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.default_features &= ~XFEATURE_MASK_KERNEL_DYNAMIC; fpu_user_cfg.default_features = fpu_user_cfg.max_features; fpu_user_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC;
Define a new XFEATURE_MASK_KERNEL_DYNAMIC mask to specify the features that can be optionally enabled by kernel components. This is similar to XFEATURE_MASK_USER_DYNAMIC in that it contains optional xfeatures that can allows the FPU buffer to be dynamically sized. The difference is that the KERNEL variant contains supervisor features and will be enabled by kernel components that need them, and not directly by the user. Currently it's used by KVM to configure guest dedicated fpstate for calculating the xfeature and fpstate storage size etc. The kernel dynamic xfeatures now only contain XFEATURE_CET_KERNEL, which is supported by host as they're enabled in kernel XSS MSR setting but relevant CPU feature, i.e., supervisor shadow stack, is not enabled in host kernel therefore it can be omitted for normal fpstate by default. Remove the kernel dynamic feature from fpu_kernel_cfg.default_features so that the bits in xstate_bv and xcomp_bv are cleared and xsaves/xrstors can be optimized by HW for normal fpstate. Suggested-by: Dave Hansen <dave.hansen@intel.com> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> --- arch/x86/include/asm/fpu/xstate.h | 5 ++++- arch/x86/kernel/fpu/xstate.c | 1 + 2 files changed, 5 insertions(+), 1 deletion(-)