[PATCH v4 0/7] arm64: Verify early CPU features

[PATCH v4 0/7] arm64: Verify early CPU features

[Suzuki K Poulose]

From: Suzuki K. Poulose <suzuki.poulose@arm.com>

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)

Changes since V3:
 - Sanitise the boot status handling code, removing unnecessary dsb()/isb().
   (aka: write the code with a little bit more of thought), removed RFC tag.
 - Store the status in head.txt when MMU turned off.

Changes since V2:
 - Add support for synchronising the booting status of a secondary
   CPU. Added RFC Tag. Patch - 4
 - Panic the system with incompatible ASIDBits



Suzuki K. Poulose (7):
  arm64: Add a helper for parking CPUs in a loop
  arm64: Introduce cpu_die_early
  arm64: Move cpu_die_early to smp.c
  arm64: Handle early CPU boot failures
  arm64: Enable CPU capability verification unconditionally
  arm64: Add helper for extracting ASIDBits
  arm64: Ensure the secondary CPUs have safe ASIDBits size

 arch/arm64/include/asm/cpufeature.h  |    6 ---
 arch/arm64/include/asm/mmu_context.h |    2 +
 arch/arm64/include/asm/smp.h         |   35 +++++++++++++++++
 arch/arm64/kernel/asm-offsets.c      |    2 +
 arch/arm64/kernel/cpufeature.c       |   48 +++++++++--------------
 arch/arm64/kernel/head.S             |   39 +++++++++++++++++--
 arch/arm64/kernel/smp.c              |   69 ++++++++++++++++++++++++++++++++++
 arch/arm64/mm/context.c              |   51 ++++++++++++++++++-------
 8 files changed, 199 insertions(+), 53 deletions(-)

-- 
1.7.9.5

[PATCH v4 1/7] arm64: Add a helper for parking CPUs in a loop

[Suzuki K Poulose]

From: Suzuki K. Poulose <suzuki.poulose@arm.com>

Adds a routine which can be used to park CPUs (spinning in kernel)
when they can't be killed.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/smp.h   |    8 ++++++++
 arch/arm64/kernel/cpufeature.c |    5 +----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
index d9c3d6a..53b53a9 100644
--- a/arch/arm64/include/asm/smp.h
+++ b/arch/arm64/include/asm/smp.h
@@ -69,4 +69,12 @@ extern int __cpu_disable(void);
 extern void __cpu_die(unsigned int cpu);
 extern void cpu_die(void);
 
+static inline void cpu_park_loop(void)
+{
+	for (;;) {
+		wfe();
+		wfi();
+	}
+}
+
 #endif /* ifndef __ASM_SMP_H */
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 5c90aa4..cc4a089 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -842,10 +842,7 @@ static void fail_incapable_cpu(char *cap_type,
 	/* Check if we can park ourselves */
 	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
 		cpu_ops[cpu]->cpu_die(cpu);
-	asm(
-	"1:	wfe\n"
-	"	wfi\n"
-	"	b	1b");
+	cpu_park_loop();
 }
 
 /*
-- 
1.7.9.5

[PATCH v4 2/7] arm64: Introduce cpu_die_early

[Suzuki K Poulose]

From: Suzuki K. Poulose <suzuki.poulose@arm.com>

Or in other words, make fail_incapable_cpu() reusable.

We use fail_incapable_cpu() to kill a secondary CPU early during the
bringup, which doesn't have the system advertised capabilities.
This patch makes the routine more generic, to kill a secondary
booting CPU, getting rid of the dependency on capability struct.
This can be used by checks which are not necessarily attached to
a capability struct (e.g, cpu ASIDBits).

In that process, renames the function to cpu_die_early() to better
match its functionality. This will be moved to arch/arm64/kernel/smp.c
later.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/cpufeature.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index cc4a089..8275e7d 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -827,15 +827,15 @@ static u64 __raw_read_system_reg(u32 sys_id)
 }
 
 /*
- * Park the CPU which doesn't have the capability as advertised
- * by the system.
+ * Kill the calling secondary CPU, early in bringup before it is turned
+ * online.
  */
-static void fail_incapable_cpu(char *cap_type,
-				 const struct arm64_cpu_capabilities *cap)
+void cpu_die_early(void)
 {
 	int cpu = smp_processor_id();
 
-	pr_crit("CPU%d: missing %s : %s\n", cpu, cap_type, cap->desc);
+	pr_crit("CPU%d: will not boot\n", cpu);
+
 	/* Mark this CPU absent */
 	set_cpu_present(cpu, 0);
 
@@ -873,8 +873,11 @@ void verify_local_cpu_capabilities(void)
 		 * If the new CPU misses an advertised feature, we cannot proceed
 		 * further, park the cpu.
 		 */
-		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i]))
-			fail_incapable_cpu("arm64_features", &caps[i]);
+		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i])) {
+			pr_crit("CPU%d: missing feature: %s\n",
+					smp_processor_id(), caps[i].desc);
+			cpu_die_early();
+		}
 		if (caps[i].enable)
 			caps[i].enable(NULL);
 	}
@@ -882,8 +885,11 @@ void verify_local_cpu_capabilities(void)
 	for (i = 0, caps = arm64_hwcaps; caps[i].desc; i++) {
 		if (!cpus_have_hwcap(&caps[i]))
 			continue;
-		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i]))
-			fail_incapable_cpu("arm64_hwcaps", &caps[i]);
+		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i])) {
+			pr_crit("CPU%d: missing HWCAP: %s\n",
+					smp_processor_id(), caps[i].desc);
+			cpu_die_early();
+		}
 	}
 }
 
-- 
1.7.9.5

[PATCH v4 3/7] arm64: Move cpu_die_early to smp.c

[Suzuki K Poulose]

From: Suzuki K. Poulose <suzuki.poulose@arm.com>

This patch moves cpu_die_early to smp.c, where it fits better.
No functional changes, except for adding the necessary checks
for CONFIG_HOTPLUG_CPU.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/smp.h   |    1 +
 arch/arm64/kernel/cpufeature.c |   19 -------------------
 arch/arm64/kernel/smp.c        |   22 ++++++++++++++++++++++
 3 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
index 53b53a9..32e75ee 100644
--- a/arch/arm64/include/asm/smp.h
+++ b/arch/arm64/include/asm/smp.h
@@ -68,6 +68,7 @@ extern int __cpu_disable(void);
 
 extern void __cpu_die(unsigned int cpu);
 extern void cpu_die(void);
+extern void cpu_die_early(void);
 
 static inline void cpu_park_loop(void)
 {
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 8275e7d..60c3f47 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -827,25 +827,6 @@ static u64 __raw_read_system_reg(u32 sys_id)
 }
 
 /*
- * Kill the calling secondary CPU, early in bringup before it is turned
- * online.
- */
-void cpu_die_early(void)
-{
-	int cpu = smp_processor_id();
-
-	pr_crit("CPU%d: will not boot\n", cpu);
-
-	/* Mark this CPU absent */
-	set_cpu_present(cpu, 0);
-
-	/* Check if we can park ourselves */
-	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
-		cpu_ops[cpu]->cpu_die(cpu);
-	cpu_park_loop();
-}
-
-/*
  * Run through the enabled system capabilities and enable() it on this CPU.
  * The capabilities were decided based on the available CPUs at the boot time.
  * Any new CPU should match the system wide status of the capability. If the
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index b1adc51..59f032b 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -313,6 +313,28 @@ void cpu_die(void)
 }
 #endif
 
+/*
+ * Kill the calling secondary CPU, early in bringup before it is turned
+ * online.
+ */
+void cpu_die_early(void)
+{
+	int cpu = smp_processor_id();
+
+	pr_crit("CPU%d: will not boot\n", cpu);
+
+	/* Mark this CPU absent */
+	set_cpu_present(cpu, 0);
+
+#ifdef CONFIG_HOTPLUG_CPU
+	/* Check if we can park ourselves */
+	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
+		cpu_ops[cpu]->cpu_die(cpu);
+#endif
+
+	cpu_park_loop();
+}
+
 static void __init hyp_mode_check(void)
 {
 	if (is_hyp_mode_available())
-- 
1.7.9.5

[PATCH v4 4/7] arm64: Handle early CPU boot failures

[Suzuki K Poulose]

From: Suzuki K. Poulose <suzuki.poulose@arm.com>

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 <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 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(-)

diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
index 32e75ee..6548769 100644
--- a/arch/arm64/include/asm/smp.h
+++ b/arch/arm64/include/asm/smp.h
@@ -16,6 +16,19 @@
 #ifndef __ASM_SMP_H
 #define __ASM_SMP_H
 
+/* Values for secondary_data.status */
+
+#define CPU_MMU_OFF		-1
+#define CPU_BOOT_SUCCESS	0
+/* The cpu invoked ops->cpu_die, synchronise it with cpu_kill */
+#define CPU_KILL_ME		1
+/* The cpu couldn't die gracefully and is looping in the kernel */
+#define CPU_STUCK_IN_KERNEL	2
+/* Fatal system error detected by secondary CPU, crash the system */
+#define CPU_PANIC_KERNEL	3
+
+#ifndef __ASSEMBLY__
+
 #include <linux/threads.h>
 #include <linux/cpumask.h>
 #include <linux/thread_info.h>
@@ -54,11 +67,17 @@ asmlinkage void secondary_start_kernel(void);
 
 /*
  * Initial data for bringing up a secondary CPU.
+ * @stack  - sp for the secondary CPU
+ * @status - Result passed back from the secondary CPU to
+ *           indicate failure.
  */
 struct secondary_data {
 	void *stack;
+	int status;
 };
+
 extern struct secondary_data secondary_data;
+extern int __early_cpu_boot_status;
 extern void secondary_entry(void);
 
 extern void arch_send_call_function_single_ipi(int cpu);
@@ -78,4 +97,11 @@ static inline void cpu_park_loop(void)
 	}
 }
 
+static inline void update_cpu_boot_status(int val)
+{
+	WRITE_ONCE(secondary_data.status, val);
+}
+
+#endif /* ifndef __ASSEMBLY__ */
+
 #endif /* ifndef __ASM_SMP_H */
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index fffa4ac6..463fcf1 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -104,6 +104,8 @@ int main(void)
   DEFINE(TZ_MINWEST,		offsetof(struct timezone, tz_minuteswest));
   DEFINE(TZ_DSTTIME,		offsetof(struct timezone, tz_dsttime));
   BLANK();
+  DEFINE(CPU_BOOT_STACK,	offsetof(struct secondary_data, stack));
+  BLANK();
 #ifdef CONFIG_KVM_ARM_HOST
   DEFINE(VCPU_CONTEXT,		offsetof(struct kvm_vcpu, arch.ctxt));
   DEFINE(CPU_GP_REGS,		offsetof(struct kvm_cpu_context, gp_regs));
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index ffe9c2b..16bfa34 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -34,6 +34,7 @@
 #include <asm/pgtable-hwdef.h>
 #include <asm/pgtable.h>
 #include <asm/page.h>
+#include <asm/smp.h>
 #include <asm/sysreg.h>
 #include <asm/thread_info.h>
 #include <asm/virt.h>
@@ -606,7 +607,7 @@ ENTRY(secondary_startup)
 ENDPROC(secondary_startup)
 
 ENTRY(__secondary_switched)
-	ldr	x0, [x21]			// get secondary_data.stack
+	ldr	x0, [x21, #CPU_BOOT_STACK]	// get secondary_data.stack
 	mov	sp, x0
 	and	x0, x0, #~(THREAD_SIZE - 1)
 	msr	sp_el0, x0			// save thread_info
@@ -615,6 +616,32 @@ ENTRY(__secondary_switched)
 ENDPROC(__secondary_switched)
 
 /*
+ * 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.
+ *
+ * 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.
+ */
+
+	.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
+	mov	x1, #\status
+	str	x1, [x0]
+	add	x1, x0, 4
+	bl	__inval_cache_range
+	mov	lr, \tmp
+	.endm
+
+ENTRY(__early_cpu_boot_status)
+	.long 	0
+END(__early_cpu_boot_status)
+
+/*
  * Enable the MMU.
  *
  *  x0  = SCTLR_EL1 value for turning on the MMU.
@@ -631,12 +658,14 @@ __enable_mmu:
 	ubfx	x2, x1, #ID_AA64MMFR0_TGRAN_SHIFT, 4
 	cmp	x2, #ID_AA64MMFR0_TGRAN_SUPPORTED
 	b.ne	__no_granule_support
+	mov	x4, x0
+	update_early_cpu_boot_status x2, 0
 	ldr	x5, =vectors
 	msr	vbar_el1, x5
 	msr	ttbr0_el1, x25			// load TTBR0
 	msr	ttbr1_el1, x26			// load TTBR1
 	isb
-	msr	sctlr_el1, x0
+	msr	sctlr_el1, x4
 	isb
 	/*
 	 * Invalidate the local I-cache so that any instructions fetched
@@ -650,6 +679,10 @@ __enable_mmu:
 ENDPROC(__enable_mmu)
 
 __no_granule_support:
+	/* Indicate that this CPU can't boot and is stuck in the kernel */
+	update_early_cpu_boot_status x2, CPU_STUCK_IN_KERNEL
+1:
 	wfe
-	b __no_granule_support
+	wfi
+	b 1b
 ENDPROC(__no_granule_support)
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 59f032b..d2721ae 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -63,6 +63,8 @@
  * where to place its SVC stack
  */
 struct secondary_data secondary_data;
+/* Number of CPUs which aren't online, but looping in kernel text. */
+u32 cpus_stuck_in_kernel;
 
 enum ipi_msg_type {
 	IPI_RESCHEDULE,
@@ -72,6 +74,16 @@ enum ipi_msg_type {
 	IPI_IRQ_WORK,
 };
 
+#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
+
+
 /*
  * 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));
 
 	/*
@@ -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();
 	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);
+
+		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;
 }
@@ -185,6 +227,9 @@ asmlinkage void secondary_start_kernel(void)
 	 */
 	pr_info("CPU%u: Booted secondary processor [%08x]\n",
 					 cpu, read_cpuid_id());
+	update_cpu_boot_status(CPU_BOOT_SUCCESS);
+	/* Make sure the status update is visible before we complete */
+	smp_wmb();
 	set_cpu_online(cpu, true);
 	complete(&cpu_running);
 
@@ -327,10 +372,12 @@ void cpu_die_early(void)
 	set_cpu_present(cpu, 0);
 
 #ifdef CONFIG_HOTPLUG_CPU
+	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);
 #endif
+	update_cpu_boot_status(CPU_STUCK_IN_KERNEL);
 
 	cpu_park_loop();
 }
-- 
1.7.9.5

[PATCH v4 4/7] arm64: Handle early CPU boot failures

[Catalin Marinas]

Hi Suzuki,

On Mon, Jan 25, 2016 at 06:07:02PM +0000, Suzuki K. Poulose wrote:
> +/* Values for secondary_data.status */
> +
> +#define CPU_MMU_OFF		-1
> +#define CPU_BOOT_SUCCESS	0
> +/* The cpu invoked ops->cpu_die, synchronise it with cpu_kill */
> +#define CPU_KILL_ME		1
> +/* The cpu couldn't die gracefully and is looping in the kernel */
> +#define CPU_STUCK_IN_KERNEL	2
> +/* 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).

>  /*
> + * 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?).

> + *
> + * 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.
> + */
> +
> +	.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.

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

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

> @@ -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));
>  
>  	/*
> @@ -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?

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

> +
> +		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;
>  }

BTW, you can send a fix-up on top of this series with corrections, I can
fold them in.

-- 
Catalin

[PATCH v4 4/7] arm64: Handle early CPU boot failures

[Mark Rutland]

On Wed, Feb 03, 2016 at 12:57:38PM +0000, Catalin Marinas wrote:
> Hi Suzuki,
> 
> On Mon, Jan 25, 2016 at 06:07:02PM +0000, Suzuki K. Poulose wrote:
> > + * 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.
> > + */
> > +
> > +	.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.
> 
> > +	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.

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

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

I think we should add a separate __writeback_aligned annotation for
stuff like this, even if it falls in .data..cacheline_aligned for now.

Otherwise, agreed.

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

To get correct data out we need to pad to the CQG regardless of whether
the reader or the writer perform the maintenance.

Mark.

[PATCH v4 4/7] arm64: Handle early CPU boot failures

[Catalin Marinas]

On Wed, Feb 03, 2016 at 04:46:32PM +0000, Mark Rutland wrote:
> On Wed, Feb 03, 2016 at 12:57:38PM +0000, Catalin Marinas wrote:
> > On Mon, Jan 25, 2016 at 06:07:02PM +0000, Suzuki K. Poulose wrote:
> > > + * 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.
> > > + */
> > > +
> > > +	.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.
> > 
> > > +	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.

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

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

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

But see my point above about invalidating on the secondary CPU before
writing the location and invalidating again on the primary CPU before
reading it.

-- 
Catalin

[PATCH v4 4/7] arm64: Handle early CPU boot failures

[Mark Rutland]

On Wed, Feb 03, 2016 at 05:34:49PM +0000, Catalin Marinas wrote:
> On Wed, Feb 03, 2016 at 04:46:32PM +0000, Mark Rutland wrote:
> > On Wed, Feb 03, 2016 at 12:57:38PM +0000, Catalin Marinas wrote:
> > > On Mon, Jan 25, 2016 at 06:07:02PM +0000, Suzuki K. Poulose wrote:
> > > > + * 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.
> > > > + */
> > > > +
> > > > +	.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.
> > > 
> > > > +	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.
> 
> 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.

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

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.

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

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.

Thanks,
Mark.

[PATCH v4 4/7] arm64: Handle early CPU boot failures

[Catalin Marinas]

On Wed, Feb 03, 2016 at 05:53:51PM +0000, Mark Rutland wrote:
> On Wed, Feb 03, 2016 at 05:34:49PM +0000, Catalin Marinas wrote:
> > On Wed, Feb 03, 2016 at 04:46:32PM +0000, Mark Rutland wrote:
> > > On Wed, Feb 03, 2016 at 12:57:38PM +0000, Catalin Marinas wrote:
> > > > On Mon, Jan 25, 2016 at 06:07:02PM +0000, Suzuki K. Poulose wrote:
> > > > > + * 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.
> > > > > + */
> > > > > +
> > > > > +	.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.
> > > > 
> > > > > +	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.
> > 
> > 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.

Indeed.

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

I assume that's true even in a guest.

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

The synchronisation on the primary CPU is a bit flaky anyway and just
relies on long timeouts. It waits for a while to complete (1 sec) and
than it reads the status. So it assumes that the secondary CPU finished
its writing but neither DSB nor cache maintenance guarantee this.

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

I agree.

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

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

-- 
Catalin

[PATCH v4 4/7] arm64: Handle early CPU boot failures

[Mark Rutland]

On Wed, Feb 03, 2016 at 06:12:52PM +0000, Catalin Marinas wrote:
> On Wed, Feb 03, 2016 at 05:53:51PM +0000, Mark Rutland wrote:
> > On Wed, Feb 03, 2016 at 05:34:49PM +0000, Catalin Marinas wrote:
> > > 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.
> 
> But does "target cache" include other CPUs in the system?

It appears so. There's a more complete description (with a table) on
page D3-1718, "Effects of instructions that operate by VA to the Point
of Coherency", where it's explicitly stated:

	For Device memory and Normal memory that is Inner Non-cacheable,
	Outer Non-cacheable, these instructions must affect the caches
	of all PEs in the Outer Shareable shareability domain of the PE
	on which the instruction is operating.

In other cases, per the table, the maintenance may be limited to a
smaller set of caches.

Thanks,
Mark.

[PATCH v4 4/7] arm64: Handle early CPU boot failures

[Suzuki K. Poulose]

Hi Catalin,

>> +/* 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).

OK, I will base my changes on top of the corrections.

>
>>   /*
>> + * 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?).

Sorry it doesn't parse :(. It should have been something like :

"The booting CPU updates the failed status @__early_cpu_boot_status (with
the MMU turned off). It is placed in head.txt to make sure it doesn't
share the same write back granule with anything else while the CPU updates
it."


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

Yes, that looks much better, will keep in mind.


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

The secondary_data.status updated by the successful CPUs which have mmu turned on, and
updating the cpu_running. But as Mark pointed out, complete() implies a barrier, hence
won't need it.

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

OK.

Thanks
Suzuki

[PATCH v4 4/7] arm64: Handle early CPU boot failures

[Mark Rutland]

On Mon, Jan 25, 2016 at 06:07:02PM +0000, Suzuki K Poulose wrote:
> From: Suzuki K. Poulose <suzuki.poulose@arm.com>
> 
> 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.

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.

A system with incompatible CPUs is a broken system to begin with...

Otherwise, comments below.

> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  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(-)

[...]

> @@ -650,6 +679,10 @@ __enable_mmu:
>  ENDPROC(__enable_mmu)
>  
>  __no_granule_support:
> +	/* Indicate that this CPU can't boot and is stuck in the kernel */
> +	update_early_cpu_boot_status x2, CPU_STUCK_IN_KERNEL
> +1:
>  	wfe
> -	b __no_granule_support
> +	wfi
> +	b 1b

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

>  ENDPROC(__no_granule_support)
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 59f032b..d2721ae 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -63,6 +63,8 @@
>   * where to place its SVC stack
>   */
>  struct secondary_data secondary_data;
> +/* Number of CPUs which aren't online, but looping in kernel text. */
> +u32 cpus_stuck_in_kernel;

Why u32 rather than int?

>  
>  enum ipi_msg_type {
>  	IPI_RESCHEDULE,
> @@ -72,6 +74,16 @@ enum ipi_msg_type {
>  	IPI_IRQ_WORK,
>  };
>  
> +#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.

> +
> +
>  /*
>   * 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));
>  
>  	/*

> @@ -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();
>  	secondary_data.stack = NULL;
> +	status = READ_ONCE(secondary_data.status);

What is the rmb intended to order here?

> +	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;
>  }
> @@ -185,6 +227,9 @@ asmlinkage void secondary_start_kernel(void)
>  	 */
>  	pr_info("CPU%u: Booted secondary processor [%08x]\n",
>  					 cpu, read_cpuid_id());
> +	update_cpu_boot_status(CPU_BOOT_SUCCESS);
> +	/* Make sure the status update is visible before we complete */
> +	smp_wmb();

Surely complete() has appropriate barriers?

>  	set_cpu_online(cpu, true);
>  	complete(&cpu_running);
>  
> @@ -327,10 +372,12 @@ void cpu_die_early(void)
>  	set_cpu_present(cpu, 0);
>  
>  #ifdef CONFIG_HOTPLUG_CPU
> +	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.

>  #endif
> +	update_cpu_boot_status(CPU_STUCK_IN_KERNEL);
>  
>  	cpu_park_loop();

Likewise here.

Mark.

[PATCH v4 4/7] arm64: Handle early CPU boot failures

[Catalin Marinas]

On Wed, Feb 03, 2016 at 05:01:15PM +0000, Mark Rutland wrote:
> On Mon, Jan 25, 2016 at 06:07:02PM +0000, Suzuki K Poulose wrote:
> > From: Suzuki K. Poulose <suzuki.poulose@arm.com>
> > 
> > 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.
> 
> When would we use this last case?

It's used in a subsequent series when verifying the ASID bits. I haven't
followed the previous discussions but I guess Suzuki aims to panic the
whole kernel rather than just stop the current CPU when incompatible
ASID size is found.

-- 
Catalin

[PATCH v4 4/7] arm64: Handle early CPU boot failures

[Suzuki K. Poulose]

On 03/02/16 17:01, Mark Rutland wrote:
> On Mon, Jan 25, 2016 at 06:07:02PM +0000, Suzuki K Poulose wrote:
>> From: Suzuki K. Poulose <suzuki.poulose@arm.com>
>>

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

As of now, it is used when we have incompatible ASID bits.

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

Right, that is another option. I am fine with either.

>> -	b __no_granule_support
>> +	wfi
>> +	b 1b
>
> The addition of wfi seems fine, but should be mentioned in the commit
> message.

Sure.

>>   struct secondary_data secondary_data;
>> +/* 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.

>> +#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

>>
>> +	/* 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?

It was for the complete(). But...

>> +	update_cpu_boot_status(CPU_BOOT_SUCCESS);
>> +	/* Make sure the status update is visible before we complete */
>> +	smp_wmb();
>
> Surely complete() has appropriate barriers?

Yes, it does. We can remove it.
  
>>   #ifdef CONFIG_HOTPLUG_CPU
>> +	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.
>

OK.

>>   #endif
>> +	update_cpu_boot_status(CPU_STUCK_IN_KERNEL);
>>
>>   	cpu_park_loop();
>
> Likewise here.

OK.

Thanks
Suzuki

[PATCH v4 4/7] arm64: Handle early CPU boot failures

[Mark Rutland]

On Wed, Feb 03, 2016 at 05:24:07PM +0000, Suzuki K. Poulose wrote:
> On 03/02/16 17:01, Mark Rutland wrote:
> >On Mon, Jan 25, 2016 at 06:07:02PM +0000, Suzuki K Poulose wrote:
> >>From: Suzuki K. Poulose <suzuki.poulose@arm.com>

[...]

> >>  struct secondary_data secondary_data;
> >>+/* 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.

Elsewhere, int or unsigned int is used to contain a cpu number. I think
either would be preferable to u32, to at least limit the inconsistency.

> >>+#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

Aargh, indeed. I had confused CONFIG_HOTPLUG_CPU and CONFIG_SMP. Sorry!

Mark. 

[PATCH v4 5/7] arm64: Enable CPU capability verification unconditionally

[Suzuki K Poulose]

From: Suzuki K. Poulose <suzuki.poulose@arm.com>

We verify the capabilities of the secondary CPUs only when
hotplug is enabled. The boot time activated CPUs do not
go through the verification by checking whether the system
wide capabilities were initialised or not.

This patch removes the capability check dependency on CONFIG_HOTPLUG_CPU,
to make sure that all the secondary CPUs go through the check.
The boot time activated CPUs will still skip the system wide
capability check. The plan is to hook in a check for CPU features
used by the kernel at early boot up, based on the Boot CPU values.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/cpufeature.h |    6 ------
 arch/arm64/kernel/cpufeature.c      |   10 ----------
 2 files changed, 16 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 8f271b8..26024c4 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -164,13 +164,7 @@ void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
 			    const char *info);
 void check_local_cpu_errata(void);
 
-#ifdef CONFIG_HOTPLUG_CPU
 void verify_local_cpu_capabilities(void);
-#else
-static inline void verify_local_cpu_capabilities(void)
-{
-}
-#endif
 
 u64 read_system_reg(u32 id);
 
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 60c3f47..5ba3ef1 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -768,8 +768,6 @@ enable_cpu_capabilities(const struct arm64_cpu_capabilities *caps)
 			on_each_cpu(caps[i].enable, NULL, true);
 }
 
-#ifdef CONFIG_HOTPLUG_CPU
-
 /*
  * Flag to indicate if we have computed the system wide
  * capabilities based on the boot time active CPUs. This
@@ -874,14 +872,6 @@ void verify_local_cpu_capabilities(void)
 	}
 }
 
-#else	/* !CONFIG_HOTPLUG_CPU */
-
-static inline void set_sys_caps_initialised(void)
-{
-}
-
-#endif	/* CONFIG_HOTPLUG_CPU */
-
 static void __init setup_feature_capabilities(void)
 {
 	update_cpu_capabilities(arm64_features, "detected feature:");
-- 
1.7.9.5

[PATCH v4 6/7] arm64: Add helper for extracting ASIDBits

[Suzuki K Poulose]

From: Suzuki K. Poulose <suzuki.poulose@arm.com>

Add a helper to extract ASIDBits on the current cpu

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/mm/context.c |   36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c
index e87f53f..643bf4b 100644
--- a/arch/arm64/mm/context.c
+++ b/arch/arm64/mm/context.c
@@ -40,6 +40,28 @@ static cpumask_t tlb_flush_pending;
 #define ASID_FIRST_VERSION	(1UL << asid_bits)
 #define NUM_USER_ASIDS		ASID_FIRST_VERSION
 
+/* Get the ASIDBits supported by the current CPU */
+static u32 get_cpu_asid_bits(void)
+{
+	u32 asid;
+	int fld = cpuid_feature_extract_field(read_cpuid(ID_AA64MMFR0_EL1),
+						ID_AA64MMFR0_ASID_SHIFT);
+
+	switch (fld) {
+	default:
+		pr_warn("CPU%d: Unknown ASID size (%d); assuming 8-bit\n",
+					smp_processor_id(),  fld);
+		/* Fallthrough */
+	case 0:
+		asid = 8;
+		break;
+	case 2:
+		asid = 16;
+	}
+
+	return asid;
+}
+
 static void flush_context(unsigned int cpu)
 {
 	int i;
@@ -187,19 +209,7 @@ switch_mm_fastpath:
 
 static int asids_init(void)
 {
-	int fld = cpuid_feature_extract_field(read_cpuid(ID_AA64MMFR0_EL1), 4);
-
-	switch (fld) {
-	default:
-		pr_warn("Unknown ASID size (%d); assuming 8-bit\n", fld);
-		/* Fallthrough */
-	case 0:
-		asid_bits = 8;
-		break;
-	case 2:
-		asid_bits = 16;
-	}
-
+	asid_bits = get_cpu_asid_bits();
 	/* If we end up with more CPUs than ASIDs, expect things to crash */
 	WARN_ON(NUM_USER_ASIDS < num_possible_cpus());
 	atomic64_set(&asid_generation, ASID_FIRST_VERSION);
-- 
1.7.9.5

[PATCH v4 7/7] arm64: Ensure the secondary CPUs have safe ASIDBits size

[Suzuki K Poulose]

From: Suzuki K. Poulose <suzuki.poulose@arm.com>

Adds a hook for checking whether a secondary CPU has the
features used already by the kernel during early boot, based
on the boot CPU and plugs in the check for ASID size.

The ID_AA64MMFR0_EL1:ASIDBits determines the size of the mm context
id and is used in the early boot to make decisions. The value is
picked up from the Boot CPU and cannot be delayed until other CPUs
are up. If a secondary CPU has a smaller size than that of the Boot
CPU, things will break horribly and the usual SANITY check is not good
enough to prevent the system from crashing. So, crash the system with
enough information.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/mmu_context.h |    2 ++
 arch/arm64/kernel/cpufeature.c       |   12 ++++++++++++
 arch/arm64/mm/context.c              |   15 +++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
index 2416578..bd8a0b9 100644
--- a/arch/arm64/include/asm/mmu_context.h
+++ b/arch/arm64/include/asm/mmu_context.h
@@ -147,4 +147,6 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
 #define deactivate_mm(tsk,mm)	do { } while (0)
 #define activate_mm(prev,next)	switch_mm(prev, next, NULL)
 
+void verify_cpu_asid_bits(void);
+
 #endif
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 5ba3ef1..3614066 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -24,6 +24,7 @@
 #include <asm/cpu.h>
 #include <asm/cpufeature.h>
 #include <asm/cpu_ops.h>
+#include <asm/mmu_context.h>
 #include <asm/processor.h>
 #include <asm/sysreg.h>
 
@@ -825,6 +826,15 @@ static u64 __raw_read_system_reg(u32 sys_id)
 }
 
 /*
+ * Check for CPU features that are used in early boot
+ * based on the Boot CPU value.
+ */
+static void check_early_cpu_features(void)
+{
+	verify_cpu_asid_bits();
+}
+
+/*
  * Run through the enabled system capabilities and enable() it on this CPU.
  * The capabilities were decided based on the available CPUs at the boot time.
  * Any new CPU should match the system wide status of the capability. If the
@@ -837,6 +847,8 @@ void verify_local_cpu_capabilities(void)
 	int i;
 	const struct arm64_cpu_capabilities *caps;
 
+	check_early_cpu_features();
+
 	/*
 	 * If we haven't computed the system capabilities, there is nothing
 	 * to verify.
diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c
index 643bf4b..3e6786d 100644
--- a/arch/arm64/mm/context.c
+++ b/arch/arm64/mm/context.c
@@ -24,6 +24,7 @@
 
 #include <asm/cpufeature.h>
 #include <asm/mmu_context.h>
+#include <asm/smp.h>
 #include <asm/tlbflush.h>
 
 static u32 asid_bits;
@@ -62,6 +63,20 @@ static u32 get_cpu_asid_bits(void)
 	return asid;
 }
 
+/* Check if the current cpu's ASIDBits is compatible with asid_bits */
+void verify_cpu_asid_bits(void)
+{
+	u32 asid = get_cpu_asid_bits();
+
+	if (asid < asid_bits) {
+		/* Differing ASIDBits is dangerous, crash the system */
+		pr_crit("CPU%d: smaller ASID size(%u) than boot CPU (%u)\n",
+				smp_processor_id(), asid, asid_bits);
+		update_cpu_boot_status(CPU_PANIC_KERNEL);
+		cpu_park_loop();
+	}
+}
+
 static void flush_context(unsigned int cpu)
 {
 	int i;
-- 
1.7.9.5

[PATCH v4 0/7] arm64: Verify early CPU features

[Will Deacon]

On Mon, Jan 25, 2016 at 06:06:58PM +0000, Suzuki K Poulose wrote:
> From: Suzuki K. Poulose <suzuki.poulose@arm.com>
> 
> 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)

Module the comments from Mark and Catalin on patch 4, the rest looks
good to me:

Acked-by: Will Deacon <will.deacon@arm.com>

Will