arm64: Verify early CPU features

/*
+ * The booting CPU updates the failed status, with MMU turned off,
+ * below which lies in head.txt to make sure it doesn't share the same writeback
+ * granule. So that we can invalidate it properly.

arch/arm64/kernel/smp.c +2 -0

@@ -89,12 +101,14 @@ static DECLARE_COMPLETION(cpu_running);
 int __cpu_up(unsigned int cpu, struct task_struct *idle)
 {
 	int ret;
+	int status;
 
 	/*
 	 * We need to tell the secondary core where to find its stack and the
 	 * page tables.
 	 */
 	secondary_data.stack = task_stack_page(idle) + THREAD_START_SP;
+	update_cpu_boot_status(CPU_MMU_OFF);
 	__flush_dcache_area(&secondary_data, sizeof(secondary_data));
 
 	/*

arch/arm64/kernel/smp.c +14 -0

+
+		switch (status) {
+		default:
+			pr_err("CPU%u: failed in unknown state : 0x%x\n",
+					cpu, status);
+			break;
+		case CPU_KILL_ME:
+			if (!op_cpu_kill(cpu)) {
+				pr_crit("CPU%u: died during early boot\n", cpu);
+				break;
+			}
+			/* Fall through */
+			pr_crit("CPU%u: may not have shut down cleanly\n", cpu);
+		case CPU_STUCK_IN_KERNEL:

Hi Suzuki,

arch/arm64/kernel/head.S ENTRY(__secondary_switched) · line 632

Quoted 10 lines · Suzuki K. Poulose

+ * update_early_cpu_boot_status tmp, status
+ * - Corrupts tmp, x0, x1
+ * - Writes 'status' to __early_cpu_boot_status and makes sure
+ * it is committed to memory.
+ */

arch/arm64/kernel/head.S +5 -0

+
+	.macro	update_early_cpu_boot_status tmp, status
+	mov	\tmp, lr
+	adrp	x0, __early_cpu_boot_status
+	add	x0, x0, #:lo12:__early_cpu_boot_status

Nitpick: you could use the adr_l macro.

arch/arm64/kernel/head.S ENTRY(__secondary_switched) · line 638

Quoted 6 lines

arch/arm64/kernel/head.S +6 -0

+	mov	x1, #\status
+	str	x1, [x0]
+	add	x1, x0, 4
+	bl	__inval_cache_range
+	mov	lr, \tmp
+	.endm

If the CPU that's currently booting has the MMU off, what's the point of invalidating the cache here?

The operation may not even be broadcast to the other CPU. So you actually need the invalidation before reading the status on the primary CPU.

arch/arm64/kernel/head.S ENTRY(__secondary_switched) · line 642

Quoted 4 lines

arch/arm64/kernel/head.S +4 -0

+
+ENTRY(__early_cpu_boot_status)
+	.long 	0
+END(__early_cpu_boot_status)

I think we should just do like __boot_cpu_mode and place it in the .data..cacheline_aligned section.

You can always use the safe clean+invalidate before reading the value so that we don't care much about the write-back granule.

Quoted 14 lines · Catalin Marinas

arch/arm64/kernel/head.S ENTRY(__secondary_switched) · line 632

Quoted 10 lines · Suzuki K. Poulose

+ * update_early_cpu_boot_status tmp, status
+ * - Corrupts tmp, x0, x1
+ * - Writes 'status' to __early_cpu_boot_status and makes sure
+ * it is committed to memory.
+ */

arch/arm64/kernel/head.S +5 -0

+
+	.macro	update_early_cpu_boot_status tmp, status
+	mov	\tmp, lr
+	adrp	x0, __early_cpu_boot_status
+	add	x0, x0, #:lo12:__early_cpu_boot_status

Nitpick: you could use the adr_l macro.

Quoted 9 lines

arch/arm64/kernel/head.S ENTRY(__secondary_switched) · line 638

Quoted 6 lines

arch/arm64/kernel/head.S +6 -0

+	mov	x1, #\status
+	str	x1, [x0]
+	add	x1, x0, 4
+	bl	__inval_cache_range
+	mov	lr, \tmp
+	.endm

If the CPU that's currently booting has the MMU off, what's the point of invalidating the cache here?

To invalidate stale lines for this address, held in any caches prior to the PoC. I'm assuming that __early_cpu_boot_status is sufficiently padded to the CWG.

Cache maintenance works when SCTLR_ELx.M == 0, though barriers are required prior to cache maintenance as non-cacheable accesses do not hazard by VA.

The MMU being off has no effect on the cache maintenance itself.

Quoted 3 lines

The operation may not even be broadcast to the other CPU. So you actually need the invalidation before reading the status on the primary CPU.

We require that CPUs are coherent when they enter the kernel, so any cache maintenance operation _must_ affect all coherent caches (i.e. it must be broadcast and must affect all coherent caches prior to the PoC in this case).

Quoted 25 lines · Mark Rutland

Quoted 23 lines · Catalin Marinas

arch/arm64/kernel/head.S ENTRY(__secondary_switched) · line 632

Quoted 10 lines · Suzuki K. Poulose

+ * update_early_cpu_boot_status tmp, status
+ * - Corrupts tmp, x0, x1
+ * - Writes 'status' to __early_cpu_boot_status and makes sure
+ * it is committed to memory.
+ */

arch/arm64/kernel/head.S +5 -0

+
+	.macro	update_early_cpu_boot_status tmp, status
+	mov	\tmp, lr
+	adrp	x0, __early_cpu_boot_status
+	add	x0, x0, #:lo12:__early_cpu_boot_status

Nitpick: you could use the adr_l macro.

arch/arm64/kernel/head.S ENTRY(__secondary_switched) · line 638

Quoted 6 lines

arch/arm64/kernel/head.S +6 -0

+	mov	x1, #\status
+	str	x1, [x0]
+	add	x1, x0, 4
+	bl	__inval_cache_range
+	mov	lr, \tmp
+	.endm

If the CPU that's currently booting has the MMU off, what's the point of invalidating the cache here?

Quoted 3 lines

To invalidate stale lines for this address, held in any caches prior to the PoC. I'm assuming that __early_cpu_boot_status is sufficiently padded to the CWG.

I would have rather invalidated it before writing the [x0], if that's what it's aimed at.

Quoted 5 lines

Cache maintenance works when SCTLR_ELx.M == 0, though barriers are required prior to cache maintenance as non-cacheable accesses do not hazard by VA.

The MMU being off has no effect on the cache maintenance itself.

I know, but whether it has an effect on other CPUs is a different question (it probably has). Anyway, I would rather do the invalidation on the CPU that actually reads this status.

Quoted 8 lines

Quoted 3 lines

The operation may not even be broadcast to the other CPU. So you actually need the invalidation before reading the status on the primary CPU.

We require that CPUs are coherent when they enter the kernel, so any cache maintenance operation _must_ affect all coherent caches (i.e. it must be broadcast and must affect all coherent caches prior to the PoC in this case).

In general, if you perform cache maintenance on a non-shareable mapping, I don't think it would be broadcast. But in this case, the MMU is off, data accesses default to Device_nGnRnE and considered outer shareable, so it may actually work. Is this stated anywhere in the ARM ARM?

Quoted 30 lines · Catalin Marinas

Quoted 28 lines · Mark Rutland

Quoted 14 lines · Catalin Marinas

arch/arm64/kernel/head.S ENTRY(__secondary_switched) · line 632

Quoted 10 lines · Suzuki K. Poulose

+ * update_early_cpu_boot_status tmp, status
+ * - Corrupts tmp, x0, x1
+ * - Writes 'status' to __early_cpu_boot_status and makes sure
+ * it is committed to memory.
+ */

arch/arm64/kernel/head.S +5 -0

+
+	.macro	update_early_cpu_boot_status tmp, status
+	mov	\tmp, lr
+	adrp	x0, __early_cpu_boot_status
+	add	x0, x0, #:lo12:__early_cpu_boot_status

Nitpick: you could use the adr_l macro.

Quoted 9 lines

arch/arm64/kernel/head.S ENTRY(__secondary_switched) · line 638

Quoted 6 lines

arch/arm64/kernel/head.S +6 -0

+	mov	x1, #\status
+	str	x1, [x0]
+	add	x1, x0, 4
+	bl	__inval_cache_range
+	mov	lr, \tmp
+	.endm

If the CPU that's currently booting has the MMU off, what's the point of invalidating the cache here?

To invalidate stale lines for this address, held in any caches prior to the PoC. I'm assuming that __early_cpu_boot_status is sufficiently padded to the CWG.

Quoted 2 lines

I would have rather invalidated it before writing the [x0], if that's what it's aimed at.

That alone wouldn't not be sufficient, due to speculative fetches allocating new (clean) lines prior to the write completing.

I was expecting the CWG-aligned region to only be written to with the MMU off, i.e. we'd only have clean stale lines and no dirty lines.

Quoted 4 lines

Quoted 4 lines

Cache maintenance works when SCTLR_ELx.M == 0, though barriers are required prior to cache maintenance as non-cacheable accesses do not hazard by VA.

Quoted 5 lines

Quoted 1 line

The MMU being off has no effect on the cache maintenance itself.

I know, but whether it has an effect on other CPUs is a different question (it probably has). Anyway, I would rather do the invalidation on the CPU that actually reads this status.

My only worry would be how this gets ordered against the (non-cacheable) store. I guess we'd complete that with a DSB SY regardless.

Given that, I have no problem doing the invalidate on the read side. Assuming we only write from the side with the MMU off, we don't need maintenance on that side.

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

The operation may not even be broadcast to the other CPU. So you actually need the invalidation before reading the status on the primary CPU.

We require that CPUs are coherent when they enter the kernel, so any cache maintenance operation _must_ affect all coherent caches (i.e. it must be broadcast and must affect all coherent caches prior to the PoC in this case).

Quoted 4 lines

In general, if you perform cache maintenance on a non-shareable mapping, I don't think it would be broadcast. But in this case, the MMU is off, data accesses default to Device_nGnRnE and considered outer shareable, so it may actually work. Is this stated anywhere in the ARM ARM?

In ARM DDI 0487A.h, D4.2.8 "The effects of disabling a stage of address translation" we state:

Cache maintenance instructions act on the target cache regardless of whether any stages of address translation are disabled, and regardless of the values of the memory attributes. However, if a stage of address translation is disabled, they use the flat address mapping for that translation stage.

Quoted 9 lines · Mark Rutland

Quoted 4 lines · Catalin Marinas

In general, if you perform cache maintenance on a non-shareable mapping, I don't think it would be broadcast. But in this case, the MMU is off, data accesses default to Device_nGnRnE and considered outer shareable, so it may actually work. Is this stated anywhere in the ARM ARM?

In ARM DDI 0487A.h, D4.2.8 "The effects of disabling a stage of address translation" we state:

Quoted 5 lines

Cache maintenance instructions act on the target cache regardless of whether any stages of address translation are disabled, and regardless of the values of the memory attributes. However, if a stage of address translation is disabled, they use the flat address mapping for that translation stage.

But does "target cache" include other CPUs in the system?

arch/arm64/include/asm/smp.h line 28

Quoted 2 lines

arch/arm64/include/asm/smp.h +2 -0

+/* Fatal system error detected by secondary CPU, crash the system */
+#define CPU_PANIC_KERNEL	3

Please add braces around these numbers, just in case (I added them locally).

Quoted 5 lines

/*
+ * The booting CPU updates the failed status, with MMU turned off,
+ * below which lies in head.txt to make sure it doesn't share the same writeback
+ * granule. So that we can invalidate it properly.

I can't really parse this (it looks like punctuation in the wrong place; also "share the same..." with what?).

arch/arm64/kernel/head.S ENTRY(__secondary_switched) · line 632

Quoted 4 lines

arch/arm64/kernel/head.S +4 -0

+	.macro	update_early_cpu_boot_status tmp, status
+	mov	\tmp, lr
+	adrp	x0, __early_cpu_boot_status
+	add	x0, x0, #:lo12:__early_cpu_boot_status

Nitpick: you could use the adr_l macro.

arch/arm64/kernel/smp.c int __cpu_up(unsigned int cpu, struct task_struct *idle) · line 135

Quoted 6 lines

arch/arm64/kernel/smp.c +2 -0

@@ -117,7 +131,35 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
  		pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
  	}
 
+	/* Make sure the update to status is visible */
+	smp_rmb();

Which status? In relation to what?

arch/arm64/kernel/smp.c int __cpu_up(unsigned int cpu, struct task_struct *idle) · line 141

Quoted 7 lines

arch/arm64/kernel/smp.c +5 -0

  	secondary_data.stack = NULL;
+	status = READ_ONCE(secondary_data.status);
+	if (ret && status) {
+
+		if (status == CPU_MMU_OFF)
+			status = READ_ONCE(__early_cpu_boot_status);

You need cache maintenance before reading this.

From: Suzuki K. Poulose <[email protected]>

A secondary CPU could fail to come online due to insufficient capabilities and could simply die or loop in the kernel. e.g, a CPU with no support for the selected kernel PAGE_SIZE loops in kernel with MMU turned off.
or a hotplugged CPU which doesn't have one of the advertised system capability will die during the activation.

There is no way to synchronise the status of the failing CPU back to the master. This patch solves the issue by adding a field to the secondary_data which can be updated by the failing CPU. If the secondary CPU fails even before turning the MMU on, it updates the status in a special variable reserved in the head.txt section to make sure that the update can be cache invalidated safely without possible sharing of cache write back granule.

Here are the possible states :

-1. CPU_MMU_OFF - Initial value set by the master CPU, this value
indicates that the CPU could not turn the MMU on, hence the status could not be reliably updated in the secondary_data. Instead, the CPU has updated the status in __early_cpu_boot_status (reserved in head.txt section)

0. CPU_BOOT_SUCCESS - CPU has booted successfully.

1. CPU_KILL_ME - CPU has invoked cpu_ops->die, indicating the
master CPU to synchronise by issuing a cpu_ops->cpu_kill.

2. CPU_STUCK_IN_KERNEL - CPU couldn't invoke die(), instead is
looping in the kernel. This information could be used by say, kexec to check if it is really safe to do a kexec reboot.

3. CPU_PANIC_KERNEL - CPU detected some serious issues which
requires kernel to crash immediately. The secondary CPU cannot call panic() until it has initialised the GIC. This flag can be used to instruct the master to do so.

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Catalin Marinas <[email protected]>

Signed-off-by Suzuki K. Poulose <[email protected]>

---
 arch/arm64/include/asm/smp.h    |   26 ++++++++++++++++++++++
 arch/arm64/kernel/asm-offsets.c |    2 ++
 arch/arm64/kernel/head.S        |   39 +++++++++++++++++++++++++++++---
 arch/arm64/kernel/smp.c         |   47 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+), 3 deletions(-)

arch/arm64/kernel/smp.c +0 -0

 enum ipi_msg_type {
 	IPI_RESCHEDULE,

@@ -72,6 +74,16 @@ enum ipi_msg_type {
 	IPI_IRQ_WORK,

arch/arm64/kernel/smp.c +4 -0

+
+
 /*
  * Boot a secondary CPU, and assign it the specified idle task.
  * This also gives us the initial stack to use for this CPU.

@@ -89,12 +101,14 @@ static DECLARE_COMPLETION(cpu_running);
 int __cpu_up(unsigned int cpu, struct task_struct *idle)
 {
 	int ret;
+	int status;
 
 	/*
 	 * We need to tell the secondary core where to find its stack and the
 	 * page tables.
 	 */
 	secondary_data.stack = task_stack_page(idle) + THREAD_START_SP;
+	update_cpu_boot_status(CPU_MMU_OFF);
 	__flush_dcache_area(&secondary_data, sizeof(secondary_data));
 
 	/*

arch/arm64/kernel/smp.c +25 -0

+	if (ret && status) {
+
+		if (status == CPU_MMU_OFF)
+			status = READ_ONCE(__early_cpu_boot_status);
+
+		switch (status) {
+		default:
+			pr_err("CPU%u: failed in unknown state : 0x%x\n",
+					cpu, status);
+			break;
+		case CPU_KILL_ME:
+			if (!op_cpu_kill(cpu)) {
+				pr_crit("CPU%u: died during early boot\n", cpu);
+				break;
+			}
+			/* Fall through */
+			pr_crit("CPU%u: may not have shut down cleanly\n", cpu);
+		case CPU_STUCK_IN_KERNEL:
+			pr_crit("CPU%u: is stuck in kernel\n", cpu);
+			cpus_stuck_in_kernel++;
+			break;
+		case CPU_PANIC_KERNEL:
+			panic("CPU%u detected unsupported configuration\n", cpu);
+		}
+	}
 
 	return ret;
 }

Quoted 34 lines · Suzuki K Poulose

From: Suzuki K. Poulose <[email protected]>

A secondary CPU could fail to come online due to insufficient capabilities and could simply die or loop in the kernel. e.g, a CPU with no support for the selected kernel PAGE_SIZE loops in kernel with MMU turned off.
or a hotplugged CPU which doesn't have one of the advertised system capability will die during the activation.

There is no way to synchronise the status of the failing CPU back to the master. This patch solves the issue by adding a field to the secondary_data which can be updated by the failing CPU. If the secondary CPU fails even before turning the MMU on, it updates the status in a special variable reserved in the head.txt section to make sure that the update can be cache invalidated safely without possible sharing of cache write back granule.

Here are the possible states :

-1. CPU_MMU_OFF - Initial value set by the master CPU, this value
indicates that the CPU could not turn the MMU on, hence the status could not be reliably updated in the secondary_data. Instead, the CPU has updated the status in __early_cpu_boot_status (reserved in head.txt section)

0. CPU_BOOT_SUCCESS - CPU has booted successfully.

1. CPU_KILL_ME - CPU has invoked cpu_ops->die, indicating the
master CPU to synchronise by issuing a cpu_ops->cpu_kill.

2. CPU_STUCK_IN_KERNEL - CPU couldn't invoke die(), instead is
looping in the kernel. This information could be used by say, kexec to check if it is really safe to do a kexec reboot.

Quoted 4 lines

3. CPU_PANIC_KERNEL - CPU detected some serious issues which
requires kernel to crash immediately. The secondary CPU cannot call panic() until it has initialised the GIC. This flag can be used to instruct the master to do so.

When would we use this last case?

Quoted 7 lines · Suzuki K Poulose

From: Suzuki K. Poulose <[email protected]>

3. CPU_PANIC_KERNEL - CPU detected some serious issues which
requires kernel to crash immediately. The secondary CPU cannot call panic() until it has initialised the GIC. This flag can be used to instruct the master to do so.

When would we use this last case?

Perhaps a better option is to always throw any incompatible CPU into an (MMU-off) pen, and assume that it's stuck in the kernel, even if we could theoretically turn it off.

arch/arm64/kernel/head.S __enable_mmu: · line 687

Quoted 3 lines

arch/arm64/kernel/head.S +2 -1

-	b __no_granule_support
+	wfi
+	b 1b

The addition of wfi seems fine, but should be mentioned in the commit message.

arch/arm64/kernel/smp.c line 67

Quoted 3 lines

struct secondary_data secondary_data;

arch/arm64/kernel/smp.c +2 -0

+/* Number of CPUs which aren't online, but looping in kernel text. */
+u32 cpus_stuck_in_kernel;

Why u32 rather than int?

arch/arm64/kernel/smp.c enum ipi_msg_type { · line 84

Quoted 8 lines

arch/arm64/kernel/smp.c +8 -0

+#ifdef CONFIG_HOTPLUG_CPU
+static int op_cpu_kill(unsigned int cpu);
+#else
+static inline int op_cpu_kill(unsigned int cpu)
+{
+	return -ENOSYS;
+}
+#endif

There is no !CONFIG_HOTPLUG_CPU configuration any more.

arch/arm64/kernel/smp.c int __cpu_up(unsigned int cpu, struct task_struct *idle) · line 137

Quoted 5 lines

arch/arm64/kernel/smp.c +3 -0

+	/* Make sure the update to status is visible */
+	smp_rmb();
  	secondary_data.stack = NULL;
+	status = READ_ONCE(secondary_data.status);

What is the rmb intended to order here?

arch/arm64/kernel/smp.c asmlinkage void secondary_start_kernel(void) · line 232

Quoted 3 lines

arch/arm64/kernel/smp.c +3 -0

+	update_cpu_boot_status(CPU_BOOT_SUCCESS);
+	/* Make sure the status update is visible before we complete */
+	smp_wmb();

Surely complete() has appropriate barriers?

arch/arm64/kernel/smp.c void cpu_die_early(void) · line 378

Quoted 5 lines

  #ifdef CONFIG_HOTPLUG_CPU

arch/arm64/kernel/smp.c +1 -0

+	update_cpu_boot_status(CPU_KILL_ME);
  	/* Check if we can park ourselves */
  	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
  		cpu_ops[cpu]->cpu_die(cpu);

I think you need a barrier to ensure visibility of the store prior to calling cpu_die (i.e. you want to order an access against execution). A dsb is what you want -- smp_wmb() only expands to a dmb.

arch/arm64/kernel/smp.c void cpu_die_early(void) · line 382

Quoted 4 lines

  #endif

arch/arm64/kernel/smp.c +1 -0

+	update_cpu_boot_status(CPU_STUCK_IN_KERNEL);
 
  	cpu_park_loop();

Likewise here.

Quoted 2 lines · Mark Rutland

Quoted 1 line · Suzuki K Poulose

From: Suzuki K. Poulose <[email protected]>

Quoted 5 lines

arch/arm64/kernel/smp.c line 67

Quoted 3 lines

struct secondary_data secondary_data;

arch/arm64/kernel/smp.c +2 -0

+/* Number of CPUs which aren't online, but looping in kernel text. */
+u32 cpus_stuck_in_kernel;

Why u32 rather than int?

No specific reasons, since it is going to be a quantity, which cannot be < 0, kept it unsigned. It could be unsigned int.

Quoted 10 lines

arch/arm64/kernel/smp.c enum ipi_msg_type { · line 84

Quoted 8 lines

arch/arm64/kernel/smp.c +8 -0

+#ifdef CONFIG_HOTPLUG_CPU
+static int op_cpu_kill(unsigned int cpu);
+#else
+static inline int op_cpu_kill(unsigned int cpu)
+{
+	return -ENOSYS;
+}
+#endif

There is no !CONFIG_HOTPLUG_CPU configuration any more.

Thats what I thought but then there was [1]. If you disable CONFIG_PM_SLEEP, you can still build with !CONFIG_HOTPLUG_CPU (or in other words allnoconfig)

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/384589.html

From: Suzuki K. Poulose <[email protected]>

This series adds support for verifying some of the cpufeatures that are decided early in the boot process based on the boot CPU and cannot be delayed until all the CPUs are up (e.g, ASIDBits and may be VHE?). It also adds support for handling the failures in booting the secondary CPUs which could not be synchronised with the master CPU, otherwise.

It also adds one of the users of this early hook, check for ASIDBits. The mm_context id is based on the ASIDBits width supported by the boot CPU and is used early in the initialisation. So we need to make sure that all the secondary CPUs supports the width reported by the booting CPU, failing which we crash the system.

This series has been tested on Juno, Fast model by injecting smaller ASIDBits, lack of page-size support and missing features(PAN)