linux-kernel - Re: [PATCH v3 09/10] x86/fpu/xstate: Introduce XFEATURE_MASK_KERNEL

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: <Z85hPxSAYAAmv16p@intel.com>
Date: Mon, 10 Mar 2025 11:49:19 +0800
From: Chao Gao <chao.gao@...el.com>
To: "Chang S. Bae" <chang.seok.bae@...el.com>
CC: <tglx@...utronix.de>, <dave.hansen@...el.com>, <x86@...nel.org>,
	<seanjc@...gle.com>, <pbonzini@...hat.com>, <linux-kernel@...r.kernel.org>,
	<kvm@...r.kernel.org>, <peterz@...radead.org>, <rick.p.edgecombe@...el.com>,
	<weijiang.yang@...el.com>, <john.allen@....com>, <bp@...en8.de>
Subject: Re: [PATCH v3 09/10] x86/fpu/xstate: Introduce
 XFEATURE_MASK_KERNEL_DYNAMIC xfeature set

>When introducing user dynamic features, AMX required a large state, so buffer
>reallocation for expansion was deferred until it was actually used. This
>introduction was associated with introducing a permission mechanism, which
>was expected to be requested by userspace.
>
>For VCPU tasks, the userspace component (QEMU) requests permission [1], and
>buffer expansion then follows based on the exposed CPUID determination [2].
>
>Now, regarding the new kernel dynamic features, I’m unsure whether this
>changelog or anything else sufficiently describes its semantics distintively.
>It appears that both permission grant and buffer allocation for the kernel
>dynamic feature occur at VCPU allocation time. However, this model differs
>from the deferred buffer expansion model for user dynamic features.
>
>If the kernel dynamic feature model were to follow the same deferred
>reallocation approach as user dynamic features, buffer reallocation would be
>expected. In that case, I'd also question whether fpu_guest_cfg is truly
>necessary.
>
>VCPU allocation could still rely on fpu_kernel_cfg, and fpu->guest_perm could
>be extrapolated from fpu->perm or fpu_kernel_cfg. Then, reallocation could
>proceed as usual based on the permission, extending
>fpu_enable_guest_xfd_features(), possibly renaming it to
>fpu_enable_dynamic_features().
>
>That said, this is a relatively small state.

Yes, there's no need to make the guest FPU dynamically sized for the CET
supervisor state, as it is only 24 bytes.

XFEATURE_MASK_KERNEL_DYNAMIC is a misnomer. It is misleading readers into
thinking it involves permission requests and dynamic sizing, similar to
XFEATURE_MASK_USER_DYNAMIC

>Even if the intent was to
>introduce a new semantic model distinct from user dynamic features, it should
>be clearly documented to avoid confusion.

The goal isn't to add a new semantic model for dynamic features.

>
>On the other hand, if the goal is rather to establish a new approach for
>handling a previously nonexistent set of guest-exclusive features, then the

Yes. This is the goal of this patch.

>current approach remains somewhat convoluted without clear descriptions.
>Perhaps, I'm missing something.

Do you mean this patch is "somewhat convoluted"? or the whole series?

I am assuming you meant this series as this patch itself is quite small.

Here is how this series is organized:

Patches 1–4 : Cleanups and preparatory fixes.
Patches 5–7 : Introduce fpu_guest_cfg to formalize guest FPU configuration.
Patch 8 (Primary Goal): Add CET supervisor state support.
Patches 9–10 : make CET supserviosr state a guest-only feature to save XSAVE buffer
	       space for non-guest FPUs (placed at the end for easier review/drop).

I believe the "somewhat convoluted" impression comes from the introduction of
fpu_guest_cfg. But as I alluded to in patch 5's changelog, fpu_guest_cfg
actually simplifies the architecture rather than adding complexity, with
minimal overhead, i.e., a single global config. It was suggested by Sean [1].
In my view, it offers three benefits:

 - Readability: Removes ambiguity in fpu_alloc_guest_fpstate() by initializing
		the guest FPU with its own config.

 - Extensibility: Supports clean addition of guest-only features (e.g., CET
		  supervisor state) or potentially kernel-only features (e.g.,
		  PASID, which is not used by guest FPUs)

 - Robustness: Prevent issues like those addressed by patches 3/4.


It is possible to make some features guest-only without fpu_guest_cfg, but
doing so would make fpu_alloc_guest_fpstate() a bit difficult to understand.
See [2].

[1]: https://lore.kernel.org/kvm/ZTf5wPKXuHBQk0AN@google.com/
[2]: https://lore.kernel.org/kvm/20230914063325.85503-8-weijiang.yang@intel.com/

>
>Thanks,
>Chang
>
>[1] https://github.com/qemu/qemu/blob/master/target/i386/kvm/kvm.c#L6395
>[2] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kvm/cpuid.c#n195

Thanks for these references.