[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