diff mbox

arm: vexpress: tc2: fix CPU hotplug and CPU idle race on cluster power down

Message ID 1380292153-10480-1-git-send-email-lorenzo.pieralisi@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lorenzo Pieralisi Sept. 27, 2013, 2:29 p.m. UTC 
On the TC2 testchip, when all CPUs in a cluster enter standbywfi and
commit a power down request, the power controller will wait for
standbywfil2 coming from L2 cache controller to shut the cluster down.
By the time all CPUs in a cluster commit a power down request and enter wfi,
the power controller cannot backtrack, or put it another way, a CPU must
not be allowed to complete execution independently of the power
controller, the only way for it to resume properly must be upon wake-up IRQ
pending and subsequent reset triggered from the power controller.

Current MCPM back-end for TC2 disables the GIC CPU IF only when power
down is committed through the suspend method, that makes sense since a
suspended CPU is still online and can receive interrupts whereas a
hotplugged CPU, since it is offline, migrated all IRQs and shutdown
the per-CPU peripherals, hence their PPIs.

The flaw with this reasoning is the following. If all CPUs in a clusters are
entering a power down state either through CPU idle or CPU hotplug, when the
last man successfully completes the MCPM power down sequence (and executes
wfi), power controller waits for L2 wfi signal to quiesce the cluster and shut
it down. If, when all CPUs are sitting in wfi, an online CPU hotplugs back in
one of the CPUs in the cluster being shutdown, that CPU receives an IPI that
causes wfi to complete (since MCPM power down method does not disable the GIC
CPU IF in that case - CPU being hotplugged out, not idle) and the power
controller will never see the stanbywfil2 signal coming from L2 that is
required for shutdown to happen and the system deadlocks.

The only solution to this problem consists in disabling the GIC CPU IF
on a CPU committed to power down regardless of the power down entry
method (CPU hotplug or CPU idle). This way, CPU wake-up is under power
controller control, which prevents unexpected wfi exit caused by a
pending IRQ.

This patch moves the GIC CPU IF disable call in the TC2 MCPM implementation
from the suspend method to the power down method to fix the mentioned
race condition.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
---
 arch/arm/mach-vexpress/tc2_pm.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Dave Martin Sept. 27, 2013, 3:10 p.m. UTC | #1 
On Fri, Sep 27, 2013 at 03:29:13PM +0100, Lorenzo Pieralisi wrote:
> On the TC2 testchip, when all CPUs in a cluster enter standbywfi and
> commit a power down request, the power controller will wait for

[...]

(Minor nit: please wrap the commit message to a shorter length for git
log.  I use 67, but I can't remember where that recommendation came from)

[...]

> This patch moves the GIC CPU IF disable call in the TC2 MCPM implementation
> from the suspend method to the power down method to fix the mentioned
> race condition.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Tested-by: Dave Martn <Dave.Martin@arm.com> (for kexec)

It's worth briefly summarising the kexec scenario too:

kexec hotplugs secondary CPUs out during kernel reload/restart.
Because kexec may (deliberately) trash the old kernel text, it is
not OK for CPUs to follow the MCPM soft reboot path, since
instructions after the WFI may have been replaced by kexec.

If tc2_pm_down() does not disable the GIC cpu interface, there is a
race between CPU powerdown in the old kernel and the IPI from the
new kernel that triggers secondary boot, particluarly if the
powerdown is slow (due to L2 cache cleaning for example).  If the
new kernel wins the race, the affected CPU(s) will not really be
reset and may execute garbage after the WFI.

(some comments below)

> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> ---
>  arch/arm/mach-vexpress/tc2_pm.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
> index 7aeb5d6..e6eb481 100644
> --- a/arch/arm/mach-vexpress/tc2_pm.c
> +++ b/arch/arm/mach-vexpress/tc2_pm.c
> @@ -131,6 +131,16 @@ static void tc2_pm_down(u64 residency)
>  	} else
>  		BUG();
>  
> +	/*
> +	 * If the CPU is committed to power down, make sure
> +	 * the power controller will be in charge of waking it
> +	 * up upon IRQ, ie IRQ lines are cut from GIC CPU IF
> +	 * to the CPU by disabling the GIC CPU IF to prevent wfi
> +	 * from completing execution behind power controller back
> +	 */
> +	if (!skip_wfi)
> +		gic_cpu_if_down();
> +

In my version of this fix, I disabled the CPU interface much earlier,
just after entering the function.  I think it probably works either
way.  Do you think the location is critical, or should it not matter?

As far as I could tell, it matters only that this is done sometime
between committing to WFI and actually doing it, but if you think
there are extra requirements then I would like to understand them.

[...]

Cheers
---Dave
Lorenzo Pieralisi Sept. 27, 2013, 3:27 p.m. UTC | #2 
On Fri, Sep 27, 2013 at 04:10:49PM +0100, Dave Martin wrote:
> On Fri, Sep 27, 2013 at 03:29:13PM +0100, Lorenzo Pieralisi wrote:
> > On the TC2 testchip, when all CPUs in a cluster enter standbywfi and
> > commit a power down request, the power controller will wait for
> 
> [...]
> 
> (Minor nit: please wrap the commit message to a shorter length for git
> log.  I use 67, but I can't remember where that recommendation came from)

Ok, will do.

> [...]
> 
> > This patch moves the GIC CPU IF disable call in the TC2 MCPM implementation
> > from the suspend method to the power down method to fix the mentioned
> > race condition.
> 
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> Tested-by: Dave Martn <Dave.Martin@arm.com> (for kexec)

Thanks !

> It's worth briefly summarising the kexec scenario too:
> 
> kexec hotplugs secondary CPUs out during kernel reload/restart.
> Because kexec may (deliberately) trash the old kernel text, it is
> not OK for CPUs to follow the MCPM soft reboot path, since
> instructions after the WFI may have been replaced by kexec.
> 
> If tc2_pm_down() does not disable the GIC cpu interface, there is a
> race between CPU powerdown in the old kernel and the IPI from the
> new kernel that triggers secondary boot, particluarly if the
> powerdown is slow (due to L2 cache cleaning for example).  If the
> new kernel wins the race, the affected CPU(s) will not really be
> reset and may execute garbage after the WFI.
> 
> (some comments below)
> 
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> > ---
> >  arch/arm/mach-vexpress/tc2_pm.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
> > index 7aeb5d6..e6eb481 100644
> > --- a/arch/arm/mach-vexpress/tc2_pm.c
> > +++ b/arch/arm/mach-vexpress/tc2_pm.c
> > @@ -131,6 +131,16 @@ static void tc2_pm_down(u64 residency)
> >  	} else
> >  		BUG();
> >  
> > +	/*
> > +	 * If the CPU is committed to power down, make sure
> > +	 * the power controller will be in charge of waking it
> > +	 * up upon IRQ, ie IRQ lines are cut from GIC CPU IF
> > +	 * to the CPU by disabling the GIC CPU IF to prevent wfi
> > +	 * from completing execution behind power controller back
> > +	 */
> > +	if (!skip_wfi)
> > +		gic_cpu_if_down();
> > +
> 
> In my version of this fix, I disabled the CPU interface much earlier,
> just after entering the function.  I think it probably works either
> way.  Do you think the location is critical, or should it not matter?
> 
> As far as I could tell, it matters only that this is done sometime
> between committing to WFI and actually doing it, but if you think
> there are extra requirements then I would like to understand them.

You are right, it can be added anywhere before executing wfi; I just added it
there for two reasons:

- MCPM checked if wfi should be skipped. If wfi should be skipped disabling
  GIC CPU IF is useless
- I want to issue the write to the GIC before we enter the no-fly zone
  (disable C-bit, clean cache, exit SMP (and disable CCI)). I know we
  could disable the GIC CPU IF right before executing wfi, but let's not
  play with fire, if that write hides a barrier we are in a bind.

Lorenzo
diff mbox

Patch

diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
index 7aeb5d6..e6eb481 100644
--- a/arch/arm/mach-vexpress/tc2_pm.c
+++ b/arch/arm/mach-vexpress/tc2_pm.c
@@ -131,6 +131,16 @@  static void tc2_pm_down(u64 residency)
 	} else
 		BUG();
 
+	/*
+	 * If the CPU is committed to power down, make sure
+	 * the power controller will be in charge of waking it
+	 * up upon IRQ, ie IRQ lines are cut from GIC CPU IF
+	 * to the CPU by disabling the GIC CPU IF to prevent wfi
+	 * from completing execution behind power controller back
+	 */
+	if (!skip_wfi)
+		gic_cpu_if_down();
+
 	if (last_man && __mcpm_outbound_enter_critical(cpu, cluster)) {
 		arch_spin_unlock(&tc2_pm_lock);
 
@@ -231,7 +241,6 @@  static void tc2_pm_suspend(u64 residency)
 	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
 	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
 	ve_spc_set_resume_addr(cluster, cpu, virt_to_phys(mcpm_entry_point));
-	gic_cpu_if_down();
 	tc2_pm_down(residency);
 }