On Fri, Mar 07, 2025 at 10:41:49AM -0800, Dave Hansen wrote:
>On 3/7/25 08:41, Chao Gao wrote:
>> From: Yang Weijiang <weijiang.yang@xxxxxxxxx>
>>
>> Use fpu_guest_cfg to initialize the guest fpstate and the guest FPU pseduo
>> container.
>>
>> The user_* fields remain unchanged for compatibility with KVM uAPIs.
>>
>> Inline the logic of __fpstate_reset() to directly utilize fpu_guest_cfg.
>
>Why? Seriously, why? Why would you just inline it? Could you please
>revisit the moment when you decided to do this? Please go back to that
>moment and try to unlearn whatever propensity you have for taking this path.
Thanks for this suggestion.
>
>There are two choices: make the existing function work for guests, or
>add a new guest-only reset function.
>
>Just an an example:
>
>static void __fpstate_reset(struct fpstate *fpstate,
> struct fpu_state_config *kernel_cfg,
> u64 xfd)
>{
> /* Initialize sizes and feature masks */
> fpstate->size = kernel_cfg->default_size;
> fpstate->xfeatures = kernel_cfg->default_features;
>
> /* Some comment about why user states don't vary */
> fpstate->user_size = fpu_user_cfg.default_size;
> fpstate->user_xfeatures = fpu_user_cfg.default_features;
>
> fpstate->xfd = xfd;
>}
>
>Then you have two call sites:
>
> __fpstate_reset(fpstate, &fpu_guest_cfg, 0);
>and
> __fpstate_reset(fpu->fpstate, &fpu_kernel_cfg,
> init_fpstate.xfd);
>
>What does this tell you?
>
>It clearly lays out that to reset an fpstate, you need a specific kernel
>config. That kernel config is (can be) different for guests.
>
>Refactoring the code as you go along is not optional. It's a requirement.
Got it. I was actually tempted to refactor __fpstate_reset() while preparing
the v3. I considered two options:
1. Move "fpstate->is_guest = true" before calling __fpstate_reset() and use it
within the function to select the right config.
2. Add a boolean parameter to __fpstate_reset() to indicate whether it is
operating on a guest fpstate.
I dislike both of them. So I gave up and left it as-is.
Your version looks good. I will incorporate it in the next version.
>
