From mboxrd@z Thu Jan  1 00:00:00 1970
From: Borislav Petkov <bp@alien8.de>
Subject: Re: [PATCH 09/22] x86/fpu: Add (__)make_fpregs_active helpers
Date: Mon, 28 Jan 2019 19:23:49 +0100
Message-ID: <20190128182349.GG20487@zn.tnic>
References: <20190109114744.10936-1-bigeasy@linutronix.de>
 <20190109114744.10936-10-bigeasy@linutronix.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
        Andy Lutomirski <luto@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        kvm@vger.kernel.org, "Jason A. Donenfeld" <Jason@zx2c4.com>,
        Rik van Riel <riel@surriel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Return-path: <linux-kernel-owner@vger.kernel.org>
Content-Disposition: inline
In-Reply-To: <20190109114744.10936-10-bigeasy@linutronix.de>
Sender: linux-kernel-owner@vger.kernel.org
List-Id: kvm.vger.kernel.org

On Wed, Jan 09, 2019 at 12:47:31PM +0100, Sebastian Andrzej Siewior wrote:
> From: Rik van Riel <riel@surriel.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@surriel.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.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.