| Message ID | 20201109213023.15092-3-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | An alternative series for asymmetric AArch32 systems | expand |
Hi Will, On Mon, Nov 09, 2020 at 09:30:18PM +0000, Will Deacon wrote: > +static bool has_32bit_el0(const struct arm64_cpu_capabilities *entry, int scope) > +{ > + if (!has_cpuid_feature(entry, scope)) > + return allow_mismatched_32bit_el0; I still don't like overriding the cpufeature mechanism in this way. What about something like below? It still doesn't fit perfectly but at least the capability represents what was detected in the system. We then decide in system_supports_32bit_el0() whether to allow asymmetry. There is an extra trick to park a non-AArch32 capable CPU in has_32bit_el0() if it comes up late and the feature has already been advertised with !allow_mismatched_32bit_el0. I find it clearer, though I probably stared at it more than at your patch ;). diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 97244d4feca9..0e0427997063 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -604,9 +604,13 @@ static inline bool cpu_supports_mixed_endian_el0(void) return id_aa64mmfr0_mixed_endian_el0(read_cpuid(ID_AA64MMFR0_EL1)); } +extern bool allow_mismatched_32bit_el0; +extern bool mismatched_32bit_el0; + static inline bool system_supports_32bit_el0(void) { - return cpus_have_const_cap(ARM64_HAS_32BIT_EL0); + return cpus_have_const_cap(ARM64_HAS_32BIT_EL0) && + (!mismatched_32bit_el0 || allow_mismatched_32bit_el0); } static inline bool system_supports_4kb_granule(void) diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index b7b6804cb931..67534327f92b 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -104,6 +104,13 @@ DECLARE_BITMAP(boot_capabilities, ARM64_NPATCHABLE); bool arm64_use_ng_mappings = false; EXPORT_SYMBOL(arm64_use_ng_mappings); +/* + * Permit PER_LINUX32 and execve() of 32-bit binaries even if not all CPUs + * support it? + */ +bool __read_mostly allow_mismatched_32bit_el0; +bool mismatched_32bit_el0; + /* * Flag to indicate if we have computed the system wide * capabilities based on the boot time active CPUs. This @@ -1193,6 +1200,35 @@ has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope) return feature_matches(val, entry); } +static bool has_32bit_el0(const struct arm64_cpu_capabilities *entry, int scope) +{ + if (has_cpuid_feature(entry, scope)) + return true; + + if (system_capabilities_finalized() && !allow_mismatched_32bit_el0) { + pr_crit("CPU%d: Asymmetric AArch32 not supported\n", + smp_processor_id()); + cpu_die_early(); + } + + mismatched_32bit_el0 = true; + return false; +} + +static int __init report_32bit_el0(void) +{ + if (!system_supports_32bit_el0()) + return 0; + + if (mismatched_32bit_el0) + pr_info("detected: asymmetric 32-bit EL0 support\n"); + else + pr_info("detected: 32-bit EL0 support\n"); + + return 0; +} +core_initcall(report_32bit_el0); + static bool has_useable_gicv3_cpuif(const struct arm64_cpu_capabilities *entry, int scope) { bool has_sre; @@ -1800,10 +1836,9 @@ static const struct arm64_cpu_capabilities arm64_features[] = { }, #endif /* CONFIG_ARM64_VHE */ { - .desc = "32-bit EL0 Support", .capability = ARM64_HAS_32BIT_EL0, - .type = ARM64_CPUCAP_SYSTEM_FEATURE, - .matches = has_cpuid_feature, + .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, + .matches = has_32bit_el0, .sys_reg = SYS_ID_AA64PFR0_EL1, .sign = FTR_UNSIGNED, .field_pos = ID_AA64PFR0_EL0_SHIFT,
On Wed, Nov 11, 2020 at 07:10:44PM +0000, Catalin Marinas wrote: > On Mon, Nov 09, 2020 at 09:30:18PM +0000, Will Deacon wrote: > > +static bool has_32bit_el0(const struct arm64_cpu_capabilities *entry, int scope) > > +{ > > + if (!has_cpuid_feature(entry, scope)) > > + return allow_mismatched_32bit_el0; > > I still don't like overriding the cpufeature mechanism in this way. What about > something like below? It still doesn't fit perfectly but at least the > capability represents what was detected in the system. We then decide in > system_supports_32bit_el0() whether to allow asymmetry. There is an > extra trick to park a non-AArch32 capable CPU in has_32bit_el0() if it > comes up late and the feature has already been advertised with > !allow_mismatched_32bit_el0. I deliberately allow late onlining of 64-bit-only cores and I don't think this is something we should forbid (although it's not clear from your patch when allow_mismatched_32bit_el0 gets set). Furthermore, killing CPUs from the matches callback feels _very_ dodgy to me, as it's invoked indirectly by things such as this_cpu_has_cap(). > I find it clearer, though I probably stared at it more than at your > patch ;). Yeah, swings and roundabouts... I think we're quibbling on implementation details a bit here whereas we should probably be focussing on what to do about execve() and CPU hotplug. Your patch doesn't apply on top of my series or replace this one, so there's not an awful lot I can do with it. I'm about to post a v3 with a tentative solution for execve(), so please could you demonstrate your idea on top of that so I can see how it fits together? I'd like to move on from the "I don't like this" (none of us do) discussion and figure out the functional aspects, if possible. We can always paint it a different colour later on, but we don't even have a full solution yet. Thanks, Will
On Fri, Nov 13, 2020 at 09:36:30AM +0000, Will Deacon wrote: > On Wed, Nov 11, 2020 at 07:10:44PM +0000, Catalin Marinas wrote: > > On Mon, Nov 09, 2020 at 09:30:18PM +0000, Will Deacon wrote: > > > +static bool has_32bit_el0(const struct arm64_cpu_capabilities *entry, int scope) > > > +{ > > > + if (!has_cpuid_feature(entry, scope)) > > > + return allow_mismatched_32bit_el0; > > > > I still don't like overriding the cpufeature mechanism in this way. What about > > something like below? It still doesn't fit perfectly but at least the > > capability represents what was detected in the system. We then decide in > > system_supports_32bit_el0() whether to allow asymmetry. There is an > > extra trick to park a non-AArch32 capable CPU in has_32bit_el0() if it > > comes up late and the feature has already been advertised with > > !allow_mismatched_32bit_el0. > > I deliberately allow late onlining of 64-bit-only cores and I don't think > this is something we should forbid I agree and my patch allows this. That's a property of WEAK_LOCAL_CPUFEATURE. > (although it's not clear from your patch when > allow_mismatched_32bit_el0 gets set). It doesn't, that was meant as a discussion point around the cpufeature framework but still relying on your other patches for cpumask, cmdline argument. > Furthermore, killing CPUs from the matches callback feels _very_ dodgy > to me, as it's invoked indirectly by things such as > this_cpu_has_cap(). Yeah, this part is not nice. What we want here is for a late 64-bit only CPU to be parked if !allow_mismatched_32bit_el0 and we detected 32-bit already (symmetric). > > I find it clearer, though I probably stared at it more than at your > > patch ;). > > Yeah, swings and roundabouts... > > I think we're quibbling on implementation details a bit here whereas we > should probably be focussing on what to do about execve() and CPU hotplug. Do we need to solve the execve() problem? If yes, we have to get the scheduler involved. The hotplug case I think is simpler, just make sure we have a last standing 32-bit capable CPU, no ABI changes. For execve(), arguably we don't necessarily break the execve() ABI as the affinity change may be done later when scheduling the task. But given that it's a user opt-in for this feature anyway, we just document the ABI changes. > Your patch doesn't apply on top of my series or replace this one, so there's > not an awful lot I can do with it. I'm about to post a v3 with a tentative > solution for execve(), so please could you demonstrate your idea on top of > that so I can see how it fits together? I'll give it a go and if it looks any nicer, I'll post something. As you say, it's more like personal preference on the implementation details. On the functional aspects, we must preserve the current behaviour like detecting symmetry and parking late CPUs if they don't comply in the !allow_mismatched_32bit_el0 case. In the allow_mismatched_32bit_el0 case, we relax this to the equivalent of a weak feature (either missing or present for early/late CPUs) but only report 32-bit if we found an early 32-bit capable CPU (I ended up with a truth table on a piece of paper).
diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h index e7d98997c09c..e6f0eb4643a0 100644 --- a/arch/arm64/include/asm/cpucaps.h +++ b/arch/arm64/include/asm/cpucaps.h @@ -20,7 +20,7 @@ #define ARM64_ALT_PAN_NOT_UAO 10 #define ARM64_HAS_VIRT_HOST_EXTN 11 #define ARM64_WORKAROUND_CAVIUM_27456 12 -#define ARM64_HAS_32BIT_EL0 13 +#define ARM64_HAS_32BIT_EL0_DO_NOT_USE 13 #define ARM64_HARDEN_EL2_VECTORS 14 #define ARM64_HAS_CNP 15 #define ARM64_HAS_NO_FPSIMD 16 diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 97244d4feca9..f447d313a9c5 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -604,9 +604,15 @@ static inline bool cpu_supports_mixed_endian_el0(void) return id_aa64mmfr0_mixed_endian_el0(read_cpuid(ID_AA64MMFR0_EL1)); } +const struct cpumask *system_32bit_el0_cpumask(void); +DECLARE_STATIC_KEY_FALSE(arm64_mismatched_32bit_el0); + static inline bool system_supports_32bit_el0(void) { - return cpus_have_const_cap(ARM64_HAS_32BIT_EL0); + u64 pfr0 = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1); + + return id_aa64pfr0_32bit_el0(pfr0) || + static_branch_unlikely(&arm64_mismatched_32bit_el0); } static inline bool system_supports_4kb_granule(void) diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index d4a7e84b1513..264998972627 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -104,6 +104,24 @@ DECLARE_BITMAP(boot_capabilities, ARM64_NPATCHABLE); bool arm64_use_ng_mappings = false; EXPORT_SYMBOL(arm64_use_ng_mappings); +/* + * Permit PER_LINUX32 and execve() of 32-bit binaries even if not all CPUs + * support it? + */ +static bool __read_mostly allow_mismatched_32bit_el0; + +/* + * Static branch enabled only if allow_mismatched_32bit_el0 is set and we have + * seen at least one CPU capable of 32-bit EL0. + */ +DEFINE_STATIC_KEY_FALSE(arm64_mismatched_32bit_el0); + +/* + * Mask of CPUs supporting 32-bit EL0. + * Only valid if arm64_mismatched_32bit_el0 is enabled. + */ +static cpumask_var_t cpu_32bit_el0_mask __cpumask_var_read_mostly; + /* * Flag to indicate if we have computed the system wide * capabilities based on the boot time active CPUs. This @@ -756,7 +774,7 @@ static void __init sort_ftr_regs(void) * Any bits that are not covered by an arm64_ftr_bits entry are considered * RES0 for the system-wide value, and must strictly match. */ -static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new) +static void init_cpu_ftr_reg(u32 sys_reg, u64 new) { u64 val = 0; u64 strict_mask = ~0x0ULL; @@ -819,7 +837,7 @@ static void __init init_cpu_hwcaps_indirect_list(void) static void __init setup_boot_cpu_capabilities(void); -static void __init init_32bit_cpu_features(struct cpuinfo_32bit *info) +static void init_32bit_cpu_features(struct cpuinfo_32bit *info) { init_cpu_ftr_reg(SYS_ID_DFR0_EL1, info->reg_id_dfr0); init_cpu_ftr_reg(SYS_ID_DFR1_EL1, info->reg_id_dfr1); @@ -935,6 +953,25 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field) WARN_ON(!ftrp->width); } +static void update_compat_elf_hwcaps(void); + +static void update_mismatched_32bit_el0_cpu_features(struct cpuinfo_arm64 *info, + struct cpuinfo_arm64 *boot) +{ + static bool boot_cpu_32bit_regs_overridden = false; + + if (!allow_mismatched_32bit_el0 || boot_cpu_32bit_regs_overridden) + return; + + if (id_aa64pfr0_32bit_el0(boot->reg_id_aa64pfr0)) + return; + + boot->aarch32 = info->aarch32; + init_32bit_cpu_features(&boot->aarch32); + update_compat_elf_hwcaps(); + boot_cpu_32bit_regs_overridden = true; +} + static int update_32bit_cpu_features(int cpu, struct cpuinfo_32bit *info, struct cpuinfo_32bit *boot) { @@ -1095,6 +1132,7 @@ void update_cpu_features(int cpu, * (e.g. SYS_ID_AA64PFR0_EL1), so we call it last. */ if (id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0)) { + update_mismatched_32bit_el0_cpu_features(info, boot); taint |= update_32bit_cpu_features(cpu, &info->aarch32, &boot->aarch32); } @@ -1196,6 +1234,52 @@ has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope) return feature_matches(val, entry); } +static int enable_mismatched_32bit_el0(unsigned int cpu) +{ + if (id_aa64pfr0_32bit_el0(per_cpu(cpu_data, cpu).reg_id_aa64pfr0)) { + cpumask_set_cpu(cpu, cpu_32bit_el0_mask); + static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0); + } + + return 0; +} + +static int __init init_32bit_el0_mask(void) +{ + if (!allow_mismatched_32bit_el0) + return 0; + + if (!alloc_cpumask_var(&cpu_32bit_el0_mask, GFP_KERNEL)) + return -ENOMEM; + + return cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, + "arm64/mismatched_32bit_el0:online", + enable_mismatched_32bit_el0, NULL); +} +early_initcall(init_32bit_el0_mask); + +const struct cpumask *system_32bit_el0_cpumask(void) +{ + if (!system_supports_32bit_el0()) + return cpu_none_mask; + + if (static_branch_unlikely(&arm64_mismatched_32bit_el0)) + return cpu_32bit_el0_mask; + + return cpu_present_mask; +} + +static bool has_32bit_el0(const struct arm64_cpu_capabilities *entry, int scope) +{ + if (!has_cpuid_feature(entry, scope)) + return allow_mismatched_32bit_el0; + + if (scope == SCOPE_SYSTEM) + pr_info("detected: 32-bit EL0 Support\n"); + + return true; +} + static bool has_useable_gicv3_cpuif(const struct arm64_cpu_capabilities *entry, int scope) { bool has_sre; @@ -1803,10 +1887,9 @@ static const struct arm64_cpu_capabilities arm64_features[] = { }, #endif /* CONFIG_ARM64_VHE */ { - .desc = "32-bit EL0 Support", - .capability = ARM64_HAS_32BIT_EL0, + .capability = ARM64_HAS_32BIT_EL0_DO_NOT_USE, .type = ARM64_CPUCAP_SYSTEM_FEATURE, - .matches = has_cpuid_feature, + .matches = has_32bit_el0, .sys_reg = SYS_ID_AA64PFR0_EL1, .sign = FTR_UNSIGNED, .field_pos = ID_AA64PFR0_EL0_SHIFT, @@ -2299,7 +2382,7 @@ static const struct arm64_cpu_capabilities compat_elf_hwcaps[] = { {}, }; -static void __init cap_set_elf_hwcap(const struct arm64_cpu_capabilities *cap) +static void cap_set_elf_hwcap(const struct arm64_cpu_capabilities *cap) { switch (cap->hwcap_type) { case CAP_HWCAP: @@ -2344,7 +2427,7 @@ static bool cpus_have_elf_hwcap(const struct arm64_cpu_capabilities *cap) return rc; } -static void __init setup_elf_hwcaps(const struct arm64_cpu_capabilities *hwcaps) +static void setup_elf_hwcaps(const struct arm64_cpu_capabilities *hwcaps) { /* We support emulation of accesses to CPU ID feature registers */ cpu_set_named_feature(CPUID); @@ -2353,6 +2436,12 @@ static void __init setup_elf_hwcaps(const struct arm64_cpu_capabilities *hwcaps) cap_set_elf_hwcap(hwcaps); } +static void update_compat_elf_hwcaps(void) +{ + if (system_capabilities_finalized()) + setup_elf_hwcaps(compat_elf_hwcaps); +} + static void update_cpu_capabilities(u16 scope_mask) { int i;
When confronted with a mixture of CPUs, some of which support 32-bit applications and others which don't, we quite sensibly treat the system as 64-bit only for userspace and prevent execve() of 32-bit binaries. Unfortunately, some crazy folks have decided to build systems like this with the intention of running 32-bit applications, so relax our sanitisation logic to continue to advertise 32-bit support to userspace on these systems and track the real 32-bit capable cores in a cpumask instead. For now, the default behaviour remains but will be tied to a command-line option in a later patch. Signed-off-by: Will Deacon <will@kernel.org> --- arch/arm64/include/asm/cpucaps.h | 2 +- arch/arm64/include/asm/cpufeature.h | 8 ++- arch/arm64/kernel/cpufeature.c | 103 ++++++++++++++++++++++++++-- 3 files changed, 104 insertions(+), 9 deletions(-)