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

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

On Tue, Apr 01, 2025 at 10:15:50AM -0700, Chang S. Bae wrote:
>On 3/18/2025 8:31 AM, Chao Gao wrote:
>> From: Yang Weijiang <weijiang.yang@intel.com>
>> 
>> 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@intel.com>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
>
>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.

Yes, this is intentional. I mentioned it in the cover letter:

"""
Reorder the patches to put the CET supervisor state patch before the
"guest-only" optimization, allowing maintainers to easily adopt or omit the
optimization.
"""

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

In my opinion, he wasn't referring to the patch introducing the CET supervisor
xstate (i.e., this patch). Rather, he requested that the patch making the CET
supervisor xstate a guest-only feature should follow the introduction of
fpu_guest_cfg and the relevant cleanups.

>
> > 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.

I believe you are suggesting that the CET supervisor xstate should be
introduced directly as a guest-only feature, rather than first introducing it
in one patch and then converting it to guest-only in a subsequent patch.

This is a valid point, and I have considered it. However, I chose to split them
into two patches because the guest-only aspect is merely an optimization, and
the decision on whether to accept it is still pending. This order and split-up
make it easier for maintainers to make a decision.