diff mbox series

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

Message ID 20250307164123.1613414-8-chao.gao@intel.com (mailing list archive)
State New
Headers show
Series Introduce CET supervisor state support | expand

Commit Message

Chao Gao March 7, 2025, 4:41 p.m. UTC 
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.

Note fpu_guest_cfg and fpu_kernel_cfg remain the same for now. So there
is no functional change.

Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/x86/kernel/fpu/core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Dave Hansen March 7, 2025, 6:41 p.m. UTC | #1 
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.

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.
Chao Gao March 8, 2025, 3:38 a.m. UTC | #2 
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.
>
diff mbox series

Patch

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index d7ae684adbad..9cb800918b6d 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -194,8 +194,6 @@  void fpu_reset_from_exception_fixup(void)
 }
 
 #if IS_ENABLED(CONFIG_KVM)
-static void __fpstate_reset(struct fpstate *fpstate, u64 xfd);
-
 static void fpu_lock_guest_permissions(struct fpu_guest *gfpu)
 {
 	struct fpu_state_perm *fpuperm;
@@ -219,19 +217,22 @@  bool fpu_alloc_guest_fpstate(struct fpu_guest *gfpu)
 	struct fpstate *fpstate;
 	unsigned int size;
 
-	size = fpu_kernel_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64);
+	size = fpu_guest_cfg.default_size + ALIGN(offsetof(struct fpstate, regs), 64);
+
 	fpstate = vzalloc(size);
 	if (!fpstate)
 		return false;
 
-	/* Leave xfd to 0 (the reset value defined by spec) */
-	__fpstate_reset(fpstate, 0);
+	fpstate->size		= fpu_guest_cfg.default_size;
+	fpstate->xfeatures	= fpu_guest_cfg.default_features;
+	fpstate->user_size	= fpu_user_cfg.default_size;
+	fpstate->user_xfeatures	= fpu_user_cfg.default_features;
 	fpstate_init_user(fpstate);
 	fpstate->is_valloc	= true;
 	fpstate->is_guest	= true;
 
 	gfpu->fpstate		= fpstate;
-	gfpu->xfeatures		= fpu_kernel_cfg.default_features;
+	gfpu->xfeatures		= fpu_guest_cfg.default_features;
 
 	/*
 	 * KVM sets the FP+SSE bits in the XSAVE header when copying FPU state