Message ID | 20240219074733.122080-5-weijiang.yang@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Enable CET Virtualization | expand |
On Sun, Feb 18, 2024, Yang Weijiang wrote:
> Define a new XFEATURE_MASK_KERNEL_DYNAMIC mask to specify the features
I still don't understand why this is being called DYNAMIC. CET_SS isn't dynamic,
as KVM is _always_ allowed to save/restore CET_SS, i.e. whether or not KVM can
expose CET_SS to a guest is a static, boot-time decision. Whether or not a guest
XSS actually enables CET_SS is "dynamic", but that's true of literally every
xfeature in XCR0 and XSS.
XFEATURE_MASK_XTILE_DATA is labeled as dynamic because userspace has to explicitly
request that XTILE_DATA be enabled, and thus whether or not KVM is allowed to
expose XTILE_DATA to the guest is a dynamic, runtime decision.
So IMO, the umbrella macro should be XFEATURE_MASK_KERNEL_GUEST_ONLY.
On 5/1/24 11:45, Sean Christopherson wrote: > On Sun, Feb 18, 2024, Yang Weijiang wrote: >> Define a new XFEATURE_MASK_KERNEL_DYNAMIC mask to specify the features > I still don't understand why this is being called DYNAMIC. CET_SS isn't dynamic, > as KVM is _always_ allowed to save/restore CET_SS, i.e. whether or not KVM can > expose CET_SS to a guest is a static, boot-time decision. Whether or not a guest > XSS actually enables CET_SS is "dynamic", but that's true of literally every > xfeature in XCR0 and XSS. > > XFEATURE_MASK_XTILE_DATA is labeled as dynamic because userspace has to explicitly > request that XTILE_DATA be enabled, and thus whether or not KVM is allowed to > expose XTILE_DATA to the guest is a dynamic, runtime decision. > > So IMO, the umbrella macro should be XFEATURE_MASK_KERNEL_GUEST_ONLY. Here's how I got that naming. First, "static" features are always there. "Dynamic" features might or might not be there. I was also much more focused on what's in the XSAVE buffer than on the enabling itself, which are _slightly_ different. Then, it's a matter of whether the feature is user or supervisor. The kernel might need new state for multiple reasons. Think of LBR state as an example. The kernel might want LBR state around for perf _or_ so it can be exposed to a guest. I just didn't want to tie it to "GUEST" too much in case we have more of these things come along that get used for things unrelated to KVM. Obviously, at this point, we've only got one and KVM is the only user so the delta that I was worried about doesn't actually exist. So I still prefer calling it "KERNEL" over "GUEST". But I also don't feel strongly about it and I've said my peace. I won't NAK it one way or the other.
On Thu, May 02, 2024, Dave Hansen wrote: > On 5/1/24 11:45, Sean Christopherson wrote: > > On Sun, Feb 18, 2024, Yang Weijiang wrote: > >> Define a new XFEATURE_MASK_KERNEL_DYNAMIC mask to specify the features > > I still don't understand why this is being called DYNAMIC. CET_SS isn't dynamic, > > as KVM is _always_ allowed to save/restore CET_SS, i.e. whether or not KVM can > > expose CET_SS to a guest is a static, boot-time decision. Whether or not a guest > > XSS actually enables CET_SS is "dynamic", but that's true of literally every > > xfeature in XCR0 and XSS. > > > > XFEATURE_MASK_XTILE_DATA is labeled as dynamic because userspace has to explicitly > > request that XTILE_DATA be enabled, and thus whether or not KVM is allowed to > > expose XTILE_DATA to the guest is a dynamic, runtime decision. > > > > So IMO, the umbrella macro should be XFEATURE_MASK_KERNEL_GUEST_ONLY. > > Here's how I got that naming. First, "static" features are always > there. "Dynamic" features might or might not be there. I was also much > more focused on what's in the XSAVE buffer than on the enabling itself, > which are _slightly_ different. Ah, and CET_KERNEL will be '0' in XSTATE_BV for non-guest buffers, but '1' for guest buffers. > Then, it's a matter of whether the feature is user or supervisor. The > kernel might need new state for multiple reasons. Think of LBR state as > an example. The kernel might want LBR state around for perf _or_ so it > can be exposed to a guest. > > I just didn't want to tie it to "GUEST" too much in case we have more of > these things come along that get used for things unrelated to KVM. > Obviously, at this point, we've only got one and KVM is the only user so > the delta that I was worried about doesn't actually exist. > > So I still prefer calling it "KERNEL" over "GUEST". But I also don't > feel strongly about it and I've said my peace. I won't NAK it one way > or the other. I assume you mean "DYNAMIC" over "GUEST"? I'm ok with DYNAMIC, reflecting the impact on each buffer makes sense. My one request would be to change the WARN in os_xsave() to fire on CET_KERNEL, not KERNEL_DYNAMIC, because it's specifically CET_KERNEL that is guest-only. Future dynamic xfeatures could be guest-only, but they could also be dynamic for some completely different reason. That was my other hang-up with "DYNAMIC"; as-is, os_xsave() implies that it really truly is GUEST_ONLY. diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h index 83ebf1e1cbb4..2a1ff49ccfd5 100644 --- a/arch/x86/kernel/fpu/xstate.h +++ b/arch/x86/kernel/fpu/xstate.h @@ -185,8 +185,7 @@ static inline void os_xsave(struct fpstate *fpstate) WARN_ON_FPU(!alternatives_patched); xfd_validate_state(fpstate, mask, false); - WARN_ON_FPU(!fpstate->is_guest && - (mask & XFEATURE_MASK_KERNEL_DYNAMIC)); + WARN_ON_FPU(!fpstate->is_guest && (mask & XFEATURE_MASK_CET_KERNEL)); XSTATE_XSAVE(&fpstate->regs.xsave, lmask, hmask, err);
On 5/7/24 15:57, Sean Christopherson wrote: >> So I still prefer calling it "KERNEL" over "GUEST". But I also don't >> feel strongly about it and I've said my peace. I won't NAK it one way >> or the other. > I assume you mean "DYNAMIC" over "GUEST"? I'm ok with DYNAMIC, reflecting the > impact on each buffer makes sense. Yes. Silly thinko/typo on my part. > My one request would be to change the WARN in os_xsave() to fire on CET_KERNEL, > not KERNEL_DYNAMIC, because it's specifically CET_KERNEL that is guest-only. > Future dynamic xfeatures could be guest-only, but they could also be dynamic for > some completely different reason. That was my other hang-up with "DYNAMIC"; > as-is, os_xsave() implies that it really truly is GUEST_ONLY. > > diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h > index 83ebf1e1cbb4..2a1ff49ccfd5 100644 > --- a/arch/x86/kernel/fpu/xstate.h > +++ b/arch/x86/kernel/fpu/xstate.h > @@ -185,8 +185,7 @@ static inline void os_xsave(struct fpstate *fpstate) > WARN_ON_FPU(!alternatives_patched); > xfd_validate_state(fpstate, mask, false); > > - WARN_ON_FPU(!fpstate->is_guest && > - (mask & XFEATURE_MASK_KERNEL_DYNAMIC)); > + WARN_ON_FPU(!fpstate->is_guest && (mask & XFEATURE_MASK_CET_KERNEL)); > > XSTATE_XSAVE(&fpstate->regs.xsave, lmask, hmask, err); Yeah, that would make a lot of sense. We could add a more generic #define for it later if another feature gets added like this.
On 5/8/2024 7:17 AM, Dave Hansen wrote: > On 5/7/24 15:57, Sean Christopherson wrote: [...] >> My one request would be to change the WARN in os_xsave() to fire on CET_KERNEL, >> not KERNEL_DYNAMIC, because it's specifically CET_KERNEL that is guest-only. >> Future dynamic xfeatures could be guest-only, but they could also be dynamic for >> some completely different reason. That was my other hang-up with "DYNAMIC"; >> as-is, os_xsave() implies that it really truly is GUEST_ONLY. >> >> diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h >> index 83ebf1e1cbb4..2a1ff49ccfd5 100644 >> --- a/arch/x86/kernel/fpu/xstate.h >> +++ b/arch/x86/kernel/fpu/xstate.h >> @@ -185,8 +185,7 @@ static inline void os_xsave(struct fpstate *fpstate) >> WARN_ON_FPU(!alternatives_patched); >> xfd_validate_state(fpstate, mask, false); >> >> - WARN_ON_FPU(!fpstate->is_guest && >> - (mask & XFEATURE_MASK_KERNEL_DYNAMIC)); >> + WARN_ON_FPU(!fpstate->is_guest && (mask & XFEATURE_MASK_CET_KERNEL)); >> >> XSTATE_XSAVE(&fpstate->regs.xsave, lmask, hmask, err); > Yeah, that would make a lot of sense. We could add a more generic > #define for it later if another feature gets added like this. Thank you for getting alignment! I will change the code accordingly.
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;