[09/22] x86/fpu: Add (__)make_fpregs_active helpers
diff mbox series

Message ID 20190109114744.10936-10-bigeasy@linutronix.de
State New
Headers show
Series
  • [v6] x86: load FPU registers on return to userland
Related show

Commit Message

Sebastian Andrzej Siewior Jan. 9, 2019, 11:47 a.m. UTC
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(-)

Comments

Borislav Petkov Jan. 28, 2019, 6:23 p.m. UTC | #1
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.
Sebastian Andrzej Siewior Feb. 7, 2019, 10:43 a.m. UTC | #2
On 2019-01-28 19:23:49 [+0100], Borislav Petkov wrote:
> > 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?

correct.

> 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.

They are accessible inside the region. But they should not be touched by
context switch code (and later BH).
Is that what you meant?

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

No. I picked up the patches, that function was named like that. I kept
it. That __ probably denotes that it is an internal function but then it
has to be used outside (KVM) if they plan to "reload" registers (which
happens if they switch between host/guest registers).

> > +{
> > +	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?

Two helpers are added:
- __fpregs_changes_{begin|end}()
  new.

- __fpregs_load_activate()
  refactored from switch_fpu_finish(),

> Thx.
> 

Sebastian
Borislav Petkov Feb. 13, 2019, 9:30 a.m. UTC | #3
On Thu, Feb 07, 2019 at 11:43:25AM +0100, Sebastian Andrzej Siewior wrote:
> They are accessible inside the region. But they should not be touched by
> context switch code (and later BH).
> Is that what you meant?

I just don't like that "changes" name. So when called, those functions
practically lock the FPU regs from being accessed by others. So with

fpregs_lock
fpregs_unlock

for example, is kinda clear what's going on and you don't have to wonder
what it does.

> No. I picked up the patches, that function was named like that. I kept
> it. That __ probably denotes that it is an internal function but then it
> has to be used outside (KVM) if they plan to "reload" registers (which
> happens if they switch between host/guest registers).

Ok, so you can drop the "__".
Sebastian Andrzej Siewior Feb. 14, 2019, 2:51 p.m. UTC | #4
On 2019-02-13 10:30:25 [+0100], Borislav Petkov wrote:
> On Thu, Feb 07, 2019 at 11:43:25AM +0100, Sebastian Andrzej Siewior wrote:
> > They are accessible inside the region. But they should not be touched by
> > context switch code (and later BH).
> > Is that what you meant?
> 
> I just don't like that "changes" name. So when called, those functions
> practically lock the FPU regs from being accessed by others. So with
> 
> fpregs_lock
> fpregs_unlock
> 
> for example, is kinda clear what's going on and you don't have to wonder
> what it does.

renamed as suggested.

Sebastian

Patch
diff mbox series

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)
+{
+	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);
 }
 
 /*