Re: [PATCH v3 03/10] x86/fpu/xstate: Correct xfeatures cache in guest pseudo fpu container

[Chao Gao]

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.