>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@xxxxxxxxxx/ [2]: https://lore.kernel.org/kvm/20230914063325.85503-8-weijiang.yang@xxxxxxxxx/ > >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.
