Re: [PATCH v3 09/10] x86/fpu/xstate: Introduce XFEATURE_MASK_KERNEL_DYNAMIC xfeature set

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

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