| Message ID | 20250307164123.1613414-4-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce CET supervisor state support | expand |
On 3/7/25 08:41, Chao Gao wrote: > The xfeatures field in struct fpu_guest is designed to track the enabled > xfeatures for guest FPUs. However, during allocation in > fpu_alloc_guest_fpstate(), gfpu->xfeatures is initialized to > fpu_user_cfg.default_features, while the corresponding > fpstate->xfeatures is set to fpu_kernel_cfg.default_features > > Correct the mismatch to avoid confusion. > > Note this mismatch does not cause any functional issues. The > gfpu->xfeatures is checked in fpu_enable_guest_xfd_features() to > verify if XFD features are already enabled: > > xfeatures &= ~guest_fpu->xfeatures; > if (!xfeatures) > return 0; > > It gets updated in fpstate_realloc() after enabling some XFD features: > > guest_fpu->xfeatures |= xfeatures; > > So, backport is not needed. I don't have any great suggestions for improving this, but I just don't seem to find this changelog compelling. I can't put my finger on it, though. I think I'd find it more convincing if you argued what the *CORRECT* value is and why rather than just arguing for consistency with a random value. I also don't get the pivot over the XFD for explaining why it is harmless. XFD isn't even used in most cases, so I'd find a justification separate from XFD more compelling.
On Fri, Mar 07, 2025 at 09:48:25AM -0800, Dave Hansen wrote: >On 3/7/25 08:41, Chao Gao wrote: >> The xfeatures field in struct fpu_guest is designed to track the enabled >> xfeatures for guest FPUs. However, during allocation in >> fpu_alloc_guest_fpstate(), gfpu->xfeatures is initialized to >> fpu_user_cfg.default_features, while the corresponding >> fpstate->xfeatures is set to fpu_kernel_cfg.default_features >> >> Correct the mismatch to avoid confusion. >> >> Note this mismatch does not cause any functional issues. The >> gfpu->xfeatures is checked in fpu_enable_guest_xfd_features() to >> verify if XFD features are already enabled: >> >> xfeatures &= ~guest_fpu->xfeatures; >> if (!xfeatures) >> return 0; >> >> It gets updated in fpstate_realloc() after enabling some XFD features: >> >> guest_fpu->xfeatures |= xfeatures; >> >> So, backport is not needed. > >I don't have any great suggestions for improving this, but I just don't >seem to find this changelog compelling. I can't put my finger on it, though. > >I think I'd find it more convincing if you argued what the *CORRECT* >value is and why rather than just arguing for consistency with a random >value. I also don't get the pivot over the XFD for explaining why it is fpstate->xfeatures isn't a random value. It is the RFBM, right? see os_xsave(). The xfeatures in the guest FPU pesudo container (gfpu->xfeatures) is to track enabled xfeatures of the guest FPU. I think "enabled" refers to RFBM because only enabled features need save/restore. so gfpu->xfeatures should be consistent with fpstate->xfeatures. They become misaligned during allocation. Specifically, gfpu->xfeatures does not track any supervisor features. Excluding all _supervisor_ features is harmless, as the value is solely used to check if XFD features, which are all _user_ features, are already enabled in fpu_enable_guest_xfd_features(). It just causes confusion. >harmless. XFD isn't even used in most cases, so I'd find a justification >separate from XFD more compelling. > To me, there is a discrepancy between the field's name and the value it holds. We have two options to fix it: 1. rename @xfeatures in struct fpu_guest to @user_xfeatures and update the comment above to state the field only tracks enabled _user_ features. 2. ensure @xfeatures in struct fpu_guest matches fpstate->xfeatures this patch implements the option #2.
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c index dc169f3d336d..6166a928d3f5 100644 --- a/arch/x86/kernel/fpu/core.c +++ b/arch/x86/kernel/fpu/core.c @@ -230,7 +230,7 @@ bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu) fpstate->is_guest = true; gfpu->fpstate = fpstate; - gfpu->xfeatures = fpu_user_cfg.default_features; + gfpu->xfeatures = fpu_kernel_cfg.default_features; /* * KVM sets the FP+SSE bits in the XSAVE header when copying FPU state
The xfeatures field in struct fpu_guest is designed to track the enabled xfeatures for guest FPUs. However, during allocation in fpu_alloc_guest_fpstate(), gfpu->xfeatures is initialized to fpu_user_cfg.default_features, while the corresponding fpstate->xfeatures is set to fpu_kernel_cfg.default_features Correct the mismatch to avoid confusion. Note this mismatch does not cause any functional issues. The gfpu->xfeatures is checked in fpu_enable_guest_xfd_features() to verify if XFD features are already enabled: xfeatures &= ~guest_fpu->xfeatures; if (!xfeatures) return 0; It gets updated in fpstate_realloc() after enabling some XFD features: guest_fpu->xfeatures |= xfeatures; So, backport is not needed. Signed-off-by: Chao Gao <chao.gao@intel.com> --- arch/x86/kernel/fpu/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)