lkml.org 
[lkml]   [2018]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 04/11] x86/fpu: eager switch PKRU state
On Fri, Oct 12, 2018 at 10:51 AM Dave Hansen
<dave.hansen@linux.intel.com> wrote:
>
> On 10/04/2018 07:05 AM, Sebastian Andrzej Siewior wrote:
> > From: Rik van Riel <riel@surriel.com>
> >
> > 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@surriel.com>
> > [bigeasy: save pkru to xstate, no cache]
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > 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?

\
 
 \ /
  Last update: 2018-10-12 20:10    [W:0.138 / U:2.540 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site