| Message ID | 20210518094725.7701-19-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for 32-bit tasks on asymmetric AArch32 systems | expand |
On Tue, May 18, 2021 at 10:47:22AM +0100, Will Deacon wrote: > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 959442f76ed7..72efdc611b14 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -2896,15 +2896,33 @@ void __init setup_cpu_features(void) > > static int enable_mismatched_32bit_el0(unsigned int cpu) > { > + static int lucky_winner = -1; > + > struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu); > bool cpu_32bit = id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0); > > if (cpu_32bit) { > cpumask_set_cpu(cpu, cpu_32bit_el0_mask); > static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0); > - setup_elf_hwcaps(compat_elf_hwcaps); > } > > + if (cpumask_test_cpu(0, cpu_32bit_el0_mask) == cpu_32bit) > + return 0; I don't fully understand this early return. AFAICT, we still call setup_elf_hwcaps() via setup_cpu_features() if the system supports 32-bit EL0 (mismatched or not) at boot. For CPU hotplug, we can add the compat hwcaps later if we didn't set them up at boot. So this part is fine. However, if CPU0 is 32-bit-capable, it looks like we'd never disable the offlining on any of the 32-bit-capable CPUs and there's nothing that prevents offlining CPU0.
Hi Catalin, On Mon, May 24, 2021 at 04:46:58PM +0100, Catalin Marinas wrote: > On Tue, May 18, 2021 at 10:47:22AM +0100, Will Deacon wrote: > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index 959442f76ed7..72efdc611b14 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -2896,15 +2896,33 @@ void __init setup_cpu_features(void) > > > > static int enable_mismatched_32bit_el0(unsigned int cpu) > > { > > + static int lucky_winner = -1; > > + > > struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu); > > bool cpu_32bit = id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0); > > > > if (cpu_32bit) { > > cpumask_set_cpu(cpu, cpu_32bit_el0_mask); > > static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0); > > - setup_elf_hwcaps(compat_elf_hwcaps); > > } > > > > + if (cpumask_test_cpu(0, cpu_32bit_el0_mask) == cpu_32bit) > > + return 0; > > I don't fully understand this early return. AFAICT, we still call > setup_elf_hwcaps() via setup_cpu_features() if the system supports > 32-bit EL0 (mismatched or not) at boot. For CPU hotplug, we can add the > compat hwcaps later if we didn't set them up at boot. So this part is > fine. > > However, if CPU0 is 32-bit-capable, it looks like we'd never disable the > offlining on any of the 32-bit-capable CPUs and there's nothing that > prevents offlining CPU0. That is also deferred until we actually detect the mismatch. For example, if CPU0 is 32-bit capable but none of the others are, then when we online CPU1 we will print: | CPU features: Asymmetric 32-bit EL0 support detected on CPU 1; CPU hot-unplug disabled on CPU 0 so the check above is really asking "Is the CPU being onlined mismatched wrt the boot CPU?". If yes, then we need to make sure that we're keeping a 32-bit-capable CPU around. Will
On Mon, May 24, 2021 at 09:32:50PM +0100, Will Deacon wrote: > On Mon, May 24, 2021 at 04:46:58PM +0100, Catalin Marinas wrote: > > On Tue, May 18, 2021 at 10:47:22AM +0100, Will Deacon wrote: > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > > index 959442f76ed7..72efdc611b14 100644 > > > --- a/arch/arm64/kernel/cpufeature.c > > > +++ b/arch/arm64/kernel/cpufeature.c > > > @@ -2896,15 +2896,33 @@ void __init setup_cpu_features(void) > > > > > > static int enable_mismatched_32bit_el0(unsigned int cpu) > > > { > > > + static int lucky_winner = -1; > > > + > > > struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu); > > > bool cpu_32bit = id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0); > > > > > > if (cpu_32bit) { > > > cpumask_set_cpu(cpu, cpu_32bit_el0_mask); > > > static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0); > > > - setup_elf_hwcaps(compat_elf_hwcaps); > > > } > > > > > > + if (cpumask_test_cpu(0, cpu_32bit_el0_mask) == cpu_32bit) > > > + return 0; > > > > I don't fully understand this early return. AFAICT, we still call > > setup_elf_hwcaps() via setup_cpu_features() if the system supports > > 32-bit EL0 (mismatched or not) at boot. For CPU hotplug, we can add the > > compat hwcaps later if we didn't set them up at boot. So this part is > > fine. > > > > However, if CPU0 is 32-bit-capable, it looks like we'd never disable the > > offlining on any of the 32-bit-capable CPUs and there's nothing that > > prevents offlining CPU0. > > That is also deferred until we actually detect the mismatch. For example, if > CPU0 is 32-bit capable but none of the others are, then when we online CPU1 > we will print: > > | CPU features: Asymmetric 32-bit EL0 support detected on CPU 1; CPU hot-unplug disabled on CPU 0 > > so the check above is really asking "Is the CPU being onlined mismatched wrt > the boot CPU?". If yes, then we need to make sure that we're keeping a > 32-bit-capable CPU around. Got it now, the offlining will only be disabled if we detected a mismatch. For this patch: Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 959442f76ed7..72efdc611b14 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -2896,15 +2896,33 @@ void __init setup_cpu_features(void) static int enable_mismatched_32bit_el0(unsigned int cpu) { + static int lucky_winner = -1; + struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu); bool cpu_32bit = id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0); if (cpu_32bit) { cpumask_set_cpu(cpu, cpu_32bit_el0_mask); static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0); - setup_elf_hwcaps(compat_elf_hwcaps); } + if (cpumask_test_cpu(0, cpu_32bit_el0_mask) == cpu_32bit) + return 0; + + if (lucky_winner >= 0) + return 0; + + /* + * We've detected a mismatch. We need to keep one of our CPUs with + * 32-bit EL0 online so that is_cpu_allowed() doesn't end up rejecting + * every CPU in the system for a 32-bit task. + */ + lucky_winner = cpu_32bit ? cpu : cpumask_any_and(cpu_32bit_el0_mask, + cpu_active_mask); + get_cpu_device(lucky_winner)->offline_disabled = true; + setup_elf_hwcaps(compat_elf_hwcaps); + pr_info("Asymmetric 32-bit EL0 support detected on CPU %u; CPU hot-unplug disabled on CPU %u\n", + cpu, lucky_winner); return 0; }
If we want to support 32-bit applications, then when we identify a CPU with mismatched 32-bit EL0 support we must ensure that we will always have an active 32-bit CPU available to us from then on. This is important for the scheduler, because is_cpu_allowed() will be constrained to 32-bit CPUs for compat tasks and forced migration due to a hotplug event will hang if no 32-bit CPUs are available. On detecting a mismatch, prevent offlining of either the mismatching CPU if it is 32-bit capable, or find the first active 32-bit capable CPU otherwise. Signed-off-by: Will Deacon <will@kernel.org> --- arch/arm64/kernel/cpufeature.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)