arm64: handle heterogeneous system register values

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 <[email protected]>

Acked-by Will Deacon <[email protected]>

Quoted 15 lines · 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.

Quoted 2 lines

Signed-off-by Mark Rutland <[email protected]>

Acked-by Will Deacon <[email protected]>

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.

Quoted 19 lines · Will Deacon

Quoted 17 lines · 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 <[email protected]>

Acked-by Will Deacon <[email protected]>

Quoted 3 lines

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.

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.

Hi Mark,

Quoted 10 lines

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.

[...]

arch/arm64/kernel/cpuinfo.c line 64

Quoted 4 lines

arch/arm64/kernel/cpuinfo.c new file +4 -0

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

this_cpu_ptr?

arch/arm64/kernel/cpuinfo.c line 72

Quoted 9 lines

arch/arm64/kernel/cpuinfo.c new file +8 -0

+	__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.

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 <[email protected]>

---
 arch/arm64/kernel/setup.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

arch/arm64/kernel/setup.c +15 -15

index edb146d..b00b7a3 100644
@@ -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

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 <[email protected]>

---
 arch/arm64/kernel/cpuinfo.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

Hi Mark,

Quoted 22 lines · 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 <[email protected]>

---
 arch/arm64/kernel/cpuinfo.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

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

arch/arm64/kernel/cpuinfo.c static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info, · line 98

Quoted 2 lines

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

+	/* 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.

Hi Mark,

Quoted 21 lines

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 <[email protected]>

---
 arch/arm64/kernel/cpuinfo.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

arch/arm64/kernel/cpuinfo.c static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info, · line 98

Quoted 4 lines

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

 
+	/* 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.

Quoted 1 line · Christopher Covington

Hi Mark,

Hi Chrisopher,

Quoted 22 lines · Mark Rutland

Quoted 21 lines

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 <[email protected]>

---
 arch/arm64/kernel/cpuinfo.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

Quoted 7 lines

arch/arm64/kernel/cpuinfo.c static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info, · line 98

Quoted 4 lines

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

 
+	/* 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.

Quoted 2 lines

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.

Hi Mark,

Quoted 39 lines · Mark Rutland

Quoted 1 line · Christopher Covington

Hi Mark,

Hi Chrisopher,

Quoted 22 lines · Mark Rutland

Quoted 21 lines

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 <[email protected]>

---
 arch/arm64/kernel/cpuinfo.c | 92 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

Quoted 7 lines

arch/arm64/kernel/cpuinfo.c static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info, · line 98

Quoted 4 lines

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

 
+	/* 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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

Quoted 2 lines

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.