linux-kernel - Re: [PATCH v3 07/10] x86/fpu/xstate: Initialize guest fpstate with fpu_guest

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z8u7uKlq4jOtwYuR@intel.com>
Date: Sat, 8 Mar 2025 11:38:32 +0800
From: Chao Gao <chao.gao@...el.com>
To: Dave Hansen <dave.hansen@...el.com>
CC: <tglx@...utronix.de>, <x86@...nel.org>, <seanjc@...gle.com>,
	<pbonzini@...hat.com>, <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
	<peterz@...radead.org>, <rick.p.edgecombe@...el.com>,
	<weijiang.yang@...el.com>, <john.allen@....com>, <bp@...en8.de>, "Maxim
 Levitsky" <mlevitsk@...hat.com>
Subject: Re: [PATCH v3 07/10] x86/fpu/xstate: Initialize guest fpstate with
 fpu_guest_config

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@...el.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.
>