| Message ID | 1403795926-17139-6-git-send-email-mark.rutland@arm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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
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.
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)
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(+)