Re: [PATCH 01/14] powerpc/64s: idle move soft interrupt mask logic into C code

[Gautham R Shenoy <ego@linux.vnet.ibm.com>]

On Tue, Jun 13, 2017 at 12:46:02AM +1000, Nicholas Piggin wrote:
> Hi Gautham,
> 
> Thanks for the reviews.
> 
> On Mon, 12 Jun 2017 14:07:27 +0530
> Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> 
> > Hi Nick,
> > 
> > (Added Paul Mackerass to the Cc)
> > On Mon, Jun 12, 2017 at 09:58:22AM +1000, Nicholas Piggin wrote:
> > > This simplifies the asm and fixes irq-off tracing over sleep
> > > instructions.
> > > 
> > > Also move powersave_nap check for POWER8 into C code, and move
> > > PSSCR register value calculation for POWER9 into C.
> > >  
> > 
> > Thanks for doing this. Only one minor comment.
> > 
> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > ---
> > > index 98a6d07ecb5c..35cf5bb7daed 100644
> > > --- a/arch/powerpc/kernel/idle_book3s.S
> > > +++ b/arch/powerpc/kernel/idle_book3s.S  
> > 
> > [..snip..]
> > 
> > > @@ -109,13 +109,9 @@ core_idle_lock_held:
> > >  /*
> > >   * Pass requested state in r3:
> > >   *	r3 - PNV_THREAD_NAP/SLEEP/WINKLE in POWER8
> > > - *	   - Requested STOP state in POWER9
> > > + *	   - Requested PSSCR value in POWER9
> > >   *
> > > - * To check IRQ_HAPPENED in r4
> > > - * 	0 - don't check
> > > - * 	1 - check
> > > - *
> > > - * Address to 'rfid' to in r5
> > > + * Address of idle handler to 'rfid' to in r4
> > >   */
> > >  pnv_powersave_common:
> > >  	/* Use r3 to pass state nap/sleep/winkle */
> > > @@ -131,30 +127,7 @@ pnv_powersave_common:
> > >  	std	r0,_LINK(r1)
> > >  	std	r0,_NIP(r1)
> > > 
> > > -	/* Hard disable interrupts */
> > > -	mfmsr	r9
> > > -	rldicl	r9,r9,48,1
> > > -	rotldi	r9,r9,16
> > > -	mtmsrd	r9,1			/* hard-disable interrupts */
> > > -
> > > -	/* Check if something happened while soft-disabled */
> > > -	lbz	r0,PACAIRQHAPPENED(r13)
> > > -	andi.	r0,r0,~PACA_IRQ_HARD_DIS@l
> > > -	beq	1f
> > > -	cmpwi	cr0,r4,0  
> > 
> > There were callers to power7_nap() in the dynamic-core-split/unsplit
> > which wanted nap to be forced irrespective of whether an irq was
> > pending or not. With this new patch, there won't be a way to enforce
> > that on POWER8.
> 
> Actually there are two APIs to sleep now, the low level one which
> does not check lazy irq or nap disable is power7_idle_insn(). The
> one which sets up the lazy irq state is power7_idle_type(). The
> dynamic core split calls the former, so AFAIKS it should be
> equivalent.

Ah, I see. Yes, my bad. I didn't check which one we are calling in the
dynamic split case.

Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

> 
> Not the best names perhaps. Open to better suggestions.
> 
> Thanks,
> Nick
> 

--
Thanks and Regards
gautham.