[PATCH v4 00/14] An alternative series for asymmetric AArch32 systems

[PATCH v4 00/14] An alternative series for asymmetric AArch32 systems

[Will Deacon]

Hello folks,

Here's version four of the wonderful patches I previously posted here:

  v1: https://lore.kernel.org/r/20201027215118.27003-1-will@kernel.org
  v2: https://lore.kernel.org/r/20201109213023.15092-1-will@kernel.org
  v3: https://lore.kernel.org/r/20201113093720.21106-1-will@kernel.org

and which started life as a reimplementation of some patches from Qais:

  https://lore.kernel.org/r/20201021104611.2744565-1-qais.yousef@arm.com

The aim of this series is to allow 32-bit ARM applications to run on
arm64 SoCs where not all of the CPUs support the 32-bit instruction set.
Unfortunately, such SoCs are real and will continue to be productised
over the next few years at least.

Changes in v4 include:

  * Take into account the cpuset 'allowed' mask on exec
  * Print a warning if we forcefully override the affinity, like we do
    in select_fallback_rq()
  * Rename arch_cpu_allowed_mask() to arch_task_cpu_possible_mask()
  * Added a comment to adjust_compat_task_affinity()

Cheers,

Will

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Morten Rasmussen <morten.rasmussen@arm.com>
Cc: Qais Yousef <qais.yousef@arm.com>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Quentin Perret <qperret@google.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: kernel-team@android.com

--->8

Will Deacon (14):
  arm64: cpuinfo: Split AArch32 registers out into a separate struct
  arm64: Allow mismatched 32-bit EL0 support
  KVM: arm64: Kill 32-bit vCPUs on systems with mismatched EL0 support
  arm64: Kill 32-bit applications scheduled on 64-bit-only CPUs
  arm64: Advertise CPUs capable of running 32-bit applications in sysfs
  arm64: Hook up cmdline parameter to allow mismatched 32-bit EL0
  sched: Introduce restrict_cpus_allowed_ptr() to limit task CPU
    affinity
  arm64: exec: Adjust affinity for compat tasks with mismatched 32-bit
    EL0
  cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1
  sched: Introduce arch_task_cpu_possible_mask() to limit fallback rq
    selection
  sched: Reject CPU affinity changes based on
    arch_task_cpu_possible_mask()
  arm64: Prevent offlining first CPU with 32-bit EL0 on mismatched
    system
  arm64: Implement arch_task_cpu_possible_mask()
  arm64: Remove logic to kill 32-bit tasks on 64-bit-only cores

 .../ABI/testing/sysfs-devices-system-cpu      |   9 +
 .../admin-guide/kernel-parameters.txt         |   7 +
 arch/arm64/include/asm/cpu.h                  |  44 ++--
 arch/arm64/include/asm/cpucaps.h              |   2 +-
 arch/arm64/include/asm/cpufeature.h           |   8 +-
 arch/arm64/include/asm/mmu_context.h          |  13 ++
 arch/arm64/kernel/cpufeature.c                | 219 ++++++++++++++----
 arch/arm64/kernel/cpuinfo.c                   |  53 +++--
 arch/arm64/kernel/process.c                   |  47 +++-
 arch/arm64/kvm/arm.c                          |  11 +-
 include/linux/sched.h                         |   1 +
 kernel/cgroup/cpuset.c                        |   6 +-
 kernel/sched/core.c                           |  90 +++++--
 13 files changed, 401 insertions(+), 109 deletions(-)

-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v4 01/14] arm64: cpuinfo: Split AArch32 registers out into a separate struct

[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.

Signed-off-by: Will Deacon <will@kernel.org>
---
 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(-)

diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
index 7faae6ff3ab4..f4e01aa0f442 100644
--- a/arch/arm64/include/asm/cpu.h
+++ b/arch/arm64/include/asm/cpu.h
@@ -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;
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index dcc165b3fc04..d4a7e84b1513 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -819,6 +819,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 */
@@ -838,29 +863,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);
@@ -931,20 +935,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.
@@ -1091,10 +1087,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
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 77605aec25fe..8ce33742ad6a 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -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))
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v4 02/14] arm64: Allow mismatched 32-bit EL0 support

[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.

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      | 106 ++++++++++++++++++++++++++--
 3 files changed, 107 insertions(+), 9 deletions(-)

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..bc71a51f1b9c 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,55 @@ 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)
+{
+	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);
+	}
+
+	return 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))
+		return -ENOMEM;
+
+	return cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+				 "arm64/mismatched_32bit_el0:online",
+				 enable_mismatched_32bit_el0, NULL);
+}
+subsys_initcall_sync(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_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;
@@ -1803,10 +1890,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 +2385,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 +2430,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 +2439,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;
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v4 02/14] arm64: Allow mismatched 32-bit EL0 support

[Marc Zyngier]

On 2020-11-24 15:50, Will Deacon wrote:
> 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      | 106 ++++++++++++++++++++++++++--
>  3 files changed, 107 insertions(+), 9 deletions(-)
> 
> 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);

nit: swapping the two sides of this expression has the potential
for slightly better code, resulting in better performance on
these asymmetric systems. Not a big real, but since this lands
on the fast path on vcpu exit, I'll take every bit of optimisation.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 02/14] arm64: Allow mismatched 32-bit EL0 support

[Will Deacon]

On Fri, Nov 27, 2020 at 10:25:06AM +0000, Marc Zyngier wrote:
> On 2020-11-24 15:50, Will Deacon wrote:
> > 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      | 106 ++++++++++++++++++++++++++--
> >  3 files changed, 107 insertions(+), 9 deletions(-)
> > 
> > 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);
> 
> nit: swapping the two sides of this expression has the potential
> for slightly better code, resulting in better performance on
> these asymmetric systems. Not a big real, but since this lands
> on the fast path on vcpu exit, I'll take every bit of optimisation.

I'll swap 'em, thanks.

Will

Re: [PATCH v4 02/14] arm64: Allow mismatched 32-bit EL0 support

[Qais Yousef]

On 11/24/20 15:50, Will Deacon wrote:
> When confronted with a mixture of CPUs, some of which support 32-bit

Confronted made me laugh, well chosen word! :D

For some reason made me think of this :p

	https://www.youtube.com/watch?v=NJbXPzSPzxc&t=1m33s

> 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      | 106 ++++++++++++++++++++++++++--
>  3 files changed, 107 insertions(+), 9 deletions(-)
> 
> 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

nit: would UNUSED be better here? Worth adding a comment as to why too?

>  #define ARM64_HARDEN_EL2_VECTORS		14
>  #define ARM64_HAS_CNP				15
>  #define ARM64_HAS_NO_FPSIMD			16

[...]

> +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 a user passes the command line by mistake on a 64bit only system, this will
return true. I'll be honest, I'm not entirely sure what the impact is. I get
lost in the features maze. It is nicely encapsulated, but hard to navigate for
the none initiated :-)

Thanks

--
Qais Yousef

> +
> +	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 +1890,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 +2385,7 @@ static const struct arm64_cpu_capabilities compat_elf_hwcaps[] = {
>  	{},
>  };

Re: [PATCH v4 02/14] arm64: Allow mismatched 32-bit EL0 support

[Will Deacon]

On Fri, Nov 27, 2020 at 01:09:41PM +0000, Qais Yousef wrote:
> On 11/24/20 15:50, Will Deacon wrote:
> > When confronted with a mixture of CPUs, some of which support 32-bit
> 
> Confronted made me laugh, well chosen word! :D
> 
> For some reason made me think of this :p
> 
> 	https://www.youtube.com/watch?v=NJbXPzSPzxc&t=1m33s

I think it just about sums it up!

> > 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      | 106 ++++++++++++++++++++++++++--
> >  3 files changed, 107 insertions(+), 9 deletions(-)
> > 
> > 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
> 
> nit: would UNUSED be better here? Worth adding a comment as to why too?

UNUSED sounds like you could delete it, but I'll add a comment.

> >  #define ARM64_HARDEN_EL2_VECTORS		14
> >  #define ARM64_HAS_CNP				15
> >  #define ARM64_HAS_NO_FPSIMD			16
> 
> [...]
> 
> > +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 a user passes the command line by mistake on a 64bit only system, this will
> return true. I'll be honest, I'm not entirely sure what the impact is. I get
> lost in the features maze. It is nicely encapsulated, but hard to navigate for
> the none initiated :-)

The thing is, we can't generally detect a 64-bit-only system because a
32-bit-capable CPU could be hotplugged on late. So passing this option
just controls what the behaviour is at the point that the 32-bit-capable
CPU appears. If one doesn't appear, then there won't be a difference.

Will

Re: [PATCH v4 02/14] arm64: Allow mismatched 32-bit EL0 support

[Qais Yousef]

On 12/01/20 16:56, Will Deacon wrote:
> On Fri, Nov 27, 2020 at 01:09:41PM +0000, Qais Yousef wrote:
> > On 11/24/20 15:50, Will Deacon wrote:
> > > When confronted with a mixture of CPUs, some of which support 32-bit
> > 
> > Confronted made me laugh, well chosen word! :D
> > 
> > For some reason made me think of this :p
> > 
> > 	https://www.youtube.com/watch?v=NJbXPzSPzxc&t=1m33s
> 
> I think it just about sums it up!
> 
> > > 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      | 106 ++++++++++++++++++++++++++--
> > >  3 files changed, 107 insertions(+), 9 deletions(-)
> > > 
> > > 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
> > 
> > nit: would UNUSED be better here? Worth adding a comment as to why too?
> 
> UNUSED sounds like you could delete it, but I'll add a comment.

+1, thanks.

> 
> > >  #define ARM64_HARDEN_EL2_VECTORS		14
> > >  #define ARM64_HAS_CNP				15
> > >  #define ARM64_HAS_NO_FPSIMD			16
> > 
> > [...]
> > 
> > > +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 a user passes the command line by mistake on a 64bit only system, this will
> > return true. I'll be honest, I'm not entirely sure what the impact is. I get
> > lost in the features maze. It is nicely encapsulated, but hard to navigate for
> > the none initiated :-)
> 
> The thing is, we can't generally detect a 64-bit-only system because a
> 32-bit-capable CPU could be hotplugged on late. So passing this option
> just controls what the behaviour is at the point that the 32-bit-capable
> CPU appears. If one doesn't appear, then there won't be a difference.

Okay, thanks for confirming.

Cheers

--
Qais Yousef

[PATCH v4 03/14] KVM: arm64: Kill 32-bit vCPUs on systems with mismatched EL0 support

[Will Deacon]

If a vCPU is caught running 32-bit code on a system with mismatched
support at EL0, then we should kill it.

Acked-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kvm/arm.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 5750ec34960e..d322ac0f4a8e 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -633,6 +633,15 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 	}
 }
 
+static bool vcpu_mode_is_bad_32bit(struct kvm_vcpu *vcpu)
+{
+	if (likely(!vcpu_mode_is_32bit(vcpu)))
+		return false;
+
+	return !system_supports_32bit_el0() ||
+		static_branch_unlikely(&arm64_mismatched_32bit_el0);
+}
+
 /**
  * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code
  * @vcpu:	The VCPU pointer
@@ -816,7 +825,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 		 * with the asymmetric AArch32 case), return to userspace with
 		 * a fatal error.
 		 */
-		if (!system_supports_32bit_el0() && vcpu_mode_is_32bit(vcpu)) {
+		if (vcpu_mode_is_bad_32bit(vcpu)) {
 			/*
 			 * As we have caught the guest red-handed, decide that
 			 * it isn't fit for purpose anymore by making the vcpu
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v4 03/14] KVM: arm64: Kill 32-bit vCPUs on systems with mismatched EL0 support

[Marc Zyngier]

On 2020-11-24 15:50, Will Deacon wrote:
> If a vCPU is caught running 32-bit code on a system with mismatched
> support at EL0, then we should kill it.
> 
> Acked-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kvm/arm.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 5750ec34960e..d322ac0f4a8e 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -633,6 +633,15 @@ static void check_vcpu_requests(struct kvm_vcpu 
> *vcpu)
>  	}
>  }
> 
> +static bool vcpu_mode_is_bad_32bit(struct kvm_vcpu *vcpu)
> +{
> +	if (likely(!vcpu_mode_is_32bit(vcpu)))
> +		return false;
> +
> +	return !system_supports_32bit_el0() ||
> +		static_branch_unlikely(&arm64_mismatched_32bit_el0);
> +}
> +
>  /**
>   * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute 
> guest code
>   * @vcpu:	The VCPU pointer
> @@ -816,7 +825,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  		 * with the asymmetric AArch32 case), return to userspace with
>  		 * a fatal error.
>  		 */
> -		if (!system_supports_32bit_el0() && vcpu_mode_is_32bit(vcpu)) {
> +		if (vcpu_mode_is_bad_32bit(vcpu)) {
>  			/*
>  			 * As we have caught the guest red-handed, decide that
>  			 * it isn't fit for purpose anymore by making the vcpu

Given the new definition of system_supports_32bit_el0() in the previous 
patch,
why do we need this patch at all?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 03/14] KVM: arm64: Kill 32-bit vCPUs on systems with mismatched EL0 support

[Will Deacon]

On Fri, Nov 27, 2020 at 10:26:47AM +0000, Marc Zyngier wrote:
> On 2020-11-24 15:50, Will Deacon wrote:
> > If a vCPU is caught running 32-bit code on a system with mismatched
> > support at EL0, then we should kill it.
> > 
> > Acked-by: Marc Zyngier <maz@kernel.org>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/kvm/arm.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 5750ec34960e..d322ac0f4a8e 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -633,6 +633,15 @@ static void check_vcpu_requests(struct kvm_vcpu
> > *vcpu)
> >  	}
> >  }
> > 
> > +static bool vcpu_mode_is_bad_32bit(struct kvm_vcpu *vcpu)
> > +{
> > +	if (likely(!vcpu_mode_is_32bit(vcpu)))
> > +		return false;
> > +
> > +	return !system_supports_32bit_el0() ||
> > +		static_branch_unlikely(&arm64_mismatched_32bit_el0);
> > +}
> > +
> >  /**
> >   * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute
> > guest code
> >   * @vcpu:	The VCPU pointer
> > @@ -816,7 +825,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> >  		 * with the asymmetric AArch32 case), return to userspace with
> >  		 * a fatal error.
> >  		 */
> > -		if (!system_supports_32bit_el0() && vcpu_mode_is_32bit(vcpu)) {
> > +		if (vcpu_mode_is_bad_32bit(vcpu)) {
> >  			/*
> >  			 * As we have caught the guest red-handed, decide that
> >  			 * it isn't fit for purpose anymore by making the vcpu
> 
> Given the new definition of system_supports_32bit_el0() in the previous
> patch,
> why do we need this patch at all?

I think the check is still needed, as this is an unusual case where we
want to reject the mismatched system. For example, imagine
'arm64_mismatched_32bit_el0' is true and we're on a mismatched system: in
this case system_supports_32bit_el0() will return 'true' because we
allow 32-bit applications to run, we support the 32-bit personality etc.

However, we still want to terminate 32-bit vCPUs if we spot them in this
situation, so we have to check for:

	!system_supports_32bit_el0() ||
	static_branch_unlikely(&arm64_mismatched_32bit_el0)

so that we only allow 32-bit vCPUs when all of the physical CPUs support
it at EL0.

I could make this clearer either by adding a comment, or avoiding
system_supports_32bit_el0() entirely here and just checking the
sanitised SYS_ID_AA64PFR0_EL1 register directly instead.

What do you prefer?

Will

Re: [PATCH v4 03/14] KVM: arm64: Kill 32-bit vCPUs on systems with mismatched EL0 support

[Marc Zyngier]

On 2020-11-27 11:53, Will Deacon wrote:
> On Fri, Nov 27, 2020 at 10:26:47AM +0000, Marc Zyngier wrote:
>> On 2020-11-24 15:50, Will Deacon wrote:
>> > If a vCPU is caught running 32-bit code on a system with mismatched
>> > support at EL0, then we should kill it.
>> >
>> > Acked-by: Marc Zyngier <maz@kernel.org>
>> > Signed-off-by: Will Deacon <will@kernel.org>
>> > ---
>> >  arch/arm64/kvm/arm.c | 11 ++++++++++-
>> >  1 file changed, 10 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> > index 5750ec34960e..d322ac0f4a8e 100644
>> > --- a/arch/arm64/kvm/arm.c
>> > +++ b/arch/arm64/kvm/arm.c
>> > @@ -633,6 +633,15 @@ static void check_vcpu_requests(struct kvm_vcpu
>> > *vcpu)
>> >  	}
>> >  }
>> >
>> > +static bool vcpu_mode_is_bad_32bit(struct kvm_vcpu *vcpu)
>> > +{
>> > +	if (likely(!vcpu_mode_is_32bit(vcpu)))
>> > +		return false;
>> > +
>> > +	return !system_supports_32bit_el0() ||
>> > +		static_branch_unlikely(&arm64_mismatched_32bit_el0);
>> > +}
>> > +
>> >  /**
>> >   * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute
>> > guest code
>> >   * @vcpu:	The VCPU pointer
>> > @@ -816,7 +825,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>> >  		 * with the asymmetric AArch32 case), return to userspace with
>> >  		 * a fatal error.
>> >  		 */
>> > -		if (!system_supports_32bit_el0() && vcpu_mode_is_32bit(vcpu)) {
>> > +		if (vcpu_mode_is_bad_32bit(vcpu)) {
>> >  			/*
>> >  			 * As we have caught the guest red-handed, decide that
>> >  			 * it isn't fit for purpose anymore by making the vcpu
>> 
>> Given the new definition of system_supports_32bit_el0() in the 
>> previous
>> patch,
>> why do we need this patch at all?
> 
> I think the check is still needed, as this is an unusual case where we
> want to reject the mismatched system. For example, imagine
> 'arm64_mismatched_32bit_el0' is true and we're on a mismatched system: 
> in
> this case system_supports_32bit_el0() will return 'true' because we
> allow 32-bit applications to run, we support the 32-bit personality 
> etc.
> 
> However, we still want to terminate 32-bit vCPUs if we spot them in 
> this
> situation, so we have to check for:
> 
> 	!system_supports_32bit_el0() ||
> 	static_branch_unlikely(&arm64_mismatched_32bit_el0)
> 
> so that we only allow 32-bit vCPUs when all of the physical CPUs 
> support
> it at EL0.
> 
> I could make this clearer either by adding a comment, or avoiding
> system_supports_32bit_el0() entirely here and just checking the
> sanitised SYS_ID_AA64PFR0_EL1 register directly instead.
> 
> What do you prefer?

Yeah, the sanitized read feels better, if only because that is
what we are going to read in all the valid cases, unfortunately.
read_sanitised_ftr_reg() is sadly not designed to be called on
a fast path, meaning that 32bit guests will do a bsearch() on
the ID-regs every time they exit...

I guess we will have to evaluate how much we loose with this.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 03/14] KVM: arm64: Kill 32-bit vCPUs on systems with mismatched EL0 support

[Quentin Perret]

On Friday 27 Nov 2020 at 17:14:11 (+0000), Marc Zyngier wrote:
> On 2020-11-27 11:53, Will Deacon wrote:
> > On Fri, Nov 27, 2020 at 10:26:47AM +0000, Marc Zyngier wrote:
> > > On 2020-11-24 15:50, Will Deacon wrote:
> > > > If a vCPU is caught running 32-bit code on a system with mismatched
> > > > support at EL0, then we should kill it.
> > > >
> > > > Acked-by: Marc Zyngier <maz@kernel.org>
> > > > Signed-off-by: Will Deacon <will@kernel.org>
> > > > ---
> > > >  arch/arm64/kvm/arm.c | 11 ++++++++++-
> > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > > index 5750ec34960e..d322ac0f4a8e 100644
> > > > --- a/arch/arm64/kvm/arm.c
> > > > +++ b/arch/arm64/kvm/arm.c
> > > > @@ -633,6 +633,15 @@ static void check_vcpu_requests(struct kvm_vcpu
> > > > *vcpu)
> > > >  	}
> > > >  }
> > > >
> > > > +static bool vcpu_mode_is_bad_32bit(struct kvm_vcpu *vcpu)
> > > > +{
> > > > +	if (likely(!vcpu_mode_is_32bit(vcpu)))
> > > > +		return false;
> > > > +
> > > > +	return !system_supports_32bit_el0() ||
> > > > +		static_branch_unlikely(&arm64_mismatched_32bit_el0);
> > > > +}
> > > > +
> > > >  /**
> > > >   * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute
> > > > guest code
> > > >   * @vcpu:	The VCPU pointer
> > > > @@ -816,7 +825,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> > > >  		 * with the asymmetric AArch32 case), return to userspace with
> > > >  		 * a fatal error.
> > > >  		 */
> > > > -		if (!system_supports_32bit_el0() && vcpu_mode_is_32bit(vcpu)) {
> > > > +		if (vcpu_mode_is_bad_32bit(vcpu)) {
> > > >  			/*
> > > >  			 * As we have caught the guest red-handed, decide that
> > > >  			 * it isn't fit for purpose anymore by making the vcpu
> > > 
> > > Given the new definition of system_supports_32bit_el0() in the
> > > previous
> > > patch,
> > > why do we need this patch at all?
> > 
> > I think the check is still needed, as this is an unusual case where we
> > want to reject the mismatched system. For example, imagine
> > 'arm64_mismatched_32bit_el0' is true and we're on a mismatched system:
> > in
> > this case system_supports_32bit_el0() will return 'true' because we
> > allow 32-bit applications to run, we support the 32-bit personality etc.
> > 
> > However, we still want to terminate 32-bit vCPUs if we spot them in this
> > situation, so we have to check for:
> > 
> > 	!system_supports_32bit_el0() ||
> > 	static_branch_unlikely(&arm64_mismatched_32bit_el0)
> > 
> > so that we only allow 32-bit vCPUs when all of the physical CPUs support
> > it at EL0.
> > 
> > I could make this clearer either by adding a comment, or avoiding
> > system_supports_32bit_el0() entirely here and just checking the
> > sanitised SYS_ID_AA64PFR0_EL1 register directly instead.
> > 
> > What do you prefer?
> 
> Yeah, the sanitized read feels better, if only because that is
> what we are going to read in all the valid cases, unfortunately.
> read_sanitised_ftr_reg() is sadly not designed to be called on
> a fast path, meaning that 32bit guests will do a bsearch() on
> the ID-regs every time they exit...
> 
> I guess we will have to evaluate how much we loose with this.

Could we use the trick we have for arm64_ftr_reg_ctrel0 to speed this
up?

Thanks,
Quentin

Re: [PATCH v4 03/14] KVM: arm64: Kill 32-bit vCPUs on systems with mismatched EL0 support

[Marc Zyngier]

On 2020-11-27 17:24, Quentin Perret wrote:
> On Friday 27 Nov 2020 at 17:14:11 (+0000), Marc Zyngier wrote:

[...]

>> Yeah, the sanitized read feels better, if only because that is
>> what we are going to read in all the valid cases, unfortunately.
>> read_sanitised_ftr_reg() is sadly not designed to be called on
>> a fast path, meaning that 32bit guests will do a bsearch() on
>> the ID-regs every time they exit...
>> 
>> I guess we will have to evaluate how much we loose with this.
> 
> Could we use the trick we have for arm64_ftr_reg_ctrel0 to speed this
> up?

Maybe. I want to first verify whether this has any measurable impact.
Another possibility would be to cache the last read_sanitised_ftr_reg()
access, just to see if that helps. There shouldn't be that many code
paths hammering it.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 03/14] KVM: arm64: Kill 32-bit vCPUs on systems with mismatched EL0 support

[Will Deacon]

On Fri, Nov 27, 2020 at 06:16:35PM +0000, Marc Zyngier wrote:
> On 2020-11-27 17:24, Quentin Perret wrote:
> > On Friday 27 Nov 2020 at 17:14:11 (+0000), Marc Zyngier wrote:
> 
> [...]
> 
> > > Yeah, the sanitized read feels better, if only because that is
> > > what we are going to read in all the valid cases, unfortunately.
> > > read_sanitised_ftr_reg() is sadly not designed to be called on
> > > a fast path, meaning that 32bit guests will do a bsearch() on
> > > the ID-regs every time they exit...
> > > 
> > > I guess we will have to evaluate how much we loose with this.
> > 
> > Could we use the trick we have for arm64_ftr_reg_ctrel0 to speed this
> > up?
> 
> Maybe. I want to first verify whether this has any measurable impact.
> Another possibility would be to cache the last read_sanitised_ftr_reg()
> access, just to see if that helps. There shouldn't be that many code
> paths hammering it.

We don't have huge numbers of ID registers, so the bsearch shouldn't be
too expensive. However, I'd like to remind myself why we can't index into
the feature register array directly as we _should_ know all of this stuff
at compile time, right?

Will

Re: [PATCH v4 03/14] KVM: arm64: Kill 32-bit vCPUs on systems with mismatched EL0 support

[Marc Zyngier]

On 2020-12-01 16:57, Will Deacon wrote:
> On Fri, Nov 27, 2020 at 06:16:35PM +0000, Marc Zyngier wrote:
>> On 2020-11-27 17:24, Quentin Perret wrote:
>> > On Friday 27 Nov 2020 at 17:14:11 (+0000), Marc Zyngier wrote:
>> 
>> [...]
>> 
>> > > Yeah, the sanitized read feels better, if only because that is
>> > > what we are going to read in all the valid cases, unfortunately.
>> > > read_sanitised_ftr_reg() is sadly not designed to be called on
>> > > a fast path, meaning that 32bit guests will do a bsearch() on
>> > > the ID-regs every time they exit...
>> > >
>> > > I guess we will have to evaluate how much we loose with this.
>> >
>> > Could we use the trick we have for arm64_ftr_reg_ctrel0 to speed this
>> > up?
>> 
>> Maybe. I want to first verify whether this has any measurable impact.
>> Another possibility would be to cache the last 
>> read_sanitised_ftr_reg()
>> access, just to see if that helps. There shouldn't be that many code
>> paths hammering it.
> 
> We don't have huge numbers of ID registers, so the bsearch shouldn't be
> too expensive. However, I'd like to remind myself why we can't index 
> into
> the feature register array directly as we _should_ know all of this 
> stuff
> at compile time, right?

Simply because it's not indexed by ID reg. It's just an ordered 
collection,
similar to the for sys_reg emulation in KVM. You can compute the index
ahead of time, but just not at compile time. At least not with the
way the arm64_ftr_regs array is built.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 03/14] KVM: arm64: Kill 32-bit vCPUs on systems with mismatched EL0 support

[Will Deacon]

On Wed, Dec 02, 2020 at 08:18:03AM +0000, Marc Zyngier wrote:
> On 2020-12-01 16:57, Will Deacon wrote:
> > On Fri, Nov 27, 2020 at 06:16:35PM +0000, Marc Zyngier wrote:
> > > On 2020-11-27 17:24, Quentin Perret wrote:
> > > > On Friday 27 Nov 2020 at 17:14:11 (+0000), Marc Zyngier wrote:
> > > 
> > > [...]
> > > 
> > > > > Yeah, the sanitized read feels better, if only because that is
> > > > > what we are going to read in all the valid cases, unfortunately.
> > > > > read_sanitised_ftr_reg() is sadly not designed to be called on
> > > > > a fast path, meaning that 32bit guests will do a bsearch() on
> > > > > the ID-regs every time they exit...
> > > > >
> > > > > I guess we will have to evaluate how much we loose with this.
> > > >
> > > > Could we use the trick we have for arm64_ftr_reg_ctrel0 to speed this
> > > > up?
> > > 
> > > Maybe. I want to first verify whether this has any measurable impact.
> > > Another possibility would be to cache the last
> > > read_sanitised_ftr_reg()
> > > access, just to see if that helps. There shouldn't be that many code
> > > paths hammering it.
> > 
> > We don't have huge numbers of ID registers, so the bsearch shouldn't be
> > too expensive. However, I'd like to remind myself why we can't index
> > into
> > the feature register array directly as we _should_ know all of this
> > stuff
> > at compile time, right?
> 
> Simply because it's not indexed by ID reg. It's just an ordered collection,
> similar to the for sys_reg emulation in KVM. You can compute the index
> ahead of time, but just not at compile time. At least not with the
> way the arm64_ftr_regs array is built.

FWIW, if your testing shows that the bsearch() is costing us, I've hacked
up an interface to access the ID registers directly (see below) which I
can include with this series.

Will

--->8

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index da250e4741bd..23766104d756 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -599,7 +599,49 @@ static inline bool id_aa64pfr0_sve(u64 pfr0)
 void __init setup_cpu_features(void);
 void check_local_cpu_capabilities(void);
 
+#define ARM64_FTR_REG2IDX(id)	id ## _IDX
+enum arm64_ftr_reg_idx {
+	ARM64_FTR_REG2IDX(SYS_ID_PFR0_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_PFR1_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_DFR0_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_MMFR0_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_MMFR1_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_MMFR2_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_MMFR3_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_ISAR0_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_ISAR1_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_ISAR2_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_ISAR3_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_ISAR4_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_ISAR5_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_MMFR4_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_ISAR6_EL1),
+	ARM64_FTR_REG2IDX(SYS_MVFR0_EL1),
+	ARM64_FTR_REG2IDX(SYS_MVFR1_EL1),
+	ARM64_FTR_REG2IDX(SYS_MVFR2_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_PFR2_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_DFR1_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_MMFR5_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_AA64PFR0_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_AA64PFR1_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_AA64ZFR0_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_AA64DFR0_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_AA64DFR1_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_AA64ISAR0_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_AA64ISAR1_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_AA64MMFR0_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_AA64MMFR1_EL1),
+	ARM64_FTR_REG2IDX(SYS_ID_AA64MMFR2_EL1),
+	ARM64_FTR_REG2IDX(SYS_ZCR_EL1),
+	ARM64_FTR_REG2IDX(SYS_CTR_EL0),
+	ARM64_FTR_REG2IDX(SYS_DCZID_EL0),
+	ARM64_FTR_REG2IDX(SYS_CNTFRQ_EL0),
+
+	ARM64_FTR_REG_IDX_MAX,
+};
+
 u64 read_sanitised_ftr_reg(u32 id);
+u64 read_sanitised_ftr_reg_by_idx(enum arm64_ftr_reg_idx idx);
 
 static inline bool cpu_supports_mixed_endian_el0(void)
 {
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 6f36c4f62f69..05223352db5d 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -546,16 +546,18 @@ static const struct arm64_ftr_bits ftr_raz[] = {
 	ARM64_FTR_END,
 };
 
-#define ARM64_FTR_REG(id, table) {		\
-	.sys_id = id,				\
-	.reg = 	&(struct arm64_ftr_reg){	\
-		.name = #id,			\
-		.ftr_bits = &((table)[0]),	\
-	}}
+#define ARM64_FTR_REG(id, table)					\
+	[id ## _IDX] = {							\
+		.sys_id = id,						\
+		.reg	= &(struct arm64_ftr_reg) {			\
+			.name		= #id,				\
+			.ftr_bits	= &((table)[0]),		\
+		}							\
+	}
 
 static const struct __ftr_reg_entry {
 	u32			sys_id;
-	struct arm64_ftr_reg 	*reg;
+	struct arm64_ftr_reg	*reg;
 } arm64_ftr_regs[] = {
 
 	/* Op1 = 0, CRn = 0, CRm = 1 */
@@ -607,7 +609,7 @@ static const struct __ftr_reg_entry {
 	ARM64_FTR_REG(SYS_ZCR_EL1, ftr_zcr),
 
 	/* Op1 = 3, CRn = 0, CRm = 0 */
-	{ SYS_CTR_EL0, &arm64_ftr_reg_ctrel0 },
+	[ARM64_FTR_REG2IDX(SYS_CTR_EL0)] = { SYS_CTR_EL0, &arm64_ftr_reg_ctrel0 },
 	ARM64_FTR_REG(SYS_DCZID_EL0, ftr_dczid),
 
 	/* Op1 = 3, CRn = 14, CRm = 0 */
@@ -1116,6 +1118,18 @@ u64 read_sanitised_ftr_reg(u32 id)
 }
 EXPORT_SYMBOL_GPL(read_sanitised_ftr_reg);
 
+u64 read_sanitised_ftr_reg_by_idx(enum arm64_ftr_reg_idx idx)
+{
+	struct arm64_ftr_reg *regp;
+
+	if (WARN_ON((unsigned)idx >= ARM64_FTR_REG_IDX_MAX))
+		return 0;
+
+	regp = arm64_ftr_regs[idx].reg;
+	return regp->sys_val;
+}
+EXPORT_SYMBOL_GPL(read_sanitised_ftr_reg_by_idx);
+
 #define read_sysreg_case(r)	\
 	case r:		return read_sysreg_s(r)
 
-- 
2.29.2.576.ga3fc446d84-goog

[PATCH v4 04/14] arm64: Kill 32-bit applications scheduled on 64-bit-only CPUs

[Will Deacon]

Scheduling a 32-bit application on a 64-bit-only CPU is a bad idea.

Ensure that 32-bit applications always take the slow-path when returning
to userspace on a system with mismatched support at EL0, so that we can
avoid trying to run on a 64-bit-only CPU and force a SIGKILL instead.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/process.c | 19 ++++++++++++++++++-
 arch/arm64/kernel/signal.c  | 26 ++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 4784011cecac..1540ab0fbf23 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -542,6 +542,15 @@ static void erratum_1418040_thread_switch(struct task_struct *prev,
 	write_sysreg(val, cntkctl_el1);
 }
 
+static void compat_thread_switch(struct task_struct *next)
+{
+	if (!is_compat_thread(task_thread_info(next)))
+		return;
+
+	if (static_branch_unlikely(&arm64_mismatched_32bit_el0))
+		set_tsk_thread_flag(next, TIF_NOTIFY_RESUME);
+}
+
 /*
  * Thread switching.
  */
@@ -558,6 +567,7 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
 	uao_thread_switch(next);
 	ssbs_thread_switch(next);
 	erratum_1418040_thread_switch(prev, next);
+	compat_thread_switch(next);
 
 	/*
 	 * Complete any pending TLB or cache maintenance on this CPU in case
@@ -620,8 +630,15 @@ unsigned long arch_align_stack(unsigned long sp)
  */
 void arch_setup_new_exec(void)
 {
-	current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
+	unsigned long mmflags = 0;
+
+	if (is_compat_task()) {
+		mmflags = MMCF_AARCH32;
+		if (static_branch_unlikely(&arm64_mismatched_32bit_el0))
+			set_tsk_thread_flag(current, TIF_NOTIFY_RESUME);
+	}
 
+	current->mm->context.flags = mmflags;
 	ptrauth_thread_init_user(current);
 
 	if (task_spec_ssb_noexec(current)) {
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index a8184cad8890..bcb6ca2d9a7c 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -911,6 +911,19 @@ static void do_signal(struct pt_regs *regs)
 	restore_saved_sigmask();
 }
 
+static bool cpu_affinity_invalid(struct pt_regs *regs)
+{
+	if (!compat_user_mode(regs))
+		return false;
+
+	/*
+	 * We're preemptible, but a reschedule will cause us to check the
+	 * affinity again.
+	 */
+	return !cpumask_test_cpu(raw_smp_processor_id(),
+				 system_32bit_el0_cpumask());
+}
+
 asmlinkage void do_notify_resume(struct pt_regs *regs,
 				 unsigned long thread_flags)
 {
@@ -948,6 +961,19 @@ asmlinkage void do_notify_resume(struct pt_regs *regs,
 			if (thread_flags & _TIF_NOTIFY_RESUME) {
 				tracehook_notify_resume(regs);
 				rseq_handle_notify_resume(NULL, regs);
+
+				/*
+				 * If we reschedule after checking the affinity
+				 * then we must ensure that TIF_NOTIFY_RESUME
+				 * is set so that we check the affinity again.
+				 * Since tracehook_notify_resume() clears the
+				 * flag, ensure that the compiler doesn't move
+				 * it after the affinity check.
+				 */
+				barrier();
+
+				if (cpu_affinity_invalid(regs))
+					force_sig(SIGKILL);
 			}
 
 			if (thread_flags & _TIF_FOREIGN_FPSTATE)
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v4 04/14] arm64: Kill 32-bit applications scheduled on 64-bit-only CPUs

[Qais Yousef]

On 11/24/20 15:50, Will Deacon wrote:
> Scheduling a 32-bit application on a 64-bit-only CPU is a bad idea.
> 
> Ensure that 32-bit applications always take the slow-path when returning
> to userspace on a system with mismatched support at EL0, so that we can
> avoid trying to run on a 64-bit-only CPU and force a SIGKILL instead.
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> ---

nit: We drop this patch at the end. Can't we avoid it altogether instead?

[...]

> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index a8184cad8890..bcb6ca2d9a7c 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -911,6 +911,19 @@ static void do_signal(struct pt_regs *regs)
>  	restore_saved_sigmask();
>  }
>  
> +static bool cpu_affinity_invalid(struct pt_regs *regs)
> +{
> +	if (!compat_user_mode(regs))
> +		return false;

Silly question. Is there an advantage of using compat_user_mode() vs
is_compat_task()? I see the latter used in the file although struct pt_regs
*regs is passed to the functions calling it.

Nothing's wrong with it, just curious.

Thanks

--
Qais Yousef

> +
> +	/*
> +	 * We're preemptible, but a reschedule will cause us to check the
> +	 * affinity again.
> +	 */
> +	return !cpumask_test_cpu(raw_smp_processor_id(),
> +				 system_32bit_el0_cpumask());
> +}

Re: [PATCH v4 04/14] arm64: Kill 32-bit applications scheduled on 64-bit-only CPUs

[Will Deacon]

On Fri, Nov 27, 2020 at 01:12:17PM +0000, Qais Yousef wrote:
> On 11/24/20 15:50, Will Deacon wrote:
> > Scheduling a 32-bit application on a 64-bit-only CPU is a bad idea.
> > 
> > Ensure that 32-bit applications always take the slow-path when returning
> > to userspace on a system with mismatched support at EL0, so that we can
> > avoid trying to run on a 64-bit-only CPU and force a SIGKILL instead.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> 
> nit: We drop this patch at the end. Can't we avoid it altogether instead?

I did it like this so that the last patch can be reverted for
testing/debugging, but also because I think it helps the structure of the
series.

> > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> > index a8184cad8890..bcb6ca2d9a7c 100644
> > --- a/arch/arm64/kernel/signal.c
> > +++ b/arch/arm64/kernel/signal.c
> > @@ -911,6 +911,19 @@ static void do_signal(struct pt_regs *regs)
> >  	restore_saved_sigmask();
> >  }
> >  
> > +static bool cpu_affinity_invalid(struct pt_regs *regs)
> > +{
> > +	if (!compat_user_mode(regs))
> > +		return false;
> 
> Silly question. Is there an advantage of using compat_user_mode() vs
> is_compat_task()? I see the latter used in the file although struct pt_regs
> *regs is passed to the functions calling it.
> 
> Nothing's wrong with it, just curious.

Not sure about advantages, but is_compat_task() is available in core code,
whereas compat_user_mode() is specific to arm64. The former implicitly
operates on 'current' and just checks thread flag, whereas the latter
actually goes and looks at mode field of the spsr to see what we're
going to be returning into.

Will

Re: [PATCH v4 04/14] arm64: Kill 32-bit applications scheduled on 64-bit-only CPUs

[Qais Yousef]

On 12/01/20 16:56, Will Deacon wrote:
> On Fri, Nov 27, 2020 at 01:12:17PM +0000, Qais Yousef wrote:
> > On 11/24/20 15:50, Will Deacon wrote:
> > > Scheduling a 32-bit application on a 64-bit-only CPU is a bad idea.
> > > 
> > > Ensure that 32-bit applications always take the slow-path when returning
> > > to userspace on a system with mismatched support at EL0, so that we can
> > > avoid trying to run on a 64-bit-only CPU and force a SIGKILL instead.
> > > 
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > > ---
> > 
> > nit: We drop this patch at the end. Can't we avoid it altogether instead?
> 
> I did it like this so that the last patch can be reverted for
> testing/debugging, but also because I think it helps the structure of the
> series.

Cool. I had a comment about the barrier(), you were worried about
cpu_affinity_invalid() being inlined by the compiler and then things get
mangled such that TIF_NOTIFY_RESUME clearing is moved after the call as you
described? Can the compiler move things if cpu_affinity_invalid() is a proper
function call (not inlined)?

> 
> > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> > > index a8184cad8890..bcb6ca2d9a7c 100644
> > > --- a/arch/arm64/kernel/signal.c
> > > +++ b/arch/arm64/kernel/signal.c
> > > @@ -911,6 +911,19 @@ static void do_signal(struct pt_regs *regs)
> > >  	restore_saved_sigmask();
> > >  }
> > >  
> > > +static bool cpu_affinity_invalid(struct pt_regs *regs)
> > > +{
> > > +	if (!compat_user_mode(regs))
> > > +		return false;
> > 
> > Silly question. Is there an advantage of using compat_user_mode() vs
> > is_compat_task()? I see the latter used in the file although struct pt_regs
> > *regs is passed to the functions calling it.
> > 
> > Nothing's wrong with it, just curious.
> 
> Not sure about advantages, but is_compat_task() is available in core code,
> whereas compat_user_mode() is specific to arm64. The former implicitly
> operates on 'current' and just checks thread flag, whereas the latter
> actually goes and looks at mode field of the spsr to see what we're
> going to be returning into.

Okay, so just 2 different ways to do the same thing and you happened to pick
the one that first came to mind.

Thanks

--
Qais Yousef

Re: [PATCH v4 04/14] arm64: Kill 32-bit applications scheduled on 64-bit-only CPUs

[Will Deacon]

On Wed, Dec 02, 2020 at 01:52:16PM +0000, Qais Yousef wrote:
> On 12/01/20 16:56, Will Deacon wrote:
> > On Fri, Nov 27, 2020 at 01:12:17PM +0000, Qais Yousef wrote:
> > > On 11/24/20 15:50, Will Deacon wrote:
> > > > Scheduling a 32-bit application on a 64-bit-only CPU is a bad idea.
> > > > 
> > > > Ensure that 32-bit applications always take the slow-path when returning
> > > > to userspace on a system with mismatched support at EL0, so that we can
> > > > avoid trying to run on a 64-bit-only CPU and force a SIGKILL instead.
> > > > 
> > > > Signed-off-by: Will Deacon <will@kernel.org>
> > > > ---
> > > 
> > > nit: We drop this patch at the end. Can't we avoid it altogether instead?
> > 
> > I did it like this so that the last patch can be reverted for
> > testing/debugging, but also because I think it helps the structure of the
> > series.
> 
> Cool. I had a comment about the barrier(), you were worried about
> cpu_affinity_invalid() being inlined by the compiler and then things get
> mangled such that TIF_NOTIFY_RESUME clearing is moved after the call as you
> described? Can the compiler move things if cpu_affinity_invalid() is a proper
> function call (not inlined)?

I think function calls implicitly clobber memory, but you'd have to annotate
the thing as noinline to prevent it being inlined.

Will

[PATCH v4 05/14] arm64: Advertise CPUs capable of running 32-bit applications in sysfs

[Will Deacon]

Since 32-bit applications will be killed if they are caught trying to
execute on a 64-bit-only CPU in a mismatched system, advertise the set
of 32-bit capable CPUs to userspace in sysfs.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
 .../ABI/testing/sysfs-devices-system-cpu      |  9 +++++++++
 arch/arm64/kernel/cpufeature.c                | 19 +++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
index 1a04ca8162ad..8a2e377b0dde 100644
--- a/Documentation/ABI/testing/sysfs-devices-system-cpu
+++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
@@ -493,6 +493,15 @@ Description:	AArch64 CPU registers
 		'identification' directory exposes the CPU ID registers for
 		identifying model and revision of the CPU.
 
+What:		/sys/devices/system/cpu/aarch32_el0
+Date:		November 2020
+Contact:	Linux ARM Kernel Mailing list <linux-arm-kernel@lists.infradead.org>
+Description:	Identifies the subset of CPUs in the system that can execute
+		AArch32 (32-bit ARM) applications. If present, the same format as
+		/sys/devices/system/cpu/{offline,online,possible,present} is used.
+		If absent, then all or none of the CPUs can execute AArch32
+		applications and execve() will behave accordingly.
+
 What:		/sys/devices/system/cpu/cpu#/cpu_capacity
 Date:		December 2016
 Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index bc71a51f1b9c..534f80edb594 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -67,6 +67,7 @@
 #include <linux/crash_dump.h>
 #include <linux/sort.h>
 #include <linux/stop_machine.h>
+#include <linux/sysfs.h>
 #include <linux/types.h>
 #include <linux/mm.h>
 #include <linux/cpu.h>
@@ -1272,6 +1273,24 @@ const struct cpumask *system_32bit_el0_cpumask(void)
 	return cpu_possible_mask;
 }
 
+static ssize_t aarch32_el0_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	const struct cpumask *mask = system_32bit_el0_cpumask();
+
+	return sysfs_emit(buf, "%*pbl\n", cpumask_pr_args(mask));
+}
+static const DEVICE_ATTR_RO(aarch32_el0);
+
+static int __init aarch32_el0_sysfs_init(void)
+{
+	if (!allow_mismatched_32bit_el0)
+		return 0;
+
+	return device_create_file(cpu_subsys.dev_root, &dev_attr_aarch32_el0);
+}
+device_initcall(aarch32_el0_sysfs_init);
+
 static bool has_32bit_el0(const struct arm64_cpu_capabilities *entry, int scope)
 {
 	if (!has_cpuid_feature(entry, scope))
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v4 06/14] arm64: Hook up cmdline parameter to allow mismatched 32-bit EL0

[Will Deacon]

Allow systems with mismatched 32-bit support at EL0 to run 32-bit
applications based on a new kernel parameter.

Signed-off-by: Will Deacon <will@kernel.org>
---
 Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
 arch/arm64/kernel/cpufeature.c                  | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 526d65d8573a..f20188c44d83 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -289,6 +289,13 @@
 			do not want to use tracing_snapshot_alloc() as it needs
 			to be done where GFP_KERNEL allocations are allowed.
 
+	allow_mismatched_32bit_el0 [ARM64]
+			Allow execve() of 32-bit applications and setting of the
+			PER_LINUX32 personality on systems where only a strict
+			subset of the CPUs support 32-bit EL0. When this
+			parameter is present, the set of CPUs supporting 32-bit
+			EL0 is indicated by /sys/devices/system/cpu/aarch32_el0.
+
 	amd_iommu=	[HW,X86-64]
 			Pass parameters to the AMD IOMMU driver in the system.
 			Possible values are:
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 534f80edb594..29017cbb6c8e 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1273,6 +1273,13 @@ const struct cpumask *system_32bit_el0_cpumask(void)
 	return cpu_possible_mask;
 }
 
+static int __init parse_32bit_el0_param(char *str)
+{
+	allow_mismatched_32bit_el0 = true;
+	return 0;
+}
+early_param("allow_mismatched_32bit_el0", parse_32bit_el0_param);
+
 static ssize_t aarch32_el0_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v4 06/14] arm64: Hook up cmdline parameter to allow mismatched 32-bit EL0

[Qais Yousef]

On 11/24/20 15:50, Will Deacon wrote:
> Allow systems with mismatched 32-bit support at EL0 to run 32-bit
> applications based on a new kernel parameter.
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
>  arch/arm64/kernel/cpufeature.c                  | 7 +++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 526d65d8573a..f20188c44d83 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -289,6 +289,13 @@
>  			do not want to use tracing_snapshot_alloc() as it needs
>  			to be done where GFP_KERNEL allocations are allowed.
>  
> +	allow_mismatched_32bit_el0 [ARM64]
> +			Allow execve() of 32-bit applications and setting of the
> +			PER_LINUX32 personality on systems where only a strict
> +			subset of the CPUs support 32-bit EL0. When this
> +			parameter is present, the set of CPUs supporting 32-bit
> +			EL0 is indicated by /sys/devices/system/cpu/aarch32_el0.

Shouldn't we document that a randomly selected 32-bit CPU will be prevented
from being hotplugged out all the time to act as the last man standing for any
currently running 32-bit application.

That was a mouthful! I'm sure you can phrase it better :-)

If we make this the last patch as it was before adding affinity handling, we
can drop patch 4 more easily I think?

Thanks

--
Qais Yousef

Re: [PATCH v4 06/14] arm64: Hook up cmdline parameter to allow mismatched 32-bit EL0

[Will Deacon]

On Fri, Nov 27, 2020 at 01:17:59PM +0000, Qais Yousef wrote:
> On 11/24/20 15:50, Will Deacon wrote:
> > Allow systems with mismatched 32-bit support at EL0 to run 32-bit
> > applications based on a new kernel parameter.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
> >  arch/arm64/kernel/cpufeature.c                  | 7 +++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 526d65d8573a..f20188c44d83 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -289,6 +289,13 @@
> >  			do not want to use tracing_snapshot_alloc() as it needs
> >  			to be done where GFP_KERNEL allocations are allowed.
> >  
> > +	allow_mismatched_32bit_el0 [ARM64]
> > +			Allow execve() of 32-bit applications and setting of the
> > +			PER_LINUX32 personality on systems where only a strict
> > +			subset of the CPUs support 32-bit EL0. When this
> > +			parameter is present, the set of CPUs supporting 32-bit
> > +			EL0 is indicated by /sys/devices/system/cpu/aarch32_el0.
> 
> Shouldn't we document that a randomly selected 32-bit CPU will be prevented
> from being hotplugged out all the time to act as the last man standing for any
> currently running 32-bit application.
> 
> That was a mouthful! I'm sure you can phrase it better :-)

Sure, I'll have a go at adding something. Thanks.

> If we make this the last patch as it was before adding affinity handling, we
> can drop patch 4 more easily I think?

I'd still prefer to keep patch 4, but I can certainly try to move this one
later in the series.

Will

[PATCH v4 07/14] sched: Introduce restrict_cpus_allowed_ptr() to limit task CPU affinity

[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.

Although userspace can carefully manage the affinity masks for such
tasks, one place where it is particularly problematic is execve()
because the CPU on which the execve() is occurring may be incompatible
with the new application image. In such a situation, it is desirable to
restrict the affinity mask of the task and ensure that the new image is
entered on a compatible CPU. From userspace's point of view, this looks
the same as if the incompatible CPUs have been hotplugged off in its
affinity mask.

In preparation for restricting the affinity mask for compat tasks on
arm64 systems without uniform support for 32-bit applications, introduce
a restrict_cpus_allowed_ptr(), which allows the current affinity mask
for a task to be shrunk to the intersection of a parameter mask.

Signed-off-by: Will Deacon <will@kernel.org>
---
 include/linux/sched.h |  1 +
 kernel/sched/core.c   | 73 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 59 insertions(+), 15 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 063cd120b459..1cd12c3ce9ee 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1631,6 +1631,7 @@ extern int task_can_attach(struct task_struct *p, const struct cpumask *cs_cpus_
 #ifdef CONFIG_SMP
 extern void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask);
 extern int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask);
+extern int restrict_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *mask);
 #else
 static inline void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
 {
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d2003a7d5ab5..818c8f7bdf2a 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1860,24 +1860,18 @@ void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
 }
 
 /*
- * Change a given task's CPU affinity. Migrate the thread to a
- * proper CPU and schedule it away if the CPU it's executing on
- * is removed from the allowed bitmask.
- *
- * NOTE: the caller must have a valid reference to the task, the
- * task must not exit() & deallocate itself prematurely. The
- * call is not atomic; no spinlocks may be held.
+ * Called with both p->pi_lock and rq->lock held; drops both before returning.
  */
-static int __set_cpus_allowed_ptr(struct task_struct *p,
-				  const struct cpumask *new_mask, bool check)
+static int __set_cpus_allowed_ptr_locked(struct task_struct *p,
+					 const struct cpumask *new_mask,
+					 bool check,
+					 struct rq *rq,
+					 struct rq_flags *rf)
 {
 	const struct cpumask *cpu_valid_mask = cpu_active_mask;
 	unsigned int dest_cpu;
-	struct rq_flags rf;
-	struct rq *rq;
 	int ret = 0;
 
-	rq = task_rq_lock(p, &rf);
 	update_rq_clock(rq);
 
 	if (p->flags & PF_KTHREAD) {
@@ -1929,7 +1923,7 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
 	if (task_running(rq, p) || p->state == TASK_WAKING) {
 		struct migration_arg arg = { p, dest_cpu };
 		/* Need help from migration thread: drop lock and wait. */
-		task_rq_unlock(rq, p, &rf);
+		task_rq_unlock(rq, p, rf);
 		stop_one_cpu(cpu_of(rq), migration_cpu_stop, &arg);
 		return 0;
 	} else if (task_on_rq_queued(p)) {
@@ -1937,20 +1931,69 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
 		 * OK, since we're going to drop the lock immediately
 		 * afterwards anyway.
 		 */
-		rq = move_queued_task(rq, &rf, p, dest_cpu);
+		rq = move_queued_task(rq, rf, p, dest_cpu);
 	}
 out:
-	task_rq_unlock(rq, p, &rf);
+	task_rq_unlock(rq, p, rf);
 
 	return ret;
 }
 
+/*
+ * Change a given task's CPU affinity. Migrate the thread to a
+ * proper CPU and schedule it away if the CPU it's executing on
+ * is removed from the allowed bitmask.
+ *
+ * NOTE: the caller must have a valid reference to the task, the
+ * task must not exit() & deallocate itself prematurely. The
+ * call is not atomic; no spinlocks may be held.
+ */
+static int __set_cpus_allowed_ptr(struct task_struct *p,
+				  const struct cpumask *new_mask, bool check)
+{
+	struct rq_flags rf;
+	struct rq *rq;
+
+	rq = task_rq_lock(p, &rf);
+	return __set_cpus_allowed_ptr_locked(p, new_mask, check, rq, &rf);
+}
+
 int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
 {
 	return __set_cpus_allowed_ptr(p, new_mask, false);
 }
 EXPORT_SYMBOL_GPL(set_cpus_allowed_ptr);
 
+/*
+ * Change a given task's CPU affinity to the intersection of its current
+ * affinity mask and @subset_mask. If the resulting mask is empty, leave
+ * the affinity unchanged and return -EINVAL.
+ */
+int restrict_cpus_allowed_ptr(struct task_struct *p,
+			      const struct cpumask *subset_mask)
+{
+	struct rq_flags rf;
+	struct rq *rq;
+	cpumask_var_t new_mask;
+	int retval;
+
+	if (!alloc_cpumask_var(&new_mask, GFP_KERNEL))
+		return -ENOMEM;
+
+	rq = task_rq_lock(p, &rf);
+	if (!cpumask_and(new_mask, &p->cpus_mask, subset_mask)) {
+		task_rq_unlock(rq, p, &rf);
+		retval = -EINVAL;
+		goto out_free_new_mask;
+	}
+
+	retval = __set_cpus_allowed_ptr_locked(p, new_mask, false, rq, &rf);
+
+out_free_new_mask:
+	free_cpumask_var(new_mask);
+	return retval;
+}
+
 void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
 {
 #ifdef CONFIG_SCHED_DEBUG
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v4 07/14] sched: Introduce restrict_cpus_allowed_ptr() to limit task CPU affinity

[Quentin Perret]

On Tuesday 24 Nov 2020 at 15:50:32 (+0000), Will Deacon wrote:
> 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.
> 
> Although userspace can carefully manage the affinity masks for such
> tasks, one place where it is particularly problematic is execve()
> because the CPU on which the execve() is occurring may be incompatible
> with the new application image. In such a situation, it is desirable to
> restrict the affinity mask of the task and ensure that the new image is
> entered on a compatible CPU. From userspace's point of view, this looks
> the same as if the incompatible CPUs have been hotplugged off in its
> affinity mask.
> 
> In preparation for restricting the affinity mask for compat tasks on
> arm64 systems without uniform support for 32-bit applications, introduce
> a restrict_cpus_allowed_ptr(), which allows the current affinity mask
> for a task to be shrunk to the intersection of a parameter mask.
> 
> Signed-off-by: Will Deacon <will@kernel.org>

Reviewed-by: Quentin Perret <qperret@google.com>

Thanks,
Quentin

Re: [PATCH v4 07/14] sched: Introduce restrict_cpus_allowed_ptr() to limit task CPU affinity

[Qais Yousef]

On 11/24/20 15:50, Will Deacon wrote:

[...]

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index d2003a7d5ab5..818c8f7bdf2a 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1860,24 +1860,18 @@ void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
>  }
>  
>  /*
> - * Change a given task's CPU affinity. Migrate the thread to a
> - * proper CPU and schedule it away if the CPU it's executing on
> - * is removed from the allowed bitmask.
> - *
> - * NOTE: the caller must have a valid reference to the task, the
> - * task must not exit() & deallocate itself prematurely. The
> - * call is not atomic; no spinlocks may be held.
> + * Called with both p->pi_lock and rq->lock held; drops both before returning.

nit: wouldn't it be better for the caller to acquire and release the locks?
Not a big deal but it's always confusing when half of the work done outside the
function and the other half done inside.

Thanks

--
Qais Yousef

>   */
> -static int __set_cpus_allowed_ptr(struct task_struct *p,
> -				  const struct cpumask *new_mask, bool check)
> +static int __set_cpus_allowed_ptr_locked(struct task_struct *p,
> +					 const struct cpumask *new_mask,
> +					 bool check,
> +					 struct rq *rq,
> +					 struct rq_flags *rf)
>  {
>  	const struct cpumask *cpu_valid_mask = cpu_active_mask;
>  	unsigned int dest_cpu;
> -	struct rq_flags rf;
> -	struct rq *rq;
>  	int ret = 0;
>  
> -	rq = task_rq_lock(p, &rf);
>  	update_rq_clock(rq);
>  
>  	if (p->flags & PF_KTHREAD) {
> @@ -1929,7 +1923,7 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
>  	if (task_running(rq, p) || p->state == TASK_WAKING) {
>  		struct migration_arg arg = { p, dest_cpu };
>  		/* Need help from migration thread: drop lock and wait. */
> -		task_rq_unlock(rq, p, &rf);
> +		task_rq_unlock(rq, p, rf);
>  		stop_one_cpu(cpu_of(rq), migration_cpu_stop, &arg);
>  		return 0;
>  	} else if (task_on_rq_queued(p)) {
> @@ -1937,20 +1931,69 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
>  		 * OK, since we're going to drop the lock immediately
>  		 * afterwards anyway.
>  		 */
> -		rq = move_queued_task(rq, &rf, p, dest_cpu);
> +		rq = move_queued_task(rq, rf, p, dest_cpu);
>  	}
>  out:
> -	task_rq_unlock(rq, p, &rf);
> +	task_rq_unlock(rq, p, rf);
>  
>  	return ret;
>  }

Re: [PATCH v4 07/14] sched: Introduce restrict_cpus_allowed_ptr() to limit task CPU affinity

[Will Deacon]

On Fri, Nov 27, 2020 at 01:19:16PM +0000, Qais Yousef wrote:
> On 11/24/20 15:50, Will Deacon wrote:
> 
> [...]
> 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index d2003a7d5ab5..818c8f7bdf2a 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -1860,24 +1860,18 @@ void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
> >  }
> >  
> >  /*
> > - * Change a given task's CPU affinity. Migrate the thread to a
> > - * proper CPU and schedule it away if the CPU it's executing on
> > - * is removed from the allowed bitmask.
> > - *
> > - * NOTE: the caller must have a valid reference to the task, the
> > - * task must not exit() & deallocate itself prematurely. The
> > - * call is not atomic; no spinlocks may be held.
> > + * Called with both p->pi_lock and rq->lock held; drops both before returning.
> 
> nit: wouldn't it be better for the caller to acquire and release the locks?
> Not a big deal but it's always confusing when half of the work done outside the
> function and the other half done inside.

That came up in the last version of the patches iirc, but the problem is
that __set_cpus_allowed_ptr_locked() can trigger migration, which can
drop the lock and take another one for the new runqueue.

Given that this function is internal to the scheduler, I think we can
probably live with it.

Will

Re: [PATCH v4 07/14] sched: Introduce restrict_cpus_allowed_ptr() to limit task CPU affinity

[Qais Yousef]

On 12/01/20 16:56, Will Deacon wrote:
> On Fri, Nov 27, 2020 at 01:19:16PM +0000, Qais Yousef wrote:
> > On 11/24/20 15:50, Will Deacon wrote:
> > 
> > [...]
> > 
> > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > index d2003a7d5ab5..818c8f7bdf2a 100644
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -1860,24 +1860,18 @@ void do_set_cpus_allowed(struct task_struct *p, const struct cpumask *new_mask)
> > >  }
> > >  
> > >  /*
> > > - * Change a given task's CPU affinity. Migrate the thread to a
> > > - * proper CPU and schedule it away if the CPU it's executing on
> > > - * is removed from the allowed bitmask.
> > > - *
> > > - * NOTE: the caller must have a valid reference to the task, the
> > > - * task must not exit() & deallocate itself prematurely. The
> > > - * call is not atomic; no spinlocks may be held.
> > > + * Called with both p->pi_lock and rq->lock held; drops both before returning.
> > 
> > nit: wouldn't it be better for the caller to acquire and release the locks?
> > Not a big deal but it's always confusing when half of the work done outside the
> > function and the other half done inside.
> 
> That came up in the last version of the patches iirc, but the problem is
> that __set_cpus_allowed_ptr_locked() can trigger migration, which can
> drop the lock and take another one for the new runqueue.
> 
> Given that this function is internal to the scheduler, I think we can
> probably live with it.

I guess task_rq_lock() always entails be prepared for surprises!

Works for me.

Thanks

--
Qais Yousef

[PATCH v4 08/14] arm64: exec: Adjust affinity for compat tasks with mismatched 32-bit EL0

[Will Deacon]

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.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/process.c | 42 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 1540ab0fbf23..72116b0c7c73 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -31,6 +31,7 @@
 #include <linux/interrupt.h>
 #include <linux/init.h>
 #include <linux/cpu.h>
+#include <linux/cpuset.h>
 #include <linux/elfcore.h>
 #include <linux/pm.h>
 #include <linux/tick.h>
@@ -625,6 +626,45 @@ unsigned long arch_align_stack(unsigned long sp)
 	return sp & ~0xf;
 }
 
+static void adjust_compat_task_affinity(struct task_struct *p)
+{
+	cpumask_var_t cpuset_mask;
+	const struct cpumask *possible_mask = system_32bit_el0_cpumask();
+	const struct cpumask *newmask = possible_mask;
+
+	/*
+	 * Restrict the CPU affinity mask for a 32-bit task so that it contains
+	 * only the 32-bit-capable subset of its original CPU mask. If this is
+	 * empty, then try again with the cpuset allowed mask. If that fails,
+	 * forcefully override it with the set of all 32-bit-capable CPUs that
+	 * we know about.
+	 *
+	 * From the perspective of the task, this looks similar to what would
+	 * happen if the 64-bit-only CPUs were hot-unplugged at the point of
+	 * execve().
+	 */
+	if (!restrict_cpus_allowed_ptr(p, possible_mask))
+		goto out;
+
+	if (alloc_cpumask_var(&cpuset_mask, GFP_KERNEL)) {
+		cpuset_cpus_allowed(p, cpuset_mask);
+		if (cpumask_and(cpuset_mask, cpuset_mask, possible_mask)) {
+			newmask = cpuset_mask;
+			goto out_set_mask;
+		}
+	}
+
+	if (printk_ratelimit()) {
+		printk_deferred("Overriding affinity for 32-bit process %d (%s) to CPUs %*pbl\n",
+				task_pid_nr(p), p->comm, cpumask_pr_args(newmask));
+	}
+out_set_mask:
+	set_cpus_allowed_ptr(p, newmask);
+	free_cpumask_var(cpuset_mask);
+out:
+	set_tsk_thread_flag(current, TIF_NOTIFY_RESUME);
+}
+
 /*
  * Called from setup_new_exec() after (COMPAT_)SET_PERSONALITY.
  */
@@ -635,7 +675,7 @@ void arch_setup_new_exec(void)
 	if (is_compat_task()) {
 		mmflags = MMCF_AARCH32;
 		if (static_branch_unlikely(&arm64_mismatched_32bit_el0))
-			set_tsk_thread_flag(current, TIF_NOTIFY_RESUME);
+			adjust_compat_task_affinity(current);
 	}
 
 	current->mm->context.flags = mmflags;
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v4 08/14] arm64: exec: Adjust affinity for compat tasks with mismatched 32-bit EL0

[Quentin Perret]

On Tuesday 24 Nov 2020 at 15:50:33 (+0000), Will Deacon wrote:
> 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.
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/process.c | 42 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 1540ab0fbf23..72116b0c7c73 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -31,6 +31,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/init.h>
>  #include <linux/cpu.h>
> +#include <linux/cpuset.h>
>  #include <linux/elfcore.h>
>  #include <linux/pm.h>
>  #include <linux/tick.h>
> @@ -625,6 +626,45 @@ unsigned long arch_align_stack(unsigned long sp)
>  	return sp & ~0xf;
>  }
>  
> +static void adjust_compat_task_affinity(struct task_struct *p)
> +{
> +	cpumask_var_t cpuset_mask;
> +	const struct cpumask *possible_mask = system_32bit_el0_cpumask();
> +	const struct cpumask *newmask = possible_mask;
> +
> +	/*
> +	 * Restrict the CPU affinity mask for a 32-bit task so that it contains
> +	 * only the 32-bit-capable subset of its original CPU mask. If this is
> +	 * empty, then try again with the cpuset allowed mask. If that fails,
> +	 * forcefully override it with the set of all 32-bit-capable CPUs that
> +	 * we know about.
> +	 *
> +	 * From the perspective of the task, this looks similar to what would
> +	 * happen if the 64-bit-only CPUs were hot-unplugged at the point of
> +	 * execve().
> +	 */
> +	if (!restrict_cpus_allowed_ptr(p, possible_mask))
> +		goto out;
> +
> +	if (alloc_cpumask_var(&cpuset_mask, GFP_KERNEL)) {
> +		cpuset_cpus_allowed(p, cpuset_mask);
> +		if (cpumask_and(cpuset_mask, cpuset_mask, possible_mask)) {
> +			newmask = cpuset_mask;
> +			goto out_set_mask;
> +		}
> +	}
> +
> +	if (printk_ratelimit()) {
> +		printk_deferred("Overriding affinity for 32-bit process %d (%s) to CPUs %*pbl\n",
> +				task_pid_nr(p), p->comm, cpumask_pr_args(newmask));
> +	}
> +out_set_mask:
> +	set_cpus_allowed_ptr(p, newmask);
> +	free_cpumask_var(cpuset_mask);
> +out:
> +	set_tsk_thread_flag(current, TIF_NOTIFY_RESUME);
> +}

This starts to look an awful lot like select_fallback_rq(), but I
suppose we shouldn't bother factoring out that code yet as we probably
don't want this pattern to be re-used all over, so:

Reviewed-by: Quentin Perret <qperret@google.com>

Thanks,
Quentin

Re: [PATCH v4 08/14] arm64: exec: Adjust affinity for compat tasks with mismatched 32-bit EL0

[Qais Yousef]

On 11/24/20 15:50, Will Deacon wrote:
> 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.
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/process.c | 42 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 1540ab0fbf23..72116b0c7c73 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -31,6 +31,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/init.h>
>  #include <linux/cpu.h>
> +#include <linux/cpuset.h>
>  #include <linux/elfcore.h>
>  #include <linux/pm.h>
>  #include <linux/tick.h>
> @@ -625,6 +626,45 @@ unsigned long arch_align_stack(unsigned long sp)
>  	return sp & ~0xf;
>  }
>  
> +static void adjust_compat_task_affinity(struct task_struct *p)
> +{
> +	cpumask_var_t cpuset_mask;
> +	const struct cpumask *possible_mask = system_32bit_el0_cpumask();
> +	const struct cpumask *newmask = possible_mask;
> +
> +	/*
> +	 * Restrict the CPU affinity mask for a 32-bit task so that it contains
> +	 * only the 32-bit-capable subset of its original CPU mask. If this is
> +	 * empty, then try again with the cpuset allowed mask. If that fails,
> +	 * forcefully override it with the set of all 32-bit-capable CPUs that
> +	 * we know about.
> +	 *
> +	 * From the perspective of the task, this looks similar to what would
> +	 * happen if the 64-bit-only CPUs were hot-unplugged at the point of
> +	 * execve().
> +	 */
> +	if (!restrict_cpus_allowed_ptr(p, possible_mask))
> +		goto out;
> +
> +	if (alloc_cpumask_var(&cpuset_mask, GFP_KERNEL)) {
> +		cpuset_cpus_allowed(p, cpuset_mask);
> +		if (cpumask_and(cpuset_mask, cpuset_mask, possible_mask)) {
> +			newmask = cpuset_mask;
> +			goto out_set_mask;
> +		}
> +	}

Wouldn't it be better to move this logic to restrict_cpus_allowed_ptr()?
I think it should always take cpusets into account and it's not special to
this particular handling here, no?

> +
> +	if (printk_ratelimit()) {
> +		printk_deferred("Overriding affinity for 32-bit process %d (%s) to CPUs %*pbl\n",
> +				task_pid_nr(p), p->comm, cpumask_pr_args(newmask));
> +	}

We have 2 cases where the affinity could have been overridden but we won't
print anything:

	1. restrict_cpus_allowed_ptr()
	2. intersection of cpuset_mask and possible mask drops some cpus.

Shouldn't we print something in these cases too?

IMO it would be better to move this print to restrict_cpus_allowed_ptr() too.

Thanks

--
Qais Yousef

> +out_set_mask:
> +	set_cpus_allowed_ptr(p, newmask);
> +	free_cpumask_var(cpuset_mask);
> +out:
> +	set_tsk_thread_flag(current, TIF_NOTIFY_RESUME);
> +}
> +
>  /*
>   * Called from setup_new_exec() after (COMPAT_)SET_PERSONALITY.
>   */
> @@ -635,7 +675,7 @@ void arch_setup_new_exec(void)
>  	if (is_compat_task()) {
>  		mmflags = MMCF_AARCH32;
>  		if (static_branch_unlikely(&arm64_mismatched_32bit_el0))
> -			set_tsk_thread_flag(current, TIF_NOTIFY_RESUME);
> +			adjust_compat_task_affinity(current);
>  	}
>  
>  	current->mm->context.flags = mmflags;
> -- 
> 2.29.2.454.gaff20da3a2-goog
> 

Re: [PATCH v4 08/14] arm64: exec: Adjust affinity for compat tasks with mismatched 32-bit EL0

[Will Deacon]

On Fri, Nov 27, 2020 at 01:23:06PM +0000, Qais Yousef wrote:
> On 11/24/20 15:50, Will Deacon wrote:
> > 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.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/kernel/process.c | 42 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index 1540ab0fbf23..72116b0c7c73 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -31,6 +31,7 @@
> >  #include <linux/interrupt.h>
> >  #include <linux/init.h>
> >  #include <linux/cpu.h>
> > +#include <linux/cpuset.h>
> >  #include <linux/elfcore.h>
> >  #include <linux/pm.h>
> >  #include <linux/tick.h>
> > @@ -625,6 +626,45 @@ unsigned long arch_align_stack(unsigned long sp)
> >  	return sp & ~0xf;
> >  }
> >  
> > +static void adjust_compat_task_affinity(struct task_struct *p)
> > +{
> > +	cpumask_var_t cpuset_mask;
> > +	const struct cpumask *possible_mask = system_32bit_el0_cpumask();
> > +	const struct cpumask *newmask = possible_mask;
> > +
> > +	/*
> > +	 * Restrict the CPU affinity mask for a 32-bit task so that it contains
> > +	 * only the 32-bit-capable subset of its original CPU mask. If this is
> > +	 * empty, then try again with the cpuset allowed mask. If that fails,
> > +	 * forcefully override it with the set of all 32-bit-capable CPUs that
> > +	 * we know about.
> > +	 *
> > +	 * From the perspective of the task, this looks similar to what would
> > +	 * happen if the 64-bit-only CPUs were hot-unplugged at the point of
> > +	 * execve().
> > +	 */
> > +	if (!restrict_cpus_allowed_ptr(p, possible_mask))
> > +		goto out;
> > +
> > +	if (alloc_cpumask_var(&cpuset_mask, GFP_KERNEL)) {
> > +		cpuset_cpus_allowed(p, cpuset_mask);
> > +		if (cpumask_and(cpuset_mask, cpuset_mask, possible_mask)) {
> > +			newmask = cpuset_mask;
> > +			goto out_set_mask;
> > +		}
> > +	}
> 
> Wouldn't it be better to move this logic to restrict_cpus_allowed_ptr()?
> I think it should always take cpusets into account and it's not special to
> this particular handling here, no?

I did actually try this but didn't pursue it further because I was worried
that I was putting too much of the "can't run a 32-bit task on a 64-bit-only
CPU" logic into what would otherwise be a potentially useful library function
if/when other architectures want something similar. But I'll have another
look because there were a couple of ideas I didn't try out.

> > +	if (printk_ratelimit()) {
> > +		printk_deferred("Overriding affinity for 32-bit process %d (%s) to CPUs %*pbl\n",
> > +				task_pid_nr(p), p->comm, cpumask_pr_args(newmask));
> > +	}
> 
> We have 2 cases where the affinity could have been overridden but we won't
> print anything:
> 
> 	1. restrict_cpus_allowed_ptr()
> 	2. intersection of cpuset_mask and possible mask drops some cpus.
> 
> Shouldn't we print something in these cases too?

I don't think so: in these cases we've found a subset of CPUs that we can
run on, and so there's no need to warn. Nothing says we _have_ to use all
the CPUs available to us. The case where we override the affinity mask
altogether, however, does warrant a warning. This is very similar to the
hotplug behaviour in select_fallback_rq().

Will

Re: [PATCH v4 08/14] arm64: exec: Adjust affinity for compat tasks with mismatched 32-bit EL0

[Qais Yousef]

On 12/01/20 16:55, Will Deacon wrote:
> > > +static void adjust_compat_task_affinity(struct task_struct *p)
> > > +{
> > > +	cpumask_var_t cpuset_mask;
> > > +	const struct cpumask *possible_mask = system_32bit_el0_cpumask();
> > > +	const struct cpumask *newmask = possible_mask;
> > > +
> > > +	/*
> > > +	 * Restrict the CPU affinity mask for a 32-bit task so that it contains
> > > +	 * only the 32-bit-capable subset of its original CPU mask. If this is
> > > +	 * empty, then try again with the cpuset allowed mask. If that fails,
> > > +	 * forcefully override it with the set of all 32-bit-capable CPUs that
> > > +	 * we know about.
> > > +	 *
> > > +	 * From the perspective of the task, this looks similar to what would
> > > +	 * happen if the 64-bit-only CPUs were hot-unplugged at the point of
> > > +	 * execve().
> > > +	 */
> > > +	if (!restrict_cpus_allowed_ptr(p, possible_mask))
> > > +		goto out;
> > > +
> > > +	if (alloc_cpumask_var(&cpuset_mask, GFP_KERNEL)) {
> > > +		cpuset_cpus_allowed(p, cpuset_mask);
> > > +		if (cpumask_and(cpuset_mask, cpuset_mask, possible_mask)) {
> > > +			newmask = cpuset_mask;
> > > +			goto out_set_mask;
> > > +		}
> > > +	}
> > 
> > Wouldn't it be better to move this logic to restrict_cpus_allowed_ptr()?
> > I think it should always take cpusets into account and it's not special to
> > this particular handling here, no?
> 
> I did actually try this but didn't pursue it further because I was worried
> that I was putting too much of the "can't run a 32-bit task on a 64-bit-only
> CPU" logic into what would otherwise be a potentially useful library function
> if/when other architectures want something similar. But I'll have another
> look because there were a couple of ideas I didn't try out.

If we improve the cpuset handling issues to take into account
arch_task_cpu_possible_mask() as discussed in the other thread, I think we can
drop the cpuset handling here.

> 
> > > +	if (printk_ratelimit()) {
> > > +		printk_deferred("Overriding affinity for 32-bit process %d (%s) to CPUs %*pbl\n",
> > > +				task_pid_nr(p), p->comm, cpumask_pr_args(newmask));
> > > +	}
> > 
> > We have 2 cases where the affinity could have been overridden but we won't
> > print anything:
> > 
> > 	1. restrict_cpus_allowed_ptr()
> > 	2. intersection of cpuset_mask and possible mask drops some cpus.
> > 
> > Shouldn't we print something in these cases too?
> 
> I don't think so: in these cases we've found a subset of CPUs that we can
> run on, and so there's no need to warn. Nothing says we _have_ to use all
> the CPUs available to us. The case where we override the affinity mask
> altogether, however, does warrant a warning. This is very similar to the
> hotplug behaviour in select_fallback_rq().

Okay. It is just to warn when we actually break the affinity because we ended
up with empty mask; not just because we changed the affinity to an intersecting
one.

I think this makes sense, yes. We might be able to drop this too if we improve
cpuset handling. The devil is in the details I guess.

Thanks

--
Qais Yousef

[PATCH v4 09/14] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1

[Will Deacon]

If the scheduler cannot find an allowed CPU for a task,
cpuset_cpus_allowed_fallback() will widen the affinity to cpu_possible_mask
if cgroup v1 is in use.

In preparation for allowing architectures to provide their own fallback
mask, just return early if we're not using cgroup v2 and allow
select_fallback_rq() to figure out the mask by itself.

Cc: Li Zefan <lizefan@huawei.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Reviewed-by: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/cgroup/cpuset.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index 57b5b5d0a5fd..e970737c3ed2 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -3299,9 +3299,11 @@ void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask)
 
 void cpuset_cpus_allowed_fallback(struct task_struct *tsk)
 {
+	if (!is_in_v2_mode())
+		return; /* select_fallback_rq will try harder */
+
 	rcu_read_lock();
-	do_set_cpus_allowed(tsk, is_in_v2_mode() ?
-		task_cs(tsk)->cpus_allowed : cpu_possible_mask);
+	do_set_cpus_allowed(tsk, task_cs(tsk)->cpus_allowed);
 	rcu_read_unlock();
 
 	/*
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v4 09/14] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1

[Qais Yousef]

On 11/24/20 15:50, Will Deacon wrote:
> If the scheduler cannot find an allowed CPU for a task,
> cpuset_cpus_allowed_fallback() will widen the affinity to cpu_possible_mask
> if cgroup v1 is in use.
> 
> In preparation for allowing architectures to provide their own fallback
> mask, just return early if we're not using cgroup v2 and allow
> select_fallback_rq() to figure out the mask by itself.

What about cpuset_attach()? When a task attaches to a new group its affinity
could be reset to possible_cpu_mask if it moves to top_cpuset or the
intersection of effective_cpus & cpu_online_mask.

Probably handled with later patches.

/me continue to look at the rest

Okay so in patch 11 we make set_cpus_allowed_ptr() fail. cpuset_attach will
just do WARN_ON_ONCE() and carry on.

I think we can fix that by making sure cpuset_can_attach() will fail. Which can
be done by fixing task_can_attach() to take into account the new arch
task_cpu_possible_mask()?

Thanks

--
Qais Yousef

> 
> Cc: Li Zefan <lizefan@huawei.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Reviewed-by: Quentin Perret <qperret@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  kernel/cgroup/cpuset.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 57b5b5d0a5fd..e970737c3ed2 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -3299,9 +3299,11 @@ void cpuset_cpus_allowed(struct task_struct *tsk, struct cpumask *pmask)
>  
>  void cpuset_cpus_allowed_fallback(struct task_struct *tsk)
>  {
> +	if (!is_in_v2_mode())
> +		return; /* select_fallback_rq will try harder */
> +
>  	rcu_read_lock();
> -	do_set_cpus_allowed(tsk, is_in_v2_mode() ?
> -		task_cs(tsk)->cpus_allowed : cpu_possible_mask);
> +	do_set_cpus_allowed(tsk, task_cs(tsk)->cpus_allowed);
>  	rcu_read_unlock();
>  
>  	/*
> -- 
> 2.29.2.454.gaff20da3a2-goog
> 

Re: [PATCH v4 09/14] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1

[Qais Yousef]

On 11/27/20 13:32, Qais Yousef wrote:
> On 11/24/20 15:50, Will Deacon wrote:
> > If the scheduler cannot find an allowed CPU for a task,
> > cpuset_cpus_allowed_fallback() will widen the affinity to cpu_possible_mask
> > if cgroup v1 is in use.
> > 
> > In preparation for allowing architectures to provide their own fallback
> > mask, just return early if we're not using cgroup v2 and allow
> > select_fallback_rq() to figure out the mask by itself.
> 
> What about cpuset_attach()? When a task attaches to a new group its affinity
> could be reset to possible_cpu_mask if it moves to top_cpuset or the
> intersection of effective_cpus & cpu_online_mask.
> 
> Probably handled with later patches.
> 
> /me continue to look at the rest
> 
> Okay so in patch 11 we make set_cpus_allowed_ptr() fail. cpuset_attach will
> just do WARN_ON_ONCE() and carry on.
> 
> I think we can fix that by making sure cpuset_can_attach() will fail. Which can
> be done by fixing task_can_attach() to take into account the new arch
> task_cpu_possible_mask()?

I ran some experiments and indeed we have some problems here :(

I create 3 cpusets: 64bit, 32bit and mix. As the name indicates, 64bit contains
all 64bit-only cpus, 32bit contains 32bit-capable ones and mix has a mixture of
both.

If I try to move my test binary to 64bit cpuset, it moves there and I see the
WARN_ON_ONCE() triggered. The task has attached to the new cpuset but
set_allowed_cpus_ptr() has failed and we end up with whatever affinity we had
previously. Breaking cpusets effectively.

This can be easily fixable with my suggestion to educate task_can_attach() to
take into account the new arch_task_cpu_cpu_possible_mask(). The attachment
will fail and userspace will get an error then.

But this introduces another issue..

If I use cpumask_subset() in task_can_attach(), then an attempt to move to
'mix' cpuset will fail. In Android foreground cpuset usually contains a mixture
of cpus. So we want this to work.

Also 'background' tasks are usually bound to littles. If the littles are 64bit
only, then user space must ensure to add a 32bit capable cpu to the list. If we
can't attach to mixed cpuset, then this workaround won't work and Android will
have to create different cpusets for 32bit and 64bit apps. Which would be
difficult since 64bit apps do embed 32bit binaries and it'd be hard for
userspace to deal with that. Unless we extend execve syscall to take a cgroup
as an arg to attach to (like clone3 syscall), then potentially we can do
something about it, I think, by 'fixing' the libc wrapper.

If I use cpumask_intersects() to validate the attachment, then the 64bit
attachment will fail but the 'mix' one will succeed, as desired. BUT we'll
re-introduce the WARN_ON_ONCE() again since __set_cpus_allowed_ptr_locked()
validates against arch_task_cpu_possible_mask() with cpumask_subset() :-/
ie: cpuset_attach()::set_cpus_allowed_ptr() will fail with just a warning
printed once when attaching to 'mix'.

We can fix this by making __set_cpus_allowed_ptr_locked() perform cpumask_and()
like restrict_cpus_allowed_ptr() does.

Though this will not only truncate cpuset mask with the intersection of
arch_task_cpu_possible_mask(), but also truncate it for sched_setaffinity()
syscall. Something to keep in mind.

Finally there's the ugly problem that I'm not sure how to address of modifying
the cpuset.cpus of, for example 'mix' cpuset, such that we end up with 64bit
only cpus. If the 32bit task has already attached, then we'll end up with
a broken cpuset with a task attached having 'invalid' cpumask.

We can teach cpuset.c:update_cpumask()::validate_change() to do something about
it. By either walking the attached tasks and validating the new mask against
the arch one. Or by storing the arch mask in struct cpuset. Though for the
latter if we want to be truly generic, we need to ensure we handle the case of
2 tasks having different arch_task_cpu_possible_mask().

Long email. Hopefully it makes sense!

I can share my test script, binary/code and fixup patches if you like.

Thanks

--
Qais Yousef

Re: [PATCH v4 09/14] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1

[Quentin Perret]

On Monday 30 Nov 2020 at 17:05:31 (+0000), Qais Yousef wrote:
> I create 3 cpusets: 64bit, 32bit and mix. As the name indicates, 64bit contains
> all 64bit-only cpus, 32bit contains 32bit-capable ones and mix has a mixture of
> both.
> 
> If I try to move my test binary to 64bit cpuset, it moves there and I see the
> WARN_ON_ONCE() triggered. The task has attached to the new cpuset but
> set_allowed_cpus_ptr() has failed and we end up with whatever affinity we had
> previously. Breaking cpusets effectively.

Right, and so does exec'ing from a 64 bit task into 32 bit executable
from within a 64 bit-only cpuset :( . And there is nothing we can really
do about it, we cannot fail the exec because we need this to work for
existing apps, and there is no way the Android framework can know
upfront.

So the only thing we can do really is WARN() and proceed to ignore the
cpuset, which is what this series does :/. It's not exactly pretty but I
don't think we can do much better than that TBH, and it's the same thing
for the example you brought up. Failing cpuset_can_attach() will not
help, we can only WARN and proceed ...

Now, Android should be fine with that I think. We only need the kernel
to implement a safe fallback mechanism when userspace gives
contradictory commands, because we know there are edge cases userspace
_cannot_ deal with correctly, but this fallback doesn't need to be
highly optimized (at least for Android), but I'm happy to hear what
others think.

Thanks,
Quentin

Re: [PATCH v4 09/14] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1

[Qais Yousef]

On 11/30/20 17:36, Quentin Perret wrote:
> On Monday 30 Nov 2020 at 17:05:31 (+0000), Qais Yousef wrote:
> > I create 3 cpusets: 64bit, 32bit and mix. As the name indicates, 64bit contains
> > all 64bit-only cpus, 32bit contains 32bit-capable ones and mix has a mixture of
> > both.
> > 
> > If I try to move my test binary to 64bit cpuset, it moves there and I see the
> > WARN_ON_ONCE() triggered. The task has attached to the new cpuset but
> > set_allowed_cpus_ptr() has failed and we end up with whatever affinity we had
> > previously. Breaking cpusets effectively.
> 
> Right, and so does exec'ing from a 64 bit task into 32 bit executable
> from within a 64 bit-only cpuset :( . And there is nothing we can really

True. The kernel can decide to kill the task or force detach it then, no?
Sending SIGKILL makes more sense.

> do about it, we cannot fail the exec because we need this to work for
> existing apps, and there is no way the Android framework can know
> upfront.

It knows upfront it has enabled asym aarch32. So it needs to make sure not to
create 'invalid' cpusets?

> 
> So the only thing we can do really is WARN() and proceed to ignore the
> cpuset, which is what this series does :/. It's not exactly pretty but I
> don't think we can do much better than that TBH, and it's the same thing
> for the example you brought up. Failing cpuset_can_attach() will not
> help, we can only WARN and proceed ...

I think for cases where we can prevent userspace from doing something wrong, we
should. Like trying to attach to a cpuset that will result in an empty mask.
FWIW, it does something similar with deadline tasks. See task_can_attach().

Similarly for the case when userspace tries to modify the cpuset.cpus such that
a task will end up with empty cpumask. We now have the new case that some tasks
can only run on a subset of cpu_possible_mask. So the definition of empty
cpumask has gained an extra meaning.

> 
> Now, Android should be fine with that I think. We only need the kernel
> to implement a safe fallback mechanism when userspace gives
> contradictory commands, because we know there are edge cases userspace
> _cannot_ deal with correctly, but this fallback doesn't need to be
> highly optimized (at least for Android), but I'm happy to hear what
> others think.

Why not go with our original patch that fixes affinity then in the arch code if
the task wakes up on the wrong cpu? It is much simpler approach IMO to achieve
the same thing.

I was under the impression that if we go down teaching the scheduler about asym
ISA, then we have to deal with these edge cases. I don't think we're far away
from getting there.

Thanks

--
Qais Yousef

Re: [PATCH v4 09/14] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1

[Quentin Perret]

On Tuesday 01 Dec 2020 at 11:58:42 (+0000), Qais Yousef wrote:
> On 11/30/20 17:36, Quentin Perret wrote:
> > On Monday 30 Nov 2020 at 17:05:31 (+0000), Qais Yousef wrote:
> > > I create 3 cpusets: 64bit, 32bit and mix. As the name indicates, 64bit contains
> > > all 64bit-only cpus, 32bit contains 32bit-capable ones and mix has a mixture of
> > > both.
> > > 
> > > If I try to move my test binary to 64bit cpuset, it moves there and I see the
> > > WARN_ON_ONCE() triggered. The task has attached to the new cpuset but
> > > set_allowed_cpus_ptr() has failed and we end up with whatever affinity we had
> > > previously. Breaking cpusets effectively.
> > 
> > Right, and so does exec'ing from a 64 bit task into 32 bit executable
> > from within a 64 bit-only cpuset :( . And there is nothing we can really
> 
> True. The kernel can decide to kill the task or force detach it then, no?
> Sending SIGKILL makes more sense.

Yeah but again, we need this to work for existing apps. Just killing it
basically means we have no support, so that doesn't work for the use
case :/

> > do about it, we cannot fail the exec because we need this to work for
> > existing apps, and there is no way the Android framework can know
> > upfront.
> 
> It knows upfront it has enabled asym aarch32. So it needs to make sure not to
> create 'invalid' cpusets?

Problem is, we _really_ don't want to keep a big CPU in the background
cpuset just for that. And even if we did, we'd have to deal with hotplug.

> > 
> > So the only thing we can do really is WARN() and proceed to ignore the
> > cpuset, which is what this series does :/. It's not exactly pretty but I
> > don't think we can do much better than that TBH, and it's the same thing
> > for the example you brought up. Failing cpuset_can_attach() will not
> > help, we can only WARN and proceed ...
> 
> I think for cases where we can prevent userspace from doing something wrong, we
> should. Like trying to attach to a cpuset that will result in an empty mask.
> FWIW, it does something similar with deadline tasks. See task_can_attach().
> 
> Similarly for the case when userspace tries to modify the cpuset.cpus such that
> a task will end up with empty cpumask. We now have the new case that some tasks
> can only run on a subset of cpu_possible_mask. So the definition of empty
> cpumask has gained an extra meaning.

I see this differently, e.g. if you affine a task to a CPU and you
hotunplug it, then the kernel falls back to the remaining online CPUs
for that task. Not pretty, but it keeps things functional. I'm thinking
a similar kind of support would be good enough here.

But yes, this cpuset mess is the part of the series I hate most too.
It's just not clear we have better solutions :/

> > 
> > Now, Android should be fine with that I think. We only need the kernel
> > to implement a safe fallback mechanism when userspace gives
> > contradictory commands, because we know there are edge cases userspace
> > _cannot_ deal with correctly, but this fallback doesn't need to be
> > highly optimized (at least for Android), but I'm happy to hear what
> > others think.
> 
> Why not go with our original patch that fixes affinity then in the arch code if
> the task wakes up on the wrong cpu? It is much simpler approach IMO to achieve
> the same thing.

I personally had no issues with that patch, but as per Peter's original
reply, that's "not going to happen". Will's proposal seems to go one
step further and tries its best to honor the contract with userspace (by
keeping the subset of the affinity mask, ...) when that can be done, so
if that can be acceptable, then be it. But I'd still rather keep this
simple if at all possible. It's just my opinion though :)

Thanks,
Quentin

Re: [PATCH v4 09/14] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1

[Qais Yousef]

On 12/01/20 12:37, Quentin Perret wrote:
> On Tuesday 01 Dec 2020 at 11:58:42 (+0000), Qais Yousef wrote:
> > On 11/30/20 17:36, Quentin Perret wrote:
> > > On Monday 30 Nov 2020 at 17:05:31 (+0000), Qais Yousef wrote:
> > > > I create 3 cpusets: 64bit, 32bit and mix. As the name indicates, 64bit contains
> > > > all 64bit-only cpus, 32bit contains 32bit-capable ones and mix has a mixture of
> > > > both.
> > > > 
> > > > If I try to move my test binary to 64bit cpuset, it moves there and I see the
> > > > WARN_ON_ONCE() triggered. The task has attached to the new cpuset but
> > > > set_allowed_cpus_ptr() has failed and we end up with whatever affinity we had
> > > > previously. Breaking cpusets effectively.
> > > 
> > > Right, and so does exec'ing from a 64 bit task into 32 bit executable
> > > from within a 64 bit-only cpuset :( . And there is nothing we can really
> > 
> > True. The kernel can decide to kill the task or force detach it then, no?
> > Sending SIGKILL makes more sense.
> 
> Yeah but again, we need this to work for existing apps. Just killing it
> basically means we have no support, so that doesn't work for the use
> case :/

I don't get you. Unless what you're saying is you want to support this without
userspace having to do anything to opt-in, then this statement is hard to
digest for me. Existing apps will work. OEMs and vendors who want to enable
this feature must configure their userspace to deal with this correctly. This
includes passing the cmdline option, and parsing the cpumask in
/sys/device/system/cpu/aarch32_el0 to ensure their cpusets contains at least
one cpu from this list.

> 
> > > do about it, we cannot fail the exec because we need this to work for
> > > existing apps, and there is no way the Android framework can know
> > > upfront.
> > 
> > It knows upfront it has enabled asym aarch32. So it needs to make sure not to
> > create 'invalid' cpusets?
> 
> Problem is, we _really_ don't want to keep a big CPU in the background

I don't get you here too.

AFAIU, OEMs have to define their cpusets. So it makes sense to me for them to
define it correctly if they want to enable asym aarch32.

Systems that don't care about this feature shouldn't be affected. If they do,
then I'm missing something.

> cpuset just for that. And even if we did, we'd have to deal with hotplug.

We deal with hotplug by not allowing one of the aarch32 cpus from going
offline.

> 
> > > 
> > > So the only thing we can do really is WARN() and proceed to ignore the
> > > cpuset, which is what this series does :/. It's not exactly pretty but I
> > > don't think we can do much better than that TBH, and it's the same thing
> > > for the example you brought up. Failing cpuset_can_attach() will not
> > > help, we can only WARN and proceed ...
> > 
> > I think for cases where we can prevent userspace from doing something wrong, we
> > should. Like trying to attach to a cpuset that will result in an empty mask.
> > FWIW, it does something similar with deadline tasks. See task_can_attach().
> > 
> > Similarly for the case when userspace tries to modify the cpuset.cpus such that
> > a task will end up with empty cpumask. We now have the new case that some tasks
> > can only run on a subset of cpu_possible_mask. So the definition of empty
> > cpumask has gained an extra meaning.
> 
> I see this differently, e.g. if you affine a task to a CPU and you
> hotunplug it, then the kernel falls back to the remaining online CPUs
> for that task. Not pretty, but it keeps things functional. I'm thinking
> a similar kind of support would be good enough here.

Sorry I can't see it this way :(

The comparison is fundamentally different. Being able to attach to a cpuset, or
modify its cpus is different than short circuiting a cpu out of the system.

For hotplug we have to make sure a single cpu stays alive. The fallback you're
talking about should still work the same if the task is not attached to
a cpuset. Just it has to take the intersection with the
arch_task_cpu_possible_cpu() into account.

For cpusets, if hotunplug results in an empty cpuset, then all tasks are moved
to the nearest ancestor if I read the code correctly. In our case, only 32bit
tasks have to move out to retain this behavior. Since now for the first time we
have tasks that can't run on all cpus.

Which by the way might be the right behavior for 64bit tasks execing 32bit
binary in a 64bit only cpuset. I suggested SIGKILL'ing them but maybe moving
them to the nearest ancestor too is more aligned with the behavior above.

> 
> But yes, this cpuset mess is the part of the series I hate most too.
> It's just not clear we have better solutions :/
> 
> > > 
> > > Now, Android should be fine with that I think. We only need the kernel
> > > to implement a safe fallback mechanism when userspace gives
> > > contradictory commands, because we know there are edge cases userspace
> > > _cannot_ deal with correctly, but this fallback doesn't need to be
> > > highly optimized (at least for Android), but I'm happy to hear what
> > > others think.
> > 
> > Why not go with our original patch that fixes affinity then in the arch code if
> > the task wakes up on the wrong cpu? It is much simpler approach IMO to achieve
> > the same thing.
> 
> I personally had no issues with that patch, but as per Peter's original
> reply, that's "not going to happen". Will's proposal seems to go one
> step further and tries its best to honor the contract with userspace (by

The only difference I see is that this series blocks sched_setaffinity(). Other
than that we fix up the task affinity in a similar way, just in different
places.

If the intersection was empty, we sent a SIGKILL; making it fallback to
something sensible is dead easy. I opted for SIGKILL because I didn't expect
Peter to agree with the fallback. But if we're arguing it's okay now, then it
should be okay in that patch too then.

> keeping the subset of the affinity mask, ...) when that can be done, so
> if that can be acceptable, then be it. But I'd still rather keep this
> simple if at all possible. It's just my opinion though :)

The way I see it, this series adds support to handle generic asym ISA. Asym
AArch32 will be the first and only user for now, but it's paving the way for
others.

It's up to the maintainers to decide, but IMHO if we'll do this via enabling
scheduler to deal with generic asym ISA support, we should deal with these
corner cases. Otherwise a specific solution for the current problem at hand in
the arch code makes more sense to me.

It all depends how much the community wants/is willing to cater for these
systems in the future too.

Thanks

--
Qais Yousef

Re: [PATCH v4 09/14] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1

[Quentin Perret]

On Tuesday 01 Dec 2020 at 14:11:21 (+0000), Qais Yousef wrote:
> AFAIU, OEMs have to define their cpusets. So it makes sense to me for them to
> define it correctly if they want to enable asym aarch32.
> 
> Systems that don't care about this feature shouldn't be affected. If they do,
> then I'm missing something.

Right, but there are 2 cases for 32 bit tasks in Android:

  1. 32 bit apps; these are not an issue, the Android framework knows
     about them and it's fine to expect it to setup cpusets accordingly
     IMO.

  2. 64 bit apps that also happen to have a 32 bit binary payload, and
     exec into it. The Android framework has no visibility over that,
     all it sees is a 64 bit app. Sadly we can't detect this stupid
     pattern, but we need these to remain somewhat functional.

I was only talking about 2. the whole time, sorry if that wasn't clear.
With that said, see below for the discussion about cpuset/hotplug.

> We deal with hotplug by not allowing one of the aarch32 cpus from going
> offline.

Sure, but that would only work if we have that 32 bit CPU present in
_all_ cpusets, no? What I'd like to avoid is to keep a (big) 32
bit CPU in the background cpuset of 64 bit tasks. That would make that
big CPU available to _all_ 64 bit apps in the background, whether they
need 32 bit support or not, because again we cannot distinguish them.
And yeah, I expect this to be not go down well in practice.


So, if we're going to support this, a requirement for Android is that
some cpusets will be 64 bit only, and it's possible that we'll exec into
32 bit from within these cpusets. It's an edge case, we don't really
want to optimize for it, but it needs to not fall apart completely.
I'm not fundamentally against doing smarter things at all, I'm saying we
(Android) just don't _need_ smarter things ATM, so we may want to keep
it simple.

My point in the previous message is, if we're accepting this for exec,
a logical next step could be to accept it for cpuset migrations too.
Failing the cgroup migration is hard since: there is no guarantee the
source cpuset has 32 bit CPUs anyway (assuming the exec'd task is kept
in the same cpuset), so why bother; userspace just doesn't know there
are 32 bit tasks in an app and would keep trying to migrate it to 64 bit
cpuset over and over again; you could end up with apps being stuck
halfway through a top-app->background transition where some tasks have
migrated but not others, ...

It's a bit of a mess :/


<snip>
> For hotplug we have to make sure a single cpu stays alive. The fallback you're
> talking about should still work the same if the task is not attached to
> a cpuset. Just it has to take the intersection with the
> arch_task_cpu_possible_cpu() into account.

Yep, agreed, there's probably room for improvement there.

> For cpusets, if hotunplug results in an empty cpuset, then all tasks are moved
> to the nearest ancestor if I read the code correctly. In our case, only 32bit
> tasks have to move out to retain this behavior. Since now for the first time we
> have tasks that can't run on all cpus.
> 
> Which by the way might be the right behavior for 64bit tasks execing 32bit
> binary in a 64bit only cpuset. I suggested SIGKILL'ing them but maybe moving
> them to the nearest ancestor too is more aligned with the behavior above.

Hmm, I guess that means putting all 32-bit-execd-from-64-bit tasks in
the root group in Android. I'll try and check the implications, but that
might be just fine... Sounds like a sensible behaviour to me anyways.

Thanks,
Quentin

Re: [PATCH v4 09/14] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1

[Will Deacon]

On Tue, Dec 01, 2020 at 03:56:49PM +0000, Quentin Perret wrote:
> On Tuesday 01 Dec 2020 at 14:11:21 (+0000), Qais Yousef wrote:
> > For cpusets, if hotunplug results in an empty cpuset, then all tasks are moved
> > to the nearest ancestor if I read the code correctly. In our case, only 32bit
> > tasks have to move out to retain this behavior. Since now for the first time we
> > have tasks that can't run on all cpus.
> > 
> > Which by the way might be the right behavior for 64bit tasks execing 32bit
> > binary in a 64bit only cpuset. I suggested SIGKILL'ing them but maybe moving
> > them to the nearest ancestor too is more aligned with the behavior above.
> 
> Hmm, I guess that means putting all 32-bit-execd-from-64-bit tasks in
> the root group in Android. I'll try and check the implications, but that
> might be just fine... Sounds like a sensible behaviour to me anyways.

I'll look into this -- anything we can do to avoid forcefully resetting the
affinity mask to the arch_task_cpu_possible_mask() is worth considering.

Will

Re: [PATCH v4 09/14] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1

[Qais Yousef]

On 12/01/20 22:30, Will Deacon wrote:
> On Tue, Dec 01, 2020 at 03:56:49PM +0000, Quentin Perret wrote:
> > On Tuesday 01 Dec 2020 at 14:11:21 (+0000), Qais Yousef wrote:
> > > For cpusets, if hotunplug results in an empty cpuset, then all tasks are moved
> > > to the nearest ancestor if I read the code correctly. In our case, only 32bit
> > > tasks have to move out to retain this behavior. Since now for the first time we
> > > have tasks that can't run on all cpus.
> > > 
> > > Which by the way might be the right behavior for 64bit tasks execing 32bit
> > > binary in a 64bit only cpuset. I suggested SIGKILL'ing them but maybe moving
> > > them to the nearest ancestor too is more aligned with the behavior above.
> > 
> > Hmm, I guess that means putting all 32-bit-execd-from-64-bit tasks in
> > the root group in Android. I'll try and check the implications, but that
> > might be just fine... Sounds like a sensible behaviour to me anyways.
> 
> I'll look into this -- anything we can do to avoid forcefully resetting the
> affinity mask to the arch_task_cpu_possible_mask() is worth considering.

Happy to lend a hand, just let me know.

Thanks

--
Qais Yousef

Re: [PATCH v4 09/14] cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1

[Qais Yousef]

On 12/01/20 15:56, Quentin Perret wrote:
> On Tuesday 01 Dec 2020 at 14:11:21 (+0000), Qais Yousef wrote:
> > AFAIU, OEMs have to define their cpusets. So it makes sense to me for them to
> > define it correctly if they want to enable asym aarch32.
> > 
> > Systems that don't care about this feature shouldn't be affected. If they do,
> > then I'm missing something.
> 
> Right, but there are 2 cases for 32 bit tasks in Android:
> 
>   1. 32 bit apps; these are not an issue, the Android framework knows
>      about them and it's fine to expect it to setup cpusets accordingly
>      IMO.
> 
>   2. 64 bit apps that also happen to have a 32 bit binary payload, and
>      exec into it. The Android framework has no visibility over that,
>      all it sees is a 64 bit app. Sadly we can't detect this stupid
>      pattern, but we need these to remain somewhat functional.
> 
> I was only talking about 2. the whole time, sorry if that wasn't clear.
> With that said, see below for the discussion about cpuset/hotplug.

Yep, I was referring to 2 too. I found out about the app that embeds the 32 bit
binary, it was our major concern if we go with user space managing affinities.

> 
> > We deal with hotplug by not allowing one of the aarch32 cpus from going
> > offline.
> 
> Sure, but that would only work if we have that 32 bit CPU present in
> _all_ cpusets, no? What I'd like to avoid is to keep a (big) 32
> bit CPU in the background cpuset of 64 bit tasks. That would make that
> big CPU available to _all_ 64 bit apps in the background, whether they
> need 32 bit support or not, because again we cannot distinguish them.
> And yeah, I expect this to be not go down well in practice.
> 
> 
> So, if we're going to support this, a requirement for Android is that
> some cpusets will be 64 bit only, and it's possible that we'll exec into
> 32 bit from within these cpusets. It's an edge case, we don't really
> want to optimize for it, but it needs to not fall apart completely.
> I'm not fundamentally against doing smarter things at all, I'm saying we
> (Android) just don't _need_ smarter things ATM, so we may want to keep
> it simple.

Fair enough. But in that case I find it neater to fix the affinities up in the
arch code as a specific solution. I'm not seeing there's a difference in the
end results between the two implementations if we don't address these issues
:(

> 
> My point in the previous message is, if we're accepting this for exec,
> a logical next step could be to accept it for cpuset migrations too.
> Failing the cgroup migration is hard since: there is no guarantee the
> source cpuset has 32 bit CPUs anyway (assuming the exec'd task is kept
> in the same cpuset), so why bother; userspace just doesn't know there
> are 32 bit tasks in an app and would keep trying to migrate it to 64 bit
> cpuset over and over again; you could end up with apps being stuck
> halfway through a top-app->background transition where some tasks have
> migrated but not others, ...
> 
> It's a bit of a mess :/

It is. I think I addressed these concerns in other parts from my previous
email. Judging by your reply below I think you see what I was talking about and
we're more on the same page now :-)

> 
> 
> <snip>
> > For hotplug we have to make sure a single cpu stays alive. The fallback you're
> > talking about should still work the same if the task is not attached to
> > a cpuset. Just it has to take the intersection with the
> > arch_task_cpu_possible_cpu() into account.
> 
> Yep, agreed, there's probably room for improvement there.
> 
> > For cpusets, if hotunplug results in an empty cpuset, then all tasks are moved
> > to the nearest ancestor if I read the code correctly. In our case, only 32bit
> > tasks have to move out to retain this behavior. Since now for the first time we
> > have tasks that can't run on all cpus.
> > 
> > Which by the way might be the right behavior for 64bit tasks execing 32bit
> > binary in a 64bit only cpuset. I suggested SIGKILL'ing them but maybe moving
> > them to the nearest ancestor too is more aligned with the behavior above.
> 
> Hmm, I guess that means putting all 32-bit-execd-from-64-bit tasks in
> the root group in Android. I'll try and check the implications, but that
> might be just fine... Sounds like a sensible behaviour to me anyways.

It'd be only the compat tasks that will have to move to root group. And only
for those minority of apps that embed a 32bit binary. I think the impact is
minimum. And I think the behavior makes sense generically.

Thanks!

--
Qais Yousef

[PATCH v4 10/14] sched: Introduce arch_task_cpu_possible_mask() to limit fallback rq selection

[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.

Introduce an arch_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.

Reviewed-by: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/sched/core.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 818c8f7bdf2a..99992d0beb65 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1696,6 +1696,11 @@ void check_preempt_curr(struct rq *rq, struct task_struct *p, int flags)
 
 #ifdef CONFIG_SMP
 
+/* Must contain at least one active CPU */
+#ifndef arch_task_cpu_possible_mask
+#define  arch_task_cpu_possible_mask(p)	cpu_possible_mask
+#endif
+
 /*
  * Per-CPU kthreads are allowed to run on !active && online CPUs, see
  * __set_cpus_allowed_ptr() and select_fallback_rq().
@@ -1708,7 +1713,10 @@ static inline bool is_cpu_allowed(struct task_struct *p, int cpu)
 	if (is_per_cpu_kthread(p))
 		return cpu_online(cpu);
 
-	return cpu_active(cpu);
+	if (!cpu_active(cpu))
+		return false;
+
+	return cpumask_test_cpu(cpu, arch_task_cpu_possible_mask(p));
 }
 
 /*
@@ -2361,10 +2369,9 @@ static int select_fallback_rq(int cpu, struct task_struct *p)
 			}
 			fallthrough;
 		case possible:
-			do_set_cpus_allowed(p, cpu_possible_mask);
+			do_set_cpus_allowed(p, arch_task_cpu_possible_mask(p));
 			state = fail;
 			break;
-
 		case fail:
 			BUG();
 			break;
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v4 11/14] sched: Reject CPU affinity changes based on arch_task_cpu_possible_mask()

[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 arch_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.

Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/sched/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 99992d0beb65..095deda50643 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1877,6 +1877,7 @@ static int __set_cpus_allowed_ptr_locked(struct task_struct *p,
 					 struct rq_flags *rf)
 {
 	const struct cpumask *cpu_valid_mask = cpu_active_mask;
+	const struct cpumask *cpu_allowed_mask = arch_task_cpu_possible_mask(p);
 	unsigned int dest_cpu;
 	int ret = 0;
 
@@ -1887,6 +1888,9 @@ static int __set_cpus_allowed_ptr_locked(struct task_struct *p,
 		 * Kernel threads are allowed on online && !active CPUs
 		 */
 		cpu_valid_mask = cpu_online_mask;
+	} else if (!cpumask_subset(new_mask, cpu_allowed_mask)) {
+		ret = -EINVAL;
+		goto out;
 	}
 
 	/*
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v4 11/14] sched: Reject CPU affinity changes based on arch_task_cpu_possible_mask()

[Quentin Perret]

On Tuesday 24 Nov 2020 at 15:50:36 (+0000), Will Deacon wrote:
> 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 arch_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.

I guess mentioning here (or as a comment) the 'funny' behaviour we get
with cpusets wouldn't hurt. But this is a sensible patch nonetheless so:

Reviewed-by: Quentin Perret <qperret@google.com>

Thanks,
Quentin

[PATCH v4 12/14] arm64: Prevent offlining first CPU with 32-bit EL0 on mismatched system

[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.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/cpufeature.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 29017cbb6c8e..fe470683b43e 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1237,6 +1237,8 @@ has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope)
 
 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);
 
@@ -1245,6 +1247,22 @@ static int enable_mismatched_32bit_el0(unsigned int cpu)
 		static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0);
 	}
 
+	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;
+	pr_info("Asymmetric 32-bit EL0 support detected on CPU %u; CPU hot-unplug disabled on CPU %u\n",
+		cpu, lucky_winner);
 	return 0;
 }
 
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v4 12/14] arm64: Prevent offlining first CPU with 32-bit EL0 on mismatched system

[Qais Yousef]

On 11/24/20 15:50, Will Deacon wrote:
> 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.
                                       ^^^^^

You use cpumask_any_and(). Better use cpumask_first_and()? We have a truly
random function now, cpumask_any_and_distribute(), if you'd like to pick
something 'truly' random.

I tried to clean up the cpumask_any* API mess, but ended up with too much
headache instead. Killng of cpumask_any*() might be the easier option. But
that's something different..

> 
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/cpufeature.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 29017cbb6c8e..fe470683b43e 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1237,6 +1237,8 @@ has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope)
>  
>  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);
>  
> @@ -1245,6 +1247,22 @@ static int enable_mismatched_32bit_el0(unsigned int cpu)
>  		static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0);
>  	}
>  
> +	if (cpumask_test_cpu(0, cpu_32bit_el0_mask) == cpu_32bit)
> +		return 0;

Hmm I'm struggling to get what you're doing here. You're treating CPU0 (the
boot CPU) specially here, but I don't get why?

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

cpumask_any_and() could return an error. It could be hard or even impossible to
trigger, but better check if lucky_winner is not >= nr_cpu_ids before calling
get_cpu_device(lucky_winner) to stay in the safe side and avoid a potential
splat?

We can do better by the way and do smarter check in remove_cpu() to block
offlining the last aarch32 capable CPU without 'hardcoding' a specific cpu. But
won't insist and happy to wait for someone to come complaining this is not good
enough first.

Some vendors play games with hotplug to help with saving power. They might want
to dynamically nominate the last man standing 32bit capable CPU. Again, we can
wait for someone to complain first I guess.

Cheers

--
Qais Yousef

> +	get_cpu_device(lucky_winner)->offline_disabled = true;
> +	pr_info("Asymmetric 32-bit EL0 support detected on CPU %u; CPU hot-unplug disabled on CPU %u\n",
> +		cpu, lucky_winner);
>  	return 0;
>  }
>  
> -- 
> 2.29.2.454.gaff20da3a2-goog
> 

Re: [PATCH v4 12/14] arm64: Prevent offlining first CPU with 32-bit EL0 on mismatched system

[Will Deacon]

On Fri, Nov 27, 2020 at 01:41:22PM +0000, Qais Yousef wrote:
> On 11/24/20 15:50, Will Deacon wrote:
> > 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.
>                                        ^^^^^
> 
> You use cpumask_any_and(). Better use cpumask_first_and()? We have a truly
> random function now, cpumask_any_and_distribute(), if you'd like to pick
> something 'truly' random.

I think cpumask_any_and() is better, because it makes it clear that I don't
care about which CPU is chosen (and under the hood it ends up calling
cpumask_first_and() _anyway_). So this is purely cosmetic.

> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index 29017cbb6c8e..fe470683b43e 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -1237,6 +1237,8 @@ has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope)
> >  
> >  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);
> >  
> > @@ -1245,6 +1247,22 @@ static int enable_mismatched_32bit_el0(unsigned int cpu)
> >  		static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0);
> >  	}
> >  
> > +	if (cpumask_test_cpu(0, cpu_32bit_el0_mask) == cpu_32bit)
> > +		return 0;
> 
> Hmm I'm struggling to get what you're doing here. You're treating CPU0 (the
> boot CPU) specially here, but I don't get why?

If our ability to execute 32-bit code is the same as the boot CPU then we
don't have to do anything. That way, we can postpone nominating the lucky
winner until we really need to.

> > +	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);
> 
> cpumask_any_and() could return an error. It could be hard or even impossible to
> trigger, but better check if lucky_winner is not >= nr_cpu_ids before calling
> get_cpu_device(lucky_winner) to stay in the safe side and avoid a potential
> splat?

I don't see how it can return an error here. There are two cases to
consider:

  1. The CPU being brought online is the first 32-bit-capable CPU. In which
     case, we don't use cpumask_any_and() at all.

  2. The CPU being brought online is the first 64-bit-only CPU. In which
     case, the CPU doing the onlining is 32-bit capable and will be in
     the active mask.

> We can do better by the way and do smarter check in remove_cpu() to block
> offlining the last aarch32 capable CPU without 'hardcoding' a specific cpu. But
> won't insist and happy to wait for someone to come complaining this is not good
> enough first.

I couldn't find a satisfactory way to do this without the possibility of
subtle races, so I'd prefer to keep it simple for the moment. In particular,
I wanted to make sure that somebody iterating over the cpu_possible_mask
and calling is_cpu_allowed(p, cpu) for each CPU and a 32-bit task can not
reach the end of the mask without ever getting a value of 'true'.

I'm open to revisiting this once some of this is merged, but right now
I don't think it's needed and it certainly adds complexity.

> Some vendors play games with hotplug to help with saving power. They might want
> to dynamically nominate the last man standing 32bit capable CPU. Again, we can
> wait for someone to complain first I guess.

The reality is that either all "big" cores or all "little" cores will be the
ones that are 32-bit capable, so I doubt it matters an awful lot which one
of the cluster is left online from a PM perspective. The real problem is
that a core has to be left online at all, but I don't think we can avoid
that.

Will

Re: [PATCH v4 12/14] arm64: Prevent offlining first CPU with 32-bit EL0 on mismatched system

[Qais Yousef]

On 12/01/20 22:13, Will Deacon wrote:
> On Fri, Nov 27, 2020 at 01:41:22PM +0000, Qais Yousef wrote:
> > On 11/24/20 15:50, Will Deacon wrote:
> > > 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.
> >                                        ^^^^^
> > 
> > You use cpumask_any_and(). Better use cpumask_first_and()? We have a truly
> > random function now, cpumask_any_and_distribute(), if you'd like to pick
> > something 'truly' random.
> 
> I think cpumask_any_and() is better, because it makes it clear that I don't
> care about which CPU is chosen (and under the hood it ends up calling
> cpumask_first_and() _anyway_). So this is purely cosmetic.
> 
> > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > index 29017cbb6c8e..fe470683b43e 100644
> > > --- a/arch/arm64/kernel/cpufeature.c
> > > +++ b/arch/arm64/kernel/cpufeature.c
> > > @@ -1237,6 +1237,8 @@ has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope)
> > >  
> > >  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);
> > >  
> > > @@ -1245,6 +1247,22 @@ static int enable_mismatched_32bit_el0(unsigned int cpu)
> > >  		static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0);
> > >  	}
> > >  
> > > +	if (cpumask_test_cpu(0, cpu_32bit_el0_mask) == cpu_32bit)
> > > +		return 0;
> > 
> > Hmm I'm struggling to get what you're doing here. You're treating CPU0 (the
> > boot CPU) specially here, but I don't get why?
> 
> If our ability to execute 32-bit code is the same as the boot CPU then we
> don't have to do anything. That way, we can postpone nominating the lucky
> winner until we really need to.

Okay I see what you're doing now. The '== cpu_32bit' part of the check gave me
trouble. If the first N cpus are 64bit only, we'll skip them here. Worth
a comment?

Wouldn't it be better to replace this with a check if cpu_32bit_el0_mask is
empty instead? That would be a lot easier to read.

> 
> > > +	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);
> > 
> > cpumask_any_and() could return an error. It could be hard or even impossible to
> > trigger, but better check if lucky_winner is not >= nr_cpu_ids before calling
> > get_cpu_device(lucky_winner) to stay in the safe side and avoid a potential
> > splat?
> 
> I don't see how it can return an error here. There are two cases to
> consider:
> 
>   1. The CPU being brought online is the first 32-bit-capable CPU. In which
>      case, we don't use cpumask_any_and() at all.
> 
>   2. The CPU being brought online is the first 64-bit-only CPU. In which
>      case, the CPU doing the onlining is 32-bit capable and will be in
>      the active mask.

Now I understand the if condition above; yeah I think this will not hit.
The condition above simply guarantees cpu_32bit_el0_mask is not empty. And
since this is online path, there's a guarantee there's a single bit shared
between the 2 masks since the same path must have set this shared bit.

> 
> > We can do better by the way and do smarter check in remove_cpu() to block
> > offlining the last aarch32 capable CPU without 'hardcoding' a specific cpu. But
> > won't insist and happy to wait for someone to come complaining this is not good
> > enough first.
> 
> I couldn't find a satisfactory way to do this without the possibility of
> subtle races, so I'd prefer to keep it simple for the moment. In particular,
> I wanted to make sure that somebody iterating over the cpu_possible_mask
> and calling is_cpu_allowed(p, cpu) for each CPU and a 32-bit task can not
> reach the end of the mask without ever getting a value of 'true'.
> 
> I'm open to revisiting this once some of this is merged, but right now
> I don't think it's needed and it certainly adds complexity.

Agreed. I just wanted to share some awareness. Let's not make this series more
complicated than it needs to be.

> 
> > Some vendors play games with hotplug to help with saving power. They might want
> > to dynamically nominate the last man standing 32bit capable CPU. Again, we can
> > wait for someone to complain first I guess.
> 
> The reality is that either all "big" cores or all "little" cores will be the
> ones that are 32-bit capable, so I doubt it matters an awful lot which one
> of the cluster is left online from a PM perspective. The real problem is
> that a core has to be left online at all, but I don't think we can avoid
> that.

I don't have specific info, but in theory it could matter. We enforce
a specific core to be always online, rather than allow any core to be offlined
until there's a single one left in the mask. I agree we shouldn't worry about
this for now.

And yes, we can't avoid keeping the last 32bit capable cpu online. The same way
we can't offline the last cpu in a normal system.

Thanks

--
Qais Yousef

Re: [PATCH v4 12/14] arm64: Prevent offlining first CPU with 32-bit EL0 on mismatched system

[Will Deacon]

On Wed, Dec 02, 2020 at 12:59:52PM +0000, Qais Yousef wrote:
> On 12/01/20 22:13, Will Deacon wrote:
> > On Fri, Nov 27, 2020 at 01:41:22PM +0000, Qais Yousef wrote:
> > > On 11/24/20 15:50, Will Deacon wrote:
> > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > > index 29017cbb6c8e..fe470683b43e 100644
> > > > --- a/arch/arm64/kernel/cpufeature.c
> > > > +++ b/arch/arm64/kernel/cpufeature.c
> > > > @@ -1237,6 +1237,8 @@ has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope)
> > > >  
> > > >  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);
> > > >  
> > > > @@ -1245,6 +1247,22 @@ static int enable_mismatched_32bit_el0(unsigned int cpu)
> > > >  		static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0);
> > > >  	}
> > > >  
> > > > +	if (cpumask_test_cpu(0, cpu_32bit_el0_mask) == cpu_32bit)
> > > > +		return 0;
> > > 
> > > Hmm I'm struggling to get what you're doing here. You're treating CPU0 (the
> > > boot CPU) specially here, but I don't get why?
> > 
> > If our ability to execute 32-bit code is the same as the boot CPU then we
> > don't have to do anything. That way, we can postpone nominating the lucky
> > winner until we really need to.
> 
> Okay I see what you're doing now. The '== cpu_32bit' part of the check gave me
> trouble. If the first N cpus are 64bit only, we'll skip them here. Worth
> a comment?
> 
> Wouldn't it be better to replace this with a check if cpu_32bit_el0_mask is
> empty instead? That would be a lot easier to read.

Sorry, but I don't follow. What if all the CPUs are 32-bit capable?

I'll leave the code as-is, since I don't think it's particularly hard to
understand, and it does the right thing.

Will

Re: [PATCH v4 12/14] arm64: Prevent offlining first CPU with 32-bit EL0 on mismatched system

[Qais Yousef]

On 12/02/20 17:42, Will Deacon wrote:
> On Wed, Dec 02, 2020 at 12:59:52PM +0000, Qais Yousef wrote:
> > On 12/01/20 22:13, Will Deacon wrote:
> > > On Fri, Nov 27, 2020 at 01:41:22PM +0000, Qais Yousef wrote:
> > > > On 11/24/20 15:50, Will Deacon wrote:
> > > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > > > index 29017cbb6c8e..fe470683b43e 100644
> > > > > --- a/arch/arm64/kernel/cpufeature.c
> > > > > +++ b/arch/arm64/kernel/cpufeature.c
> > > > > @@ -1237,6 +1237,8 @@ has_cpuid_feature(const struct arm64_cpu_capabilities *entry, int scope)
> > > > >  
> > > > >  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);
> > > > >  
> > > > > @@ -1245,6 +1247,22 @@ static int enable_mismatched_32bit_el0(unsigned int cpu)
> > > > >  		static_branch_enable_cpuslocked(&arm64_mismatched_32bit_el0);
> > > > >  	}
> > > > >  
> > > > > +	if (cpumask_test_cpu(0, cpu_32bit_el0_mask) == cpu_32bit)
> > > > > +		return 0;
> > > > 
> > > > Hmm I'm struggling to get what you're doing here. You're treating CPU0 (the
> > > > boot CPU) specially here, but I don't get why?
> > > 
> > > If our ability to execute 32-bit code is the same as the boot CPU then we
> > > don't have to do anything. That way, we can postpone nominating the lucky
> > > winner until we really need to.
> > 
> > Okay I see what you're doing now. The '== cpu_32bit' part of the check gave me
> > trouble. If the first N cpus are 64bit only, we'll skip them here. Worth
> > a comment?
> > 
> > Wouldn't it be better to replace this with a check if cpu_32bit_el0_mask is
> > empty instead? That would be a lot easier to read.
> 
> Sorry, but I don't follow. What if all the CPUs are 32-bit capable?

You're right I missed this case.

--
Qais Yousef

[PATCH v4 13/14] arm64: Implement arch_task_cpu_possible_mask()

[Will Deacon]

Provide an implementation of arch_task_cpu_possible_mask() so that we
can prevent 64-bit-only cores being added to the 'cpus_mask' for compat
tasks on systems with mismatched 32-bit support at EL0,

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/mmu_context.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
index 0672236e1aea..641dff35a56f 100644
--- a/arch/arm64/include/asm/mmu_context.h
+++ b/arch/arm64/include/asm/mmu_context.h
@@ -251,6 +251,19 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
 #define deactivate_mm(tsk,mm)	do { } while (0)
 #define activate_mm(prev,next)	switch_mm(prev, next, current)
 
+static inline const struct cpumask *
+arch_task_cpu_possible_mask(struct task_struct *p)
+{
+	if (!static_branch_unlikely(&arm64_mismatched_32bit_el0))
+		return cpu_possible_mask;
+
+	if (!is_compat_thread(task_thread_info(p)))
+		return cpu_possible_mask;
+
+	return system_32bit_el0_cpumask();
+}
+#define arch_task_cpu_possible_mask	arch_task_cpu_possible_mask
+
 void verify_cpu_asid_bits(void);
 void post_ttbr_update_workaround(void);
 
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v4 13/14] arm64: Implement arch_task_cpu_possible_mask()

[Qais Yousef]

On 11/24/20 15:50, Will Deacon wrote:
> Provide an implementation of arch_task_cpu_possible_mask() so that we
> can prevent 64-bit-only cores being added to the 'cpus_mask' for compat
> tasks on systems with mismatched 32-bit support at EL0,
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/mmu_context.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
> index 0672236e1aea..641dff35a56f 100644
> --- a/arch/arm64/include/asm/mmu_context.h
> +++ b/arch/arm64/include/asm/mmu_context.h

nit: wouldn't cpufeature.h be a better header? No strong opinion really, it
just looked weird to see this among memory management related code.

Cheers

--
Qais Yousef

> @@ -251,6 +251,19 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
>  #define deactivate_mm(tsk,mm)	do { } while (0)
>  #define activate_mm(prev,next)	switch_mm(prev, next, current)
>  
> +static inline const struct cpumask *
> +arch_task_cpu_possible_mask(struct task_struct *p)
> +{
> +	if (!static_branch_unlikely(&arm64_mismatched_32bit_el0))
> +		return cpu_possible_mask;
> +
> +	if (!is_compat_thread(task_thread_info(p)))
> +		return cpu_possible_mask;
> +
> +	return system_32bit_el0_cpumask();
> +}
> +#define arch_task_cpu_possible_mask	arch_task_cpu_possible_mask
> +
>  void verify_cpu_asid_bits(void);
>  void post_ttbr_update_workaround(void);
>  
> -- 
> 2.29.2.454.gaff20da3a2-goog
> 

[PATCH v4 14/14] arm64: Remove logic to kill 32-bit tasks on 64-bit-only cores

[Will Deacon]

The scheduler now knows enough about these braindead systems to place
32-bit tasks accordingly, so throw out the safety checks and allow the
ret-to-user path to avoid do_notify_resume() if there is nothing to do.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/process.c | 14 +-------------
 arch/arm64/kernel/signal.c  | 26 --------------------------
 2 files changed, 1 insertion(+), 39 deletions(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 72116b0c7c73..e21c631c8b17 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -543,15 +543,6 @@ static void erratum_1418040_thread_switch(struct task_struct *prev,
 	write_sysreg(val, cntkctl_el1);
 }
 
-static void compat_thread_switch(struct task_struct *next)
-{
-	if (!is_compat_thread(task_thread_info(next)))
-		return;
-
-	if (static_branch_unlikely(&arm64_mismatched_32bit_el0))
-		set_tsk_thread_flag(next, TIF_NOTIFY_RESUME);
-}
-
 /*
  * Thread switching.
  */
@@ -568,7 +559,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
 	uao_thread_switch(next);
 	ssbs_thread_switch(next);
 	erratum_1418040_thread_switch(prev, next);
-	compat_thread_switch(next);
 
 	/*
 	 * Complete any pending TLB or cache maintenance on this CPU in case
@@ -644,7 +634,7 @@ static void adjust_compat_task_affinity(struct task_struct *p)
 	 * execve().
 	 */
 	if (!restrict_cpus_allowed_ptr(p, possible_mask))
-		goto out;
+		return;
 
 	if (alloc_cpumask_var(&cpuset_mask, GFP_KERNEL)) {
 		cpuset_cpus_allowed(p, cpuset_mask);
@@ -661,8 +651,6 @@ static void adjust_compat_task_affinity(struct task_struct *p)
 out_set_mask:
 	set_cpus_allowed_ptr(p, newmask);
 	free_cpumask_var(cpuset_mask);
-out:
-	set_tsk_thread_flag(current, TIF_NOTIFY_RESUME);
 }
 
 /*
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index bcb6ca2d9a7c..a8184cad8890 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -911,19 +911,6 @@ static void do_signal(struct pt_regs *regs)
 	restore_saved_sigmask();
 }
 
-static bool cpu_affinity_invalid(struct pt_regs *regs)
-{
-	if (!compat_user_mode(regs))
-		return false;
-
-	/*
-	 * We're preemptible, but a reschedule will cause us to check the
-	 * affinity again.
-	 */
-	return !cpumask_test_cpu(raw_smp_processor_id(),
-				 system_32bit_el0_cpumask());
-}
-
 asmlinkage void do_notify_resume(struct pt_regs *regs,
 				 unsigned long thread_flags)
 {
@@ -961,19 +948,6 @@ asmlinkage void do_notify_resume(struct pt_regs *regs,
 			if (thread_flags & _TIF_NOTIFY_RESUME) {
 				tracehook_notify_resume(regs);
 				rseq_handle_notify_resume(NULL, regs);
-
-				/*
-				 * If we reschedule after checking the affinity
-				 * then we must ensure that TIF_NOTIFY_RESUME
-				 * is set so that we check the affinity again.
-				 * Since tracehook_notify_resume() clears the
-				 * flag, ensure that the compiler doesn't move
-				 * it after the affinity check.
-				 */
-				barrier();
-
-				if (cpu_affinity_invalid(regs))
-					force_sig(SIGKILL);
 			}
 
 			if (thread_flags & _TIF_FOREIGN_FPSTATE)
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v4 00/14] An alternative series for asymmetric AArch32 systems

[Qais Yousef]

Hi Will

On 11/24/20 15:50, Will Deacon wrote:
> Hello folks,
> 
> Here's version four of the wonderful patches I previously posted here:
> 
>   v1: https://lore.kernel.org/r/20201027215118.27003-1-will@kernel.org
>   v2: https://lore.kernel.org/r/20201109213023.15092-1-will@kernel.org
>   v3: https://lore.kernel.org/r/20201113093720.21106-1-will@kernel.org
> 
> and which started life as a reimplementation of some patches from Qais:
> 
>   https://lore.kernel.org/r/20201021104611.2744565-1-qais.yousef@arm.com
> 
> The aim of this series is to allow 32-bit ARM applications to run on
> arm64 SoCs where not all of the CPUs support the 32-bit instruction set.
> Unfortunately, such SoCs are real and will continue to be productised
> over the next few years at least.
> 
> Changes in v4 include:
> 
>   * Take into account the cpuset 'allowed' mask on exec
>   * Print a warning if we forcefully override the affinity, like we do
>     in select_fallback_rq()
>   * Rename arch_cpu_allowed_mask() to arch_task_cpu_possible_mask()
>   * Added a comment to adjust_compat_task_affinity()

Thanks for that and sorry for the delayed response. I wanted to dig in sooner
but a combination of holiday and other matters kept me away longer than
I wanted.

I think this approach is definitely the most elegant option and something
Morten and myself tried to argue for on the first post. The new execve problem
now mandates this approach as it's hard to fix otherwise. The only other
suggestion I had is to extend execve syscall to accept a cgroup to attach the
task to. Similar to what we have in clone3 syscall. I think libc can be
modified then to ensure 32bit binaries execute into a cpuset with the right
affinity set. But that's less elegant approach.

I'm trying to run more tests on it, specially with regard to cpuset handling as
this is the trickiest bit. The approach and patches look good to me in general.
Most of my comments were minor remarks or nits.

My only worry is that this approach might be too elegant to deter these SoCs
from proliferating :-p

Cheers

--
Qais Yousef

> 
> Cheers,
> 
> Will
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Morten Rasmussen <morten.rasmussen@arm.com>
> Cc: Qais Yousef <qais.yousef@arm.com>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Quentin Perret <qperret@google.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Li Zefan <lizefan@huawei.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Juri Lelli <juri.lelli@redhat.com>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: kernel-team@android.com
> 
> --->8
> 
> Will Deacon (14):
>   arm64: cpuinfo: Split AArch32 registers out into a separate struct
>   arm64: Allow mismatched 32-bit EL0 support
>   KVM: arm64: Kill 32-bit vCPUs on systems with mismatched EL0 support
>   arm64: Kill 32-bit applications scheduled on 64-bit-only CPUs
>   arm64: Advertise CPUs capable of running 32-bit applications in sysfs
>   arm64: Hook up cmdline parameter to allow mismatched 32-bit EL0
>   sched: Introduce restrict_cpus_allowed_ptr() to limit task CPU
>     affinity
>   arm64: exec: Adjust affinity for compat tasks with mismatched 32-bit
>     EL0
>   cpuset: Don't use the cpu_possible_mask as a last resort for cgroup v1
>   sched: Introduce arch_task_cpu_possible_mask() to limit fallback rq
>     selection
>   sched: Reject CPU affinity changes based on
>     arch_task_cpu_possible_mask()
>   arm64: Prevent offlining first CPU with 32-bit EL0 on mismatched
>     system
>   arm64: Implement arch_task_cpu_possible_mask()
>   arm64: Remove logic to kill 32-bit tasks on 64-bit-only cores
> 
>  .../ABI/testing/sysfs-devices-system-cpu      |   9 +
>  .../admin-guide/kernel-parameters.txt         |   7 +
>  arch/arm64/include/asm/cpu.h                  |  44 ++--
>  arch/arm64/include/asm/cpucaps.h              |   2 +-
>  arch/arm64/include/asm/cpufeature.h           |   8 +-
>  arch/arm64/include/asm/mmu_context.h          |  13 ++
>  arch/arm64/kernel/cpufeature.c                | 219 ++++++++++++++----
>  arch/arm64/kernel/cpuinfo.c                   |  53 +++--
>  arch/arm64/kernel/process.c                   |  47 +++-
>  arch/arm64/kvm/arm.c                          |  11 +-
>  include/linux/sched.h                         |   1 +
>  kernel/cgroup/cpuset.c                        |   6 +-
>  kernel/sched/core.c                           |  90 +++++--
>  13 files changed, 401 insertions(+), 109 deletions(-)
> 
> -- 
> 2.29.2.454.gaff20da3a2-goog
> 

Re: [PATCH v4 00/14] An alternative series for asymmetric AArch32 systems

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

On Tue 2020-11-24 15:50:25, Will Deacon wrote:
> Hello folks,
> 
> Here's version four of the wonderful patches I previously posted here:
> 
>   v1: https://lore.kernel.org/r/20201027215118.27003-1-will@kernel.org
>   v2: https://lore.kernel.org/r/20201109213023.15092-1-will@kernel.org
>   v3: https://lore.kernel.org/r/20201113093720.21106-1-will@kernel.org
> 
> and which started life as a reimplementation of some patches from Qais:
> 
>   https://lore.kernel.org/r/20201021104611.2744565-1-qais.yousef@arm.com
> 
> The aim of this series is to allow 32-bit ARM applications to run on
> arm64 SoCs where not all of the CPUs support the 32-bit instruction set.
> Unfortunately, such SoCs are real and will continue to be productised
> over the next few years at least.

Out of curiosity, what systems are that?

Is the 32-bit available on the big or on the little cores?

And... fun way to accelerate demise of arm32 :-).

Best regards,
									Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]