arm: versatile: don't mark pen as __INIT

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.

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 <[email protected]>

Acked-by Pawel Moll <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

---
 arch/arm/plat-versatile/headsmp.S | 2 --
 1 file changed, 2 deletions(-)

arch/arm/plat-versatile/headsmp.S +0 -0

index b178d44..2677bc3 100644

Quoted 28 lines · 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 <[email protected]>

Acked-by Pawel Moll <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

---
 arch/arm/plat-versatile/headsmp.S | 2 --
 1 file changed, 2 deletions(-)

arch/arm/plat-versatile/headsmp.S +0 -0

index b178d44..2677bc3 100644
@@ -11,8 +11,6 @@
 #include <linux/linkage.h>
 #include <linux/init.h>
 

arch/arm/plat-versatile/headsmp.S line 16

Quoted 5 lines

arch/arm/plat-versatile/headsmp.S +0 -2

-	__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?

Quoted 29 lines · Stephen Boyd

Quoted 28 lines · 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 <[email protected]>

Acked-by Pawel Moll <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

---
 arch/arm/plat-versatile/headsmp.S | 2 --
 1 file changed, 2 deletions(-)

arch/arm/plat-versatile/headsmp.S +0 -0

index b178d44..2677bc3 100644
@@ -11,8 +11,6 @@
 #include <linux/linkage.h>
 #include <linux/init.h>
 

Quoted 6 lines

arch/arm/plat-versatile/headsmp.S line 16

Quoted 5 lines

arch/arm/plat-versatile/headsmp.S +0 -2

-	__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?

Quoted 36 lines · Mark Rutland

Quoted 35 lines · Stephen Boyd

Quoted 25 lines · 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 <[email protected]>

Acked-by Pawel Moll <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

---
 arch/arm/plat-versatile/headsmp.S | 2 --
 1 file changed, 2 deletions(-)

arch/arm/plat-versatile/headsmp.S +0 -0

index b178d44..2677bc3 100644

arch/arm/plat-versatile/headsmp.S line 16

Quoted 8 lines

arch/arm/plat-versatile/headsmp.S +0 -2

@@ -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?

Quoted 3 lines

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.

Quoted 40 lines · Stephen Boyd

Quoted 39 lines · Mark Rutland

Quoted 29 lines · Stephen Boyd

Quoted 28 lines · 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 <[email protected]>

Acked-by Pawel Moll <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

---
 arch/arm/plat-versatile/headsmp.S | 2 --
 1 file changed, 2 deletions(-)

arch/arm/plat-versatile/headsmp.S +0 -0

index b178d44..2677bc3 100644
@@ -11,8 +11,6 @@
 #include <linux/linkage.h>
 #include <linux/init.h>
 

Quoted 6 lines

arch/arm/plat-versatile/headsmp.S line 16

Quoted 5 lines

arch/arm/plat-versatile/headsmp.S +0 -2

-	__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?

Quoted 5 lines

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.

Quoted 41 lines · Stephen Boyd

Quoted 40 lines · Stephen Boyd

Quoted 39 lines · Mark Rutland

Quoted 29 lines · Stephen Boyd

Quoted 28 lines · 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 <[email protected]>

Acked-by Pawel Moll <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

---
 arch/arm/plat-versatile/headsmp.S | 2 --
 1 file changed, 2 deletions(-)

arch/arm/plat-versatile/headsmp.S +0 -0

index b178d44..2677bc3 100644
@@ -11,8 +11,6 @@
 #include <linux/linkage.h>
 #include <linux/init.h>
 

Quoted 6 lines

arch/arm/plat-versatile/headsmp.S line 16

Quoted 5 lines

arch/arm/plat-versatile/headsmp.S +0 -2

-	__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?

Quoted 8 lines

Quoted 5 lines

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).

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.

Quoted 9 lines · 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.

Quoted 3 lines

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.

Quoted 10 lines · Russell King - ARM Linux

Quoted 9 lines · 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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 31 lines · Mark Rutland

Quoted 10 lines · Russell King - ARM Linux

Quoted 9 lines · 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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 42 lines · Nicolas Pitre

Quoted 31 lines · Mark Rutland

Quoted 10 lines · Russell King - ARM Linux

Quoted 9 lines · 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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 3 lines

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 ?

Quoted 2 lines

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

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

Quoted 43 lines · Mark Rutland

Quoted 42 lines · Nicolas Pitre

Quoted 31 lines · Mark Rutland

Quoted 10 lines · Russell King - ARM Linux

Quoted 9 lines · 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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 3 lines

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 ?

Quoted 4 lines

Quoted 2 lines

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.

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 <[email protected]>

Acked-by Pawel Moll <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

---

 arch/arm/plat-versatile/headsmp.S | 2 --
 1 file changed, 2 deletions(-)

arch/arm/plat-versatile/headsmp.S +0 -2

index b178d44..2677bc3 100644
@@ -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

Quoted 9 lines · 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.

Quoted 8 lines

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 <[email protected]>

Acked-by Pawel Moll <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

---

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...?

Quoted 34 lines · Jon Medhurst (Tixy)

Quoted 9 lines · 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.

Quoted 8 lines

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 <[email protected]>

Acked-by Pawel Moll <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

---

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.

Quoted 1 line

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.