Re: [PATCH 04/11] x86/fpu: eager switch PKRU state

[Andy Lutomirski]

On Fri, Oct 12, 2018 at 10:51 AM Dave Hansen
<dave.hansen@xxxxxxxxxxxxxxx> wrote:
>
> On 10/04/2018 07:05 AM, Sebastian Andrzej Siewior wrote:
> > From: Rik van Riel <riel@xxxxxxxxxxx>
> >
> > While most of a task's FPU state is only needed in user space,
> > the protection keys need to be in place immediately after a
> > context switch.
> >
> > The reason is that any accesses to userspace memory while running
> > in kernel mode also need to abide by the memory permissions
> > specified in the protection keys.
> >
> > The "eager switch" is a preparation for loading the FPU state on return
> > to userland. Instead of decoupling PKRU state from xstate I update PKRU
> > within xstate on write operations by the kernel.
> >
> > Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
> > [bigeasy: save pkru to xstate, no cache]
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> > ---
> >  arch/x86/include/asm/fpu/internal.h | 20 +++++++++++++++----
> >  arch/x86/include/asm/fpu/xstate.h   |  2 ++
> >  arch/x86/include/asm/pgtable.h      |  6 +-----
> >  arch/x86/include/asm/pkeys.h        |  2 +-
> >  arch/x86/kernel/fpu/core.c          |  2 +-
> >  arch/x86/mm/pkeys.c                 | 31 ++++++++++++++++++++++-------
> >  include/linux/pkeys.h               |  2 +-
> >  7 files changed, 46 insertions(+), 19 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
> > index 16c4077ffc945..956d967ca824a 100644
> > --- a/arch/x86/include/asm/fpu/internal.h
> > +++ b/arch/x86/include/asm/fpu/internal.h
> > @@ -570,11 +570,23 @@ switch_fpu_prepare(struct fpu *old_fpu, int cpu)
> >   */
> >  static inline void switch_fpu_finish(struct fpu *new_fpu, int cpu)
> >  {
> > -     bool preload = static_cpu_has(X86_FEATURE_FPU) &&
> > -                    new_fpu->initialized;
> > +     bool load_fpu;
> >
> > -     if (preload)
> > -             __fpregs_load_activate(new_fpu, cpu);
> > +     load_fpu = static_cpu_has(X86_FEATURE_FPU) && new_fpu->initialized;
> > +     if (!load_fpu)
> > +             return;
>
> Needs comments, please.  Especially around what an uninitialized new_fpu
> means.

See my other comment about getting rid of ->initialized *first*.

>
> > +     __fpregs_load_activate(new_fpu, cpu);
> > +
> > +#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
> > +     if (static_cpu_has(X86_FEATURE_OSPKE)) {
>
> FWIW, you should be able to use cpu_feature_enabled() instead of an
> explicit #ifdef here.
>
> > +             struct pkru_state *pk;
> > +
> > +             pk = __raw_xsave_addr(&new_fpu->state.xsave, XFEATURE_PKRU);
> > +             if (pk->pkru != __read_pkru())
> > +                     __write_pkru(pk->pkru);
> > +     }
> > +#endif
> >  }
>
> Comments here as well, please.
>
> I think the goal is to keep the PKRU state in the 'init state' when
> possible and also to save the cost of WRPKRU.  But, it would be really
> nice to be explicit.

I suspect that this makes basically no difference.  PKRU is almost
never in the init state on Linux.  Also, it's a single word -- I doubt
that the init state optimization is worth much.

But maybe WRPKRU is more expensive than RDPKRU and a branch?