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?
