I addressed (most) of the review comments. The printk for mismatching
ucode is still there. And it's still walking all CPUs, but since that
happens in parallel it's not really exponentially longer delay.
There's a check now for mismatching model numbers and the
minimum ucode tracking is disabled for this case.
Also I implemented CPU loading earlier in the CPU notifier
for the microcode driver.
Also this actually fixes the code, it didn't work before for
hotplugged CPUs and didn't update the microcodes there.
-Andi
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 899057b..63e8a71 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: Andi Kleen <[email protected]>
When the model/family don't match force the common microcode to zero.
On such configurations there is no sane way to check for microcode
revisions, so don't even try.
Signed-off-by: Andi Kleen <[email protected]>
---
arch/x86/kernel/cpu/common.c | 26 ++++++++++++++++++++------
1 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index cfc50e0..9642929 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1183,12 +1183,26 @@ __cpuinit void boot_update_min_microcode(struct cpuinfo_x86 *c)
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;
+ } 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;
+ }
+ /* Assume steppings have common microcode version numbers */
+ if (c->x86 != boot_cpu_data.x86 ||
+ c->x86_model != boot_cpu_data.x86_model) {
+ pr_warn("CPU %d model mismatch to boot CPU: %d,%d vs %d,%d\n",
+ smp_processor_id(),
+ boot_cpu_data.x86,
+ boot_cpu_data.x86_model,
+ c->x86,
+ c->x86_model);
+ /* Disable any common microcode checks. */
+ boot_cpu_data.microcode = 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
From: Andi Kleen <[email protected]>
Do microcode updates of resuming or newling plugged CPUs earlier
in CPU_STARTING instead of later when ONLINE. This prevents races
with parallel users who may need a microcode update to avoid some
problem.
Since we cannot request the microcode from udev at this stage,
try to grab the microcode from another CPU. This is also more efficient
because it avoids redundant loads. In addition to that
it avoids the need for separate paths for resume and CPU bootup.
This requires invalidating the microcodes on other CPUs on free.
Each CPU does this in parallel, so it's not a big problem.
When there is no good microcode available the update is delayed
until the update can be requested. In the normal cases it should
be available.
Signed-off-by: Andi Kleen <[email protected]>
---
arch/x86/kernel/microcode_core.c | 65 +++++++++++++++++++++++++------------
arch/x86/kernel/microcode_intel.c | 13 +++++++-
2 files changed, 56 insertions(+), 22 deletions(-)
diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index fbdfc69..f947ef7 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -358,20 +358,7 @@ static void microcode_fini_cpu(int cpu)
uci->valid = 0;
}
-static enum ucode_state microcode_resume_cpu(int cpu)
-{
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
- if (!uci->mc)
- return UCODE_NFOUND;
-
- pr_debug("CPU%d updated upon resume\n", cpu);
- apply_microcode_on_target(cpu);
-
- return UCODE_OK;
-}
-
-static enum ucode_state microcode_init_cpu(int cpu)
+static enum ucode_state microcode_init_cpu_late(int cpu)
{
enum ucode_state ustate;
@@ -392,15 +379,44 @@ static enum ucode_state microcode_init_cpu(int cpu)
return ustate;
}
-static enum ucode_state microcode_update_cpu(int cpu)
+/* Grab ucode from another CPU */
+static void clone_ucode_data(void)
+{
+ int cpu = smp_processor_id();
+ int i;
+
+ for_each_online_cpu (i) {
+ if (ucode_cpu_info[i].mc &&
+ ucode_cpu_info[i].valid &&
+ cpu_data(i).x86 == cpu_data(cpu).x86 &&
+ cpu_data(i).x86_model == cpu_data(cpu).x86_model) {
+ ucode_cpu_info[cpu].mc = ucode_cpu_info[i].mc;
+ break;
+ }
+ }
+}
+
+static enum ucode_state microcode_init_cpu_early(int cpu)
+{
+ clone_ucode_data();
+ /* We can request later when the CPU is online */
+ if (ucode_cpu_info[cpu].mc == NULL)
+ return UCODE_ERROR;
+ if (microcode_ops->collect_cpu_info(cpu, &ucode_cpu_info[cpu].cpu_sig))
+ return UCODE_ERROR;
+ if (microcode_ops->apply_microcode(smp_processor_id()))
+ pr_warn("CPU%d microcode update failed\n", cpu);
+ return UCODE_OK;
+}
+
+static enum ucode_state microcode_update_cpu_late(int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
enum ucode_state ustate;
- if (uci->valid)
- ustate = microcode_resume_cpu(cpu);
- else
- ustate = microcode_init_cpu(cpu);
+ /* Resume already done early */
+ if (!uci->valid)
+ ustate = microcode_init_cpu_late(cpu);
return ustate;
}
@@ -418,7 +434,7 @@ static int mc_device_add(struct device *dev, struct subsys_interface *sif)
if (err)
return err;
- if (microcode_init_cpu(cpu) == UCODE_ERROR)
+ if (microcode_init_cpu_late(cpu) == UCODE_ERROR)
return -EINVAL;
return err;
@@ -468,9 +484,16 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
dev = get_cpu_device(cpu);
switch (action) {
+ case CPU_STARTING:
+ case CPU_STARTING_FROZEN:
+ microcode_init_cpu_early(cpu);
+ break;
+
case CPU_ONLINE:
case CPU_ONLINE_FROZEN:
- microcode_update_cpu(cpu);
+ /* Retry again in case we couldn't request early */
+ if (cpu_data(cpu).microcode < ucode_cpu_info[cpu].cpu_sig.rev)
+ microcode_update_cpu_late(cpu);
case CPU_DOWN_FAILED:
case CPU_DOWN_FAILED_FROZEN:
pr_debug("CPU%d added\n", cpu);
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 0327e2b..899057b 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -329,6 +329,16 @@ static int apply_microcode(int cpu)
return 0;
}
+static void invalidate_microcode(void *data)
+{
+ int i;
+
+ for_each_possible_cpu (i) {
+ if (ucode_cpu_info[i].mc == data)
+ ucode_cpu_info[i].mc = NULL;
+ }
+}
+
static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
int (*get_ucode_data)(void *, const void *, size_t))
{
@@ -391,6 +401,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
}
vfree(uci->mc);
+ invalidate_microcode(uci->mc);
uci->mc = (struct microcode_intel *)new_mc;
pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",
@@ -444,7 +455,7 @@ static void microcode_fini_cpu(int cpu)
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
vfree(uci->mc);
- uci->mc = NULL;
+ invalidate_microcode(uci->mc);
}
static struct microcode_ops microcode_intel_ops = {
--
1.7.7.6
On Wed, 2012-06-13 at 13:20 -0700, Andi Kleen wrote:
> +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)) {
Stephane actually wrote:
"Ok, so to close on this, I tried the 6/6/2012 ucode update on a few
SNB-EP systems.
I got two answers depending on the stepping:
C1 (stepping 6) -> 0x618
C2 (stepping 7) -> 0x70c
So we need to check x86_mask for stepping and adjust the value of
snb_ucode_rev accordingly for model 45."
On Wed, 2012-06-13 at 13:20 -0700, Andi Kleen wrote:
> + for_each_online_cpu (i)
# git grep "for_each_[^(]*(" arch/x86/ | wc -l
315
# git grep "for_each_[^(]* (" arch/x86/ | wc -l
0
> Stephane actually wrote:
>
> "Ok, so to close on this, I tried the 6/6/2012 ucode update on a few
> SNB-EP systems.
>
> I got two answers depending on the stepping:
> C1 (stepping 6) -> 0x618
> C2 (stepping 7) -> 0x70c
>
> So we need to check x86_mask for stepping and adjust the value of
> snb_ucode_rev accordingly for model 45."
not sure i understood your point?
What do you want me to change?
I check different numbers on the different models.
FWIW it works on a Sandy Bridge E and I believe I didn't change
the logic for non E, which Stephane tested.
-Andi
--
[email protected] -- Speaking for myself only
On Wed, 2012-06-13 at 13:20 -0700, Andi Kleen wrote:
> 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 899057b..63e8a71 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;
> }
This would really much better live in common code where it can be
preemptible in some sites. There really is no need for this to be
non-preemptible.
On Wed, 2012-06-13 at 14:34 -0700, Andi Kleen wrote:
> > Stephane actually wrote:
> >
> > "Ok, so to close on this, I tried the 6/6/2012 ucode update on a few
> > SNB-EP systems.
> >
> > I got two answers depending on the stepping:
> > C1 (stepping 6) -> 0x618
> > C2 (stepping 7) -> 0x70c
> >
> > So we need to check x86_mask for stepping and adjust the value of
> > snb_ucode_rev accordingly for model 45."
>
> not sure i understood your point?
> What do you want me to change?
>
> I check different numbers on the different models.
>
> FWIW it works on a Sandy Bridge E and I believe I didn't change
> the logic for non E, which Stephane tested.
Is there a ucode revision for C2 higher than 0x618 but lower than
0x70c ? If so, your code is wrong for it would enable PEBS on that chip.
On Wed, Jun 13, 2012 at 11:34:21PM +0200, Peter Zijlstra wrote:
> On Wed, 2012-06-13 at 13:20 -0700, Andi Kleen wrote:
> > + for_each_online_cpu (i)
>
> # git grep "for_each_[^(]*(" arch/x86/ | wc -l
> 315
> # git grep "for_each_[^(]* (" arch/x86/ | wc -l
> 0
Well, it's a for loop. Do you want me to grep for all for loops.
We've never written for loops without space. Loops without space
do not make any sense to me.
Anyways I'm changing it but under protest.
-Andi
--
[email protected] -- Speaking for myself only
> This would really much better live in common code where it can be
> preemptible in some sites. There really is no need for this to be
> non-preemptible.
Okay I can look. But you're really optimizing the wrong things here.
Even on a 4096 CPU system such a loop is miniscule compared to the
actual cost of the microcode update, which stops the complete CPU
for quite some time.
Basically microcode updates and low latency are incompatible
Don't do it when it hurts.
Unfortunately i'm changing all the callers two patches further,
so it'll be messy too.
-Andi
--
[email protected] -- Speaking for myself only
Peter Zijlstra <[email protected]> writes:
>
> Is there a ucode revision for C2 higher than 0x618 but lower than
> 0x70c ? If so, your code is wrong for it would enable PEBS on that chip.
I was told there's only a single revision per model number that goes up.
That's also the model that other microcode checks in Linux follow.
Not sure what's happening with Stephane's setup.
I get 0x618 on Stepping 6. I'll try to find a C2.
-Andi
--
[email protected] -- Speaking for myself only
* Andi Kleen <[email protected]> wrote:
> On Wed, Jun 13, 2012 at 11:34:21PM +0200, Peter Zijlstra wrote:
> > On Wed, 2012-06-13 at 13:20 -0700, Andi Kleen wrote:
> > > + for_each_online_cpu (i)
> >
> > # git grep "for_each_[^(]*(" arch/x86/ | wc -l
> > 315
> > # git grep "for_each_[^(]* (" arch/x86/ | wc -l
> > 0
>
> Well, it's a for loop. Do you want me to grep for all for
> loops. We've never written for loops without space. Loops
> without space do not make any sense to me.
Good riddance, what Peter showed you is the standard Linux
kernel coding style:
$ git grep "for_each_[^(]*(" kernel/ | wc -l
758
$ git grep "for_each_[^(]* (" kernel/ | wc -l
0
... which standard style almost every single new patch of yours
continues to violate in some trivial way.
We are not going to allow you to mess up kernel code that we
maintain, so please address the review feedback you got from
Peter and don't submit such trivially flawed patches next time
around.
This is the fourth review round and counting, your stubborn
idiocy continues to waste reviewer and maintainer time.
> Anyways I'm changing it but under protest.
Be careful with holding back your breath for too long, it can do
permanent damage.
Thanks,
Ingo
On Wed, Jun 13, 2012 at 01:20:40PM -0700, Andi Kleen wrote:
> +/*
> + * 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);
You still haven't told me what's the use of this printk statement to the
user. Should she update her microcode, buy a new box or go up in flames?
--
Regards/Gruss,
Boris.
On Wed, Jun 13, 2012 at 01:20:39PM -0700, Andi Kleen wrote:
> From: Andi Kleen <[email protected]>
>
> Do microcode updates of resuming or newling plugged CPUs earlier
> in CPU_STARTING instead of later when ONLINE. This prevents races
> with parallel users who may need a microcode update to avoid some
> problem.
>
> Since we cannot request the microcode from udev at this stage,
> try to grab the microcode from another CPU. This is also more efficient
> because it avoids redundant loads. In addition to that
> it avoids the need for separate paths for resume and CPU bootup.
>
> This requires invalidating the microcodes on other CPUs on free.
> Each CPU does this in parallel, so it's not a big problem.
>
> When there is no good microcode available the update is delayed
> until the update can be requested. In the normal cases it should
> be available.
>
> Signed-off-by: Andi Kleen <[email protected]>
> ---
> arch/x86/kernel/microcode_core.c | 65 +++++++++++++++++++++++++------------
> arch/x86/kernel/microcode_intel.c | 13 +++++++-
> 2 files changed, 56 insertions(+), 22 deletions(-)
>
> diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
> index fbdfc69..f947ef7 100644
> --- a/arch/x86/kernel/microcode_core.c
> +++ b/arch/x86/kernel/microcode_core.c
> @@ -358,20 +358,7 @@ static void microcode_fini_cpu(int cpu)
> uci->valid = 0;
> }
>
> -static enum ucode_state microcode_resume_cpu(int cpu)
> -{
> - struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> -
> - if (!uci->mc)
> - return UCODE_NFOUND;
> -
> - pr_debug("CPU%d updated upon resume\n", cpu);
> - apply_microcode_on_target(cpu);
> -
> - return UCODE_OK;
> -}
> -
> -static enum ucode_state microcode_init_cpu(int cpu)
> +static enum ucode_state microcode_init_cpu_late(int cpu)
> {
> enum ucode_state ustate;
>
> @@ -392,15 +379,44 @@ static enum ucode_state microcode_init_cpu(int cpu)
> return ustate;
> }
>
> -static enum ucode_state microcode_update_cpu(int cpu)
> +/* Grab ucode from another CPU */
> +static void clone_ucode_data(void)
> +{
> + int cpu = smp_processor_id();
> + int i;
> +
> + for_each_online_cpu (i) {
> + if (ucode_cpu_info[i].mc &&
> + ucode_cpu_info[i].valid &&
> + cpu_data(i).x86 == cpu_data(cpu).x86 &&
> + cpu_data(i).x86_model == cpu_data(cpu).x86_model) {
> + ucode_cpu_info[cpu].mc = ucode_cpu_info[i].mc;
> + break;
> + }
> + }
> +}
> +
> +static enum ucode_state microcode_init_cpu_early(int cpu)
> +{
> + clone_ucode_data();
> + /* We can request later when the CPU is online */
> + if (ucode_cpu_info[cpu].mc == NULL)
> + return UCODE_ERROR;
> + if (microcode_ops->collect_cpu_info(cpu, &ucode_cpu_info[cpu].cpu_sig))
> + return UCODE_ERROR;
> + if (microcode_ops->apply_microcode(smp_processor_id()))
> + pr_warn("CPU%d microcode update failed\n", cpu);
> + return UCODE_OK;
> +}
This is run only from the notifier and nothing is looking at its return
values. It should be returning void in such cases.
> +
> +static enum ucode_state microcode_update_cpu_late(int cpu)
> {
> struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> enum ucode_state ustate;
>
> - if (uci->valid)
> - ustate = microcode_resume_cpu(cpu);
> - else
> - ustate = microcode_init_cpu(cpu);
> + /* Resume already done early */
> + if (!uci->valid)
> + ustate = microcode_init_cpu_late(cpu);
>
> return ustate;
> }
> @@ -418,7 +434,7 @@ static int mc_device_add(struct device *dev, struct subsys_interface *sif)
> if (err)
> return err;
>
> - if (microcode_init_cpu(cpu) == UCODE_ERROR)
> + if (microcode_init_cpu_late(cpu) == UCODE_ERROR)
> return -EINVAL;
>
> return err;
> @@ -468,9 +484,16 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
>
> dev = get_cpu_device(cpu);
> switch (action) {
> + case CPU_STARTING:
> + case CPU_STARTING_FROZEN:
> + microcode_init_cpu_early(cpu);
> + break;
> +
> case CPU_ONLINE:
> case CPU_ONLINE_FROZEN:
> - microcode_update_cpu(cpu);
> + /* Retry again in case we couldn't request early */
> + if (cpu_data(cpu).microcode < ucode_cpu_info[cpu].cpu_sig.rev)
> + microcode_update_cpu_late(cpu);
This implies newer ucode versions are numerically higher than
what's currently present. And it is probably correct in the 99% of
the cases but it would be more correct IMHO to have "!=" there.
microcode_update_cpu_late takes care of checking the correct ucode
version anyway down the path.
--
Regards/Gruss,
Boris.
On Wed, Jun 13, 2012 at 01:20:40PM -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.
>
> v2: Use boot_cpu_data.microcode to track minimum revision (H. Peter Anvin)
> Signed-off-by: Andi Kleen <[email protected]>
> ---
[ … ]
> +/*
> + * 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;
> + }
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: x86: Track minimum microcode revision globally v2
/home/boris/kernel/.git/rebase-apply/patch:52: trailing whitespace.
{
/home/boris/kernel/.git/rebase-apply/patch:60: trailing whitespace.
smp_processor_id(),
warning: 2 lines add whitespace errors.
--
Regards/Gruss,
Boris.
On Wed, Jun 13, 2012 at 01:20:40PM -0700, Andi Kleen wrote:
> +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);
This needs to be stating explicitly that the ucode version is in hex -
0x%x - as everywhere else in the kernel where we output ucode version.
Ditto for the remaining printks which will end up in the final version
of your patchset.
--
Regards/Gruss,
Boris.
> You still haven't told me what's the use of this printk statement to the
> user. Should she update her microcode, buy a new box or go up in flames?
It gives any kernel developer a hint that something is fishy with the
configuration. For once it will disable the bug workarounds.
-Andi
--
[email protected] -- Speaking for myself only.
On Thu, 14 Jun 2012, Borislav Petkov wrote:
> On Wed, Jun 13, 2012 at 01:20:39PM -0700, Andi Kleen wrote:
> > case CPU_ONLINE:
> > case CPU_ONLINE_FROZEN:
> > - microcode_update_cpu(cpu);
> > + /* Retry again in case we couldn't request early */
> > + if (cpu_data(cpu).microcode < ucode_cpu_info[cpu].cpu_sig.rev)
> > + microcode_update_cpu_late(cpu);
>
> This implies newer ucode versions are numerically higher than
> what's currently present. And it is probably correct in the 99% of
> the cases but it would be more correct IMHO to have "!=" there.
> microcode_update_cpu_late takes care of checking the correct ucode
> version anyway down the path.
Indeed. For Intel, the microcode rev is a *signed* value, and the
intended way for it to work is that you always install negative rev
microcode (used only internally at Intel, I am told), or install
microcode if rev(new microcode) > rev (old microcode) >= 0, and refuse
to install if rev(new microcode) == 0.
It is better to use != and have stricter checks in the vendor-specific
code.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Wed, Jun 13, 2012 at 01:20:42PM -0700, Andi Kleen wrote:
> From: Andi Kleen <[email protected]>
>
> When the model/family don't match force the common microcode to zero.
> On such configurations there is no sane way to check for microcode
> revisions, so don't even try.
>
> Signed-off-by: Andi Kleen <[email protected]>
> ---
> arch/x86/kernel/cpu/common.c | 26 ++++++++++++++++++++------
> 1 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index cfc50e0..9642929 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1183,12 +1183,26 @@ __cpuinit void boot_update_min_microcode(struct cpuinfo_x86 *c)
> 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;
> + } 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;
> + }
> + /* Assume steppings have common microcode version numbers */
> + if (c->x86 != boot_cpu_data.x86 ||
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: x86: Detect model/family mismatches for common microcode revision
/home/boris/kernel/.git/rebase-apply/patch:22: trailing whitespace.
smp_processor_id(),
/home/boris/kernel/.git/rebase-apply/patch:28: trailing whitespace.
if (c->x86 != boot_cpu_data.x86 ||
warning: 2 lines add whitespace errors.
--
Regards/Gruss,
Boris.