| Message ID | 20160817034323.23053-3-cyrilbur@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, 2016-08-17 at 13:43 +1000, Cyril Bur wrote: > Comment from arch/powerpc/kernel/process.c:967: > If userspace is inside a transaction (whether active or > suspended) and FP/VMX/VSX instructions have ever been enabled > inside that transaction, then we have to keep them enabled > and keep the FP/VMX/VSX state loaded while ever the transaction > continues. The reason is that if we didn't, and subsequently > got a FP/VMX/VSX unavailable interrupt inside a transaction, > we don't know whether it's the same transaction, and thus we > don't know which of the checkpointed state and the ransactional > state to use. > > restore_math() restore_fp() and restore_altivec() currently may not > restore the registers. It doesn't appear that this is more serious > than a performance penalty. If the math registers aren't restored the > userspace thread will still be run with the facility disabled. > Userspace will not be able to read invalid values. On the first access > it will take an facility unavailable exception and the kernel will > detected an active transaction, at which point it will abort the > transaction. There is the possibility for a pathological case > preventing any progress by transactions, however, transactions > are never guaranteed to make progress. > > Fixes: 70fe3d9 ("powerpc: Restore FPU/VEC/VSX if previously used") > Signed-off-by: Cyril Bur <cyrilbur@gmail.com> > --- > arch/powerpc/kernel/process.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kernel/process.c > b/arch/powerpc/kernel/process.c > index 58ccf86..cdf2d20 100644 > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -88,7 +88,13 @@ static void check_if_tm_restore_required(struct > task_struct *tsk) > set_thread_flag(TIF_RESTORE_TM); > } > } > + > +static inline bool msr_tm_active(unsigned long msr) > +{ > + return MSR_TM_ACTIVE(msr); > +} I'm not sure what value this function is adding. The MSR_TM_ACTIVE() is used in a lot of other places and is well known so I'd prefer to just keep using it, rather than adding some other abstraction that others have to learn. Other than that, the patch seems good. Mikey > #else > +static inline bool msr_tm_active(unsigned long msr) { return false; } > static inline void check_if_tm_restore_required(struct task_struct *tsk) > { } > #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */ > > @@ -208,7 +214,7 @@ void enable_kernel_fp(void) > EXPORT_SYMBOL(enable_kernel_fp); > > static int restore_fp(struct task_struct *tsk) { > - if (tsk->thread.load_fp) { > + if (tsk->thread.load_fp || msr_tm_active(tsk->thread.regs->msr)) > { > load_fp_state(¤t->thread.fp_state); > current->thread.load_fp++; > return 1; > @@ -278,7 +284,8 @@ EXPORT_SYMBOL_GPL(flush_altivec_to_thread); > > static int restore_altivec(struct task_struct *tsk) > { > - if (cpu_has_feature(CPU_FTR_ALTIVEC) && tsk->thread.load_vec) { > + if (cpu_has_feature(CPU_FTR_ALTIVEC) && > + (tsk->thread.load_vec || msr_tm_active(tsk->thread.regs- > >msr))) { > load_vr_state(&tsk->thread.vr_state); > tsk->thread.used_vr = 1; > tsk->thread.load_vec++; > @@ -464,7 +471,8 @@ void restore_math(struct pt_regs *regs) > { > unsigned long msr; > > - if (!current->thread.load_fp && !loadvec(current->thread)) > + if (!msr_tm_active(regs->msr) && > + !current->thread.load_fp && !loadvec(current->thread)) > return; > > msr = regs->msr; > @@ -983,6 +991,13 @@ void restore_tm_state(struct pt_regs *regs) > msr_diff = current->thread.ckpt_regs.msr & ~regs->msr; > msr_diff &= MSR_FP | MSR_VEC | MSR_VSX; > > + /* Ensure that restore_math() will restore */ > + if (msr_diff & MSR_FP) > + current->thread.load_fp = 1; > +#ifdef CONFIG_ALIVEC > + if (cpu_has_feature(CPU_FTR_ALTIVEC) && msr_diff & MSR_VEC) > + current->thread.load_vec = 1; > +#endif > restore_math(regs); > > regs->msr |= msr_diff;
On Fri, 2016-08-19 at 16:33 +1000, Michael Neuling wrote: > On Wed, 2016-08-17 at 13:43 +1000, Cyril Bur wrote: > > > > Comment from arch/powerpc/kernel/process.c:967: > > If userspace is inside a transaction (whether active or > > suspended) and FP/VMX/VSX instructions have ever been enabled > > inside that transaction, then we have to keep them enabled > > and keep the FP/VMX/VSX state loaded while ever the transaction > > continues. The reason is that if we didn't, and subsequently > > got a FP/VMX/VSX unavailable interrupt inside a transaction, > > we don't know whether it's the same transaction, and thus we > > don't know which of the checkpointed state and the ransactional > > state to use. > > > > restore_math() restore_fp() and restore_altivec() currently may not > > restore the registers. It doesn't appear that this is more serious > > than a performance penalty. If the math registers aren't restored > > the > > userspace thread will still be run with the facility disabled. > > Userspace will not be able to read invalid values. On the first > > access > > it will take an facility unavailable exception and the kernel will > > detected an active transaction, at which point it will abort the > > transaction. There is the possibility for a pathological case > > preventing any progress by transactions, however, transactions > > are never guaranteed to make progress. > > > > Fixes: 70fe3d9 ("powerpc: Restore FPU/VEC/VSX if previously used") > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com> > > --- > > arch/powerpc/kernel/process.c | 21 ++++++++++++++++++--- > > 1 file changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/arch/powerpc/kernel/process.c > > b/arch/powerpc/kernel/process.c > > index 58ccf86..cdf2d20 100644 > > --- a/arch/powerpc/kernel/process.c > > +++ b/arch/powerpc/kernel/process.c > > @@ -88,7 +88,13 @@ static void check_if_tm_restore_required(struct > > task_struct *tsk) > > set_thread_flag(TIF_RESTORE_TM); > > } > > } > > + > > +static inline bool msr_tm_active(unsigned long msr) > > +{ > > + return MSR_TM_ACTIVE(msr); > > +} > > I'm not sure what value this function is adding. The MSR_TM_ACTIVE() > is > used in a lot of other places and is well known so I'd prefer to just > keep > using it, rather than adding some other abstraction that others have > to > learn. > Admitedly right now it does seem silly, I want to add inlines to tm.h to replace the macros so that we can stop having #ifdef CONFIG_TM... everywhere and use the inlines which will work regardless of CONFIG_TM..., I was going to add that patch to this series but it got a bit long and I really just want to get this series finished. Happy to replace with with more MSR_TM_ACTIVE() with #ifdefs for now... Overall it is mostly a solution for the fact that I keep using these macros in 32bit and 64bit code and every time forget the #ifdef... > Other than that, the patch seems good. > > Mikey > > > > > #else > > +static inline bool msr_tm_active(unsigned long msr) { return > > false; } > > static inline void check_if_tm_restore_required(struct task_struct > > *tsk) > > { } > > #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */ > > > > @@ -208,7 +214,7 @@ void enable_kernel_fp(void) > > EXPORT_SYMBOL(enable_kernel_fp); > > > > static int restore_fp(struct task_struct *tsk) { > > - if (tsk->thread.load_fp) { > > + if (tsk->thread.load_fp || msr_tm_active(tsk->thread.regs- > > >msr)) > > { > > load_fp_state(¤t->thread.fp_state); > > current->thread.load_fp++; > > return 1; > > @@ -278,7 +284,8 @@ EXPORT_SYMBOL_GPL(flush_altivec_to_thread); > > > > static int restore_altivec(struct task_struct *tsk) > > { > > - if (cpu_has_feature(CPU_FTR_ALTIVEC) && tsk- > > >thread.load_vec) { > > + if (cpu_has_feature(CPU_FTR_ALTIVEC) && > > + (tsk->thread.load_vec || msr_tm_active(tsk- > > >thread.regs- > > > > > > msr))) { > > load_vr_state(&tsk->thread.vr_state); > > tsk->thread.used_vr = 1; > > tsk->thread.load_vec++; > > @@ -464,7 +471,8 @@ void restore_math(struct pt_regs *regs) > > { > > unsigned long msr; > > > > - if (!current->thread.load_fp && !loadvec(current->thread)) > > + if (!msr_tm_active(regs->msr) && > > + !current->thread.load_fp && !loadvec(current- > > >thread)) > > return; > > > > msr = regs->msr; > > @@ -983,6 +991,13 @@ void restore_tm_state(struct pt_regs *regs) > > msr_diff = current->thread.ckpt_regs.msr & ~regs->msr; > > msr_diff &= MSR_FP | MSR_VEC | MSR_VSX; > > > > + /* Ensure that restore_math() will restore */ > > + if (msr_diff & MSR_FP) > > + current->thread.load_fp = 1; > > +#ifdef CONFIG_ALIVEC > > + if (cpu_has_feature(CPU_FTR_ALTIVEC) && msr_diff & > > MSR_VEC) > > + current->thread.load_vec = 1; > > +#endif > > restore_math(regs); > > > > regs->msr |= msr_diff;
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 58ccf86..cdf2d20 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -88,7 +88,13 @@ static void check_if_tm_restore_required(struct task_struct *tsk) set_thread_flag(TIF_RESTORE_TM); } } + +static inline bool msr_tm_active(unsigned long msr) +{ + return MSR_TM_ACTIVE(msr); +} #else +static inline bool msr_tm_active(unsigned long msr) { return false; } static inline void check_if_tm_restore_required(struct task_struct *tsk) { } #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */ @@ -208,7 +214,7 @@ void enable_kernel_fp(void) EXPORT_SYMBOL(enable_kernel_fp); static int restore_fp(struct task_struct *tsk) { - if (tsk->thread.load_fp) { + if (tsk->thread.load_fp || msr_tm_active(tsk->thread.regs->msr)) { load_fp_state(¤t->thread.fp_state); current->thread.load_fp++; return 1; @@ -278,7 +284,8 @@ EXPORT_SYMBOL_GPL(flush_altivec_to_thread); static int restore_altivec(struct task_struct *tsk) { - if (cpu_has_feature(CPU_FTR_ALTIVEC) && tsk->thread.load_vec) { + if (cpu_has_feature(CPU_FTR_ALTIVEC) && + (tsk->thread.load_vec || msr_tm_active(tsk->thread.regs->msr))) { load_vr_state(&tsk->thread.vr_state); tsk->thread.used_vr = 1; tsk->thread.load_vec++; @@ -464,7 +471,8 @@ void restore_math(struct pt_regs *regs) { unsigned long msr; - if (!current->thread.load_fp && !loadvec(current->thread)) + if (!msr_tm_active(regs->msr) && + !current->thread.load_fp && !loadvec(current->thread)) return; msr = regs->msr; @@ -983,6 +991,13 @@ void restore_tm_state(struct pt_regs *regs) msr_diff = current->thread.ckpt_regs.msr & ~regs->msr; msr_diff &= MSR_FP | MSR_VEC | MSR_VSX; + /* Ensure that restore_math() will restore */ + if (msr_diff & MSR_FP) + current->thread.load_fp = 1; +#ifdef CONFIG_ALIVEC + if (cpu_has_feature(CPU_FTR_ALTIVEC) && msr_diff & MSR_VEC) + current->thread.load_vec = 1; +#endif restore_math(regs); regs->msr |= msr_diff;
Comment from arch/powerpc/kernel/process.c:967: If userspace is inside a transaction (whether active or suspended) and FP/VMX/VSX instructions have ever been enabled inside that transaction, then we have to keep them enabled and keep the FP/VMX/VSX state loaded while ever the transaction continues. The reason is that if we didn't, and subsequently got a FP/VMX/VSX unavailable interrupt inside a transaction, we don't know whether it's the same transaction, and thus we don't know which of the checkpointed state and the ransactional state to use. restore_math() restore_fp() and restore_altivec() currently may not restore the registers. It doesn't appear that this is more serious than a performance penalty. If the math registers aren't restored the userspace thread will still be run with the facility disabled. Userspace will not be able to read invalid values. On the first access it will take an facility unavailable exception and the kernel will detected an active transaction, at which point it will abort the transaction. There is the possibility for a pathological case preventing any progress by transactions, however, transactions are never guaranteed to make progress. Fixes: 70fe3d9 ("powerpc: Restore FPU/VEC/VSX if previously used") Signed-off-by: Cyril Bur <cyrilbur@gmail.com> --- arch/powerpc/kernel/process.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)