| Message ID | 27511c1235ee58c66eabdea65b39f5dd35d91426.1491996797.git.ego@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, 2017-04-12 at 17:16 +0530, Gautham R. Shenoy wrote: > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > The idle-exit code assumes that if Timebase is not lost, then neither > are the per-core hypervisor resources lost. Double negative! How about: The idle-exit code assumes that if the timebase is restored, then the per-core hypervisor resources are also restored. > This was true on POWER8 > where fast-sleep lost only TB but not per-core resources, and winkle > lost both. > > This assumption is not true for POWER9 however, since there can be > states which do not lose timebase but can lose per-core SPRs. > > Hence check if we need to restore the per-core hypervisor state even > if timebase is not lost. I think I understand what you're doing, just seems awkwardly worded. Is this actually what the patch is doing? It seem to be just changing one branch. Mikey > > 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 9b747e9..6a9bd28 100644 > --- a/arch/powerpc/kernel/idle_book3s.S > +++ b/arch/powerpc/kernel/idle_book3s.S > @@ -723,13 +723,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 > > /*
On Thu, Apr 13, 2017 at 04:55:45PM +1000, Michael Neuling wrote: > On Wed, 2017-04-12 at 17:16 +0530, Gautham R. Shenoy wrote: > > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > > > The idle-exit code assumes that if Timebase is not lost, then neither > > are the per-core hypervisor resources lost. > > Double negative! How about: > > The idle-exit code assumes that if the timebase is restored, then the > per-core hypervisor resources are also restored. Ok. To be more explicit, this was the intention: 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 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 if this is the case. But that is no longer true on POWER9 where we do have stop states where 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. > > > This was true on POWER8 > > where fast-sleep lost only TB but not per-core resources, and winkle > > lost both. > > > > This assumption is not true for POWER9 however, since there can be > > states which do not lose timebase but can lose per-core SPRs. > > > > Hence check if we need to restore the per-core hypervisor state even > > if timebase is not lost. > > I think I understand what you're doing, just seems awkwardly worded. Will fix the wording. > > Is this actually what the patch is doing? It seem to be just changing one > branch. > Yes, we change the branch to .Ltb_resynced which will ensure 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). > Mikey > > > > > 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 9b747e9..6a9bd28 100644 > > --- a/arch/powerpc/kernel/idle_book3s.S > > +++ b/arch/powerpc/kernel/idle_book3s.S > > @@ -723,13 +723,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 > > > > /*
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S index 9b747e9..6a9bd28 100644 --- a/arch/powerpc/kernel/idle_book3s.S +++ b/arch/powerpc/kernel/idle_book3s.S @@ -723,13 +723,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 /*