perf/x86: check ucode before disabling PEBS on SandyBridge

arch/x86/kernel/cpu/perf_event_intel.c +21 -0

+static int check_pebs_quirks(void)
+{
+       int uversion = cpu_data(smp_processor_id()).microcode;
+       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 || model == 45) && uversion < 0x28) {
+               pr_warn("SandyBridge PEBS unavailable due to CPU erratum, "
+                       " update microcode (was 0x%x, needs at least 0x28).\n",
+                       uversion);
+               return -ENOTSUPP;
+       }
+       return 0;
+}
+
 static int intel_pmu_hw_config(struct perf_event *event)
 {
        int ret = x86_pmu_hw_config(event);

arch/x86/kernel/cpu/perf_event_intel.c +4 -0

+
+               if (x86_pmu.pebs_aliases)
+                       x86_pmu.pebs_aliases(event);
+       }
 
        if (intel_pmu_needs_lbr_smpl(event)) {
                ret = intel_pmu_setup_lbr_filter(event);

@@ -1714,13 +1741,6 @@ static __init void intel_clovertown_quirk(void)
        x86_pmu.pebs_constraints = NULL;
 }

Quoted 28 lines · Stephane Eranian

arch/x86/kernel/cpu/perf_event_intel.c +21 -0

+static int check_pebs_quirks(void)
+{
+       int uversion = cpu_data(smp_processor_id()).microcode;
+       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 || model == 45) && uversion < 0x28) {
+               pr_warn("SandyBridge PEBS unavailable due to CPU erratum, "
+                       " update microcode (was 0x%x, needs at least 0x28).\n",
+                       uversion);
+               return -ENOTSUPP;
+       }
+       return 0;
+}
+

 static int intel_pmu_hw_config(struct perf_event *event) {
        int ret = x86_pmu_hw_config(event);

arch/x86/kernel/cpu/perf_event_intel.c +0 -0

@@ -1401,8 +1422,14 @@ static int intel_pmu_hw_config(struct perf_event *event)
         if (ret)
                 return ret;
 

arch/x86/kernel/cpu/perf_event_intel.c static int intel_pmu_hw_config(struct perf_event *event) · line 1428

Quoted 6 lines

arch/x86/kernel/cpu/perf_event_intel.c +4 -2

-       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;

This will only warn about the PEBS issue once you try and use a PEBS counter. Shouldn't we do this on boot? IOW. put check_pebs_quirks() inside the existing quirk code instead of here?

Quoted 10 lines

arch/x86/kernel/cpu/perf_event_intel.c +4 -0

+
+               if (x86_pmu.pebs_aliases)
+                       x86_pmu.pebs_aliases(event);
+       }
 
         if (intel_pmu_needs_lbr_smpl(event)) {
                 ret = intel_pmu_setup_lbr_filter(event);

@@ -1714,13 +1741,6 @@ static __init void intel_clovertown_quirk(void)
         x86_pmu.pebs_constraints = NULL;
  }

Quoted 4 lines

This will only warn about the PEBS issue once you try and use a PEBS counter. Shouldn't we do this on boot? IOW. put check_pebs_quirks() inside the existing quirk code instead of here?

The warning could also be done on boot. But the check has to be done
when the event is created.

The other thing is that as it is now, if you
get an error, you check dmesg and it's obvious what is wrong. With the boot warning, you'd have to look back all the way to the early boot messages. Whatever you prefer, I am fine.

Quoted 6 lines · Stephane Eranian

Quoted 4 lines

This will only warn about the PEBS issue once you try and use a PEBS counter. Shouldn't we do this on boot? IOW. put check_pebs_quirks() inside the existing quirk code instead of here?

The warning could also be done on boot. But the check has to be done when the event is created.

Is this because of the ucode loader not having done its thing yet etc..?

Quoted 4 lines

The other thing is that as it is now, if you
get an error, you check dmesg and it's obvious what is wrong. With the boot warning, you'd have to look back all the way to the early boot messages. Whatever you prefer, I am fine.

We could do both I guess. But the event creation one needs a rate limiter.

Quoted 4 lines

Quoted 2 lines

The warning could also be done on boot. But the check has to be done
when the event is created.

Is this because of the ucode loader not having done its thing yet etc..?

Quoted 4 lines · Stephane Eranian

Quoted 4 lines

Quoted 2 lines

The warning could also be done on boot. But the check has to be done
when the event is created.

Is this because of the ucode loader not having done its thing yet etc..?

Ah, its because you can update ucode at runtime of course.

A quick look at the ucode loader doesn't show it having a notifier either, so then yeah we need to check on every event creation :/

Or we could put a hook in the ucode loader.

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines · Stephane Eranian

Quoted 4 lines

Quoted 2 lines

The warning could also be done on boot. But the check has to be done
when the event is created.

Is this because of the ucode loader not having done its thing yet etc..?

Ah, its because you can update ucode at runtime of course.

Quoted 5 lines

A quick look at the ucode loader doesn't show it having a notifier either, so then yeah we need to check on every event creation :/

Or we could put a hook in the ucode loader.

I'd really suggest the latter - I doubt this will be our only ucode dependent quirk, going forward ...

Quoted 14 lines · Ingo Molnar

Quoted 12 lines · Peter Zijlstra

Quoted 4 lines · Stephane Eranian

Quoted 4 lines

Quoted 2 lines

The warning could also be done on boot. But the check has to be done when the event is created.

Is this because of the ucode loader not having done its thing yet etc..?

Ah, its because you can update ucode at runtime of course.

A quick look at the ucode loader doesn't show it having a notifier either, so then yeah we need to check on every event creation :/

Quoted 4 lines

Quoted 1 line

Or we could put a hook in the ucode loader.

I'd really suggest the latter - I doubt this will be our only ucode dependent quirk, going forward ...

Yeah, am in the middle of writing that..

Quoted 4 lines

Quoted 1 line

Or we could put a hook in the ucode loader.

I'd really suggest the latter - I doubt this will be our only ucode dependent quirk, going forward ...

Yeah, am in the middle of writing that..

Quoted 5 lines · Peter Zijlstra

Quoted 4 lines

Quoted 1 line

Or we could put a hook in the ucode loader.

I'd really suggest the latter - I doubt this will be our only ucode dependent quirk, going forward ...

Quoted 1 line

Yeah, am in the middle of writing that..

OK so the micro-code stuff is a complete trainwreck.

The very worst is that it does per-cpu micro-code updates, not machine wide. This results in it being able to have different revisions on different cpus. This in turn makes the below O(n^2) :/

The biggest problem is finding when the minimum revision changes, at best this is a n log n sorting problem due to the per-cpu setup, but I couldn't be arsed to implement a tree or anything fancy since it all stinks anyway.

Why do we have per-cpu firmware anyway?

arch/x86/kernel/cpu/perf_event_intel.c +40 -0

index 5ec146c..bde86cf 100644
@@ -15,6 +15,7 @@
 
 #include <asm/hardirq.h>
 #include <asm/apic.h>
+#include <asm/microcode.h>
 
 #include "perf_event.h"
 

@@ -1714,11 +1715,67 @@ static __init void intel_clovertown_quirk(void)
 	x86_pmu.pebs_constraints = NULL;
 }
 
+static const u32 snb_ucode_rev = 0x28;
+
+static void intel_snb_verify_ucode(void)
+{
+	u32 rev = UINT_MAX;
+	int pebs_broken = 0;
+	int cpu;
+
+	get_online_cpus();
+	/*
+	 * Because the microcode loader is bloody stupid and allows different
+	 * revisions per cpu and does strictly per-cpu loading, we now have to
+	 * check all cpus to determine the minimally installed revision.
+	 *
+	 * This makes updating the microcode O(n^2) in the number of CPUs :/
+	 */
+	for_each_online_cpu(cpu)
+		rev = min(cpu_data(cpu).microcode, rev);
+	put_online_cpus();
+
+	pebs_broken = (rev < snb_ucode_rev);
+
+	if (pebs_broken == x86_pmu.pebs_broken)
+		return;
+
+	/*
+	 * Serialized by the microcode lock..
+	 */
+	if (x86_pmu.pebs_broken) {
+		pr_info("PEBS enabled due to micro-code update\n");
+		x86_pmu.pebs_broken = 0;
+	} else {
+		pr_info("PEBS disabled due to CPU errata, "
+			"please upgrade micro-code to at least %x (current: %x)\n",
+			snb_ucode_rev, rev);
+		x86_pmu.pebs_broken = 1;
+	}
+}
+

Reportedly, there are some obscure systems which need different microcode versions per CPU:

http://lkml.indiana.edu/hypermail/linux/kernel/1105.3/01010.html

Quoted 4 lines

The biggest problem is finding when the minimum revision changes, at best this is a n log n sorting problem due to the per-cpu setup, but I couldn't be arsed to implement a tree or anything fancy since it all stinks anyway.

I know. Can't you just iterate over all CPUs and collect the lowest ucode version? Provided, of course, newer microcode versions means a higher version number.

Quoted 4 lines · Borislav Petkov

Reportedly, there are some obscure systems which need different microcode versions per CPU:

http://lkml.indiana.edu/hypermail/linux/kernel/1105.3/01010.html

*groan*,.. ok.

Quoted 8 lines

Quoted 4 lines

The biggest problem is finding when the minimum revision changes, at best this is a n log n sorting problem due to the per-cpu setup, but I couldn't be arsed to implement a tree or anything fancy since it all stinks anyway.

I know. Can't you just iterate over all CPUs and collect the lowest ucode version? Provided, of course, newer microcode versions means a higher version number.

That's what I do.. but that's O(n) per cpu, so if you update all cpus, that's O(n^2).

You don't know when userspace is done updating the 'last' cpu, so you have to scan all cpus for every cpu.

I'm sure the SGI people with their silly CPU counts aren't too thrilled to have this.

have a variable which gets initialized to the number of all CPUs and each time ->apply_microcode() finishes by returning 0, we decrement it once.

Hmm, I'm probably missing some obscure case.

Quoted 6 lines · Borislav Petkov

have a variable which gets initialized to the number of all CPUs and each time ->apply_microcode() finishes by returning 0, we decrement it once.

Hmm, I'm probably missing some obscure case.

Since its all per-cpu sysfs muck, userspace could update a random subsets of cpus.. leaving us hanging.

The 'bestestet' idea I came up with is doing the verify thing I have from a delayed work -- say 1 second into the future. That way, when there's lots of cpus they all try and enqueue the one work, which at the end executes only once, provided the entire update scan took less than the second.

Quoted 10 lines · Peter Zijlstra

Quoted 6 lines · Borislav Petkov

have a variable which gets initialized to the number of all CPUs and each time ->apply_microcode() finishes by returning 0, we decrement it once.

Hmm, I'm probably missing some obscure case.

Since its all per-cpu sysfs muck, userspace could update a random subsets of cpus.. leaving us hanging.

I'm afraid I don't understand - when you modprobe microcode.ko, it goes and loads /lib/firmware/amd-ucode/microcode_amd.bin (in the AMD case) on each CPU when the driver gets regged through subsys_interface_register().

It calls ->add_dev() on each CPU - this should be guaranteed because it uses the cpu_subsys from drivers/base/cpu.c which onlines all CPUs, I'd assume.

So, I'd say that once subsys_interface_register() returns, we have updated ucode on all CPUs, if successful...

We probably could run the notifier at that moment, before we do put_online_cpus().

Quoted 5 lines

The 'bestestet' idea I came up with is doing the verify thing I have from a delayed work -- say 1 second into the future. That way, when there's lots of cpus they all try and enqueue the one work, which at the end executes only once, provided the entire update scan took less than the second.

You're saying, you want the last CPU that gets to update its microcode gets to also run the delayed work...? Probably, I'd assume ucode update on a single CPU takes less than a second IIUC.

I was thinking about reload_store(), that seems to only reload ucode for a single cpu.

Quoted 9 lines

Quoted 5 lines

The 'bestestet' idea I came up with is doing the verify thing I have from a delayed work -- say 1 second into the future. That way, when there's lots of cpus they all try and enqueue the one work, which at the end executes only once, provided the entire update scan took less than the second.

You're saying, you want the last CPU that gets to update its microcode gets to also run the delayed work...? Probably, I'd assume ucode update on a single CPU takes less than a second IIUC.

Nah.. it'll probably be the first. But it doesn't matter which cpu does it. So the idea was:

static void intel_snb_verify_work(struct work_struct *work)

{
	/* do the verify thing.. */
}

static DECLARE_DELAYED_WORK(intel_snb_delayed_work, intel_snb_verify_ucode);

static int intel_snb_ucode_notifier(struct notifier_block *self,
unsigned long action, void *_uci)

{
        /*
         * Since ucode cannot be down-graded, and no future ucode revision
         * is known to break PEBS again, we're ok with MICROCODE_CAN_UPDATE.
         */

        if (action == MICROCODE_UPDATED)
		schedule_delayed_work(&intel_snb_delayed_work, HZ);

        return NOTIFY_DONE;
}

Thus it will queue the delayed work when the work isn't already queued for execution. Resulting in the work only happening once a second (at most).

In these cases, you already have PEBS fixed but since you're not loading any ucode, you won't run intel_snb_verify_ucode().

So, you want to check for PEBS twice (and for all other features fixed by ucode and tested for earlier than the ucode loader, for that matter):

* once when perf inits

* twice, a bit later when the ucode loader has loaded something from userspace and the notifier corrects it.

static __init void intel_sandybridge_quirk(void)
{

arch/x86/kernel/cpu/perf_event_intel.c static __init void intel_clovertown_quirk(void) · line 1779

Quoted 9 lines

static __init void intel_sandybridge_quirk(void)
{

arch/x86/kernel/cpu/perf_event_intel.c +6 -0

+       intel_snb_verify_ucode();
+       /*
+        * we're still single threaded, so while there's a hole here,
+        * you can't trigger it.
+        */
+       microcode_notifier(intel_snb_ucode_notifier);
 }

Like that you mean?

Provided, of course, newer microcode versions means a higher version number.

Quoted 2 lines · Borislav Petkov

Provided, of course, newer microcode versions means a higher version number.

Fair enough, I assumed this.. is this true?

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · Borislav Petkov

Provided, of course, newer microcode versions means a higher version number.

Fair enough, I assumed this.. is this true?

I would think so, otherwise it gets even messier!

Quoted 8 lines · Stephane Eranian

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · Borislav Petkov

Provided, of course, newer microcode versions means a higher version number.

Fair enough, I assumed this.. is this true?

I would think so, otherwise it gets even messier!

Need to get Intel folk to say something here, hpa?

Quoted 10 lines · Peter Zijlstra

Quoted 6 lines · Peter Zijlstra

Quoted 4 lines

Quoted 1 line

Or we could put a hook in the ucode loader.

I'd really suggest the latter - I doubt this will be our only ucode dependent quirk, going forward ...

Yeah, am in the middle of writing that..

OK so the micro-code stuff is a complete trainwreck.

Quoted 3 lines

The very worst is that it does per-cpu micro-code updates, not machine wide. This results in it being able to have different revisions on different cpus. This in turn makes the below O(n^2) :/

Reportedly, there are some obscure systems which need different microcode versions per CPU:

http://lkml.indiana.edu/hypermail/linux/kernel/1105.3/01010.html

Quoted 1 line

Why do we have per-cpu firmware anyway?

See above.

Actually, the deal here is that you could have received microcode already from BIOS and you won't have to necessarily load ucode from userspace with the Linux ucode loader.

Btw, this is why we wanted to load ucode as early as possible but there's no progress on the whole thing right now...

Quoted 4 lines

Reportedly, there are some obscure systems which need different microcode versions per CPU:

http://lkml.indiana.edu/hypermail/linux/kernel/1105.3/01010.html

I wrote that email you linked above. All it means is that you can have more than one microcode *type* per system, because you can have more than one processor type per system.

The lack of per-system microcode handling is, as far as I am concerned, a rather annoying misfeature, plain and simple. A per-core interface that lets you have mismatched microcode across cores of the same type (i.e. that take the same microcode) is, IMO, a bottomless pit of pain and suffering which should diediediediedie as soon as possible.

It just means the firmware core has to do a per-cpu job, not that we should have mismatched microcodes across the same type of cpus, or even that we should have the required per-cpu handling of firmware exposed to the rest of the kernel and the worst of it all, exposed to userspace.

Quoted 3 lines

Actually, the deal here is that you could have received microcode already from BIOS and you won't have to necessarily load ucode from userspace with the Linux ucode loader.

Or from an enhanced bootloader. Yes.

Quoted 2 lines

Btw, this is why we wanted to load ucode as early as possible but there's no progress on the whole thing right now...

Which is a pity.

Quoted 5 lines · Peter Zijlstra

Quoted 4 lines

Quoted 1 line

Or we could put a hook in the ucode loader.

I'd really suggest the latter - I doubt this will be our only ucode dependent quirk, going forward ...

Quoted 1 line

Yeah, am in the middle of writing that..

OK so the micro-code stuff is a complete trainwreck.

The very worst is that it does per-cpu micro-code updates, not machine wide. This results in it being able to have different revisions on different cpus. This in turn makes the below O(n^2) :/

The biggest problem is finding when the minimum revision changes, at best this is a n log n sorting problem due to the per-cpu setup, but I couldn't be arsed to implement a tree or anything fancy since it all stinks anyway.

Why do we have per-cpu firmware anyway?

Anyway, I have the below in two patches, but I thought to first post the entire mess since if anybody has a better idea I'm all ears.

It does work though..

---

 arch/x86/include/asm/microcode.h       |   12 ++++++
 arch/x86/kernel/cpu/perf_event.c       |   14 +++----
 arch/x86/kernel/cpu/perf_event.h       |    2 +-

 arch/x86/kernel/cpu/perf_event_intel.c |   63 ++++++++++++++++++++++++++++++-- arch/x86/kernel/microcode_amd.c        |    1 + arch/x86/kernel/microcode_core.c       |   53 +++++++++++++++++++++++---- arch/x86/kernel/microcode_intel.c      |    1 + arch/x86/kernel/setup.c                |    5 +++

 8 files changed, 132 insertions(+), 19 deletions(-)

arch/x86/include/asm/microcode.h +1 -0

index 4ebe157..1cd2dc5 100644
@@ -32,6 +32,7 @@ struct microcode_ops {
 
  struct ucode_cpu_info {
        struct cpu_signature    cpu_sig;
+       int                     new_rev;
        int                     valid;
        void                    *mc;
  };

arch/x86/include/asm/microcode.h +9 -0

+#define MICROCODE_CAN_UPDATE   0x01
+#define MICROCODE_UPDATED      0x02
+
+#define microcode_notifier(fn)                                         \
+do {                                                                   \
+       static struct notifier_block fn##_nb ={ .notifier_call = fn };  \
+       blocking_notifier_chain_register(&microcode_notifier, &fn##_nb);\
+} while (0)
+
  #endif /* _ASM_X86_MICROCODE_H */

arch/x86/kernel/cpu/perf_event.c +7 -7

index 000a474..d3ef86c1 100644
@@ -377,7 +377,7 @@ int x86_pmu_hw_config(struct perf_event *event)
                int precise = 0;
 
                /* Support for constant skid */
-               if (x86_pmu.pebs_active) {
+               if (x86_pmu.pebs_active && !x86_pmu.pebs_broken) {
                        precise++;
 
                        /* Support for IP fixup */

@@ -1677,9 +1677,9 @@ static struct pmu pmu = {
        .pmu_enable             = x86_pmu_enable,
        .pmu_disable            = x86_pmu_disable,
 
-       .attr_groups    = x86_pmu_attr_groups,
+       .attr_groups            = x86_pmu_attr_groups,
 
-       .event_init     = x86_pmu_event_init,
+       .event_init             = x86_pmu_event_init,
 
        .add                    = x86_pmu_add,
        .del                    = x86_pmu_del,

@@ -1687,11 +1687,11 @@ static struct pmu pmu = {
        .stop                   = x86_pmu_stop,
        .read                   = x86_pmu_read,
 
-       .start_txn      = x86_pmu_start_txn,
-       .cancel_txn     = x86_pmu_cancel_txn,
-       .commit_txn     = x86_pmu_commit_txn,
+       .start_txn              = x86_pmu_start_txn,
+       .cancel_txn             = x86_pmu_cancel_txn,
+       .commit_txn             = x86_pmu_commit_txn,
 
-       .event_idx      = x86_pmu_event_idx,
+       .event_idx              = x86_pmu_event_idx,
        .flush_branch_stack     = x86_pmu_flush_branch_stack,
  };
 

arch/x86/kernel/cpu/perf_event.h +1 -1

index 3df3de9..a5ecfe8 100644
@@ -373,7 +373,7 @@ struct x86_pmu {
         * Intel DebugStore bits
         */
        int             bts, pebs;
-       int             bts_active, pebs_active;
+       int             bts_active, pebs_active, pebs_broken;
        int             pebs_record_size;
        void            (*drain_pebs)(struct pt_regs *regs);
        struct event_constraint *pebs_constraints;

arch/x86/kernel/cpu/perf_event_intel.c +1 -0

index 5ec146c..bde86cf 100644
@@ -15,6 +15,7 @@
 
  #include <asm/hardirq.h>
  #include <asm/apic.h>
+#include <asm/microcode.h>
 
  #include "perf_event.h"
 

arch/x86/kernel/cpu/perf_event_intel.c +57 -3

+static void intel_snb_verify_ucode(void)
+{
+       u32 rev = UINT_MAX;
+       int pebs_broken = 0;
+       int cpu;
+
+       get_online_cpus();
+       /*
+        * Because the microcode loader is bloody stupid and allows different
+        * revisions per cpu and does strictly per-cpu loading, we now have to
+        * check all cpus to determine the minimally installed revision.
+        *
+        * This makes updating the microcode O(n^2) in the number of CPUs :/
+        */
+       for_each_online_cpu(cpu)
+               rev = min(cpu_data(cpu).microcode, rev);
+       put_online_cpus();
+
+       pebs_broken = (rev < snb_ucode_rev);
+
+       if (pebs_broken == x86_pmu.pebs_broken)
+               return;
+
+       /*
+        * Serialized by the microcode lock..
+        */
+       if (x86_pmu.pebs_broken) {
+               pr_info("PEBS enabled due to micro-code update\n");
+               x86_pmu.pebs_broken = 0;
+       } else {
+               pr_info("PEBS disabled due to CPU errata, "
+                       "please upgrade micro-code to at least %x (current: %x)\n",
+                       snb_ucode_rev, rev);
+               x86_pmu.pebs_broken = 1;
+       }
+}
+
+static int intel_snb_ucode_notifier(struct notifier_block *self,
+                                  unsigned long action, void *_uci)
+{
+       /*
+        * Since ucode cannot be down-graded, and no future ucode revision
+        * is known to break PEBS again, we're ok with MICROCODE_CAN_UPDATE.
+        */
+
+       if (action == MICROCODE_UPDATED)
+               intel_snb_verify_ucode();
+
+       return NOTIFY_DONE;
+}
+
  static __init void intel_sandybridge_quirk(void)
  {
-       pr_warn("PEBS disabled due to CPU errata\n");
-       x86_pmu.pebs = 0;
-       x86_pmu.pebs_constraints = NULL;
+       intel_snb_verify_ucode();
+       /*
+        * we're still single threaded, so while there's a hole here,
+        * you can't trigger it.
+        */
+       microcode_notifier(intel_snb_ucode_notifier);
  }
 
  static const struct { int id; char *name; } intel_arch_events_map[] __initconst = {

arch/x86/kernel/microcode_amd.c +0 -0

index 8a2ce8f..265831e 100644
@@ -294,6 +294,7 @@ generic_load_microcode(int cpu, const u8 *data, size_t size)
        }
 

 out_ok:
+       uci->new_rev = new_rev;
       uci->mc = new_mc;
       state = UCODE_OK;
       pr_debug("CPU%d update ucode (0x%08x -> 0x%08x)\n",

arch/x86/kernel/microcode_core.c +45 -8

index fbdfc69..a6cad81 100644
@@ -167,15 +167,43 @@ static void apply_microcode_local(void *arg)
        ctx->err = microcode_ops->apply_microcode(smp_processor_id());
  }
 
+/*
+ * Call the notifier chain before we update to verify we can indeed
+ * update to the desired revision.
+ */
+
+static int microcode_notifier_check(struct ucode_cpu_info *uci)
+{
+       int ret = blocking_notifier_call_chain(&microcode_notifier,
+                       MICROCODE_CAN_UPDATE, uci);
+       return notifier_to_errno(ret);
+}
+
+/*
+ * Call the notifier chain after we've updated to
+ */
+
+static void microcode_notifier_done(void)
+{
+       blocking_notifier_call_chain(&microcode_notifier, MICROCODE_UPDATED, NULL);
+}
+
  static int apply_microcode_on_target(int cpu)
  {
        struct apply_microcode_ctx ctx = { .err = 0 };
        int ret;
 
+       ret = microcode_notifier_check(ucode_cpu_info + cpu);
+       if (ret)
+               return ret;
+
        ret = smp_call_function_single(cpu, apply_microcode_local, &ctx, 1);
        if (!ret)
                ret = ctx.err;
 
+       if (!ret)
+               microcode_notifier_done();
+
        return ret;
  }
 

@@ -196,8 +224,11 @@ static int do_microcode_update(const void __user *buf, size_t size)
                if (ustate == UCODE_ERROR) {
                        error = -1;
                        break;
-               } else if (ustate == UCODE_OK)
-                       apply_microcode_on_target(cpu);
+               } else if (ustate == UCODE_OK) {
+                       error = apply_microcode_on_target(cpu);
+                       if (error)
+                               break;
+               }
        }
 
        return error;

@@ -282,11 +313,12 @@ static int reload_for_cpu(int cpu)
                enum ucode_state ustate;
 
                ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev);
-               if (ustate == UCODE_OK)
-                       apply_microcode_on_target(cpu);
-               else
+               if (ustate == UCODE_OK) {
+                       err = apply_microcode_on_target(cpu);
+               } else {
                        if (ustate == UCODE_ERROR)
                                err = -EINVAL;
+               }
        }
        mutex_unlock(&microcode_mutex);
 

@@ -361,14 +393,15 @@ static void microcode_fini_cpu(int cpu)
  static enum ucode_state microcode_resume_cpu(int cpu)
  {
        struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+       int err;
 
        if (!uci->mc)
                return UCODE_NFOUND;
 
        pr_debug("CPU%d updated upon resume\n", cpu);
-       apply_microcode_on_target(cpu);
+       err = apply_microcode_on_target(cpu);
 
-       return UCODE_OK;
+       return !err ? UCODE_OK : UCODE_ERROR;
  }
 
  static enum ucode_state microcode_init_cpu(int cpu)

@@ -385,8 +418,12 @@ static enum ucode_state microcode_init_cpu(int cpu)
        ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev);
 
        if (ustate == UCODE_OK) {
+               int err;
+
                pr_debug("CPU%d updated upon init\n", cpu);
-               apply_microcode_on_target(cpu);
+               err = apply_microcode_on_target(cpu);
+               if (err)
+                       ustate = UCODE_ERROR;
        }
 
        return ustate;

arch/x86/kernel/microcode_intel.c +1 -0

index 0327e2b..6b87bd5 100644
@@ -391,6 +391,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
        }
 
        vfree(uci->mc);
+       uci->new_rev = new_rev;
        uci->mc = (struct microcode_intel *)new_mc;
 
        pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",

arch/x86/kernel/setup.c +5 -0

index f4b9b80..98b5b5c 100644
@@ -68,6 +68,7 @@
  #include <linux/percpu.h>
  #include <linux/crash_dump.h>
  #include <linux/tboot.h>
+#include <linux/notifier.h>
 
  #include <video/edid.h>
 

@@ -205,6 +206,10 @@ struct cpuinfo_x86 boot_cpu_data __read_mostly = {
  EXPORT_SYMBOL(boot_cpu_data);
  #endif
 
+/*
+ * Lives here because the microcode stuff is modular.
+ */
+struct atomic_notifier_head microcode_notifier;
 
  #if !defined(CONFIG_X86_PAE) || defined(CONFIG_X86_64)
  unsigned long mmu_cr4_features;

Quoted 4 lines

The very worst is that it does per-cpu micro-code updates, not machine wide. This results in it being able to have different revisions on different cpus. This in turn makes the below O(n^2) :/

But it's not like this is a frequent operation either...

arch/x86/include/asm/microcode.h static inline struct microcode_ops * __init init_amd_microcode(void) · line 68

Quoted 13 lines

---

arch/x86/include/asm/microcode.h +2 -0

index 4ebe157..1cd2dc5 100644
@@ -63,4 +64,15 @@ static inline struct microcode_ops * __init init_amd_microcode(void)
 static inline void __exit exit_amd_microcode(void) {}
 #endif
 
+extern struct blocking_notifier_head microcode_notifier;
+

That is a problem because microcode can be compiled as a module. When I tried compiling your patch I got undefined for this notifier because I have microcode update as a module...

arch/x86/include/asm/microcode.h static inline struct microcode_ops * __init init_amd_microcode(void) · line 78

Quoted 10 lines

arch/x86/include/asm/microcode.h +9 -0

+#define MICROCODE_CAN_UPDATE   0x01
+#define MICROCODE_UPDATED      0x02
+
+#define microcode_notifier(fn)                                         \
+do {                                                                   \
+       static struct notifier_block fn##_nb ={ .notifier_call = fn };  \
+       blocking_notifier_chain_register(&microcode_notifier, &fn##_nb);\
+} while (0)
+
 #endif /* _ASM_X86_MICROCODE_H */

Quoted 12 lines

arch/x86/kernel/setup.c +1 -0

index f4b9b80..98b5b5c 100644
@@ -68,6 +68,7 @@
 #include <linux/percpu.h>
 #include <linux/crash_dump.h>
 #include <linux/tboot.h>
+#include <linux/notifier.h>
 
 #include <video/edid.h>
 

arch/x86/kernel/setup.c struct cpuinfo_x86 boot_cpu_data __read_mostly = { · line 212

Quoted 8 lines

arch/x86/kernel/setup.c +4 -0

@@ -205,6 +206,10 @@ struct cpuinfo_x86 boot_cpu_data __read_mostly = {
 EXPORT_SYMBOL(boot_cpu_data);
 #endif
 
+/*
+ * Lives here because the microcode stuff is modular.
+ */
+struct atomic_notifier_head microcode_notifier;

arch/x86/kernel/setup.c struct cpuinfo_x86 boot_cpu_data __read_mostly = { · line 215

Quoted 2 lines

arch/x86/kernel/setup.c +0 -0

 #if !defined(CONFIG_X86_PAE) || defined(CONFIG_X86_64)
 unsigned long mmu_cr4_features;

Quoted 4 lines

The very worst is that it does per-cpu micro-code updates, not machine wide. This results in it being able to have different revisions on different cpus. This in turn makes the below O(n^2) :/

But it's not like this is a frequent operation either...

Review

Quoted 2 lines

(quoted diff) +2 -0

+extern struct blocking_notifier_head microcode_notifier;
+

That is a problem because microcode can be compiled as a module. When I tried compiling your patch I got undefined for this notifier because I have microcode update as a module...

Review

Quoted 2 lines

(quoted diff) +2 -0

+extern struct blocking_notifier_head microcode_notifier;
+

That is a problem because microcode can be compiled as a module. When I tried compiling your patch I got undefined for this notifier because I have microcode update as a module...

Quoted 5 lines · Stephane Eranian

Review

Quoted 2 lines

(quoted diff) +2 -0

+extern struct blocking_notifier_head microcode_notifier;
+

That is a problem because microcode can be compiled as a module. When I tried compiling your patch I got undefined for this notifier because I have microcode update as a module...

Ah, no, setup.c needs an EXPORT_SYMBOL_GPL(microcode_notifier);. Then it all builds again.

Review

Quoted 2 lines

(quoted diff) +2 -0

+static const u32 snb_ucode_rev = 0x28;
+

That needs to be a per CPU model value. It
is not the same for SNB vs. SNB-EP. On EP
it may even depends on stepping.

Quoted 6 lines · Stephane Eranian

Review

Quoted 2 lines

(quoted diff) +2 -0

+static const u32 snb_ucode_rev = 0x28;
+

That needs to be a per CPU model value. It
is not the same for SNB vs. SNB-EP. On EP
it may even depends on stepping.

Yeah, you said, easiest is removing the const and putting some assignments to the thing somewhere in our model switch.

Do you know what values for what chip we should use?

Quoted 6 lines · Stephane Eranian

Review

Quoted 2 lines

(quoted diff) +2 -0

+static const u32 snb_ucode_rev = 0x28;
+

That needs to be a per CPU model value. It
is not the same for SNB vs. SNB-EP. On EP
it may even depends on stepping.

Yeah, you said, easiest is removing the const and putting some assignments to the thing somewhere in our model switch.

Do you know what values for what chip we should use?

Quoted 6 lines · Peter Zijlstra

Quoted 4 lines

Quoted 1 line

Or we could put a hook in the ucode loader.

I'd really suggest the latter - I doubt this will be our only ucode dependent quirk, going forward ...

Yeah, am in the middle of writing that..

OK so the micro-code stuff is a complete trainwreck.

The very worst is that it does per-cpu micro-code updates, not machine wide. This results in it being able to have different revisions on different cpus. This in turn makes the below O(n^2) :/

The biggest problem is finding when the minimum revision changes, at best this is a n log n sorting problem due to the per-cpu setup, but I couldn't be arsed to implement a tree or anything fancy since it all stinks anyway.

Why do we have per-cpu firmware anyway?

Anyway, I have the below in two patches, but I thought to first post the entire mess since if anybody has a better idea I'm all ears.

It does work though..

---

 arch/x86/include/asm/microcode.h       |   12 ++++++
 arch/x86/kernel/cpu/perf_event.c       |   14 +++----
 arch/x86/kernel/cpu/perf_event.h       |    2 +-

 arch/x86/kernel/cpu/perf_event_intel.c |   63 ++++++++++++++++++++++++++++++-- arch/x86/kernel/microcode_amd.c        |    1 + arch/x86/kernel/microcode_core.c       |   53 +++++++++++++++++++++++---- arch/x86/kernel/microcode_intel.c      |    1 + arch/x86/kernel/setup.c                |    5 +++

 8 files changed, 132 insertions(+), 19 deletions(-)

arch/x86/include/asm/microcode.h +12 -0

index 4ebe157..1cd2dc5 100644
@@ -32,6 +32,7 @@ struct microcode_ops {
 
  struct ucode_cpu_info {
        struct cpu_signature    cpu_sig;
+       int                     new_rev;
        int                     valid;
        void                    *mc;
  };

@@ -63,4 +64,15 @@ static inline struct microcode_ops * __init init_amd_microcode(void)
  static inline void __exit exit_amd_microcode(void) {}
  #endif
 
+extern struct blocking_notifier_head microcode_notifier;
+
+#define MICROCODE_CAN_UPDATE   0x01
+#define MICROCODE_UPDATED      0x02
+
+#define microcode_notifier(fn)                                         \
+do {                                                                   \
+       static struct notifier_block fn##_nb ={ .notifier_call = fn };  \
+       blocking_notifier_chain_register(&microcode_notifier, &fn##_nb);\
+} while (0)
+
  #endif /* _ASM_X86_MICROCODE_H */

arch/x86/kernel/cpu/perf_event.c +7 -7

index 000a474..d3ef86c1 100644
@@ -377,7 +377,7 @@ int x86_pmu_hw_config(struct perf_event *event)
                int precise = 0;
 
                /* Support for constant skid */
-               if (x86_pmu.pebs_active) {
+               if (x86_pmu.pebs_active && !x86_pmu.pebs_broken) {
                        precise++;
 
                        /* Support for IP fixup */

@@ -1677,9 +1677,9 @@ static struct pmu pmu = {
        .pmu_enable             = x86_pmu_enable,
        .pmu_disable            = x86_pmu_disable,
 
-       .attr_groups    = x86_pmu_attr_groups,
+       .attr_groups            = x86_pmu_attr_groups,
 
-       .event_init     = x86_pmu_event_init,
+       .event_init             = x86_pmu_event_init,
 
        .add                    = x86_pmu_add,
        .del                    = x86_pmu_del,

@@ -1687,11 +1687,11 @@ static struct pmu pmu = {
        .stop                   = x86_pmu_stop,
        .read                   = x86_pmu_read,
 
-       .start_txn      = x86_pmu_start_txn,
-       .cancel_txn     = x86_pmu_cancel_txn,
-       .commit_txn     = x86_pmu_commit_txn,
+       .start_txn              = x86_pmu_start_txn,
+       .cancel_txn             = x86_pmu_cancel_txn,
+       .commit_txn             = x86_pmu_commit_txn,
 
-       .event_idx      = x86_pmu_event_idx,
+       .event_idx              = x86_pmu_event_idx,
        .flush_branch_stack     = x86_pmu_flush_branch_stack,
  };
 

arch/x86/kernel/cpu/perf_event.h +1 -1

index 3df3de9..a5ecfe8 100644
@@ -373,7 +373,7 @@ struct x86_pmu {
         * Intel DebugStore bits
         */
        int             bts, pebs;
-       int             bts_active, pebs_active;
+       int             bts_active, pebs_active, pebs_broken;
        int             pebs_record_size;
        void            (*drain_pebs)(struct pt_regs *regs);
        struct event_constraint *pebs_constraints;

arch/x86/kernel/cpu/perf_event_intel.c +60 -3

index 5ec146c..bde86cf 100644
@@ -15,6 +15,7 @@
 
  #include <asm/hardirq.h>
  #include <asm/apic.h>
+#include <asm/microcode.h>
 
  #include "perf_event.h"
 

@@ -1714,11 +1715,67 @@ static __init void intel_clovertown_quirk(void)
        x86_pmu.pebs_constraints = NULL;
  }
 
+static const u32 snb_ucode_rev = 0x28;
+
+static void intel_snb_verify_ucode(void)
+{
+       u32 rev = UINT_MAX;
+       int pebs_broken = 0;
+       int cpu;
+
+       get_online_cpus();
+       /*
+        * Because the microcode loader is bloody stupid and allows different
+        * revisions per cpu and does strictly per-cpu loading, we now have to
+        * check all cpus to determine the minimally installed revision.
+        *
+        * This makes updating the microcode O(n^2) in the number of CPUs :/
+        */
+       for_each_online_cpu(cpu)
+               rev = min(cpu_data(cpu).microcode, rev);
+       put_online_cpus();
+
+       pebs_broken = (rev < snb_ucode_rev);
+
+       if (pebs_broken == x86_pmu.pebs_broken)
+               return;
+
+       /*
+        * Serialized by the microcode lock..
+        */
+       if (x86_pmu.pebs_broken) {
+               pr_info("PEBS enabled due to micro-code update\n");
+               x86_pmu.pebs_broken = 0;
+       } else {
+               pr_info("PEBS disabled due to CPU errata, "
+                       "please upgrade micro-code to at least %x (current: %x)\n",
+                       snb_ucode_rev, rev);
+               x86_pmu.pebs_broken = 1;
+       }
+}
+
+static int intel_snb_ucode_notifier(struct notifier_block *self,
+                                  unsigned long action, void *_uci)
+{
+       /*
+        * Since ucode cannot be down-graded, and no future ucode revision
+        * is known to break PEBS again, we're ok with MICROCODE_CAN_UPDATE.
+        */
+
+       if (action == MICROCODE_UPDATED)
+               intel_snb_verify_ucode();
+
+       return NOTIFY_DONE;
+}
+
  static __init void intel_sandybridge_quirk(void)
  {
-       pr_warn("PEBS disabled due to CPU errata\n");
-       x86_pmu.pebs = 0;
-       x86_pmu.pebs_constraints = NULL;
+       intel_snb_verify_ucode();
+       /*
+        * we're still single threaded, so while there's a hole here,
+        * you can't trigger it.
+        */
+       microcode_notifier(intel_snb_ucode_notifier);
  }
 
  static const struct { int id; char *name; } intel_arch_events_map[] __initconst = {

arch/x86/kernel/microcode_amd.c +0 -0

index 8a2ce8f..265831e 100644
@@ -294,6 +294,7 @@ generic_load_microcode(int cpu, const u8 *data, size_t size)
        }
 

 out_ok:
+       uci->new_rev = new_rev;
       uci->mc = new_mc;
       state = UCODE_OK;
       pr_debug("CPU%d update ucode (0x%08x -> 0x%08x)\n",

arch/x86/kernel/microcode_core.c +22 -0

index fbdfc69..a6cad81 100644
@@ -167,15 +167,43 @@ static void apply_microcode_local(void *arg)
        ctx->err = microcode_ops->apply_microcode(smp_processor_id());
  }
 
+/*
+ * Call the notifier chain before we update to verify we can indeed
+ * update to the desired revision.
+ */
+
+static int microcode_notifier_check(struct ucode_cpu_info *uci)
+{
+       int ret = blocking_notifier_call_chain(&microcode_notifier,
+                       MICROCODE_CAN_UPDATE, uci);
+       return notifier_to_errno(ret);
+}
+
+/*
+ * Call the notifier chain after we've updated to
+ */
+
+static void microcode_notifier_done(void)
+{
+       blocking_notifier_call_chain(&microcode_notifier, MICROCODE_UPDATED, NULL);
+}
+
  static int apply_microcode_on_target(int cpu)
  {
        struct apply_microcode_ctx ctx = { .err = 0 };
        int ret;
 
+       ret = microcode_notifier_check(ucode_cpu_info + cpu);

arch/x86/kernel/microcode_core.c +17 -8

        return ret;
  }
 

@@ -196,8 +224,11 @@ static int do_microcode_update(const void __user *buf, size_t size)
                if (ustate == UCODE_ERROR) {
                        error = -1;
                        break;
-               } else if (ustate == UCODE_OK)
-                       apply_microcode_on_target(cpu);
+               } else if (ustate == UCODE_OK) {
+                       error = apply_microcode_on_target(cpu);
+                       if (error)
+                               break;
+               }
        }
 
        return error;

@@ -282,11 +313,12 @@ static int reload_for_cpu(int cpu)
                enum ucode_state ustate;
 
                ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev);
-               if (ustate == UCODE_OK)
-                       apply_microcode_on_target(cpu);
-               else
+               if (ustate == UCODE_OK) {
+                       err = apply_microcode_on_target(cpu);
+               } else {
                        if (ustate == UCODE_ERROR)
                                err = -EINVAL;
+               }
        }
        mutex_unlock(&microcode_mutex);
 

@@ -361,14 +393,15 @@ static void microcode_fini_cpu(int cpu)
  static enum ucode_state microcode_resume_cpu(int cpu)
  {
        struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+       int err;
 
        if (!uci->mc)
                return UCODE_NFOUND;
 
        pr_debug("CPU%d updated upon resume\n", cpu);
-       apply_microcode_on_target(cpu);
+       err = apply_microcode_on_target(cpu);
 
-       return UCODE_OK;
+       return !err ? UCODE_OK : UCODE_ERROR;
  }
 
  static enum ucode_state microcode_init_cpu(int cpu)

@@ -385,8 +418,12 @@ static enum ucode_state microcode_init_cpu(int cpu)
        ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev);
 
        if (ustate == UCODE_OK) {
+               int err;
+
                pr_debug("CPU%d updated upon init\n", cpu);
-               apply_microcode_on_target(cpu);
+               err = apply_microcode_on_target(cpu);
+               if (err)
+                       ustate = UCODE_ERROR;
        }
 
        return ustate;

arch/x86/kernel/microcode_intel.c +1 -0

index 0327e2b..6b87bd5 100644
@@ -391,6 +391,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
        }
 
        vfree(uci->mc);
+       uci->new_rev = new_rev;
        uci->mc = (struct microcode_intel *)new_mc;
 
        pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",

arch/x86/kernel/setup.c +5 -0

index f4b9b80..98b5b5c 100644
@@ -68,6 +68,7 @@
  #include <linux/percpu.h>
  #include <linux/crash_dump.h>
  #include <linux/tboot.h>
+#include <linux/notifier.h>
 
  #include <video/edid.h>
 

@@ -205,6 +206,10 @@ struct cpuinfo_x86 boot_cpu_data __read_mostly = {
  EXPORT_SYMBOL(boot_cpu_data);
  #endif
 
+/*
+ * Lives here because the microcode stuff is modular.
+ */
+struct atomic_notifier_head microcode_notifier;
 
  #if !defined(CONFIG_X86_PAE) || defined(CONFIG_X86_64)
  unsigned long mmu_cr4_features;

Quoted 13 lines

static int apply_microcode_on_target(int cpu)

 {
       struct apply_microcode_ctx ctx = { .err = 0 };
       int ret;

arch/x86/kernel/microcode_core.c +4 -0

+       ret = microcode_notifier_check(ucode_cpu_info + cpu);
+       if (ret)
+               return ret;
+
       ret = smp_call_function_single(cpu, apply_microcode_local, &ctx, 1);
       if (!ret)
               ret = ctx.err;

arch/x86/kernel/microcode_core.c static void apply_microcode_local(void *arg) · line 206

Quoted 3 lines

arch/x86/kernel/microcode_core.c +3 -0

+       if (!ret)
+               microcode_notifier_done();
+

I suspect you want to do this here and not after the update() has completed over all CPU (for_each_online_cpu()), because you want to prevent a race condition with perf_event users trying PEBS at the same time. If not, then why not move the callback after all the smp_call() are done.

Why do we have per-cpu firmware anyway?

The very worst is that it does per-cpu micro-code updates, not machine wide. This results in it being able to have different revisions on different cpus. This in turn makes the below O(n^2) :/

How about this: since the ucode cannot be downgraded and since higher ucode versions are supposed to fix current and older problems (otherwise ucoders will get an earlfull) you shouldn't be needing to verify the ucode version on all CPUs per-CPU, i.e. the O(n^2) overhead.

Rather, simply track which CPUs _haven't_ been updated yet, and once this is the empty set, run the verify thing to check ucode version on all CPUs.

And this should happen only when we update ucode from version A to version B, where B > A.

And unless I'm missing something, this should be O(n) and ucode update should happen very seldomly anyway.

Checking a bitmap of n bits for being all zero is O(n), so the total is still O(n^2). Still, probably faster than the for_each_online_cpu() scan I do now.

arch/x86/kernel/cpu/perf_event_intel.c +53 -0

+static const u32 snb_ucode_rev = 0x28;
+
+static void intel_snb_verify_ucode(void)
+{
+	u32 rev = UINT_MAX;
+	int pebs_broken = 0;
+	int cpu;
+
+	get_online_cpus();
+	/*
+	 * Because the microcode loader is bloody stupid and allows different
+	 * revisions per cpu and does strictly per-cpu loading, we now have to
+	 * check all cpus to determine the minimally installed revision.
+	 *
+	 * This makes updating the microcode O(n^2) in the number of CPUs :/
+	 */
+	for_each_online_cpu(cpu)
+		rev = min(cpu_data(cpu).microcode, rev);
+	put_online_cpus();
+
+	pebs_broken = (rev < snb_ucode_rev);
+
+	if (pebs_broken == x86_pmu.pebs_broken)
+		return;
+
+	/*
+	 * Serialized by the microcode lock..
+	 */
+	if (x86_pmu.pebs_broken) {
+		pr_info("PEBS enabled due to micro-code update\n");
+		x86_pmu.pebs_broken = 0;
+	} else {
+		pr_info("PEBS disabled due to CPU errata, "
+			"please upgrade micro-code to at least %x (current: %x)\n",
+			snb_ucode_rev, rev);
+		x86_pmu.pebs_broken = 1;
+	}
+}
+
+static int intel_snb_ucode_notifier(struct notifier_block *self,
+				   unsigned long action, void *_uci)
+{
+	/*
+	 * Since ucode cannot be down-graded, and no future ucode revision
+	 * is known to break PEBS again, we're ok with MICROCODE_CAN_UPDATE.
+	 */
+
+	if (action == MICROCODE_UPDATED)
+		intel_snb_verify_ucode();
+
+	return NOTIFY_DONE;
+}
+
 static __init void intel_sandybridge_quirk(void)
 {

Quoted 57 lines · Peter Zijlstra

arch/x86/kernel/cpu/perf_event_intel.c +53 -0

+static const u32 snb_ucode_rev = 0x28;
+
+static void intel_snb_verify_ucode(void)
+{
+     u32 rev = UINT_MAX;
+     int pebs_broken = 0;
+     int cpu;
+
+     get_online_cpus();
+     /*
+      * Because the microcode loader is bloody stupid and allows different
+      * revisions per cpu and does strictly per-cpu loading, we now have to
+      * check all cpus to determine the minimally installed revision.
+      *
+      * This makes updating the microcode O(n^2) in the number of CPUs :/
+      */
+     for_each_online_cpu(cpu)
+             rev = min(cpu_data(cpu).microcode, rev);
+     put_online_cpus();
+
+     pebs_broken = (rev < snb_ucode_rev);
+
+     if (pebs_broken == x86_pmu.pebs_broken)
+             return;
+
+     /*
+      * Serialized by the microcode lock..
+      */
+     if (x86_pmu.pebs_broken) {
+             pr_info("PEBS enabled due to micro-code update\n");
+             x86_pmu.pebs_broken = 0;
+     } else {
+             pr_info("PEBS disabled due to CPU errata, "
+                     "please upgrade micro-code to at least %x (current: %x)\n",
+                     snb_ucode_rev, rev);
+             x86_pmu.pebs_broken = 1;
+     }
+}
+
+static int intel_snb_ucode_notifier(struct notifier_block *self,
+                                unsigned long action, void *_uci)
+{
+     /*
+      * Since ucode cannot be down-graded, and no future ucode revision
+      * is known to break PEBS again, we're ok with MICROCODE_CAN_UPDATE.
+      */
+
+     if (action == MICROCODE_UPDATED)
+             intel_snb_verify_ucode();
+
+     return NOTIFY_DONE;
+}
+

 static __init void intel_sandybridge_quirk(void) {
-     pr_warn("PEBS disabled due to CPU errata\n");
-     x86_pmu.pebs = 0;

Quoted 8 lines

-     x86_pmu.pebs_constraints = NULL;
+     intel_snb_verify_ucode();
+     /*
+      * we're still single threaded, so while there's a hole here,
+      * you can't trigger it.
+      */
+     microcode_notifier(intel_snb_ucode_notifier);
 }

Instead of registering a microcode notifier, why not checking the availability of pebs dynamically with each syscall in intel_pmu_hw_config()? It looks like intel_snb_verify_ucode() is not that much expensive. We can perform the check only if the event could be for pebs and if pebs is broken. The check could be repeated when setting up a new event after ucode could potentially has been updated (e.g. after bringing a cpu online or so).

-Robert

--
Advanced Micro Devices, Inc.
Operating System Research Center

Quoted 4 lines

Instead of registering a microcode notifier, why not checking the availability of pebs dynamically with each syscall in intel_pmu_hw_config()? It looks like intel_snb_verify_ucode() is not that much expensive. We can perform the check only if the event

could

Quoted 2 lines

be for pebs and if pebs is broken. The check could be repeated when setting up a new event after ucode could potentially has been

updated

Quoted 1 line

(e.g. after bringing a cpu online or so).

That's what I had in my original version.

Quoted 1 line

That's what I had in my original version.

Right, but you really need to check all cpus, not just the one you happen to run on or the boot cpu.

Quoted 4 lines · Peter Zijlstra

Quoted 1 line

That's what I had in my original version.

Right, but you really need to check all cpus, not just the one you happen to run on or the boot cpu.

Ok, the answer to that question is whether sandybridge can support mixed ucode revisions or not. If not, you only need to check the current cpu you're on.

But the ucode loader can be (ab)used to only update one of the many CPUs.

Quoted 2 lines · Peter Zijlstra

But the ucode loader can be (ab)used to only update one of the many CPUs.

Not if you don't support mixed ucode revisions. In that case, you want the same ucode patch applied to _all_ cores on the system. Otherwise, random freezes, explosions and other cataclysmic events can happen.

You want, yes, but afaict the stuff in arch/x86/kernel/microcode_core.c:reload_store means you can force a single cpu to update while leaving the others alone.

Afaict this is not a sane thing to do, but quite possible.

Quoted 3 lines · Peter Zijlstra

You want, yes, but afaict the stuff in arch/x86/kernel/microcode_core.c:reload_store means you can force a single cpu to update while leaving the others alone.

Btw, this doesn't work on AMD... for a reason.

Quoted 1 line

Afaict this is not a sane thing to do, but quite possible.

I'd guess this is still there to support mixed ucode revisions for some oldish platforms. But if SB doesn't support mixed ucode revisions, you don't want to use the reload_store interface anyway for reasons described above. Maybe this interface should be behind a family, model check or so, so that users don't shoot themselves in the foot but it is root-only anyway.

Also, you can create a pebs event while updating micro-code. There's a race window there if you don't check all cpus.

How so? afaict there's nothing stopping it from working.

Ideally this interface should be removed, but yeah. As long as its there you have to check all CPUs, because officially supported or not simply doesn't matter, the user can do it.

Also, you can create a pebs event while updating micro-code. There's a race window there if you don't check all cpus.

I'd guess this is still there to support mixed ucode revisions for some

described above. Maybe this interface should be behind a family, model check or so, so that users don't shoot themselves in the foot but it is root-only anyway.

Quoted 1 line · Borislav Petkov

I'd guess this is still there to support mixed ucode revisions for some

It is still there because of the stable ABI rules. As far as I can tell, it was a crap interface when it got in, and it still is a crap interface now because there simply isn't any sane usercase that requires it, it is dangerous as implemented right now (at least in the Intel case), and even if we fixed the kernel to do the right thing, userspace would not be able to know that and would still need to request 1 microcode refresh per core.

As far as I know, we always want to refresh the microcode on every core, use the firmware interface to pick up a copy of the newest version of the microcode matching the signature of each core, and leave no core behind without an update.

And preferably, we want to request_firmware() only once per microcode, which is rather easy to do: cache every microcode that will be needed, check cache first before doing request_firmware() in the per-core worker threads, and invalidate the cache when the user requests "refresh_all_microcode". So, the cache speeds up multi-core updates, and is also usable when restoring the system from suspend/hibernation, but doesn't get in the way of userspace trying to apply a microcode update.

This would make my pathetic system do one request_firmware instead of eight. And even the old junk with mixed-stepping SMP at work would only require two, instead of four (or eight? I don't recall how many cores per die it has) request_firmware calls.

Quoted 3 lines

described above. Maybe this interface should be behind a family, model check or so, so that users don't shoot themselves in the foot but it is root-only anyway.

This interface should _DIE_.

Perhaps we could make it work only for CPU 0 and return EBUSY or whatever for all others (or just don't publish the sysfs attribute for the others), and change the CPU0 refresh firmware request into a refresh all cores call.

We could then add a new sysfs attribute to cleanly do the update-all-cores request.

Quoted 9 lines · Henrique de Moraes Holschuh

Quoted 1 line · Borislav Petkov

I'd guess this is still there to support mixed ucode revisions for some

It is still there because of the stable ABI rules. As far as I can tell, it was a crap interface when it got in, and it still is a crap interface now because there simply isn't any sane usercase that requires it, it is dangerous as implemented right now (at least in the Intel case), and even if we fixed the kernel to do the right thing, userspace would not be able to know that and would still need to request 1 microcode refresh per core.

Well, I'm disabling it on AMD.

We could make it iterate over _all_ cores and do the update on each one of them even though the user does a sysfs write only for a single core..

Quoted 3 lines

Perhaps we could make it work only for CPU 0 and return EBUSY or whatever for all others (or just don't publish the sysfs attribute for the others), and change the CPU0 refresh firmware request into a refresh all cores call.

That could also work, it sounds like a sane thing to do having in mind the current sysfs layout. I will cook up something soonish.

Quoted 2 lines

We could then add a new sysfs attribute to cleanly do the update-all-cores request.

That's what Fenghua is doing.

Yes, please. I suggest we use core 0 for that. Using my Debian maintainer hat, I'd rather you got rid of the sysfs entries for every other core while at it, as it will make our life a lot simpler, distro-side.

This is still not the proper fix, which would be to add a new sysfs node to access the proper update-every-core functionality, but it is a damn good start in the right direction and required to make it safe without ripping out the old ABI entirely without a deprecation period.

Since Intel processors don't want the per-core behaviour either, you could fix it in the microcode core itself...

Ok. Please CC me in the patches, if the new ABI arrives in time, I'll even be able to get it supported on the next Debian stable (and push to get this stuff backported to the kernel 3.2 which we will ship, I consider this an important bug-fix to a pontentially very serious issue. We have *zero* chance of finding out what's wrong if an users' system start getting subtly crazy because it is running with skewed microcode among cores.

Quoted 4 lines · Henrique de Moraes Holschuh

Yes, please. I suggest we use core 0 for that. Using my Debian maintainer hat, I'd rather you got rid of the sysfs entries for every other core while at it, as it will make our life a lot simpler, distro-side.

Wouldn't we have some sort of ABI breakage if I remove the sysfs files?

Instead, I was thinking of having the rest of the files not on the BSP return -EINVAL and only the BSP reload ucode on the whole system.

Quoted 4 lines

This is still not the proper fix, which would be to add a new sysfs node to access the proper update-every-core functionality, but it is a damn good start in the right direction and required to make it safe without ripping out the old ABI entirely without a deprecation period.

Yes, I'd like to have the system-wide sysfs node somewhere under /sys/devices/system/cpu/microcode/ but we'll see how that goes.

Quoted 7 lines

Ok. Please CC me in the patches, if the new ABI arrives in time, I'll even be able to get it supported on the next Debian stable (and push to get this stuff backported to the kernel 3.2 which we will ship, I consider this an important bug-fix to a pontentially very serious issue. We have *zero* chance of finding out what's wrong if an users' system start getting subtly crazy because it is running with skewed microcode among cores.

Ok, will do.

Btw, I have to think about whether we really want to backport this to stable since it is not a regression fix but functionality change which kinda fixes a some sort of bug. Hmm, the stable rules are kinda blurry here. I could write a minimal fix with stable in mind though... we'll see.

hpa, what is our take here, should we backport a minimal change disabling reloading of ucode per-cpu for stable? It is the wrong thing to do anyway.

This is a minimal fix which we'd like to see in stable too thus the more-or-less arbitrary decision to allow system-wide reloading only on the BSP:

$ echo 1 > /sys/devices/system/cpu/cpu0/microcode/reload
...

and disable the interface on the other cores:

$ echo 1 > /sys/devices/system/cpu/cpu23/microcode/reload
-bash: echo: write error: Invalid argument 

Quoted 7 lines · Borislav Petkov

This is a minimal fix which we'd like to see in stable too thus the more-or-less arbitrary decision to allow system-wide reloading only on the BSP:

$ echo 1 > /sys/devices/system/cpu/cpu0/microcode/reload
...

Quoted 4 lines

and disable the interface on the other cores:

$ echo 1 > /sys/devices/system/cpu/cpu23/microcode/reload
-bash: echo: write error: Invalid argument 

One could allow the reload from all cpus and simply do all cpus, but I guess the rationale for not doing that and restricting it to cpu0 is to avoid the O(n^2) thing in case userspace issues a reload on all cpus?

If so it would be good to mention in the Changelog so people see its not a 'random' choice.

Actually, the BSP thing was more-or-less an arbitrary deal. What we want as an endresult is to do:

$ echo 1 > /sys/devices/system/cpu/microcode/reload

(notice this is in the toplevel "cpu" directory in sysfs)

and do the system-wide reload there.

Actually, that was easier than I thought, see below. And it seems to workie too.

Quoted 2 lines · Borislav Petkov

Actually, that was easier than I thought, see below. And it seems to workie too.

This is on top of that other patch that makes reload_store() do the whole system, right?

From: Borislav Petkov <[email protected]>
Date: Fri, 15 Jun 2012 18:46:05 +0200
Subject: [PATCH] x86, microcode: Make reload interface per system

The reload interface should be per-system so that a full system ucode reload happens (on each core) when doing

echo 1 > /sys/devices/system/cpu/microcode/reload

Move it to the cpu subsys directory instead of it being per-cpu.

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kernel/microcode_core.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

arch/x86/kernel/microcode_core.c +28 -4

index 24b852b61be3..4c6f3b37ed3c 100644
@@ -351,7 +351,6 @@ static DEVICE_ATTR(version, 0400, version_show, NULL);
 static DEVICE_ATTR(processor_flags, 0400, pf_show, NULL);
 
 static struct attribute *mc_default_attrs[] = {
-	&dev_attr_reload.attr,
 	&dev_attr_version.attr,
 	&dev_attr_processor_flags.attr,
 	NULL

@@ -528,6 +527,16 @@ static const struct x86_cpu_id microcode_id[] = {
 MODULE_DEVICE_TABLE(x86cpu, microcode_id);
 #endif
 
+static struct attribute *cpu_root_microcode_attrs[] = {
+	&dev_attr_reload.attr,
+	NULL
+};
+
+static struct attribute_group cpu_root_microcode_group = {
+	.name  = "microcode",
+	.attrs = cpu_root_microcode_attrs,
+};
+
 static int __init microcode_init(void)
 {
 	struct cpuinfo_x86 *c = &cpu_data(0);

@@ -559,9 +568,17 @@ static int __init microcode_init(void)
 	if (error)
 		goto out_pdev;
 
+	error = sysfs_create_group(&cpu_subsys.dev_root->kobj,
+				   &cpu_root_microcode_group);
+
+	if (error) {
+		pr_err("Error creating microcode group!\n");
+		goto out_driver;
+	}
+
 	error = microcode_dev_init();
 	if (error)
-		goto out_driver;
+		goto out_ucode_group;
 
 	register_syscore_ops(&mc_syscore_ops);
 	register_hotcpu_notifier(&mc_cpu_notifier);

@@ -571,7 +588,11 @@ static int __init microcode_init(void)
 
 	return 0;
 
-out_driver:
+ out_ucode_group:
+	sysfs_remove_group(&cpu_subsys.dev_root->kobj,
+			   &cpu_root_microcode_group);
+
+ out_driver:
 	get_online_cpus();
 	mutex_lock(&microcode_mutex);
 

@@ -580,7 +601,7 @@ out_driver:
 	mutex_unlock(&microcode_mutex);
 	put_online_cpus();
 
-out_pdev:
+ out_pdev:
 	platform_device_unregister(microcode_pdev);
 	return error;
 

@@ -596,6 +617,9 @@ static void __exit microcode_exit(void)
 	unregister_hotcpu_notifier(&mc_cpu_notifier);
 	unregister_syscore_ops(&mc_syscore_ops);
 
+	sysfs_remove_group(&cpu_subsys.dev_root->kobj,
+			   &cpu_root_microcode_group);
+
 	get_online_cpus();
 	mutex_lock(&microcode_mutex);

It is a pity this one is much harder to backport to 3.2 and 3.0. I'd really like to have the new interface there. But it looks good, and we will support the new /sys/devices/system/cpu/microcode/reload sysfs node in Debian for the benefit of anyone using a newer kernel than the distro's (which will be based on 3.2).

So, fwiw, you have my:
Acked-by-unimportant-person: Henrique de Moraes Holschuh <[email protected]>

Quoted 7 lines · Henrique de Moraes Holschuh

It is a pity this one is much harder to backport to 3.2 and 3.0. I'd really like to have the new interface there. But it looks good, and we will support the new /sys/devices/system/cpu/microcode/reload sysfs node in Debian for the benefit of anyone using a newer kernel than the distro's (which will be based on 3.2).

Quoted 3 lines

So, fwiw, you have my:
Acked-by-unimportant-person: Henrique de Moraes Holschuh <[email protected]>

I have to admit to being slightly questioning to the whole "sysfs trigger, request_firmware" interface... it seems royally backwards to me (it makes sense for the initial firmware load, I guess, but that is better done early.)

Either way I don't have a hugely strong preference.

$ rmmod microcode; modprobe microcode

but then the same guys come :-) and say they don't want to reload modules for security reasons.

Quoted 4 lines · Borislav Petkov

$ rmmod microcode; modprobe microcode

but then the same guys come :-) and say they don't want to reload modules for security reasons.

Nah, they built their kernel without modules, much more secure!

But I haven't tested what happens when it is built in and I use the reload interface, hehe something to play with today.

Quoted 3 lines · Borislav Petkov

But I haven't tested what happens when it is built in and I use the reload interface, hehe something to play with today.

I couldn't easily find where to place the damn ucode image anyway, so I built in the old-style /dev interface and used the microcode.ctl package to load it.

Fwiw, that works just fine when its built-in.

Now we load the ucode driver very early if it is built-in (look at printk timestamps):

[    3.651275] microcode: CPU0: patch_level=0x010000c4
[    3.664943] microcode: CPU1: patch_level=0x010000c4
[    3.670177] microcode: CPU2: patch_level=0x010000c4
[    3.675407] microcode: CPU3: patch_level=0x010000c4

...

and then later, when we have userspace, we can use the reload thing to re-request microcode. Which is also fine, I guess.

Quoted 2 lines · Borislav Petkov

But I haven't tested what happens when it is built in and I use the reload interface, hehe something to play with today.

I couldn't easily find where to place the damn ucode image anyway, so I built in the old-style /dev interface and used the microcode.ctl package to load it.

Even if we have the very early microcode facility, we *still* want the runtime one based on sysfs+request_firmware in order to update microcode without the need for a reboot.

Quoted 3 lines · Henrique de Moraes Holschuh

Even if we have the very early microcode facility, we *still* want the runtime one based on sysfs+request_firmware in order to update microcode without the need for a reboot.

I am slightly questioning the clause "based on sysfs+request_firmware" in the above sentence, but again, not a huge deal either way.

Well, it makes the kernel tell us what (amd/intel/whatever) firmware it wants, and requires (after patching) a simple echo 1 >foo to activate. It is also harmless if activated without updated firmware in place.

/dev/cpu/microcode requires special software (or a dd with bs>=file size), since it borks if userspace breaks a microcode into two write() syscalls, and requires detection of the cpu vendor. Very annoying.

Quoted 8 lines · Henrique de Moraes Holschuh

Well, it makes the kernel tell us what (amd/intel/whatever) firmware it wants, and requires (after patching) a simple echo 1 >foo to activate. It is also harmless if activated without updated firmware in place.

/dev/cpu/microcode requires special software (or a dd with bs>=file size), since it borks if userspace breaks a microcode into two write() syscalls, and requires detection of the cpu vendor. Very annoying.

Yeah, because dd is such an exotic piece of software...

No, because dd if=<whatever> of=/dev/cpu/microcode bs=1M means I have to add dd to the initramfs or to busybox, AND it will break the day the microcode data file gets bigger than 1M. And it will be at best very annoying to have to special case each vendor to locate the correct microcode, etc.

Quoted 5 lines · Henrique de Moraes Holschuh

No, because dd if=<whatever> of=/dev/cpu/microcode bs=1M means I have to add dd to the initramfs or to busybox, AND it will break the day the microcode data file gets bigger than 1M. And it will be at best very annoying to have to special case each vendor to locate the correct microcode, etc.

No.

The whole point is you won't have to put ANYTHING in your initramfs, period. The early microcode blob will be all you need there.

So the only issue left is what to do when you want to update the microcode in an already running system.

Quoted 7 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>
Date: Fri, 15 Jun 2012 18:46:05 +0200
Subject: [PATCH] x86, microcode: Make reload interface per system

The reload interface should be per-system so that a full system ucode reload happens (on each core) when doing

Quoted 5 lines

echo 1 > /sys/devices/system/cpu/microcode/reload

Move it to the cpu subsys directory instead of it being per-cpu.

Signed-off-by Borislav Petkov <[email protected]>

It is a pity this one is much harder to backport to 3.2 and 3.0.

I'd really like to have the new interface there. But it looks good, and we will support the new /sys/devices/system/cpu/microcode/reload sysfs node in Debian for the benefit of anyone using a newer kernel than the distro's (which will be based on 3.2).

So, fwiw, you have my:
Acked-by-unimportant-person: Henrique de Moraes Holschuh <[email protected]>

Quoted 12 lines · Henrique de Moraes Holschuh

Quoted 11 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>
Date: Fri, 15 Jun 2012 18:46:05 +0200
Subject: [PATCH] x86, microcode: Make reload interface per system

The reload interface should be per-system so that a full system ucode reload happens (on each core) when doing

echo 1 > /sys/devices/system/cpu/microcode/reload

Move it to the cpu subsys directory instead of it being per-cpu.

Quoted 3 lines

Quoted 1 line

Signed-off-by Borislav Petkov <[email protected]>

It is a pity this one is much harder to backport to 3.2 and 3.0.

Yeah, not harder. It changes the sysfs interface and we can't do that for already released kernels.

But I think you meant that.

Quoted 4 lines

I'd really like to have the new interface there. But it looks good, and we will support the new /sys/devices/system/cpu/microcode/reload sysfs node in Debian for the benefit of anyone using a newer kernel than the distro's (which will be based on 3.2).

Right, hopefully the next Debian release will have that nice shiny new interface :-)

Quoted 2 lines

So, fwiw, you have my:
Acked-by-unimportant-person: Henrique de Moraes Holschuh <[email protected]>

No, don't underestimate yourself. I think it helps a lot to have someone look at the patch and even better: give comments whether what we're doing is sane for userspace. So, I appreciate your comments a lot, keep 'em coming :-)

Thanks for review and testing, I'll send out the patches soon.

I was wondering whether it would be worthwhile to backport the new ABI for the Debian 3.2 kernel or not. Probably not.

Backporting to 3.2 and 3.0 is straigthforward, however it will look nasty as one has to "inappropriately touch" the cpu sysdev class to get the attribute group directly connected to /sys/devices/system/cpu.

I did notice there were no stable backports of the error unwind during module init, but that one is a very rarely used codepath. Maybe worth to backport a fix to stable, though.

Well, my ack is unimportant in the "this is not an area of the kernel I have any authority to ack things" sense. But we don't have a "I-wanna-that-by:" or even a "Thumbs-up-by:"...

Quoted 1 line

Thanks for review and testing, I'll send out the patches soon.

Thank you for addressing these issues and writing the patches!

From: Borislav Petkov <[email protected]>
Date: Fri, 15 Jun 2012 14:19:55 +0200
Subject: [PATCH] x86, microcode: Sanitize per-cpu microcode reloading
interface

Microcode reloading in a per-core manner is a very bad idea for both major x86 vendors. And the thing is, we have such interface with which we can end up with different microcode versions applied on different cores of an otherwise homogeneous wrt (family,model,stepping) system.

So turn off the possibility of doing that per core and allow it only system-wide.

This is a minimal fix which we'd like to see in stable too thus the more-or-less arbitrary decision to allow system-wide reloading only on the BSP:

$ echo 1 > /sys/devices/system/cpu/cpu0/microcode/reload
...

and disable the interface on the other cores:

$ echo 1 > /sys/devices/system/cpu/cpu23/microcode/reload
-bash: echo: write error: Invalid argument

A more generic fix will follow.

Cc Henrique de Moraes Holschuh <[email protected]>

Cc [email protected]

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kernel/microcode_core.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

arch/x86/kernel/microcode_core.c +19 -7

index fbdfc6917180..24b852b61be3 100644
@@ -298,19 +298,31 @@ static ssize_t reload_store(struct device *dev,
 			    const char *buf, size_t size)
 {
 	unsigned long val;
-	int cpu = dev->id;
-	ssize_t ret = 0;
+	int cpu;
+	ssize_t ret = 0, tmp_ret;
+
+	/* allow reload only from the BSP */
+	if (boot_cpu_data.cpu_index != dev->id)
+		return -EINVAL;
 
 	ret = kstrtoul(buf, 0, &val);
 	if (ret)
 		return ret;
 
-	if (val == 1) {
-		get_online_cpus();
-		if (cpu_online(cpu))
-			ret = reload_for_cpu(cpu);
-		put_online_cpus();
+	if (val != 1)
+		return size;
+
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		tmp_ret = reload_for_cpu(cpu);
+		if (tmp_ret != 0)
+			pr_warn("Error reloading microcode on CPU %d\n", cpu);
+
+		/* save retval of the first encountered reload error */
+		if (!ret)
+			ret = tmp_ret;
 	}
+	put_online_cpus();
 
 	if (!ret)
 		ret = size;

Quoted 5 lines · Stephane Eranian

Quoted 4 lines

Instead of registering a microcode notifier, why not checking the availability of pebs dynamically with each syscall in intel_pmu_hw_config()? It looks like intel_snb_verify_ucode() is not that much expensive. We can perform the check only if the event

could

Quoted 4 lines

Quoted 2 lines

be for pebs and if pebs is broken. The check could be repeated when setting up a new event after ucode could potentially has been

updated

Quoted 1 line

(e.g. after bringing a cpu online or so).

Because you then end up with a for_each_online_cpu() loop in there, that's not pretty and quite horrible on large systems when you need to create nr_cpus events.

Furthermore, ucode update is the rare thing, creating events happens much more frequently.

Quoted 1 line

That's what I had in my original version.

Right, but you really need to check all cpus, not just the one you happen to run on or the boot cpu.

Quoted 7 lines

Instead of registering a microcode notifier, why not checking the availability of pebs dynamically with each syscall in intel_pmu_hw_config()? It looks like intel_snb_verify_ucode() is not that much expensive. We can perform the check only if the event could be for pebs and if pebs is broken. The check could be repeated when setting up a new event after ucode could potentially has been updated (e.g. after bringing a cpu online or so).

That's what I had in my original version.

This will only warn about the PEBS issue once you try and use a PEBS counter. Shouldn't we do this on boot? IOW. put check_pebs_quirks()

arch/x86/kernel/cpu/perf_event_intel.c +4 -0

+		pr_warn("SandyBridge PEBS unavailable due to CPU erratum, "
+			" update microcode (was 0x%x, needs at least 0x28).\n",
+			uversion);
+		return -ENOTSUPP;

Andi just alerted me to this thread.

I, Thomas and Borislav discussed this at LCE last year, and it really comes up that we have a *large* number of reasons to load microcode early. We then need to keep the microcode around so that it can be loaded on APs, hotplug CPUs, and after S3 resume.

The protocol discussion happened in this thread:

http://marc.info/[email protected]

Fenghua Yu at Intel is currently working on the implementation.

Nice, this is the initrd cpio thing, cool, no changes needed for the bootloader. Ping me when you have something ready so that I can start testing it here too.

Quoted 3 lines · Borislav Petkov

Nice, this is the initrd cpio thing, cool, no changes needed for the bootloader. Ping me when you have something ready so that I can start testing it here too.

What do people like me do that don't use initrd like nonsense?

Quoted 3 lines · Borislav Petkov

Nice, this is the initrd cpio thing, cool, no changes needed for the bootloader. Ping me when you have something ready so that I can start testing it here too.

What do people like me do that don't use initrd like nonsense?

Also, I'd assume we need some of the current functionality to be able to load ucode without rebooting the box.

Quoted 4 lines · Borislav Petkov

Also, I'd assume we need some of the current functionality to be able to load ucode without rebooting the box.

Yes, the basic idea is that we'll keep the ucode around anyway so we can feed it to hotplug CPUs or S3 resume. If the ucode is updated from userspace we'd replace the in-memory copy and load it into the CPUs.

Quoted 9 lines · H. Peter Anvin

Quoted 4 lines · Borislav Petkov

Also, I'd assume we need some of the current functionality to be able to load ucode without rebooting the box.

Yes, the basic idea is that we'll keep the ucode around anyway so we can feed it to hotplug CPUs or S3 resume. If the ucode is updated from userspace we'd replace the in-memory copy and load it into the CPUs.

You say CPUs, i.e. plural. What is your take on loading ucode on a single core, i.e. the /sys/devices/system/cpu/cpuX/microcode/reload interface? Do you guys need to load ucode only system-wide or can you stomach per-core loads?

I want to disable it (or even remove it, modulo the whole userspace compat blah blah) on AMD.

Also, if we do system-wide, can we have a single sysfs node where we give the new ucode from userspace instead of per-cpu files?

I personally don't know of any valid use case for per-core loads, and it sounds like a horrid idea.

And yes, I would prefer a single sysfs file, or better yet a plain old device node.

Quoted 2 lines · H. Peter Anvin

I personally don't know of any valid use case for per-core loads, and it sounds like a horrid idea.

Vehemently agreed.

However, last time we talked about Ingo mentioned some possibility of having different microcode versions on different CPUs:
http://lkml.indiana.edu/hypermail/linux/kernel/1105.3/00749.html

Hmm...

Quoted 2 lines

And yes, I would prefer a single sysfs file, or better yet a plain old device node.

Also agreed.

There is a difference here. Mixed steppings or even mixed CPUs are, indeed, supported (even things like Nehalem + Westmere in the same system). However, the notion is that we should upload the *entire set* of microcodes to the kernel, and let it flash the appropriate one into each CPU.

This may result in different version number on different CPUs.

even things like Nehalem + Westmere in the same system

Quoted 1 line · H. Peter Anvin

even things like Nehalem + Westmere in the same system

Uhm,.. how well do you expect Linux to run on a franken-system like that? I'm pretty sure perf will malfunction in such a setup.

Quoted 6 lines · Peter Zijlstra

Quoted 1 line · H. Peter Anvin

even things like Nehalem + Westmere in the same system

Uhm,.. how well do you expect Linux to run on a franken-system like that? I'm pretty sure perf will malfunction in such a setup.

perf might, but I have seen such frankensystems in the field and they work (although I haven't seen that particular combo, I have to admit.)

Quoted 3 lines

perf might, but I have seen such frankensystems in the field and they work (although I haven't seen that particular combo, I have to admit.)

Thing is, if we boot on the wsm, perf could fault the system with nonexistent MSR accesses, if we boot on the nhm worst is wrong numbers I think.

I'm not really inclined to 'fix' this.. the best I'm willing to accept is a patch that would detect this situation and simply disable all hardware perf support for the platform.

And yes, I would prefer a single sysfs file,
or better yet a plain old device node.

Quoted 2 lines · H. Peter Anvin

And yes, I would prefer a single sysfs file,
or better yet a plain old device node.

So how about we rip out all this sysfs crap and go back to MICROCODE_OLD_INTERFACE ? Afaict that's relatively sane.

Quoted 2 lines · H. Peter Anvin

And yes, I would prefer a single sysfs file,
or better yet a plain old device node.

So how about we rip out all this sysfs crap and go back to MICROCODE_OLD_INTERFACE ? Afaict that's relatively sane.

But why, do you see a better option for how to tell the kernel to load a binary blob early at boot?

Quoted 3 lines · Borislav Petkov

But why, do you see a better option for how to tell the kernel to load a binary blob early at boot?

built it in? There's this feature where you can included firmware blobs in the kernel image. It has all the benefits of initrd, since the only time I ever would update initrd if used it, would be when I install a new kernel, which I just built anyway.

Quoted 5 lines · Peter Zijlstra

Quoted 3 lines · Borislav Petkov

But why, do you see a better option for how to tell the kernel to load a binary blob early at boot?

Quoted 5 lines

built it in? There's this feature where you can included firmware blobs in the kernel image. It has all the benefits of initrd, since the only time I ever would update initrd if used it, would be when I install a new kernel, which I just built anyway.

You know you can build in an initramfs blob in the kernel, right? In fact, there always is one there even if you don't create it explicitly.

Problem solved, methinks.

Quoted 12 lines · H. Peter Anvin

Quoted 10 lines · Peter Zijlstra

Quoted 3 lines · Borislav Petkov

But why, do you see a better option for how to tell the kernel to load a binary blob early at boot?

built it in? There's this feature where you can included firmware blobs in the kernel image. It has all the benefits of initrd, since the only time I ever would update initrd if used it, would be when I install a new kernel, which I just built anyway.

Quoted 2 lines

You know you can build in an initramfs blob in the kernel, right? In fact, there always is one there even if you don't create it explicitly.

Didn't know that..

Quoted 1 line

Problem solved, methinks.

Yep.

Quoted 3 lines · Borislav Petkov

Nice, this is the initrd cpio thing, cool, no changes needed for the bootloader. Ping me when you have something ready so that I can start testing it here too.

What do people like me do that don't use initrd like nonsense?