Re: [PATCH v4 3/8] x86/fpu/xstate: Add CET supervisor xfeature support

[Chang S. Bae]

On 3/18/2025 8:31 AM, Chao Gao wrote:
> From: Yang Weijiang <weijiang.yang@xxxxxxxxx>
>
> 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, host FPUs do not utilize the CET supervisor xstate. Enabling
> this state for host FPUs would lead to a 24-byte waste in the XSAVE buffer
> on CET-capable parts.
>
> Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> Reviewed-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>

Placing this patch immediately after a few mainline fixes looks to 
suggest that supervisor CET state can be enabled as-is, implying that 
the follow-up patches are merely optional optimizations.

In V2, Dave provided feedback [1] when you placed this patch second out 
of six:

 > This series is starting to look backward to me.
 >
 > The normal way you do these things is that you introduce new
 > abstractions and refactor the code. Then you go adding features.
 >
 > For instance, this series should spend a few patches introducing
 > 'fpu_guest_cfg' and using it before ever introducing the concept of a
 > dynamic xfeature.

In V3, you moved this patch further back to position 8 out of 10. Now, 
in this version, you've placed it at position 3 out of 8.

This raises the question of whether you've fully internalized his advice.

If your intent is to save kernel memory, the xstate infrastructure 
should first be properly adjusted. Specifically:

  1. Initialize the VCPU’s default xfeature set and its XSAVE buffer
     size.

  2. Reference them in the two sites:

    (a) for fpu->guest_perm

    (b) at VCPU allocation time.

  3. Introduce a new feature set (you named "guest supervisor state") as
     a placeholder and integrate it into initialization, along with the
     XSAVE sanity check.

With these adjustments in place, you may consider enabling a new 
xfeature, defining it as a guest-supervisor state simply.

Thanks,
Chang

[1] 
https://lore.kernel.org/kvm/d6127d2e-ea95-4e52-b3db-b39203bad935@xxxxxxxxx/