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

["Yang, Weijiang" <weijiang.yang@intel.com>]

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@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 */
> 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
>
>
>
>