Re: [PATCH RFC V3 4/9] x86/pks: Preserve the PKRS MSR on context switch
On Mon, Oct 19, 2020 at 11:37:14AM +0200, Peter Zijlstra wrote: > On Fri, Oct 16, 2020 at 10:14:10PM -0700, Ira Weiny wrote: > > > so it either needs to > > > explicitly do so, or have an assertion that preemption is indeed > > > disabled. > > > > However, I don't think I understand clearly. Doesn't [get|put]_cpu_ptr() > > handle the preempt_disable() for us? > > It does. > > > Is it not sufficient to rely on that? > > It is. > > > Dave's comment seems to be the opposite where we need to eliminate preempt > > disable before calling write_pkrs(). > > > > FWIW I think I'm mistaken in my response to Dave regarding the > > preempt_disable() in pks_update_protection(). > > Dave's concern is that we're calling with with preemption already > disabled so disabling it again is superfluous. Ok, thanks, and after getting my head straight I think I agree with him, and you. Thanks I've reworked the code to removed the superfluous calls. Sorry about being so dense... :-D Ira
Re: [PATCH RFC V3 4/9] x86/pks: Preserve the PKRS MSR on context switch
On Fri, Oct 16, 2020 at 10:14:10PM -0700, Ira Weiny wrote: > > so it either needs to > > explicitly do so, or have an assertion that preemption is indeed > > disabled. > > However, I don't think I understand clearly. Doesn't [get|put]_cpu_ptr() > handle the preempt_disable() for us? It does. > Is it not sufficient to rely on that? It is. > Dave's comment seems to be the opposite where we need to eliminate preempt > disable before calling write_pkrs(). > > FWIW I think I'm mistaken in my response to Dave regarding the > preempt_disable() in pks_update_protection(). Dave's concern is that we're calling with with preemption already disabled so disabling it again is superfluous.
Re: [PATCH RFC V3 4/9] x86/pks: Preserve the PKRS MSR on context switch
On Fri, Oct 16, 2020 at 01:06:36PM +0200, Peter Zijlstra wrote:
> On Fri, Oct 09, 2020 at 12:42:53PM -0700, ira.we...@intel.com wrote:
>
> > @@ -644,6 +663,8 @@ void __switch_to_xtra(struct task_struct *prev_p,
> > struct task_struct *next_p)
> >
> > if ((tifp ^ tifn) & _TIF_SLD)
> > switch_to_sld(tifn);
> > +
> > + pks_sched_in();
> > }
> >
>
> You seem to have lost the comment proposed here:
>
>
> https://lkml.kernel.org/r/20200717083140.gw10...@hirez.programming.kicks-ass.net
>
> It is useful and important information that the wrmsr normally doesn't
> happen.
Added back in here.
>
> > diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c
> > index 3cf8f775f36d..30f65dd3d0c5 100644
> > --- a/arch/x86/mm/pkeys.c
> > +++ b/arch/x86/mm/pkeys.c
> > @@ -229,3 +229,31 @@ u32 update_pkey_val(u32 pk_reg, int pkey, unsigned int
> > flags)
> >
> > return pk_reg;
> > }
> > +
> > +DEFINE_PER_CPU(u32, pkrs_cache);
> > +
> > +/**
> > + * It should also be noted that the underlying WRMSR(MSR_IA32_PKRS) is not
> > + * serializing but still maintains ordering properties similar to WRPKRU.
> > + * The current SDM section on PKRS needs updating but should be the same as
> > + * that of WRPKRU. So to quote from the WRPKRU text:
> > + *
> > + * WRPKRU will never execute transiently. Memory accesses
> > + * affected by PKRU register will not execute (even transiently)
> > + * until all prior executions of WRPKRU have completed execution
> > + * and updated the PKRU register.
>
> (whitespace damage; space followed by tabstop)
Fixed thanks.
>
> > + */
> > +void write_pkrs(u32 new_pkrs)
> > +{
> > + u32 *pkrs;
> > +
> > + if (!static_cpu_has(X86_FEATURE_PKS))
> > + return;
> > +
> > + pkrs = get_cpu_ptr(&pkrs_cache);
> > + if (*pkrs != new_pkrs) {
> > + *pkrs = new_pkrs;
> > + wrmsrl(MSR_IA32_PKRS, new_pkrs);
> > + }
> > + put_cpu_ptr(pkrs);
> > +}
>
> looks familiar that... :-)
Added you as a co-developer if that is ok?
Ira
Re: [PATCH RFC V3 4/9] x86/pks: Preserve the PKRS MSR on context switch
On Fri, Oct 16, 2020 at 01:12:26PM +0200, Peter Zijlstra wrote:
> On Tue, Oct 13, 2020 at 11:31:45AM -0700, Dave Hansen wrote:
> > > +/**
> > > + * It should also be noted that the underlying WRMSR(MSR_IA32_PKRS) is
> > > not
> > > + * serializing but still maintains ordering properties similar to WRPKRU.
> > > + * The current SDM section on PKRS needs updating but should be the same
> > > as
> > > + * that of WRPKRU. So to quote from the WRPKRU text:
> > > + *
> > > + * WRPKRU will never execute transiently. Memory accesses
> > > + * affected by PKRU register will not execute (even transiently)
> > > + * until all prior executions of WRPKRU have completed execution
> > > + * and updated the PKRU register.
> > > + */
> > > +void write_pkrs(u32 new_pkrs)
> > > +{
> > > + u32 *pkrs;
> > > +
> > > + if (!static_cpu_has(X86_FEATURE_PKS))
> > > + return;
> > > +
> > > + pkrs = get_cpu_ptr(&pkrs_cache);
> > > + if (*pkrs != new_pkrs) {
> > > + *pkrs = new_pkrs;
> > > + wrmsrl(MSR_IA32_PKRS, new_pkrs);
> > > + }
> > > + put_cpu_ptr(pkrs);
> > > +}
> > >
> >
> > It bugs me a *bit* that this is being called in a preempt-disabled
> > region, but we still bother with the get/put_cpu jazz. Are there other
> > future call-sites for this that aren't in preempt-disabled regions?
>
> So the previous version had a useful comment that got lost.
Ok Looking back I see what happened... This comment...
/*
* 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.
*/
... was added to pks_sched_in() but that got simplified down because cleaning
up write_pkrs() made the code there obsolete.
> This stuff
> needs to fundamentally be preempt disabled,
I agree, the update to the percpu cache value and MSR can not be torn.
> so it either needs to
> explicitly do so, or have an assertion that preemption is indeed
> disabled.
However, I don't think I understand clearly. Doesn't [get|put]_cpu_ptr()
handle the preempt_disable() for us? Is it not sufficient to rely on that?
Dave's comment seems to be the opposite where we need to eliminate preempt
disable before calling write_pkrs().
FWIW I think I'm mistaken in my response to Dave regarding the
preempt_disable() in pks_update_protection().
Ira
Re: [PATCH RFC V3 4/9] x86/pks: Preserve the PKRS MSR on context switch
On Tue, Oct 13, 2020 at 11:31:45AM -0700, Dave Hansen wrote:
> > +/**
> > + * It should also be noted that the underlying WRMSR(MSR_IA32_PKRS) is not
> > + * serializing but still maintains ordering properties similar to WRPKRU.
> > + * The current SDM section on PKRS needs updating but should be the same as
> > + * that of WRPKRU. So to quote from the WRPKRU text:
> > + *
> > + * WRPKRU will never execute transiently. Memory accesses
> > + * affected by PKRU register will not execute (even transiently)
> > + * until all prior executions of WRPKRU have completed execution
> > + * and updated the PKRU register.
> > + */
> > +void write_pkrs(u32 new_pkrs)
> > +{
> > + u32 *pkrs;
> > +
> > + if (!static_cpu_has(X86_FEATURE_PKS))
> > + return;
> > +
> > + pkrs = get_cpu_ptr(&pkrs_cache);
> > + if (*pkrs != new_pkrs) {
> > + *pkrs = new_pkrs;
> > + wrmsrl(MSR_IA32_PKRS, new_pkrs);
> > + }
> > + put_cpu_ptr(pkrs);
> > +}
> >
>
> It bugs me a *bit* that this is being called in a preempt-disabled
> region, but we still bother with the get/put_cpu jazz. Are there other
> future call-sites for this that aren't in preempt-disabled regions?
So the previous version had a useful comment that got lost. This stuff
needs to fundamentally be preempt disabled, so it either needs to
explicitly do so, or have an assertion that preemption is indeed
disabled.
Re: [PATCH RFC V3 4/9] x86/pks: Preserve the PKRS MSR on context switch
On Fri, Oct 09, 2020 at 12:42:53PM -0700, ira.we...@intel.com wrote:
> @@ -644,6 +663,8 @@ void __switch_to_xtra(struct task_struct *prev_p, struct
> task_struct *next_p)
>
> if ((tifp ^ tifn) & _TIF_SLD)
> switch_to_sld(tifn);
> +
> + pks_sched_in();
> }
>
You seem to have lost the comment proposed here:
https://lkml.kernel.org/r/20200717083140.gw10...@hirez.programming.kicks-ass.net
It is useful and important information that the wrmsr normally doesn't
happen.
> diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c
> index 3cf8f775f36d..30f65dd3d0c5 100644
> --- a/arch/x86/mm/pkeys.c
> +++ b/arch/x86/mm/pkeys.c
> @@ -229,3 +229,31 @@ u32 update_pkey_val(u32 pk_reg, int pkey, unsigned int
> flags)
>
> return pk_reg;
> }
> +
> +DEFINE_PER_CPU(u32, pkrs_cache);
> +
> +/**
> + * It should also be noted that the underlying WRMSR(MSR_IA32_PKRS) is not
> + * serializing but still maintains ordering properties similar to WRPKRU.
> + * The current SDM section on PKRS needs updating but should be the same as
> + * that of WRPKRU. So to quote from the WRPKRU text:
> + *
> + * WRPKRU will never execute transiently. Memory accesses
> + * affected by PKRU register will not execute (even transiently)
> + * until all prior executions of WRPKRU have completed execution
> + * and updated the PKRU register.
(whitespace damage; space followed by tabstop)
> + */
> +void write_pkrs(u32 new_pkrs)
> +{
> + u32 *pkrs;
> +
> + if (!static_cpu_has(X86_FEATURE_PKS))
> + return;
> +
> + pkrs = get_cpu_ptr(&pkrs_cache);
> + if (*pkrs != new_pkrs) {
> + *pkrs = new_pkrs;
> + wrmsrl(MSR_IA32_PKRS, new_pkrs);
> + }
> + put_cpu_ptr(pkrs);
> +}
looks familiar that... :-)
Re: [PATCH RFC V3 4/9] x86/pks: Preserve the PKRS MSR on context switch
On Tue, Oct 13, 2020 at 11:31:45AM -0700, Dave Hansen wrote:
> On 10/9/20 12:42 PM, ira.we...@intel.com wrote:
> > From: Ira Weiny
> >
> > The PKRS MSR is defined as a per-logical-processor register. This
> > isolates memory access by logical CPU. Unfortunately, the MSR is not
> > managed by XSAVE. Therefore, tasks must save/restore the MSR value on
> > context switch.
> >
> > Define a saved PKRS value in the task struct, as well as a cached
> > per-logical-processor MSR value which mirrors the MSR value of the
> > current CPU. Initialize all tasks with the default MSR value. Then, on
> > schedule in, check the saved task MSR vs the per-cpu value. If
> > different proceed to write the MSR. If not avoid the overhead of the
> > MSR write and continue.
>
> It's probably nice to note how the WRMSR is special here, in addition to
> the comments below.
Sure,
>
> > #endif /*_ASM_X86_PKEYS_INTERNAL_H */
> > diff --git a/arch/x86/include/asm/processor.h
> > b/arch/x86/include/asm/processor.h
> > index 97143d87994c..da2381136b2d 100644
> > --- a/arch/x86/include/asm/processor.h
> > +++ b/arch/x86/include/asm/processor.h
> > @@ -18,6 +18,7 @@ struct vm86;
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -542,6 +543,11 @@ struct thread_struct {
> >
> > unsigned intsig_on_uaccess_err:1;
> >
> > +#ifdef CONFIG_ARCH_HAS_SUPERVISOR_PKEYS
> > + /* Saved Protection key register for supervisor mappings */
> > + u32 saved_pkrs;
> > +#endif
>
> Could you take a look around thread_struct and see if there are some
> other MSRs near which you can stash this? This seems like a bit of a
> lonely place.
Are you more concerned with aesthetics or the in memory struct layout?
How about I put it after error_code?
unsigned long error_code;
+
+#ifdef CONFIG_ARCH_HAS_SUPERVISOR_PKEYS
+ /* Saved Protection key register for supervisor mappings */
+ u32 saved_pkrs;
+#endif
+
?
>
> ...
> > void flush_thread(void)
> > {
> > struct task_struct *tsk = current;
> > @@ -195,6 +212,8 @@ void flush_thread(void)
> > memset(tsk->thread.tls_array, 0, sizeof(tsk->thread.tls_array));
> >
> > fpu__clear_all(&tsk->thread.fpu);
> > +
> > + pks_init_task(tsk);
> > }
> >
> > void disable_TSC(void)
> > @@ -644,6 +663,8 @@ void __switch_to_xtra(struct task_struct *prev_p,
> > struct task_struct *next_p)
> >
> > if ((tifp ^ tifn) & _TIF_SLD)
> > switch_to_sld(tifn);
> > +
> > + pks_sched_in();
> > }
> >
> > /*
> > diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c
> > index 3cf8f775f36d..30f65dd3d0c5 100644
> > --- a/arch/x86/mm/pkeys.c
> > +++ b/arch/x86/mm/pkeys.c
> > @@ -229,3 +229,31 @@ u32 update_pkey_val(u32 pk_reg, int pkey, unsigned int
> > flags)
> >
> > return pk_reg;
> > }
> > +
> > +DEFINE_PER_CPU(u32, pkrs_cache);
> > +
> > +/**
> > + * It should also be noted that the underlying WRMSR(MSR_IA32_PKRS) is not
> > + * serializing but still maintains ordering properties similar to WRPKRU.
> > + * The current SDM section on PKRS needs updating but should be the same as
> > + * that of WRPKRU. So to quote from the WRPKRU text:
> > + *
> > + * WRPKRU will never execute transiently. Memory accesses
> > + * affected by PKRU register will not execute (even transiently)
> > + * until all prior executions of WRPKRU have completed execution
> > + * and updated the PKRU register.
> > + */
> > +void write_pkrs(u32 new_pkrs)
> > +{
> > + u32 *pkrs;
> > +
> > + if (!static_cpu_has(X86_FEATURE_PKS))
> > + return;
> > +
> > + pkrs = get_cpu_ptr(&pkrs_cache);
> > + if (*pkrs != new_pkrs) {
> > + *pkrs = new_pkrs;
> > + wrmsrl(MSR_IA32_PKRS, new_pkrs);
> > + }
> > + put_cpu_ptr(pkrs);
> > +}
> >
>
> It bugs me a *bit* that this is being called in a preempt-disabled
> region, but we still bother with the get/put_cpu jazz. Are there other
> future call-sites for this that aren't in preempt-disabled regions?
I'm not specifically disabling preempt before calling write_pkrs except in the
next patch (which is buggy because I meant to have it around the modification
of thread.saved_pkrs as well). But that was to protect the thread variable not
the percpu cache vs MSR.
My thought above was it is safer for this call to ensure the per-cpu variable
is consistent with the register. The other calls to write_pkrs() may require
preemption disable but for reasons unrelated to write_pkrs' state.
After some research
Re: [PATCH RFC V3 4/9] x86/pks: Preserve the PKRS MSR on context switch
On 10/9/20 12:42 PM, ira.we...@intel.com wrote:
> From: Ira Weiny
>
> The PKRS MSR is defined as a per-logical-processor register. This
> isolates memory access by logical CPU. Unfortunately, the MSR is not
> managed by XSAVE. Therefore, tasks must save/restore the MSR value on
> context switch.
>
> Define a saved PKRS value in the task struct, as well as a cached
> per-logical-processor MSR value which mirrors the MSR value of the
> current CPU. Initialize all tasks with the default MSR value. Then, on
> schedule in, check the saved task MSR vs the per-cpu value. If
> different proceed to write the MSR. If not avoid the overhead of the
> MSR write and continue.
It's probably nice to note how the WRMSR is special here, in addition to
the comments below.
> #endif /*_ASM_X86_PKEYS_INTERNAL_H */
> diff --git a/arch/x86/include/asm/processor.h
> b/arch/x86/include/asm/processor.h
> index 97143d87994c..da2381136b2d 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -18,6 +18,7 @@ struct vm86;
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -542,6 +543,11 @@ struct thread_struct {
>
> unsigned intsig_on_uaccess_err:1;
>
> +#ifdef CONFIG_ARCH_HAS_SUPERVISOR_PKEYS
> + /* Saved Protection key register for supervisor mappings */
> + u32 saved_pkrs;
> +#endif
Could you take a look around thread_struct and see if there are some
other MSRs near which you can stash this? This seems like a bit of a
lonely place.
...
> void flush_thread(void)
> {
> struct task_struct *tsk = current;
> @@ -195,6 +212,8 @@ void flush_thread(void)
> memset(tsk->thread.tls_array, 0, sizeof(tsk->thread.tls_array));
>
> fpu__clear_all(&tsk->thread.fpu);
> +
> + pks_init_task(tsk);
> }
>
> void disable_TSC(void)
> @@ -644,6 +663,8 @@ void __switch_to_xtra(struct task_struct *prev_p, struct
> task_struct *next_p)
>
> if ((tifp ^ tifn) & _TIF_SLD)
> switch_to_sld(tifn);
> +
> + pks_sched_in();
> }
>
> /*
> diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c
> index 3cf8f775f36d..30f65dd3d0c5 100644
> --- a/arch/x86/mm/pkeys.c
> +++ b/arch/x86/mm/pkeys.c
> @@ -229,3 +229,31 @@ u32 update_pkey_val(u32 pk_reg, int pkey, unsigned int
> flags)
>
> return pk_reg;
> }
> +
> +DEFINE_PER_CPU(u32, pkrs_cache);
> +
> +/**
> + * It should also be noted that the underlying WRMSR(MSR_IA32_PKRS) is not
> + * serializing but still maintains ordering properties similar to WRPKRU.
> + * The current SDM section on PKRS needs updating but should be the same as
> + * that of WRPKRU. So to quote from the WRPKRU text:
> + *
> + * WRPKRU will never execute transiently. Memory accesses
> + * affected by PKRU register will not execute (even transiently)
> + * until all prior executions of WRPKRU have completed execution
> + * and updated the PKRU register.
> + */
> +void write_pkrs(u32 new_pkrs)
> +{
> + u32 *pkrs;
> +
> + if (!static_cpu_has(X86_FEATURE_PKS))
> + return;
> +
> + pkrs = get_cpu_ptr(&pkrs_cache);
> + if (*pkrs != new_pkrs) {
> + *pkrs = new_pkrs;
> + wrmsrl(MSR_IA32_PKRS, new_pkrs);
> + }
> + put_cpu_ptr(pkrs);
> +}
>
It bugs me a *bit* that this is being called in a preempt-disabled
region, but we still bother with the get/put_cpu jazz. Are there other
future call-sites for this that aren't in preempt-disabled regions?
