[2/6] powernv:idle: Decouple Timebase restore & Per-core SPRs restore

Submitted by Gautham R. Shenoy on May 16, 2017, 8:49 a.m.

Details

Message ID d83df78bd8693f9a9814eb7b1d95537f663e47ea.1494585671.git.ego@linux.vnet.ibm.com
State Accepted
Commit ec4867355244755fb5c06037ad2fff760701b465
Headers show

Commit Message

Gautham R. Shenoy May 16, 2017, 8:49 a.m.
From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

On POWER8, in case of
   -  nap: both timebase and hypervisor state is retained.
   -  fast-sleep: timebase is lost. But the hypervisor state is retained.
   -  winkle: timebase and hypervisor state is lost.

Hence, the current code for handling exit from a idle state assumes
that if the timebase value is retained, then so is the hypervisor
state. Thus, the current code doesn't restore per-core hypervisor
state in such cases.

But that is no longer the case on POWER9 where we do have stop states
in which timebase value is retained, but the hypervisor state is
lost. So we have to ensure that the per-core hypervisor state gets
restored in such cases.

Fix this by ensuring that even in the case when timebase is retained,
we explicitly check if we are waking up from a deep stop that loses
per-core hypervisor state (indicated by cr4 being eq or gt), and if
this is the case, we restore the per-core hypervisor state.

Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/idle_book3s.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Nicholas Piggin May 30, 2017, 6:12 a.m.
On Tue, 16 May 2017 14:19:44 +0530
"Gautham R. Shenoy" <ego@linux.vnet.ibm.com> wrote:

> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> On POWER8, in case of
>    -  nap: both timebase and hypervisor state is retained.
>    -  fast-sleep: timebase is lost. But the hypervisor state is retained.
>    -  winkle: timebase and hypervisor state is lost.
> 
> Hence, the current code for handling exit from a idle state assumes
> that if the timebase value is retained, then so is the hypervisor
> state. Thus, the current code doesn't restore per-core hypervisor
> state in such cases.
> 
> But that is no longer the case on POWER9 where we do have stop states
> in which timebase value is retained, but the hypervisor state is
> lost. So we have to ensure that the per-core hypervisor state gets
> restored in such cases.
> 
> Fix this by ensuring that even in the case when timebase is retained,
> we explicitly check if we are waking up from a deep stop that loses
> per-core hypervisor state (indicated by cr4 being eq or gt), and if
> this is the case, we restore the per-core hypervisor state.
> 
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/idle_book3s.S | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> index 4898d67..afd029f 100644
> --- a/arch/powerpc/kernel/idle_book3s.S
> +++ b/arch/powerpc/kernel/idle_book3s.S
> @@ -731,13 +731,14 @@ timebase_resync:
>  	 * Use cr3 which indicates that we are waking up with atleast partial
>  	 * hypervisor state loss to determine if TIMEBASE RESYNC is needed.
>  	 */
> -	ble	cr3,clear_lock
> +	ble	cr3,.Ltb_resynced
>  	/* Time base re-sync */
>  	bl	opal_resync_timebase;
>  	/*
> -	 * If waking up from sleep, per core state is not lost, skip to
> -	 * clear_lock.
> +	 * If waking up from sleep (POWER8), per core state
> +	 * is not lost, skip to clear_lock.
>  	 */
> +.Ltb_resynced:
>  	blt	cr4,clear_lock
>  
>  	/*

It's more that timebase was not lost, rather than resynced.
Can we just do a 'bgtl cr3,opal_resync_timebase'?

I guess cr4 branch will never be true on POWER9... I think
pnv_wakeup_tb_loss is going to end up clearer being split
into two between isa 207 and 300. But that can wait until
after POWER9 is working properly.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Gautham R. Shenoy May 30, 2017, 10:28 a.m.
On Tue, May 30, 2017 at 04:12:38PM +1000, Nicholas Piggin wrote:
> On Tue, 16 May 2017 14:19:44 +0530
> "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> wrote:
> 
> > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> > 
> > On POWER8, in case of
> >    -  nap: both timebase and hypervisor state is retained.
> >    -  fast-sleep: timebase is lost. But the hypervisor state is retained.
> >    -  winkle: timebase and hypervisor state is lost.
> > 
> > Hence, the current code for handling exit from a idle state assumes
> > that if the timebase value is retained, then so is the hypervisor
> > state. Thus, the current code doesn't restore per-core hypervisor
> > state in such cases.
> > 
> > But that is no longer the case on POWER9 where we do have stop states
> > in which timebase value is retained, but the hypervisor state is
> > lost. So we have to ensure that the per-core hypervisor state gets
> > restored in such cases.
> > 
> > Fix this by ensuring that even in the case when timebase is retained,
> > we explicitly check if we are waking up from a deep stop that loses
> > per-core hypervisor state (indicated by cr4 being eq or gt), and if
> > this is the case, we restore the per-core hypervisor state.
> > 
> > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kernel/idle_book3s.S | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> > index 4898d67..afd029f 100644
> > --- a/arch/powerpc/kernel/idle_book3s.S
> > +++ b/arch/powerpc/kernel/idle_book3s.S
> > @@ -731,13 +731,14 @@ timebase_resync:
> >  	 * Use cr3 which indicates that we are waking up with atleast partial
> >  	 * hypervisor state loss to determine if TIMEBASE RESYNC is needed.
> >  	 */
> > -	ble	cr3,clear_lock
> > +	ble	cr3,.Ltb_resynced
> >  	/* Time base re-sync */
> >  	bl	opal_resync_timebase;
> >  	/*
> > -	 * If waking up from sleep, per core state is not lost, skip to
> > -	 * clear_lock.
> > +	 * If waking up from sleep (POWER8), per core state
> > +	 * is not lost, skip to clear_lock.
> >  	 */
> > +.Ltb_resynced:
> >  	blt	cr4,clear_lock
> >  
> >  	/*
> 
> It's more that timebase was not lost, rather than resynced.
> Can we just do a 'bgtl cr3,opal_resync_timebase'?

Yes, that's looks much better. I hadn't thought of bgtl. Will update
this.


> 
> I guess cr4 branch will never be true on POWER9... I think

Yes. Inside pnv_wakeup_tb_loss, cr4 will either have gt or
eq set. This branch applies only to POWER8 where cr4 would be eq only
on winkle, and not on fastsleep (a state that loses timebase but not
hypervisor state).


> pnv_wakeup_tb_loss is going to end up clearer being split
> into two between isa 207 and 300. But that can wait until
> after POWER9 is working properly.

Sure. 

> 
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> 

--
Thanks and Regards
gautham.

Patch hide | download patch | download mbox

diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index 4898d67..afd029f 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -731,13 +731,14 @@  timebase_resync:
 	 * Use cr3 which indicates that we are waking up with atleast partial
 	 * hypervisor state loss to determine if TIMEBASE RESYNC is needed.
 	 */
-	ble	cr3,clear_lock
+	ble	cr3,.Ltb_resynced
 	/* Time base re-sync */
 	bl	opal_resync_timebase;
 	/*
-	 * If waking up from sleep, per core state is not lost, skip to
-	 * clear_lock.
+	 * If waking up from sleep (POWER8), per core state
+	 * is not lost, skip to clear_lock.
 	 */
+.Ltb_resynced:
 	blt	cr4,clear_lock
 
 	/*