[v2 PATCH 4/4] powernv: Recover correct PACA on wakeup from a stop on P9 DD1

[Gautham R Shenoy]

On Tue, Mar 21, 2017 at 02:59:46AM +1000, Nicholas Piggin wrote:
> On Mon, 20 Mar 2017 21:24:18 +0530
> "Gautham R. Shenoy" <ego at linux.vnet.ibm.com> wrote:
> 
> > From: "Gautham R. Shenoy" <ego at linux.vnet.ibm.com>
> > 
> > POWER9 DD1.0 hardware has an issue due to which the SPRs of a thread
> > waking up from stop 0,1,2 with ESL=1 can endup being misplaced in the
> > core. Thus the HSPRG0 of a thread waking up from can contain the paca
> > pointer of its sibling.
> > 
> > This patch implements a context recovery framework within threads of a
> > core, by provisioning space in paca_struct for saving every sibling
> > threads's paca pointers. Basically, we should be able to arrive at the
> > right paca pointer from any of the thread's existing paca pointer.
> > 
> > At bootup, during powernv idle-init, we save the paca address of every
> > CPU in each one its siblings paca_struct in the slot corresponding to
> > this CPU's index in the core.
> > 
> > On wakeup from a stop, the thread will determine its index in the core
> > from the lower 2 bits of the PIR register and recover its PACA pointer
> > by indexing into the correct slot in the provisioned space in the
> > current PACA.
> > 
> > Furthermore, ensure that the NVGPRs are restored from the stack on the
> > way out by setting the NAPSTATELOST in paca.
> 
> Thanks for expanding on this, it makes the patch easier to follow :)
> 
> As noted before, I think if we use PACA_EXNMI for system reset, then
> *hopefully* there should be minimal races with the initial use of other
> thread's PACA at the start of the exception. So I'll work on getting
> that in, but it need not prevent this patch from being merged first
> IMO.
> 
> > [Changelog written with inputs from svaidy at linux.vnet.ibm.com]
> > 
> > Signed-off-by: Gautham R. Shenoy <ego at linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/include/asm/paca.h       |  5 ++++
> >  arch/powerpc/kernel/asm-offsets.c     |  1 +
> >  arch/powerpc/kernel/idle_book3s.S     | 49 ++++++++++++++++++++++++++++++++++-
> >  arch/powerpc/platforms/powernv/idle.c | 22 ++++++++++++++++
> >  4 files changed, 76 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> > index 708c3e5..4405630 100644
> > --- a/arch/powerpc/include/asm/paca.h
> > +++ b/arch/powerpc/include/asm/paca.h
> > @@ -172,6 +172,11 @@ struct paca_struct {
> >  	u8 thread_mask;
> >  	/* Mask to denote subcore sibling threads */
> >  	u8 subcore_sibling_mask;
> > +	/*
> > +	 * Pointer to an array which contains pointer
> > +	 * to the sibling threads' paca.
> > +	 */
> > +	struct paca_struct *thread_sibling_pacas[8];

> 
> Is 8 the right number? I wonder if we have a define for it.

Thats the maximum number of threads per core that we have had on POWER
so far.

Perhaps, I can make this

	 struct paca_struct **thread_sibling_pacas;

and allocate threads_per_core number of slots in
pnv_init_idle_states. Sounds ok ?


> 
> >  #endif
> >  
> >  #ifdef CONFIG_PPC_BOOK3S_64
> > diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> > index 4367e7d..6ec5016 100644
> > --- a/arch/powerpc/kernel/asm-offsets.c
> > +++ b/arch/powerpc/kernel/asm-offsets.c
> > @@ -727,6 +727,7 @@ int main(void)
> >  	OFFSET(PACA_THREAD_IDLE_STATE, paca_struct, thread_idle_state);
> >  	OFFSET(PACA_THREAD_MASK, paca_struct, thread_mask);
> >  	OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
> > +	OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
> >  #endif
> >  
> >  	DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER);
> > diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> > index 9957287..c2f2cfb 100644
> > --- a/arch/powerpc/kernel/idle_book3s.S
> > +++ b/arch/powerpc/kernel/idle_book3s.S
> > @@ -375,6 +375,46 @@ _GLOBAL(power9_idle_stop)
> >  	li	r4,1
> >  	b	pnv_powersave_common
> >  	/* No return */
> > +
> > +
> > +/*
> > + * On waking up from stop 0,1,2 with ESL=1 on POWER9 DD1,
> > + * HSPRG0 will be set to the HSPRG0 value of one of the
> > + * threads in this core. Thus the value we have in r13
> > + * may not be this thread's paca pointer.
> > + *
> > + * Fortunately, the PIR remains invariant. Since this thread's
> > + * paca pointer is recorded in all its sibling's paca, we can
> > + * correctly recover this thread's paca pointer if we
> > + * know the index of this thread in the core.
> > + *
> > + * This index can be obtained from the lower two bits of the PIR.
> > + *
> > + * i.e, thread's position in the core = PIR[62:63].
> > + * If this value is i, then this thread's paca is
> > + * paca->thread_sibling_pacas[i].
> > + */
> > +power9_dd1_recover_paca:
> > +	mfspr	r4, SPRN_PIR
> > +	clrldi	r4, r4, 62
> 
> Does SPRN_TIR work?

I wasn't aware of SPRN_TIR!

I can check this. If my reading of the ISA is correct, TIR should
contain the thread number which are in the range [0..3].

> 
> > +	/*
> > +	 * Since each entry in thread_sibling_pacas is 8 bytes
> > +	 * we need to left-shift by 3 bits. Thus r4 = i * 8
> > +	 */
> > +	sldi	r4, r4, 3
> > +	/* Get &paca->thread_sibling_pacas[0] in r5 */
> > +	addi	r5, r13, PACA_SIBLING_PACA_PTRS
> > +	/* Load paca->thread_sibling_pacas[i] into r13 */
> > +	ldx	r13, r4, r5
> > +	SET_PACA(r13)
> > +	/*
> > +	 * Indicate that we have lost NVGPR state
> > +	 * which needs to be restored from the stack.
> > +	 */
> > +	li	r3, 1
> > +	stb	r0,PACA_NAPSTATELOST(r13)
> > +	blr
> > +
> >  /*
> >   * Called from reset vector. Check whether we have woken up with
> >   * hypervisor state loss. If yes, restore hypervisor state and return
> > @@ -385,7 +425,14 @@ _GLOBAL(power9_idle_stop)
> >   */
> >  _GLOBAL(pnv_restore_hyp_resource)
> >  BEGIN_FTR_SECTION
> > -	ld	r2,PACATOC(r13);
> > +BEGIN_FTR_SECTION_NESTED(70)
> > +	mflr 	r6
> > +	bl	power9_dd1_recover_paca
> > +	mtlr	r6
> > +	ld	r2, PACATOC(r13)
> > +FTR_SECTION_ELSE_NESTED(70)
> > +	ld	r2, PACATOC(r13)
> > +ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_POWER9_DD1, 70)
> 
> This is quite neat now you've moved it to its own function. Nice.
> It will be only a trivial clash with my patches now, I think.

Sure!
> 
> Reviewed-by: Nicholas Piggin <npiggin at gmail.com>
> 

Thanks for reviewing the patch.

--
Thanks and Regards
gautham.