powernv:idle: Fixes for CPU-Hotplug on POWER DD1.0

From: "Gautham R. Shenoy" <[email protected]>

Move the piece of code in powernv/smp.c::pnv_smp_cpu_kill_self() which transitions the CPU to the deepest available platform idle state to a new function named pnv_cpu_offline() in powernv/idle.c. The rationale behind this code movement is that the data required to determine the deepest available platform state resides in powernv/idle.c.

Signed-off-by Gautham R. Shenoy <[email protected]>

From: "Gautham R. Shenoy" <[email protected]>

Currently during idle-init on power9, if we don't find suitable stop states in the device tree that can be used as the default_stop/deepest_stop, we set stop0 (ESL=1,EC=1) as the default stop state psscr to be used by power9_idle and deepest stop state which is used by CPU-Hotplug.

However, if the platform firmware has not configured or enabled a stop state, the kernel should not make any assumptions and fallback to a default choice.

If the kernel uses a stop state that is not configured by the platform firmware, it may lead to further failures which should be avoided.

In this patch, we modify the init code to ensure that the kernel uses only the stop states exposed by the firmware through the device tree. When a suitable default stop state isn't found, we disable ppc_md.power_save for power9. Similarly, when a suitable deepest_stop_state is not found in the device tree exported by the firmware, fall back to the default busy-wait loop in the CPU-Hotplug code.

[Changelog written with inputs from [email protected]]

Signed-off-by Gautham R. Shenoy <[email protected]>

---
 arch/powerpc/platforms/powernv/idle.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

arch/powerpc/platforms/powernv/idle.c +3 -3

index f335e0f..63ade78 100644
@@ -147,7 +147,6 @@ u32 pnv_get_supported_cpuidle_states(void)
 }
 EXPORT_SYMBOL_GPL(pnv_get_supported_cpuidle_states);
 
-
 static void pnv_fastsleep_workaround_apply(void *info)
 
 {

@@ -241,8 +240,9 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600,
  * The default stop state that will be used by ppc_md.power_save
  * function on platforms that support stop instruction.
  */
-u64 pnv_default_stop_val;
-u64 pnv_default_stop_mask;
+static u64 pnv_default_stop_val;
+static u64 pnv_default_stop_mask;
+static bool default_stop_found;
 
 /*
  * Used for ppc_md.power_save which needs a function with no parameters

From: "Gautham R. Shenoy" <[email protected]>

POWER9 DD1.0 hardware has an issue due to which the SPRs of a thread waking up from stop 0,1,2 with ESL=1 can endup being misplaced in the core. Thus the HSPRG0 of a thread waking up from can contain the paca pointer of its sibling.

This patch implements a context recovery framework within threads of a core, by provisioning space in paca_struct for saving every sibling threads's paca pointers. Basically, we should be able to arrive at the right paca pointer from any of the thread's existing paca pointer.

At bootup, during powernv idle-init, we save the paca address of every CPU in each one its siblings paca_struct in the slot corresponding to this CPU's index in the core.

On wakeup from a stop, the thread will determine its index in the core from the lower 2 bits of the PIR register and recover its PACA pointer by indexing into the correct slot in the provisioned space in the current PACA.

Furthermore, ensure that the NVGPRs are restored from the stack on the way out by setting the NAPSTATELOST in paca.

[Changelog written with inputs from [email protected]]

Signed-off-by Gautham R. Shenoy <[email protected]>

---
 arch/powerpc/include/asm/paca.h       |  5 ++++
 arch/powerpc/kernel/asm-offsets.c     |  1 +
 arch/powerpc/kernel/idle_book3s.S     | 49 ++++++++++++++++++++++++++++++++++-
 arch/powerpc/platforms/powernv/idle.c | 22 ++++++++++++++++
 4 files changed, 76 insertions(+), 1 deletion(-)

arch/powerpc/include/asm/paca.h +0 -0

index 708c3e5..4405630 100644

arch/powerpc/include/asm/paca.h +0 -0

 #endif
 
 #ifdef CONFIG_PPC_BOOK3S_64

arch/powerpc/kernel/asm-offsets.c +1 -0

index 4367e7d..6ec5016 100644
@@ -727,6 +727,7 @@ int main(void)
 	OFFSET(PACA_THREAD_IDLE_STATE, paca_struct, thread_idle_state);
 	OFFSET(PACA_THREAD_MASK, paca_struct, thread_mask);
 	OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
+	OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
 #endif
 
 	DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER);

arch/powerpc/kernel/idle_book3s.S +12 -0

index 9957287..c2f2cfb 100644
@@ -375,6 +375,46 @@ _GLOBAL(power9_idle_stop)
 	li	r4,1
 	b	pnv_powersave_common
 	/* No return */
+
+
+/*
+ * On waking up from stop 0,1,2 with ESL=1 on POWER9 DD1,
+ * HSPRG0 will be set to the HSPRG0 value of one of the
+ * threads in this core. Thus the value we have in r13
+ * may not be this thread's paca pointer.
+ *
+ * Fortunately, the PIR remains invariant. Since this thread's
+ * paca pointer is recorded in all its sibling's paca, we can
+ * correctly recover this thread's paca pointer if we
+ * know the index of this thread in the core.

arch/powerpc/kernel/idle_book3s.S +18 -0

+	/*
+	 * Since each entry in thread_sibling_pacas is 8 bytes
+	 * we need to left-shift by 3 bits. Thus r4 = i * 8
+	 */
+	sldi	r4, r4, 3
+	/* Get &paca->thread_sibling_pacas[0] in r5 */
+	addi	r5, r13, PACA_SIBLING_PACA_PTRS
+	/* Load paca->thread_sibling_pacas[i] into r13 */
+	ldx	r13, r4, r5
+	SET_PACA(r13)
+	/*
+	 * Indicate that we have lost NVGPR state
+	 * which needs to be restored from the stack.
+	 */
+	li	r3, 1
+	stb	r0,PACA_NAPSTATELOST(r13)
+	blr
+
 /*
  * Called from reset vector. Check whether we have woken up with
  * hypervisor state loss. If yes, restore hypervisor state and return

@@ -385,7 +425,14 @@ _GLOBAL(power9_idle_stop)
  */
 _GLOBAL(pnv_restore_hyp_resource)

On Mon, 20 Mar 2017 21:24:18 +0530

This is quite neat now you've moved it to its own function. Nice. It will be only a trivial clash with my patches now, I think.

Reviewed-by Nicholas Piggin <[email protected]>

Quoted 8 lines · On Mon, 20 Mar 2017 21:24:15 +0530 "Gautham R. Shenoy"…

From: "Gautham R. Shenoy" <[email protected]>

Move the piece of code in powernv/smp.c::pnv_smp_cpu_kill_self() which transitions the CPU to the deepest available platform idle state to a new function named pnv_cpu_offline() in powernv/idle.c. The rationale behind this code movement is that the data required to determine the deepest available platform state resides in powernv/idle.c.

Quoted 1 line

Signed-off-by Gautham R. Shenoy <[email protected]>

Looks good. As a nitpick, possibly one or two variables in idle.c could become static (pnv_deepest_stop_psscr_*).

Reviewed-by Nicholas Piggin <[email protected]>

Quoted 13 lines

arch/powerpc/platforms/powernv/idle.c +3 -2

@@ -241,8 +240,9 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600,
  * The default stop state that will be used by ppc_md.power_save
  * function on platforms that support stop instruction.
  */
-u64 pnv_default_stop_val;
-u64 pnv_default_stop_mask;
+static u64 pnv_default_stop_val;
+static u64 pnv_default_stop_mask;
+static bool default_stop_found;
 
 /*
  * Used for ppc_md.power_save which needs a function with no parameters

arch/powerpc/platforms/powernv/idle.c static void power9_idle(void) · line 267

Quoted 8 lines

arch/powerpc/platforms/powernv/idle.c +3 -2

@@ -262,8 +262,9 @@ static void power9_idle(void)
  * psscr value and mask of the deepest stop idle state.
  * Used when a cpu is offlined.
  */
-u64 pnv_deepest_stop_psscr_val;
-u64 pnv_deepest_stop_psscr_mask;
+static u64 pnv_deepest_stop_psscr_val;
+static u64 pnv_deepest_stop_psscr_mask;
+static bool deepest_stop_found;

Aha you have made them static. Nitpick withdrawn :)

The log messages look good now.

Reviewed-by Nicholas Piggin <[email protected]>

Quoted 16 lines · On Mon, 20 Mar 2017 21:24:18 +0530 "Gautham R. Shenoy"…

From: "Gautham R. Shenoy" <[email protected]>

POWER9 DD1.0 hardware has an issue due to which the SPRs of a thread waking up from stop 0,1,2 with ESL=1 can endup being misplaced in the core. Thus the HSPRG0 of a thread waking up from can contain the paca pointer of its sibling.

This patch implements a context recovery framework within threads of a core, by provisioning space in paca_struct for saving every sibling threads's paca pointers. Basically, we should be able to arrive at the right paca pointer from any of the thread's existing paca pointer.

At bootup, during powernv idle-init, we save the paca address of every CPU in each one its siblings paca_struct in the slot corresponding to this CPU's index in the core.

Quoted 7 lines

On wakeup from a stop, the thread will determine its index in the core from the lower 2 bits of the PIR register and recover its PACA pointer by indexing into the correct slot in the provisioned space in the current PACA.

Furthermore, ensure that the NVGPRs are restored from the stack on the way out by setting the NAPSTATELOST in paca.

Thanks for expanding on this, it makes the patch easier to follow :)

As noted before, I think if we use PACA_EXNMI for system reset, then *hopefully* there should be minimal races with the initial use of other thread's PACA at the start of the exception. So I'll work on getting that in, but it need not prevent this patch from being merged first IMO.

Quoted 10 lines

[Changelog written with inputs from [email protected]]

Signed-off-by Gautham R. Shenoy <[email protected]>

---
 arch/powerpc/include/asm/paca.h       |  5 ++++
 arch/powerpc/kernel/asm-offsets.c     |  1 +
 arch/powerpc/kernel/idle_book3s.S     | 49 ++++++++++++++++++++++++++++++++++-
 arch/powerpc/platforms/powernv/idle.c | 22 ++++++++++++++++
 4 files changed, 76 insertions(+), 1 deletion(-)

arch/powerpc/include/asm/paca.h struct paca_struct { · line 179

Quoted 13 lines

arch/powerpc/include/asm/paca.h +5 -0

index 708c3e5..4405630 100644
@@ -172,6 +172,11 @@ struct paca_struct {
 	u8 thread_mask;
 	/* Mask to denote subcore sibling threads */
 	u8 subcore_sibling_mask;
+	/*
+	 * Pointer to an array which contains pointer
+	 * to the sibling threads' paca.
+	 */
+	struct paca_struct *thread_sibling_pacas[8];

Is 8 the right number? I wonder if we have a define for it.

Quoted 40 lines

arch/powerpc/include/asm/paca.h +0 -0

 #endif
 
 #ifdef CONFIG_PPC_BOOK3S_64

arch/powerpc/kernel/asm-offsets.c +1 -0

index 4367e7d..6ec5016 100644
@@ -727,6 +727,7 @@ int main(void)
 	OFFSET(PACA_THREAD_IDLE_STATE, paca_struct, thread_idle_state);
 	OFFSET(PACA_THREAD_MASK, paca_struct, thread_mask);
 	OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
+	OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
 #endif
 
 	DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER);

arch/powerpc/kernel/idle_book3s.S +16 -0

index 9957287..c2f2cfb 100644
@@ -375,6 +375,46 @@ _GLOBAL(power9_idle_stop)
 	li	r4,1
 	b	pnv_powersave_common
 	/* No return */
+
+
+/*
+ * On waking up from stop 0,1,2 with ESL=1 on POWER9 DD1,
+ * HSPRG0 will be set to the HSPRG0 value of one of the
+ * threads in this core. Thus the value we have in r13
+ * may not be this thread's paca pointer.
+ *
+ * Fortunately, the PIR remains invariant. Since this thread's
+ * paca pointer is recorded in all its sibling's paca, we can
+ * correctly recover this thread's paca pointer if we
+ * know the index of this thread in the core.
+ *
+ * This index can be obtained from the lower two bits of the PIR.
+ *
+ * i.e, thread's position in the core = PIR[62:63].

arch/powerpc/kernel/idle_book3s.S _GLOBAL(power9_idle_stop) · line 399

Quoted 6 lines

arch/powerpc/kernel/idle_book3s.S +6 -0

+ * If this value is i, then this thread's paca is
+ * paca->thread_sibling_pacas[i].
+ */
+power9_dd1_recover_paca:
+	mfspr	r4, SPRN_PIR
+	clrldi	r4, r4, 62

Does SPRN_TIR work?

Quoted 30 lines

arch/powerpc/kernel/idle_book3s.S +21 -1

+	/*
+	 * Since each entry in thread_sibling_pacas is 8 bytes
+	 * we need to left-shift by 3 bits. Thus r4 = i * 8
+	 */
+	sldi	r4, r4, 3
+	/* Get &paca->thread_sibling_pacas[0] in r5 */
+	addi	r5, r13, PACA_SIBLING_PACA_PTRS
+	/* Load paca->thread_sibling_pacas[i] into r13 */
+	ldx	r13, r4, r5
+	SET_PACA(r13)
+	/*
+	 * Indicate that we have lost NVGPR state
+	 * which needs to be restored from the stack.
+	 */
+	li	r3, 1
+	stb	r0,PACA_NAPSTATELOST(r13)
+	blr
+
 /*
  * Called from reset vector. Check whether we have woken up with
  * hypervisor state loss. If yes, restore hypervisor state and return

@@ -385,7 +425,14 @@ _GLOBAL(power9_idle_stop)
  */
 _GLOBAL(pnv_restore_hyp_resource)
 BEGIN_FTR_SECTION
-	ld	r2,PACATOC(r13);
+BEGIN_FTR_SECTION_NESTED(70)
+	mflr 	r6
+	bl	power9_dd1_recover_paca

arch/powerpc/kernel/idle_book3s.S _GLOBAL(power9_idle_stop) · line 435

Quoted 5 lines

arch/powerpc/kernel/idle_book3s.S +5 -0

+	mtlr	r6
+	ld	r2, PACATOC(r13)
+FTR_SECTION_ELSE_NESTED(70)
+	ld	r2, PACATOC(r13)
+ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_POWER9_DD1, 70)

This is quite neat now you've moved it to its own function. Nice. It will be only a trivial clash with my patches now, I think.

Reviewed-by Nicholas Piggin <[email protected]>

Quoted 45 lines · Nicholas Piggin

Quoted 16 lines · On Mon, 20 Mar 2017 21:24:18 +0530 "Gautham R. Shenoy"…

From: "Gautham R. Shenoy" <[email protected]>

POWER9 DD1.0 hardware has an issue due to which the SPRs of a thread waking up from stop 0,1,2 with ESL=1 can endup being misplaced in the core. Thus the HSPRG0 of a thread waking up from can contain the paca pointer of its sibling.

This patch implements a context recovery framework within threads of a core, by provisioning space in paca_struct for saving every sibling threads's paca pointers. Basically, we should be able to arrive at the right paca pointer from any of the thread's existing paca pointer.

At bootup, during powernv idle-init, we save the paca address of every CPU in each one its siblings paca_struct in the slot corresponding to this CPU's index in the core.

Quoted 7 lines

On wakeup from a stop, the thread will determine its index in the core from the lower 2 bits of the PIR register and recover its PACA pointer by indexing into the correct slot in the provisioned space in the current PACA.

Furthermore, ensure that the NVGPRs are restored from the stack on the way out by setting the NAPSTATELOST in paca.

Thanks for expanding on this, it makes the patch easier to follow :)

As noted before, I think if we use PACA_EXNMI for system reset, then *hopefully* there should be minimal races with the initial use of other thread's PACA at the start of the exception. So I'll work on getting that in, but it need not prevent this patch from being merged first IMO.

Quoted 10 lines

[Changelog written with inputs from [email protected]]

Signed-off-by Gautham R. Shenoy <[email protected]>

---
 arch/powerpc/include/asm/paca.h       |  5 ++++
 arch/powerpc/kernel/asm-offsets.c     |  1 +
 arch/powerpc/kernel/idle_book3s.S     | 49 ++++++++++++++++++++++++++++++++++-
 arch/powerpc/platforms/powernv/idle.c | 22 ++++++++++++++++
 4 files changed, 76 insertions(+), 1 deletion(-)

Quoted 16 lines

arch/powerpc/include/asm/paca.h struct paca_struct { · line 179

Quoted 13 lines

arch/powerpc/include/asm/paca.h +5 -0

index 708c3e5..4405630 100644
@@ -172,6 +172,11 @@ struct paca_struct {
 	u8 thread_mask;
 	/* Mask to denote subcore sibling threads */
 	u8 subcore_sibling_mask;
+	/*
+	 * Pointer to an array which contains pointer
+	 * to the sibling threads' paca.
+	 */
+	struct paca_struct *thread_sibling_pacas[8];  

Is 8 the right number? I wonder if we have a define for it.

Thats the maximum number of threads per core that we have had on POWER so far.

Perhaps, I can make this

	 struct paca_struct **thread_sibling_pacas;

and allocate threads_per_core number of slots in
pnv_init_idle_states. Sounds ok ?

Quoted 5 lines

arch/powerpc/include/asm/paca.h struct paca_struct { · line 182

Quoted 3 lines

arch/powerpc/include/asm/paca.h +3 -0

+power9_dd1_recover_paca:
+	mfspr	r4, SPRN_PIR
+	clrldi	r4, r4, 62  

Does SPRN_TIR work?

I wasn't aware of SPRN_TIR!

I can check this. If my reading of the ISA is correct, TIR should contain the thread number which are in the range [0..3].

Quoted 2 lines

Reviewed-by Nicholas Piggin <[email protected]>

Thanks for reviewing the patch.