Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1690319 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via stop-api only on Hotplug "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> - 2017-07-18 16:30 +0200
    Re: [PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via stop-api  only on Hotplug Nicholas Piggin <npiggin@gmail.com> - 2017-07-19 04:20 +0200
      Re: [PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via stop-api  only on Hotplug Gautham R Shenoy <ego@linux.vnet.ibm.com> - 2017-07-19 07:10 +0200
        Re: [PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via stop-api  only on Hotplug Nicholas Piggin <npiggin@gmail.com> - 2017-07-19 07:30 +0200
          Re: [PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via stop-api only on Hotplug Michael Ellerman <mpe@ellerman.id.au> - 2017-07-25 12:50 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1695604

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web