On Mon, Dec 10, 2018 at 05:08:31PM +0000, Steve Capper wrote:

On Mon, Dec 10, 2018 at 04:55:38PM +0000, Steve Capper wrote:

quoted

On Mon, Dec 10, 2018 at 04:18:26PM +0000, Will Deacon wrote:

quoted

On Mon, Dec 10, 2018 at 04:04:02PM +0000, Steve Capper wrote:

quoted

On Mon, Dec 10, 2018 at 01:36:40PM +0000, Will Deacon wrote:

quoted

On Fri, Dec 07, 2018 at 05:28:58PM +0000, Suzuki K Poulose wrote:

quoted

On 07/12/2018 15:26, Will Deacon wrote:

quoted

On Fri, Dec 07, 2018 at 10:47:57AM +0000, Suzuki K Poulose wrote:

quoted

On 12/06/2018 10:50 PM, Steve Capper wrote:

quoted

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index f60081be9a1b..58fcc1edd852 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S

@@ -707,6 +707,7 @@ secondary_startup:
   	/*
   	 * Common entry point for secondary CPUs.
   	 */
+	bl	__cpu_secondary_check52bitva
   	bl	__cpu_setup			// initialise processor
   	adrp	x1, swapper_pg_dir
   	bl	__enable_mmu

@@ -785,6 +786,31 @@ ENTRY(__enable_mmu)
   	ret
   ENDPROC(__enable_mmu)
+ENTRY(__cpu_secondary_check52bitva)
+#ifdef CONFIG_ARM64_52BIT_VA
+	ldr_l	x0, vabits_user
+	cmp	x0, #52
+	b.ne	2f > +
+	mrs_s	x0, SYS_ID_AA64MMFR2_EL1
+	and	x0, x0, #(0xf << ID_AA64MMFR2_LVA_SHIFT)
+	cbnz	x0, 2f
+
+	adr_l	x0, va52mismatch
+	mov	w1, #1
+	strb	w1, [x0]
+	dmb	sy
+	dc	ivac, x0	// Invalidate potentially stale cache line

You may have to clear this variable before a CPU is brought up to avoid
raising a false error message when another secondary CPU doesn't boot
for some other reason (say granule support) after a CPU failed with lack
of 52bitva. It is really a crazy corner case.

Can't we just follow the example set by the EL2 setup in the way that is
uses __boot_cpu_mode? In that case, we only need one variable and you can
detect a problem by comparing the two halves.

The only difference here is, the support is bolted at boot CPU time and hence
we need to verify each and every CPU, unlike the __boot_cpu_mode where we
check for mismatch after the SMP CPUs are brought up. If we decide to make
the choice later, something like that could work. The only caveat is the 52bit
kernel VA will have to do something like the above.

So looking at this a bit more, I think we're better off repurposing the
upper bits of the early boot status word to contain a reason code, rather
than introducing new variables for every possible mismatch.

Does the untested diff below look remotely sane to you?

Will

Thanks Will,
This looks good to me, I will test now and fold this into a patch.

Cheers, Steve. Testing would be handy, but don't worry about respinning the
patches as I'm already on top of this and hope to push this out later today.

Thanks Will,
This looks good to me so FWIW:
Tested-by: Steve Capper <redacted>

(for both the 52-bit VA mismatch and 64KB granule not supported cases
using the model).

The only small issue I see is that if subsequent CPUs aren't brought
online (because they don't exist in the model) then the error reason is
repeated.

I'll dig into this.

I think __early_cpu_boot_status needs to be reset at the beginning of
__cpu_up before the secondary is booted. Testing a check for this now.

Hi Will,

The following fixed the repeating error message problem for me. If you
want, I can send a separate patch to fix this?

Cheers,
-- 
Steve


--->8

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 4e3bfbde829a..936156a7ae88 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c

@@ -123,6 +123,11 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
 	update_cpu_boot_status(CPU_MMU_OFF);
 	__flush_dcache_area(&secondary_data, sizeof(secondary_data));
 
+	__early_cpu_boot_status = 0;
+	dsb(ishst);
+	__flush_dcache_area(&__early_cpu_boot_status,
+			sizeof(__early_cpu_boot_status));
+
 	/*
 	 * Now bring the CPU into our world.
 	 */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel