Re: [PATCH v3 4/4] powerpc/64s: idle ESL=0 stop can avoid MSR and save/restore overhead

[Nicholas Piggin <npiggin@gmail.com>]

On Tue, 29 Aug 2017 10:20:48 +1000
Paul Mackerras <paulus@ozlabs.org> wrote:

> On Fri, Aug 25, 2017 at 02:30:36PM +1000, Nicholas Piggin wrote:
> > When stop is executed with EC=ESL=0, it appears to execute like a
> > normal instruction (resuming from NIP when woken by interrupt).
> > So all the save/restore handling can be avoided completely. In
> > particular NV GPRs do not have to be saved, and MSR does not have
> > to be switched back to kernel MSR.
> > 
> > So move the test for "lite" sleep states out to power9_idle_stop.
> > 
> > Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  arch/powerpc/kernel/idle_book3s.S | 35 ++++++++++++++++++++++++-----------
> >  1 file changed, 24 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> > index 32d65ee323a0..fa56120bd0bc 100644
> > --- a/arch/powerpc/kernel/idle_book3s.S
> > +++ b/arch/powerpc/kernel/idle_book3s.S
> > @@ -315,9 +315,6 @@ enter_winkle:
> >  
> >  	ARCH207_IDLE_STATE_ENTER_SEQ_NORET(PPC_WINKLE)
> >  
> > -/*
> > - * r3 - PSSCR value corresponding to the requested stop state.
> > - */
> >  #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> >  power_enter_stop_kvm_rm:
> >  	/*
> > @@ -330,14 +327,11 @@ power_enter_stop_kvm_rm:
> >  	li	r4,KVM_HWTHREAD_IN_IDLE
> >  	/* DO THIS IN REAL MODE!  See comment above. */
> >  	stb	r4,HSTATE_HWTHREAD_STATE(r13)
> > -#endif
> > -power_enter_stop:
> >  /*
> >   * Check if we are executing the lite variant with ESL=EC=0
> >   */
> > -	andis.   r4,r3,PSSCR_EC_ESL_MASK_SHIFTED
> > -	clrldi   r3,r3,60 /* r3 = Bits[60:63] = Requested Level (RL) */
> > -	bne	 .Lhandle_esl_ec_set
> > +	andis.	r4,r3,PSSCR_EC_ESL_MASK_SHIFTED
> > +	bne	power_enter_stop_esl
> >  	PPC_STOP
> >  	li	r3,0  /* Since we didn't lose state, return 0 */
> >  
> > @@ -354,8 +348,13 @@ power_enter_stop:
> >  	 */
> >  	li	r12, 0
> >  	b 	pnv_wakeup_noloss
> > +#endif
> >  
> > -.Lhandle_esl_ec_set:
> > +/*
> > + * r3 - PSSCR value corresponding to the requested stop state.
> > + */
> > +power_enter_stop_esl:
> > +	clrldi   r3,r3,60 /* r3 = Bits[60:63] = Requested Level (RL) */
> >  	/*
> >  	 * POWER9 DD2 can incorrectly set PMAO when waking up after a
> >  	 * state-loss idle. Saving and restoring MMCR0 over idle is a
> > @@ -428,9 +427,23 @@ ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66);		\
> >   * r3 contains desired PSSCR register value.
> >   */
> >  _GLOBAL(power9_idle_stop)
> > -	std	r3, PACA_REQ_PSSCR(r13)
> >  	mtspr 	SPRN_PSSCR,r3
> > -	LOAD_REG_ADDR(r4,power_enter_stop)
> > +
> > +	/*
> > +	 * Check if we are executing the lite variant with ESL=EC=0
> > +	 * This case resumes execution after the stop instruction without
> > +	 * losing any state, so nothing has to be saved. The following
> > +	 * instructions up to the blr must be skipped if we want to
> > +	 * use power_enter_stop_kvm_rm.
> > +	 */
> > +	andis.	r4,r3,PSSCR_EC_ESL_MASK_SHIFTED  
> 
> I realize you're just moving existing code, but I think this would be
> clearer (to me, anyway) as
> 
> 	andis.	r4, r3, (PSSCR_EC | PSSCR_ESL)@h

Agreed, it's not a very helpful extra indirection. Perhaps we could
do a cleanup patch to fix that (and your other points like the 206/207
confusion).

Thanks for the reviews.

> Apart from that very minor nit,
> 
> Reviewed-by: Paul Mackerras <paulus@ozlabs.org>