lkml.org 
[lkml]   [2012]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] perf/x86: check ucode before disabling PEBS on SandyBridge
    From
    On Fri, Jun 8, 2012 at 3:26 PM, Peter Zijlstra <peterz@infradead.org> 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) :/
    >
    > 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(&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;
    > +
    > +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();
    > +
    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.

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

    \
     
     \ /
      Last update: 2012-06-08 19:01    [W:4.284 / U:0.380 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site