Re: [PATCH V8 15/44] x86/pkeys: Preserve the PKS MSR on context switch

Ira Weiny
2022-02-11T06:11:03Z

On Fri, Jan 28, 2022 at 04:22:42PM -0800, Dave Hansen wrote:
> On 1/27/22 09:54, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > The PKS MSR (PKRS) is defined as a per-logical-processor register. This
>
> s/defined as//

Done.

>
> > isolates memory access by logical CPU.
>
> This second sentence is a bit confusing to me. I *think* you're trying
> to say that PKRS only affects accesses from one logical CPU.

Yes.

> But, it
> just comes out strangely. I think I'd just axe the sentence.

Yea done.

>
> > Unfortunately, the MSR is not
> > managed by XSAVE. Therefore, tasks must save/restore the MSR value on
> > context switch.
> >
> > Define pks_saved_pkrs in struct thread_struct. Initialize all tasks,
> > including the init_task, with the PKS_INIT_VALUE when created. Restore
> > the CPU's MSR to the saved task value on schedule in.
> >
> > pks_write_current() is added to ensures non-supervisor pkey
>
> ^ ensure

Done.

>
> ...
> > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> > index 2c5f12ae7d04..3530a0e50b4f 100644
> > --- a/arch/x86/include/asm/processor.h
> > +++ b/arch/x86/include/asm/processor.h
> > @@ -2,6 +2,8 @@
> > #ifndef _ASM_X86_PROCESSOR_H
> > #define _ASM_X86_PROCESSOR_H
> >
> > +#include <linux/pks-keys.h>
> > +
> > #include <asm/processor-flags.h>
> >
> > /* Forward declaration, a strange C thing */
> > @@ -502,6 +504,12 @@ struct thread_struct {
> > unsigned long cr2;
> > unsigned long trap_nr;
> > unsigned long error_code;
> > +
> > +#ifdef CONFIG_ARCH_ENABLE_SUPERVISOR_PKEYS
> > + /* Saved Protection key register for supervisor mappings */
> > + u32 pks_saved_pkrs;
> > +#endif
>
> There are a bunch of other "saved" registers in thread_struct. They all
> just have their register name, including pkru.
>
> Can you just stick this next to 'pkru' and call it plain old 'pkrs'?

Sure. I was trying to use the same 'pks_*' prefix everywhere. But pkrs makes
sense too.

> That will probably even take up less space than this since the two
> 32-bit values can be packed together.

Yes done.

[]

> > diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> > index 3402edec236c..81fc0b638308 100644
> > --- a/arch/x86/kernel/process_64.c
> > +++ b/arch/x86/kernel/process_64.c
> > @@ -59,6 +59,7 @@
> > /* Not included via unistd.h */
> > #include <asm/unistd_32_ia32.h>
> > #endif
> > +#include <asm/pks.h>
> >
> > #include "process.h"
> >
> > @@ -657,6 +658,8 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
> > /* Load the Intel cache allocation PQR MSR. */
> > resctrl_sched_in();
> >
> > + pks_write_current();
> > +
> > return prev_p;
> > }
>
> At least for pkru and fsgsbase, these have the form:
>
> x86_<register>_load();
>
> Should this be
>
> x86_pkrs_load();

Ok done.

>
> and be located next to:
>
> x86_pkru_load()?

This presents a problem. As defined this can't happen until current is loaded.
For now I've passed in the next thread_struct but I fear that is going to cause
some bad header dependencies. I'll see what 0day has to say about it and
adjust as needed.

Ira