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?
