Add support for 32-bit tasks on asymmetric AArch32 systems

In preparation for late initialisation of the "sanitised" AArch32 register state, move the AArch32 registers out of 'struct cpuinfo' and into their own struct definition.

Reviewed-by Catalin Marinas <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/cpu.h   | 44 +++++++++++----------
 arch/arm64/kernel/cpufeature.c | 71 ++++++++++++++++++----------------
 arch/arm64/kernel/cpuinfo.c    | 53 +++++++++++++------------
 3 files changed, 89 insertions(+), 79 deletions(-)

arch/arm64/include/asm/cpu.h +24 -20

index 7faae6ff3ab4..f4e01aa0f442 100644
@@ -12,26 +12,7 @@
 /*
  * Records attributes of an individual CPU.
  */
-struct cpuinfo_arm64 {
-	struct cpu	cpu;
-	struct kobject	kobj;
-	u32		reg_ctr;
-	u32		reg_cntfrq;
-	u32		reg_dczid;
-	u32		reg_midr;
-	u32		reg_revidr;
-
-	u64		reg_id_aa64dfr0;
-	u64		reg_id_aa64dfr1;
-	u64		reg_id_aa64isar0;
-	u64		reg_id_aa64isar1;
-	u64		reg_id_aa64mmfr0;
-	u64		reg_id_aa64mmfr1;
-	u64		reg_id_aa64mmfr2;
-	u64		reg_id_aa64pfr0;
-	u64		reg_id_aa64pfr1;
-	u64		reg_id_aa64zfr0;
-
+struct cpuinfo_32bit {
 	u32		reg_id_dfr0;
 	u32		reg_id_dfr1;
 	u32		reg_id_isar0;

@@ -54,6 +35,29 @@ struct cpuinfo_arm64 {
 	u32		reg_mvfr0;
 	u32		reg_mvfr1;
 	u32		reg_mvfr2;
+};
+
+struct cpuinfo_arm64 {
+	struct cpu	cpu;
+	struct kobject	kobj;
+	u32		reg_ctr;
+	u32		reg_cntfrq;
+	u32		reg_dczid;
+	u32		reg_midr;
+	u32		reg_revidr;
+
+	u64		reg_id_aa64dfr0;
+	u64		reg_id_aa64dfr1;
+	u64		reg_id_aa64isar0;
+	u64		reg_id_aa64isar1;
+	u64		reg_id_aa64mmfr0;
+	u64		reg_id_aa64mmfr1;
+	u64		reg_id_aa64mmfr2;
+	u64		reg_id_aa64pfr0;
+	u64		reg_id_aa64pfr1;
+	u64		reg_id_aa64zfr0;
+
+	struct cpuinfo_32bit	aarch32;
 
 	/* pseudo-ZCR for recording maximum ZCR_EL1 LEN value: */
 	u64		reg_zcr;

arch/arm64/kernel/cpufeature.c +37 -34

index efed2830d141..a4db25cd7122 100644
@@ -863,6 +863,31 @@ 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)
+{
+	init_cpu_ftr_reg(SYS_ID_DFR0_EL1, info->reg_id_dfr0);
+	init_cpu_ftr_reg(SYS_ID_DFR1_EL1, info->reg_id_dfr1);
+	init_cpu_ftr_reg(SYS_ID_ISAR0_EL1, info->reg_id_isar0);
+	init_cpu_ftr_reg(SYS_ID_ISAR1_EL1, info->reg_id_isar1);
+	init_cpu_ftr_reg(SYS_ID_ISAR2_EL1, info->reg_id_isar2);
+	init_cpu_ftr_reg(SYS_ID_ISAR3_EL1, info->reg_id_isar3);
+	init_cpu_ftr_reg(SYS_ID_ISAR4_EL1, info->reg_id_isar4);
+	init_cpu_ftr_reg(SYS_ID_ISAR5_EL1, info->reg_id_isar5);
+	init_cpu_ftr_reg(SYS_ID_ISAR6_EL1, info->reg_id_isar6);
+	init_cpu_ftr_reg(SYS_ID_MMFR0_EL1, info->reg_id_mmfr0);
+	init_cpu_ftr_reg(SYS_ID_MMFR1_EL1, info->reg_id_mmfr1);
+	init_cpu_ftr_reg(SYS_ID_MMFR2_EL1, info->reg_id_mmfr2);
+	init_cpu_ftr_reg(SYS_ID_MMFR3_EL1, info->reg_id_mmfr3);
+	init_cpu_ftr_reg(SYS_ID_MMFR4_EL1, info->reg_id_mmfr4);
+	init_cpu_ftr_reg(SYS_ID_MMFR5_EL1, info->reg_id_mmfr5);
+	init_cpu_ftr_reg(SYS_ID_PFR0_EL1, info->reg_id_pfr0);
+	init_cpu_ftr_reg(SYS_ID_PFR1_EL1, info->reg_id_pfr1);
+	init_cpu_ftr_reg(SYS_ID_PFR2_EL1, info->reg_id_pfr2);
+	init_cpu_ftr_reg(SYS_MVFR0_EL1, info->reg_mvfr0);
+	init_cpu_ftr_reg(SYS_MVFR1_EL1, info->reg_mvfr1);
+	init_cpu_ftr_reg(SYS_MVFR2_EL1, info->reg_mvfr2);
+}
+
 void __init init_cpu_features(struct cpuinfo_arm64 *info)
 {
 	/* Before we start using the tables, make sure it is sorted */

@@ -882,29 +907,8 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info)
 	init_cpu_ftr_reg(SYS_ID_AA64PFR1_EL1, info->reg_id_aa64pfr1);
 	init_cpu_ftr_reg(SYS_ID_AA64ZFR0_EL1, info->reg_id_aa64zfr0);
 
-	if (id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0)) {
-		init_cpu_ftr_reg(SYS_ID_DFR0_EL1, info->reg_id_dfr0);
-		init_cpu_ftr_reg(SYS_ID_DFR1_EL1, info->reg_id_dfr1);
-		init_cpu_ftr_reg(SYS_ID_ISAR0_EL1, info->reg_id_isar0);
-		init_cpu_ftr_reg(SYS_ID_ISAR1_EL1, info->reg_id_isar1);
-		init_cpu_ftr_reg(SYS_ID_ISAR2_EL1, info->reg_id_isar2);
-		init_cpu_ftr_reg(SYS_ID_ISAR3_EL1, info->reg_id_isar3);
-		init_cpu_ftr_reg(SYS_ID_ISAR4_EL1, info->reg_id_isar4);
-		init_cpu_ftr_reg(SYS_ID_ISAR5_EL1, info->reg_id_isar5);
-		init_cpu_ftr_reg(SYS_ID_ISAR6_EL1, info->reg_id_isar6);
-		init_cpu_ftr_reg(SYS_ID_MMFR0_EL1, info->reg_id_mmfr0);
-		init_cpu_ftr_reg(SYS_ID_MMFR1_EL1, info->reg_id_mmfr1);
-		init_cpu_ftr_reg(SYS_ID_MMFR2_EL1, info->reg_id_mmfr2);
-		init_cpu_ftr_reg(SYS_ID_MMFR3_EL1, info->reg_id_mmfr3);
-		init_cpu_ftr_reg(SYS_ID_MMFR4_EL1, info->reg_id_mmfr4);
-		init_cpu_ftr_reg(SYS_ID_MMFR5_EL1, info->reg_id_mmfr5);
-		init_cpu_ftr_reg(SYS_ID_PFR0_EL1, info->reg_id_pfr0);
-		init_cpu_ftr_reg(SYS_ID_PFR1_EL1, info->reg_id_pfr1);
-		init_cpu_ftr_reg(SYS_ID_PFR2_EL1, info->reg_id_pfr2);
-		init_cpu_ftr_reg(SYS_MVFR0_EL1, info->reg_mvfr0);
-		init_cpu_ftr_reg(SYS_MVFR1_EL1, info->reg_mvfr1);
-		init_cpu_ftr_reg(SYS_MVFR2_EL1, info->reg_mvfr2);
-	}
+	if (id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0))
+		init_32bit_cpu_features(&info->aarch32);
 
 	if (id_aa64pfr0_sve(info->reg_id_aa64pfr0)) {
 		init_cpu_ftr_reg(SYS_ZCR_EL1, info->reg_zcr);

@@ -975,20 +979,12 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field)
 	WARN_ON(!ftrp->width);
 }
 
-static int update_32bit_cpu_features(int cpu, struct cpuinfo_arm64 *info,
-				     struct cpuinfo_arm64 *boot)
+static int update_32bit_cpu_features(int cpu, struct cpuinfo_32bit *info,
+				     struct cpuinfo_32bit *boot)
 {
 	int taint = 0;
 	u64 pfr0 = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
 
-	/*
-	 * If we don't have AArch32 at all then skip the checks entirely
-	 * as the register values may be UNKNOWN and we're not going to be
-	 * using them for anything.
-	 */
-	if (!id_aa64pfr0_32bit_el0(pfr0))
-		return taint;
-
 	/*
 	 * If we don't have AArch32 at EL1, then relax the strictness of
 	 * EL1-dependent register fields to avoid spurious sanity check fails.

@@ -1135,10 +1131,17 @@ void update_cpu_features(int cpu,
 	}
 
 	/*
+	 * If we don't have AArch32 at all then skip the checks entirely
+	 * as the register values may be UNKNOWN and we're not going to be
+	 * using them for anything.
+	 *
 	 * This relies on a sanitised view of the AArch64 ID registers
 	 * (e.g. SYS_ID_AA64PFR0_EL1), so we call it last.
 	 */
-	taint |= update_32bit_cpu_features(cpu, info, boot);
+	if (id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0)) {
+		taint |= update_32bit_cpu_features(cpu, &info->aarch32,
+						   &boot->aarch32);
+	}
 
 	/*
 	 * Mismatched CPU features are a recipe for disaster. Don't even

arch/arm64/kernel/cpuinfo.c +28 -25

index 51fcf99d5351..264c119a6cae 100644
@@ -344,6 +344,32 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)
 	pr_info("Detected %s I-cache on CPU%d\n", icache_policy_str[l1ip], cpu);
 }
 
+static void __cpuinfo_store_cpu_32bit(struct cpuinfo_32bit *info)
+{
+	info->reg_id_dfr0 = read_cpuid(ID_DFR0_EL1);
+	info->reg_id_dfr1 = read_cpuid(ID_DFR1_EL1);
+	info->reg_id_isar0 = read_cpuid(ID_ISAR0_EL1);
+	info->reg_id_isar1 = read_cpuid(ID_ISAR1_EL1);
+	info->reg_id_isar2 = read_cpuid(ID_ISAR2_EL1);
+	info->reg_id_isar3 = read_cpuid(ID_ISAR3_EL1);
+	info->reg_id_isar4 = read_cpuid(ID_ISAR4_EL1);
+	info->reg_id_isar5 = read_cpuid(ID_ISAR5_EL1);
+	info->reg_id_isar6 = read_cpuid(ID_ISAR6_EL1);
+	info->reg_id_mmfr0 = read_cpuid(ID_MMFR0_EL1);
+	info->reg_id_mmfr1 = read_cpuid(ID_MMFR1_EL1);
+	info->reg_id_mmfr2 = read_cpuid(ID_MMFR2_EL1);
+	info->reg_id_mmfr3 = read_cpuid(ID_MMFR3_EL1);
+	info->reg_id_mmfr4 = read_cpuid(ID_MMFR4_EL1);
+	info->reg_id_mmfr5 = read_cpuid(ID_MMFR5_EL1);
+	info->reg_id_pfr0 = read_cpuid(ID_PFR0_EL1);
+	info->reg_id_pfr1 = read_cpuid(ID_PFR1_EL1);
+	info->reg_id_pfr2 = read_cpuid(ID_PFR2_EL1);
+
+	info->reg_mvfr0 = read_cpuid(MVFR0_EL1);
+	info->reg_mvfr1 = read_cpuid(MVFR1_EL1);
+	info->reg_mvfr2 = read_cpuid(MVFR2_EL1);
+}
+
 static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
 {
 	info->reg_cntfrq = arch_timer_get_cntfrq();

@@ -371,31 +397,8 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
 	info->reg_id_aa64pfr1 = read_cpuid(ID_AA64PFR1_EL1);
 	info->reg_id_aa64zfr0 = read_cpuid(ID_AA64ZFR0_EL1);
 
-	/* Update the 32bit ID registers only if AArch32 is implemented */
-	if (id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0)) {
-		info->reg_id_dfr0 = read_cpuid(ID_DFR0_EL1);
-		info->reg_id_dfr1 = read_cpuid(ID_DFR1_EL1);
-		info->reg_id_isar0 = read_cpuid(ID_ISAR0_EL1);
-		info->reg_id_isar1 = read_cpuid(ID_ISAR1_EL1);
-		info->reg_id_isar2 = read_cpuid(ID_ISAR2_EL1);
-		info->reg_id_isar3 = read_cpuid(ID_ISAR3_EL1);
-		info->reg_id_isar4 = read_cpuid(ID_ISAR4_EL1);
-		info->reg_id_isar5 = read_cpuid(ID_ISAR5_EL1);
-		info->reg_id_isar6 = read_cpuid(ID_ISAR6_EL1);
-		info->reg_id_mmfr0 = read_cpuid(ID_MMFR0_EL1);
-		info->reg_id_mmfr1 = read_cpuid(ID_MMFR1_EL1);
-		info->reg_id_mmfr2 = read_cpuid(ID_MMFR2_EL1);
-		info->reg_id_mmfr3 = read_cpuid(ID_MMFR3_EL1);
-		info->reg_id_mmfr4 = read_cpuid(ID_MMFR4_EL1);
-		info->reg_id_mmfr5 = read_cpuid(ID_MMFR5_EL1);
-		info->reg_id_pfr0 = read_cpuid(ID_PFR0_EL1);
-		info->reg_id_pfr1 = read_cpuid(ID_PFR1_EL1);
-		info->reg_id_pfr2 = read_cpuid(ID_PFR2_EL1);
-
-		info->reg_mvfr0 = read_cpuid(MVFR0_EL1);
-		info->reg_mvfr1 = read_cpuid(MVFR1_EL1);
-		info->reg_mvfr2 = read_cpuid(MVFR2_EL1);
-	}
+	if (id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0))
+		__cpuinfo_store_cpu_32bit(&info->aarch32);
 
 	if (IS_ENABLED(CONFIG_ARM64_SVE) &&
 	    id_aa64pfr0_sve(info->reg_id_aa64pfr0))

Signature · 1 line

2.32.0.rc0.204.g9fa02ecfa5-goog

Quoted 4 lines · Will Deacon

In preparation for late initialisation of the "sanitised" AArch32 register state, move the AArch32 registers out of 'struct cpuinfo' and into their own struct definition.

Quoted 2 lines

Reviewed-by Catalin Marinas <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Makes sense to me; if it's not too painful to change, I'd suggest aarch32 rather than 32bit in the name, but either way:

Acked-by Mark Rutland <[email protected]>

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.

Reviewed-by Catalin Marinas <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/cpufeature.h |   8 +-
 arch/arm64/kernel/cpufeature.c      | 114 ++++++++++++++++++++++++----
 arch/arm64/tools/cpucaps            |   3 +-
 3 files changed, 110 insertions(+), 15 deletions(-)

arch/arm64/include/asm/cpufeature.h +1 -0

index 338840c00e8e..603bf4160cd6 100644
@@ -630,9 +630,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);

arch/arm64/include/asm/cpufeature.h +0 -0

 static inline bool system_supports_4kb_granule(void)

arch/arm64/kernel/cpufeature.c +20 -2

index a4db25cd7122..4194a47de62d 100644
@@ -107,6 +107,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

@@ -767,7 +785,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;

@@ -863,7 +881,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);

arch/arm64/kernel/cpufeature.c +54 -11

+
 static int update_32bit_cpu_features(int cpu, struct cpuinfo_32bit *info,
 				     struct cpuinfo_32bit *boot)
 {

@@ -1139,6 +1173,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);
 	}

@@ -1251,6 +1286,28 @@ has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope)
 	return feature_matches(val, entry);
 }
 
+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_possible_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;

@@ -1869,10 +1926,9 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.cpu_enable = cpu_copy_el2regs,
 	},
 	{
-		.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,

@@ -2381,7 +2437,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:

@@ -2426,7 +2482,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);

@@ -2601,7 +2657,7 @@ static void check_early_cpu_features(void)
 }
 
 static void
-verify_local_elf_hwcaps(const struct arm64_cpu_capabilities *caps)
+__verify_local_elf_hwcaps(const struct arm64_cpu_capabilities *caps)
 {
 
 	for (; caps->matches; caps++)

@@ -2612,6 +2668,14 @@ verify_local_elf_hwcaps(const struct arm64_cpu_capabilities *caps)
 		}
 }
 
+static void verify_local_elf_hwcaps(void)
+{
+	__verify_local_elf_hwcaps(arm64_elf_hwcaps);
+
+	if (id_aa64pfr0_32bit_el0(read_cpuid(ID_AA64PFR0_EL1)))
+		__verify_local_elf_hwcaps(compat_elf_hwcaps);
+}
+
 static void verify_sve_features(void)
 {
 	u64 safe_zcr = read_sanitised_ftr_reg(SYS_ZCR_EL1);

@@ -2676,11 +2740,7 @@ static void verify_local_cpu_capabilities(void)
 	 * on all secondary CPUs.
 	 */
 	verify_local_cpu_caps(SCOPE_ALL & ~SCOPE_BOOT_CPU);
-
-	verify_local_elf_hwcaps(arm64_elf_hwcaps);
-
-	if (system_supports_32bit_el0())
-		verify_local_elf_hwcaps(compat_elf_hwcaps);
+	verify_local_elf_hwcaps();
 
 	if (system_supports_sve())
 		verify_sve_features();

@@ -2815,6 +2875,34 @@ void __init setup_cpu_features(void)
 			ARCH_DMA_MINALIGN);
 }
 
+static int enable_mismatched_32bit_el0(unsigned int cpu)
+{
+	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);
+	}
+
+	return 0;
+}
+
+static int __init init_32bit_el0_mask(void)
+{

arch/arm64/kernel/cpufeature.c +2 -0

+subsys_initcall_sync(init_32bit_el0_mask);
+
 static void __maybe_unused cpu_enable_cnp(struct arm64_cpu_capabilities const *cap)
 {
 	cpu_replace_ttbr1(lm_alias(swapper_pg_dir));

arch/arm64/tools/cpucaps +2 -1

index 21fbdda7086e..49305c2e6dfd 100644
@@ -3,7 +3,8 @@
 # Internal CPU capabilities constants, keep this list sorted
 
 BTI
-HAS_32BIT_EL0
+# Unreliable: use system_supports_32bit_el0() instead.
+HAS_32BIT_EL0_DO_NOT_USE
 HAS_32BIT_EL1
 HAS_ADDRESS_AUTH
 HAS_ADDRESS_AUTH_ARCH

Signature · 1 line

2.32.0.rc0.204.g9fa02ecfa5-goog

Quoted 27 lines · Will Deacon

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.

Reviewed-by Catalin Marinas <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/cpufeature.h |   8 +-
 arch/arm64/kernel/cpufeature.c      | 114 ++++++++++++++++++++++++----
 arch/arm64/tools/cpucaps            |   3 +-
 3 files changed, 110 insertions(+), 15 deletions(-)

arch/arm64/include/asm/cpufeature.h +0 -0

index 338840c00e8e..603bf4160cd6 100644
@@ -630,9 +630,15 @@ static inline bool cpu_supports_mixed_endian_el0(void)
 	return id_aa64mmfr0_mixed_endian_el0(read_cpuid(ID_AA64MMFR0_EL1));
 }
 

arch/arm64/include/asm/cpufeature.h static inline bool cpu_supports_mixed_endian_el0(void) · line 642

Quoted 11 lines

arch/arm64/include/asm/cpufeature.h +7 -1

+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 static_branch_unlikely(&arm64_mismatched_32bit_el0) ||
+	       id_aa64pfr0_32bit_el0(pfr0);
 }

Note that read_sanitised_ftr_reg() has to do a bsearch() to find the arm64_ftr_reg, so this will make system_32bit_el0_cpumask() a fair amount more expensive than it needs to be.

Can we follow the pattern we have for arm64_ftr_reg_ctrel0, and have a arm64_ftr_reg_id_aa64pfr0_el1 that we can address directly here?

That said. I reckon this could be much cleaner if we maintained separate caps:

ARM64_ALL_CPUS_HAVE_32BIT_EL0
ARM64_SOME_CPUS_HAVE_32BIT_EL0

... and allow arm64_mismatched_32bit_el0 to be set dependent on ARM64_SOME_CPUS_HAVE_32BIT_EL0. With that, this can be simplified to:

static inline bool system_supports_32bit_el0(void)

{
	return (cpus_have_const_cap(ARM64_ALL_CPUS_HAVE_32BIT_EL0)) ||
		static_branch_unlikely(&arm64_mismatched_32bit_el0))

Quoted 49 lines

arch/arm64/include/asm/cpufeature.h +0 -0

 static inline bool system_supports_4kb_granule(void)

arch/arm64/kernel/cpufeature.c +20 -2

index a4db25cd7122..4194a47de62d 100644
@@ -107,6 +107,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

@@ -767,7 +785,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;

@@ -863,7 +881,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);

arch/arm64/kernel/cpufeature.c static void relax_cpu_ftr_reg(u32 sys_id, int field) · line 1001

Quoted 5 lines

arch/arm64/kernel/cpufeature.c +2 -0

@@ -979,6 +997,22 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field)
 	WARN_ON(!ftrp->width);
 }
 
+static void update_mismatched_32bit_el0_cpu_features(struct cpuinfo_arm64 *info,
+						     struct cpuinfo_arm64 *boot)

Could we s/update/lazy_init/ here?

IIUC this caters for the case where CPU0 doesn't have AArch32 but a secondary does. That, and the naming looks odd in update_cpu_features() when we have:

	update_mismatched_32bit_el0_cpu_features(...)
	update_32bit_cpu_features(...);

arch/arm64/kernel/cpufeature.c static void relax_cpu_ftr_reg(u32 sys_id, int field) · line 1014

Quoted 13 lines

arch/arm64/kernel/cpufeature.c +13 -0

+{
+	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);
+	boot_cpu_32bit_regs_overridden = true;
+}

Can't we share this with the boot CPU path if we do:

/*
* Initialize the common AArch32 features on the first CPU with AArch32. */
static void lazy_init_32bit_el0_cpu_features(struct cpuinfo_arm64 *info,
struct cpuinfo_arm64 *boot)

{
	static bool initialised = false;
	if (initialised || !id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0))
		return;
	
	boot->aarch32 = info->aarch32;
	init_32bit_cpu_features(&boot->aarch32);
	initiaised = true;
}

... or is the allow_mismatched_32bit_el0 check necessary for late hotplug?

Quoted 6 lines

arch/arm64/kernel/cpufeature.c +6 -0

+static int __init init_32bit_el0_mask(void)
+{
+	if (!allow_mismatched_32bit_el0)
+		return 0;
+
+	if (!zalloc_cpumask_var(&cpu_32bit_el0_mask, GFP_KERNEL))

arch/arm64/kernel/cpufeature.c void __init setup_cpu_features(void) · line 2903

Quoted 6 lines

arch/arm64/kernel/cpufeature.c +6 -0

+		return -ENOMEM;
+
+	return cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+				 "arm64/mismatched_32bit_el0:online",
+				 enable_mismatched_32bit_el0, NULL);
+}

Shouldn't we clear this on a hot-unplug?

Quoted 28 lines · Mark Rutland

Quoted 27 lines · Will Deacon

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.

Reviewed-by Catalin Marinas <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/cpufeature.h |   8 +-
 arch/arm64/kernel/cpufeature.c      | 114 ++++++++++++++++++++++++----
 arch/arm64/tools/cpucaps            |   3 +-
 3 files changed, 110 insertions(+), 15 deletions(-)

arch/arm64/include/asm/cpufeature.h +0 -0

index 338840c00e8e..603bf4160cd6 100644
@@ -630,9 +630,15 @@ static inline bool cpu_supports_mixed_endian_el0(void)
 	return id_aa64mmfr0_mixed_endian_el0(read_cpuid(ID_AA64MMFR0_EL1));
 }
 

Quoted 15 lines

arch/arm64/include/asm/cpufeature.h static inline bool cpu_supports_mixed_endian_el0(void) · line 642

Quoted 11 lines

arch/arm64/include/asm/cpufeature.h +7 -1

+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 static_branch_unlikely(&arm64_mismatched_32bit_el0) ||
+	       id_aa64pfr0_32bit_el0(pfr0);
 }

Note that read_sanitised_ftr_reg() has to do a bsearch() to find the arm64_ftr_reg, so this will make system_32bit_el0_cpumask() a fair amount more expensive than it needs to be.

I seriously doubt that it matters, but it did come up before and I proposed a potential solution if it's actually a concern:

https://lore.kernel.org/r/20201202172727.GC29813@willie-the-truck

so if you can show that it's a problem, we can resurrect something like that.

Quoted 2 lines

Can we follow the pattern we have for arm64_ftr_reg_ctrel0, and have a arm64_ftr_reg_id_aa64pfr0_el1 that we can address directly here?

I mean, clearly its possible, but based on what data?

Quoted 9 lines

That said. I reckon this could be much cleaner if we maintained separate caps:

ARM64_ALL_CPUS_HAVE_32BIT_EL0
ARM64_SOME_CPUS_HAVE_32BIT_EL0

... and allow arm64_mismatched_32bit_el0 to be set dependent on ARM64_SOME_CPUS_HAVE_32BIT_EL0. With that, this can be simplified to:

Quoted 4 lines

static inline bool system_supports_32bit_el0(void)

{
	return (cpus_have_const_cap(ARM64_ALL_CPUS_HAVE_32BIT_EL0)) ||
		static_branch_unlikely(&arm64_mismatched_32bit_el0))

Something similar was discussed in November last year but this falls apart with late onlining because its not generally possible to tell whether you've seen all the CPUs or not.

I'm mostly reluctant to make significant changes based on cosmetic preferences because testing and debugging this with all the system combinations is really difficult.

Quoted 31 lines

arch/arm64/include/asm/cpufeature.h static inline bool cpu_supports_mixed_endian_el0(void)

Quoted 13 lines

arch/arm64/include/asm/cpufeature.h +13 -0

+{
+	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);
+	boot_cpu_32bit_regs_overridden = true;
+}

Can't we share this with the boot CPU path if we do:

/*
* Initialize the common AArch32 features on the first CPU with AArch32. */
static void lazy_init_32bit_el0_cpu_features(struct cpuinfo_arm64 *info,
struct cpuinfo_arm64 *boot)

{
	static bool initialised = false;
	if (initialised || !id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0))
		return;
	
	boot->aarch32 = info->aarch32;
	init_32bit_cpu_features(&boot->aarch32);
	initiaised = true;
}

Quoted 2 lines

... or is the allow_mismatched_32bit_el0 check necessary for late hotplug?

Interesting. I think this works, but I'm wary that it results in the 32-bit features of a 64-bit-only boot CPU being populated using the first 32-bit-capable CPU even if we're not running with allow_mismatched_32bit_el0 enabled. That feels like setting ourselves up for future bugs. For example, compat_has_neon() would unexpectedly return true even though 32-bit execve() would be forbidden.

Quoted 29 lines · Will Deacon

Quoted 9 lines · Mark Rutland

Quoted 8 lines · Will Deacon

arch/arm64/include/asm/cpufeature.h +0 -0

index 338840c00e8e..603bf4160cd6 100644
@@ -630,9 +630,15 @@ static inline bool cpu_supports_mixed_endian_el0(void)
 	return id_aa64mmfr0_mixed_endian_el0(read_cpuid(ID_AA64MMFR0_EL1));
 }
 

Quoted 15 lines

arch/arm64/include/asm/cpufeature.h static inline bool cpu_supports_mixed_endian_el0(void) · line 642

Quoted 11 lines

arch/arm64/include/asm/cpufeature.h +7 -1

+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 static_branch_unlikely(&arm64_mismatched_32bit_el0) ||
+	       id_aa64pfr0_32bit_el0(pfr0);
 }

Note that read_sanitised_ftr_reg() has to do a bsearch() to find the arm64_ftr_reg, so this will make system_32bit_el0_cpumask() a fair amount more expensive than it needs to be.

I seriously doubt that it matters, but it did come up before and I proposed a potential solution if it's actually a concern:

Quoted 4 lines

https://lore.kernel.org/r/20201202172727.GC29813@willie-the-truck

so if you can show that it's a problem, we can resurrect something like that.

I'm happy to leave that for future. I raised this because elsewhere this is an issue when we need to avoid instrumentation; if that's not a concern here on any path then I am not aware of a functional issue.

Quoted 9 lines

Quoted 9 lines

That said. I reckon this could be much cleaner if we maintained separate caps:

ARM64_ALL_CPUS_HAVE_32BIT_EL0
ARM64_SOME_CPUS_HAVE_32BIT_EL0

... and allow arm64_mismatched_32bit_el0 to be set dependent on ARM64_SOME_CPUS_HAVE_32BIT_EL0. With that, this can be simplified to:

Quoted 8 lines

Quoted 4 lines

static inline bool system_supports_32bit_el0(void)

{
	return (cpus_have_const_cap(ARM64_ALL_CPUS_HAVE_32BIT_EL0)) ||
		static_branch_unlikely(&arm64_mismatched_32bit_el0))

Something similar was discussed in November last year but this falls apart with late onlining because its not generally possible to tell whether you've seen all the CPUs or not.

Ah; is that for when your boot CPU set is all AArch32-capable, but a late-onlined CPU is not?

I assume that we require at least one of the set of boot CPUs to be AArch32 cpable, and don't settle the compat hwcaps after userspace has started.

Quoted 23 lines · Mark Rutland

Quoted 13 lines · Will Deacon

Quoted 12 lines · Mark Rutland

arch/arm64/include/asm/cpufeature.h

Quoted 1 line · Will Deacon

arch/arm64/include/asm/cpufeature.h +0 -0

That said. I reckon this could be much cleaner if we maintained separate caps:

ARM64_ALL_CPUS_HAVE_32BIT_EL0
ARM64_SOME_CPUS_HAVE_32BIT_EL0

... and allow arm64_mismatched_32bit_el0 to be set dependent on ARM64_SOME_CPUS_HAVE_32BIT_EL0. With that, this can be simplified to:

Quoted 8 lines

Quoted 4 lines

static inline bool system_supports_32bit_el0(void)

{
	return (cpus_have_const_cap(ARM64_ALL_CPUS_HAVE_32BIT_EL0)) ||
		static_branch_unlikely(&arm64_mismatched_32bit_el0))

Something similar was discussed in November last year but this falls apart with late onlining because its not generally possible to tell whether you've seen all the CPUs or not.

Quoted 6 lines

Ah; is that for when your boot CPU set is all AArch32-capable, but a late-onlined CPU is not?

I assume that we require at least one of the set of boot CPUs to be AArch32 cpable, and don't settle the compat hwcaps after userspace has started.

Heh, you assume wrong :)

When we allow the mismatch, then we do actually defer initialisation of the compat hwcaps until we see a 32-bit CPU. That's fine, as they won't be visible to userspace until then anyway (PER_LINUX32 is unavailable).

Quoted 31 lines · Will Deacon

Quoted 23 lines · Mark Rutland

Quoted 13 lines · Will Deacon

Quoted 12 lines · Mark Rutland

arch/arm64/include/asm/cpufeature.h

Quoted 1 line · Will Deacon

arch/arm64/include/asm/cpufeature.h +0 -0

That said. I reckon this could be much cleaner if we maintained separate caps:

ARM64_ALL_CPUS_HAVE_32BIT_EL0
ARM64_SOME_CPUS_HAVE_32BIT_EL0

... and allow arm64_mismatched_32bit_el0 to be set dependent on ARM64_SOME_CPUS_HAVE_32BIT_EL0. With that, this can be simplified to:

Quoted 8 lines

Quoted 4 lines

static inline bool system_supports_32bit_el0(void)

{
	return (cpus_have_const_cap(ARM64_ALL_CPUS_HAVE_32BIT_EL0)) ||
		static_branch_unlikely(&arm64_mismatched_32bit_el0))

Something similar was discussed in November last year but this falls apart with late onlining because its not generally possible to tell whether you've seen all the CPUs or not.

Quoted 6 lines

Ah; is that for when your boot CPU set is all AArch32-capable, but a late-onlined CPU is not?

I assume that we require at least one of the set of boot CPUs to be AArch32 cpable, and don't settle the compat hwcaps after userspace has started.

Quoted 5 lines

Heh, you assume wrong :)

When we allow the mismatch, then we do actually defer initialisation of the compat hwcaps until we see a 32-bit CPU. That's fine, as they won't be visible to userspace until then anyway (PER_LINUX32 is unavailable).

That sounds quite scary, to me, though I don't have a concrete problem to hand. :/

Do we really need to support initializing that so late? For all other caps we've settled things when the boot CPUs come up, and it's unfortunate to have to treat this differently.

I'll go see if there's anything that's liable to break today.

Asymmetric systems may not offer the same level of userspace ISA support across all CPUs, meaning that some applications cannot be executed by some CPUs. As a concrete example, upcoming arm64 big.LITTLE designs do not feature support for 32-bit applications on both clusters.

On such a system, we must take care not to migrate a task to an unsupported CPU when forcefully moving tasks in select_fallback_rq() in response to a CPU hot-unplug operation.

Introduce a task_cpu_possible_mask() hook which, given a task argument, allows an architecture to return a cpumask of CPUs that are capable of executing that task. The default implementation returns the cpu_possible_mask, since sane machines do not suffer from per-cpu ISA limitations that affect scheduling. The new mask is used when selecting the fallback runqueue as a last resort before forcing a migration to the first active CPU.

Quoted 9 lines · Will Deacon

Asymmetric systems may not offer the same level of userspace ISA support across all CPUs, meaning that some applications cannot be executed by some CPUs. As a concrete example, upcoming arm64 big.LITTLE designs do not feature support for 32-bit applications on both clusters.

On such a system, we must take care not to migrate a task to an unsupported CPU when forcefully moving tasks in select_fallback_rq() in response to a CPU hot-unplug operation.

Quoted 8 lines

Introduce a task_cpu_possible_mask() hook which, given a task argument, allows an architecture to return a cpumask of CPUs that are capable of executing that task. The default implementation returns the cpu_possible_mask, since sane machines do not suffer from per-cpu ISA limitations that affect scheduling. The new mask is used when selecting the fallback runqueue as a last resort before forcing a migration to the first active CPU.

Nit: Some uses of this mask (cpu_is_allowed(), __set_cpus_allowed_ptr()) don't apply to kthreads. This makes sense for the 32-bit@EL0 faff, but it wouldn't hurt to point this out somewhere IMO.

Also, that's an odd place for the definitions, but IIRC there isn't a much better choice.

Reviewed-by Valentin Schneider <[email protected]>

kernel/cgroup/cpuset.c +0 -1

      rcu_read_unlock();
 
      /*
--

2.32.0.rc0.204.g9fa02ecfa5-goog

Review void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask) · line 3331

Quoted 13 lines · Will Deacon

(quoted diff) +6 -2

@@ -3322,9 +3322,13 @@ void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask)
 
 void cpuset_cpus_allowed_fallback(struct task_struct *tsk)
 {
+	const struct cpumask *cs_mask;
+	const struct cpumask *possible_mask = task_cpu_possible_mask(tsk);
+
      rcu_read_lock();
-	do_set_cpus_allowed(tsk, is_in_v2_mode() ?
-		task_cs(tsk)->cpus_allowed : cpu_possible_mask);
+	cs_mask = task_cs(tsk)->cpus_allowed;
+	if (is_in_v2_mode() && cpumask_subset(cs_mask, possible_mask))
+		do_set_cpus_allowed(tsk, cs_mask);

Since the task will still go through the is_cpu_allowed() loop in select_fallback_rq() after this, is the subset check actually required here?

It would have more merit if cpuset_cpus_allowed_fallback() returned whether it actually changed the allowed mask or not, in which case we could branch either to the is_cpu_allowed() loop (as we do unconditionally now), or to the 'state == possible' switch case.

Quoted 18 lines · Valentin Schneider

Review void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask) · line 3331

Quoted 13 lines · Will Deacon

(quoted diff) +6 -2

@@ -3322,9 +3322,13 @@ void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask)
 
 void cpuset_cpus_allowed_fallback(struct task_struct *tsk)
 {
+	const struct cpumask *cs_mask;
+	const struct cpumask *possible_mask = task_cpu_possible_mask(tsk);
+
      rcu_read_lock();
-	do_set_cpus_allowed(tsk, is_in_v2_mode() ?
-		task_cs(tsk)->cpus_allowed : cpu_possible_mask);
+	cs_mask = task_cs(tsk)->cpus_allowed;
+	if (is_in_v2_mode() && cpumask_subset(cs_mask, possible_mask))
+		do_set_cpus_allowed(tsk, cs_mask);

Since the task will still go through the is_cpu_allowed() loop in select_fallback_rq() after this, is the subset check actually required here?

Yes, I think it's needed. do_set_cpus_allowed() doesn't do any checking against the task_cpu_possible_mask, so if we returned to select_fallback_rq() with a mask containing a mixture of 32-bit-capable and 64-bit-only CPUs then we'd end up setting an affinity mask for a 32-bit task which contains 64-bit-only cores.

Asymmetric systems may not offer the same level of userspace ISA support across all CPUs, meaning that some applications cannot be executed by some CPUs. As a concrete example, upcoming arm64 big.LITTLE designs do not feature support for 32-bit applications on both clusters.

Modify guarantee_online_cpus() to take task_cpu_possible_mask() into account when trying to find a suitable set of online CPUs for a given task. This will avoid passing an invalid mask to set_cpus_allowed_ptr() during ->attach() and will subsequently allow the cpuset hierarchy to be taken into account when forcefully overriding the affinity mask for a task which requires migration to a compatible CPU.

Cc Li Zefan <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Johannes Weiner <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Reject explicit requests to change the affinity mask of a task via set_cpus_allowed_ptr() if the requested mask is not a subset of the mask returned by task_cpu_possible_mask(). This ensures that the 'cpus_mask' for a given task cannot contain CPUs which are incapable of executing it, except in cases where the affinity is forced.

Reviewed-by Quentin Perret <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 kernel/sched/core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

kernel/sched/core.c +3 -1

index 0c1b6f1a6c91..b23c7f0ab31a 100644
@@ -2347,15 +2347,17 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
                                u32 flags)
 {
      const struct cpumask *cpu_valid_mask = cpu_active_mask;
+	const struct cpumask *cpu_allowed_mask = task_cpu_possible_mask(p);
      unsigned int dest_cpu;
      struct rq_flags rf;
      struct rq *rq;
      int ret = 0;
+	bool kthread = p->flags & PF_KTHREAD;
 
      rq = task_rq_lock(p, &rf);
      update_rq_clock(rq);
 
-	if (p->flags & PF_KTHREAD || is_migration_disabled(p)) {
+	if (kthread || is_migration_disabled(p)) {
              /*
               * Kernel threads are allowed on online && !active CPUs,
               * however, during cpu-hot-unplug, even these might get pushed

kernel/sched/core.c +0 -1

      /*
       * Must re-check here, to close a race against __kthread_bind(),
       * sched_setaffinity() is not guaranteed to observe the flag.
--

2.32.0.rc0.204.g9fa02ecfa5-goog

Quoted 6 lines · Will Deacon

Reject explicit requests to change the affinity mask of a task via set_cpus_allowed_ptr() if the requested mask is not a subset of the mask returned by task_cpu_possible_mask(). This ensures that the 'cpus_mask' for a given task cannot contain CPUs which are incapable of executing it, except in cases where the affinity is forced.

Quoted 2 lines

Reviewed-by Quentin Perret <[email protected]>

Signed-off-by Will Deacon <[email protected]>

One comment/observation below, but regardless:

Reviewed-by Valentin Schneider <[email protected]>

Quoted 24 lines

kernel/sched/core.c +3 -1

index 0c1b6f1a6c91..b23c7f0ab31a 100644
@@ -2347,15 +2347,17 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
                                u32 flags)
 {
      const struct cpumask *cpu_valid_mask = cpu_active_mask;
+	const struct cpumask *cpu_allowed_mask = task_cpu_possible_mask(p);
      unsigned int dest_cpu;
      struct rq_flags rf;
      struct rq *rq;
      int ret = 0;
+	bool kthread = p->flags & PF_KTHREAD;
 
      rq = task_rq_lock(p, &rf);
      update_rq_clock(rq);
 
-	if (p->flags & PF_KTHREAD || is_migration_disabled(p)) {
+	if (kthread || is_migration_disabled(p)) {
              /*
               * Kernel threads are allowed on online && !active CPUs,
               * however, during cpu-hot-unplug, even these might get pushed

kernel/sched/core.c static int __set_cpus_allowed_ptr(struct task_struct *p, · line 2378

Quoted 8 lines

kernel/sched/core.c +5 -0

@@ -2369,6 +2371,11 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
              cpu_valid_mask = cpu_online_mask;
      }
 
+	if (!kthread && !cpumask_subset(new_mask, cpu_allowed_mask)) {
+		ret = -EINVAL;
+		goto out;
+	}
+

IIUC this wouldn't be required if guarantee_online_cpus() couldn't build a mask that extends beyond task_cpu_possible_mask(p): if the new mask doesn't intersect with that possible mask, it means we're carrying an empty cpumask and the cpumask_any_and_distribute() below would return nr_cpu_ids, so we'd bail with -EINVAL.

I don't really see a way around it though due to the expectations behind guarantee_online_cpus() :/

In preparation for saving and restoring the user-requested CPU affinity mask of a task, add a new cpumask_t pointer to 'struct task_struct'.

If the pointer is non-NULL, then the mask is copied across fork() and freed on task exit.

Signed-off-by Will Deacon <[email protected]>

In preparation for replaying user affinity requests using a saved mask, split sched_setaffinity() up so that the initial task lookup and security checks are only performed when the request is coming directly from userspace.

Signed-off-by Will Deacon <[email protected]>

kernel/sched/core.c +32 -0

+static int restrict_cpus_allowed_ptr(struct task_struct *p,
+				     struct cpumask *new_mask,
+				     const struct cpumask *subset_mask)
+{
+	struct rq_flags rf;
+	struct rq *rq;
+	int err;
+	struct cpumask *user_mask = NULL;
+
+	if (!p->user_cpus_ptr) {
+		user_mask = kmalloc(cpumask_size(), GFP_KERNEL);
+
+		if (!user_mask)
+			return -ENOMEM;
+	}
+
+	rq = task_rq_lock(p, &rf);
+
+	/*
+	 * Forcefully restricting the affinity of a deadline task is
+	 * likely to cause problems, so fail and noisily override the
+	 * mask entirely.
+	 */
+	if (task_has_dl_policy(p) && dl_bandwidth_enabled()) {
+		err = -EPERM;
+		goto err_unlock;
+	}
+
+	if (!cpumask_and(new_mask, &p->cpus_mask, subset_mask)) {
+		err = -EINVAL;
+		goto err_unlock;
+	}

kernel/sched/core.c +17 -0

+	return __set_cpus_allowed_ptr_locked(p, new_mask, 0, rq, &rf);
+
+err_unlock:
+	task_rq_unlock(rq, p, &rf);
+	kfree(user_mask);
+	return err;
+}
+
+/*
+ * Restrict the CPU affinity of task @p so that it is a subset of
+ * task_cpu_possible_mask() and point @p->user_cpu_ptr to a copy of the
+ * old affinity mask. If the resulting mask is empty, we warn and walk
+ * up the cpuset hierarchy until we find a suitable mask.
+ */
+void force_compatible_cpus_allowed_ptr(struct task_struct *p)
+{
+	cpumask_var_t new_mask;

kernel/sched/core.c +40 -0

+	if (!cpumask_available(new_mask))
+		goto out_set_mask;
+
+	if (!restrict_cpus_allowed_ptr(p, new_mask, override_mask))
+		goto out_free_mask;
+
+	/*
+	 * We failed to find a valid subset of the affinity mask for the
+	 * task, so override it based on its cpuset hierarchy.
+	 */
+	cpuset_cpus_allowed(p, new_mask);
+	override_mask = new_mask;
+
+out_set_mask:
+	if (printk_ratelimit()) {
+		printk_deferred("Overriding affinity for process %d (%s) to CPUs %*pbl\n",
+				task_pid_nr(p), p->comm,
+				cpumask_pr_args(override_mask));
+	}
+
+	WARN_ON(set_cpus_allowed_ptr(p, override_mask));
+out_free_mask:
+	cpus_read_unlock();
+	free_cpumask_var(new_mask);
+}
+
+static int
+__sched_setaffinity(struct task_struct *p, const struct cpumask *mask);
+
+/*
+ * Restore the affinity of a task @p which was previously restricted by a
+ * call to force_compatible_cpus_allowed_ptr(). This will clear (and free)
+ * @p->user_cpus_ptr.
+ */
+void relax_compatible_cpus_allowed_ptr(struct task_struct *p)
+{
+	unsigned long flags;
+	struct cpumask *mask = p->user_cpus_ptr;
+
+	/*

Quoted 38 lines · Will Deacon

kernel/sched/core.c +38 -0

+static int restrict_cpus_allowed_ptr(struct task_struct *p,
+				     struct cpumask *new_mask,
+				     const struct cpumask *subset_mask)
+{
+	struct rq_flags rf;
+	struct rq *rq;
+	int err;
+	struct cpumask *user_mask = NULL;
+
+	if (!p->user_cpus_ptr) {
+		user_mask = kmalloc(cpumask_size(), GFP_KERNEL);
+
+		if (!user_mask)
+			return -ENOMEM;
+	}
+
+	rq = task_rq_lock(p, &rf);
+
+	/*
+	 * Forcefully restricting the affinity of a deadline task is
+	 * likely to cause problems, so fail and noisily override the
+	 * mask entirely.
+	 */
+	if (task_has_dl_policy(p) && dl_bandwidth_enabled()) {
+		err = -EPERM;
+		goto err_unlock;
+	}
+
+	if (!cpumask_and(new_mask, &p->cpus_mask, subset_mask)) {
+		err = -EINVAL;
+		goto err_unlock;
+	}
+
+	/*
+	 * We're about to butcher the task affinity, so keep track of what
+	 * the user asked for in case we're able to restore it later on.
+	 */
+	if (user_mask) {

kernel/sched/core.c static int __set_cpus_allowed_ptr(struct task_struct *p, · line 2512

Quoted 4 lines

kernel/sched/core.c +4 -0

+		cpumask_copy(user_mask, p->cpus_ptr);
+		p->user_cpus_ptr = user_mask;
+	}
+

Shouldn't that be done before any of the bailouts above, so we can potentially restore the mask even if we end up forcefully expanding the affinity?

Quoted 17 lines

kernel/sched/core.c +17 -0

+	return __set_cpus_allowed_ptr_locked(p, new_mask, 0, rq, &rf);
+
+err_unlock:
+	task_rq_unlock(rq, p, &rf);
+	kfree(user_mask);
+	return err;
+}
+
+/*
+ * Restrict the CPU affinity of task @p so that it is a subset of
+ * task_cpu_possible_mask() and point @p->user_cpu_ptr to a copy of the
+ * old affinity mask. If the resulting mask is empty, we warn and walk
+ * up the cpuset hierarchy until we find a suitable mask.
+ */
+void force_compatible_cpus_allowed_ptr(struct task_struct *p)
+{
+	cpumask_var_t new_mask;

kernel/sched/core.c static int __set_cpus_allowed_ptr(struct task_struct *p, · line 2539

Quoted 10 lines

kernel/sched/core.c +10 -0

+	const struct cpumask *override_mask = task_cpu_possible_mask(p);
+
+	alloc_cpumask_var(&new_mask, GFP_KERNEL);
+
+	/*
+	 * __migrate_task() can fail silently in the face of concurrent
+	 * offlining of the chosen destination CPU, so take the hotplug
+	 * lock to ensure that the migration succeeds.
+	 */
+	cpus_read_lock();

I'm thinking this might not be required with:

http://lore.kernel.org/r/[email protected]

but then again this isn't merged yet :-)

Quoted 22 lines

kernel/sched/core.c +22 -0

+static int
+__sched_setaffinity(struct task_struct *p, const struct cpumask *mask);
+
+/*
+ * Restore the affinity of a task @p which was previously restricted by a
+ * call to force_compatible_cpus_allowed_ptr(). This will clear (and free)
+ * @p->user_cpus_ptr.
+ */
+void relax_compatible_cpus_allowed_ptr(struct task_struct *p)
+{
+	unsigned long flags;
+	struct cpumask *mask = p->user_cpus_ptr;
+
+	/*
+	 * Try to restore the old affinity mask. If this fails, then
+	 * we free the mask explicitly to avoid it being inherited across
+	 * a subsequent fork().
+	 */
+	if (!mask || !__sched_setaffinity(p, mask))
+		return;
+
+	raw_spin_lock_irqsave(&p->pi_lock, flags);

kernel/sched/core.c static int __set_cpus_allowed_ptr(struct task_struct *p, · line 2589

Quoted 2 lines

kernel/sched/core.c +2 -0

+	release_user_cpus_ptr(p);
+	raw_spin_unlock_irqrestore(&p->pi_lock, flags);

AFAICT an affinity change can happen between __sched_setaffinity() and reacquiring the ->pi_lock. Right now this can't be another force_compatible_cpus_allowed_ptr() because this is only driven by arch_setup_new_exec() against current, so we should be fine, but here be dragons.

Quoted 21 lines · Valentin Schneider

Quoted 20 lines · Will Deacon

kernel/sched/core.c +20 -0

+	/*
+	 * Forcefully restricting the affinity of a deadline task is
+	 * likely to cause problems, so fail and noisily override the
+	 * mask entirely.
+	 */
+	if (task_has_dl_policy(p) && dl_bandwidth_enabled()) {
+		err = -EPERM;
+		goto err_unlock;
+	}
+
+	if (!cpumask_and(new_mask, &p->cpus_mask, subset_mask)) {
+		err = -EINVAL;
+		goto err_unlock;
+	}
+
+	/*
+	 * We're about to butcher the task affinity, so keep track of what
+	 * the user asked for in case we're able to restore it later on.
+	 */
+	if (user_mask) {

Quoted 8 lines

kernel/sched/core.c static int __set_cpus_allowed_ptr(struct task_struct *p, · line 2512

Quoted 4 lines

kernel/sched/core.c +4 -0

+		cpumask_copy(user_mask, p->cpus_ptr);
+		p->user_cpus_ptr = user_mask;
+	}
+

Shouldn't that be done before any of the bailouts above, so we can potentially restore the mask even if we end up forcefully expanding the affinity?

I don't think so. I deliberately only track the old mask if we've managed to take a subset for the 32-bit task. If we end up having to override the mask entirely, then I treat it the same way as an explicit affinity change (only with a warning printed) and don't then try to restore the old mask -- it feels like we'd be overriding the affinity twice if we tried to do that.

In preparation for restricting the affinity of a task during execve() on arm64, introduce a new task_cpus_dl_admissible() helper function to give an indication as to whether the restricted mask is admissible for a deadline task.

Signed-off-by Will Deacon <[email protected]>

---
 include/linux/sched.h |  6 ++++++
 kernel/sched/core.c   | 44 +++++++++++++++++++++++++++----------------
 2 files changed, 34 insertions(+), 16 deletions(-)

include/linux/sched.h +6 -0

index 91a6cfeae242..9b17d8cfa6ef 100644
@@ -1691,6 +1691,7 @@ extern void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new
 extern int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask);
 extern int dup_user_cpus_ptr(struct task_struct *dst, struct task_struct *src, int node);
 extern void release_user_cpus_ptr(struct task_struct *p);
+extern bool task_cpus_dl_admissible(struct task_struct *p, const struct cpumask *mask);
 extern void force_compatible_cpus_allowed_ptr(struct task_struct *p);
 extern void relax_compatible_cpus_allowed_ptr(struct task_struct *p);
 #else

@@ -1713,6 +1714,11 @@ static inline void release_user_cpus_ptr(struct task_struct *p)
 {
 	WARN_ON(p->user_cpus_ptr);
 }
+
+static inline bool task_cpus_dl_admissible(struct task_struct *p, const struct cpumask *mask)
+{
+	return true;
+}
 #endif
 
 extern int yield_to(struct task_struct *p, bool preempt);

kernel/sched/core.c +0 -0

index 58e2cf7520c0..b4f8dc18ae11 100644

Quoted 43 lines · Will Deacon

In preparation for restricting the affinity of a task during execve() on arm64, introduce a new task_cpus_dl_admissible() helper function to give an indication as to whether the restricted mask is admissible for a deadline task.

Signed-off-by Will Deacon <[email protected]>

---
 include/linux/sched.h |  6 ++++++
 kernel/sched/core.c   | 44 +++++++++++++++++++++++++++----------------
 2 files changed, 34 insertions(+), 16 deletions(-)

include/linux/sched.h +6 -0

index 91a6cfeae242..9b17d8cfa6ef 100644
@@ -1691,6 +1691,7 @@ extern void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new
 extern int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask);
 extern int dup_user_cpus_ptr(struct task_struct *dst, struct task_struct *src, int node);
 extern void release_user_cpus_ptr(struct task_struct *p);
+extern bool task_cpus_dl_admissible(struct task_struct *p, const struct cpumask *mask);
 extern void force_compatible_cpus_allowed_ptr(struct task_struct *p);
 extern void relax_compatible_cpus_allowed_ptr(struct task_struct *p);
 #else

@@ -1713,6 +1714,11 @@ static inline void release_user_cpus_ptr(struct task_struct *p)
 {
 	WARN_ON(p->user_cpus_ptr);
 }
+
+static inline bool task_cpus_dl_admissible(struct task_struct *p, const struct cpumask *mask)
+{
+	return true;
+}
 #endif
 
 extern int yield_to(struct task_struct *p, bool preempt);

kernel/sched/core.c +0 -0

index 58e2cf7520c0..b4f8dc18ae11 100644
@@ -6933,6 +6933,31 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
 	return retval;
 }
 

kernel/sched/core.c SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr, · line 6937

Quoted 2 lines

kernel/sched/core.c +2 -0

+#ifdef CONFIG_SMP
+bool task_cpus_dl_admissible(struct task_struct *p, const struct cpumask *mask)

Would you mind renaming it to dl_task_check_affinity(), in the case of a v9? It will look coherent with dl_task_can_attach()...

Reviewed-by Daniel Bristot de Oliveira <[email protected]>

When exec'ing a 32-bit task on a system with mismatched support for 32-bit EL0, try to ensure that it starts life on a CPU that can actually run it.

Similarly, when exec'ing a 64-bit task on such a system, try to restore the old affinity mask if it was previously restricted.

Reviewed-by Quentin Perret <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---

arch/arm64/kernel/process.c +11 -0

+#ifdef CONFIG_COMPAT
+int compat_elf_check_arch(const struct elf32_hdr *hdr)
+{
+	if (!system_supports_32bit_el0())
+		return false;
+
+	if ((hdr)->e_machine != EM_ARM)
+		return false;
+
+	if (!((hdr)->e_flags & EF_ARM_EABI_MASK))
+		return false;

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.

Reviewed-by Catalin Marinas <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/kernel/cpufeature.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

arch/arm64/kernel/cpufeature.c +0 -0

index 4194a47de62d..b31d7a1eaed6 100644

arch/arm64/kernel/cpufeature.c +10 -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

Quoted 23 lines · Will Deacon

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.

Reviewed-by Catalin Marinas <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/kernel/cpufeature.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

arch/arm64/kernel/cpufeature.c +0 -0

index 4194a47de62d..b31d7a1eaed6 100644
@@ -2877,15 +2877,33 @@ void __init setup_cpu_features(void)
 

arch/arm64/kernel/cpufeature.c void __init setup_cpu_features(void) · line 2880

Quoted 3 lines

arch/arm64/kernel/cpufeature.c +1 -0

 static int enable_mismatched_32bit_el0(unsigned int cpu)
 {
+	static int lucky_winner = -1;

This is cute, but could we please give it a meaningful name, e.g. pinned_cpu ?

Quoted 22 lines

arch/arm64/kernel/cpufeature.c +13 -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);

arch/arm64/kernel/cpufeature.c void __init setup_cpu_features(void) · line 2908

Quoted 6 lines

arch/arm64/kernel/cpufeature.c +4 -0

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

I guess this is going to play havoc with kexec and hibernate. :/

Quoted 24 lines · Mark Rutland

Quoted 23 lines · Will Deacon

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.

Reviewed-by Catalin Marinas <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/kernel/cpufeature.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

arch/arm64/kernel/cpufeature.c +0 -0

index 4194a47de62d..b31d7a1eaed6 100644
@@ -2877,15 +2877,33 @@ void __init setup_cpu_features(void)
 

Quoted 6 lines

arch/arm64/kernel/cpufeature.c void __init setup_cpu_features(void) · line 2880

Quoted 3 lines

arch/arm64/kernel/cpufeature.c +1 -0

 static int enable_mismatched_32bit_el0(unsigned int cpu)
 {
+	static int lucky_winner = -1;

This is cute, but could we please give it a meaningful name, e.g. pinned_cpu ?

I really don't see the problem, nor why it's "cute".

Tell you what, I'll add a comment instead:

/*
* The first 32-bit-capable CPU we detected and so can no longer
* be offlined by userspace. -1 indicates we haven't yet onlined
* a 32-bit-capable CPU.
*/

Quoted 22 lines

Quoted 22 lines

arch/arm64/kernel/cpufeature.c +13 -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);

Quoted 8 lines

arch/arm64/kernel/cpufeature.c void __init setup_cpu_features(void) · line 2908

Quoted 6 lines

arch/arm64/kernel/cpufeature.c +4 -0

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

I guess this is going to play havoc with kexec and hibernate. :/

The kernel can still offline the CPUs (see the whole freezer mess that I linked to in the cover letter). What specific havoc are you thinking of?

Quoted 34 lines · Will Deacon

Quoted 24 lines · Mark Rutland

Quoted 23 lines · Will Deacon

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.

Reviewed-by Catalin Marinas <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/kernel/cpufeature.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

arch/arm64/kernel/cpufeature.c +0 -0

index 4194a47de62d..b31d7a1eaed6 100644
@@ -2877,15 +2877,33 @@ void __init setup_cpu_features(void)
 

Quoted 6 lines

arch/arm64/kernel/cpufeature.c void __init setup_cpu_features(void) · line 2880

Quoted 3 lines

arch/arm64/kernel/cpufeature.c +1 -0

 static int enable_mismatched_32bit_el0(unsigned int cpu)
 {
+	static int lucky_winner = -1;

This is cute, but could we please give it a meaningful name, e.g. pinned_cpu ?

I really don't see the problem, nor why it's "cute".

Quoted 7 lines

Tell you what, I'll add a comment instead:

/*
* The first 32-bit-capable CPU we detected and so can no longer
* be offlined by userspace. -1 indicates we haven't yet onlined
* a 32-bit-capable CPU.
*/

Thanks for the comment; that's helpful.

However, my concern here is that when we inevitably have to discuss this with others in future, "lucky winner" is jarring (and also unclear to those where English is not their native language). For clarity, it would be really nice to use a term like "cpu", "chosen_cpu", "pinned_cpu", etc.

However, you're the maintainer; choose what you think is appropriate.

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

arch/arm64/kernel/cpufeature.c +13 -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);

Quoted 8 lines

arch/arm64/kernel/cpufeature.c void __init setup_cpu_features(void) · line 2908

Quoted 6 lines

arch/arm64/kernel/cpufeature.c +4 -0

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

I guess this is going to play havoc with kexec and hibernate. :/

Quoted 2 lines

The kernel can still offline the CPUs (see the whole freezer mess that I linked to in the cover letter). What specific havoc are you thinking of?

Ah. If this is just inhibiting userspace-driven offlining, that sounds fine.

For kexec, I was concerned that either this would inhibit kexec, or smp_shutdown_nonboot_cpus() would fail to offline the pinned CPU, and that'd trigger a BUG(), which would be unfortunate.

For hibernate, the equivalent is freeze_secondary_cpus(), which I guess is dealt with by the freezer bits you mention.