| Message ID | 20181206225042.11548-6-steve.capper@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 52-bit userspace VAs | expand |
Hi Steve, On 12/06/2018 10:50 PM, Steve Capper wrote: > For cases where there is a mismatch in ARMv8.2-LVA support between CPUs > we have to be careful in allowing secondary CPUs to boot if 52-bit > virtual addresses have already been enabled on the boot CPU. > > This patch adds code to the secondary startup path. If the boot CPU has > enabled 52-bit VAs then ID_AA64MMFR2_EL1 is checked to see if the > secondary can also enable 52-bit support. If not, the secondary is > prevented from booting and an error message is displayed indicating why. > > Technically this patch could be implemented using the cpufeature code > when considering 52-bit userspace support. However, we employ low level > checks here as the cpufeature code won't be able to run if we have > mismatched 52-bit kernel va support. > > Signed-off-by: Steve Capper <steve.capper@arm.com> > The patch looks good to me, except for one comment below. > --- > > Patch is new in V5 of the series > --- > arch/arm64/kernel/head.S | 26 ++++++++++++++++++++++++++ > arch/arm64/kernel/smp.c | 5 +++++ > 2 files changed, 31 insertions(+) > > 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. Otherwise looks good to me. Cheers Suzuki
On Fri, Dec 07, 2018 at 10:47:57AM +0000, Suzuki K Poulose wrote: > On 12/06/2018 10:50 PM, Steve Capper wrote: > > For cases where there is a mismatch in ARMv8.2-LVA support between CPUs > > we have to be careful in allowing secondary CPUs to boot if 52-bit > > virtual addresses have already been enabled on the boot CPU. > > > > This patch adds code to the secondary startup path. If the boot CPU has > > enabled 52-bit VAs then ID_AA64MMFR2_EL1 is checked to see if the > > secondary can also enable 52-bit support. If not, the secondary is > > prevented from booting and an error message is displayed indicating why. > > > > Technically this patch could be implemented using the cpufeature code > > when considering 52-bit userspace support. However, we employ low level > > checks here as the cpufeature code won't be able to run if we have > > mismatched 52-bit kernel va support. > > > > Signed-off-by: Steve Capper <steve.capper@arm.com> > > > > The patch looks good to me, except for one comment below. > > > --- > > > > Patch is new in V5 of the series > > --- > > arch/arm64/kernel/head.S | 26 ++++++++++++++++++++++++++ > > arch/arm64/kernel/smp.c | 5 +++++ > > 2 files changed, 31 insertions(+) > > > > 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. Will
On 07/12/2018 15:26, Will Deacon wrote: > On Fri, Dec 07, 2018 at 10:47:57AM +0000, Suzuki K Poulose wrote: >> On 12/06/2018 10:50 PM, Steve Capper wrote: >>> For cases where there is a mismatch in ARMv8.2-LVA support between CPUs >>> we have to be careful in allowing secondary CPUs to boot if 52-bit >>> virtual addresses have already been enabled on the boot CPU. >>> >>> This patch adds code to the secondary startup path. If the boot CPU has >>> enabled 52-bit VAs then ID_AA64MMFR2_EL1 is checked to see if the >>> secondary can also enable 52-bit support. If not, the secondary is >>> prevented from booting and an error message is displayed indicating why. >>> >>> Technically this patch could be implemented using the cpufeature code >>> when considering 52-bit userspace support. However, we employ low level >>> checks here as the cpufeature code won't be able to run if we have >>> mismatched 52-bit kernel va support. >>> >>> Signed-off-by: Steve Capper <steve.capper@arm.com> >>> >> >> The patch looks good to me, except for one comment below. >> >>> --- >>> >>> Patch is new in V5 of the series >>> --- >>> arch/arm64/kernel/head.S | 26 ++++++++++++++++++++++++++ >>> arch/arm64/kernel/smp.c | 5 +++++ >>> 2 files changed, 31 insertions(+) >>> >>> 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. Cheers Suzuki > > Will >
On Fri, Dec 07, 2018 at 05:28:58PM +0000, Suzuki K Poulose wrote: > > > On 07/12/2018 15:26, Will Deacon wrote: > > On Fri, Dec 07, 2018 at 10:47:57AM +0000, Suzuki K Poulose wrote: > > > On 12/06/2018 10:50 PM, Steve Capper wrote: > > > > 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 --->8 diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h index f82b447bd34f..1895561839a9 100644 --- a/arch/arm64/include/asm/smp.h +++ b/arch/arm64/include/asm/smp.h @@ -17,15 +17,20 @@ #define __ASM_SMP_H /* Values for secondary_data.status */ +#define CPU_STUCK_REASON_SHIFT (8) +#define CPU_BOOT_STATUS_MASK ((1U << CPU_STUCK_REASON_SHIFT) - 1) -#define CPU_MMU_OFF (-1) -#define CPU_BOOT_SUCCESS (0) +#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) +#define CPU_KILL_ME (1) /* The cpu couldn't die gracefully and is looping in the kernel */ -#define CPU_STUCK_IN_KERNEL (2) +#define CPU_STUCK_IN_KERNEL (2) /* Fatal system error detected by secondary CPU, crash the system */ -#define CPU_PANIC_KERNEL (3) +#define CPU_PANIC_KERNEL (3) + +#define CPU_STUCK_REASON_52_BIT_VA (1U << CPU_STUCK_REASON_SHIFT) +#define CPU_STUCK_REASON_NO_GRAN (2U << CPU_STUCK_REASON_SHIFT) #ifndef __ASSEMBLY__ diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index c229d9cfe9bf..7377e14884e3 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -809,13 +809,8 @@ ENTRY(__cpu_secondary_check52bitva) 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 - - update_early_cpu_boot_status CPU_STUCK_IN_KERNEL, x0, x1 + update_early_cpu_boot_status \ + CPU_STUCK_IN_KERNEL | CPU_STUCK_REASON_52_BIT_VA, x0, x1 1: wfe wfi b 1b @@ -826,7 +821,8 @@ ENDPROC(__cpu_secondary_check52bitva) __no_granule_support: /* Indicate that this CPU can't boot and is stuck in the kernel */ - update_early_cpu_boot_status CPU_STUCK_IN_KERNEL, x1, x2 + update_early_cpu_boot_status \ + CPU_STUCK_IN_KERNEL | CPU_STUCK_REASON_NO_GRAN, x1, x2 1: wfe wfi diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index e15b0b64d4d0..4e3bfbde829a 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -108,7 +108,6 @@ static int boot_secondary(unsigned int cpu, struct task_struct *idle) } static DECLARE_COMPLETION(cpu_running); -bool va52mismatch __ro_after_init; int __cpu_up(unsigned int cpu, struct task_struct *idle) { @@ -138,10 +137,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle) if (!cpu_online(cpu)) { pr_crit("CPU%u: failed to come online\n", cpu); - - if (IS_ENABLED(CONFIG_ARM64_52BIT_VA) && va52mismatch) - pr_crit("CPU%u: does not support 52-bit VAs\n", cpu); - ret = -EIO; } } else { @@ -156,7 +151,7 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle) if (status == CPU_MMU_OFF) status = READ_ONCE(__early_cpu_boot_status); - switch (status) { + switch (status & CPU_BOOT_STATUS_MASK) { default: pr_err("CPU%u: failed in unknown state : 0x%lx\n", cpu, status); @@ -170,6 +165,10 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle) 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); + if (status & CPU_STUCK_REASON_52_BIT_VA) + pr_crit("CPU%u: does not support 52-bit VAs\n", cpu); + if (status & CPU_STUCK_REASON_NO_GRAN) + pr_crit("CPU%u: does not support %luK granule \n", cpu, PAGE_SIZE / SZ_1K); cpus_stuck_in_kernel++; break; case CPU_PANIC_KERNEL:
On Mon, Dec 10, 2018 at 01:36:40PM +0000, Will Deacon wrote: > On Fri, Dec 07, 2018 at 05:28:58PM +0000, Suzuki K Poulose wrote: > > > > > > On 07/12/2018 15:26, Will Deacon wrote: > > > On Fri, Dec 07, 2018 at 10:47:57AM +0000, Suzuki K Poulose wrote: > > > > On 12/06/2018 10:50 PM, Steve Capper wrote: > > > > > 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,
On Mon, Dec 10, 2018 at 04:04:02PM +0000, Steve Capper wrote: > On Mon, Dec 10, 2018 at 01:36:40PM +0000, Will Deacon wrote: > > On Fri, Dec 07, 2018 at 05:28:58PM +0000, Suzuki K Poulose wrote: > > > On 07/12/2018 15:26, Will Deacon wrote: > > > > On Fri, Dec 07, 2018 at 10:47:57AM +0000, Suzuki K Poulose wrote: > > > > > On 12/06/2018 10:50 PM, Steve Capper wrote: > > > > > > 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. Will
On Mon, Dec 10, 2018 at 04:18:26PM +0000, Will Deacon wrote: > On Mon, Dec 10, 2018 at 04:04:02PM +0000, Steve Capper wrote: > > On Mon, Dec 10, 2018 at 01:36:40PM +0000, Will Deacon wrote: > > > On Fri, Dec 07, 2018 at 05:28:58PM +0000, Suzuki K Poulose wrote: > > > > On 07/12/2018 15:26, Will Deacon wrote: > > > > > On Fri, Dec 07, 2018 at 10:47:57AM +0000, Suzuki K Poulose wrote: > > > > > > On 12/06/2018 10:50 PM, Steve Capper wrote: > > > > > > > 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 <steve.capper@arm.com> (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. Cheers,
On Mon, Dec 10, 2018 at 04:55:38PM +0000, Steve Capper wrote: > On Mon, Dec 10, 2018 at 04:18:26PM +0000, Will Deacon wrote: > > On Mon, Dec 10, 2018 at 04:04:02PM +0000, Steve Capper wrote: > > > On Mon, Dec 10, 2018 at 01:36:40PM +0000, Will Deacon wrote: > > > > On Fri, Dec 07, 2018 at 05:28:58PM +0000, Suzuki K Poulose wrote: > > > > > On 07/12/2018 15:26, Will Deacon wrote: > > > > > > On Fri, Dec 07, 2018 at 10:47:57AM +0000, Suzuki K Poulose wrote: > > > > > > > On 12/06/2018 10:50 PM, Steve Capper wrote: > > > > > > > > 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 <steve.capper@arm.com> > > (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. Cheers,
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: > > On Mon, Dec 10, 2018 at 04:18:26PM +0000, Will Deacon wrote: > > > On Mon, Dec 10, 2018 at 04:04:02PM +0000, Steve Capper wrote: > > > > On Mon, Dec 10, 2018 at 01:36:40PM +0000, Will Deacon wrote: > > > > > On Fri, Dec 07, 2018 at 05:28:58PM +0000, Suzuki K Poulose wrote: > > > > > > On 07/12/2018 15:26, Will Deacon wrote: > > > > > > > On Fri, Dec 07, 2018 at 10:47:57AM +0000, Suzuki K Poulose wrote: > > > > > > > > On 12/06/2018 10:50 PM, Steve Capper wrote: > > > > > > > > > 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 <steve.capper@arm.com> > > > > (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,
On Mon, Dec 10, 2018 at 05:42:45PM +0000, Steve Capper wrote: > 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: > > > On Mon, Dec 10, 2018 at 04:18:26PM +0000, Will Deacon wrote: > > > > On Mon, Dec 10, 2018 at 04:04:02PM +0000, Steve Capper wrote: > > > > > On Mon, Dec 10, 2018 at 01:36:40PM +0000, Will Deacon wrote: > > > > > > On Fri, Dec 07, 2018 at 05:28:58PM +0000, Suzuki K Poulose wrote: > > > > > > > On 07/12/2018 15:26, Will Deacon wrote: > > > > > > > > On Fri, Dec 07, 2018 at 10:47:57AM +0000, Suzuki K Poulose wrote: > > > > > > > > > On 12/06/2018 10:50 PM, Steve Capper wrote: > > > > > > > > > > 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 <steve.capper@arm.com> > > > > > > (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. > */ > I have tested Will's changes and hit the issue reported by Steve. But this is mainly due to a bug in our __cpu_up() code, which ignores the errors reported by the firmware and goes ahead assuming that the CPU entered the kernel and failed so in the process. e.g, with a missing CPU3: [ 78.050880] psci: failed to boot CPU3 (-22) [ 78.051079] CPU3: failed to boot: -22 [ 78.051319] CPU3: is stuck in kernel [ 78.051496] CPU3: does not support 52-bit VAs With the fix attached below, I get: # echo 1 > cpu5/online [ 101.883862] CPU5: failed to come online [ 101.884860] CPU5: is stuck in kernel [ 101.885060] CPU5: does not support 52-bit VAs -sh: echo: write error: Input/output error # echo 1 > cpu3/online [ 106.746141] psci: failed to boot CPU3 (-22) [ 106.746360] CPU3: failed to boot: -22 -sh: echo: write error: Invalid argument ----8>---- arm64: smp: Handle errors reported by the firmware The __cpu_up() routine ignores the errors reported by the firmware for a CPU bringup operation and looks for the error status set by the booting CPU. If the CPU never entered the kernel, we could end up in assuming stale error status, which otherwise would have been set/cleared appropriately by the booting CPU. Reported-by: Steve Capper <steve.capper@arm.com> Cc: Will Deacon <will.deacon@arm.com> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> --- arch/arm64/kernel/smp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index ac73d6c..a854e3d 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -141,6 +141,7 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle) } } else { pr_err("CPU%u: failed to boot: %d\n", cpu, ret); + return ret; } secondary_data.task = NULL;
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 + + update_early_cpu_boot_status CPU_STUCK_IN_KERNEL, x0, x1 +1: wfe + wfi + b 1b + +#endif +2: ret +ENDPROC(__cpu_secondary_check52bitva) + __no_granule_support: /* Indicate that this CPU can't boot and is stuck in the kernel */ update_early_cpu_boot_status CPU_STUCK_IN_KERNEL, x1, x2 diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index 96b8f2f51ab2..e15b0b64d4d0 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -108,6 +108,7 @@ static int boot_secondary(unsigned int cpu, struct task_struct *idle) } static DECLARE_COMPLETION(cpu_running); +bool va52mismatch __ro_after_init; int __cpu_up(unsigned int cpu, struct task_struct *idle) { @@ -137,6 +138,10 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle) if (!cpu_online(cpu)) { pr_crit("CPU%u: failed to come online\n", cpu); + + if (IS_ENABLED(CONFIG_ARM64_52BIT_VA) && va52mismatch) + pr_crit("CPU%u: does not support 52-bit VAs\n", cpu); + ret = -EIO; } } else {
For cases where there is a mismatch in ARMv8.2-LVA support between CPUs we have to be careful in allowing secondary CPUs to boot if 52-bit virtual addresses have already been enabled on the boot CPU. This patch adds code to the secondary startup path. If the boot CPU has enabled 52-bit VAs then ID_AA64MMFR2_EL1 is checked to see if the secondary can also enable 52-bit support. If not, the secondary is prevented from booting and an error message is displayed indicating why. Technically this patch could be implemented using the cpufeature code when considering 52-bit userspace support. However, we employ low level checks here as the cpufeature code won't be able to run if we have mismatched 52-bit kernel va support. Signed-off-by: Steve Capper <steve.capper@arm.com> --- Patch is new in V5 of the series --- arch/arm64/kernel/head.S | 26 ++++++++++++++++++++++++++ arch/arm64/kernel/smp.c | 5 +++++ 2 files changed, 31 insertions(+)