[PATCH 0/2] x86, microcode: AMD adjustments

[PATCH 0/2] x86, microcode: AMD adjustments

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Hi,

this should take care of the recent addition of microcode revision to
/proc/cpuinfo for AMD too. Patches are against tip/x88/microcode and
available also on

git://amd64.org/linux/bp.git tip-ucode

Please pull,
thanks.

The following changes since commit ca8bc8dc044793ee4d59ff7fe40138eee27d0325:

  coretemp: Get microcode revision from cpu_data (2011-10-14 13:16:42 +0200)

are available in the git repository at:
  git://amd64.org/linux/bp.git tip-ucode

Borislav Petkov (2):
      x86, microcode: Correct microcode revision format
      x86, microcode, AMD: Add microcode revision to /proc/cpuinfo

 arch/x86/kernel/cpu/amd.c        |    4 ++++
 arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
 arch/x86/kernel/cpu/proc.c       |    2 +-
 arch/x86/kernel/microcode_amd.c  |    5 +++--
 4 files changed, 9 insertions(+), 4 deletions(-)

[PATCH 1/2] x86, microcode: Correct microcode revision format

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

506ed6b53e00ba303ad778122f08e1fca7cf5efb added microcode revision format
to /proc/cpuinfo and the MCE handler in decimal format but both AMD and
Intel patch levels are handled as hex numbers. Fix it.

Cc: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
 arch/x86/kernel/cpu/proc.c       |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 8af6fa4..ad8d897 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -221,7 +221,7 @@ static void print_mce(struct mce *m)
 	 * Note this output is parsed by external tools and old fields
 	 * should not be changed.
 	 */
-	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x microcode %u\n",
+	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x microcode %x\n",
 		m->cpuvendor, m->cpuid, m->time, m->socketid, m->apicid,
 		cpu_data(m->extcpu).microcode);
 
diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index 6254fda..14b2314 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -86,7 +86,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 	else
 		seq_printf(m, "stepping\t: unknown\n");
 	if (c->microcode)
-		seq_printf(m, "microcode\t: %u\n", c->microcode);
+		seq_printf(m, "microcode\t: 0x%x\n", c->microcode);
 
 	if (cpu_has(c, X86_FEATURE_TSC)) {
 		unsigned int freq = cpufreq_quick_get(cpu);
-- 
1.7.4.rc2

Re: [PATCH 1/2] x86, microcode: Correct microcode revision format

[David Rientjes]

On Mon, 17 Oct 2011, Borislav Petkov wrote:

> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 8af6fa4..ad8d897 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -221,7 +221,7 @@ static void print_mce(struct mce *m)
>  	 * Note this output is parsed by external tools and old fields
>  	 * should not be changed.
>  	 */
> -	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x microcode %u\n",
> +	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x microcode %x\n",
>  		m->cpuvendor, m->cpuid, m->time, m->socketid, m->apicid,
>  		cpu_data(m->extcpu).microcode);
>  

Any reason why this isn't prefixed with "0x"?

> diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
> index 6254fda..14b2314 100644
> --- a/arch/x86/kernel/cpu/proc.c
> +++ b/arch/x86/kernel/cpu/proc.c
> @@ -86,7 +86,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>  	else
>  		seq_printf(m, "stepping\t: unknown\n");
>  	if (c->microcode)
> -		seq_printf(m, "microcode\t: %u\n", c->microcode);
> +		seq_printf(m, "microcode\t: 0x%x\n", c->microcode);
>  
>  	if (cpu_has(c, X86_FEATURE_TSC)) {
>  		unsigned int freq = cpufreq_quick_get(cpu);

When here it is?

Re: [PATCH 1/2] x86, microcode: Correct microcode revision format

[Borislav Petkov]

On Tue, Oct 18, 2011 at 02:50:27AM -0400, David Rientjes wrote:
> On Mon, 17 Oct 2011, Borislav Petkov wrote:
> 
> > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> > index 8af6fa4..ad8d897 100644
> > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > @@ -221,7 +221,7 @@ static void print_mce(struct mce *m)
> >  	 * Note this output is parsed by external tools and old fields
> >  	 * should not be changed.
> >  	 */
> > -	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x microcode %u\n",
> > +	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x microcode %x\n",
> >  		m->cpuvendor, m->cpuid, m->time, m->socketid, m->apicid,
> >  		cpu_data(m->extcpu).microcode);
> >  
> 
> Any reason why this isn't prefixed with "0x"?

Well, no strong reason except that APIC is without '0x' and I leaned
towards the same for 'microcode'. And since this output format is legacy
and MCE stanzas are being parsed by scripts, keeping the format for new
fields sounded like the right thing to do, IMHO.

> 
> > diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
> > index 6254fda..14b2314 100644
> > --- a/arch/x86/kernel/cpu/proc.c
> > +++ b/arch/x86/kernel/cpu/proc.c
> > @@ -86,7 +86,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
> >  	else
> >  		seq_printf(m, "stepping\t: unknown\n");
> >  	if (c->microcode)
> > -		seq_printf(m, "microcode\t: %u\n", c->microcode);
> > +		seq_printf(m, "microcode\t: 0x%x\n", c->microcode);
> >  
> >  	if (cpu_has(c, X86_FEATURE_TSC)) {
> >  		unsigned int freq = cpufreq_quick_get(cpu);
> 
> When here it is?

This needed to explicitly state that it is a hex number because the rest
of the /proc/cpuinfo fields are decimals.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 1/2] x86, microcode: Correct microcode revision format

[David Rientjes]

On Tue, 18 Oct 2011, Borislav Petkov wrote:

> > > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> > > index 8af6fa4..ad8d897 100644
> > > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > > @@ -221,7 +221,7 @@ static void print_mce(struct mce *m)
> > >  	 * Note this output is parsed by external tools and old fields
> > >  	 * should not be changed.
> > >  	 */
> > > -	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x microcode %u\n",
> > > +	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x microcode %x\n",
> > >  		m->cpuvendor, m->cpuid, m->time, m->socketid, m->apicid,
> > >  		cpu_data(m->extcpu).microcode);
> > >  
> > 
> > Any reason why this isn't prefixed with "0x"?
> 
> Well, no strong reason except that APIC is without '0x' and I leaned
> towards the same for 'microcode'. And since this output format is legacy
> and MCE stanzas are being parsed by scripts, keeping the format for new
> fields sounded like the right thing to do, IMHO.
> 

Anytime there's a string that prints decimal, then hex, then decimal, then 
decimal, then hex, then hex, I think it's always better to include a 
prefix where it's not clear.  It's printed here without the prefix and in 
other places with the prefix, so I think it would be better to just be as 
explicit as possible.

And, the argument that scripts are parsing this is actually bogus since it 
would be expecting decimal there and you'd actually be doing them a favor 
by breaking if they can't handle the "0x" since you've changed it to hex.  
I know the comment says not to change old fields, but the microcode field 
hasn't hit Linus' tree yet, either.

Re: [PATCH 1/2] x86, microcode: Correct microcode revision format

[Borislav Petkov]

On Tue, Oct 18, 2011 at 04:14:05AM -0400, David Rientjes wrote:
> On Tue, 18 Oct 2011, Borislav Petkov wrote:
> 
> > > > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> > > > index 8af6fa4..ad8d897 100644
> > > > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > > > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > > > @@ -221,7 +221,7 @@ static void print_mce(struct mce *m)
> > > >  	 * Note this output is parsed by external tools and old fields
> > > >  	 * should not be changed.
> > > >  	 */
> > > > -	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x microcode %u\n",
> > > > +	pr_emerg(HW_ERR "PROCESSOR %u:%x TIME %llu SOCKET %u APIC %x microcode %x\n",
> > > >  		m->cpuvendor, m->cpuid, m->time, m->socketid, m->apicid,
> > > >  		cpu_data(m->extcpu).microcode);
> > > >  
> > > 
> > > Any reason why this isn't prefixed with "0x"?
> > 
> > Well, no strong reason except that APIC is without '0x' and I leaned
> > towards the same for 'microcode'. And since this output format is legacy
> > and MCE stanzas are being parsed by scripts, keeping the format for new
> > fields sounded like the right thing to do, IMHO.
> > 
> 
> Anytime there's a string that prints decimal, then hex, then decimal, then 
> decimal, then hex, then hex, I think it's always better to include a 
> prefix where it's not clear.  It's printed here without the prefix and in 
> other places with the prefix, so I think it would be better to just be as 
> explicit as possible.

Why do I need to be explicit in the MCE stanza? I'm fine with what
you're saying but I don't see a compelling reason why, sorry. Especially
since it was defined crappy to begin with.

> And, the argument that scripts are parsing this is actually bogus
> since it would be expecting decimal there and you'd actually be doing
> them a favor by breaking if they can't handle the "0x" since you've
> changed it to hex. I know the comment says not to change old fields,
> but the microcode field hasn't hit Linus' tree yet, either.

Right, either way I don't seem to care too much. The only thing I care
about is having ucode revision in hex in all places, with or without the
"0x" prefix.

So what do the others think, let's take a poll here. :-)

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 1/2] x86, microcode: Correct microcode revision format

[Andi Kleen]

On Mon, Oct 17, 2011 at 05:25:14PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> 506ed6b53e00ba303ad778122f08e1fca7cf5efb added microcode revision format
> to /proc/cpuinfo and the MCE handler in decimal format but both AMD and
> Intel patch levels are handled as hex numbers. Fix it.

Acked-by: Andi Kleen <ak@linux.intel.com>

BTW if you want microcode output on AMD you'll need to implement
code to fill the field in on those CPUs.

-Andi

Re: [PATCH 1/2] x86, microcode: Correct microcode revision format

[Borislav Petkov]

On Tue, Oct 18, 2011 at 11:13:16AM -0400, Andi Kleen wrote:
> On Mon, Oct 17, 2011 at 05:25:14PM +0200, Borislav Petkov wrote:
> > From: Borislav Petkov <borislav.petkov@amd.com>
> > 
> > 506ed6b53e00ba303ad778122f08e1fca7cf5efb added microcode revision format
> > to /proc/cpuinfo and the MCE handler in decimal format but both AMD and
> > Intel patch levels are handled as hex numbers. Fix it.
> 
> Acked-by: Andi Kleen <ak@linux.intel.com>
> 
> BTW if you want microcode output on AMD you'll need to implement
> code to fill the field in on those CPUs.

Yeah, a second patch does that already.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

[PATCH 2/2] x86, microcode, AMD: Add microcode revision to /proc/cpuinfo

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Enable microcode revision output for AMD after
506ed6b53e00ba303ad778122f08e1fca7cf5efb did it for Intel.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/amd.c       |    4 ++++
 arch/x86/kernel/microcode_amd.c |    5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index b13ed39..c193b8d 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -412,6 +412,8 @@ static void __cpuinit early_init_amd_mc(struct cpuinfo_x86 *c)
 
 static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 {
+	u32 dummy;
+
 	early_init_amd_mc(c);
 
 	/*
@@ -446,6 +448,8 @@ static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 	if (c != &boot_cpu_data)
 		return;
 
+	rdmsr(MSR_AMD64_PATCH_LEVEL, c->microcode, dummy);
+
 	if (cpu_has(c, X86_FEATURE_CONSTANT_TSC)) {
 
 		if (c->x86 > 0x10 ||
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 591be0e..d494799 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -74,14 +74,13 @@ static struct equiv_cpu_entry *equiv_cpu_table;
 static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
-	u32 dummy;
 
 	if (c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10) {
 		pr_warning("CPU%d: family %d not supported\n", cpu, c->x86);
 		return -1;
 	}
 
-	rdmsr(MSR_AMD64_PATCH_LEVEL, csig->rev, dummy);
+	csig->rev = c->microcode;
 	pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
 
 	return 0;
@@ -130,6 +129,7 @@ static int apply_microcode_amd(int cpu)
 	int cpu_num = raw_smp_processor_id();
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;
 	struct microcode_amd *mc_amd = uci->mc;
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
 
 	/* We should bind the task to the CPU */
 	BUG_ON(cpu_num != cpu);
@@ -150,6 +150,7 @@ static int apply_microcode_amd(int cpu)
 
 	pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
 	uci->cpu_sig.rev = rev;
+	c->microcode = rev;
 
 	return 0;
 }
-- 
1.7.4.rc2

Re: [PATCH 2/2] x86, microcode, AMD: Add microcode revision to /proc/cpuinfo

[Ingo Molnar]

* Borislav Petkov <bp@amd64.org> wrote:

> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> Enable microcode revision output for AMD after
> 506ed6b53e00ba303ad778122f08e1fca7cf5efb did it for Intel.

The standard way we mention commits in changelogs is:

  Enable microcode revision output for AMD after commit 506ed6b53e00 
  ("x86, intel: Output microcode revision in /proc/cpuinfo") did it 
  for Intel.

... because humans are pretty bad at transforming sha1's to the real 
commit title when reading such text.

Please fix the changelog for the first commit as well.

The changes look good otherwise.

Thanks,

	Ingo

Re: [PATCH 2/2] x86, microcode, AMD: Add microcode revision to /proc/cpuinfo

[Borislav Petkov]

On Tue, Oct 18, 2011 at 02:25:10AM -0400, Ingo Molnar wrote:
> The standard way we mention commits in changelogs is:
> 
>   Enable microcode revision output for AMD after commit 506ed6b53e00 
>   ("x86, intel: Output microcode revision in /proc/cpuinfo") did it 
>   for Intel.
> 
> ... because humans are pretty bad at transforming sha1's to the real 
> commit title when reading such text.

Why, I thought transforming sha1's to commit messages is one of the job
requirements of serious kernel hackers.

:-)

> Please fix the changelog for the first commit as well.

Done, git branch at

git://amd64.org/linux/bp.git tip-ucode

updated.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH 2/2] x86, microcode, AMD: Add microcode revision to /proc/cpuinfo

[Ingo Molnar]

* Borislav Petkov <bp@amd64.org> wrote:

> On Tue, Oct 18, 2011 at 02:25:10AM -0400, Ingo Molnar wrote:
> > The standard way we mention commits in changelogs is:
> > 
> >   Enable microcode revision output for AMD after commit 506ed6b53e00 
> >   ("x86, intel: Output microcode revision in /proc/cpuinfo") did it 
> >   for Intel.
> > 
> > ... because humans are pretty bad at transforming sha1's to the real 
> > commit title when reading such text.
> 
> Why, I thought transforming sha1's to commit messages is one of the job
> requirements of serious kernel hackers.
> 
> :-)

But they are not the only ones reading changelogs ;-)

> > Please fix the changelog for the first commit as well.
> 
> Done, git branch at
> 
> git://amd64.org/linux/bp.git tip-ucode
> 
> updated.

Pulled, thanks,

	Ingo