[PATCH v3 0/4] powerpc/64s: idle POWER9 stop improvements

[PATCH v3 0/4] powerpc/64s: idle POWER9 stop improvements

[Nicholas Piggin]

These are rebased patches leftover from the unmerged bit of
the idle series.

Based on feedback, I dropped one of the KVM patches, and reworked
the code a bit so it is easier to restore the ability for KVM to
grab secondaries into real mode.

I did a bit more benchmarking, and all up these patches improve 2
CPU ping-pong context switch benchmark on a POWER9 by around 4-6%
(depending on what CPUs and idle states are used).

Nicholas Piggin (4):
  KVM: PPC: Book3S HV: POWER9 does not require secondary thread
    management
  powerpc/64s: idle POWER9 can execute stop without a sync sequence
  powerpc/64s: idle POWER9 can execute stop in virtual mode
  powerpc/64s: idle ESL=0 stop can avoid MSR and save/restore overhead

 arch/powerpc/include/asm/cpuidle.h        |  16 -----
 arch/powerpc/include/asm/kvm_book3s_asm.h |   4 ++
 arch/powerpc/kernel/idle_book3s.S         | 103 ++++++++++++++++++++++--------
 arch/powerpc/kvm/book3s_hv.c              |  14 +++-
 arch/powerpc/kvm/book3s_hv_rmhandlers.S   |   8 +++
 5 files changed, 101 insertions(+), 44 deletions(-)

-- 
2.13.3

[PATCH v3 1/4] KVM: PPC: Book3S HV: POWER9 does not require secondary thread management

[Nicholas Piggin]

POWER9 CPUs have independent MMU contexts per thread, so KVM does not
need to quiesce secondary threads, so the hwthread_req/hwthread_state
protocol does not have to be used. So patch it away on POWER9, and patch
away the branch from the Linux idle wakeup to kvm_start_guest that is
never used.

Add a warning and error out of kvmppc_grab_hwthread in case it is ever
called on POWER9.

This avoids a hwsync in the idle wakeup path on POWER9.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/kvm_book3s_asm.h |  4 ++++
 arch/powerpc/kernel/idle_book3s.S         | 35 +++++++++++++++++++++----------
 arch/powerpc/kvm/book3s_hv.c              | 14 ++++++++++++-
 arch/powerpc/kvm/book3s_hv_rmhandlers.S   |  8 +++++++
 4 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_book3s_asm.h b/arch/powerpc/include/asm/kvm_book3s_asm.h
index 7cea76f11c26..83596f32f50b 100644
--- a/arch/powerpc/include/asm/kvm_book3s_asm.h
+++ b/arch/powerpc/include/asm/kvm_book3s_asm.h
@@ -104,6 +104,10 @@ struct kvmppc_host_state {
 	u8 napping;
 
 #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
+	/*
+	 * hwthread_req/hwthread_state pair is used to pull sibling threads
+	 * out of guest on pre-ISAv3.0B CPUs where threads share MMU.
+	 */
 	u8 hwthread_req;
 	u8 hwthread_state;
 	u8 host_ipi;
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index bfbf0976fc09..4924647d964d 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -296,13 +296,20 @@ enter_winkle:
 /*
  * r3 - PSSCR value corresponding to the requested stop state.
  */
-power_enter_stop:
 #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
-	/* Tell KVM we're entering idle */
+power_enter_stop_kvm_rm:
+	/*
+	 * This is currently unused because POWER9 KVM does not have to
+	 * gather secondary threads into sibling mode, but the code is
+	 * here in case that function is required.
+	 *
+	 * Tell KVM we're entering idle.
+	 */
 	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
  */
@@ -465,6 +472,18 @@ pnv_powersave_wakeup_mce:
 
 	b	pnv_powersave_wakeup
 
+#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
+kvm_start_guest_check:
+	li	r0,KVM_HWTHREAD_IN_KERNEL
+	stb	r0,HSTATE_HWTHREAD_STATE(r13)
+	/* Order setting hwthread_state vs. testing hwthread_req */
+	sync
+	lbz	r0,HSTATE_HWTHREAD_REQ(r13)
+	cmpwi	r0,0
+	beqlr
+	b	kvm_start_guest
+#endif
+
 /*
  * Called from reset vector for powersave wakeups.
  * cr3 - set to gt if waking up with partial/complete hypervisor state loss
@@ -489,15 +508,9 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_ARCH_300)
 	mr	r3,r12
 
 #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
-	li	r0,KVM_HWTHREAD_IN_KERNEL
-	stb	r0,HSTATE_HWTHREAD_STATE(r13)
-	/* Order setting hwthread_state vs. testing hwthread_req */
-	sync
-	lbz	r0,HSTATE_HWTHREAD_REQ(r13)
-	cmpwi	r0,0
-	beq	1f
-	b	kvm_start_guest
-1:
+BEGIN_FTR_SECTION
+	bl	kvm_start_guest_check
+END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
 #endif
 
 	/* Return SRR1 from power7_nap() */
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 359c79cdf0cc..e34cd6fb947b 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -2111,6 +2111,16 @@ static int kvmppc_grab_hwthread(int cpu)
 	struct paca_struct *tpaca;
 	long timeout = 10000;
 
+	/*
+	 * ISA v3.0 idle routines do not set hwthread_state or test
+	 * hwthread_req, so they can not grab idle threads.
+	 */
+	if (cpu_has_feature(CPU_FTR_ARCH_300)) {
+		WARN_ON(1);
+		pr_err("KVM: can not control sibling threads\n");
+		return -EBUSY;
+	}
+
 	tpaca = &paca[cpu];
 
 	/* Ensure the thread won't go into the kernel if it wakes */
@@ -2145,10 +2155,12 @@ static void kvmppc_release_hwthread(int cpu)
 	struct paca_struct *tpaca;
 
 	tpaca = &paca[cpu];
-	tpaca->kvm_hstate.hwthread_req = 0;
 	tpaca->kvm_hstate.kvm_vcpu = NULL;
 	tpaca->kvm_hstate.kvm_vcore = NULL;
 	tpaca->kvm_hstate.kvm_split_mode = NULL;
+	if (!cpu_has_feature(CPU_FTR_ARCH_300))
+		tpaca->kvm_hstate.hwthread_req = 0;
+
 }
 
 static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index c52184a8efdf..3e024fd71fe8 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -149,9 +149,11 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
 	subf	r4, r4, r3
 	mtspr	SPRN_DEC, r4
 
+BEGIN_FTR_SECTION
 	/* hwthread_req may have got set by cede or no vcpu, so clear it */
 	li	r0, 0
 	stb	r0, HSTATE_HWTHREAD_REQ(r13)
+END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
 
 	/*
 	 * For external interrupts we need to call the Linux
@@ -314,6 +316,7 @@ kvm_novcpu_exit:
  * Relocation is off and most register values are lost.
  * r13 points to the PACA.
  * r3 contains the SRR1 wakeup value, SRR1 is trashed.
+ * This is not used by ISAv3.0B processors.
  */
 	.globl	kvm_start_guest
 kvm_start_guest:
@@ -432,6 +435,9 @@ kvm_secondary_got_guest:
  * While waiting we also need to check if we get given a vcpu to run.
  */
 kvm_no_guest:
+BEGIN_FTR_SECTION
+	twi	31,0,0
+END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
 	lbz	r3, HSTATE_HWTHREAD_REQ(r13)
 	cmpwi	r3, 0
 	bne	53f
@@ -2509,8 +2515,10 @@ kvm_do_nap:
 	clrrdi	r0, r0, 1
 	mtspr	SPRN_CTRLT, r0
 
+BEGIN_FTR_SECTION
 	li	r0,1
 	stb	r0,HSTATE_HWTHREAD_REQ(r13)
+END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
 	mfspr	r5,SPRN_LPCR
 	ori	r5,r5,LPCR_PECE0 | LPCR_PECE1
 BEGIN_FTR_SECTION
-- 
2.13.3

Re: [PATCH v3 1/4] KVM: PPC: Book3S HV: POWER9 does not require secondary thread management

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:

> POWER9 CPUs have independent MMU contexts per thread, so KVM does not
> need to quiesce secondary threads, so the hwthread_req/hwthread_state
> protocol does not have to be used. So patch it away on POWER9, and patch
> away the branch from the Linux idle wakeup to kvm_start_guest that is
> never used.
>
> Add a warning and error out of kvmppc_grab_hwthread in case it is ever
> called on POWER9.
>
> This avoids a hwsync in the idle wakeup path on POWER9.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/include/asm/kvm_book3s_asm.h |  4 ++++
>  arch/powerpc/kernel/idle_book3s.S         | 35 +++++++++++++++++++++----------
>  arch/powerpc/kvm/book3s_hv.c              | 14 ++++++++++++-
>  arch/powerpc/kvm/book3s_hv_rmhandlers.S   |  8 +++++++
>  4 files changed, 49 insertions(+), 12 deletions(-)

Paulus do you mind acking this and I'll put it in my ppc-kvm topic
branch.

cheers

Re: [PATCH v3 1/4] KVM: PPC: Book3S HV: POWER9 does not require secondary thread management

[Paul Mackerras]

On Fri, Aug 25, 2017 at 02:30:33PM +1000, Nicholas Piggin wrote:
> POWER9 CPUs have independent MMU contexts per thread, so KVM does not
> need to quiesce secondary threads, so the hwthread_req/hwthread_state
> protocol does not have to be used. So patch it away on POWER9, and patch
> away the branch from the Linux idle wakeup to kvm_start_guest that is
> never used.
> 
> Add a warning and error out of kvmppc_grab_hwthread in case it is ever
> called on POWER9.
> 
> This avoids a hwsync in the idle wakeup path on POWER9.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Some of the feature sections seem a little unnecessary except for
documentation purposes (e.g. the second one added after the
kvm_no_guest label), but they won't actually hurt, so:

Acked-by: Paul Mackerras <paulus@ozlabs.org>

Re: [v3, 1/4] KVM: PPC: Book3S HV: POWER9 does not require secondary thread management

[Michael Ellerman]

On Fri, 2017-08-25 at 04:30:33 UTC, Nicholas Piggin wrote:
> POWER9 CPUs have independent MMU contexts per thread, so KVM does not
> need to quiesce secondary threads, so the hwthread_req/hwthread_state
> protocol does not have to be used. So patch it away on POWER9, and patch
> away the branch from the Linux idle wakeup to kvm_start_guest that is
> never used.
> 
> Add a warning and error out of kvmppc_grab_hwthread in case it is ever
> called on POWER9.
> 
> This avoids a hwsync in the idle wakeup path on POWER9.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Acked-by: Paul Mackerras <paulus@ozlabs.org>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/94a04bc25a2c6296bd0c5e82c10e82

cheers

[PATCH v3 2/4] powerpc/64s: idle POWER9 can execute stop without a sync sequence

[Nicholas Piggin]

Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/cpuidle.h | 16 ----------------
 arch/powerpc/kernel/idle_book3s.S  | 26 ++++++++++++++++++++------
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/include/asm/cpuidle.h b/arch/powerpc/include/asm/cpuidle.h
index 8a174cba5567..eb43b5c3a7b5 100644
--- a/arch/powerpc/include/asm/cpuidle.h
+++ b/arch/powerpc/include/asm/cpuidle.h
@@ -101,20 +101,4 @@ static inline void report_invalid_psscr_val(u64 psscr_val, int err)
 
 #endif
 
-/* Idle state entry routines */
-#ifdef	CONFIG_PPC_P7_NAP
-#define IDLE_STATE_ENTER_SEQ(IDLE_INST)                         \
-	/* Magic NAP/SLEEP/WINKLE mode enter sequence */	\
-	std	r0,0(r1);					\
-	ptesync;						\
-	ld	r0,0(r1);					\
-236:	cmpd	cr0,r0,r0;					\
-	bne	236b;						\
-	IDLE_INST;						\
-
-#define	IDLE_STATE_ENTER_SEQ_NORET(IDLE_INST)			\
-	IDLE_STATE_ENTER_SEQ(IDLE_INST)                         \
-	b	.
-#endif /* CONFIG_PPC_P7_NAP */
-
 #endif
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index 4924647d964d..14e97f442167 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -205,6 +205,19 @@ pnv_powersave_common:
 	mtmsrd	r7,0
 	bctr
 
+/*
+ * This is the sequence required to execute idle instructions, as
+ * specified in ISA v2.07. MSR[IR] and MSR[DR] must be 0.
+ */
+#define ARCH207_IDLE_STATE_ENTER_SEQ_NORET(IDLE_INST)		\
+	/* Magic NAP/SLEEP/WINKLE mode enter sequence */	\
+	std	r0,0(r1);					\
+	ptesync;						\
+	ld	r0,0(r1);					\
+236:	cmpd	cr0,r0,r0;					\
+	bne	236b;						\
+	IDLE_INST;
+
 	.globl pnv_enter_arch207_idle_mode
 pnv_enter_arch207_idle_mode:
 #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
@@ -230,7 +243,7 @@ pnv_enter_arch207_idle_mode:
 	stb	r3,PACA_THREAD_IDLE_STATE(r13)
 	cmpwi	cr3,r3,PNV_THREAD_SLEEP
 	bge	cr3,2f
-	IDLE_STATE_ENTER_SEQ_NORET(PPC_NAP)
+	ARCH207_IDLE_STATE_ENTER_SEQ_NORET(PPC_NAP)
 	/* No return */
 2:
 	/* Sleep or winkle */
@@ -269,7 +282,7 @@ pnv_fastsleep_workaround_at_entry:
 
 common_enter: /* common code for all the threads entering sleep or winkle */
 	bgt	cr3,enter_winkle
-	IDLE_STATE_ENTER_SEQ_NORET(PPC_SLEEP)
+	ARCH207_IDLE_STATE_ENTER_SEQ_NORET(PPC_SLEEP)
 
 fastsleep_workaround_at_entry:
 	oris	r15,r15,PNV_CORE_IDLE_LOCK_BIT@h
@@ -291,7 +304,7 @@ fastsleep_workaround_at_entry:
 enter_winkle:
 	bl	save_sprs_to_stack
 
-	IDLE_STATE_ENTER_SEQ_NORET(PPC_WINKLE)
+	ARCH207_IDLE_STATE_ENTER_SEQ_NORET(PPC_WINKLE)
 
 /*
  * r3 - PSSCR value corresponding to the requested stop state.
@@ -316,7 +329,7 @@ power_enter_stop:
 	andis.   r4,r3,PSSCR_EC_ESL_MASK_SHIFTED
 	clrldi   r3,r3,60 /* r3 = Bits[60:63] = Requested Level (RL) */
 	bne	 .Lhandle_esl_ec_set
-	IDLE_STATE_ENTER_SEQ(PPC_STOP)
+	PPC_STOP
 	li	r3,0  /* Since we didn't lose state, return 0 */
 
 	/*
@@ -349,7 +362,8 @@ power_enter_stop:
 	ld	r4,ADDROFF(pnv_first_deep_stop_state)(r5)
 	cmpd	r3,r4
 	bge	.Lhandle_deep_stop
-	IDLE_STATE_ENTER_SEQ_NORET(PPC_STOP)
+	PPC_STOP	/* Does not return (system reset interrupt) */
+
 .Lhandle_deep_stop:
 /*
  * Entering deep idle state.
@@ -371,7 +385,7 @@ lwarx_loop_stop:
 
 	bl	save_sprs_to_stack
 
-	IDLE_STATE_ENTER_SEQ_NORET(PPC_STOP)
+	PPC_STOP	/* Does not return (system reset interrupt) */
 
 /*
  * Entered with MSR[EE]=0 and no soft-masked interrupts pending.
-- 
2.13.3

Re: [PATCH v3 2/4] powerpc/64s: idle POWER9 can execute stop without a sync sequence

[Paul Mackerras]

On Fri, Aug 25, 2017 at 02:30:34PM +1000, Nicholas Piggin wrote:
> Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/include/asm/cpuidle.h | 16 ----------------
>  arch/powerpc/kernel/idle_book3s.S  | 26 ++++++++++++++++++++------
>  2 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/cpuidle.h b/arch/powerpc/include/asm/cpuidle.h
> index 8a174cba5567..eb43b5c3a7b5 100644
> --- a/arch/powerpc/include/asm/cpuidle.h
> +++ b/arch/powerpc/include/asm/cpuidle.h
> @@ -101,20 +101,4 @@ static inline void report_invalid_psscr_val(u64 psscr_val, int err)
>  
>  #endif
>  
> -/* Idle state entry routines */
> -#ifdef	CONFIG_PPC_P7_NAP
> -#define IDLE_STATE_ENTER_SEQ(IDLE_INST)                         \
> -	/* Magic NAP/SLEEP/WINKLE mode enter sequence */	\
> -	std	r0,0(r1);					\
> -	ptesync;						\
> -	ld	r0,0(r1);					\
> -236:	cmpd	cr0,r0,r0;					\
> -	bne	236b;						\
> -	IDLE_INST;						\
> -
> -#define	IDLE_STATE_ENTER_SEQ_NORET(IDLE_INST)			\
> -	IDLE_STATE_ENTER_SEQ(IDLE_INST)                         \
> -	b	.
> -#endif /* CONFIG_PPC_P7_NAP */
> -
>  #endif
> diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> index 4924647d964d..14e97f442167 100644
> --- a/arch/powerpc/kernel/idle_book3s.S
> +++ b/arch/powerpc/kernel/idle_book3s.S
> @@ -205,6 +205,19 @@ pnv_powersave_common:
>  	mtmsrd	r7,0
>  	bctr
>  
> +/*
> + * This is the sequence required to execute idle instructions, as
> + * specified in ISA v2.07. MSR[IR] and MSR[DR] must be 0.
> + */
> +#define ARCH207_IDLE_STATE_ENTER_SEQ_NORET(IDLE_INST)		\

We had to do this sequence on POWER7 also, which is architecture
v2.06.  Thus the comments and the naming (ARCH207_*) are a bit
misleading here.  The actual code change looks OK.

Paul.

Re: [PATCH v3 2/4] powerpc/64s: idle POWER9 can execute stop without a sync sequence

[Michael Ellerman]

Paul Mackerras <paulus@ozlabs.org> writes:
> On Fri, Aug 25, 2017 at 02:30:34PM +1000, Nicholas Piggin wrote:
>> diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
>> index 4924647d964d..14e97f442167 100644
>> --- a/arch/powerpc/kernel/idle_book3s.S
>> +++ b/arch/powerpc/kernel/idle_book3s.S
>> @@ -205,6 +205,19 @@ pnv_powersave_common:
>>  	mtmsrd	r7,0
>>  	bctr
>>  
>> +/*
>> + * This is the sequence required to execute idle instructions, as
>> + * specified in ISA v2.07. MSR[IR] and MSR[DR] must be 0.
>> + */
>> +#define ARCH207_IDLE_STATE_ENTER_SEQ_NORET(IDLE_INST)		\
>
> We had to do this sequence on POWER7 also, which is architecture
> v2.06.  Thus the comments and the naming (ARCH207_*) are a bit
> misleading here.  The actual code change looks OK.

I'll just drop the name change, I don't think it's crucial. That makes
P9 the special case.

We can come up with a better name or something in future.

Unless Nick objects?

cheers

Re: [v3, 2/4] powerpc/64s: idle POWER9 can execute stop without a sync sequence

[Michael Ellerman]

On Fri, 2017-08-25 at 04:30:34 UTC, Nicholas Piggin wrote:
> Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/56ee52408ed0bd4af400c04ad60f98

cheers

[PATCH v3 3/4] powerpc/64s: idle POWER9 can execute stop in virtual mode

[Nicholas Piggin]

The hardware can execute stop in any context, and KVM does not
require real mode because siblings do not share MMU state. This
saves a switch to real-mode when going idle.

Acked-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/idle_book3s.S | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index 14e97f442167..32d65ee323a0 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -195,7 +195,16 @@ pnv_powersave_common:
 	std	r5,_CCR(r1)
 	std	r1,PACAR1(r13)
 
+BEGIN_FTR_SECTION
+	/*
+	 * POWER9 does not require real mode to stop, and presently does not
+	 * set hwthread_state for KVM (threads don't share MMU context), so
+	 * we can remain in virtual mode for this.
+	 */
+	bctr
+END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
 	/*
+	 * POWER8
 	 * Go to real mode to do the nap, as required by the architecture.
 	 * Also, we need to be in real mode before setting hwthread_state,
 	 * because as soon as we do that, another thread can switch
-- 
2.13.3

Re: [PATCH v3 3/4] powerpc/64s: idle POWER9 can execute stop in virtual mode

[Paul Mackerras]

On Fri, Aug 25, 2017 at 02:30:35PM +1000, Nicholas Piggin wrote:
> The hardware can execute stop in any context, and KVM does not
> require real mode because siblings do not share MMU state. This
> saves a switch to real-mode when going idle.
> 
> Acked-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/kernel/idle_book3s.S | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> index 14e97f442167..32d65ee323a0 100644
> --- a/arch/powerpc/kernel/idle_book3s.S
> +++ b/arch/powerpc/kernel/idle_book3s.S
> @@ -195,7 +195,16 @@ pnv_powersave_common:
>  	std	r5,_CCR(r1)
>  	std	r1,PACAR1(r13)
>  
> +BEGIN_FTR_SECTION
> +	/*
> +	 * POWER9 does not require real mode to stop, and presently does not
> +	 * set hwthread_state for KVM (threads don't share MMU context), so
> +	 * we can remain in virtual mode for this.
> +	 */
> +	bctr
> +END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
>  	/*
> +	 * POWER8

... and POWER7 too.

Since that's just a comment,

Reviewed-by: Paul Mackerras <paulus@ozlabs.org>

Re: [v3, 3/4] powerpc/64s: idle POWER9 can execute stop in virtual mode

[Michael Ellerman]

On Fri, 2017-08-25 at 04:30:35 UTC, Nicholas Piggin wrote:
> The hardware can execute stop in any context, and KVM does not
> require real mode because siblings do not share MMU state. This
> saves a switch to real-mode when going idle.
> 
> Acked-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Reviewed-by: Paul Mackerras <paulus@ozlabs.org>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/72b0d51d973beab5a06c97279b61a0

cheers

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

[Nicholas Piggin]

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
+	bne	1f
+	PPC_STOP
+	li	r3,0  /* Since we didn't lose state, return 0 */
+	blr
+1:	/* state-loss idle */
+	std	r3, PACA_REQ_PSSCR(r13)
+	LOAD_REG_ADDR(r4,power_enter_stop_esl)
 	b	pnv_powersave_common
 	/* No return */
 
-- 
2.13.3

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

[Paul Mackerras]

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

Apart from that very minor nit,

Reviewed-by: Paul Mackerras <paulus@ozlabs.org>

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

[Nicholas Piggin]

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>

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

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:

> 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(-)

This is blowing up for me on mambo:

  usbcore: registered new interface driver usb-storage
  Disabling lock debugging due to kernel taint
  Severe Machine check interrupt [Not recovered]
    NIP [c0000000002a0c04]: kmem_cache_free+0x64/0x2c0
    Initiator: CPU
    Error type: Real address [Load/Store (foreign)]
  opal: Hardware platform error: Unrecoverable Machine Check exception
  CPU: 0 PID: 0 Comm: swapper/0 Tainted: G   M            4.13.0-rc2-gcc-6.3.1-00257-g26268bb39bff #543
  task: c0000000016b1200 task.stack: c00000000175c000
  NIP:  c0000000002a0c04 LR: c0000000001128ec CTR: c000000000112800
  REGS: c00000003fff7d80 TRAP: 0200   Tainted: G   M             (4.13.0-rc2-gcc-6.3.1-00257-g26268bb39bff)
  MSR:  9000000000209003 <SF,HV,EE,ME,RI,LE>  CR: 28002828  XER: 20000000
  CFAR: c0000000001128e8 DAR: c00a0000003c3ce0 DSISR: 00000008 SOFTE: 1 
  GPR00: c0000000001128ec c00000000175f7b0 c000000001760500 c0000000f001a000 
  GPR04: c0000000f0f37f00 0000000000000003 00000000000220c3 0000000000000000 
  GPR08: 0000000000000000 00000000003c3cc0 c0000000017e7758 0000000000000000 
  GPR12: c000000000112800 c00000000fff0000 c000000000112800 c0000000f0f37f90 
  GPR16: c00000000175c000 c00000000175c000 0000000000000001 c0000000016d7900 
  GPR20: c00000000179ba98 7fffffffffffffff c00000000175c000 0000000000000000 
  GPR24: c00000000017a8c0 000000000000000a c0000000016d8a00 c00000000175f8d0 
  GPR28: c0000000001128ec c0000000f0f37f00 c00a0000003c3cc0 c0000000f001a000 
  NIP [c0000000002a0c04] kmem_cache_free+0x64/0x2c0
  LR [c0000000001128ec] put_cred_rcu+0xec/0x140
  Call Trace:
  [c00000000175f7b0] [c00000000175f800] init_thread_union+0x3800/0x4000 (unreliable)
  [c00000000175f840] [c0000000001128ec] put_cred_rcu+0xec/0x140
  [c00000000175f8b0] [c00000000017a908] rcu_process_callbacks+0x438/0x6a0
  [c00000000175f980] [c0000000000e5e28] __do_softirq+0x198/0x310
  [c00000000175fa70] [c0000000000e6248] irq_exit+0xf8/0x140
  [c00000000175fa90] [c000000000023710] timer_interrupt+0xa0/0xe0
  [c00000000175fac0] [c000000000008fcc] decrementer_common+0x11c/0x120
  --- interrupt: 901 at replay_interrupt_return+0x0/0x4
      LR = arch_local_irq_restore.part.1+0x84/0xb0
  [c00000000175fdb0] [c00000000175c000] init_thread_union+0x0/0x4000 (unreliable)
  [c00000000175fdd0] [c00000000001cde0] arch_cpu_idle+0xe0/0x140
  [c00000000175fe00] [c0000000007f4f44] default_idle_call+0x44/0x84
  [c00000000175fe20] [c000000000142e54] do_idle+0x254/0x320
  [c00000000175fe90] [c000000000143280] cpu_startup_entry+0x30/0x40
  [c00000000175fec0] [c00000000000d2d8] rest_init+0x2f8/0x320
  [c00000000175ff00] [c000000001000d74] start_kernel+0x510/0x52c
  [c00000000175ff90] [c00000000000ab70] start_here_common+0x1c/0x4ac
  Instruction dump:
  60000000 e9230008 71280100 40820170 2fbf0000 419e013c 3d420008 394a7258 
  7bbe8502 7bc93664 ebca0000 7fde4a14 <e93e0020> 712a0001 40820234 e93f0008 
  [    1.217346467,0] OPAL: Reboot requested due to Platform error.
  [    1.217351582,3] OPAL: failed to log an error
  [    1.217358188,5] OPAL: Reboot request...


I'll just drop it for now.

cheers

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

[Nicholas Piggin]

On Wed, 30 Aug 2017 21:25:59 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > 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(-)  
> 
> This is blowing up for me on mambo:

Oh this is a known bug in mambo that does not match the hardware.

You need >= Mambo.7.8.21, or this firmware patch to work around
the issue for old mambos.

mambo.git 58d3162f4d6204fc077ff4a6ba47e4d1e19d5120

https://lists.ozlabs.org/pipermail/skiboot/2017-August/008768.html

Thanks,
Nick

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

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:

> On Wed, 30 Aug 2017 21:25:59 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
>> Nicholas Piggin <npiggin@gmail.com> writes:
>> 
>> > 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(-)  
>> 
>> This is blowing up for me on mambo:
>
> Oh this is a known bug in mambo that does not match the hardware.
>
> You need >= Mambo.7.8.21, or this firmware patch to work around
> the issue for old mambos.

As discussed elsewhere this still breaks on new mambo with more than one
CPU. So I've dropped it for now.

cheers

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

[Nicholas Piggin]

On Fri, 01 Sep 2017 19:39:41 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > On Wed, 30 Aug 2017 21:25:59 +1000
> > Michael Ellerman <mpe@ellerman.id.au> wrote:
> >  
> >> Nicholas Piggin <npiggin@gmail.com> writes:
> >>   
> >> > 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(-)    
> >> 
> >> This is blowing up for me on mambo:  
> >
> > Oh this is a known bug in mambo that does not match the hardware.
> >
> > You need >= Mambo.7.8.21, or this firmware patch to work around
> > the issue for old mambos.  
> 
> As discussed elsewhere this still breaks on new mambo with more than one
> CPU. So I've dropped it for now.

This now seems to be properly fixed in mambo with commit 11783550ee11.

Skiboot also has a patch merged which disables the problematic state on
mambo so older versions won't crash.

d2a24406a49 ("idle: disable stop*_lite POWER9 idle states for Mambo platform")

So this could be re-applied now.

Thanks,
Nick