Re: [PATCH v3 04/10] x86/fpu/xstate: Correct guest fpstate size calculation

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

On Mon, Mar 10, 2025 at 10:33:20AM -0700, Chang S. Bae wrote:
>On 3/10/2025 12:06 AM, Chao Gao wrote:
>> 
>> Should patch 2 be posted separately?
>
>gfpu->perm has been somewhat overlooked, as __xstate_request_perm() does not
>update this field. However, I see that as a separate issue. The options are
>either to fix it so that it remains in sync with fpu->guest_perm consistently
>or to remove it entirely, as you proposed, if it has no actual use.
>
>There hasn’t been any relevant change that would justify a quick follow-up
>like the other case. So, I'd assume it as part of this series.
>
>But yes, I think gfpu->perm is also going to be
>fpu_kernel_cfg.default_features at the moment.
>
>> Regarding the changelog, I am uncertain what's quite different in the context.
>> It seems both you and I are talking about the inconsistency between
>> gfpu->xfeatures and fpstate->xfeatures. Did I miss something obvious?
>
>I saw a distinction between inconsistencies within a function and
>inconsistencies across functions.
>
>Stepping back a bit, the approach for defining the VCPU xfeature set was
>originally intended to include only user features, but it now appears
>somewhat inconsistent:
>
>(a) In fpu_alloc_guest_fpstate(), fpu_user_cfg is used.
>(b) However, __fpstate_reset() references fpu_kernel_cfg to set storage
>    attributes.
>(c) Additionally, fpu->guest_perm takes fpu_kernel_cfg, which affects
>    fpstate_realloc().
>
>To maintain a consistent VCPU xfeature set, (b) and (c) should be corrected.
>
>Alternatively, the VCPU xfeature set could be reconsidered to align with how
>other tasks handle it. This might offer better maintainability across
>functions. In that case, another option would be simply updating
>fpu_alloc_guest_fpstate().
>
>The recent tip-tree change seems somewhat incomplete — perhaps in hindsight.
>If following up on this, the changelog should specifically address
>inconsistencies within a function. I saw this as a way to solidify an
>upcoming change, where addressing it sooner rather than later would be
>beneficial.
>
>In patch 3, you've pointed out the inconsistency between (a) and (b), which
>is a valid point. However, the fix is only partial and does not fully address
>the issue either. Moreover, the patch does not reference the recent tip-tree
>change as it didn't have any context at that time.

Hi Chang,

All of the above makes sense to me. Thank you for your review and suggestions.

I will update the changelog to reference the recent change in tip-tree and post
it separately.

One thing I'm not entirely clear on is "the fix is only partial". I assume I
need to update gfpu->perm to reference fpu_kernel_cfg to complement the fix.
Is that correct?