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

[Ira Weiny <ira.weiny@intel.com>]

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