Re: [PATCH v7 04/26] x86/fpu/xstate: Introduce XFEATURE_MASK_KERNEL_DYNAMIC xfeature set

[Yang, Weijiang]

On 12/1/2023 1:33 AM, Maxim Levitsky wrote:
> On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote:
> > Define new XFEATURE_MASK_KERNEL_DYNAMIC set including the features can be
> I am not sure though that this name is correct, but I don't know if I can
> suggest a better name.

It's a symmetry of XFEATURE_MASK_USER_DYNAMIC ;-)
> > optionally enabled by kernel components, i.e., the features are required by
> > specific kernel components. 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 xsaves/xrstors operating xfeature set
> > (XCR0 | XSS), but the relevant CPU feature, i.e., supervisor shadow stack, is
> > not enabled in host kernel so 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@xxxxxxxxx>
> > Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> > ---
> >   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 */
> I suggest to explain this a bit better. How about something like that:
>
> "Kernel features that are not enabled by default for all processes, but can
> be still used by some processes, for example to support guest virtualization"

It looks good to me, will apply it in next version, thanks!

> But feel free to keep it as is or propose something else. IMHO this will
> be confusing this way or another.
>
>
> Another question: kernel already has a notion of 'independent features'
> which are currently kernel features that are enabled in IA32_XSS but not present in 'fpu_kernel_cfg.max_features'
>
> Currently only 'XFEATURE_LBR' is in this set. These features are saved/restored manually
> from independent buffer (in case of LBRs, perf code cares for this).
>
> Does it make sense to add CET_S to there as well instead of having XFEATURE_MASK_KERNEL_DYNAMIC,

CET_S here refers to PL{0,1,2}_SSP, right?

IMHO, perf relies on dedicated code to switch LBR MSRs for various reason, e.g., overhead, the feature
owns dozens of MSRs, remove xfeature bit will offload the burden of common FPU/xsave framework.

But CET only has 3 supervisor MSRs and they need to be managed together with user mode MSRs.
Enabling it in common FPU framework would make the switch/swap much easier without additional
support code.

>   and maybe rename the
> 'XFEATURE_MASK_INDEPENDENT' to something like 'XFEATURES_THE_KERNEL_DOESNT_CARE_ABOUT'
> (terrible name, but you might think of a better name)
>
>
> > +#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 b57d909facca..ba4172172afd 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;
>
>
> Best regards,
> 	Maxim Levitsky