linux-kernel - Re: [PATCH 09/22] x86/fpu: Add (__)make_fpregs

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: <20190128182349.GG20487@zn.tnic>
Date:   Mon, 28 Jan 2019 19:23:49 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Andy Lutomirski <luto@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        kvm@...r.kernel.org, "Jason A. Donenfeld" <Jason@...c4.com>,
        Rik van Riel <riel@...riel.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH 09/22] x86/fpu: Add (__)make_fpregs_active helpers

On Wed, Jan 09, 2019 at 12:47:31PM +0100, Sebastian Andrzej Siewior wrote:
> From: Rik van Riel <riel@...riel.com>
> 
> Add helper function that ensures the floating point registers for
> the current task are active. Use with preemption disabled.
> 
> Signed-off-by: Rik van Riel <riel@...riel.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> ---
>  arch/x86/include/asm/fpu/api.h      | 11 +++++++++++
>  arch/x86/include/asm/fpu/internal.h | 19 +++++++++++--------
>  2 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
> index b56d504af6545..31b66af8eb914 100644
> --- a/arch/x86/include/asm/fpu/api.h
> +++ b/arch/x86/include/asm/fpu/api.h
> @@ -10,6 +10,7 @@
>  
>  #ifndef _ASM_X86_FPU_API_H
>  #define _ASM_X86_FPU_API_H
> +#include <linux/preempt.h>
>  
>  /*
>   * Use kernel_fpu_begin/end() if you intend to use FPU in kernel context. It
> @@ -22,6 +23,16 @@ extern void kernel_fpu_begin(void);
>  extern void kernel_fpu_end(void);
>  extern bool irq_fpu_usable(void);
>  
> +static inline void __fpregs_changes_begin(void)
> +{
> +	preempt_disable();
> +}
> +
> +static inline void __fpregs_changes_end(void)

How am I to understand that "fpregs_changes" thing? That FPU registers
changes will begin and end respectively?

I probably would call them fpregs_lock and fpregs_unlock even if
it isn't doing any locking to denote that FPU regs are locked and
inaccessible inside the region.

And why the "__" prefix? Is there a counterpart without the "__" coming?

> +{
> +	preempt_enable();
> +}
> +
>  /*
>   * Query the presence of one or more xfeatures. Works on any legacy CPU as well.
>   *
> diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
> index 03acb9aeb32fc..795a0a2df135e 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -515,6 +515,15 @@ static inline void fpregs_activate(struct fpu *fpu)
>  	trace_x86_fpu_regs_activated(fpu);
>  }
>  
> +static inline void __fpregs_load_activate(struct fpu *fpu, int cpu)
> +{
> +	if (!fpregs_state_valid(fpu, cpu)) {
> +		if (current->mm)
> +			copy_kernel_to_fpregs(&fpu->state);
> +		fpregs_activate(fpu);
> +	}
> +}
> +
>  /*
>   * FPU state switching for scheduling.
>   *
> @@ -550,14 +559,8 @@ switch_fpu_prepare(struct fpu *old_fpu, int cpu)
>   */
>  static inline void switch_fpu_finish(struct fpu *new_fpu, int cpu)
>  {
> -	if (static_cpu_has(X86_FEATURE_FPU)) {
> -		if (!fpregs_state_valid(new_fpu, cpu)) {
> -			if (current->mm)
> -				copy_kernel_to_fpregs(&new_fpu->state);
> -		}
> -
> -		fpregs_activate(new_fpu);
> -	}
> +	if (static_cpu_has(X86_FEATURE_FPU))
> +		__fpregs_load_activate(new_fpu, cpu);

And that second part of a cleanup looks strange in this patch. Why isn't
it in a separate patch or how is it related to the addition of the
helpers?

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.