diff mbox series

[v10,02/28] x86/fpu/xstate: Modify the initialization helper to handle both static and dynamic buffers

Message ID 20210825155413.19673-3-chang.seok.bae@intel.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Chang S. Bae Aug. 25, 2021, 3:53 p.m. UTC 
Have the function initializing the XSTATE buffer take a struct fpu *
pointer in preparation for dynamic state buffer support.

init_fpstate is a special case, which is indicated by a null pointer
parameter to fpstate_init().

Also, fpstate_init_xstate() now accepts the state component bitmap to
customize the compacted format.

No functional change.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kvm@vger.kernel.org
---
Changes from v5:
* Moved fpstate_init_xstate() back to the header (again).
* Massaged the changelog.

Changes from v4:
* Added a proper function description. (Borislav Petkov)
* Added the likely() statement as a null pointer is a special case.

Changes from v3:
* Updated the changelog. (Borislav Petkov)
* Updated the function comment to use kernel-doc style. (Borislav Petkov)

Changes from v2:
* Updated the changelog with task->fpu removed. (Borislav Petkov)
---
 arch/x86/include/asm/fpu/internal.h | 11 ++++++++++-
 arch/x86/kernel/fpu/core.c          | 28 +++++++++++++++++-----------
 arch/x86/kernel/fpu/init.c          |  2 +-
 arch/x86/kernel/fpu/xstate.c        |  3 +--
 arch/x86/kvm/x86.c                  |  2 +-
 5 files changed, 30 insertions(+), 16 deletions(-)

Comments

Thomas Gleixner Oct. 1, 2021, 12:45 p.m. UTC | #1 
Chang,

On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:
> Have the function initializing the XSTATE buffer take a struct fpu *
> pointer in preparation for dynamic state buffer support.
>
> init_fpstate is a special case, which is indicated by a null pointer
> parameter to fpstate_init().
>
> Also, fpstate_init_xstate() now accepts the state component bitmap to
> customize the compacted format.

That's not a changelog. Changelogs have to explain the WHY not the WHAT.

I can see the WHY when I look at the later changes, but that's not how
it works.

Also the subject of this patch is just wrong. It does not make the
functions handle dynamic buffers, it prepares them to add support for
that later.

> +static inline void fpstate_init_xstate(struct xregs_state *xsave, u64 mask)
> +{
> +	/*
> +	 * XRSTORS requires these bits set in xcomp_bv, or it will
> +	 * trigger #GP:
> +	 */
> +	xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | mask;
> +}

This wants to be a separate cleanup patch which replaces the open coded
variant here:

> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index fc1d529547e6..0fed7fbcf2e8 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -395,8 +395,7 @@ static void __init setup_init_fpu_buf(void)
>  	print_xstate_features();
>  
>  	if (boot_cpu_has(X86_FEATURE_XSAVES))
> -		init_fpstate.xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
> -						     xfeatures_mask_all;
> +		fpstate_init_xstate(&init_fpstate.xsave, xfeatures_mask_all);

Thanks,

        tglx
Chang S. Bae Oct. 3, 2021, 10:35 p.m. UTC | #2 
On Oct 1, 2021, at 05:45, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:
>> Have the function initializing the XSTATE buffer take a struct fpu *
>> pointer in preparation for dynamic state buffer support.
>> 
>> init_fpstate is a special case, which is indicated by a null pointer
>> parameter to fpstate_init().
>> 
>> Also, fpstate_init_xstate() now accepts the state component bitmap to
>> customize the compacted format.
> 
> That's not a changelog. Changelogs have to explain the WHY not the WHAT.
> 
> I can see the WHY when I look at the later changes, but that's not how
> it works.

The same feedback was raised before [1]. I thought this changelog has been
settled down with Boris [2].

How about:

    “To prepare dynamic features, change fpstate_init()’s argument to a struct
     fpu * pointer instead of a struct fpregs_state * pointer.  A struct fpu
     will have new fields to handle dynamic features."

With fpstate_init_xstate() changes in a separate patch and defining init_fpu,
the last two sentences shall be removed.

> Also the subject of this patch is just wrong. It does not make the
> functions handle dynamic buffers, it prepares them to add support for
> that later.

How about “Prepare fpstate_init() to handle dynamic features"

>> +static inline void fpstate_init_xstate(struct xregs_state *xsave, u64 mask)
>> +{
>> +	/*
>> +	 * XRSTORS requires these bits set in xcomp_bv, or it will
>> +	 * trigger #GP:
>> +	 */
>> +	xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | mask;
>> +}
> 
> This wants to be a separate cleanup patch which replaces the open coded
> variant here:

Okay, maybe the change becomes to be the new patch1.

>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>> index fc1d529547e6..0fed7fbcf2e8 100644
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -395,8 +395,7 @@ static void __init setup_init_fpu_buf(void)
>> 	print_xstate_features();
>> 
>> 	if (boot_cpu_has(X86_FEATURE_XSAVES))
>> -		init_fpstate.xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
>> -						     xfeatures_mask_all;
>> +		fpstate_init_xstate(&init_fpstate.xsave, xfeatures_mask_all);

Thanks,
Chang

[1] https://lore.kernel.org/lkml/20201207171251.GB16640@zn.tnic/
[2] https://lore.kernel.org/lkml/20210115124038.GA11337@zn.tnic/
diff mbox series

Patch

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 5a18694a89b2..c7a64e2806a9 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -80,7 +80,7 @@  static __always_inline __pure bool use_fxsr(void)
 
 extern union fpregs_state init_fpstate;
 
-extern void fpstate_init(union fpregs_state *state);
+extern void fpstate_init(struct fpu *fpu);
 #ifdef CONFIG_MATH_EMULATION
 extern void fpstate_init_soft(struct swregs_state *soft);
 #else
@@ -88,6 +88,15 @@  static inline void fpstate_init_soft(struct swregs_state *soft) {}
 #endif
 extern void save_fpregs_to_fpstate(struct fpu *fpu);
 
+static inline void fpstate_init_xstate(struct xregs_state *xsave, u64 mask)
+{
+	/*
+	 * XRSTORS requires these bits set in xcomp_bv, or it will
+	 * trigger #GP:
+	 */
+	xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | mask;
+}
+
 /* Returns 0 or the negated trap number, which results in -EFAULT for #PF */
 #define user_insn(insn, output, input...)				\
 ({									\
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 7ada7bd03a32..c0098f8422de 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -203,15 +203,6 @@  void fpu_sync_fpstate(struct fpu *fpu)
 	fpregs_unlock();
 }
 
-static inline void fpstate_init_xstate(struct xregs_state *xsave)
-{
-	/*
-	 * XRSTORS requires these bits set in xcomp_bv, or it will
-	 * trigger #GP:
-	 */
-	xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | xfeatures_mask_all;
-}
-
 static inline void fpstate_init_fxstate(struct fxregs_state *fx)
 {
 	fx->cwd = 0x37f;
@@ -229,8 +220,23 @@  static inline void fpstate_init_fstate(struct fregs_state *fp)
 	fp->fos = 0xffff0000u;
 }
 
-void fpstate_init(union fpregs_state *state)
+/**
+ *
+ * fpstate_init - initialize the xstate buffer
+ *
+ * If @fpu is NULL, initialize init_fpstate.
+ *
+ * @fpu:	A struct fpu * pointer
+ */
+void fpstate_init(struct fpu *fpu)
 {
+	union fpregs_state *state;
+
+	if (likely(fpu))
+		state = &fpu->state;
+	else
+		state = &init_fpstate;
+
 	if (!static_cpu_has(X86_FEATURE_FPU)) {
 		fpstate_init_soft(&state->soft);
 		return;
@@ -239,7 +245,7 @@  void fpstate_init(union fpregs_state *state)
 	memset(state, 0, fpu_kernel_xstate_size);
 
 	if (static_cpu_has(X86_FEATURE_XSAVES))
-		fpstate_init_xstate(&state->xsave);
+		fpstate_init_xstate(&state->xsave, xfeatures_mask_all);
 	if (static_cpu_has(X86_FEATURE_FXSR))
 		fpstate_init_fxstate(&state->fxsave);
 	else
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 64e29927cc32..e14c72bc8706 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -124,7 +124,7 @@  static void __init fpu__init_system_generic(void)
 	 * Set up the legacy init FPU context. (xstate init might overwrite this
 	 * with a more modern format, if the CPU supports it.)
 	 */
-	fpstate_init(&init_fpstate);
+	fpstate_init(NULL);
 
 	fpu__init_system_mxcsr();
 }
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index fc1d529547e6..0fed7fbcf2e8 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -395,8 +395,7 @@  static void __init setup_init_fpu_buf(void)
 	print_xstate_features();
 
 	if (boot_cpu_has(X86_FEATURE_XSAVES))
-		init_fpstate.xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
-						     xfeatures_mask_all;
+		fpstate_init_xstate(&init_fpstate.xsave, xfeatures_mask_all);
 
 	/*
 	 * Init all the features state with header.xfeatures being 0x0
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e5d5c5ed7dd4..ce529ab233d7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10608,7 +10608,7 @@  static void fx_init(struct kvm_vcpu *vcpu)
 	if (!vcpu->arch.guest_fpu)
 		return;
 
-	fpstate_init(&vcpu->arch.guest_fpu->state);
+	fpstate_init(vcpu->arch.guest_fpu);
 	if (boot_cpu_has(X86_FEATURE_XSAVES))
 		vcpu->arch.guest_fpu->state.xsave.header.xcomp_bv =
 			host_xcr0 | XSTATE_COMPACTION_ENABLED;