From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-14 09:24:03
Hi,
This is a resend of the previous idle series, with the addition that
I accounted for Gautham's feedback, and also re-introduced the feature
to avoid full state restore by counting winkles rather than special
HSPRG0 bit.
The two big things we get from this, is no longer messing with HSPRG0
in the idle wakeup path on POWER8, and not crashing on POWER9 machine
check from power saving mode (tested in mambo according to ISA specs,
but have not triggered it on real hardware).
I only added the reviewed-by for patches which were not significantly
chaged since last time.
Thanks,
Nick
Nicholas Piggin (8):
powerpc/64s: move remaining system reset idle code into idle_book3s.S
powerpc/64s: stop using bit in HSPRG0 to test winkle
powerpc/64s: use alternative feature patching
powerpc/64s: fix POWER9 machine check handler from stop state
powerpc/64s: use PACA_THREAD_IDLE_STATE only in POWER8
powerpc/64s: idle expand usable core idle state bits
powerpc/64s: idle do not hold reservation longer than required
powerpc/64s: idle POWER8 avoid full state loss recovery when possible
arch/powerpc/include/asm/cpuidle.h | 32 +++++-
arch/powerpc/include/asm/exception-64s.h | 13 +--
arch/powerpc/include/asm/paca.h | 12 +-
arch/powerpc/include/asm/reg.h | 1 +
arch/powerpc/kernel/exceptions-64s.S | 112 ++++++------------
arch/powerpc/kernel/idle_book3s.S | 189 ++++++++++++++++++++-----------
arch/powerpc/platforms/powernv/idle.c | 13 ---
7 files changed, 202 insertions(+), 170 deletions(-)
--
2.11.0
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-14 09:24:12
The POWER8 idle code has a neat trick of programming the power on engine
to restore a low bit into HSPRG0, so idle wakeup code can test and see
if it has been programmed this way and therefore lost all state, and
avoiding the expensive full restore if not.
However this messes with our r13 PACA pointer, and requires HSPRG0 to
be written to throughout the exception handlers and idle wakeup, rather
than just once on kernel entry.
Remove this complexity and assume winkle sleeps always require a state
restore. This speedup is later re-introduced by counting per-core winkles
and setting a bitmap of threads with state loss when all are in winkle.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/exception-64s.h | 13 ++-----------
arch/powerpc/kernel/exceptions-64s.S | 21 +++------------------
arch/powerpc/kernel/idle_book3s.S | 23 ++++++++---------------
arch/powerpc/platforms/powernv/idle.c | 13 -------------
4 files changed, 13 insertions(+), 57 deletions(-)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-14 09:24:16
This reduces the number of nops for POWER8.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/idle_book3s.S | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-14 09:24:19
The ISA specifies power save wakeup can cause a machine check interrupt.
The machine check handler currently has code to handle that for POWER8,
but POWER9 crashes when trying to execute the P8 style sleep
instructions.
So queue up the machine check, then call into the idle code to wake up
as the system reset interrupt does, rather than attempting to sleep
again without going through the main idle path.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/reg.h | 1 +
arch/powerpc/kernel/exceptions-64s.S | 69 ++++++++++++++++++------------------
2 files changed, 35 insertions(+), 35 deletions(-)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-14 09:24:24
POWER9 does not use this field, so it should be moved into the POWER8
code. Update the documentation in the paca struct too.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/paca.h | 12 ++++++++++--
arch/powerpc/kernel/idle_book3s.S | 13 +++++++------
2 files changed, 17 insertions(+), 8 deletions(-)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-14 09:24:27
In preparation for adding more bits to the core idle state word,
move the lock bit up, and unlock by flipping the lock bit rather
than masking off all but the thread bits.
Add branch hints for atomic operations while we're here.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/cpuidle.h | 4 ++--
arch/powerpc/kernel/idle_book3s.S | 33 +++++++++++++++++----------------
2 files changed, 19 insertions(+), 18 deletions(-)
@@ -241,7 +241,7 @@ common_enter: /* common code for all the threads entering sleep or winkle */IDLE_STATE_ENTER_SEQ_NORET(PPC_SLEEP)fastsleep_workaround_at_entry:-orir15,r15,PNV_CORE_IDLE_LOCK_BIT+orisr15,r15,PNV_CORE_IDLE_LOCK_BIT@hstwcx.r15,0,r14bne-lwarx_loop1isync
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-14 09:24:30
When taking the core idle state lock, grab it immediately like a
regular lock, rather than adding more tests in there. Holding the lock
keeps it stable, so there is no need to do it whole holding the
reservation.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/idle_book3s.S | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-14 09:24:34
If not all threads were in winkle, full state loss recovery is not
necessary and can be avoided. A previous patch removed this optimisation
due to some complexity with the implementation. Re-implement it by
counting the number of threads in winkle with the per-core idle state.
Only restore full state loss if all threads were in winkle.
This has a small window of false positives right before threads execute
winkle and just after they wake up, when the winkle count does not
reflect the true number of threads in winkle. This is not a significant
problem in comparison with even the minimum winkle duration. For
correctness, a false positive is not a problem (only false negatives
would be).
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/cpuidle.h | 32 ++++++++++++++++++++++++---
arch/powerpc/kernel/idle_book3s.S | 45 +++++++++++++++++++++++++++++++++-----
2 files changed, 68 insertions(+), 9 deletions(-)
From: Gautham R Shenoy <hidden> Date: 2017-03-16 11:14:46
Hi Nick,
On Tue, Mar 14, 2017 at 07:23:43PM +1000, Nicholas Piggin wrote:
The POWER8 idle code has a neat trick of programming the power on engine
to restore a low bit into HSPRG0, so idle wakeup code can test and see
if it has been programmed this way and therefore lost all state, and
avoiding the expensive full restore if not.
However this messes with our r13 PACA pointer, and requires HSPRG0 to
be written to throughout the exception handlers and idle wakeup, rather
than just once on kernel entry.
Remove this complexity and assume winkle sleeps always require a state
restore. This speedup is later re-introduced by counting per-core winkles
and setting a bitmap of threads with state loss when all are in winkle.
Looks good to me.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Gautham R. Shenoy <redacted>
--
Thanks and Regards
gautham.
From: Gautham R Shenoy <hidden> Date: 2017-03-16 11:54:15
Hi Nick,
On Tue, Mar 14, 2017 at 07:23:46PM +1000, Nicholas Piggin wrote:
quoted hunk
POWER9 does not use this field, so it should be moved into the POWER8
code. Update the documentation in the paca struct too.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/paca.h | 12 ++++++++++--
arch/powerpc/kernel/idle_book3s.S | 13 +++++++------
2 files changed, 17 insertions(+), 8 deletions(-)
@@ -165,11 +165,19 @@ struct paca_struct {#endif#ifdef CONFIG_PPC_POWERNV-/* Per-core mask tracking idle threads and a lock bit-[L][TTTTTTTT] */+/* CPU idle fields */++/*+*Per-corewordusedtosynchronizebetweenthreads.See+*asm/cpuidle.h,PNV_CORE_IDLE_*+*/u32*core_idle_state_ptr;-u8thread_idle_state;/* PNV_THREAD_RUNNING/NAP/SLEEP *//* Mask to indicate thread id in core */u8thread_mask;++/* POWER8 specific fields */+/* PNV_THREAD_RUNNING/NAP/SLEEP */+u8thread_idle_state;
I am planning to use this in POWER9 DD1 to distinguish between a
SRESET received when the thread was running vs when it was in stop.
Unfortunately the SRR1[46:47] are not cleared in the former case. So
we need a way in software to distinguish between the two.
From: Gautham R Shenoy <hidden> Date: 2017-03-16 12:11:04
Hi Nick,
On Tue, Mar 14, 2017 at 07:23:47PM +1000, Nicholas Piggin wrote:
In preparation for adding more bits to the core idle state word,
move the lock bit up, and unlock by flipping the lock bit rather
than masking off all but the thread bits.
Add branch hints for atomic operations while we're here.
Looks good.
Reviewed-by: Gautham R. Shenoy <redacted>
@@ -241,7 +241,7 @@ common_enter: /* common code for all the threads entering sleep or winkle */IDLE_STATE_ENTER_SEQ_NORET(PPC_SLEEP)fastsleep_workaround_at_entry:-orir15,r15,PNV_CORE_IDLE_LOCK_BIT+orisr15,r15,PNV_CORE_IDLE_LOCK_BIT@hstwcx.r15,0,r14bne-lwarx_loop1isync
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-16 12:16:53
On Thu, 16 Mar 2017 17:24:03 +0530
Gautham R Shenoy [off-list ref] wrote:
Hi Nick,
On Tue, Mar 14, 2017 at 07:23:46PM +1000, Nicholas Piggin wrote:
quoted
POWER9 does not use this field, so it should be moved into the POWER8
code. Update the documentation in the paca struct too.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/paca.h | 12 ++++++++++--
arch/powerpc/kernel/idle_book3s.S | 13 +++++++------
2 files changed, 17 insertions(+), 8 deletions(-)
@@ -165,11 +165,19 @@ struct paca_struct {#endif#ifdef CONFIG_PPC_POWERNV-/* Per-core mask tracking idle threads and a lock bit-[L][TTTTTTTT] */+/* CPU idle fields */++/*+*Per-corewordusedtosynchronizebetweenthreads.See+*asm/cpuidle.h,PNV_CORE_IDLE_*+*/u32*core_idle_state_ptr;-u8thread_idle_state;/* PNV_THREAD_RUNNING/NAP/SLEEP *//* Mask to indicate thread id in core */u8thread_mask;++/* POWER8 specific fields */+/* PNV_THREAD_RUNNING/NAP/SLEEP */+u8thread_idle_state;
I am planning to use this in POWER9 DD1 to distinguish between a
SRESET received when the thread was running vs when it was in stop.
Unfortunately the SRR1[46:47] are not cleared in the former case. So
we need a way in software to distinguish between the two.
Okay, we can skip this for now. It was not a critical part of my
patches, just a tidy up.
Thanks,
Nick
The ISA specifies power save wakeup can cause a machine check interrupt.
The machine check handler currently has code to handle that for POWER8,
but POWER9 crashes when trying to execute the P8 style sleep
instructions.
So queue up the machine check, then call into the idle code to wake up
as the system reset interrupt does, rather than attempting to sleep
again without going through the main idle path.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/reg.h | 1 +
arch/powerpc/kernel/exceptions-64s.S | 69 ++++++++++++++++++------------------
2 files changed, 35 insertions(+), 35 deletions(-)
Looks like we are not winding up.. Shouldn't we ? What if we may end up
in pnv_wakeup_noloss() which assumes that no GPRs are lost. Am I missing
anything ?
+ b pnv_powersave_wakeup
+#endif
/*
[...]
Rest looks good to me.
Reviewed-by: Mahesh J Salgaonkar <redacted>
Thanks,
-Mahesh.
From: Gautham R Shenoy <hidden> Date: 2017-03-16 12:43:44
Hi Nick,
On Tue, Mar 14, 2017 at 07:23:48PM +1000, Nicholas Piggin wrote:
When taking the core idle state lock, grab it immediately like a
regular lock, rather than adding more tests in there. Holding the lock
keeps it stable, so there is no need to do it whole holding the
reservation.
Is reversing the order of loads into r7 and r14 intentional?
Other than that,
Reviewed-by: Gautham R. Shenoy <redacted>
quoted hunk
+
/*
+ * Take the core lock to synchronize against other threads.
+ *
* Lock bit is set in one of the 2 cases-
* a. In the sleep/winkle enter path, the last thread is executing
* fastsleep workaround code.
@@ -501,7 +501,14 @@ lwarx_loop2: * workaround undo code or resyncing timebase or restoring context * In either case loop until the lock bit is cleared. */+1:+ lwarx r15,0,r14+ andis. r9,r15,PNV_CORE_IDLE_LOCK_BIT@h bnel- core_idle_lock_held+ oris r15,r15,PNV_CORE_IDLE_LOCK_BIT@h+ stwcx. r15,0,r14+ bne- 1b+ isync andi. r9,r15,PNV_CORE_IDLE_THREAD_BITS cmpwi cr2,r9,0
@@ -513,11 +520,6 @@ lwarx_loop2: * cr4 - gt or eq if waking up from complete hypervisor state loss. */- oris r15,r15,PNV_CORE_IDLE_LOCK_BIT@h- stwcx. r15,0,r14- bne- lwarx_loop2- isync- BEGIN_FTR_SECTION lbz r4,PACA_SUBCORE_SIBLING_MASK(r13) and r4,r4,r15
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-16 12:55:34
On Thu, 16 Mar 2017 18:13:28 +0530
Gautham R Shenoy [off-list ref] wrote:
Hi Nick,
On Tue, Mar 14, 2017 at 07:23:48PM +1000, Nicholas Piggin wrote:
quoted
When taking the core idle state lock, grab it immediately like a
regular lock, rather than adding more tests in there. Holding the lock
keeps it stable, so there is no need to do it whole holding the
reservation.
Is reversing the order of loads into r7 and r14 intentional?
Oh, yes I guess it is because we use r14 result first. I should have
mentioned it but I forgot about it. Probably they decode together,
but you might get them in different cycles.
Thanks for the review!
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-16 13:05:34
On Thu, 16 Mar 2017 18:10:48 +0530
Mahesh Jagannath Salgaonkar [off-list ref] wrote:
On 03/14/2017 02:53 PM, Nicholas Piggin wrote:
quoted
The ISA specifies power save wakeup can cause a machine check interrupt.
The machine check handler currently has code to handle that for POWER8,
but POWER9 crashes when trying to execute the P8 style sleep
instructions.
So queue up the machine check, then call into the idle code to wake up
as the system reset interrupt does, rather than attempting to sleep
again without going through the main idle path.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/reg.h | 1 +
arch/powerpc/kernel/exceptions-64s.S | 69 ++++++++++++++++++------------------
2 files changed, 35 insertions(+), 35 deletions(-)
Looks like we are not winding up.. Shouldn't we ? What if we may end up
in pnv_wakeup_noloss() which assumes that no GPRs are lost. Am I missing
anything ?
Hmm, no I think you're right. Thanks, good catch. But can we do it with
just setting PACA_NAPSTATELOST?
quoted
+ b pnv_powersave_wakeup
+#endif
/*
[...]
Rest looks good to me.
Reviewed-by: Mahesh J Salgaonkar <redacted>
From: Gautham R Shenoy <hidden> Date: 2017-03-16 13:19:20
Hi,
On Thu, Mar 16, 2017 at 11:05:20PM +1000, Nicholas Piggin wrote:
On Thu, 16 Mar 2017 18:10:48 +0530
Mahesh Jagannath Salgaonkar [off-list ref] wrote:
quoted
On 03/14/2017 02:53 PM, Nicholas Piggin wrote:
quoted
The ISA specifies power save wakeup can cause a machine check interrupt.
The machine check handler currently has code to handle that for POWER8,
but POWER9 crashes when trying to execute the P8 style sleep
instructions.
So queue up the machine check, then call into the idle code to wake up
as the system reset interrupt does, rather than attempting to sleep
again without going through the main idle path.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/reg.h | 1 +
arch/powerpc/kernel/exceptions-64s.S | 69 ++++++++++++++++++------------------
2 files changed, 35 insertions(+), 35 deletions(-)
Looks like we are not winding up.. Shouldn't we ? What if we may end up
in pnv_wakeup_noloss() which assumes that no GPRs are lost. Am I missing
anything ?
Nice catch! This can occur if SRR1[46:47] == 0b01.
Hmm, no I think you're right. Thanks, good catch. But can we do it with
just setting PACA_NAPSTATELOST?
Unconditionally setting PACA_NAPSTATELOST should be sufficient.
quoted
quoted
+ b pnv_powersave_wakeup
+#endif
/*
[...]
Rest looks good to me.
Reviewed-by: Mahesh J Salgaonkar <redacted>
From: Gautham R Shenoy <hidden> Date: 2017-03-16 16:12:19
Hi Nick,
On Tue, Mar 14, 2017 at 07:23:49PM +1000, Nicholas Piggin wrote:
If not all threads were in winkle, full state loss recovery is not
necessary and can be avoided. A previous patch removed this optimisation
due to some complexity with the implementation. Re-implement it by
counting the number of threads in winkle with the per-core idle state.
Only restore full state loss if all threads were in winkle.
This has a small window of false positives right before threads execute
winkle and just after they wake up, when the winkle count does not
reflect the true number of threads in winkle. This is not a significant
problem in comparison with even the minimum winkle duration. For
correctness, a false positive is not a problem (only false negatives
would be).
For ISA 300, we need to restore hypervisor thread resources if we are
waking up a state that is as deep or deeper than
pnv_first_deep_stop_state. In that case, we expect either the "gt" bit
or the "eq" bit of cr4 to be set.
Before this patch, on ISA 207, cr4 would be "eq" if we are waking up
from winkle.
*/
BEGIN_FTR_SECTION
+ /*
+ * Were we in winkle?
+ * If yes, check if all threads were in winkle, decrement our
+ * winkle count, set all thread winkle bits if all were in winkle.
+ * Check if our thread has a winkle bit set, and set cr4 accordingly
+ * (to match ISA300, above).
+ */
+ cmpwi r18,PNV_THREAD_WINKLE
+ bne 2f
+ andis. r9,r15,PNV_CORE_IDLE_WINKLE_COUNT_ALL_BIT@h
+ subis r15,r15,PNV_CORE_IDLE_WINKLE_COUNT@h
+ beq 2f
+ ori r15,r15,PNV_CORE_IDLE_THREAD_WINKLE_BITS /* all were winkle */
So PNV_CORE_IDLE_THREAD_WINKLE_BITS will be set by the first waking
thread in a winkle'd core. Subsequent waking thread(s) can only clear
their respective bits from the winkle bits.
+2:
+ /* Shift thread bit to winkle mask, then test if this thread is set,
+ * and remove it from the winkle bits */
+ slwi r8,r7,8
+ and r8,r8,r15
+ andc r15,r15,r8
+ cmpwi cr4,r8,1 /* cr4 will be gt if our bit is set, lt if not */
Very clever indeed! So we seem to be doing the following:
winkle_entry(thread_bit)
{
atomic_inc(core_winkle_count);
}
winkle_exit(thread_bit)
{
atomic {
if (core_winkle_count == 8)
set all thread bits in core_winkle_mask;
if (thread_bit set in core_winkle_mask) {
cr4 will be "gt".
}
clear thread_bit from core_winkle_mask;
}
}
I would suggest documenting this a bit more documentation given the
subtlety.
+
+ cmpwi cr4,r18,PNV_THREAD_WINKLE
This second comparision seems to be undoing the benefit of the
optimization by setting "eq" in cr4 for every thread that wakes up
from winkle irrespective of whether the core has winkled or not.
However, this is quite subtle, so good chances that my addled brain
has gotten it wrong.
Case 1: If a thread were waking up from winkle. We have 2
possibilities
a) The entire core winkled at least once ever since this thread
went to winkle. In this case we want cr4 to be "eq" or "gt" so
that we restore the hypervisor resources.
Now, for this case the first thread in the core that wakes up would find
count == PNV_CORE_IDLE_WINKLE_COUNT_ALL_BIT and hence would have
set PNV_CORE_IDLE_THREAD_WINKLE_BITS which includes this
thread's bit as well. Hence, this thread
would find its bit set in r8, thereby cr4 would be "gt". Which
is good for us!
b) The entire core has not entered winkle. In which case we
haven't lost any hypervisor resources, so no need to restore
these. We would like cr4 to have neither "eq" nor "gt" set here.
In this case, no thread that has woken up prior to this thread
would find count == PNV_CORE_IDLE_WINKLE_COUNT_ALL_BIT. Hence
none of those earlier threads would set
PNV_CORE_IDLE_THREAD_WINKLE_BITS. Thus, for this thread, r8
would be 0, and hence cr4 would be "lt". Which is what we want.
Case 2: If the thread is waking up from fast-sleep. In which case we
haven't lost any hypervisor resources, so no need to restore
these. We would like cr4 to have neither "eq" nor "gt" set here.
While entering fastsleep, this thread wouldn't have contributed
to the winkle count. Thus any thread that would have woken up
before this thread would not find
winkle count == PNV_CORE_IDLE_WINKLE_COUNT_ALL_BIT
and hence wouldn't have set the bits in
PNV_CORE_IDLE_THREAD_WINKLE_BITS. Thus, r8 would
be 0, and hence cr4 would be "lt".
So it seems to be the case that the first comparison suffices. What am
I missing here ?
+
lbz r4,PACA_SUBCORE_SIBLING_MASK(r13)
and r4,r4,r15
cmpwi r4,0 /* Check if first in subcore */
--
2.11.0
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-17 02:49:44
On Thu, 16 Mar 2017 18:10:48 +0530
Mahesh Jagannath Salgaonkar [off-list ref] wrote:
On 03/14/2017 02:53 PM, Nicholas Piggin wrote:
quoted
The ISA specifies power save wakeup can cause a machine check interrupt.
The machine check handler currently has code to handle that for POWER8,
but POWER9 crashes when trying to execute the P8 style sleep
instructions.
So queue up the machine check, then call into the idle code to wake up
as the system reset interrupt does, rather than attempting to sleep
again without going through the main idle path.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/reg.h | 1 +
arch/powerpc/kernel/exceptions-64s.S | 69 ++++++++++++++++++------------------
2 files changed, 35 insertions(+), 35 deletions(-)
Looks like we are not winding up.. Shouldn't we ? What if we may end up
in pnv_wakeup_noloss() which assumes that no GPRs are lost. Am I missing
anything ?
Hmm, on second look, I don't think any non-volatile GPRs are overwritten
in this path. But this MCE is a slow path, and it is a much longer path
than the system reset idle wakeup... So I'll add the napstatelost with
a comment.
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-17 05:15:29
On Fri, 17 Mar 2017 12:49:27 +1000
Nicholas Piggin [off-list ref] wrote:
On Thu, 16 Mar 2017 18:10:48 +0530
Mahesh Jagannath Salgaonkar [off-list ref] wrote:
quoted
On 03/14/2017 02:53 PM, Nicholas Piggin wrote:
quoted
Looks like we are not winding up.. Shouldn't we ? What if we may end up
in pnv_wakeup_noloss() which assumes that no GPRs are lost. Am I missing
anything ?
Hmm, on second look, I don't think any non-volatile GPRs are overwritten
in this path. But this MCE is a slow path, and it is a much longer path
than the system reset idle wakeup... So I'll add the napstatelost with
a comment.
On third look, I'll just add the comment. The windup does not restore
non-volatile GPRs either, and in general we're careful not to use them
in exception handlers. So I think it's okay.
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2017-03-17 05:24:35
On Thu, 16 Mar 2017 21:42:01 +0530
Gautham R Shenoy [off-list ref] wrote:
Hey, thanks for the review.
Hi Nick,
On Tue, Mar 14, 2017 at 07:23:49PM +1000, Nicholas Piggin wrote:
quoted
If not all threads were in winkle, full state loss recovery is not
necessary and can be avoided. A previous patch removed this optimisation
due to some complexity with the implementation. Re-implement it by
counting the number of threads in winkle with the per-core idle state.
Only restore full state loss if all threads were in winkle.
This has a small window of false positives right before threads execute
winkle and just after they wake up, when the winkle count does not
reflect the true number of threads in winkle. This is not a significant
problem in comparison with even the minimum winkle duration. For
correctness, a false positive is not a problem (only false negatives
would be).
quoted
@@ -517,10 +526,34 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE) * At this stage * cr2 - eq if first thread to wakeup in core * cr3- gt if waking up with partial/complete hypervisor state loss+ * ISA300: * cr4 - gt or eq if waking up from complete hypervisor state loss.
For ISA 300, we need to restore hypervisor thread resources if we are
waking up a state that is as deep or deeper than
pnv_first_deep_stop_state. In that case, we expect either the "gt" bit
or the "eq" bit of cr4 to be set.
Before this patch, on ISA 207, cr4 would be "eq" if we are waking up
from winkle.
Yes, that was based on testing the thread idle state (nap/sleep/winkle).
The condition has become more complex now, so it moved below.
quoted
*/
BEGIN_FTR_SECTION
+ /*
+ * Were we in winkle?
+ * If yes, check if all threads were in winkle, decrement our
+ * winkle count, set all thread winkle bits if all were in winkle.
+ * Check if our thread has a winkle bit set, and set cr4 accordingly
+ * (to match ISA300, above).
+ */
+ cmpwi r18,PNV_THREAD_WINKLE
+ bne 2f
+ andis. r9,r15,PNV_CORE_IDLE_WINKLE_COUNT_ALL_BIT@h
+ subis r15,r15,PNV_CORE_IDLE_WINKLE_COUNT@h
+ beq 2f
+ ori r15,r15,PNV_CORE_IDLE_THREAD_WINKLE_BITS /* all were winkle */
So PNV_CORE_IDLE_THREAD_WINKLE_BITS will be set by the first waking
thread in a winkle'd core. Subsequent waking thread(s) can only clear
their respective bits from the winkle bits.
Yes. It was easier to do it on the wakeup side because the sleep side
does not take the lock, only uses atomic load/store.
quoted
+2:
+ /* Shift thread bit to winkle mask, then test if this thread is set,
+ * and remove it from the winkle bits */
+ slwi r8,r7,8
+ and r8,r8,r15
+ andc r15,r15,r8
+ cmpwi cr4,r8,1 /* cr4 will be gt if our bit is set, lt if not */
Very clever indeed! So we seem to be doing the following:
winkle_entry(thread_bit)
{
atomic_inc(core_winkle_count);
}
winkle_exit(thread_bit)
{
atomic {
if (core_winkle_count == 8)
set all thread bits in core_winkle_mask;
if (thread_bit set in core_winkle_mask) {
cr4 will be "gt".
}
clear thread_bit from core_winkle_mask;
}
}
I would suggest documenting this a bit more documentation given the
subtlety.
Yes, commenting the algorithm in pseudo C is probably a good idea. It is
a bit tricky to follow.
quoted
+
+ cmpwi cr4,r18,PNV_THREAD_WINKLE
This second comparision seems to be undoing the benefit of the
optimization by setting "eq" in cr4 for every thread that wakes up
from winkle irrespective of whether the core has winkled or not.
No, good catch. That was some leftover debugging code that got in there
I think. Hmm, I'm not sure how much of my testing that's invalidated, so
I'll have to fix it up and re-run some more tests.
However, this is quite subtle, so good chances that my addled brain
has gotten it wrong.
Case 1: If a thread were waking up from winkle. We have 2
possibilities
a) The entire core winkled at least once ever since this thread
went to winkle. In this case we want cr4 to be "eq" or "gt" so
that we restore the hypervisor resources.
Now, for this case the first thread in the core that wakes up would find
count == PNV_CORE_IDLE_WINKLE_COUNT_ALL_BIT and hence would have
set PNV_CORE_IDLE_THREAD_WINKLE_BITS which includes this
thread's bit as well. Hence, this thread
would find its bit set in r8, thereby cr4 would be "gt". Which
is good for us!
b) The entire core has not entered winkle. In which case we
haven't lost any hypervisor resources, so no need to restore
these. We would like cr4 to have neither "eq" nor "gt" set here.
In this case, no thread that has woken up prior to this thread
would find count == PNV_CORE_IDLE_WINKLE_COUNT_ALL_BIT. Hence
none of those earlier threads would set
PNV_CORE_IDLE_THREAD_WINKLE_BITS. Thus, for this thread, r8
would be 0, and hence cr4 would be "lt". Which is what we want.
Case 2: If the thread is waking up from fast-sleep. In which case we
haven't lost any hypervisor resources, so no need to restore
these. We would like cr4 to have neither "eq" nor "gt" set here.
While entering fastsleep, this thread wouldn't have contributed
to the winkle count. Thus any thread that would have woken up
before this thread would not find
winkle count == PNV_CORE_IDLE_WINKLE_COUNT_ALL_BIT
and hence wouldn't have set the bits in
PNV_CORE_IDLE_THREAD_WINKLE_BITS. Thus, r8 would
be 0, and hence cr4 would be "lt".
So it seems to be the case that the first comparison suffices. What am
I missing here ?
No, nothing you're quite right. Very thorough, thank you.
I'll respin and repost with the feedback.
Thanks,
Nick
Hi,
On Thu, Mar 16, 2017 at 11:05:20PM +1000, Nicholas Piggin wrote:
quoted
On Thu, 16 Mar 2017 18:10:48 +0530
Mahesh Jagannath Salgaonkar [off-list ref] wrote:
quoted
On 03/14/2017 02:53 PM, Nicholas Piggin wrote:
quoted
The ISA specifies power save wakeup can cause a machine check interrupt.
The machine check handler currently has code to handle that for POWER8,
but POWER9 crashes when trying to execute the P8 style sleep
instructions.
So queue up the machine check, then call into the idle code to wake up
as the system reset interrupt does, rather than attempting to sleep
again without going through the main idle path.
Reviewed-by: Gautham R. Shenoy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/reg.h | 1 +
arch/powerpc/kernel/exceptions-64s.S | 69 ++++++++++++++++++------------------
2 files changed, 35 insertions(+), 35 deletions(-)
Looks like we are not winding up.. Shouldn't we ? What if we may end up
in pnv_wakeup_noloss() which assumes that no GPRs are lost. Am I missing
anything ?
Nice catch! This can occur if SRR1[46:47] == 0b01.
quoted
Hmm, no I think you're right. Thanks, good catch. But can we do it with
just setting PACA_NAPSTATELOST?
Unconditionally setting PACA_NAPSTATELOST should be sufficient.
Agree, that should take care.
quoted
quoted
quoted
+ b pnv_powersave_wakeup
+#endif
/*
[...]
Rest looks good to me.
Reviewed-by: Mahesh J Salgaonkar <redacted>