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 <cyril...@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;
