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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z+ygwMmxRJQIRGoy@intel.com>
Date: Wed, 2 Apr 2025 10:28:16 +0800
From: Chao Gao <chao.gao@...el.com>
To: "Chang S. Bae" <chang.seok.bae@...el.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
	<tglx@...utronix.de>, <dave.hansen@...el.com>, <seanjc@...gle.com>,
	<pbonzini@...hat.com>, <peterz@...radead.org>, <rick.p.edgecombe@...el.com>,
	<weijiang.yang@...el.com>, <john.allen@....com>, <bp@...en8.de>,
	<xin3.li@...el.com>, Maxim Levitsky <mlevitsk@...hat.com>, Ingo Molnar
	<mingo@...hat.com>, Dave Hansen <dave.hansen@...ux.intel.com>, "H. Peter
 Anvin" <hpa@...or.com>, Mitchell Levy <levymitchell0@...il.com>, "Samuel
 Holland" <samuel.holland@...ive.com>, Aruna Ramakrishna
	<aruna.ramakrishna@...cle.com>, Vignesh Balasubramanian <vigbalas@....com>
Subject: Re: [PATCH v4 3/8] x86/fpu/xstate: Add CET supervisor xfeature
 support

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@...el.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@...el.com>
>> Signed-off-by: Chao Gao <chao.gao@...el.com>
>> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@...el.com>
>> Reviewed-by: Maxim Levitsky <mlevitsk@...hat.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.