From: Andi Kleen <[email protected]>
For bug workarounds depending on microcode revisions we need to
know the minimum microcode revision shared by all CPUs.
This patch adds infrastructure to track this.
Every time microcode is updated we update a global variable for
the minimum microcode revision of all the CPUs in the system.
At boot time we use the lowest available microcode (and warn
if they are inconsistent)
At CPU hotplug or S3 resume time there is a short race window
where something might run on the CPUs but before the microcode
update notifier runs. For the current workarounds that need this
this is acceptable and shouldn't be a problem.
Only tested on Intel CPUs, but should work for AMD too.
v2: Use boot_cpu_data.microcode to track minimum revision (H. Peter Anvin)
Signed-off-by: Andi Kleen <[email protected]>
---
arch/x86/include/asm/processor.h | 2 +
arch/x86/kernel/cpu/amd.c | 2 +
arch/x86/kernel/cpu/common.c | 38 +++++++++++++++++++++++++++++++++++++
arch/x86/kernel/cpu/cpu.h | 3 ++
arch/x86/kernel/cpu/intel.c | 2 +
arch/x86/kernel/microcode_amd.c | 1 +
arch/x86/kernel/microcode_intel.c | 1 +
7 files changed, 49 insertions(+), 0 deletions(-)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 39bc577..bc0d361 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -984,4 +984,6 @@ bool set_pm_idle_to_default(void);
void stop_this_cpu(void *dummy);
+void update_min_microcode(struct cpuinfo_x86 *c);
+
#endif /* _ASM_X86_PROCESSOR_H */
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 146bb62..e3f9937 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -684,6 +684,8 @@ static void __cpuinit init_amd(struct cpuinfo_x86 *c)
}
rdmsr_safe(MSR_AMD64_PATCH_LEVEL, &c->microcode, &dummy);
+
+ boot_update_min_microcode(c);
}
#ifdef CONFIG_X86_32
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 6b9333b..cfc50e0 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1168,6 +1168,44 @@ static void dbg_restore_debug_regs(void)
#define dbg_restore_debug_regs()
#endif /* ! CONFIG_KGDB */
+
+/*
+ * Track the minimum global microcode version. On early bootup assume
+ * the BIOS set all CPUs to the same revision. If that's not the case
+ * some code may be already running assuming the newer revision, but
+ * there's not much we can do about that (but it's unlikely to be
+ * problem in early bootup)
+ */
+__cpuinit void boot_update_min_microcode(struct cpuinfo_x86 *c)
+{
+ static int boot_min_microcode;
+
+ if (!boot_min_microcode) {
+ boot_min_microcode = c->microcode;
+ boot_cpu_data.microcode = c->microcode;
+ } else if (c->microcode < boot_min_microcode) {
+ pr_warn("CPU %d has lower microcode revision %x at boot than boot CPU (%x)\n",
+ smp_processor_id(),
+ c->microcode,
+ boot_min_microcode);
+ boot_cpu_data.microcode = c->microcode;
+ }
+}
+
+void update_min_microcode(struct cpuinfo_x86 *c)
+{
+ int i;
+
+ for_each_online_cpu (i)
+ if (cpu_data(i).microcode < c->microcode)
+ return;
+ if (boot_cpu_data.microcode != c->microcode) {
+ boot_cpu_data.microcode = c->microcode;
+ pr_info("Minimum microcode revision updated to %x\n", c->microcode);
+ }
+}
+EXPORT_SYMBOL_GPL(update_min_microcode);
+
/*
* cpu_init() initializes state that is per-CPU. Some data is already
* initialized (naturally) in the bootstrap process, such as the GDT
diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
index 8bacc78..7f92040 100644
--- a/arch/x86/kernel/cpu/cpu.h
+++ b/arch/x86/kernel/cpu/cpu.h
@@ -34,4 +34,7 @@ extern const struct cpu_dev *const __x86_cpu_dev_start[],
extern void get_cpu_cap(struct cpuinfo_x86 *c);
extern void cpu_detect_cache_sizes(struct cpuinfo_x86 *c);
+
+extern void boot_update_min_microcode(struct cpuinfo_x86 *c);
+
#endif /* ARCH_X86_CPU_H */
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 3e6ff6c..d48a13c 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -54,6 +54,8 @@ static void __cpuinit early_init_intel(struct cpuinfo_x86 *c)
/* Required by the SDM */
sync_core();
rdmsr(MSR_IA32_UCODE_REV, lower_word, c->microcode);
+
+ boot_update_min_microcode(c);
}
/*
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 8a2ce8f..b589c7a 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -215,6 +215,7 @@ static int apply_microcode_amd(int cpu)
pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
uci->cpu_sig.rev = rev;
c->microcode = rev;
+ update_min_microcode(c);
return 0;
}
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 0327e2b..50afbb9 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -326,6 +326,7 @@ static int apply_microcode(int cpu)
uci->cpu_sig.rev = val[1];
c->microcode = val[1];
+ update_min_microcode(c);
return 0;
}
--
1.7.7.6
From: Stephane Eranian <[email protected]>
[AK: Updated version of Stephane's patch, now using the new global
tracking microcode number and with the correct microcode revision
for SandyBridge-E*. Also use pr_warn_once and checkpatch fixes.]
This patch checks the microcode version before disabling
PEBS on SandyBridge model 42 (desktop, mobile), and 45 (SNB-EP).
PEBS was disabled for both models due to an erratum.
A workaround is implemented by micro-code 0x28. This patch checks
the microcode version and disables PEBS support if version < 0x28.
The check is done each time a PEBS event is created and NOT at boot
time because the micro-code update may only be done after the kernel
has booted.
Go to downloadcenter.intel.com to download microcode updates.
Need microcode update 6/6/2012 or later.
v2: Was Stephane's old revision
v3: Use boot_cpu_data.microcode (H. Peter Anvin)
Signed-off-by: Stephane Eranian <[email protected]>
Signed-off-by: Andi Kleen <[email protected]>
---
arch/x86/kernel/cpu/perf_event_intel.c | 37 +++++++++++++++++++++++--------
1 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index 187c294..102d153 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -13,6 +13,7 @@
#include <asm/hardirq.h>
#include <asm/apic.h>
+#include <asm/processor.h>
#include "perf_event.h"
@@ -1392,6 +1393,25 @@ static void intel_pebs_aliases_snb(struct perf_event *event)
}
}
+static int check_pebs_quirks(void)
+{
+ int model = cpu_data(smp_processor_id()).x86_model;
+
+ /* do not have PEBS to begin with */
+ if (!x86_pmu.pebs)
+ return 0;
+
+ /*
+ * check ucode version for SNB, SNB-EP
+ */
+ if ((model == 42 && boot_cpu_data.microcode < 0x28) ||
+ (model == 45 && boot_cpu_data.microcode < 0x618)) {
+ pr_warn_once("SandyBridge PEBS unavailable due to CPU erratum, update microcode\n");
+ return -ENOTSUPP;
+ }
+ return 0;
+}
+
static int intel_pmu_hw_config(struct perf_event *event)
{
int ret = x86_pmu_hw_config(event);
@@ -1399,8 +1419,13 @@ static int intel_pmu_hw_config(struct perf_event *event)
if (ret)
return ret;
- if (event->attr.precise_ip && x86_pmu.pebs_aliases)
- x86_pmu.pebs_aliases(event);
+ if (event->attr.precise_ip) {
+ if (check_pebs_quirks())
+ return -ENOTSUPP;
+
+ if (x86_pmu.pebs_aliases)
+ x86_pmu.pebs_aliases(event);
+ }
if (intel_pmu_needs_lbr_smpl(event)) {
ret = intel_pmu_setup_lbr_filter(event);
@@ -1712,13 +1737,6 @@ static __init void intel_clovertown_quirk(void)
x86_pmu.pebs_constraints = NULL;
}
-static __init void intel_sandybridge_quirk(void)
-{
- printk(KERN_WARNING "PEBS disabled due to CPU errata.\n");
- x86_pmu.pebs = 0;
- x86_pmu.pebs_constraints = NULL;
-}
-
static const struct { int id; char *name; } intel_arch_events_map[] __initconst = {
{ PERF_COUNT_HW_CPU_CYCLES, "cpu cycles" },
{ PERF_COUNT_HW_INSTRUCTIONS, "instructions" },
@@ -1910,7 +1928,6 @@ __init int intel_pmu_init(void)
case 42: /* SandyBridge */
case 45: /* SandyBridge, "Romely-EP" */
- x86_add_quirk(intel_sandybridge_quirk);
case 58: /* IvyBridge */
memcpy(hw_cache_event_ids, snb_hw_cache_event_ids,
sizeof(hw_cache_event_ids));
--
1.7.7.6
On Tue, Jun 12, 2012 at 02:25:49PM -0700, Andi Kleen wrote:
> From: Andi Kleen <[email protected]>
>
> For bug workarounds depending on microcode revisions we need to
> know the minimum microcode revision shared by all CPUs.
>
> This patch adds infrastructure to track this.
>
> Every time microcode is updated we update a global variable for
> the minimum microcode revision of all the CPUs in the system.
> At boot time we use the lowest available microcode (and warn
> if they are inconsistent)
>
> At CPU hotplug or S3 resume time there is a short race window
> where something might run on the CPUs but before the microcode
> update notifier runs. For the current workarounds that need this
> this is acceptable and shouldn't be a problem.
>
> Only tested on Intel CPUs, but should work for AMD too.
Yeah, will test this one soonish.
> v2: Use boot_cpu_data.microcode to track minimum revision (H. Peter Anvin)
> Signed-off-by: Andi Kleen <[email protected]>
> ---
> arch/x86/include/asm/processor.h | 2 +
> arch/x86/kernel/cpu/amd.c | 2 +
> arch/x86/kernel/cpu/common.c | 38 +++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/cpu/cpu.h | 3 ++
> arch/x86/kernel/cpu/intel.c | 2 +
> arch/x86/kernel/microcode_amd.c | 1 +
> arch/x86/kernel/microcode_intel.c | 1 +
> 7 files changed, 49 insertions(+), 0 deletions(-)
[ … ]
> +/*
> + * Track the minimum global microcode version. On early bootup assume
> + * the BIOS set all CPUs to the same revision. If that's not the case
> + * some code may be already running assuming the newer revision, but
> + * there's not much we can do about that (but it's unlikely to be
> + * problem in early bootup)
> + */
> +__cpuinit void boot_update_min_microcode(struct cpuinfo_x86 *c)
> +{
> + static int boot_min_microcode;
> +
> + if (!boot_min_microcode) {
> + boot_min_microcode = c->microcode;
> + boot_cpu_data.microcode = c->microcode;
> + } else if (c->microcode < boot_min_microcode) {
> + pr_warn("CPU %d has lower microcode revision %x at boot than boot CPU (%x)\n",
> + smp_processor_id(),
> + c->microcode,
> + boot_min_microcode);
> + boot_cpu_data.microcode = c->microcode;
Ok, is it only me or is this boot_min_microcode superfluous?
IOW, you can only use boot_cpu_data.microcode instead and drop
boot_min_microcode.
--
Regards/Gruss,
Boris.
> Ok, is it only me or is this boot_min_microcode superfluous?
> IOW, you can only use boot_cpu_data.microcode instead and drop
> boot_min_microcode.
boot_cpu_data.microcode contains a copy of the original microcode,
so we couldn't detect the boot cpu case. In theory could hard code CPU
#0 is boot cpu or so,
but I prefer to track it with the separate variable.
-Andi
On Tue, Jun 12, 2012 at 03:09:25PM -0700, Andi Kleen wrote:
>
> >Ok, is it only me or is this boot_min_microcode superfluous?
> >IOW, you can only use boot_cpu_data.microcode instead and drop
> >boot_min_microcode.
>
> boot_cpu_data.microcode contains a copy of the original microcode,
> so we couldn't detect the boot cpu case. In theory could hard code
> CPU #0 is boot cpu or so,
> but I prefer to track it with the separate variable.
Ok, this begs the next question then: why do we need to say that some AP
has a lower ucode version than the BSP?
--
Regards/Gruss,
Boris.
On Wed, Jun 13, 2012 at 12:19:31AM +0200, Borislav Petkov wrote:
> On Tue, Jun 12, 2012 at 03:09:25PM -0700, Andi Kleen wrote:
> >
> > >Ok, is it only me or is this boot_min_microcode superfluous?
> > >IOW, you can only use boot_cpu_data.microcode instead and drop
> > >boot_min_microcode.
> >
> > boot_cpu_data.microcode contains a copy of the original microcode,
> > so we couldn't detect the boot cpu case. In theory could hard code
> > CPU #0 is boot cpu or so,
> > but I prefer to track it with the separate variable.
>
> Ok, this begs the next question then: why do we need to say that some AP
> has a lower ucode version than the BSP?
I would consider that a BIOS bug.
-Andi
On Wed, Jun 13, 2012 at 12:53:44AM +0200, Andi Kleen wrote:
> I would consider that a BIOS bug.
Ok, but does the user need to know this? If it is of only informative
nature, he can find out each core's ucode version from /proc/cpuinfo so
why do we need to issue this at boot?
--
Regards/Gruss,
Boris.
On Tue, 2012-06-12 at 14:25 -0700, Andi Kleen wrote:
> +void update_min_microcode(struct cpuinfo_x86 *c)
> +{
> + int i;
> +
> + for_each_online_cpu (i)
Superfluous whitespace
> + if (cpu_data(i).microcode < c->microcode)
> + return;
That needs {}
> + if (boot_cpu_data.microcode != c->microcode) {
> + boot_cpu_data.microcode = c->microcode;
> + pr_info("Minimum microcode revision updated to %x\n", c->microcode);
> + }
> +}
> +EXPORT_SYMBOL_GPL(update_min_microcode);
> +
> /*
> * cpu_init() initializes state that is per-CPU. Some data is already
> * initialized (naturally) in the bootstrap process, such as the GDT
> diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
> index 8a2ce8f..b589c7a 100644
> --- a/arch/x86/kernel/microcode_amd.c
> +++ b/arch/x86/kernel/microcode_amd.c
> @@ -215,6 +215,7 @@ static int apply_microcode_amd(int cpu)
> pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
> uci->cpu_sig.rev = rev;
> c->microcode = rev;
> + update_min_microcode(c);
>
> return 0;
> }
> diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
> index 0327e2b..50afbb9 100644
> --- a/arch/x86/kernel/microcode_intel.c
> +++ b/arch/x86/kernel/microcode_intel.c
> @@ -326,6 +326,7 @@ static int apply_microcode(int cpu)
> uci->cpu_sig.rev = val[1];
> c->microcode = val[1];
>
> + update_min_microcode(c);
> return 0;
> }
Doing it here means doing the for_each_cpu thing with preempt/irqs
disabled, that's not funny.
Also this is still a O(n^2) proposition.. so how is this better than the
notifier thing I had?
We should just kill reload_store() dead, and do a notifier per system
update, that gives sane semantics and avoids all the O(n^2) nonsense.
On Tue, 2012-06-12 at 14:25 -0700, Andi Kleen wrote:
> At CPU hotplug or S3 resume time there is a short race window
> where something might run on the CPUs but before the microcode
> update notifier runs. For the current workarounds that need this
> this is acceptable and shouldn't be a problem.
I'm not convinced,.. the CPU_ONLINE notifier stuff runs pretty late, by
then its already running normal userspace.
We really should do the ucode update from CPU_STARTING.
On Wed, Jun 13, 2012 at 10:49:52AM +0200, Peter Zijlstra wrote:
> On Tue, 2012-06-12 at 14:25 -0700, Andi Kleen wrote:
> > +void update_min_microcode(struct cpuinfo_x86 *c)
> > +{
> > + int i;
> > +
> > + for_each_online_cpu (i)
>
> Superfluous whitespace
>
> > + if (cpu_data(i).microcode < c->microcode)
> > + return;
>
> That needs {}
You must be following a different code style guide than the Linux one.
> > c->microcode = val[1];
> >
> > + update_min_microcode(c);
> > return 0;
> > }
>
> Doing it here means doing the for_each_cpu thing with preempt/irqs
> disabled, that's not funny.
Well a ucode update is a really slow operation anyways. And the loop
gets stopped at the first mismatch. So it'll never be n^2 and in most
cases much faster. Generally the loop should be several orders
of magnitude less than the actual cost of the update, even on large
systems.
> Also this is still a O(n^2) proposition.. so how is this better than the
> notifier thing I had?
Simpler at least.
I don't know why people love notifiers, they are a "COME FROM" and make
every code who uses them a mess.
As for CPU_STARTING don't know how complicated it would be. I suppose
it could be done as a follow up.
-Andi
--
[email protected] -- Speaking for myself only