[PATCHv3 0/5] arm64: handle heterogeneous system register values

[PATCHv3 0/5] arm64: handle heterogeneous system register values

[Mark Rutland]

Currently the arm64 kernel assumes CPUs are homogeneous, which is a source of
several potential problems as this is not always true. This series is an
attempt to ameliorate the situation. The first posting [1] was titled as
"arm64: extend/fix /proc/cpuinfo + runtime sanity checks", but with the
addition of the I-cache policy fix this title was either going to become
unwieldy or misleading.

Due to rework since v2 I've dropped Acks from most patches. A once-over from
those who have reviewed this before would be much appreciated.

I had a go at shrinking the set of hwcaps if features were not uniform, but
after several false starts and realisation of further problems (What if dczid
varies? How do we tear down kernel-side feature usage?) I decided that way
madness lies. I now set TAINT_CPU_OUT_OF_SPEC when registers are fundamentally
mismatched, and WARN the user. I am very tempted to panic() instead given how
easy it is to miss the splat the sanity checks produce at boot-time, but I
suspect that might be a little mean ;)

I'm still not happy with printing the hwcaps once rather than per-cpu, but
given everyone else seems to think that's the right way to handle things I'll
give up on that.

Since v2 [4]:
* Print the hwcaps once in /proc/cpinfo, per Ard's comments.
* Apply fixupps per Will's comments.
* Sanity check additional registers: dczid id_aa64{isar,mmfr,pfr}1.
* We care about GICv3 support reporting, don't mask it out.
* Set TAINT_CPU_OUT_OF_SPEC upon sanity check failure.
* Store boot CPU values separately.
* Separate value stashing into new patch.
* Rebased the branch to v3.16-rc2.
Since v1 [1]:
* Add fix for differing I-cache policies
* Make read_cpuid_part_number and read_cpu_implementor return extracted fields.

This series adds infrastructure to record various system registers upon the
booting of each CPU into a new per-CPU struct cpuinfo_arm64. This data can then
be used to solve various problems:

* On arm64 /proc/cpuinfo only contains information regarding the current CPU
  (i.e. that which issued the read), due to use of read_cpuid_id() within
  c_show. This isn't fantastic as it doesn't match arm and x86 (which print
  information per-cpu), and on big.LITTLE AArch64 systems it will mean we don't
  get accurate information regarding each CPU. Using the newly recorded
  information, we can print correct information per-CPU in /proc/cpuinfo.

* Level 1 instruction cache policy can differ across CPU micro-architectures in
  big.LITTLE configurations, which can lead to a subset of cores reporting
  aliasing I-caches in CTR_EL0.L1Ip. If any CPUs in the system have VIPT or
  AIVIVT I-caches, the appropriate maintenance must be performed even from those
  CPUs whose caches are non-aliasing. We can use the newly recorded information
  to detect when this is the case.

* Sometimes CPUs are erroneously configured differently by firmware such that a
  feature is only enabled on a subset of CPUs. Even in the presence of
  big.LITTLE systems we do not generally expect this, and such problems can be
  difficult to detect. To enable early detection of mismatched capabilities in
  SMP systems, we can use the newly recorded information to compare CPUs
  feature-wise and identify potential issues.

The series is based on v3.16-rc2, and can be found in my
arm64/heterogeneous-sysreg branch [2,3].

Cheers,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/255389.html
[2] http://linux-arm.org/git?p=linux-mr.git;a=shortlog;h=refs/heads/arm64/heterogeneous-sysreg
[3] git://linux-arm.org/linux-mr.git arm64/heterogeneous-sysreg
[4] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/264387.html

Mark Rutland (5):
  arm64: add MIDR_EL1 field accessors
  arm64: cpuinfo: record cpu system register values
  arm64: cpuinfo: print info for all CPUs
  arm64: cachetype: report weakest cache policy
  arm64: add runtime system sanity checks

 arch/arm64/include/asm/cachetype.h |  16 +--
 arch/arm64/include/asm/cpu.h       |  59 ++++++++++++
 arch/arm64/include/asm/cputype.h   |  33 +++++--
 arch/arm64/kernel/Makefile         |   3 +-
 arch/arm64/kernel/cpuinfo.c        | 193 +++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/setup.c          |  36 +++----
 arch/arm64/kernel/smp.c            |   6 ++
 7 files changed, 315 insertions(+), 31 deletions(-)
 create mode 100644 arch/arm64/include/asm/cpu.h
 create mode 100644 arch/arm64/kernel/cpuinfo.c

-- 
1.9.1

[PATCHv3 1/5] arm64: add MIDR_EL1 field accessors

[Mark Rutland]

The MIDR_EL1 register is composed of a number of bitfields, and uses of
the fields has so far involved open-coding of the shifts and masks
required.

This patch adds shifts and masks for each of the MIDR_EL1 subfields, and
also provides accessors built atop of these. Existing uses within
cputype.h are updated to use these accessors.

The read_cpuid_part_number macro is modified to return the extracted
bitfield rather than returning the value in-place with all other fields
(including revision) masked out, to better match the other accessors.
As the value is only used in comparison with the *_CPU_PART_* macros
which are similarly updated, and these values are never exposed to
userspace, this change should not affect any functionality.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/cputype.h | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
index 27f54a7..ec5e41c 100644
--- a/arch/arm64/include/asm/cputype.h
+++ b/arch/arm64/include/asm/cputype.h
@@ -36,15 +36,34 @@
 	__val;								\
 })
 
+#define MIDR_REVISION_MASK	0xf
+#define MIDR_REVISION(midr)	((midr) & MIDR_REVISION_MASK)
+#define MIDR_PARTNUM_SHIFT	4
+#define MIDR_PARTNUM_MASK	(0xfff << MIDR_PARTNUM_SHIFT)
+#define MIDR_PARTNUM(midr)	\
+	(((midr) & MIDR_PARTNUM_MASK) >> MIDR_PARTNUM_SHIFT)
+#define MIDR_ARCHITECTURE_SHIFT	16
+#define MIDR_ARCHITECTURE_MASK	(0xf << MIDR_ARCHITECTURE_SHIFT)
+#define MIDR_ARCHITECTURE(midr)	\
+	(((midr) & MIDR_ARCHITECTURE_MASK) >> MIDR_ARCHITECTURE_SHIFT)
+#define MIDR_VARIANT_SHIFT	20
+#define MIDR_VARIANT_MASK	(0xf << MIDR_VARIANT_SHIFT)
+#define MIDR_VARIANT(midr)	\
+	(((midr) & MIDR_VARIANT_MASK) >> MIDR_VARIANT_SHIFT)
+#define MIDR_IMPLEMENTOR_SHIFT	24
+#define MIDR_IMPLEMENTOR_MASK	(0xff << MIDR_IMPLEMENTOR_SHIFT)
+#define MIDR_IMPLEMENTOR(midr)	\
+	(((midr) & MIDR_IMPLEMENTOR_MASK) >> MIDR_IMPLEMENTOR_SHIFT)
+
 #define ARM_CPU_IMP_ARM		0x41
 #define ARM_CPU_IMP_APM		0x50
 
-#define ARM_CPU_PART_AEM_V8	0xD0F0
-#define ARM_CPU_PART_FOUNDATION	0xD000
-#define ARM_CPU_PART_CORTEX_A53	0xD030
-#define ARM_CPU_PART_CORTEX_A57	0xD070
+#define ARM_CPU_PART_AEM_V8	0xD0F
+#define ARM_CPU_PART_FOUNDATION	0xD00
+#define ARM_CPU_PART_CORTEX_A57	0xD07
+#define ARM_CPU_PART_CORTEX_A53	0xD03
 
-#define APM_CPU_PART_POTENZA	0x0000
+#define APM_CPU_PART_POTENZA	0x000
 
 #ifndef __ASSEMBLY__
 
@@ -65,12 +84,12 @@ static inline u64 __attribute_const__ read_cpuid_mpidr(void)
 
 static inline unsigned int __attribute_const__ read_cpuid_implementor(void)
 {
-	return (read_cpuid_id() & 0xFF000000) >> 24;
+	return MIDR_IMPLEMENTOR(read_cpuid_id());
 }
 
 static inline unsigned int __attribute_const__ read_cpuid_part_number(void)
 {
-	return (read_cpuid_id() & 0xFFF0);
+	return MIDR_PARTNUM(read_cpuid_id());
 }
 
 static inline u32 __attribute_const__ read_cpuid_cachetype(void)
-- 
1.9.1

[PATCHv3 2/5] arm64: cpuinfo: record cpu system register values

[Mark Rutland]

Several kernel subsystems need to know details about CPU system register
values, sometimes for CPUs other than that they are executing on. Rather
than hard-coding system register accesses and cross-calls for these
cases, this patch adds logic to record various system register values at
boot-time. This may be used for feature reporting, firmware bug
detection, etc.

Separate hooks are added for the boot and hotplug paths to enable
one-time intialisation and cold/warm boot value mismatch detection in
later patches.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/include/asm/cpu.h | 59 ++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/Makefile   |  3 +-
 arch/arm64/kernel/cpuinfo.c  | 75 ++++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/setup.c    |  7 +++--
 arch/arm64/kernel/smp.c      |  6 ++++
 5 files changed, 146 insertions(+), 4 deletions(-)
 create mode 100644 arch/arm64/include/asm/cpu.h
 create mode 100644 arch/arm64/kernel/cpuinfo.c

diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
new file mode 100644
index 0000000..0564430
--- /dev/null
+++ b/arch/arm64/include/asm/cpu.h
@@ -0,0 +1,59 @@
+/*
+  * Copyright (C) 2014 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __ASM_CPU_H
+#define __ASM_CPU_H
+
+#include <linux/cpu.h>
+#include <linux/init.h>
+#include <linux/percpu.h>
+
+/*
+ * Records attributes of an individual CPU.
+ */
+struct cpuinfo_arm64 {
+	struct cpu	cpu;
+	u32		reg_ctr;
+	u32		reg_cntfrq;
+	u32		reg_dczid;
+	u32		reg_midr;
+
+	u64		reg_id_aa64isar0;
+	u64		reg_id_aa64isar1;
+	u64		reg_id_aa64mmfr0;
+	u64		reg_id_aa64mmfr1;
+	u64		reg_id_aa64pfr0;
+	u64		reg_id_aa64pfr1;
+
+	u32		reg_id_isar0;
+	u32		reg_id_isar1;
+	u32		reg_id_isar2;
+	u32		reg_id_isar3;
+	u32		reg_id_isar4;
+	u32		reg_id_isar5;
+	u32		reg_id_mmfr0;
+	u32		reg_id_mmfr1;
+	u32		reg_id_mmfr2;
+	u32		reg_id_mmfr3;
+	u32		reg_id_pfr0;
+	u32		reg_id_pfr1;
+};
+
+DECLARE_PER_CPU(struct cpuinfo_arm64, cpu_data);
+
+void cpuinfo_store_cpu(void);
+void __init cpuinfo_store_boot_cpu(void);
+
+#endif /* __ASM_CPU_H */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index cdaedad..27c72ef 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -15,7 +15,8 @@ CFLAGS_REMOVE_return_address.o = -pg
 arm64-obj-y		:= cputable.o debug-monitors.o entry.o irq.o fpsimd.o	\
 			   entry-fpsimd.o process.o ptrace.o setup.o signal.o	\
 			   sys.o stacktrace.o time.o traps.o io.o vdso.o	\
-			   hyp-stub.o psci.o cpu_ops.o insn.o return_address.o
+			   hyp-stub.o psci.o cpu_ops.o insn.o return_address.o	\
+			   cpuinfo.o
 
 arm64-obj-$(CONFIG_COMPAT)		+= sys32.o kuser32.o signal32.o 	\
 					   sys_compat.o
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
new file mode 100644
index 0000000..65907f3
--- /dev/null
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -0,0 +1,75 @@
+/*
+ * Record and handle CPU attributes.
+ *
+ * Copyright (C) 2014 ARM Ltd.
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <asm/arch_timer.h>
+#include <asm/cachetype.h>
+#include <asm/cpu.h>
+#include <asm/cputype.h>
+
+#include <linux/init.h>
+#include <linux/smp.h>
+
+/*
+ * In case the boot CPU is hotpluggable, we record its initial state and
+ * current state separately. Certain system registers may contain different
+ * values depending on configuration at or after reset.
+ */
+DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
+static struct cpuinfo_arm64 boot_cpu_data;
+
+static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
+{
+	info->reg_cntfrq = arch_timer_get_cntfrq();
+	info->reg_ctr = read_cpuid_cachetype();
+	info->reg_dczid = read_cpuid(DCZID_EL0);
+	info->reg_midr = read_cpuid_id();
+
+	info->reg_id_aa64isar0 = read_cpuid(ID_AA64ISAR0_EL1);
+	info->reg_id_aa64isar1 = read_cpuid(ID_AA64ISAR1_EL1);
+	info->reg_id_aa64mmfr0 = read_cpuid(ID_AA64MMFR0_EL1);
+	info->reg_id_aa64mmfr1 = read_cpuid(ID_AA64MMFR1_EL1);
+	info->reg_id_aa64pfr0 = read_cpuid(ID_AA64PFR0_EL1);
+	info->reg_id_aa64pfr1 = read_cpuid(ID_AA64PFR1_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_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_pfr0 = read_cpuid(ID_PFR0_EL1);
+	info->reg_id_pfr1 = read_cpuid(ID_PFR1_EL1);
+}
+
+void cpuinfo_store_cpu(void)
+{
+	unsigned int cpu = smp_processor_id();
+	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu);
+	__cpuinfo_store_cpu(info);
+}
+
+void __init cpuinfo_store_boot_cpu(void)
+{
+	unsigned int cpu = smp_processor_id();
+	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu);
+	__cpuinfo_store_cpu(info);
+
+	boot_cpu_data = *info;
+}
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 46d1125..edb146d 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -45,6 +45,7 @@
 #include <linux/efi.h>
 
 #include <asm/fixmap.h>
+#include <asm/cpu.h>
 #include <asm/cputype.h>
 #include <asm/elf.h>
 #include <asm/cputable.h>
@@ -219,6 +220,8 @@ static void __init setup_processor(void)
 	sprintf(init_utsname()->machine, ELF_PLATFORM);
 	elf_hwcap = 0;
 
+	cpuinfo_store_boot_cpu();
+
 	/*
 	 * Check for sane CTR_EL0.CWG value.
 	 */
@@ -417,14 +420,12 @@ static int __init arm64_device_init(void)
 }
 arch_initcall_sync(arm64_device_init);
 
-static DEFINE_PER_CPU(struct cpu, cpu_data);
-
 static int __init topology_init(void)
 {
 	int i;
 
 	for_each_possible_cpu(i) {
-		struct cpu *cpu = &per_cpu(cpu_data, i);
+		struct cpu *cpu = &per_cpu(cpu_data.cpu, i);
 		cpu->hotpluggable = 1;
 		register_cpu(cpu, i);
 	}
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 40f38f4..3e2f5eb 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -39,6 +39,7 @@
 
 #include <asm/atomic.h>
 #include <asm/cacheflush.h>
+#include <asm/cpu.h>
 #include <asm/cputype.h>
 #include <asm/cpu_ops.h>
 #include <asm/mmu_context.h>
@@ -155,6 +156,11 @@ asmlinkage void secondary_start_kernel(void)
 		cpu_ops[cpu]->cpu_postboot();
 
 	/*
+	 * Log the CPU info before it is marked online and might get read.
+	 */
+	cpuinfo_store_cpu();
+
+	/*
 	 * Enable GIC and timers.
 	 */
 	notify_cpu_starting(cpu);
-- 
1.9.1

[PATCHv3 3/5] arm64: cpuinfo: print info for all CPUs

[Mark Rutland]

Currently reading /proc/cpuinfo will result in information being read
out of the MIDR_EL1 of the current CPU, and the information is not
associated with any particular logical CPU number.

This is problematic for systems with heterogeneous CPUs (i.e.
big.LITTLE) where fields will vary across CPUs, and the output will
differ depending on the executing CPU.

This patch reorganises the code responsible for /proc/cpuinfo to print
information per-cpu. As hwcaps are expected to be identical across all
CPUs, this information is printed once.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/setup.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index edb146d..b00b7a3 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -448,20 +448,30 @@ static const char *hwcap_str[] = {
 
 static int c_show(struct seq_file *m, void *v)
 {
-	int i;
+	int c, i;
 
-	seq_printf(m, "Processor\t: %s rev %d (%s)\n",
-		   cpu_name, read_cpuid_id() & 15, ELF_PLATFORM);
+	for_each_online_cpu(c) {
+		struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, c);
+		u32 midr = cpuinfo->reg_midr;
 
-	for_each_online_cpu(i) {
 		/*
 		 * glibc reads /proc/cpuinfo to determine the number of
 		 * online processors, looking for lines beginning with
 		 * "processor".  Give glibc what it expects.
 		 */
 #ifdef CONFIG_SMP
-		seq_printf(m, "processor\t: %d\n", i);
+		seq_printf(m, "processor\t: %d\n", c);
 #endif
+		seq_printf(m, "Type\t\t: %s rev %d (%s)\n",
+			   cpu_name, MIDR_REVISION(midr), ELF_PLATFORM);
+		seq_printf(m, "CPU implementer\t: 0x%02x\n",
+			   MIDR_IMPLEMENTOR(midr));
+		seq_printf(m, "CPU architecture: AArch64\n");
+		seq_printf(m, "CPU variant\t: 0x%x\n", MIDR_VARIANT(midr));
+		seq_printf(m, "CPU part\t: 0x%03x\n", MIDR_PARTNUM(midr));
+		seq_printf(m, "CPU revision\t: %d\n", MIDR_REVISION(midr));
+
+		seq_puts(m, "\n");
 	}
 
 	/* dump out the processor features */
@@ -470,17 +480,8 @@ static int c_show(struct seq_file *m, void *v)
 	for (i = 0; hwcap_str[i]; i++)
 		if (elf_hwcap & (1 << i))
 			seq_printf(m, "%s ", hwcap_str[i]);
-
-	seq_printf(m, "\nCPU implementer\t: 0x%02x\n", read_cpuid_id() >> 24);
-	seq_printf(m, "CPU architecture: AArch64\n");
-	seq_printf(m, "CPU variant\t: 0x%x\n", (read_cpuid_id() >> 20) & 15);
-	seq_printf(m, "CPU part\t: 0x%03x\n", (read_cpuid_id() >> 4) & 0xfff);
-	seq_printf(m, "CPU revision\t: %d\n", read_cpuid_id() & 15);
-
 	seq_puts(m, "\n");
 
-	seq_printf(m, "Hardware\t: %s\n", machine_name);
-
 	return 0;
 }
 
-- 
1.9.1

[PATCHv3 4/5] arm64: cachetype: report weakest cache policy

[Mark Rutland]

In big.LITTLE systems, the I-cache policy may differ across CPUs, and
thus we must always meet the most stringent maintenance requirements of
any I-cache in the system when performing maintenance to ensure
correctness. Unfortunately this requirement is not met as we always look
at the current CPU's cache type register to determine the maintenance
requirements.

This patch causes the I-cache policy of all CPUs to be taken into
account for icache_is_aliasing and icache_is_aivivt. If any I-cache in
the system is aliasing or AIVIVT, the respective function will return
true. At boot each CPU may set flags to identify that at least one
I-cache in the system is aliasing and/or AIVIVT.

The now unused and potentially misleading icache_policy function is
removed.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/cachetype.h | 16 ++++++++++------
 arch/arm64/kernel/cpuinfo.c        | 32 +++++++++++++++++++++++++++++---
 2 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/cachetype.h b/arch/arm64/include/asm/cachetype.h
index 4b23e75..7a2e076 100644
--- a/arch/arm64/include/asm/cachetype.h
+++ b/arch/arm64/include/asm/cachetype.h
@@ -30,10 +30,14 @@
 
 #ifndef __ASSEMBLY__
 
-static inline u32 icache_policy(void)
-{
-	return (read_cpuid_cachetype() >> CTR_L1IP_SHIFT) & CTR_L1IP_MASK;
-}
+#include <linux/bitops.h>
+
+#define CTR_L1IP(ctr)	(((ctr) >> CTR_L1IP_SHIFT) & CTR_L1IP_MASK)
+
+#define ICACHEF_ALIASING	BIT(0)
+#define ICACHEF_AIVIVT		BIT(1)
+
+extern unsigned long __icache_flags;
 
 /*
  * Whilst the D-side always behaves as PIPT on AArch64, aliasing is
@@ -41,12 +45,12 @@ static inline u32 icache_policy(void)
  */
 static inline int icache_is_aliasing(void)
 {
-	return icache_policy() != ICACHE_POLICY_PIPT;
+	return test_bit(ICACHEF_ALIASING, &__icache_flags);
 }
 
 static inline int icache_is_aivivt(void)
 {
-	return icache_policy() == ICACHE_POLICY_AIVIVT;
+	return test_bit(ICACHEF_AIVIVT, &__icache_flags);
 }
 
 static inline u32 cache_type_cwg(void)
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 65907f3..20ca6d0 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -19,7 +19,9 @@
 #include <asm/cpu.h>
 #include <asm/cputype.h>
 
+#include <linux/bitops.h>
 #include <linux/init.h>
+#include <linux/printk.h>
 #include <linux/smp.h>
 
 /*
@@ -30,7 +32,29 @@
 DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
 static struct cpuinfo_arm64 boot_cpu_data;
 
-static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
+static char *icache_policy_str[] = {
+	[ICACHE_POLICY_RESERVED] = "RESERVED/UNKNOWN",
+	[ICACHE_POLICY_AIVIVT] = "AIVIVT",
+	[ICACHE_POLICY_VIPT] = "VIPT",
+	[ICACHE_POLICY_PIPT] = "PIPT",
+};
+
+unsigned long __icache_flags;
+
+static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info,
+					 unsigned int cpu)
+{
+	u32 l1ip = CTR_L1IP(info->reg_ctr);
+
+	if (l1ip != ICACHE_POLICY_PIPT)
+		set_bit(ICACHEF_ALIASING, &__icache_flags);
+	if (l1ip == ICACHE_POLICY_AIVIVT);
+		set_bit(ICACHEF_AIVIVT, &__icache_flags);
+
+	pr_info("Detected %s I-cache on CPU%d", icache_policy_str[l1ip], cpu);
+}
+
+static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info, unsigned int cpu)
 {
 	info->reg_cntfrq = arch_timer_get_cntfrq();
 	info->reg_ctr = read_cpuid_cachetype();
@@ -56,20 +80,22 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
 	info->reg_id_mmfr3 = read_cpuid(ID_MMFR3_EL1);
 	info->reg_id_pfr0 = read_cpuid(ID_PFR0_EL1);
 	info->reg_id_pfr1 = read_cpuid(ID_PFR1_EL1);
+
+	cpuinfo_detect_icache_policy(info, cpu);
 }
 
 void cpuinfo_store_cpu(void)
 {
 	unsigned int cpu = smp_processor_id();
 	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu);
-	__cpuinfo_store_cpu(info);
+	__cpuinfo_store_cpu(info, cpu);
 }
 
 void __init cpuinfo_store_boot_cpu(void)
 {
 	unsigned int cpu = smp_processor_id();
 	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu);
-	__cpuinfo_store_cpu(info);
+	__cpuinfo_store_cpu(info, cpu);
 
 	boot_cpu_data = *info;
 }
-- 
1.9.1

[PATCHv3 5/5] arm64: add runtime system sanity checks

[Mark Rutland]

Unexpected variation in certain system register values across CPUs is an
indicator of potential problems with a system. The kernel expects CPUs
to be mostly identical in terms of supported features, even in systems
with heterogeneous CPUs, with uniform instruction set support being
critical for the correct operation of userspace.

To help detect issues early where hardware violates the expectations of
the kernel, this patch adds simple runtime sanity checks on important ID
registers in the bring up path of each CPU.

Where CPUs are fundamentally mismatched, set TAINT_CPU_OUT_OF_SPEC.
Given that the kernel assumes CPUs are identical feature wise, let's not
pretend that we expect such configurations to work. Supporting such
configurations would require massive rework, and hopefully they will
never exist.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/cpuinfo.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 20ca6d0..fb4a70e 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -21,6 +21,7 @@
 
 #include <linux/bitops.h>
 #include <linux/init.h>
+#include <linux/kernel.h>
 #include <linux/printk.h>
 #include <linux/smp.h>
 
@@ -54,6 +55,96 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info,
 	pr_info("Detected %s I-cache on CPU%d", icache_policy_str[l1ip], cpu);
 }
 
+static int check_reg_mask(char *name, u64 mask, u64 boot, u64 cur, int cpu)
+{
+	if ((boot & mask) == (cur & mask))
+		return 0;
+
+	pr_warn("SANITY CHECK: Unexpected variation in %s. Boot CPU: %#016lx, CPU%d: %#016lx\n",
+		name, (unsigned long)boot, cpu, (unsigned long)cur);
+
+	return 1;
+}
+
+#define CHECK_MASK(field, mask, boot, cur, cpu) \
+	check_reg_mask(#field, mask, (boot)->reg_ ## field, (cur)->reg_ ## field, cpu)
+
+#define CHECK(field, boot, cur, cpu) \
+	CHECK_MASK(field, ~0ULL, boot, cur, cpu)
+
+/*
+ * Verify that CPUs don't have unexpected differences that will cause problems.
+ */
+static void cpuinfo_sanity_check(struct cpuinfo_arm64 *cur, int cpu)
+{
+	struct cpuinfo_arm64 *boot = &boot_cpu_data;
+	unsigned int diff = 0;
+
+	/*
+	 * The kernel can handle differing I-cache policies, but otherwise
+	 * caches should look identical. Userspace JITs will make use of
+	 * *minLine.
+	 */
+	diff |= CHECK_MASK(ctr, 0xffff3fff, boot, cur, cpu);
+
+	/*
+	 * Userspace may perform DC ZVA instructions. Mismatched block sizes
+	 * could result in too much or too little memory being zeroed if a
+	 * process is preempted and migrated between CPUs.
+	 */
+	diff |= CHECK(dczid, boot, cur, cpu);
+
+	/* If different, timekeeping will be broken (especially with KVM) */
+	diff |= CHECK(cntfrq, boot, cur, cpu);
+
+	/*
+	 * Even in big.LITTLE, processors should be identical instruction-set
+	 * wise.
+	 */
+	diff |= CHECK(id_aa64isar0, boot, cur, cpu);
+	diff |= CHECK(id_aa64isar1, boot, cur, cpu);
+
+	/*
+	 * Differing PARange support is fine as long as all peripherals and
+	 * memory are mapped within the minimum PARange of all CPUs.
+	 * Linux should not care about secure memory.
+	 * ID_AA64MMFR1 is currently RES0.
+	 */
+	diff |= CHECK_MASK(id_aa64mmfr0, 0xffffffffffff0ff0, boot, cur, cpu);
+	diff |= CHECK(id_aa64mmfr1, boot, cur, cpu);
+
+	/*
+	 * EL3 is not our concern.
+	 * ID_AA64PFR1 is currently RES0.
+	 */
+	diff |= CHECK_MASK(id_aa64pfr0, 0xffffffffffff0fff, boot, cur, cpu);
+	diff |= CHECK(id_aa64pfr1, boot, cur, cpu);
+
+	/*
+	 * If we have AArch32, we care about 32-bit features for compat. These
+	 * registers should be RES0 otherwise.
+	 */
+	diff |= CHECK(id_isar0, boot, cur, cpu);
+	diff |= CHECK(id_isar1, boot, cur, cpu);
+	diff |= CHECK(id_isar2, boot, cur, cpu);
+	diff |= CHECK(id_isar3, boot, cur, cpu);
+	diff |= CHECK(id_isar4, boot, cur, cpu);
+	diff |= CHECK(id_isar5, boot, cur, cpu);
+	diff |= CHECK(id_mmfr0, boot, cur, cpu);
+	diff |= CHECK(id_mmfr1, boot, cur, cpu);
+	diff |= CHECK(id_mmfr2, boot, cur, cpu);
+	diff |= CHECK(id_mmfr3, boot, cur, cpu);
+	diff |= CHECK(id_pfr0, boot, cur, cpu);
+	diff |= CHECK(id_pfr1, boot, cur, cpu);
+
+	/*
+	 * Mismatched CPU features are a recipe for disaster. Don't even
+	 * pretend to support them.
+	 */
+	WARN_TAINT_ONCE(diff, TAINT_CPU_OUT_OF_SPEC,
+			"Unsupported CPU feature variation.");
+}
+
 static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info, unsigned int cpu)
 {
 	info->reg_cntfrq = arch_timer_get_cntfrq();
@@ -89,6 +180,7 @@ void cpuinfo_store_cpu(void)
 	unsigned int cpu = smp_processor_id();
 	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu);
 	__cpuinfo_store_cpu(info, cpu);
+	cpuinfo_sanity_check(info, cpu);
 }
 
 void __init cpuinfo_store_boot_cpu(void)
-- 
1.9.1

[PATCHv3 5/5] arm64: add runtime system sanity checks

[Christopher Covington]

Hi Mark,

On 06/26/2014 11:18 AM, Mark Rutland wrote:
> Unexpected variation in certain system register values across CPUs is an
> indicator of potential problems with a system. The kernel expects CPUs
> to be mostly identical in terms of supported features, even in systems
> with heterogeneous CPUs, with uniform instruction set support being
> critical for the correct operation of userspace.
> 
> To help detect issues early where hardware violates the expectations of
> the kernel, this patch adds simple runtime sanity checks on important ID
> registers in the bring up path of each CPU.
> 
> Where CPUs are fundamentally mismatched, set TAINT_CPU_OUT_OF_SPEC.
> Given that the kernel assumes CPUs are identical feature wise, let's not
> pretend that we expect such configurations to work. Supporting such
> configurations would require massive rework, and hopefully they will
> never exist.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/kernel/cpuinfo.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c

> +	/* If different, timekeeping will be broken (especially with KVM) */
> +	diff |= CHECK(cntfrq, boot, cur, cpu);

You're calling this a "CPU feature" but I thought this was purely a firmware
setting. Does the architecture even allow hardware to program this register?
Additionally, in arch_timer_detect_rate it appears that a device tree setting
takes precedence, but you're not checking that.

> +	/*
> +	 * Mismatched CPU features are a recipe for disaster. Don't even
> +	 * pretend to support them.
> +	 */
> +	WARN_TAINT_ONCE(diff, TAINT_CPU_OUT_OF_SPEC,
> +			"Unsupported CPU feature variation.");
> +}

Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

[PATCHv3 5/5] arm64: add runtime system sanity checks

[Will Deacon]

On Thu, Jun 26, 2014 at 09:29:10PM +0100, Christopher Covington wrote:
> Hi Mark,
> 
> On 06/26/2014 11:18 AM, Mark Rutland wrote:
> > Unexpected variation in certain system register values across CPUs is an
> > indicator of potential problems with a system. The kernel expects CPUs
> > to be mostly identical in terms of supported features, even in systems
> > with heterogeneous CPUs, with uniform instruction set support being
> > critical for the correct operation of userspace.
> > 
> > To help detect issues early where hardware violates the expectations of
> > the kernel, this patch adds simple runtime sanity checks on important ID
> > registers in the bring up path of each CPU.
> > 
> > Where CPUs are fundamentally mismatched, set TAINT_CPU_OUT_OF_SPEC.
> > Given that the kernel assumes CPUs are identical feature wise, let's not
> > pretend that we expect such configurations to work. Supporting such
> > configurations would require massive rework, and hopefully they will
> > never exist.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > ---
> >  arch/arm64/kernel/cpuinfo.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 92 insertions(+)
> > 
> > diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> 
> > +	/* If different, timekeeping will be broken (especially with KVM) */
> > +	diff |= CHECK(cntfrq, boot, cur, cpu);
> 
> You're calling this a "CPU feature" but I thought this was purely a firmware
> setting. Does the architecture even allow hardware to program this register?
> Additionally, in arch_timer_detect_rate it appears that a device tree setting
> takes precedence, but you're not checking that.

KVM virtual machines tend to rely on CNTFRQ being programmed correctly,
since it's not generally possible for the software generating the
device-tree (kvmtool, qemu) to probe the frequency on the host.

Will

[PATCHv3 5/5] arm64: add runtime system sanity checks

[Mark Rutland]

On Thu, Jun 26, 2014 at 09:29:10PM +0100, Christopher Covington wrote:
> Hi Mark,

Hi Chrisopher,

> On 06/26/2014 11:18 AM, Mark Rutland wrote:
> > Unexpected variation in certain system register values across CPUs is an
> > indicator of potential problems with a system. The kernel expects CPUs
> > to be mostly identical in terms of supported features, even in systems
> > with heterogeneous CPUs, with uniform instruction set support being
> > critical for the correct operation of userspace.
> > 
> > To help detect issues early where hardware violates the expectations of
> > the kernel, this patch adds simple runtime sanity checks on important ID
> > registers in the bring up path of each CPU.
> > 
> > Where CPUs are fundamentally mismatched, set TAINT_CPU_OUT_OF_SPEC.
> > Given that the kernel assumes CPUs are identical feature wise, let's not
> > pretend that we expect such configurations to work. Supporting such
> > configurations would require massive rework, and hopefully they will
> > never exist.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > ---
> >  arch/arm64/kernel/cpuinfo.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 92 insertions(+)
> > 
> > diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> 
> > +	/* If different, timekeeping will be broken (especially with KVM) */
> > +	diff |= CHECK(cntfrq, boot, cur, cpu);
> 
> You're calling this a "CPU feature" but I thought this was purely a firmware
> setting. Does the architecture even allow hardware to program this register?

The CNTFRQ register must be set by the firmware/bootloader on each CPU.
While we can argue over whether this makes sense or not, it's simply the
way the architecture works.

Feature registers can vary depending on how more prvileged levels of the
stack have configured the CPU, and/or implementation defined signal out
of reset.

In both cases what we care about its a (mostly) uniform view of
hardware. Perhaps "Feature" is not the correct word, but I'm having
difficulty finding a better way of expressing the requirement.

> Additionally, in arch_timer_detect_rate it appears that a device tree setting
> takes precedence, but you're not checking that.

While that property exists, it's a half-baked workaround and a source of
further problems (e.g. guests seeing the wrong view of time). If
anything I'd like to disable it for arm64; so far systems have been sane
and there's no need to encourage new systems to be broken for no good
reason.

This series should help people to spot and fix these issues at bringup
time so we never have to see them out in the wild.

Thanks,
Mark.

[PATCHv3 1/5] arm64: add MIDR_EL1 field accessors

[Will Deacon]

On Thu, Jun 26, 2014 at 04:18:42PM +0100, Mark Rutland wrote:
> The MIDR_EL1 register is composed of a number of bitfields, and uses of
> the fields has so far involved open-coding of the shifts and masks
> required.
> 
> This patch adds shifts and masks for each of the MIDR_EL1 subfields, and
> also provides accessors built atop of these. Existing uses within
> cputype.h are updated to use these accessors.
> 
> The read_cpuid_part_number macro is modified to return the extracted
> bitfield rather than returning the value in-place with all other fields
> (including revision) masked out, to better match the other accessors.
> As the value is only used in comparison with the *_CPU_PART_* macros
> which are similarly updated, and these values are never exposed to
> userspace, this change should not affect any functionality.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Acked-by: Will Deacon <will.deacon@arm.com>

We need to make sure this doesn't conflict horribly with the missing arm64
hunk from Russell's series in this area. Ideally, we'd take Russell's patch
as part of this series.

Will

[PATCHv3 1/5] arm64: add MIDR_EL1 field accessors

[Russell King - ARM Linux]

On Fri, Jun 27, 2014 at 03:01:56PM +0100, Will Deacon wrote:
> On Thu, Jun 26, 2014 at 04:18:42PM +0100, Mark Rutland wrote:
> > The MIDR_EL1 register is composed of a number of bitfields, and uses of
> > the fields has so far involved open-coding of the shifts and masks
> > required.
> > 
> > This patch adds shifts and masks for each of the MIDR_EL1 subfields, and
> > also provides accessors built atop of these. Existing uses within
> > cputype.h are updated to use these accessors.
> > 
> > The read_cpuid_part_number macro is modified to return the extracted
> > bitfield rather than returning the value in-place with all other fields
> > (including revision) masked out, to better match the other accessors.
> > As the value is only used in comparison with the *_CPU_PART_* macros
> > which are similarly updated, and these values are never exposed to
> > userspace, this change should not affect any functionality.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Acked-by: Will Deacon <will.deacon@arm.com>
> 
> We need to make sure this doesn't conflict horribly with the missing arm64
> hunk from Russell's series in this area. Ideally, we'd take Russell's patch
> as part of this series.

The "missing arm64 hunk" was intentionally dropped as I couldn't see,
and still don't see a justification for it.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCHv3 1/5] arm64: add MIDR_EL1 field accessors

[Will Deacon]

On Fri, Jun 27, 2014 at 03:06:22PM +0100, Russell King - ARM Linux wrote:
> On Fri, Jun 27, 2014 at 03:01:56PM +0100, Will Deacon wrote:
> > On Thu, Jun 26, 2014 at 04:18:42PM +0100, Mark Rutland wrote:
> > > The MIDR_EL1 register is composed of a number of bitfields, and uses of
> > > the fields has so far involved open-coding of the shifts and masks
> > > required.
> > > 
> > > This patch adds shifts and masks for each of the MIDR_EL1 subfields, and
> > > also provides accessors built atop of these. Existing uses within
> > > cputype.h are updated to use these accessors.
> > > 
> > > The read_cpuid_part_number macro is modified to return the extracted
> > > bitfield rather than returning the value in-place with all other fields
> > > (including revision) masked out, to better match the other accessors.
> > > As the value is only used in comparison with the *_CPU_PART_* macros
> > > which are similarly updated, and these values are never exposed to
> > > userspace, this change should not affect any functionality.
> > > 
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Acked-by: Will Deacon <will.deacon@arm.com>
> > 
> > We need to make sure this doesn't conflict horribly with the missing arm64
> > hunk from Russell's series in this area. Ideally, we'd take Russell's patch
> > as part of this series.
> 
> The "missing arm64 hunk" was intentionally dropped as I couldn't see,
> and still don't see a justification for it.

Okey doke then, I just wanted to make sure we didn't get conflicts between
our trees.

Will

[PATCHv3 2/5] arm64: cpuinfo: record cpu system register values

[Will Deacon]

Hi Mark,

On Thu, Jun 26, 2014 at 04:18:43PM +0100, Mark Rutland wrote:
> Several kernel subsystems need to know details about CPU system register
> values, sometimes for CPUs other than that they are executing on. Rather
> than hard-coding system register accesses and cross-calls for these
> cases, this patch adds logic to record various system register values at
> boot-time. This may be used for feature reporting, firmware bug
> detection, etc.
> 
> Separate hooks are added for the boot and hotplug paths to enable
> one-time intialisation and cold/warm boot value mismatch detection in
> later patches.

[...]

> +void cpuinfo_store_cpu(void)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu);

this_cpu_ptr?

> +	__cpuinfo_store_cpu(info);
> +}
> +
> +void __init cpuinfo_store_boot_cpu(void)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu);
> +	__cpuinfo_store_cpu(info);

This looks familiar. Can you just call cpuinfo_store_cpu here, or even move
that code into __cpu_info_store_cpu, which will only work if it runs on the
CPU owning that data anyway.

Will

[PATCHv3 2/5] arm64: cpuinfo: record cpu system register values

[Mark Rutland]

On Fri, Jun 27, 2014 at 04:34:44PM +0100, Will Deacon wrote:
> Hi Mark,

Hi Will,

> On Thu, Jun 26, 2014 at 04:18:43PM +0100, Mark Rutland wrote:
> > Several kernel subsystems need to know details about CPU system register
> > values, sometimes for CPUs other than that they are executing on. Rather
> > than hard-coding system register accesses and cross-calls for these
> > cases, this patch adds logic to record various system register values at
> > boot-time. This may be used for feature reporting, firmware bug
> > detection, etc.
> > 
> > Separate hooks are added for the boot and hotplug paths to enable
> > one-time intialisation and cold/warm boot value mismatch detection in
> > later patches.
> 
> [...]
> 
> > +void cpuinfo_store_cpu(void)
> > +{
> > +	unsigned int cpu = smp_processor_id();
> > +	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu);
> 
> this_cpu_ptr?

Sure.

> 
> > +	__cpuinfo_store_cpu(info);
> > +}
> > +
> > +void __init cpuinfo_store_boot_cpu(void)
> > +{
> > +	unsigned int cpu = smp_processor_id();
> > +	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, cpu);
> > +	__cpuinfo_store_cpu(info);
> 
> This looks familiar. Can you just call cpuinfo_store_cpu here, or even move
> that code into __cpu_info_store_cpu, which will only work if it runs on the
> CPU owning that data anyway.

I can't call cpuinfo_store_cpu() here, as in patch 5 I'll end up doing
erroneous validation of cpu0 against the (not yet initialised) boot cpu
data.

Given the use of this_cpu_ptr, I can get rid of the smp_processor_id()
calls for now, as they're only necessary when reporting to the user. For
patches 4 and 5 I've moved them to cpuinfo_detect_icache_policy() and
cpuinfo_sanity_check().

Thanks,
Mark.

[PATCHv3 5/5] arm64: add runtime system sanity checks

[Christopher Covington]

Hi Mark,

On 06/27/2014 05:56 AM, Mark Rutland wrote:
> On Thu, Jun 26, 2014 at 09:29:10PM +0100, Christopher Covington wrote:
>> Hi Mark,
> 
> Hi Chrisopher,
> 
>> On 06/26/2014 11:18 AM, Mark Rutland wrote:
>>> Unexpected variation in certain system register values across CPUs is an
>>> indicator of potential problems with a system. The kernel expects CPUs
>>> to be mostly identical in terms of supported features, even in systems
>>> with heterogeneous CPUs, with uniform instruction set support being
>>> critical for the correct operation of userspace.
>>>
>>> To help detect issues early where hardware violates the expectations of
>>> the kernel, this patch adds simple runtime sanity checks on important ID
>>> registers in the bring up path of each CPU.
>>>
>>> Where CPUs are fundamentally mismatched, set TAINT_CPU_OUT_OF_SPEC.
>>> Given that the kernel assumes CPUs are identical feature wise, let's not
>>> pretend that we expect such configurations to work. Supporting such
>>> configurations would require massive rework, and hopefully they will
>>> never exist.
>>>
>>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>>> ---
>>>  arch/arm64/kernel/cpuinfo.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 92 insertions(+)
>>>
>>> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
>>
>>> +	/* If different, timekeeping will be broken (especially with KVM) */
>>> +	diff |= CHECK(cntfrq, boot, cur, cpu);
>>
>> You're calling this a "CPU feature" but I thought this was purely a firmware
>> setting. Does the architecture even allow hardware to program this register?
> 
> The CNTFRQ register must be set by the firmware/bootloader on each CPU.
> While we can argue over whether this makes sense or not, it's simply the
> way the architecture works.
> 
> Feature registers can vary depending on how more prvileged levels of the
> stack have configured the CPU, and/or implementation defined signal out
> of reset.

Is this really the general case? It appears to me as though all of registers
you're checking, except for CNTFRQ, are read only at every exception level,
although I haven't checked for indirect writes.

If a field is configurable by software, I don't think TAINT_CPU_OUT_OF_SPEC is
appropriate.

> In both cases what we care about its a (mostly) uniform view of
> hardware. Perhaps "Feature" is not the correct word, but I'm having
> difficulty finding a better way of expressing the requirement.

It's the CPU part of "CPU feature" that I object to. Calling CNTFRQ a firmware
feature would be fine.

>> Additionally, in arch_timer_detect_rate it appears that a device tree setting
>> takes precedence, but you're not checking that.
> 
> While that property exists, it's a half-baked workaround and a source of
> further problems (e.g. guests seeing the wrong view of time). If
> anything I'd like to disable it for arm64; so far systems have been sane
> and there's no need to encourage new systems to be broken for no good
> reason.

Perhaps checking CNTFRQ should be moved there and TAINT_FIRMWARE_WORKAROUND used.

> This series should help people to spot and fix these issues at bringup
> time so we never have to see them out in the wild.

This is excellent.

Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

[PATCHv3 3/5] arm64: cpuinfo: print info for all CPUs

[Ard Biesheuvel]

On 26 June 2014 17:18, Mark Rutland <mark.rutland@arm.com> wrote:
> Currently reading /proc/cpuinfo will result in information being read
> out of the MIDR_EL1 of the current CPU, and the information is not
> associated with any particular logical CPU number.
>
> This is problematic for systems with heterogeneous CPUs (i.e.
> big.LITTLE) where fields will vary across CPUs, and the output will
> differ depending on the executing CPU.
>
> This patch reorganises the code responsible for /proc/cpuinfo to print
> information per-cpu. As hwcaps are expected to be identical across all
> CPUs, this information is printed once.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


> ---
>  arch/arm64/kernel/setup.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index edb146d..b00b7a3 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -448,20 +448,30 @@ static const char *hwcap_str[] = {
>
>  static int c_show(struct seq_file *m, void *v)
>  {
> -       int i;
> +       int c, i;
>
> -       seq_printf(m, "Processor\t: %s rev %d (%s)\n",
> -                  cpu_name, read_cpuid_id() & 15, ELF_PLATFORM);
> +       for_each_online_cpu(c) {
> +               struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, c);
> +               u32 midr = cpuinfo->reg_midr;
>
> -       for_each_online_cpu(i) {
>                 /*
>                  * glibc reads /proc/cpuinfo to determine the number of
>                  * online processors, looking for lines beginning with
>                  * "processor".  Give glibc what it expects.
>                  */
>  #ifdef CONFIG_SMP
> -               seq_printf(m, "processor\t: %d\n", i);
> +               seq_printf(m, "processor\t: %d\n", c);
>  #endif
> +               seq_printf(m, "Type\t\t: %s rev %d (%s)\n",
> +                          cpu_name, MIDR_REVISION(midr), ELF_PLATFORM);
> +               seq_printf(m, "CPU implementer\t: 0x%02x\n",
> +                          MIDR_IMPLEMENTOR(midr));
> +               seq_printf(m, "CPU architecture: AArch64\n");
> +               seq_printf(m, "CPU variant\t: 0x%x\n", MIDR_VARIANT(midr));
> +               seq_printf(m, "CPU part\t: 0x%03x\n", MIDR_PARTNUM(midr));
> +               seq_printf(m, "CPU revision\t: %d\n", MIDR_REVISION(midr));
> +
> +               seq_puts(m, "\n");
>         }
>
>         /* dump out the processor features */
> @@ -470,17 +480,8 @@ static int c_show(struct seq_file *m, void *v)
>         for (i = 0; hwcap_str[i]; i++)
>                 if (elf_hwcap & (1 << i))
>                         seq_printf(m, "%s ", hwcap_str[i]);
> -
> -       seq_printf(m, "\nCPU implementer\t: 0x%02x\n", read_cpuid_id() >> 24);
> -       seq_printf(m, "CPU architecture: AArch64\n");
> -       seq_printf(m, "CPU variant\t: 0x%x\n", (read_cpuid_id() >> 20) & 15);
> -       seq_printf(m, "CPU part\t: 0x%03x\n", (read_cpuid_id() >> 4) & 0xfff);
> -       seq_printf(m, "CPU revision\t: %d\n", read_cpuid_id() & 15);
> -
>         seq_puts(m, "\n");
>
> -       seq_printf(m, "Hardware\t: %s\n", machine_name);
> -
>         return 0;
>  }
>
> --
> 1.9.1
>

[PATCHv3 5/5] arm64: add runtime system sanity checks

[Mark Rutland]

Hi Christopher,

On Fri, Jun 27, 2014 at 05:56:58PM +0100, Christopher Covington wrote:
> Hi Mark,
> 
> On 06/27/2014 05:56 AM, Mark Rutland wrote:
> > On Thu, Jun 26, 2014 at 09:29:10PM +0100, Christopher Covington wrote:
> >> Hi Mark,
> > 
> > Hi Chrisopher,
> > 
> >> On 06/26/2014 11:18 AM, Mark Rutland wrote:
> >>> Unexpected variation in certain system register values across CPUs is an
> >>> indicator of potential problems with a system. The kernel expects CPUs
> >>> to be mostly identical in terms of supported features, even in systems
> >>> with heterogeneous CPUs, with uniform instruction set support being
> >>> critical for the correct operation of userspace.
> >>>
> >>> To help detect issues early where hardware violates the expectations of
> >>> the kernel, this patch adds simple runtime sanity checks on important ID
> >>> registers in the bring up path of each CPU.
> >>>
> >>> Where CPUs are fundamentally mismatched, set TAINT_CPU_OUT_OF_SPEC.
> >>> Given that the kernel assumes CPUs are identical feature wise, let's not
> >>> pretend that we expect such configurations to work. Supporting such
> >>> configurations would require massive rework, and hopefully they will
> >>> never exist.
> >>>
> >>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> >>> ---
> >>>  arch/arm64/kernel/cpuinfo.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 92 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> >>
> >>> +	/* If different, timekeeping will be broken (especially with KVM) */
> >>> +	diff |= CHECK(cntfrq, boot, cur, cpu);
> >>
> >> You're calling this a "CPU feature" but I thought this was purely a firmware
> >> setting. Does the architecture even allow hardware to program this register?
> > 
> > The CNTFRQ register must be set by the firmware/bootloader on each CPU.
> > While we can argue over whether this makes sense or not, it's simply the
> > way the architecture works.
> > 
> > Feature registers can vary depending on how more prvileged levels of the
> > stack have configured the CPU, and/or implementation defined signal out
> > of reset.
> 
> Is this really the general case? It appears to me as though all of registers
> you're checking, except for CNTFRQ, are read only at every exception level,
> although I haven't checked for indirect writes.

There's at least one case caused be indirect writes: a hypervisor might
set HCR_EL2.TDZ, which will cause DCZID_EL0.DZP to read as set. DCZID
also contains a hardware property in the form of DCZID_EL0.BS -- if that
differs then DC ZVA is basically unusable.

For signals out of reset we have precedent with TC2. The Cortex-A7
CTR.IminLine field can be configured by writes to an SCC register to
match that of Cortex-A15. That kind of configuration is obviously
outside of the scope of the architecture and thus difficult to quantify.

> If a field is configurable by software, I don't think
> TAINT_CPU_OUT_OF_SPEC is appropriate.

Perhaps. But we're still left with the CPU (rather than the firmware)
reporting a value it shouldn't.

> > In both cases what we care about its a (mostly) uniform view of
> > hardware. Perhaps "Feature" is not the correct word, but I'm having
> > difficulty finding a better way of expressing the requirement.
> 
> It's the CPU part of "CPU feature" that I object to. Calling CNTFRQ a firmware
> feature would be fine.

I would disagree with calling this a firmware feature as it makes it
sound like the firmware is continuously involved. We could go for a
mouthful and call this a "run-time CPU property", if that's any better?

> >> Additionally, in arch_timer_detect_rate it appears that a device tree setting
> >> takes precedence, but you're not checking that.
> > 
> > While that property exists, it's a half-baked workaround and a source of
> > further problems (e.g. guests seeing the wrong view of time). If
> > anything I'd like to disable it for arm64; so far systems have been sane
> > and there's no need to encourage new systems to be broken for no good
> > reason.
> 
> Perhaps checking CNTFRQ should be moved there and TAINT_FIRMWARE_WORKAROUND used.

For the moment I would like to keep this here so as to enable this
sanity checking ASAP. Plugging it into the generic timer driver is a
little painful because the cp15 and MMIO timer code is intertwined, and
there's a large body of 32-bit systems on which CNTFRQ is not
initialised.

I have looked into putting better checks/warnings into the arch timer
driver, but admittedly I haven't posted those. I'll see about having
another go, I agree it would be good to have checks there.

Cheers,
Mark.