On Fri, Jul 17, 2020 at 10:31:40AM +0200, Peter Zijlstra wrote:
> On Fri, Jul 17, 2020 at 12:20:43AM -0700, ira.weiny@xxxxxxxxx wrote:
>
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index f362ce0d5ac0..d69250a7c1bf 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -42,6 +42,7 @@
> > #include <asm/spec-ctrl.h>
> > #include <asm/io_bitmap.h>
> > #include <asm/proto.h>
> > +#include <asm/pkeys_internal.h>
> >
> > #include "process.h"
> >
> > @@ -184,6 +185,36 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
> > return ret;
> > }
> >
> > +/*
> > + * NOTE: We wrap pks_init_task() and pks_sched_in() with
> > + * CONFIG_ARCH_HAS_SUPERVISOR_PKEYS because using IS_ENABLED() fails
> > + * due to the lack of task_struct->saved_pkrs in this configuration.
> > + * Furthermore, we place them here because of the complexity introduced by
> > + * header conflicts introduced to get the task_struct definition in the pkeys
> > + * headers.
> > + */
>
> I don't see anything much useful in that comment.
I'm happy to delete. Internal reviews questioned the motive here so I added
the comment to inform why this style was chosen rather than the preferred
IS_ENABLED().
I've deleted it now.
>
> > +#ifdef CONFIG_ARCH_HAS_SUPERVISOR_PKEYS
> > +DECLARE_PER_CPU(u32, pkrs_cache);
> > +static inline void pks_init_task(struct task_struct *tsk)
> > +{
> > + /* New tasks get the most restrictive PKRS value */
> > + tsk->thread.saved_pkrs = INIT_PKRS_VALUE;
> > +}
> > +static inline void pks_sched_in(void)
> > +{
> > + u64 current_pkrs = current->thread.saved_pkrs;
> > +
> > + /* Only update the MSR when current's pkrs is different from the MSR. */
> > + if (this_cpu_read(pkrs_cache) == current_pkrs)
> > + return;
> > +
> > + write_pkrs(current_pkrs);
>
> Should we write that like:
>
> /*
> * PKRS is only temporarily changed during specific code paths.
> * Only a preemption during these windows away from the default
> * value would require updating the MSR.
> */
> if (unlikely(this_cpu_read(pkrs_cache) != current_pkrs))
> write_pkrs(current_pkrs);
>
> ?
Yes I think the unlikely is better.
Thanks,
Ira
>
> > +}
> > +#else
> > +static inline void pks_init_task(struct task_struct *tsk) { }
> > +static inline void pks_sched_in(void) { }
> > +#endif
