[PATCH 0/2] powerpc: powernv: Enable stop4 via cpuidle

[PATCH 0/2] powerpc: powernv: Enable stop4 via cpuidle

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

Hi,

The stop4 idle state on POWER9 is a deep idle state which loses
hypervisor resources, but whose latency is low enough that it can be
exposed via cpuidle.

Until now, the deep idle states which lose hypervisor resources (eg:
winkle) were only exposed via CPU-Hotplug.  Hence currently on wakeup
from such states, barring a few SPRs which need to be restored to
their older value, rest of the SPRS are reinitialized to their values
corresponding to that at boot time. When stop4 is used in the context
of cpuidle, we want these additional SPRs to be restored to their
older value, to ensure that the context on the CPU coming back from
idle is same as it was before going idle.

Additionally, the CPU which is in stop4 while idling can be woken up
by the decrementer interrupts. So we need to ensure that the LPCR is
programmed with PECE1 bit cleared via the stop-api only for the
CPU-Hotplug case and not for cpuidle.

The two patches in the series address this problem.

Gautham R. Shenoy (2):
  powernv/powerpc: Save/Restore additional SPRs for stop4 cpuidle
  powernv/powerpc: Clear PECE1 in LPCR via stop-api only on Hotplug

 arch/powerpc/include/asm/paca.h       |  7 ++++++
 arch/powerpc/kernel/asm-offsets.c     | 12 +++++++++
 arch/powerpc/kernel/idle_book3s.S     | 46 +++++++++++++++++++++++++++++++++--
 arch/powerpc/platforms/powernv/idle.c | 12 ++++++++-
 arch/powerpc/platforms/powernv/smp.c  | 28 ++++++++++++++++++---
 5 files changed, 98 insertions(+), 7 deletions(-)

-- 
1.9.4

[PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for stop4 cpuidle

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

The stop4 idle state on POWER9 is a deep idle state which loses
hypervisor resources, but whose latency is low enough that it can be
exposed via cpuidle.

Until now, the deep idle states which lose hypervisor resources (eg:
winkle) were only exposed via CPU-Hotplug.  Hence currently on wakeup
from such states, barring a few SPRs which need to be restored to
their older value, rest of the SPRS are reinitialized to their values
corresponding to that at boot time.

When stop4 is used in the context of cpuidle, we want these additional
SPRs to be restored to their older value, to ensure that the context
on the CPU coming back from idle is same as it was before going idle.

In this patch, we define a SPR save area in PACA (since we have used
up the volatile register space in the stack) and on POWER9, we restore
SPRN_PID, SPRN_LDBAR, SPRN_FSCR, SPRN_HFSCR, SPRN_MMCRA, SPRN_MMCR1,
SPRN_MMCR2 to the values they had before entering stop.

Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/paca.h   |  7 ++++++
 arch/powerpc/kernel/asm-offsets.c | 12 ++++++++++
 arch/powerpc/kernel/idle_book3s.S | 46 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index dc88a31..a6b9ea6 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -48,6 +48,7 @@
 #define get_lppaca()	(get_paca()->lppaca_ptr)
 #define get_slb_shadow()	(get_paca()->slb_shadow_ptr)
 
+#define MAX_STOP_SPRS     7
 struct task_struct;
 
 /*
@@ -183,6 +184,12 @@ struct paca_struct {
 	struct paca_struct **thread_sibling_pacas;
 	/* The PSSCR value that the kernel requested before going to stop */
 	u64 requested_psscr;
+
+	/*
+	 * Save area for additional SPRs that need to be
+	 * saved/restored during cpuidle stop.
+	 */
+	u64 stop_spr_save_area[MAX_STOP_SPRS];
 #endif
 
 #ifdef CONFIG_PPC_STD_MMU_64
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index a7b5af3..0262283 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -743,6 +743,18 @@ int main(void)
 	OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
 	OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
 	OFFSET(PACA_REQ_PSSCR, paca_struct, requested_psscr);
+
+	OFFSET(PACA_PID, paca_struct, stop_spr_save_area[0]);
+	OFFSET(PACA_LDBAR, paca_struct, stop_spr_save_area[1]);
+	OFFSET(PACA_FSCR, paca_struct, stop_spr_save_area[2]);
+	OFFSET(PACA_HFSCR, paca_struct, stop_spr_save_area[3]);
+
+	/* On POWER9, we are already saving MMCR0 for ESL=EC=1 */
+	OFFSET(PACA_MMCRA, paca_struct, stop_spr_save_area[4]);
+	OFFSET(PACA_MMCR1, paca_struct, stop_spr_save_area[5]);
+	OFFSET(PACA_MMCR2, paca_struct, stop_spr_save_area[6]);
+
+
 #endif
 
 	DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER);
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index 5adb390e..7083f63 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -84,7 +84,50 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_ARCH_300)
 	std	r3,_WORT(r1)
 	mfspr	r3,SPRN_WORC
 	std	r3,_WORC(r1)
+BEGIN_FTR_SECTION
+	blr
+END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
+	/* Save additional SPRs for POWER9 */
+power9_save_additional_sprs:
+	mfspr	r3, SPRN_PID
+	mfspr	r4, SPRN_LDBAR
+	std	r3, PACA_PID(r13)
+	std	r4, PACA_LDBAR(r13)
+
+	mfspr	r3, SPRN_FSCR
+	mfspr	r4, SPRN_HFSCR
+	std	r3, PACA_FSCR(r13)
+	std	r4, PACA_HFSCR(r13)
+
+	mfspr	r3, SPRN_MMCRA
+	mfspr	r4, SPRN_MMCR1
+	std	r3, PACA_MMCRA(r13)
+	std	r4, PACA_MMCR1(r13)
+
+	mfspr	r3, SPRN_MMCR2
+	std	r3, PACA_MMCR2(r13)
+	blr
 
+power9_restore_additional_sprs:
+	ld	r3,_LPCR(r1)
+	ld	r4, PACA_PID(r13)
+	mtspr	SPRN_LPCR,r3
+	mtspr	SPRN_PID, r4
+
+	ld	r3, PACA_LDBAR(r13)
+	ld	r4, PACA_FSCR(r13)
+	mtspr	SPRN_LDBAR, r3
+	mtspr	SPRN_FSCR, r4
+
+	ld	r3, PACA_HFSCR(r13)
+	ld	r4, PACA_MMCRA(r13)
+	mtspr	SPRN_HFSCR, r3
+	mtspr	SPRN_MMCRA, r4
+	/* We have already restored PACA_MMCR0 */
+	ld	r3, PACA_MMCR1(r13)
+	ld	r4, PACA_MMCR2(r13)
+	mtspr	SPRN_MMCR1, r3
+	mtspr	SPRN_MMCR2, r4
 	blr
 
 /*
@@ -791,8 +834,7 @@ no_segments:
 	bctrl
 
 BEGIN_FTR_SECTION
-	ld	r4,_LPCR(r1)
-	mtspr	SPRN_LPCR,r4
+	bl 	power9_restore_additional_sprs
 END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
 hypervisor_state_restored:
 
-- 
1.9.4

[PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via stop-api only on Hotplug

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>


Currently we use the stop-api provided by the firmware to program the
SLW engine to restore the values of hypervisor resources that get lost
on deeper idle states (such as winkle). Since the deep states were
only used for CPU-Hotplug on POWER8 systems, we would program the LPCR
to have the PECE1 bit since Hotplugged CPUs shouldn't be spuriously
woken up by decrementer.

On POWER9, some of the deep platform idle states such as stop4 can be
used in cpuidle as well. In this case, we want the CPU in stop4 to be
woken up by the decrementer when some timer on the CPU expires.

In this patch, for POWER9, we program the stop-api for LPCR with PECE1
bit cleared only when we are offlining the CPU.

Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/idle.c | 12 +++++++++++-
 arch/powerpc/platforms/powernv/smp.c  | 28 ++++++++++++++++++++++++----
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 2abee07..f4a29d4 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -68,7 +68,7 @@ static int pnv_save_sprs_for_deep_states(void)
 	 * all cpus at boot. Get these reg values of current cpu and use the
 	 * same across all cpus.
 	 */
-	uint64_t lpcr_val = mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1;
+	uint64_t lpcr_val = mfspr(SPRN_LPCR);
 	uint64_t hid0_val = mfspr(SPRN_HID0);
 	uint64_t hid1_val = mfspr(SPRN_HID1);
 	uint64_t hid4_val = mfspr(SPRN_HID4);
@@ -85,6 +85,16 @@ static int pnv_save_sprs_for_deep_states(void)
 		if (rc != 0)
 			return rc;
 
+		/*
+		 * On POWER8, the only state that uses SLW engine is
+		 * winkle.  This is only used for CPU-Hotplug. So we
+		 * clear the decrementer bit from LPCR since we
+		 * don't want to be woken up on decrementer when in
+		 * winkle.
+		 */
+		if (!cpu_has_feature(CPU_FTR_ARCH_300))
+			lpcr_val = lpcr_val & ~(u64)LPCR_PECE1;
+
 		rc = opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
 		if (rc != 0)
 			return rc;
diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
index 40dae96..5f2a712 100644
--- a/arch/powerpc/platforms/powernv/smp.c
+++ b/arch/powerpc/platforms/powernv/smp.c
@@ -143,7 +143,8 @@ static void pnv_smp_cpu_kill_self(void)
 {
 	unsigned int cpu;
 	unsigned long srr1, wmask;
-
+	uint64_t lpcr_val;
+	uint64_t pir;
 	/* Standard hot unplug procedure */
 	/*
 	 * This hard disables local interurpts, ensuring we have no lazy
@@ -164,13 +165,17 @@ static void pnv_smp_cpu_kill_self(void)
 	if (cpu_has_feature(CPU_FTR_ARCH_207S))
 		wmask = SRR1_WAKEMASK_P8;
 
+	pir = get_hard_smp_processor_id(cpu);
 	/* We don't want to take decrementer interrupts while we are offline,
 	 * so clear LPCR:PECE1. We keep PECE2 (and LPCR_PECE_HVEE on P9)
 	 * enabled as to let IPIs in.
 	 */
-	mtspr(SPRN_LPCR, mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1);
+	lpcr_val = mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1;
+	mtspr(SPRN_LPCR, lpcr_val);
 
 	while (!generic_check_cpu_restart(cpu)) {
+
+
 		/*
 		 * Clear IPI flag, since we don't handle IPIs while
 		 * offline, except for those when changing micro-threading
@@ -180,8 +185,15 @@ static void pnv_smp_cpu_kill_self(void)
 		 */
 		kvmppc_set_host_ipi(cpu, 0);
 
+		/*
+		 * If the CPU gets woken up by a special wakeup,
+		 * ensure that the SLW engine sets LPCR with
+		 * decrementer bit cleared, else we will get spurious
+		 * wakeups.
+		 */
+		if (cpu_has_feature(CPU_FTR_ARCH_300))
+			opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
 		srr1 = pnv_cpu_offline(cpu);
-
 		WARN_ON(lazy_irq_pending());
 
 		/*
@@ -220,7 +232,15 @@ static void pnv_smp_cpu_kill_self(void)
 	}
 
 	/* Re-enable decrementer interrupts */
-	mtspr(SPRN_LPCR, mfspr(SPRN_LPCR) | LPCR_PECE1);
+	lpcr_val = mfspr(SPRN_LPCR) | LPCR_PECE1;
+	mtspr(SPRN_LPCR, lpcr_val);
+
+	/*
+	 * We want stop states to be woken up by decrementer for
+	 * non-hotplug cases.
+	 */
+	if (cpu_has_feature(CPU_FTR_ARCH_300))
+		opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
 	DBG("CPU%d coming online...\n", cpu);
 }
 
-- 
1.9.4

Re: [PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via stop-api only on Hotplug

[Nicholas Piggin]

Thanks for working on these patches. We really need to get this stuff
merged and tested asap :)

On Tue, 18 Jul 2017 19:58:49 +0530
"Gautham R. Shenoy" <ego@linux.vnet.ibm.com> wrote:

> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> 
> Currently we use the stop-api provided by the firmware to program the
> SLW engine to restore the values of hypervisor resources that get lost
> on deeper idle states (such as winkle). Since the deep states were
> only used for CPU-Hotplug on POWER8 systems, we would program the LPCR
> to have the PECE1 bit since Hotplugged CPUs shouldn't be spuriously
> woken up by decrementer.
> 
> On POWER9, some of the deep platform idle states such as stop4 can be
> used in cpuidle as well. In this case, we want the CPU in stop4 to be
> woken up by the decrementer when some timer on the CPU expires.
> 
> In this patch, for POWER9, we program the stop-api for LPCR with PECE1
> bit cleared only when we are offlining the CPU.
> 
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/idle.c | 12 +++++++++++-
>  arch/powerpc/platforms/powernv/smp.c  | 28 ++++++++++++++++++++++++----
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
> index 2abee07..f4a29d4 100644
> --- a/arch/powerpc/platforms/powernv/idle.c
> +++ b/arch/powerpc/platforms/powernv/idle.c
> @@ -68,7 +68,7 @@ static int pnv_save_sprs_for_deep_states(void)
>  	 * all cpus at boot. Get these reg values of current cpu and use the
>  	 * same across all cpus.
>  	 */
> -	uint64_t lpcr_val = mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1;
> +	uint64_t lpcr_val = mfspr(SPRN_LPCR);
>  	uint64_t hid0_val = mfspr(SPRN_HID0);
>  	uint64_t hid1_val = mfspr(SPRN_HID1);
>  	uint64_t hid4_val = mfspr(SPRN_HID4);
> @@ -85,6 +85,16 @@ static int pnv_save_sprs_for_deep_states(void)
>  		if (rc != 0)
>  			return rc;
>  
> +		/*
> +		 * On POWER8, the only state that uses SLW engine is
> +		 * winkle.  This is only used for CPU-Hotplug. So we
> +		 * clear the decrementer bit from LPCR since we
> +		 * don't want to be woken up on decrementer when in
> +		 * winkle.
> +		 */
> +		if (!cpu_has_feature(CPU_FTR_ARCH_300))
> +			lpcr_val = lpcr_val & ~(u64)LPCR_PECE1;
> +
>  		rc = opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
>  		if (rc != 0)
>  			return rc;
> diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
> index 40dae96..5f2a712 100644
> --- a/arch/powerpc/platforms/powernv/smp.c
> +++ b/arch/powerpc/platforms/powernv/smp.c
> @@ -143,7 +143,8 @@ static void pnv_smp_cpu_kill_self(void)
>  {
>  	unsigned int cpu;
>  	unsigned long srr1, wmask;
> -
> +	uint64_t lpcr_val;
> +	uint64_t pir;
>  	/* Standard hot unplug procedure */
>  	/*
>  	 * This hard disables local interurpts, ensuring we have no lazy
> @@ -164,13 +165,17 @@ static void pnv_smp_cpu_kill_self(void)
>  	if (cpu_has_feature(CPU_FTR_ARCH_207S))
>  		wmask = SRR1_WAKEMASK_P8;
>  
> +	pir = get_hard_smp_processor_id(cpu);
>  	/* We don't want to take decrementer interrupts while we are offline,
>  	 * so clear LPCR:PECE1. We keep PECE2 (and LPCR_PECE_HVEE on P9)
>  	 * enabled as to let IPIs in.
>  	 */
> -	mtspr(SPRN_LPCR, mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1);
> +	lpcr_val = mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1;
> +	mtspr(SPRN_LPCR, lpcr_val);
>  
>  	while (!generic_check_cpu_restart(cpu)) {
> +
> +
>  		/*
>  		 * Clear IPI flag, since we don't handle IPIs while
>  		 * offline, except for those when changing micro-threading
> @@ -180,8 +185,15 @@ static void pnv_smp_cpu_kill_self(void)
>  		 */
>  		kvmppc_set_host_ipi(cpu, 0);
>  
> +		/*
> +		 * If the CPU gets woken up by a special wakeup,
> +		 * ensure that the SLW engine sets LPCR with
> +		 * decrementer bit cleared, else we will get spurious
> +		 * wakeups.
> +		 */
> +		if (cpu_has_feature(CPU_FTR_ARCH_300))
> +			opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);

Can you put these details into pnv_cpu_offline? Possibly even from there
into another special SPR save function? E.g.,

  pnv_save_sprs_for_deep_state_decrementer_wakeup(bool decrementer_wakeup)

I'd like to put the LPCR manipulation for idle wake settings into idle.c
as well (pnv_cpu_offline), I think it fits better in there.

Thanks,
Nick

Re: [PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via stop-api only on Hotplug

[Gautham R Shenoy]

Hello Nicholas,

On Wed, Jul 19, 2017 at 12:14:12PM +1000, Nicholas Piggin wrote:
> Thanks for working on these patches. We really need to get this stuff
> merged and tested asap :)
>

> On Tue, 18 Jul 2017 19:58:49 +0530

[..snip..]

> > diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
> > index 40dae96..5f2a712 100644
> > --- a/arch/powerpc/platforms/powernv/smp.c
> > +++ b/arch/powerpc/platforms/powernv/smp.c
> > @@ -143,7 +143,8 @@ static void pnv_smp_cpu_kill_self(void)
> >  {
> >  	unsigned int cpu;
> >  	unsigned long srr1, wmask;
> > -
> > +	uint64_t lpcr_val;
> > +	uint64_t pir;
> >  	/* Standard hot unplug procedure */
> >  	/*
> >  	 * This hard disables local interurpts, ensuring we have no lazy
> > @@ -164,13 +165,17 @@ static void pnv_smp_cpu_kill_self(void)
> >  	if (cpu_has_feature(CPU_FTR_ARCH_207S))
> >  		wmask = SRR1_WAKEMASK_P8;
> >  
> > +	pir = get_hard_smp_processor_id(cpu);
> >  	/* We don't want to take decrementer interrupts while we are offline,
> >  	 * so clear LPCR:PECE1. We keep PECE2 (and LPCR_PECE_HVEE on P9)
> >  	 * enabled as to let IPIs in.
> >  	 */
> > -	mtspr(SPRN_LPCR, mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1);
> > +	lpcr_val = mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1;
> > +	mtspr(SPRN_LPCR, lpcr_val);
> >  
> >  	while (!generic_check_cpu_restart(cpu)) {
> > +
> > +
> >  		/*
> >  		 * Clear IPI flag, since we don't handle IPIs while
> >  		 * offline, except for those when changing micro-threading
> > @@ -180,8 +185,15 @@ static void pnv_smp_cpu_kill_self(void)
> >  		 */
> >  		kvmppc_set_host_ipi(cpu, 0);
> >  
> > +		/*
> > +		 * If the CPU gets woken up by a special wakeup,
> > +		 * ensure that the SLW engine sets LPCR with
> > +		 * decrementer bit cleared, else we will get spurious
> > +		 * wakeups.
> > +		 */
> > +		if (cpu_has_feature(CPU_FTR_ARCH_300))
> > +			opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
> 
> Can you put these details into pnv_cpu_offline? Possibly even from there
> into another special SPR save function? E.g.,

We could move this to pnv_cpu_offline. Except that if we do get
spurious interrupts, we will end up programming the the LPCR via
stop-api again which is not needed.

Even this patch above can be optimized further. We need to program the
LPCR with the PECE1 bit cleared only once, before the while loop, and
once again program the LPCR with PECE1 bit set once we are out of the
while loop.

But then, perhaps getting spurious interrupts when the CPU is
hotplugged is an unlikely event. So I will move this to pnv_cpu_offline.

> 
>   pnv_save_sprs_for_deep_state_decrementer_wakeup(bool decrementer_wakeup)
> 
> I'd like to put the LPCR manipulation for idle wake settings into idle.c
> as well (pnv_cpu_offline), I think it fits better in there.
>

I agree. Will respin this,test and send out the v2.

> Thanks,

Thanks for the review.
> Nick
> 

--
Thanks and Regards
gautham.

Re: [PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via stop-api only on Hotplug

[Nicholas Piggin]

On Wed, 19 Jul 2017 10:34:59 +0530
Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:

> Hello Nicholas,
> 
> On Wed, Jul 19, 2017 at 12:14:12PM +1000, Nicholas Piggin wrote:
> > Thanks for working on these patches. We really need to get this stuff
> > merged and tested asap :)
> >  
> 
> > On Tue, 18 Jul 2017 19:58:49 +0530  
> 
> [..snip..]
> 
> > > diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
> > > index 40dae96..5f2a712 100644
> > > --- a/arch/powerpc/platforms/powernv/smp.c
> > > +++ b/arch/powerpc/platforms/powernv/smp.c
> > > @@ -143,7 +143,8 @@ static void pnv_smp_cpu_kill_self(void)
> > >  {
> > >  	unsigned int cpu;
> > >  	unsigned long srr1, wmask;
> > > -
> > > +	uint64_t lpcr_val;
> > > +	uint64_t pir;
> > >  	/* Standard hot unplug procedure */
> > >  	/*
> > >  	 * This hard disables local interurpts, ensuring we have no lazy
> > > @@ -164,13 +165,17 @@ static void pnv_smp_cpu_kill_self(void)
> > >  	if (cpu_has_feature(CPU_FTR_ARCH_207S))
> > >  		wmask = SRR1_WAKEMASK_P8;
> > >  
> > > +	pir = get_hard_smp_processor_id(cpu);
> > >  	/* We don't want to take decrementer interrupts while we are offline,
> > >  	 * so clear LPCR:PECE1. We keep PECE2 (and LPCR_PECE_HVEE on P9)
> > >  	 * enabled as to let IPIs in.
> > >  	 */
> > > -	mtspr(SPRN_LPCR, mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1);
> > > +	lpcr_val = mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1;
> > > +	mtspr(SPRN_LPCR, lpcr_val);
> > >  
> > >  	while (!generic_check_cpu_restart(cpu)) {
> > > +
> > > +
> > >  		/*
> > >  		 * Clear IPI flag, since we don't handle IPIs while
> > >  		 * offline, except for those when changing micro-threading
> > > @@ -180,8 +185,15 @@ static void pnv_smp_cpu_kill_self(void)
> > >  		 */
> > >  		kvmppc_set_host_ipi(cpu, 0);
> > >  
> > > +		/*
> > > +		 * If the CPU gets woken up by a special wakeup,
> > > +		 * ensure that the SLW engine sets LPCR with
> > > +		 * decrementer bit cleared, else we will get spurious
> > > +		 * wakeups.
> > > +		 */
> > > +		if (cpu_has_feature(CPU_FTR_ARCH_300))
> > > +			opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);  
> > 
> > Can you put these details into pnv_cpu_offline? Possibly even from there
> > into another special SPR save function? E.g.,  
> 
> We could move this to pnv_cpu_offline. Except that if we do get
> spurious interrupts, we will end up programming the the LPCR via
> stop-api again which is not needed.
> 
> Even this patch above can be optimized further. We need to program the
> LPCR with the PECE1 bit cleared only once, before the while loop, and
> once again program the LPCR with PECE1 bit set once we are out of the
> while loop.
> 
> But then, perhaps getting spurious interrupts when the CPU is
> hotplugged is an unlikely event. So I will move this to pnv_cpu_offline.

Yeah, I think it just tidies it up a bit. You're right, but as you say
it's not really a critical path.


> >   pnv_save_sprs_for_deep_state_decrementer_wakeup(bool decrementer_wakeup)
> > 
> > I'd like to put the LPCR manipulation for idle wake settings into idle.c
> > as well (pnv_cpu_offline), I think it fits better in there.
> >  
> 
> I agree. Will respin this,test and send out the v2.

Great thanks. The first patch seemed okay to me.

I wonder if we should think about a more structured kernel API for
modifying these kind of system registers so we always have the
up-to-date values stored in memory. Many of them do need to be
restored after sleep, but they don't need to be saved per-thread
or saved every time we go to sleep.

That's far more work of course, but it might be something we want
to think about for the future.

Thanks,
Nick

Re: [PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via stop-api only on Hotplug

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:
...
> I wonder if we should think about a more structured kernel API for
> modifying these kind of system registers so we always have the
> up-to-date values stored in memory. Many of them do need to be
> restored after sleep, but they don't need to be saved per-thread
> or saved every time we go to sleep.

Yes that's on my mental TODO. Paul and I have talked about it in the
past for KVM.

I'll write it down sometime so we at least remember :D

chers