| Message ID | 1370876844-6599-1-git-send-email-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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?
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.
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.
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.
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
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.
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.
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
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
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.
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
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
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.
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. :-(
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
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