[PATCHv2 0/4] arm64: handle heterogeneous system register values

[PATCHv2 0/4] 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.

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-rc1, 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

Mark Rutland (4):
  arm64: add MIDR_EL1 field accessors
  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       |  48 ++++++++++++
 arch/arm64/include/asm/cputype.h   |  33 ++++++--
 arch/arm64/kernel/Makefile         |   3 +-
 arch/arm64/kernel/cpuinfo.c        | 149 +++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/setup.c          |  46 ++++++------
 arch/arm64/kernel/smp.c            |   6 ++
 7 files changed, 265 insertions(+), 36 deletions(-)
 create mode 100644 arch/arm64/include/asm/cpu.h
 create mode 100644 arch/arm64/kernel/cpuinfo.c

-- 
1.9.1

[PATCHv2 1/4] 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>
---
 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

[PATCHv2 2/4] 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. Additionally the output is
different in format to the 32-bit ARM Linux port, where information is
printed out for each CPU.

This patch adds the necessary infrastructure to log the relevant
registers (currently just MIDR_EL1) and print out the logged
information.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 arch/arm64/include/asm/cpu.h | 30 +++++++++++++++++++++++++++++
 arch/arm64/kernel/Makefile   |  3 ++-
 arch/arm64/kernel/cpuinfo.c  | 31 +++++++++++++++++++++++++++++
 arch/arm64/kernel/setup.c    | 46 +++++++++++++++++++++++---------------------
 arch/arm64/kernel/smp.c      |  6 ++++++
 5 files changed, 93 insertions(+), 23 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..74bf9bb
--- /dev/null
+++ b/arch/arm64/include/asm/cpu.h
@@ -0,0 +1,30 @@
+/*
+ *  arch/arm/include/asm/cpu.h
+ *
+ *  Copyright (C) 2004-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.
+ */
+#ifndef __ASM_ARM64_CPU_H
+#define __ASM_ARM64_CPU_H
+
+#include <linux/percpu.h>
+#include <linux/cpu.h>
+
+/*
+ * Records attributes of an individual CPU.
+ *
+ * This is used to cache data for /proc/cpuinfo.
+ */
+struct cpuinfo_arm64 {
+	struct cpu	cpu;
+	u32		reg_midr;
+};
+
+DECLARE_PER_CPU(struct cpuinfo_arm64, cpu_data);
+
+void cpuinfo_store_cpu(void);
+
+#endif
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..340621d
--- /dev/null
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -0,0 +1,31 @@
+/*
+ * Record CPU attributes for later retrieval
+ *
+ * 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/cpu.h>
+#include <asm/cputype.h>
+
+#include <linux/smp.h>
+
+DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
+
+void cpuinfo_store_cpu(void)
+{
+	int cpu = smp_processor_id();
+	struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, cpu);
+
+	cpuinfo->reg_midr = read_cpuid_id();
+}
+
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 46d1125..ba2b15f 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>
@@ -396,6 +397,7 @@ void __init setup_arch(char **cmdline_p)
 
 	cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
 	cpu_read_bootcpu_ops();
+	cpuinfo_store_cpu();
 #ifdef CONFIG_SMP
 	smp_init_cpus();
 	smp_build_mpidr_hash();
@@ -417,14 +419,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);
 	}
@@ -447,38 +447,40 @@ static const char *hwcap_str[] = {
 
 static int c_show(struct seq_file *m, void *v)
 {
-	int i;
+	int c;
 
-	seq_printf(m, "Processor\t: %s rev %d (%s)\n",
-		   cpu_name, read_cpuid_id() & 15, ELF_PLATFORM);
+	for_each_online_cpu(c) {
+		int i;
+		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);
 
-	/* dump out the processor features */
-	seq_puts(m, "Features\t: ");
+		/* dump out the processor features */
+		seq_puts(m, "Features\t: ");
 
-	for (i = 0; hwcap_str[i]; i++)
-		if (elf_hwcap & (1 << i))
-			seq_printf(m, "%s ", hwcap_str[i]);
+		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_printf(m, "\nCPU 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");
-
-	seq_printf(m, "Hardware\t: %s\n", machine_name);
+		seq_puts(m, "\n");
+	}
 
 	return 0;
 }
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 40f38f4..7c730a6 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>
@@ -162,6 +163,11 @@ asmlinkage void secondary_start_kernel(void)
 	smp_store_cpu_info(cpu);
 
 	/*
+	 * Log the CPU info before it is marked online and might get read.
+	 */
+	cpuinfo_store_cpu();
+
+	/*
 	 * OK, now it's safe to let the boot CPU continue.  Wait for
 	 * the CPU migration code to notice that the CPU is online
 	 * before we continue.
-- 
1.9.1

[PATCHv2 3/4] 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>
---
 arch/arm64/include/asm/cachetype.h | 16 ++++++++++------
 arch/arm64/include/asm/cpu.h       |  1 +
 arch/arm64/kernel/cpuinfo.c        | 28 +++++++++++++++++++++++++++-
 3 files changed, 38 insertions(+), 7 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/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
index 74bf9bb..daca48d 100644
--- a/arch/arm64/include/asm/cpu.h
+++ b/arch/arm64/include/asm/cpu.h
@@ -20,6 +20,7 @@
  */
 struct cpuinfo_arm64 {
 	struct cpu	cpu;
+	u32		reg_ctr;
 	u32		reg_midr;
 };
 
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 340621d..b146148 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -14,18 +14,44 @@
  * 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/cachetype.h>
 #include <asm/cpu.h>
 #include <asm/cputype.h>
 
+#include <linux/bitops.h>
+#include <linux/printk.h>
 #include <linux/smp.h>
 
 DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
 
+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, 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);
+}
+
 void cpuinfo_store_cpu(void)
 {
 	int cpu = smp_processor_id();
 	struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, cpu);
 
+	cpuinfo->reg_ctr = read_cpuid_cachetype();
 	cpuinfo->reg_midr = read_cpuid_id();
-}
 
+	cpuinfo_detect_icache_policy(cpuinfo, cpu);
+}
-- 
1.9.1

[PATCHv2 4/4] 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.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/include/asm/cpu.h | 19 ++++++++-
 arch/arm64/kernel/cpuinfo.c  | 94 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 111 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
index daca48d..8e6c9aa 100644
--- a/arch/arm64/include/asm/cpu.h
+++ b/arch/arm64/include/asm/cpu.h
@@ -16,12 +16,29 @@
 /*
  * Records attributes of an individual CPU.
  *
- * This is used to cache data for /proc/cpuinfo.
+ * This is used to cache data for /proc/cpuinfo and run-time sanity checks.
  */
 struct cpuinfo_arm64 {
 	struct cpu	cpu;
 	u32		reg_ctr;
+	u32		reg_cntfrq;
 	u32		reg_midr;
+
+	u64		reg_id_aa64isar0;
+	u64		reg_id_aa64mmfr0;
+	u64		reg_id_aa64pfr0;
+	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);
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index b146148..5ef96a0 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -1,5 +1,6 @@
 /*
- * Record CPU attributes for later retrieval
+ * Record CPU attributes for later retrieval, and sanity-check that processor
+ * features do not vary unexpectedly.
  *
  * Copyright (C) 2014 ARM Ltd.
  * This program is free software; you can redistribute it and/or modify
@@ -14,6 +15,7 @@
  * 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>
@@ -21,6 +23,7 @@
 #include <linux/bitops.h>
 #include <linux/printk.h>
 #include <linux/smp.h>
+#include <linux/types.h>
 
 DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
 
@@ -45,13 +48,102 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info, int cpu)
 	pr_info("Detected %s I-cache on CPU%d", icache_policy_str[l1ip], cpu);
 }
 
+static void check_reg_mask(char *name, u64 mask, u64 boot, u64 cur, int cpu)
+{
+	if ((boot & mask) == (cur & mask))
+		return;
+
+	pr_warn("SANITY CHECK: Unexpected variation in %s. cpu0: %#016lx, cpu%d: %#016lx\n",
+		name, (unsigned long)boot, cpu, (unsigned long)cur);
+}
+
+#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, (u64)-1, boot, cur, cpu)
+
+/*
+ * Verify that CPUs don't have unexpected differences that will cause problems.
+ */
+void cpuinfo_sanity_check(struct cpuinfo_arm64 *cur)
+{
+	struct cpuinfo_arm64 *boot = &per_cpu(cpu_data, 0);
+	int cpu = smp_processor_id();
+
+	/*
+	 * The kernel can handle differing I-cache policies, but otherwise
+	 * caches should look identical. Userspace JITs will make use of
+	 * *minLine.
+	 */
+	CHECK_MASK(ctr, 0xffff3fff, boot, cur, cpu);
+
+	/* If different, timekeeping will be broken (especially with KVM) */
+	CHECK(cntfrq, boot, cur, cpu);
+
+	/*
+	 * Even in big.LITTLE, processors should be identical instruction-set
+	 * wise.
+	 */
+	CHECK(id_aa64isar0, 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.
+	 */
+	CHECK_MASK(id_aa64mmfr0, 0xffffffffffff0ff0, boot, cur, cpu);
+
+	/*
+	 * EL3 is not our concern, and GIC system register support only matters
+	 * if GICv3 is in use.
+	 */
+	CHECK_MASK(id_aa64pfr0, 0xfffffffff0ff0fff, boot, cur, cpu);
+
+	/*
+	 * If we have AArch32, we care about 32-bit features for compat. These
+	 * registers should be RES0 otherwise.
+	 */
+	CHECK(id_isar0, boot, cur, cpu);
+	CHECK(id_isar1, boot, cur, cpu);
+	CHECK(id_isar2, boot, cur, cpu);
+	CHECK(id_isar3, boot, cur, cpu);
+	CHECK(id_isar4, boot, cur, cpu);
+	CHECK(id_isar5, boot, cur, cpu);
+	CHECK(id_mmfr0, boot, cur, cpu);
+	CHECK(id_mmfr1, boot, cur, cpu);
+	CHECK(id_mmfr2, boot, cur, cpu);
+	CHECK(id_mmfr3, boot, cur, cpu);
+	CHECK(id_pfr0, boot, cur, cpu);
+	CHECK(id_pfr1, boot, cur, cpu);
+}
+
 void cpuinfo_store_cpu(void)
 {
 	int cpu = smp_processor_id();
 	struct cpuinfo_arm64 *cpuinfo = &per_cpu(cpu_data, cpu);
 
+	cpuinfo->reg_cntfrq = arch_timer_get_cntfrq();
 	cpuinfo->reg_ctr = read_cpuid_cachetype();
 	cpuinfo->reg_midr = read_cpuid_id();
 
+	cpuinfo->reg_id_aa64isar0 = read_cpuid(ID_AA64ISAR0_EL1);
+	cpuinfo->reg_id_aa64mmfr0 = read_cpuid(ID_AA64MMFR0_EL1);
+	cpuinfo->reg_id_aa64pfr0 = read_cpuid(ID_AA64PFR0_EL1);
+
+	cpuinfo->reg_id_isar0 = read_cpuid(ID_ISAR0_EL1);
+	cpuinfo->reg_id_isar1 = read_cpuid(ID_ISAR1_EL1);
+	cpuinfo->reg_id_isar2 = read_cpuid(ID_ISAR2_EL1);
+	cpuinfo->reg_id_isar3 = read_cpuid(ID_ISAR3_EL1);
+	cpuinfo->reg_id_isar4 = read_cpuid(ID_ISAR4_EL1);
+	cpuinfo->reg_id_isar5 = read_cpuid(ID_ISAR5_EL1);
+	cpuinfo->reg_id_mmfr0 = read_cpuid(ID_MMFR0_EL1);
+	cpuinfo->reg_id_mmfr1 = read_cpuid(ID_MMFR1_EL1);
+	cpuinfo->reg_id_mmfr2 = read_cpuid(ID_MMFR2_EL1);
+	cpuinfo->reg_id_mmfr3 = read_cpuid(ID_MMFR3_EL1);
+	cpuinfo->reg_id_pfr0 = read_cpuid(ID_PFR0_EL1);
+	cpuinfo->reg_id_pfr1 = read_cpuid(ID_PFR1_EL1);
+
 	cpuinfo_detect_icache_policy(cpuinfo, cpu);
+	cpuinfo_sanity_check(cpuinfo);
 }
-- 
1.9.1

[PATCHv2 2/4] arm64: cpuinfo: print info for all CPUs

[Will Deacon]

On Tue, Jun 17, 2014 at 06:04:32PM +0100, Mark Rutland 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. Additionally the output is
> different in format to the 32-bit ARM Linux port, where information is
> printed out for each CPU.
> 
> This patch adds the necessary infrastructure to log the relevant
> registers (currently just MIDR_EL1) and print out the logged
> information.

[...]

> diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
> new file mode 100644
> index 0000000..74bf9bb
> --- /dev/null
> +++ b/arch/arm64/include/asm/cpu.h
> @@ -0,0 +1,30 @@
> +/*
> + *  arch/arm/include/asm/cpu.h
> + *
> + *  Copyright (C) 2004-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.
> + */
> +#ifndef __ASM_ARM64_CPU_H
> +#define __ASM_ARM64_CPU_H

We're trying to have a consistent style for these guards, so please use
__ASM_CPU_H here instead. That said, does this actually need to be in a
header file? If you move cpuinfo_store_cpu into setup.c, this structure can
be private to that file, no?

Will

[PATCHv2 3/4] arm64: cachetype: report weakest cache policy

[Will Deacon]

On Tue, Jun 17, 2014 at 06:04:33PM +0100, Mark Rutland wrote:
> 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.

Ah, ok, you're growing cpuinfo.c. Ignore my earlier comment about deleting
the header then :)

Will

[PATCHv2 4/4] arm64: add runtime system sanity checks

[Will Deacon]

On Tue, Jun 17, 2014 at 06:04:34PM +0100, 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.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>

[...]

> +/*
> + * Verify that CPUs don't have unexpected differences that will cause problems.
> + */
> +void cpuinfo_sanity_check(struct cpuinfo_arm64 *cur)
> +{
> +	struct cpuinfo_arm64 *boot = &per_cpu(cpu_data, 0);
> +	int cpu = smp_processor_id();

You could just as easily pass in the cpu number here, like you do for
cpuinfo_detect_icache_policy.

Will

[PATCHv2 0/4] arm64: handle heterogeneous system register values

[Will Deacon]

On Tue, Jun 17, 2014 at 06:04:30PM +0100, Mark Rutland wrote:
> 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.

If you address the few minor comments I had, then:

  Acked-by: Will Deacon <will.deacon@arm.com>

for the series.

Cheers,

Will

[PATCHv2 2/4] arm64: cpuinfo: print info for all CPUs

[Ard Biesheuvel]

Hi Mark,

On 17 June 2014 19:04, 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. Additionally the output is
> different in format to the 32-bit ARM Linux port, where information is
> printed out for each CPU.
>
> This patch adds the necessary infrastructure to log the relevant
> registers (currently just MIDR_EL1) and print out the logged
> information.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>  arch/arm64/include/asm/cpu.h | 30 +++++++++++++++++++++++++++++
>  arch/arm64/kernel/Makefile   |  3 ++-
>  arch/arm64/kernel/cpuinfo.c  | 31 +++++++++++++++++++++++++++++
>  arch/arm64/kernel/setup.c    | 46 +++++++++++++++++++++++---------------------
>  arch/arm64/kernel/smp.c      |  6 ++++++
>  5 files changed, 93 insertions(+), 23 deletions(-)
>  create mode 100644 arch/arm64/include/asm/cpu.h
>  create mode 100644 arch/arm64/kernel/cpuinfo.c
>
[...]
> @@ -447,38 +447,40 @@ static const char *hwcap_str[] = {
>
>  static int c_show(struct seq_file *m, void *v)
>  {
> -       int i;
> +       int c;
>
> -       seq_printf(m, "Processor\t: %s rev %d (%s)\n",
> -                  cpu_name, read_cpuid_id() & 15, ELF_PLATFORM);
> +       for_each_online_cpu(c) {
> +               int i;
> +               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);
>
> -       /* dump out the processor features */
> -       seq_puts(m, "Features\t: ");
> +               /* dump out the processor features */
> +               seq_puts(m, "Features\t: ");
>
> -       for (i = 0; hwcap_str[i]; i++)
> -               if (elf_hwcap & (1 << i))
> -                       seq_printf(m, "%s ", hwcap_str[i]);
> +               for (i = 0; hwcap_str[i]; i++)
> +                       if (elf_hwcap & (1 << i))
> +                               seq_printf(m, "%s ", hwcap_str[i]);
>

Same question as first time around: does it really make sense to print
the value of elf_hwcap (whose value depends only on the capabilities
of the boot cpu) for every CPU listed?

Regards,
-- 
Ard.


> -       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_printf(m, "\nCPU 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");
> -
> -       seq_printf(m, "Hardware\t: %s\n", machine_name);
> +               seq_puts(m, "\n");
> +       }
>
>         return 0;
>  }
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 40f38f4..7c730a6 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>
> @@ -162,6 +163,11 @@ asmlinkage void secondary_start_kernel(void)
>         smp_store_cpu_info(cpu);
>
>         /*
> +        * Log the CPU info before it is marked online and might get read.
> +        */
> +       cpuinfo_store_cpu();
> +
> +       /*
>          * OK, now it's safe to let the boot CPU continue.  Wait for
>          * the CPU migration code to notice that the CPU is online
>          * before we continue.
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2 2/4] arm64: cpuinfo: print info for all CPUs

[Mark Rutland]

On Wed, Jun 18, 2014 at 06:15:20PM +0100, Will Deacon wrote:
> On Tue, Jun 17, 2014 at 06:04:32PM +0100, Mark Rutland 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. Additionally the output is
> > different in format to the 32-bit ARM Linux port, where information is
> > printed out for each CPU.
> > 
> > This patch adds the necessary infrastructure to log the relevant
> > registers (currently just MIDR_EL1) and print out the logged
> > information.
> 
> [...]
> 
> > diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
> > new file mode 100644
> > index 0000000..74bf9bb
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/cpu.h
> > @@ -0,0 +1,30 @@
> > +/*
> > + *  arch/arm/include/asm/cpu.h
> > + *
> > + *  Copyright (C) 2004-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.
> > + */
> > +#ifndef __ASM_ARM64_CPU_H
> > +#define __ASM_ARM64_CPU_H
> 
> We're trying to have a consistent style for these guards, so please use
> __ASM_CPU_H here instead.

Done (also added /* __ASM_CPU_H */ to the #endif for consistency).

> That said, does this actually need to be in a header file? If you move
> cpuinfo_store_cpu into setup.c, this structure can be private to that
> file, no?

Per your comments on patch 3 I'll ignore this :)

Mark.

[PATCHv2 2/4] arm64: cpuinfo: print info for all CPUs

[Mark Rutland]

On Wed, Jun 18, 2014 at 07:29:01PM +0100, Ard Biesheuvel wrote:
> Hi Mark,

Hi Ard,

> On 17 June 2014 19:04, 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. Additionally the output is
> > different in format to the 32-bit ARM Linux port, where information is
> > printed out for each CPU.
> >
> > This patch adds the necessary infrastructure to log the relevant
> > registers (currently just MIDR_EL1) and print out the logged
> > information.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> >  arch/arm64/include/asm/cpu.h | 30 +++++++++++++++++++++++++++++
> >  arch/arm64/kernel/Makefile   |  3 ++-
> >  arch/arm64/kernel/cpuinfo.c  | 31 +++++++++++++++++++++++++++++
> >  arch/arm64/kernel/setup.c    | 46 +++++++++++++++++++++++---------------------
> >  arch/arm64/kernel/smp.c      |  6 ++++++
> >  5 files changed, 93 insertions(+), 23 deletions(-)
> >  create mode 100644 arch/arm64/include/asm/cpu.h
> >  create mode 100644 arch/arm64/kernel/cpuinfo.c
> >
> [...]
> > @@ -447,38 +447,40 @@ static const char *hwcap_str[] = {
> >
> >  static int c_show(struct seq_file *m, void *v)
> >  {
> > -       int i;
> > +       int c;
> >
> > -       seq_printf(m, "Processor\t: %s rev %d (%s)\n",
> > -                  cpu_name, read_cpuid_id() & 15, ELF_PLATFORM);
> > +       for_each_online_cpu(c) {
> > +               int i;
> > +               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);
> >
> > -       /* dump out the processor features */
> > -       seq_puts(m, "Features\t: ");
> > +               /* dump out the processor features */
> > +               seq_puts(m, "Features\t: ");
> >
> > -       for (i = 0; hwcap_str[i]; i++)
> > -               if (elf_hwcap & (1 << i))
> > -                       seq_printf(m, "%s ", hwcap_str[i]);
> > +               for (i = 0; hwcap_str[i]; i++)
> > +                       if (elf_hwcap & (1 << i))
> > +                               seq_printf(m, "%s ", hwcap_str[i]);
> >
> 
> Same question as first time around: does it really make sense to print
> the value of elf_hwcap (whose value depends only on the capabilities
> of the boot cpu) for every CPU listed?

Sorry, factoring out the hwcap parsing slipped my mind when I went to
investigate the I-cache policy issue.

I've taken another look, and it's somewhat painful to sort out.

Ideally we'd read all of the cpuinfo data, parse that into hwcap fields,
then copy the boot CPU fields into the globals. Unfortunately
setup_processor happens before we setup the percpu offsets, and some of
the hwcaps are set elsewhere (fpsimd_init and
arch_timer_evtstrm_enable). It's not obvious to me how to handle those,
the timer case should certainly live in the timer driver.

We expect globally uniform hwcaps anyway, the CPUs should be identical
instruction-set wise (and the sanity checks from later in this series
will trigger were this not the case), so is this a problem?

I agree it would be nicer to have accurate per-CPU hwcaps were they to
differ, but that's a case we don't expect to support as far as I am
aware.

Cheers,
Mark.

[PATCHv2 4/4] arm64: add runtime system sanity checks

[Mark Rutland]

On Wed, Jun 18, 2014 at 06:20:36PM +0100, Will Deacon wrote:
> On Tue, Jun 17, 2014 at 06:04:34PM +0100, 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.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> 
> [...]
> 
> > +/*
> > + * Verify that CPUs don't have unexpected differences that will cause problems.
> > + */
> > +void cpuinfo_sanity_check(struct cpuinfo_arm64 *cur)
> > +{
> > +	struct cpuinfo_arm64 *boot = &per_cpu(cpu_data, 0);
> > +	int cpu = smp_processor_id();
> 
> You could just as easily pass in the cpu number here, like you do for
> cpuinfo_detect_icache_policy.

Sure; done.

Mark.

[PATCHv2 2/4] arm64: cpuinfo: print info for all CPUs

[Ard Biesheuvel]

On 19 June 2014 13:31, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jun 18, 2014 at 07:29:01PM +0100, Ard Biesheuvel wrote:
>> Hi Mark,
>
> Hi Ard,
>
>> On 17 June 2014 19:04, 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. Additionally the output is
>> > different in format to the 32-bit ARM Linux port, where information is
>> > printed out for each CPU.
>> >
>> > This patch adds the necessary infrastructure to log the relevant
>> > registers (currently just MIDR_EL1) and print out the logged
>> > information.
>> >
>> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>> > Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> > ---
>> >  arch/arm64/include/asm/cpu.h | 30 +++++++++++++++++++++++++++++
>> >  arch/arm64/kernel/Makefile   |  3 ++-
>> >  arch/arm64/kernel/cpuinfo.c  | 31 +++++++++++++++++++++++++++++
>> >  arch/arm64/kernel/setup.c    | 46 +++++++++++++++++++++++---------------------
>> >  arch/arm64/kernel/smp.c      |  6 ++++++
>> >  5 files changed, 93 insertions(+), 23 deletions(-)
>> >  create mode 100644 arch/arm64/include/asm/cpu.h
>> >  create mode 100644 arch/arm64/kernel/cpuinfo.c
>> >
>> [...]
>> > @@ -447,38 +447,40 @@ static const char *hwcap_str[] = {
>> >
>> >  static int c_show(struct seq_file *m, void *v)
>> >  {
>> > -       int i;
>> > +       int c;
>> >
>> > -       seq_printf(m, "Processor\t: %s rev %d (%s)\n",
>> > -                  cpu_name, read_cpuid_id() & 15, ELF_PLATFORM);
>> > +       for_each_online_cpu(c) {
>> > +               int i;
>> > +               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);
>> >
>> > -       /* dump out the processor features */
>> > -       seq_puts(m, "Features\t: ");
>> > +               /* dump out the processor features */
>> > +               seq_puts(m, "Features\t: ");
>> >
>> > -       for (i = 0; hwcap_str[i]; i++)
>> > -               if (elf_hwcap & (1 << i))
>> > -                       seq_printf(m, "%s ", hwcap_str[i]);
>> > +               for (i = 0; hwcap_str[i]; i++)
>> > +                       if (elf_hwcap & (1 << i))
>> > +                               seq_printf(m, "%s ", hwcap_str[i]);
>> >
>>
>> Same question as first time around: does it really make sense to print
>> the value of elf_hwcap (whose value depends only on the capabilities
>> of the boot cpu) for every CPU listed?
>
> Sorry, factoring out the hwcap parsing slipped my mind when I went to
> investigate the I-cache policy issue.
>
> I've taken another look, and it's somewhat painful to sort out.
>
> Ideally we'd read all of the cpuinfo data, parse that into hwcap fields,
> then copy the boot CPU fields into the globals. Unfortunately
> setup_processor happens before we setup the percpu offsets, and some of
> the hwcaps are set elsewhere (fpsimd_init and
> arch_timer_evtstrm_enable). It's not obvious to me how to handle those,
> the timer case should certainly live in the timer driver.
>
> We expect globally uniform hwcaps anyway, the CPUs should be identical
> instruction-set wise (and the sanity checks from later in this series
> will trigger were this not the case), so is this a problem?
>
> I agree it would be nicer to have accurate per-CPU hwcaps were they to
> differ, but that's a case we don't expect to support as far as I am
> aware.
>

Well, considering that the purpose of this patch is to reveal minor
differences between CPUs, it will be confusing at the least if you
print elf_hwcap derived from CPU#0 for all other CPUs. So why not move
the capabilties line out of the loop? I know you prefer it inside the
loop for parity with other archs but IMO that only makes sense if the
information presented is 100% accurate.

-- 
Ard.