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

[Nicholas Piggin]

On Wed, 22 Mar 2017 11:28:46 +0530
Gautham R Shenoy <ego@xxxxxxxxxxxxxxxxxx> wrote:

> 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@xxxxxxxxxxxxxxxxxx> wrote:
> >   
> > > From: "Gautham R. Shenoy" <ego@xxxxxxxxxxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxxxxxxxxx]
> > > 
> > > Signed-off-by: Gautham R. Shenoy <ego@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >  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 ?

I guess that would minimise PACA overhead for non-DD1 machines,
so if it's not too much trouble, that might be good.


> > > +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].

Yep.


> > Reviewed-by: Nicholas Piggin <npiggin@xxxxxxxxx>
> >   
> 
> Thanks for reviewing the patch.

No problems. Don't worry about the machine check wakeup for the moment
either. It's more important to just get the normal wakeup fix in I think.
We can revisit what to do there after my machine check patches go in
(idle machine check does not really work right now for POWER9 anyway).

Thanks,
Nick