Re: [PATCH] perf/x86: check ucode before disabling PEBS on SandyBridge

[Stephane Eranian]

On Fri, Jun 8, 2012 at 3:26 PM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> 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) :/
>
But it's not like this is a frequent operation either...

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

> +#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 */
> 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;
> +

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.

> +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(&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;
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/