RE: [PATCH] x86/fpu: Remove dynamic features from xcomp_bv for init_fpstate

[Yao, Yuan]

>-----Original Message-----
>From: Bae, Chang Seok <chang.seok.bae@xxxxxxxxx>
>Sent: Friday, October 14, 2022 11:54
>To: Hansen, Dave <dave.hansen@xxxxxxxxx>; Yao, Yuan <yuan.yao@xxxxxxxxx>; Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>;
>linux-kernel@xxxxxxxxxxxxxxx
>Cc: x86@xxxxxxxxxx; Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>Subject: Re: [PATCH] x86/fpu: Remove dynamic features from xcomp_bv for init_fpstate
>
>On 10/13/2022 10:44 AM, Dave Hansen wrote:
>>
>> A better comment for that would be:
>>
>> 	 * Some user_xfeatures may not be present in the fpstate.
>> 	 * Remove those from 'mask' to zero those features in the
>> 	 * user buffer instead of retrieving them from fpstate.
>
>Yes, indeed!
>
>Also this xstate copy routine looks to need some updates.
>
>If an xfeature is present in fpstate, and in init state, the value is
>retrieved from init_fpstate via copy_feature(). But, it has no space for
>dynamic states. Also, for extended states, the init state is known to be
>zero.
>
>Then, perhaps, init_fpstate is better not to be accessed in the
>for_each_extended_xfeature loop; instead of using copy_feature(), the
>feature can be zeroed like this:
>
>diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>index 13b83b11b3d8..0fdfd03938b6 100644
>--- a/arch/x86/kernel/fpu/xstate.c
>+++ b/arch/x86/kernel/fpu/xstate.c
>@@ -1150,11 +1150,11 @@ void __copy_xstate_to_uabi_buf(struct membuf to,
>struct fpstate *fpstate,
>                          */
>                         pkru.pkru = pkru_val;
>                         membuf_write(&to, &pkru, sizeof(pkru));
>-               } else {
>-                       copy_feature(header.xfeatures & BIT_ULL(i), &to,
>-                                    __raw_xsave_addr(xsave, i),
>-                                    __raw_xsave_addr(xinit, i),
>+               } else if (header.xfeatures & BIT_ULL(i)) {
>+                       membuf_write(&to, __raw_xsave_addr(xsave, i),
>                                      xstate_sizes[i]);
>+               } else {
>+                       membuf_zero(&to, xstate_sizes[i]);

Ah, I didn’t aware your reply before sent mine :- ) 

Does init_fpstate saves the "init" state for all non-dynamic components in its buffer ? if no than 
this change is better, else read from init_fpstate for allocated buffer and only zero the not exist buffer is preferred to me.
The head.xfeatures is 0 before the guest fpu is used to xsaves at least once.

>                 }
>
>Thanks,
>Chang