Messages in this thread | | | Date | Thu, 14 Jun 2012 13:00:50 +0200 | From | Borislav Petkov <> | Subject | Re: [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.
| |