Messages in this thread | | | Date | Mon, 20 Oct 2014 15:32:52 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH 2/8] x86, microcode, intel: don't update each HT core twice |
| |
On Mon, Sep 08, 2014 at 02:37:48PM -0300, Henrique de Moraes Holschuh wrote: > Fix a regression introduced by 506ed6b53e00ba303ad778122f08e1fca7cf5efb, > "x86, intel: Output microcode revision in /proc/cpuinfo", which added a > cache of the thread microcode revision to cpu_data()->microcode and > switched the microcode driver to using the cached value. > > This caused the driver to needlessly update each processor core twice > when hyper-threading is enabled (once per hardware thread). The early > microcode update code that runs during BSP/AP setup does not have this > problem. > > Intel microcode update operations are extremely expensive. The WRMSR > 79H instruction could take anywhere from a hundred-thousand to several > million cycles to successfully apply a microcode update, depending on > processor model and microcode update size. > > To avoid updating the same core twice per microcode update run, refresh > the microcode revision of each CPU (hardware thread) before deciding > whether it needs an update or not. > > A silent version of collect_cpu_info() is required for this fix, > otherwise the logs produced by a microcode update run would be twice as > long and very confusing. > > Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> > --- > arch/x86/kernel/cpu/microcode/intel.c | 39 ++++++++++++++++++++++----------- > 1 file changed, 26 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c > index c6826d1..2c629d1 100644 > --- a/arch/x86/kernel/cpu/microcode/intel.c > +++ b/arch/x86/kernel/cpu/microcode/intel.c > @@ -87,7 +87,7 @@ MODULE_DESCRIPTION("Microcode Update Driver"); > MODULE_AUTHOR("Tigran Aivazian <tigran@aivazian.fsnet.co.uk>"); > MODULE_LICENSE("GPL"); > > -static int collect_cpu_info(int cpu_num, struct cpu_signature *csig) > +static void __collect_cpu_info(int cpu_num, struct cpu_signature *csig) > { > struct cpuinfo_x86 *c = &cpu_data(cpu_num); > unsigned int val[2]; > @@ -102,7 +102,19 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig) > csig->pf = 1 << ((val[1] >> 18) & 7); > } > > - csig->rev = c->microcode; > + /* get the current microcode revision from MSR 0x8B */ > + wrmsr(MSR_IA32_UCODE_REV, 0, 0); > + sync_core(); > + rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]); > + > + csig->rev = val[1]; > + c->microcode = val[1]; /* re-sync */ > +} > + > +static int collect_cpu_info(int cpu_num, struct cpu_signature *csig) > +{ > + __collect_cpu_info(cpu_num, csig); > + > pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n", > cpu_num, csig->sig, csig->pf, csig->rev);
We probably should downgrade this to pr_debug and use collect_cpu_info() everywhere instead of having a __ version.
> > @@ -118,7 +130,10 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu) > struct cpu_signature cpu_sig; > unsigned int csig, cpf, crev; > > - collect_cpu_info(cpu, &cpu_sig); > + /* NOTE: cpu_data()->microcode will be outdated on HT > + * processors during an update run, it must be refreshed > + * from MSR 0x8B */ > + __collect_cpu_info(cpu, &cpu_sig); > > csig = cpu_sig.sig; > cpf = cpu_sig.pf; > @@ -145,23 +160,21 @@ static int apply_microcode_intel(int cpu) > return 0; > > /* > - * Microcode on this CPU could be updated earlier. Only apply the > - * microcode patch in mc_intel when it is newer than the one on this > - * CPU. > + * Microcode on this CPU might be already up-to-date. Only apply > + * the microcode patch in mc_intel when it is newer than the one > + * on this CPU. > */ > if (get_matching_mc(mc_intel, cpu) == 0) > return 0; > > - /* write microcode via MSR 0x79 */ > + /* write microcode via MSR 0x79. THIS IS VERY EXPENSIVE */
No need for screaming here - we know MSR accesses are expensive. This comment is totally useless here so drop it altogether.
> wrmsr(MSR_IA32_UCODE_WRITE, > - (unsigned long) mc_intel->bits, > - (unsigned long) mc_intel->bits >> 16 >> 16); > - wrmsr(MSR_IA32_UCODE_REV, 0, 0); > - > - /* As documented in the SDM: Do a CPUID 1 here */ > - sync_core(); > + lower_32_bits((unsigned long) mc_intel->bits), > + upper_32_bits((unsigned long) mc_intel->bits));
wrmsrl() takes u64 directly - no need for the splitting.
> /* get the current revision from MSR 0x8B */ > + wrmsr(MSR_IA32_UCODE_REV, 0, 0); > + sync_core(); > rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]); > > if (val[1] != mc_intel->hdr.rev) { > -- > 1.7.10.4 > >
-- Regards/Gruss, Boris.
Sent from a fat crate under my desk. Formatting is fine. --
| |