lkml.org 
[lkml]   [2012]   [Jun]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/4] x86: Do microcode updates at CPU_STARTING, not CPU_ONLINE
    On Wed, Jun 13, 2012 at 01:20:39PM -0700, Andi Kleen wrote:
    > From: Andi Kleen <ak@linux.intel.com>
    >
    > Do microcode updates of resuming or newling plugged CPUs earlier
    > in CPU_STARTING instead of later when ONLINE. This prevents races
    > with parallel users who may need a microcode update to avoid some
    > problem.
    >
    > Since we cannot request the microcode from udev at this stage,
    > try to grab the microcode from another CPU. This is also more efficient
    > because it avoids redundant loads. In addition to that
    > it avoids the need for separate paths for resume and CPU bootup.
    >
    > This requires invalidating the microcodes on other CPUs on free.
    > Each CPU does this in parallel, so it's not a big problem.
    >
    > When there is no good microcode available the update is delayed
    > until the update can be requested. In the normal cases it should
    > be available.
    >
    > Signed-off-by: Andi Kleen <ak@linux.intel.com>
    > ---
    > arch/x86/kernel/microcode_core.c | 65 +++++++++++++++++++++++++------------
    > arch/x86/kernel/microcode_intel.c | 13 +++++++-
    > 2 files changed, 56 insertions(+), 22 deletions(-)
    >
    > diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
    > index fbdfc69..f947ef7 100644
    > --- a/arch/x86/kernel/microcode_core.c
    > +++ b/arch/x86/kernel/microcode_core.c
    > @@ -358,20 +358,7 @@ static void microcode_fini_cpu(int cpu)
    > uci->valid = 0;
    > }
    >
    > -static enum ucode_state microcode_resume_cpu(int cpu)
    > -{
    > - struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
    > -
    > - if (!uci->mc)
    > - return UCODE_NFOUND;
    > -
    > - pr_debug("CPU%d updated upon resume\n", cpu);
    > - apply_microcode_on_target(cpu);
    > -
    > - return UCODE_OK;
    > -}
    > -
    > -static enum ucode_state microcode_init_cpu(int cpu)
    > +static enum ucode_state microcode_init_cpu_late(int cpu)
    > {
    > enum ucode_state ustate;
    >
    > @@ -392,15 +379,44 @@ static enum ucode_state microcode_init_cpu(int cpu)
    > return ustate;
    > }
    >
    > -static enum ucode_state microcode_update_cpu(int cpu)
    > +/* Grab ucode from another CPU */
    > +static void clone_ucode_data(void)
    > +{
    > + int cpu = smp_processor_id();
    > + int i;
    > +
    > + for_each_online_cpu (i) {
    > + if (ucode_cpu_info[i].mc &&
    > + ucode_cpu_info[i].valid &&
    > + cpu_data(i).x86 == cpu_data(cpu).x86 &&
    > + cpu_data(i).x86_model == cpu_data(cpu).x86_model) {
    > + ucode_cpu_info[cpu].mc = ucode_cpu_info[i].mc;
    > + break;
    > + }
    > + }
    > +}
    > +
    > +static enum ucode_state microcode_init_cpu_early(int cpu)
    > +{
    > + clone_ucode_data();
    > + /* We can request later when the CPU is online */
    > + if (ucode_cpu_info[cpu].mc == NULL)
    > + return UCODE_ERROR;
    > + if (microcode_ops->collect_cpu_info(cpu, &ucode_cpu_info[cpu].cpu_sig))
    > + return UCODE_ERROR;
    > + if (microcode_ops->apply_microcode(smp_processor_id()))
    > + pr_warn("CPU%d microcode update failed\n", cpu);
    > + return UCODE_OK;
    > +}

    This is run only from the notifier and nothing is looking at its return
    values. It should be returning void in such cases.

    > +
    > +static enum ucode_state microcode_update_cpu_late(int cpu)
    > {
    > struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
    > enum ucode_state ustate;
    >
    > - if (uci->valid)
    > - ustate = microcode_resume_cpu(cpu);
    > - else
    > - ustate = microcode_init_cpu(cpu);
    > + /* Resume already done early */
    > + if (!uci->valid)
    > + ustate = microcode_init_cpu_late(cpu);
    >
    > return ustate;
    > }
    > @@ -418,7 +434,7 @@ static int mc_device_add(struct device *dev, struct subsys_interface *sif)
    > if (err)
    > return err;
    >
    > - if (microcode_init_cpu(cpu) == UCODE_ERROR)
    > + if (microcode_init_cpu_late(cpu) == UCODE_ERROR)
    > return -EINVAL;
    >
    > return err;
    > @@ -468,9 +484,16 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
    >
    > dev = get_cpu_device(cpu);
    > switch (action) {
    > + case CPU_STARTING:
    > + case CPU_STARTING_FROZEN:
    > + microcode_init_cpu_early(cpu);
    > + break;
    > +
    > case CPU_ONLINE:
    > case CPU_ONLINE_FROZEN:
    > - microcode_update_cpu(cpu);
    > + /* Retry again in case we couldn't request early */
    > + if (cpu_data(cpu).microcode < ucode_cpu_info[cpu].cpu_sig.rev)
    > + microcode_update_cpu_late(cpu);

    This implies newer ucode versions are numerically higher than
    what's currently present. And it is probably correct in the 99% of
    the cases but it would be more correct IMHO to have "!=" there.
    microcode_update_cpu_late takes care of checking the correct ucode
    version anyway down the path.

    --
    Regards/Gruss,
    Boris.


    \
     
     \ /
      Last update: 2012-06-14 13:41    [from the cache]
    ©2003-2011 Jasper Spaans