Re: [RFC PATCH 10/12] [WIP] powerpc/tm: Correctly save/restore checkpointed sprs

[Michael Neuling <mikey@neuling.org>]

On Tue, 2018-02-20 at 14:59 +1100, Cyril Bur wrote:
> On Tue, 2018-02-20 at 14:00 +1100, Michael Neuling wrote:
> > This needs a description of what you're trying to do.  "Correctly" does=
n't
> > really mean anything.
> >=20
> >=20
> > On Tue, 2018-02-20 at 11:22 +1100, Cyril Bur wrote:
> > > ---
> > >  arch/powerpc/kernel/process.c | 57 +++++++++++++++++++++++++++++++++=
++++++++-
> > > -
> > >  arch/powerpc/kernel/ptrace.c  |  9 +++----
> > >  2 files changed, 58 insertions(+), 8 deletions(-)
> > >=20
> > > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/proc=
ess.c
> > > index cd3ae80a6878..674f75c56172 100644
> > > --- a/arch/powerpc/kernel/process.c
> > > +++ b/arch/powerpc/kernel/process.c
> > > @@ -859,6 +859,8 @@ static inline bool tm_enabled(struct task_struct =
*tsk)
> > >  	return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
> > >  }
> > > =20
> > > +static inline void save_sprs(struct thread_struct *t);
> > > +
> > >  static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cau=
se)
> > >  {
> > >  	/*
> > > @@ -879,6 +881,8 @@ static void tm_reclaim_thread(struct thread_struc=
t *thr,
> > > uint8_t cause)
> > >  	if (!MSR_TM_SUSPENDED(mfmsr()))
> > >  		return;
> > > =20
> > > +	save_sprs(thr);
> > > +
> > >  	giveup_all(container_of(thr, struct task_struct, thread));
> > > =20
> > >  	tm_reclaim(thr, cause);
> > > @@ -991,6 +995,37 @@ void tm_recheckpoint(struct thread_struct *threa=
d)
> > > =20
> > >  	__tm_recheckpoint(thread);
> > > =20
> > > +	/*
> > > +	 * This is a stripped down restore_sprs(), we need to do this
> > > +	 * now as we might go straight out to userspace and currently
> > > +	 * the checkpointed values are on the CPU.
> > > +	 *
> > > +	 * TODO: Improve
> > > +	 */
> > > +#ifdef CONFIG_ALTIVEC
> > > +	if (cpu_has_feature(CPU_FTR_ALTIVEC))
> > > +		mtspr(SPRN_VRSAVE, thread->vrsave);
> > > +#endif
> > > +#ifdef CONFIG_PPC_BOOK3S_64
> > > +	if (cpu_has_feature(CPU_FTR_DSCR)) {
> > > +		u64 dscr =3D get_paca()->dscr_default;
> > > +		if (thread->dscr_inherit)
> > > +			dscr =3D thread->dscr;
> > > +
> > > +		mtspr(SPRN_DSCR, dscr);
> > > +	}
> > > +
> > > +	if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
> > > +		/* The EBB regs aren't checkpointed */
> > > +		mtspr(SPRN_FSCR, thread->fscr);
> > > +
> > > +		mtspr(SPRN_TAR, thread->tar);
> > > +	}
> > > +
> > > +	/* I think we don't need to */
> > > +	if (cpu_has_feature(CPU_FTR_ARCH_300))
> > > +		mtspr(SPRN_TIDR, thread->tidr);
> > > +#endif
> >=20
> > Why are you touching all the above hunk?
>=20
> I copied restore_sprs. I'm tidying that up now - we can't call
> restore_sprs because we don't have a prev and next thread.

Yeah needs to be tided up... we can't have another copy of the code.. obvio=
usly.

>=20
> >=20
> > >  	local_irq_restore(flags);
> > >  }
> > > =20
> > > @@ -1193,6 +1228,11 @@ struct task_struct *__switch_to(struct task_st=
ruct
> > > *prev,
> > >  #endif
> > > =20
> > >  	new_thread =3D &new->thread;
> > > +	/*
> > > +	 * Why not &prev->thread; ?
> > > +	 * What is the difference between &prev->thread and
> > > +	 * &current->thread ?
> > > +	 */
> >=20
> > Why not just work it out and FIX THE CODE, rather than just rabbiting o=
n about
> > it! :-P
>=20
> Agreed - I started to and then had a mini freakout that things would
> end really badly if they're not the same. So I left that comment as a
> reminder to investigate.
>=20
> They should be the same though right?

Should be if prev =3D=3D current.

Mikey