Re: [PATCH 2/8] x86, microcode, intel: don't update each HT core twice

[Borislav Petkov <bp@alien8.de>]

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