Re: [PATCH v3 08/10] x86/fpu/xstate: Add CET supervisor xfeature support

[Chao Gao <chao.gao@intel.com>]

On Fri, Mar 07, 2025 at 10:39:47AM -0800, Dave Hansen wrote:
>On 3/7/25 08:41, Chao Gao wrote:
>> From: Yang Weijiang <weijiang.yang@intel.com>
>> 
>> To support CET virtualization, KVM needs the kernel to save/restore CET
>> supervisor xstate in guest FPUs when switching between guest and host
>> FPUs.
>> 
>> Add CET supervisor xstate (i.e., XFEATURE_CET_KERNEL) support. Both the
>> guest FPU and the kernel FPU will allocate memory for the new xstate.
>> For the guest FPU, the xstate remains unused until the upcoming CET
>> virtualization is added to KVM. For the kernel FPU, the xstate is unused
>> until CET_S is enabled within the kernel.
>> 
>> Note CET_S may or may not be enabled within the kernel, so always
>> allocating memory for XFEATURE_CET_KERNEL could potentially waste some
>> XSAVE buffer space. If necessary, this issue can be addressed by making
>> XFEATURE_CET_KERNEL a guest-only feature.
>
>I feel like these changelogs are long but say very little.
>
>This patch *WASTES* resources. Granted, it's only for a single patch,
>but it's totally not obvious.
>
>Could you work on tightening down the changelog, please?

ok. will update the changelog to:

To support CET virtualization, KVM needs the kernel to save and restore the CET
supervisor xstate in guest FPUs when switching between guest and host FPUs.

Add CET supervisor xstate support in preparation for the upcoming CET
virtualization in KVM.

Currently, kernel FPUs will not utilize the CET supervisor xstate, resulting in
some wasted XSAVE buffer space (24 Bytes) for all kernel FPUs.

>
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -55,7 +55,7 @@ static const char *xfeature_names[] =
>>  	"Protection Keys User registers",
>>  	"PASID state",
>>  	"Control-flow User registers",
>> -	"Control-flow Kernel registers (unused)",
>> +	"Control-flow Kernel registers",
>
>This should probably be:
>
>> +	"Control-flow Kernel registers (KVM only)",
>
>or something similar for now. XFEATURE_CET_KERNEL is *VERY* different
>from all of the other features and it's silly to pretend that it's the same.

Agreed. Should "KVM only" tag be added in the next patch, where CET supervisor
xstate becomes a guest-only feature?