Re: [PATCH 1/3] x86, intel: Output microcode revision v2

[Jean Delvare]

Hi Andi,

On Wed, 25 May 2011 12:32:26 -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> 
> I got a request to make it easier to determine the microcode update level
> on Intel CPUs. This patch adds a new "cpu update" field to /proc/cpuinfo,
> which I added at the end to minimize impact on parsers.
> 
> The update level is also outputed on fatal machine checks together
> with the other CPUID model information.
> 
> I removed the respective code from the microcode update driver, it
> just reads the field from cpu_data. Also when the microcode is updated
> it fills in the new values too.
> 
> I had to add a memory barrier to native_cpuid to prevent it being
> optimized away when the result is not used.
> 
> This turns out to clean up further code which already got this
> information manually. This is done in followon patches.
> 
> v2:  Lots of updates based on feedback.

Please run scripts/checkpatch.pl on your patches before your post them.
This one adds trailing white space on 3 lines and leading spaces on 3
lines as well. Please fix.

> Signed-off-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> ---
>  arch/x86/include/asm/processor.h  |    5 ++++-
>  arch/x86/kernel/cpu/intel.c       |   14 ++++++++++++++
>  arch/x86/kernel/cpu/mcheck/mce.c  |    5 +++--
>  arch/x86/kernel/cpu/proc.c        |    3 ++-
>  arch/x86/kernel/microcode_intel.c |   14 +++++---------
>  5 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 4c25ab4..3d0f214 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -111,6 +111,8 @@ struct cpuinfo_x86 {
>  	/* Index into per_cpu list: */
>  	u16			cpu_index;
>  #endif
> +	/* CPU update signature */
> +	u32			cpu_update;
>  } __attribute__((__aligned__(SMP_CACHE_BYTES)));
>  
>  #define X86_VENDOR_INTEL	0
> @@ -179,7 +181,8 @@ static inline void native_cpuid(unsigned int *eax, unsigned int *ebx,
>  	      "=b" (*ebx),
>  	      "=c" (*ecx),
>  	      "=d" (*edx)
> -	    : "0" (*eax), "2" (*ecx));
> +	    : "0" (*eax), "2" (*ecx)
> +	    : "memory");
>  }
>  
>  static inline void load_cr3(pgd_t *pgdir)
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 1edf5ba..ba5ba17 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -364,6 +364,20 @@ static void __cpuinit init_intel(struct cpuinfo_x86 *c)
>  
>  	early_init_intel(c);
>  
> +	/* Determine CPU update level */
> +	if (c->x86 >= 6 && !cpu_has(c, X86_FEATURE_IA64)) {	
> +		unsigned lower_word;
> +
> +		wrmsr(MSR_IA32_UCODE_REV, 0, 0);
> +		/* The CPUID 1 fills in the MSR as documented in the SDM */
> +		/* 
> + 		 * Wrong comment from microcode_intel.c:
> + 		 * see notes above for revision 1.07.  Apparent chip bug
> + 		 */

Why copy this comment if it is wrong? Plus "above" is meaningless now
that you moved it. If anyone really needs the info, it's in the git
history forever, no need to pollute the source code.

> +		cpuid_eax(1);
> +		rdmsr(MSR_IA32_UCODE_REV, lower_word, c->cpu_update);
> +	}
> +
>  	intel_workarounds(c);
>  
>  	/*
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index ff1ae9b..1d3cf21 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -220,8 +220,9 @@ static void print_mce(struct mce *m)
>  		pr_cont("MISC %llx ", m->misc);
>  
>  	pr_cont("\n");
> -	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x\n",
> -		m->cpuvendor, m->cpuid, m->time, m->socketid, m->apicid);
> +	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x CPU-UPDATE %u\n",
> +		m->cpuvendor, m->cpuid, m->time, m->socketid, m->apicid, 
> +		cpu_data(m->extcpu).cpu_update);
>  
>  	/*
>  	 * Print out human-readable details about the MCE error,
> diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
> index 62ac8cb..f27d6e5 100644
> --- a/arch/x86/kernel/cpu/proc.c
> +++ b/arch/x86/kernel/cpu/proc.c
> @@ -85,6 +85,8 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>  		seq_printf(m, "stepping\t: %d\n", c->x86_mask);
>  	else
>  		seq_printf(m, "stepping\t: unknown\n");
> +	if (c->cpu_update)
> +		seq_printf(m, "cpu update\t: %u\n", c->cpu_update);
>  
>  	if (cpu_has(c, X86_FEATURE_TSC)) {
>  		unsigned int freq = cpufreq_quick_get(cpu);
> @@ -132,7 +134,6 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>  				seq_printf(m, " [%d]", i);
>  		}
>  	}
> -
>  	seq_printf(m, "\n\n");
>  
>  	return 0;

Please don't mix white space changes with real code changes.

> diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
> index 1a1b606..1efe9d4 100644
> --- a/arch/x86/kernel/microcode_intel.c
> +++ b/arch/x86/kernel/microcode_intel.c
> @@ -161,12 +161,7 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
>  		csig->pf = 1 << ((val[1] >> 18) & 7);
>  	}
>  
> -	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
> -	/* see notes above for revision 1.07.  Apparent chip bug */
> -	sync_core();
> -	/* get the current revision from MSR 0x8B */
> -	rdmsr(MSR_IA32_UCODE_REV, val[0], csig->rev);
> -
> +	csig->rev = c->cpu_update;
>  	pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
>  		cpu_num, csig->sig, csig->pf, csig->rev);
>  
> @@ -299,9 +294,9 @@ static int apply_microcode(int cpu)
>  	struct microcode_intel *mc_intel;
>  	struct ucode_cpu_info *uci;
>  	unsigned int val[2];
> -	int cpu_num;
> +	int cpu_num = raw_smp_processor_id();
> +	struct cpuinfo_x86 *c = &cpu_data(cpu_num);
>  
> -	cpu_num = raw_smp_processor_id();
>  	uci = ucode_cpu_info + cpu;
>  	mc_intel = uci->mc;
>  
> @@ -317,7 +312,7 @@ static int apply_microcode(int cpu)
>  	      (unsigned long) mc_intel->bits >> 16 >> 16);
>  	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
>  
> -	/* see notes above for revision 1.07.  Apparent chip bug */
> +	/* As documented in the SDM: Do a CPUID 1 here */
>  	sync_core();
>  
>  	/* get the current revision from MSR 0x8B */
> @@ -335,6 +330,7 @@ static int apply_microcode(int cpu)
>  		(mc_intel->hdr.date >> 16) & 0xff);
>  
>  	uci->cpu_sig.rev = val[1];
> +	c->cpu_update = val[1];
>  
>  	return 0;
>  }


-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/