[PATCH] arm: versatile: don't mark pen as __INIT

[PATCH] arm: versatile: don't mark pen as __INIT

[Mark Rutland]

When booting fewer cores than are physically present on a versatile
platform (e.g. when passing maxcpus=N on the command line), some
secondary cores may remain in the holding pen, which is marked __INIT.
Late in the boot process, the memory comprising the holding pen will be
released to the kernel for more general use, and may be overwritten with
arbitrary data, which can cause the held secondaries to start behaving
unpredictably. This can lead to all manner of odd behaviour from the
kernel.

Instead don't mark the section as __INIT. This means we can't reuse the
pen memory, but we won't get secondaries corrupting the rest of the
kernel.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Pawel Moll <pawel.moll@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 arch/arm/plat-versatile/headsmp.S | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/plat-versatile/headsmp.S b/arch/arm/plat-versatile/headsmp.S
index b178d44..2677bc3 100644
--- a/arch/arm/plat-versatile/headsmp.S
+++ b/arch/arm/plat-versatile/headsmp.S
@@ -11,8 +11,6 @@
 #include <linux/linkage.h>
 #include <linux/init.h>
 
-	__INIT
-
 /*
  * Realview/Versatile Express specific entry point for secondary CPUs.
  * This provides a "holding pen" into which all secondary cores are held
-- 
1.8.1.1

[PATCH] arm: versatile: don't mark pen as __INIT

[Jonathan Austin]

On 10/06/13 16:07, Mark Rutland wrote:
> When booting fewer cores than are physically present on a versatile
> platform (e.g. when passing maxcpus=N on the command line), some
> secondary cores may remain in the holding pen, which is marked __INIT.
> Late in the boot process, the memory comprising the holding pen will be
> released to the kernel for more general use, and may be overwritten with
> arbitrary data, which can cause the held secondaries to start behaving
> unpredictably. This can lead to all manner of odd behaviour from the
> kernel.
>
> Instead don't mark the section as __INIT. This means we can't reuse the
> pen memory, but we won't get secondaries corrupting the rest of the
> kernel.
>
Thanks for the patch, Mark,

I got bitten by this today booting Russell's devel-stable branch (with 4 
CPUs). The corruption you get can lead you up the garden path debugging 
something totally different!

Does this need to be applied to stable kernels, too?

Jonny

[PATCH] arm: versatile: don't mark pen as __INIT

[Stephen Boyd]

On 06/10/13 08:07, Mark Rutland wrote:
> When booting fewer cores than are physically present on a versatile
> platform (e.g. when passing maxcpus=N on the command line), some
> secondary cores may remain in the holding pen, which is marked __INIT.
> Late in the boot process, the memory comprising the holding pen will be
> released to the kernel for more general use, and may be overwritten with
> arbitrary data, which can cause the held secondaries to start behaving
> unpredictably. This can lead to all manner of odd behaviour from the
> kernel.
>
> Instead don't mark the section as __INIT. This means we can't reuse the
> pen memory, but we won't get secondaries corrupting the rest of the
> kernel.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Acked-by: Pawel Moll <pawel.moll@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>  arch/arm/plat-versatile/headsmp.S | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/arch/arm/plat-versatile/headsmp.S b/arch/arm/plat-versatile/headsmp.S
> index b178d44..2677bc3 100644
> --- a/arch/arm/plat-versatile/headsmp.S
> +++ b/arch/arm/plat-versatile/headsmp.S
> @@ -11,8 +11,6 @@
>  #include <linux/linkage.h>
>  #include <linux/init.h>
>  
> -	__INIT
> -
>  /*
>   * Realview/Versatile Express specific entry point for secondary CPUs.
>   * This provides a "holding pen" into which all secondary cores are held

Why doesn't __CPUINIT work?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] arm: versatile: don't mark pen as __INIT

[Mark Rutland]

On Mon, Jun 10, 2013 at 07:39:27PM +0100, Stephen Boyd wrote:
> On 06/10/13 08:07, Mark Rutland wrote:
> > When booting fewer cores than are physically present on a versatile
> > platform (e.g. when passing maxcpus=N on the command line), some
> > secondary cores may remain in the holding pen, which is marked __INIT.
> > Late in the boot process, the memory comprising the holding pen will be
> > released to the kernel for more general use, and may be overwritten with
> > arbitrary data, which can cause the held secondaries to start behaving
> > unpredictably. This can lead to all manner of odd behaviour from the
> > kernel.
> >
> > Instead don't mark the section as __INIT. This means we can't reuse the
> > pen memory, but we won't get secondaries corrupting the rest of the
> > kernel.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Acked-by: Pawel Moll <pawel.moll@arm.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> >  arch/arm/plat-versatile/headsmp.S | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/arch/arm/plat-versatile/headsmp.S b/arch/arm/plat-versatile/headsmp.S
> > index b178d44..2677bc3 100644
> > --- a/arch/arm/plat-versatile/headsmp.S
> > +++ b/arch/arm/plat-versatile/headsmp.S
> > @@ -11,8 +11,6 @@
> >  #include <linux/linkage.h>
> >  #include <linux/init.h>
> >  
> > -	__INIT
> > -
> >  /*
> >   * Realview/Versatile Express specific entry point for secondary CPUs.
> >   * This provides a "holding pen" into which all secondary cores are held
> 
> Why doesn't __CPUINIT work?

Won't we then encounter the same problem on builds without CPU_HOTPLUG? I
thought we'd throw away the .cpuinit.* section(s) in that case?

Thanks,
Mark.

[PATCH] arm: versatile: don't mark pen as __INIT

[Stephen Boyd]

On 06/10/13 11:52, Mark Rutland wrote:
> On Mon, Jun 10, 2013 at 07:39:27PM +0100, Stephen Boyd wrote:
>> On 06/10/13 08:07, Mark Rutland wrote:
>>> When booting fewer cores than are physically present on a versatile
>>> platform (e.g. when passing maxcpus=N on the command line), some
>>> secondary cores may remain in the holding pen, which is marked __INIT.
>>> Late in the boot process, the memory comprising the holding pen will be
>>> released to the kernel for more general use, and may be overwritten with
>>> arbitrary data, which can cause the held secondaries to start behaving
>>> unpredictably. This can lead to all manner of odd behaviour from the
>>> kernel.
>>>
>>> Instead don't mark the section as __INIT. This means we can't reuse the
>>> pen memory, but we won't get secondaries corrupting the rest of the
>>> kernel.
>>>
>>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>>> Acked-by: Pawel Moll <pawel.moll@arm.com>
>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> ---
>>>  arch/arm/plat-versatile/headsmp.S | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-versatile/headsmp.S b/arch/arm/plat-versatile/headsmp.S
>>> index b178d44..2677bc3 100644
>>> --- a/arch/arm/plat-versatile/headsmp.S
>>> +++ b/arch/arm/plat-versatile/headsmp.S
>>> @@ -11,8 +11,6 @@
>>>  #include <linux/linkage.h>
>>>  #include <linux/init.h>
>>>  
>>> -	__INIT
>>> -
>>>  /*
>>>   * Realview/Versatile Express specific entry point for secondary CPUs.
>>>   * This provides a "holding pen" into which all secondary cores are held
>> Why doesn't __CPUINIT work?
> Won't we then encounter the same problem on builds without CPU_HOTPLUG? I
> thought we'd throw away the .cpuinit.* section(s) in that case?
>

The generic linker macros look to set it up so that all __CPUINIT
sections become __INIT in that scenario. Since we don't support hotplug
booting with maxcpus < nr_present_cpus can't lead to any corruption
because we can't bring online any of the offline and present CPUs.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] arm: versatile: don't mark pen as __INIT

[Stephen Boyd]

On 06/10/13 12:09, Stephen Boyd wrote:
> On 06/10/13 11:52, Mark Rutland wrote:
>> On Mon, Jun 10, 2013 at 07:39:27PM +0100, Stephen Boyd wrote:
>>> On 06/10/13 08:07, Mark Rutland wrote:
>>>> When booting fewer cores than are physically present on a versatile
>>>> platform (e.g. when passing maxcpus=N on the command line), some
>>>> secondary cores may remain in the holding pen, which is marked __INIT.
>>>> Late in the boot process, the memory comprising the holding pen will be
>>>> released to the kernel for more general use, and may be overwritten with
>>>> arbitrary data, which can cause the held secondaries to start behaving
>>>> unpredictably. This can lead to all manner of odd behaviour from the
>>>> kernel.
>>>>
>>>> Instead don't mark the section as __INIT. This means we can't reuse the
>>>> pen memory, but we won't get secondaries corrupting the rest of the
>>>> kernel.
>>>>
>>>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>>>> Acked-by: Pawel Moll <pawel.moll@arm.com>
>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>> ---
>>>>  arch/arm/plat-versatile/headsmp.S | 2 --
>>>>  1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm/plat-versatile/headsmp.S b/arch/arm/plat-versatile/headsmp.S
>>>> index b178d44..2677bc3 100644
>>>> --- a/arch/arm/plat-versatile/headsmp.S
>>>> +++ b/arch/arm/plat-versatile/headsmp.S
>>>> @@ -11,8 +11,6 @@
>>>>  #include <linux/linkage.h>
>>>>  #include <linux/init.h>
>>>>  
>>>> -	__INIT
>>>> -
>>>>  /*
>>>>   * Realview/Versatile Express specific entry point for secondary CPUs.
>>>>   * This provides a "holding pen" into which all secondary cores are held
>>> Why doesn't __CPUINIT work?
>> Won't we then encounter the same problem on builds without CPU_HOTPLUG? I
>> thought we'd throw away the .cpuinit.* section(s) in that case?
>>
> The generic linker macros look to set it up so that all __CPUINIT
> sections become __INIT in that scenario. Since we don't support hotplug
> booting with maxcpus < nr_present_cpus can't lead to any corruption
> because we can't bring online any of the offline and present CPUs.
>

Sorry I should clarify further. We can't bring online any offline and
present CPUs after the init sections are freed.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] arm: versatile: don't mark pen as __INIT

[Lorenzo Pieralisi]

On Mon, Jun 10, 2013 at 08:22:36PM +0100, Stephen Boyd wrote:
> On 06/10/13 12:09, Stephen Boyd wrote:
> > On 06/10/13 11:52, Mark Rutland wrote:
> >> On Mon, Jun 10, 2013 at 07:39:27PM +0100, Stephen Boyd wrote:
> >>> On 06/10/13 08:07, Mark Rutland wrote:
> >>>> When booting fewer cores than are physically present on a versatile
> >>>> platform (e.g. when passing maxcpus=N on the command line), some
> >>>> secondary cores may remain in the holding pen, which is marked __INIT.
> >>>> Late in the boot process, the memory comprising the holding pen will be
> >>>> released to the kernel for more general use, and may be overwritten with
> >>>> arbitrary data, which can cause the held secondaries to start behaving
> >>>> unpredictably. This can lead to all manner of odd behaviour from the
> >>>> kernel.
> >>>>
> >>>> Instead don't mark the section as __INIT. This means we can't reuse the
> >>>> pen memory, but we won't get secondaries corrupting the rest of the
> >>>> kernel.
> >>>>
> >>>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> >>>> Acked-by: Pawel Moll <pawel.moll@arm.com>
> >>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >>>> ---
> >>>>  arch/arm/plat-versatile/headsmp.S | 2 --
> >>>>  1 file changed, 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm/plat-versatile/headsmp.S b/arch/arm/plat-versatile/headsmp.S
> >>>> index b178d44..2677bc3 100644
> >>>> --- a/arch/arm/plat-versatile/headsmp.S
> >>>> +++ b/arch/arm/plat-versatile/headsmp.S
> >>>> @@ -11,8 +11,6 @@
> >>>>  #include <linux/linkage.h>
> >>>>  #include <linux/init.h>
> >>>>  
> >>>> -	__INIT
> >>>> -
> >>>>  /*
> >>>>   * Realview/Versatile Express specific entry point for secondary CPUs.
> >>>>   * This provides a "holding pen" into which all secondary cores are held
> >>> Why doesn't __CPUINIT work?
> >> Won't we then encounter the same problem on builds without CPU_HOTPLUG? I
> >> thought we'd throw away the .cpuinit.* section(s) in that case?
> >>
> > The generic linker macros look to set it up so that all __CPUINIT
> > sections become __INIT in that scenario. Since we don't support hotplug
> > booting with maxcpus < nr_present_cpus can't lead to any corruption
> > because we can't bring online any of the offline and present CPUs.
> >
> 
> Sorry I should clarify further. We can't bring online any offline and
> present CPUs after the init sections are freed.

Problem is, since now GIC broadcasts IPIs at boot to all CPUs connected
to the GIC so that they are woken up for GIC CPU IF id discovery, on
some platforms (ie versatile express, where CPUs are in wfi waiting for
an IPI, with a common jump address), they are all thrown at the kernel
waiting for the pen release value to be set to their MPIDR. Since we
want to boot fewer CPUs than the number of present ones, if we free the pen
assembly stub after boot, well, those CPUs end up executing undefined
bytes. There is no way to put those CPUs back to sleep on old versatile
platforms and probably no way to prevent them from entering the kernel upon
wfi since the jump address is set using a single register common to all
CPUs (if there was a register per-CPU, the bootloader could check its
value and put the CPU back in wfi if jump address was, say, NULL; if
there were per-CPU resets that would even be simpler).

HTH,
Lorenzo

[PATCH] arm: versatile: don't mark pen as __INIT

[Stephen Boyd]

On 06/10, Lorenzo Pieralisi wrote:
> On Mon, Jun 10, 2013 at 08:22:36PM +0100, Stephen Boyd wrote:
> > On 06/10/13 12:09, Stephen Boyd wrote:
> > > On 06/10/13 11:52, Mark Rutland wrote:
> > >> On Mon, Jun 10, 2013 at 07:39:27PM +0100, Stephen Boyd wrote:
> > >>> On 06/10/13 08:07, Mark Rutland wrote:
> > >>>> When booting fewer cores than are physically present on a versatile
> > >>>> platform (e.g. when passing maxcpus=N on the command line), some
> > >>>> secondary cores may remain in the holding pen, which is marked __INIT.
> > >>>> Late in the boot process, the memory comprising the holding pen will be
> > >>>> released to the kernel for more general use, and may be overwritten with
> > >>>> arbitrary data, which can cause the held secondaries to start behaving
> > >>>> unpredictably. This can lead to all manner of odd behaviour from the
> > >>>> kernel.
> > >>>>
> > >>>> Instead don't mark the section as __INIT. This means we can't reuse the
> > >>>> pen memory, but we won't get secondaries corrupting the rest of the
> > >>>> kernel.
> > >>>>
> > >>>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > >>>> Acked-by: Pawel Moll <pawel.moll@arm.com>
> > >>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > >>>> ---
> > >>>>  arch/arm/plat-versatile/headsmp.S | 2 --
> > >>>>  1 file changed, 2 deletions(-)
> > >>>>
> > >>>> diff --git a/arch/arm/plat-versatile/headsmp.S b/arch/arm/plat-versatile/headsmp.S
> > >>>> index b178d44..2677bc3 100644
> > >>>> --- a/arch/arm/plat-versatile/headsmp.S
> > >>>> +++ b/arch/arm/plat-versatile/headsmp.S
> > >>>> @@ -11,8 +11,6 @@
> > >>>>  #include <linux/linkage.h>
> > >>>>  #include <linux/init.h>
> > >>>>  
> > >>>> -	__INIT
> > >>>> -
> > >>>>  /*
> > >>>>   * Realview/Versatile Express specific entry point for secondary CPUs.
> > >>>>   * This provides a "holding pen" into which all secondary cores are held
> > >>> Why doesn't __CPUINIT work?
> > >> Won't we then encounter the same problem on builds without CPU_HOTPLUG? I
> > >> thought we'd throw away the .cpuinit.* section(s) in that case?
> > >>
> > > The generic linker macros look to set it up so that all __CPUINIT
> > > sections become __INIT in that scenario. Since we don't support hotplug
> > > booting with maxcpus < nr_present_cpus can't lead to any corruption
> > > because we can't bring online any of the offline and present CPUs.
> > >
> > 
> > Sorry I should clarify further. We can't bring online any offline and
> > present CPUs after the init sections are freed.
> 
> Problem is, since now GIC broadcasts IPIs at boot to all CPUs connected
> to the GIC so that they are woken up for GIC CPU IF id discovery, on
> some platforms (ie versatile express, where CPUs are in wfi waiting for
> an IPI, with a common jump address), they are all thrown at the kernel
> waiting for the pen release value to be set to their MPIDR. Since we
> want to boot fewer CPUs than the number of present ones, if we free the pen
> assembly stub after boot, well, those CPUs end up executing undefined
> bytes. There is no way to put those CPUs back to sleep on old versatile
> platforms and probably no way to prevent them from entering the kernel upon
> wfi since the jump address is set using a single register common to all
> CPUs (if there was a register per-CPU, the bootloader could check its
> value and put the CPU back in wfi if jump address was, say, NULL; if
> there were per-CPU resets that would even be simpler).
> 

Ah ok. I didn't think any of those CPUs were going to come out of
the boot monitor into the pen.

At the least please add a comment to this effect in the headsmp.S
file and/or in the commit text. It seems that if the GIC code
wasn't written that way this could be marked __CPUINIT.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] arm: versatile: don't mark pen as __INIT

[Russell King - ARM Linux]

On Mon, Jun 10, 2013 at 04:07:24PM +0100, Mark Rutland wrote:
> When booting fewer cores than are physically present on a versatile
> platform (e.g. when passing maxcpus=N on the command line), some
> secondary cores may remain in the holding pen, which is marked __INIT.
> Late in the boot process, the memory comprising the holding pen will be
> released to the kernel for more general use, and may be overwritten with
> arbitrary data, which can cause the held secondaries to start behaving
> unpredictably. This can lead to all manner of odd behaviour from the
> kernel.
> 
> Instead don't mark the section as __INIT. This means we can't reuse the
> pen memory, but we won't get secondaries corrupting the rest of the
> kernel.

__CPUINIT is appropriate here; __CPUINIT will be kept around if you have
hotplug CPU suport, but if you don't it will be discarded after all
secondary CPUs have booted.  And without hotplug CPU, you can't ask
for the offline CPUs to be onlined.

[PATCH] arm: versatile: don't mark pen as __INIT

[Mark Rutland]

On Tue, Jun 11, 2013 at 12:24:41AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 10, 2013 at 04:07:24PM +0100, Mark Rutland wrote:
> > When booting fewer cores than are physically present on a versatile
> > platform (e.g. when passing maxcpus=N on the command line), some
> > secondary cores may remain in the holding pen, which is marked __INIT.
> > Late in the boot process, the memory comprising the holding pen will be
> > released to the kernel for more general use, and may be overwritten with
> > arbitrary data, which can cause the held secondaries to start behaving
> > unpredictably. This can lead to all manner of odd behaviour from the
> > kernel.
> > 
> > Instead don't mark the section as __INIT. This means we can't reuse the
> > pen memory, but we won't get secondaries corrupting the rest of the
> > kernel.
> 
> __CPUINIT is appropriate here; __CPUINIT will be kept around if you have
> hotplug CPU suport, but if you don't it will be discarded after all
> secondary CPUs have booted.  And without hotplug CPU, you can't ask
> for the offline CPUs to be onlined.

Since 384a290283: "ARM: gic: use a private mapping for CPU target interfaces",
each CPU's gic_cpu_map entry is initialised to 0xff, so a call to
gic_raise_softirq will target *all* CPUs attached to the GIC if one of the CPUs
targetted has not been initialised.

Thus any call to versatile_boot_secondary will wake up *all* secondaries
physically present, throwing them all into the pen. If we use a subset of these
(e.g. from having "maxcpus=N" on the command line), some will be left in the
pen, even though we didn't ask for them explicitly. This will happen with or
without CPU_HOTPLUG.

Another option would be to add an optional description of a CPU's gic id to the
dt, which would allow us to avoid throwing these secondaries into the pen in
the first place.

Thanks,
Mark

[PATCH] arm: versatile: don't mark pen as __INIT

[Nicolas Pitre]

On Tue, 11 Jun 2013, Mark Rutland wrote:

> On Tue, Jun 11, 2013 at 12:24:41AM +0100, Russell King - ARM Linux wrote:
> > On Mon, Jun 10, 2013 at 04:07:24PM +0100, Mark Rutland wrote:
> > > When booting fewer cores than are physically present on a versatile
> > > platform (e.g. when passing maxcpus=N on the command line), some
> > > secondary cores may remain in the holding pen, which is marked __INIT.
> > > Late in the boot process, the memory comprising the holding pen will be
> > > released to the kernel for more general use, and may be overwritten with
> > > arbitrary data, which can cause the held secondaries to start behaving
> > > unpredictably. This can lead to all manner of odd behaviour from the
> > > kernel.
> > > 
> > > Instead don't mark the section as __INIT. This means we can't reuse the
> > > pen memory, but we won't get secondaries corrupting the rest of the
> > > kernel.
> > 
> > __CPUINIT is appropriate here; __CPUINIT will be kept around if you have
> > hotplug CPU suport, but if you don't it will be discarded after all
> > secondary CPUs have booted.  And without hotplug CPU, you can't ask
> > for the offline CPUs to be onlined.
> 
> Since 384a290283: "ARM: gic: use a private mapping for CPU target interfaces",
> each CPU's gic_cpu_map entry is initialised to 0xff, so a call to
> gic_raise_softirq will target *all* CPUs attached to the GIC if one of the CPUs
> targetted has not been initialised.
> 
> Thus any call to versatile_boot_secondary will wake up *all* secondaries
> physically present, throwing them all into the pen. If we use a subset of these
> (e.g. from having "maxcpus=N" on the command line), some will be left in the
> pen, even though we didn't ask for them explicitly. This will happen with or
> without CPU_HOTPLUG.
> 
> Another option would be to add an optional description of a CPU's gic id to the
> dt, which would allow us to avoid throwing these secondaries into the pen in
> the first place.

You mean adding extra description in DT for hardware information that is 
already perfectly self-discoverable, plus the code to parse it, just for 
those rare cases where someone might want to use maxcpus=N on the kernel 
cmdline while CONFIG_CPU_HOTPLUG=n ?

IMHO removing __CPUINIT from the holding pen is probably the preferable 
alternative.


Nicolas

[PATCH] arm: versatile: don't mark pen as __INIT

[Mark Rutland]

On Tue, Jun 11, 2013 at 04:43:59PM +0100, Nicolas Pitre wrote:
> On Tue, 11 Jun 2013, Mark Rutland wrote:
> 
> > On Tue, Jun 11, 2013 at 12:24:41AM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Jun 10, 2013 at 04:07:24PM +0100, Mark Rutland wrote:
> > > > When booting fewer cores than are physically present on a versatile
> > > > platform (e.g. when passing maxcpus=N on the command line), some
> > > > secondary cores may remain in the holding pen, which is marked __INIT.
> > > > Late in the boot process, the memory comprising the holding pen will be
> > > > released to the kernel for more general use, and may be overwritten with
> > > > arbitrary data, which can cause the held secondaries to start behaving
> > > > unpredictably. This can lead to all manner of odd behaviour from the
> > > > kernel.
> > > > 
> > > > Instead don't mark the section as __INIT. This means we can't reuse the
> > > > pen memory, but we won't get secondaries corrupting the rest of the
> > > > kernel.
> > > 
> > > __CPUINIT is appropriate here; __CPUINIT will be kept around if you have
> > > hotplug CPU suport, but if you don't it will be discarded after all
> > > secondary CPUs have booted.  And without hotplug CPU, you can't ask
> > > for the offline CPUs to be onlined.
> > 
> > Since 384a290283: "ARM: gic: use a private mapping for CPU target interfaces",
> > each CPU's gic_cpu_map entry is initialised to 0xff, so a call to
> > gic_raise_softirq will target *all* CPUs attached to the GIC if one of the CPUs
> > targetted has not been initialised.
> > 
> > Thus any call to versatile_boot_secondary will wake up *all* secondaries
> > physically present, throwing them all into the pen. If we use a subset of these
> > (e.g. from having "maxcpus=N" on the command line), some will be left in the
> > pen, even though we didn't ask for them explicitly. This will happen with or
> > without CPU_HOTPLUG.
> > 
> > Another option would be to add an optional description of a CPU's gic id to the
> > dt, which would allow us to avoid throwing these secondaries into the pen in
> > the first place.
> 
> You mean adding extra description in DT for hardware information that is 
> already perfectly self-discoverable, plus the code to parse it, just for 
> those rare cases where someone might want to use maxcpus=N on the kernel 
> cmdline while CONFIG_CPU_HOTPLUG=n ?
> 
> IMHO removing __CPUINIT from the holding pen is probably the preferable 
> alternative.

Ok, could I take that as your ack on the original patch?

Thanks,
Mark.

[PATCH] arm: versatile: don't mark pen as __INIT

[Nicolas Pitre]

On Mon, 17 Jun 2013, Mark Rutland wrote:

> On Tue, Jun 11, 2013 at 04:43:59PM +0100, Nicolas Pitre wrote:
> > On Tue, 11 Jun 2013, Mark Rutland wrote:
> > 
> > > On Tue, Jun 11, 2013 at 12:24:41AM +0100, Russell King - ARM Linux wrote:
> > > > On Mon, Jun 10, 2013 at 04:07:24PM +0100, Mark Rutland wrote:
> > > > > When booting fewer cores than are physically present on a versatile
> > > > > platform (e.g. when passing maxcpus=N on the command line), some
> > > > > secondary cores may remain in the holding pen, which is marked __INIT.
> > > > > Late in the boot process, the memory comprising the holding pen will be
> > > > > released to the kernel for more general use, and may be overwritten with
> > > > > arbitrary data, which can cause the held secondaries to start behaving
> > > > > unpredictably. This can lead to all manner of odd behaviour from the
> > > > > kernel.
> > > > > 
> > > > > Instead don't mark the section as __INIT. This means we can't reuse the
> > > > > pen memory, but we won't get secondaries corrupting the rest of the
> > > > > kernel.
> > > > 
> > > > __CPUINIT is appropriate here; __CPUINIT will be kept around if you have
> > > > hotplug CPU suport, but if you don't it will be discarded after all
> > > > secondary CPUs have booted.  And without hotplug CPU, you can't ask
> > > > for the offline CPUs to be onlined.
> > > 
> > > Since 384a290283: "ARM: gic: use a private mapping for CPU target interfaces",
> > > each CPU's gic_cpu_map entry is initialised to 0xff, so a call to
> > > gic_raise_softirq will target *all* CPUs attached to the GIC if one of the CPUs
> > > targetted has not been initialised.
> > > 
> > > Thus any call to versatile_boot_secondary will wake up *all* secondaries
> > > physically present, throwing them all into the pen. If we use a subset of these
> > > (e.g. from having "maxcpus=N" on the command line), some will be left in the
> > > pen, even though we didn't ask for them explicitly. This will happen with or
> > > without CPU_HOTPLUG.
> > > 
> > > Another option would be to add an optional description of a CPU's gic id to the
> > > dt, which would allow us to avoid throwing these secondaries into the pen in
> > > the first place.
> > 
> > You mean adding extra description in DT for hardware information that is 
> > already perfectly self-discoverable, plus the code to parse it, just for 
> > those rare cases where someone might want to use maxcpus=N on the kernel 
> > cmdline while CONFIG_CPU_HOTPLUG=n ?
> > 
> > IMHO removing __CPUINIT from the holding pen is probably the preferable 
> > alternative.
> 
> Ok, could I take that as your ack on the original patch?

Yes.  And feel free to complement it with the above justification.


Nicolas

[PATCH] arm: versatile: don't mark pen as __INIT

[Mark Rutland]

On Mon, Jun 17, 2013 at 12:25:48PM +0100, Nicolas Pitre wrote:
> On Mon, 17 Jun 2013, Mark Rutland wrote:
> 
> > On Tue, Jun 11, 2013 at 04:43:59PM +0100, Nicolas Pitre wrote:
> > > On Tue, 11 Jun 2013, Mark Rutland wrote:
> > > 
> > > > On Tue, Jun 11, 2013 at 12:24:41AM +0100, Russell King - ARM Linux wrote:
> > > > > On Mon, Jun 10, 2013 at 04:07:24PM +0100, Mark Rutland wrote:
> > > > > > When booting fewer cores than are physically present on a versatile
> > > > > > platform (e.g. when passing maxcpus=N on the command line), some
> > > > > > secondary cores may remain in the holding pen, which is marked __INIT.
> > > > > > Late in the boot process, the memory comprising the holding pen will be
> > > > > > released to the kernel for more general use, and may be overwritten with
> > > > > > arbitrary data, which can cause the held secondaries to start behaving
> > > > > > unpredictably. This can lead to all manner of odd behaviour from the
> > > > > > kernel.
> > > > > > 
> > > > > > Instead don't mark the section as __INIT. This means we can't reuse the
> > > > > > pen memory, but we won't get secondaries corrupting the rest of the
> > > > > > kernel.
> > > > > 
> > > > > __CPUINIT is appropriate here; __CPUINIT will be kept around if you have
> > > > > hotplug CPU suport, but if you don't it will be discarded after all
> > > > > secondary CPUs have booted.  And without hotplug CPU, you can't ask
> > > > > for the offline CPUs to be onlined.
> > > > 
> > > > Since 384a290283: "ARM: gic: use a private mapping for CPU target interfaces",
> > > > each CPU's gic_cpu_map entry is initialised to 0xff, so a call to
> > > > gic_raise_softirq will target *all* CPUs attached to the GIC if one of the CPUs
> > > > targetted has not been initialised.
> > > > 
> > > > Thus any call to versatile_boot_secondary will wake up *all* secondaries
> > > > physically present, throwing them all into the pen. If we use a subset of these
> > > > (e.g. from having "maxcpus=N" on the command line), some will be left in the
> > > > pen, even though we didn't ask for them explicitly. This will happen with or
> > > > without CPU_HOTPLUG.
> > > > 
> > > > Another option would be to add an optional description of a CPU's gic id to the
> > > > dt, which would allow us to avoid throwing these secondaries into the pen in
> > > > the first place.
> > > 
> > > You mean adding extra description in DT for hardware information that is 
> > > already perfectly self-discoverable, plus the code to parse it, just for 
> > > those rare cases where someone might want to use maxcpus=N on the kernel 
> > > cmdline while CONFIG_CPU_HOTPLUG=n ?
> > > 
> > > IMHO removing __CPUINIT from the holding pen is probably the preferable 
> > > alternative.
> > 
> > Ok, could I take that as your ack on the original patch?
> 
> Yes.  And feel free to complement it with the above justification.

Cheers.

I've dropped a version with an expanded description into Russell's patch system
as 7766/1, in the hope this could go in soon, as it appears to be affecting
others [1].

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/177499.html

[PATCH] arm: versatile: don't mark pen as __INIT

[Jon Medhurst (Tixy)]

On Mon, 2013-06-10 at 16:07 +0100, Mark Rutland wrote:
> When booting fewer cores than are physically present on a versatile
> platform (e.g. when passing maxcpus=N on the command line), some
> secondary cores may remain in the holding pen, which is marked __INIT.
> Late in the boot process, the memory comprising the holding pen will be
> released to the kernel for more general use, and may be overwritten with
> arbitrary data, which can cause the held secondaries to start behaving
> unpredictably. This can lead to all manner of odd behaviour from the
> kernel.
> 
> Instead don't mark the section as __INIT. This means we can't reuse the
> pen memory, but we won't get secondaries corrupting the rest of the
> kernel.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Acked-by: Pawel Moll <pawel.moll@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---

This gives section mismatch warnings when compiling with
vexpress_defconfig:

        WARNING: vmlinux.o(.text+0x14128): Section mismatch in reference from the variable pen to the function .cpuinit.text:secondary_startup()
        The function pen() references
        the function __cpuinit secondary_startup().
        This is often because pen lacks a __cpuinit 
        annotation or the annotation of secondary_startup is wrong.
        
        WARNING: vmlinux.o(.text+0x14130): Section mismatch in reference from the variable pen to the variable .cpuinit.data:pen_release
        The function pen() references
        the variable __cpuinitdata pen_release.
        This is often because pen lacks a __cpuinitdata 
        annotation or the annotation of pen_release is wrong.

wonder if this is opening up a small can of worms...?


>  arch/arm/plat-versatile/headsmp.S | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/arm/plat-versatile/headsmp.S b/arch/arm/plat-versatile/headsmp.S
> index b178d44..2677bc3 100644
> --- a/arch/arm/plat-versatile/headsmp.S
> +++ b/arch/arm/plat-versatile/headsmp.S
> @@ -11,8 +11,6 @@
>  #include <linux/linkage.h>
>  #include <linux/init.h>
>  
> -	__INIT
> -
>  /*
>   * Realview/Versatile Express specific entry point for secondary CPUs.
>   * This provides a "holding pen" into which all secondary cores are held

-- 
Tixy

[PATCH] arm: versatile: don't mark pen as __INIT

[Russell King - ARM Linux]

On Tue, Jun 18, 2013 at 02:26:42PM +0100, Jon Medhurst (Tixy) wrote:
> On Mon, 2013-06-10 at 16:07 +0100, Mark Rutland wrote:
> > When booting fewer cores than are physically present on a versatile
> > platform (e.g. when passing maxcpus=N on the command line), some
> > secondary cores may remain in the holding pen, which is marked __INIT.
> > Late in the boot process, the memory comprising the holding pen will be
> > released to the kernel for more general use, and may be overwritten with
> > arbitrary data, which can cause the held secondaries to start behaving
> > unpredictably. This can lead to all manner of odd behaviour from the
> > kernel.
> > 
> > Instead don't mark the section as __INIT. This means we can't reuse the
> > pen memory, but we won't get secondaries corrupting the rest of the
> > kernel.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Acked-by: Pawel Moll <pawel.moll@arm.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> 
> This gives section mismatch warnings when compiling with
> vexpress_defconfig:
> 
>         WARNING: vmlinux.o(.text+0x14128): Section mismatch in reference from the variable pen to the function .cpuinit.text:secondary_startup()
>         The function pen() references
>         the function __cpuinit secondary_startup().
>         This is often because pen lacks a __cpuinit 
>         annotation or the annotation of secondary_startup is wrong.
>         
>         WARNING: vmlinux.o(.text+0x14130): Section mismatch in reference from the variable pen to the variable .cpuinit.data:pen_release
>         The function pen() references
>         the variable __cpuinitdata pen_release.
>         This is often because pen lacks a __cpuinitdata 
>         annotation or the annotation of pen_release is wrong.
> 
> wonder if this is opening up a small can of worms...?

With the death of __cpuinit and __CPUINIT, this becomes a non-issue.
Realistically, how many ARM platforms are built without CPU hotplug?
My guess is none what so ever, because CPU hotplug is needed for PM.

[PATCH] arm: versatile: don't mark pen as __INIT

[Jon Medhurst (Tixy)]

On Tue, 2013-06-18 at 16:13 +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 18, 2013 at 02:26:42PM +0100, Jon Medhurst (Tixy) wrote:
> > On Mon, 2013-06-10 at 16:07 +0100, Mark Rutland wrote:
> > > When booting fewer cores than are physically present on a versatile
> > > platform (e.g. when passing maxcpus=N on the command line), some
> > > secondary cores may remain in the holding pen, which is marked __INIT.
> > > Late in the boot process, the memory comprising the holding pen will be
> > > released to the kernel for more general use, and may be overwritten with
> > > arbitrary data, which can cause the held secondaries to start behaving
> > > unpredictably. This can lead to all manner of odd behaviour from the
> > > kernel.
> > > 
> > > Instead don't mark the section as __INIT. This means we can't reuse the
> > > pen memory, but we won't get secondaries corrupting the rest of the
> > > kernel.
> > > 
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Acked-by: Pawel Moll <pawel.moll@arm.com>
> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > ---
> > 
> > This gives section mismatch warnings when compiling with
> > vexpress_defconfig:
> > 
> >         WARNING: vmlinux.o(.text+0x14128): Section mismatch in reference from the variable pen to the function .cpuinit.text:secondary_startup()
> >         The function pen() references
> >         the function __cpuinit secondary_startup().
> >         This is often because pen lacks a __cpuinit 
> >         annotation or the annotation of secondary_startup is wrong.
> >         
> >         WARNING: vmlinux.o(.text+0x14130): Section mismatch in reference from the variable pen to the variable .cpuinit.data:pen_release
> >         The function pen() references
> >         the variable __cpuinitdata pen_release.
> >         This is often because pen lacks a __cpuinitdata 
> >         annotation or the annotation of pen_release is wrong.
> > 
> > wonder if this is opening up a small can of worms...?
> 
> With the death of __cpuinit and __CPUINIT, this becomes a non-issue.
> Realistically, how many ARM platforms are built without CPU hotplug?
> My guess is none what so ever, because CPU hotplug is needed for PM.

Indeed, a storm in a teacup. I had even seen the  __cpuinit removal
patch a few hours earlier and didn't connect the dots. :-(

-- 
Tixy