Re: [RFC PATCH bpf-next 1/5] x86/fpu: Move context.h to include/asm

[David Vernet <void@manifault.com>]

On Thu, May 12, 2022 at 12:43:17AM -0700, Dave Marchevsky wrote:
> The file's fpregs_state_valid function is useful outside of
> arch/x86/kernel/fpu dir. Further commits in this series use
> fpregs_state_valid to determine whether a BPF helper should fetch
> fpu reg value from xsave'd memory or register.
> 
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
>  arch/x86/{kernel => include/asm}/fpu/context.h | 2 ++
>  arch/x86/kernel/fpu/core.c                     | 2 +-
>  arch/x86/kernel/fpu/regset.c                   | 2 +-
>  arch/x86/kernel/fpu/signal.c                   | 2 +-
>  arch/x86/kernel/fpu/xstate.c                   | 2 +-
>  5 files changed, 6 insertions(+), 4 deletions(-)
>  rename arch/x86/{kernel => include/asm}/fpu/context.h (96%)
> 
> diff --git a/arch/x86/kernel/fpu/context.h b/arch/x86/include/asm/fpu/context.h
> similarity index 96%
> rename from arch/x86/kernel/fpu/context.h
> rename to arch/x86/include/asm/fpu/context.h
> index 958accf2ccf0..39dac18cd22c 100644
> --- a/arch/x86/kernel/fpu/context.h
> +++ b/arch/x86/include/asm/fpu/context.h
> @@ -51,6 +51,8 @@ static inline void fpregs_activate(struct fpu *fpu)
>  	trace_x86_fpu_regs_activated(fpu);
>  }
>  
> +extern void restore_fpregs_from_fpstate(struct fpstate *fpstate, u64 mask);

This signature is already included in arch/x86/include/asm/fpu/signal.h.
Should we just include that header from arch/x86/include/asm/fpu/context.h
rather than declaring the signature twice?

> +
>  /* Internal helper for switch_fpu_return() and signal frame setup */
>  static inline void fpregs_restore_userregs(void)
>  {
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index c049561f373a..5296112d4273 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -7,6 +7,7 @@
>   *	Gareth Hughes <gareth@valinux.com>, May 2000
>   */
>  #include <asm/fpu/api.h>
> +#include <asm/fpu/context.h>
>  #include <asm/fpu/regset.h>
>  #include <asm/fpu/sched.h>
>  #include <asm/fpu/signal.h>
> @@ -18,7 +19,6 @@
>  #include <linux/pkeys.h>
>  #include <linux/vmalloc.h>
>  
> -#include "context.h"
>  #include "internal.h"
>  #include "legacy.h"
>  #include "xstate.h"
> diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
> index 75ffaef8c299..f93336f332e3 100644
> --- a/arch/x86/kernel/fpu/regset.c
> +++ b/arch/x86/kernel/fpu/regset.c
> @@ -6,10 +6,10 @@
>  #include <linux/vmalloc.h>
>  
>  #include <asm/fpu/api.h>
> +#include <asm/fpu/context.h>
>  #include <asm/fpu/signal.h>
>  #include <asm/fpu/regset.h>
>  
> -#include "context.h"
>  #include "internal.h"
>  #include "legacy.h"
>  #include "xstate.h"
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 91d4b6de58ab..f099a56c9a93 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -7,6 +7,7 @@
>  #include <linux/cpu.h>
>  #include <linux/pagemap.h>
>  
> +#include <asm/fpu/context.h>
>  #include <asm/fpu/signal.h>
>  #include <asm/fpu/regset.h>
>  #include <asm/fpu/xstate.h>
> @@ -15,7 +16,6 @@
>  #include <asm/trapnr.h>
>  #include <asm/trace/fpu.h>
>  
> -#include "context.h"
>  #include "internal.h"
>  #include "legacy.h"
>  #include "xstate.h"
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 39e1c8626ab9..ab5e26075716 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -15,6 +15,7 @@
>  #include <linux/vmalloc.h>
>  
>  #include <asm/fpu/api.h>
> +#include <asm/fpu/context.h>
>  #include <asm/fpu/regset.h>
>  #include <asm/fpu/signal.h>
>  #include <asm/fpu/xcr.h>
> @@ -23,7 +24,6 @@
>  #include <asm/prctl.h>
>  #include <asm/elf.h>
>  
> -#include "context.h"
>  #include "internal.h"
>  #include "legacy.h"
>  #include "xstate.h"
> -- 
> 2.30.2
> 

Looks reasonable otherwise, though it's unclear to me (as I'm not an expert
in this code) whether we should or shouldn't export all of these functions
if some of them are specific to the logic in arch/x86/kernel/fpu. It looks
like there's already a precedent for doing that with e.g.
fpu__clear_user_states(), so it seems fine.

Thanks,
David