lkml.org 
[lkml]   [2021]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v3 08/21] x86/fpu/xstate: Define the scope of the initial xstate data
On Wed, Dec 23, 2020 at 07:57:04AM -0800, Chang S. Bae wrote:
> init_fpstate is used to record the initial xstate value for convenience

convenience?

> and covers all the states. But it is wasteful to cover large states all
> with trivial initial data.
>
> Limit init_fpstate by clarifying its size and coverage, which are all but
> dynamic user states. The dynamic states are assumed to be large but having
> initial data with zeros.
>
> No functional change until the kernel supports dynamic user states.

What does that mean?

This patch either makes no functional change or it does...

> 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
> ---
> Changes from v2:
> * Updated the changelog for clarification.
> * Updated the code comments.
> ---
> arch/x86/include/asm/fpu/internal.h | 18 +++++++++++++++---
> arch/x86/include/asm/fpu/xstate.h | 1 +
> arch/x86/kernel/fpu/core.c | 4 ++--
> arch/x86/kernel/fpu/xstate.c | 4 ++--
> 4 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
> index 37ea5e37f21c..bbdd304719c6 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -80,6 +80,18 @@ static __always_inline __pure bool use_fxsr(void)
>
> extern union fpregs_state init_fpstate;
>
> +static inline u64 get_init_fpstate_mask(void)
> +{
> + /* init_fpstate covers states in fpu->state. */
> + return (xfeatures_mask_all & ~xfeatures_mask_user_dynamic);
> +}

If you're going to introduce such a helper, then use it everywhere in the code:

$ git grep "xfeatures_mask_all & ~xfeatures_mask_user_dynamic"
arch/x86/kernel/fpu/core.c:239: dst_fpu->state_mask = xfeatures_mask_all & ~xfeatures_mask_user_dynamic;
arch/x86/kernel/fpu/xstate.c:148: else if (mask == (xfeatures_mask_all & ~xfeatures_mask_user_dynamic))
arch/x86/kernel/fpu/xstate.c:932: current->thread.fpu.state_mask = (xfeatures_mask_all & ~xfeatures_mask_user_dynamic);

and if you do that, do that in a separate pre-patch which does only this
conversion.

> +static inline unsigned int get_init_fpstate_size(void)
> +{
> + /* fpu->state size is aligned with the init_fpstate size. */
> + return fpu_kernel_xstate_min_size;
> +}
> +
> extern void fpstate_init(struct fpu *fpu);
> #ifdef CONFIG_MATH_EMULATION
> extern void fpstate_init_soft(struct swregs_state *soft);
> @@ -269,12 +281,12 @@ static inline void copy_fxregs_to_kernel(struct fpu *fpu)
> : "memory")
>
> /*
> - * This function is called only during boot time when x86 caps are not set
> - * up and alternative can not be used yet.
> + * Use this function to dump the initial state, only during boot time when x86
> + * caps not set up and alternative not available yet.
> */

What's the point of this change? Also, "dump"?!

> static inline void copy_xregs_to_kernel_booting(struct xregs_state *xstate)
> {
> - u64 mask = xfeatures_mask_all;
> + u64 mask = get_init_fpstate_mask();
> u32 lmask = mask;
> u32 hmask = mask >> 32;
> int err;
> diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
> index 379e8f8b8440..62f6583f34fa 100644
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -103,6 +103,7 @@ extern void __init update_regset_xstate_info(unsigned int size,
> u64 xstate_mask);
>
> void *get_xsave_addr(struct fpu *fpu, int xfeature_nr);
> +unsigned int get_xstate_size(u64 mask);
> int alloc_xstate_buffer(struct fpu *fpu, u64 mask);
> void free_xstate_buffer(struct fpu *fpu);
>
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 6dafed34be4f..aad1a7102096 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -206,10 +206,10 @@ void fpstate_init(struct fpu *fpu)
> return;
> }
>
> - memset(state, 0, fpu_kernel_xstate_min_size);
> + memset(state, 0, fpu ? get_xstate_size(fpu->state_mask) : get_init_fpstate_size());
>
> if (static_cpu_has(X86_FEATURE_XSAVES))
> - fpstate_init_xstate(&state->xsave, xfeatures_mask_all);
> + fpstate_init_xstate(&state->xsave, fpu ? fpu->state_mask : get_init_fpstate_mask());

<---- newline here.

> if (static_cpu_has(X86_FEATURE_FXSR))
> fpstate_init_fxstate(&state->fxsave);
> else

...

--
Regards/Gruss,
Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

\
 
 \ /
  Last update: 2021-02-08 13:40    [W:0.571 / U:0.840 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site