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
