diff mbox series

[v10,04/27] x86/fpu/xstate: Introduce XFEATURE_MASK_KERNEL_DYNAMIC xfeature set

Message ID 20240219074733.122080-5-weijiang.yang@intel.com (mailing list archive)
State New, archived
Headers show
Series Enable CET Virtualization | expand

Commit Message

Yang, Weijiang Feb. 19, 2024, 7:47 a.m. UTC 
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>
---
 arch/x86/include/asm/fpu/xstate.h | 5 ++++-
 arch/x86/kernel/fpu/xstate.c      | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Sean Christopherson May 1, 2024, 6:45 p.m. UTC | #1 
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.
Dave Hansen May 2, 2024, 5:46 p.m. UTC | #2 
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.
Sean Christopherson May 7, 2024, 10:57 p.m. UTC | #3 
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);
Dave Hansen May 7, 2024, 11:17 p.m. UTC | #4 
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.
Yang, Weijiang May 8, 2024, 1:19 a.m. UTC | #5 
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 mbox series

Patch

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;