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.
Thanks,
Chang
