| Message ID | 20190109114744.10936-10-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] x86: load FPU registers on return to userland | expand |
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.
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
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 "__".
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
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); } /*