Re: [PATCH v3 07/10] x86/fpu/xstate: Initialize guest fpstate with fpu_guest_config

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

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@intel.com>
>> 
>> 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.
>