| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1339161972.2507.13.camel@laptop>
Date: Fri, 08 Jun 2012 15:26:12 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Ingo Molnar <mingo@...nel.org>
Cc: Stephane Eranian <eranian@...gle.com>,
linux-kernel@...r.kernel.org, andi@...stfloor.org, mingo@...e.hu,
ming.m.lin@...el.com, Andreas Herrmann <andreas.herrmann3@....com>,
Borislav Petkov <borislav.petkov@....com>,
Dimitri Sivanich <sivanich@....com>,
Dmitry Adamushko <dmitry.adamushko@...il.com>
Subject: Re: [PATCH] perf/x86: check ucode before disabling PEBS on
SandyBridge
On Fri, 2012-06-08 at 12:00 +0200, Peter Zijlstra wrote:
> > > 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(-)
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 4ebe157..1cd2dc5 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -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(µcode_notifier, &fn##_nb);\
+} while (0)
+
#endif /* _ASM_X86_MICROCODE_H */
diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 000a474..d3ef86c1 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -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,
};
diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
index 3df3de9..a5ecfe8 100644
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -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;
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index 5ec146c..bde86cf 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -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 = {
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 8a2ce8f..265831e 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -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",
diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index fbdfc69..a6cad81 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -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(µcode_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(µcode_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, µcode_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(µcode_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, µcode_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;
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 0327e2b..6b87bd5 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -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",
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f4b9b80..98b5b5c 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists