x86, microcode: AMD adjustments

PATCH In review Borislav Petkov 4 participants 12 messages Oct 17, 2011 – Oct 23, 2011 x86

lore

Share & tools

lore

Borislav Petkov

author

2011-10-17 15:25 UTC #0 lore raw

cover x86, microcode: AMD adjustments

From: Borislav Petkov <[email protected]>

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(-)

Borislav Petkov

author

2011-10-17 15:25 UTC #1 lore raw

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

From: Borislav Petkov <[email protected]>

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 <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

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

arch/x86/kernel/cpu/mcheck/mce.c +1 -1

index 8af6fa4..ad8d897 100644
@@ -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);

arch/x86/kernel/cpu/proc.c +1 -1

index 6254fda..14b2314 100644
@@ -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);

Signature · 1 line

1.7.4.rc2

David Rientjes

2011-10-18 06:50 UTC #2 lore raw

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

arch/x86/kernel/cpu/mcheck/mce.c static void print_mce(struct mce *m) · line 227

Quoted 13 lines · Borislav Petkov

arch/x86/kernel/cpu/mcheck/mce.c +1 -1

index 8af6fa4..ad8d897 100644
@@ -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"?

arch/x86/kernel/cpu/proc.c static int show_cpuinfo(struct seq_file *m, void *v) · line 92

Quoted 13 lines

arch/x86/kernel/cpu/proc.c +1 -1

index 6254fda..14b2314 100644
@@ -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?

Borislav Petkov

author

2011-10-18 07:52 UTC #3 lore raw

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

Quoted 17 lines · David Rientjes

arch/x86/kernel/cpu/mcheck/mce.c static void print_mce(struct mce *m) · line 227

Quoted 13 lines · Borislav Petkov

arch/x86/kernel/cpu/mcheck/mce.c +1 -1

index 8af6fa4..ad8d897 100644
@@ -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.

Quoted 12 lines

arch/x86/kernel/cpu/proc.c +1 -1

index 6254fda..14b2314 100644
@@ -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);

Quoted 4 lines

arch/x86/kernel/cpu/proc.c static int show_cpuinfo(struct seq_file *m, void *v) · line 92
Quoted 2 lines
arch/x86/kernel/cpu/proc.c +0 -0
 	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.

Signature · 8 lines

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

David Rientjes

2011-10-18 08:14 UTC #4 lore raw

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

Quoted 16 lines · Borislav Petkov

Quoted 7 lines
Quoted 7 lines
arch/x86/kernel/cpu/mcheck/mce.c +0 -0
index 8af6fa4..ad8d897 100644
@@ -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.
Quoted 8 lines
arch/x86/kernel/cpu/mcheck/mce.c static void print_mce(struct mce *m) · line 227
Quoted 6 lines
arch/x86/kernel/cpu/mcheck/mce.c +1 -1
 	 */
-	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"?

Quoted 5 lines

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.

Borislav Petkov

author

2011-10-18 09:48 UTC #5 lore raw

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

Quoted 24 lines · David Rientjes

Quoted 16 lines · Borislav Petkov
Quoted 7 lines
Quoted 7 lines
arch/x86/kernel/cpu/mcheck/mce.c +0 -0
index 8af6fa4..ad8d897 100644
@@ -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.
Quoted 8 lines
arch/x86/kernel/cpu/mcheck/mce.c static void print_mce(struct mce *m) · line 227
Quoted 6 lines
arch/x86/kernel/cpu/mcheck/mce.c +1 -1
 	 */
-	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"?
Quoted 5 lines
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.

Quoted 5 lines

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.

Quoted 5 lines

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

Signature · 8 lines

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

Andi Kleen

Acked-by on v1 1/2

2011-10-18 15:13 UTC #6 lore raw

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

In reply to Borislav Petkov

Quoted 5 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>
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 <[email protected]>

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

-Andi

Borislav Petkov

author

2011-10-18 15:49 UTC #7 lore raw

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

Quoted 11 lines · Andi Kleen

Quoted 5 lines · Borislav Petkov
From: Borislav Petkov <[email protected]>
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 <[email protected]>
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.

Signature · 8 lines

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

Borislav Petkov

author

2011-10-17 15:25 UTC #8 lore raw

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

In reply to Borislav Petkov

From: Borislav Petkov <[email protected]>

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

Signed-off-by Borislav Petkov <[email protected]>

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

arch/x86/kernel/cpu/amd.c +4 -0

index b13ed39..c193b8d 100644
@@ -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 ||

arch/x86/kernel/microcode_amd.c +3 -2

index 591be0e..d494799 100644
@@ -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;
 }

Signature · 1 line

1.7.4.rc2

Ingo Molnar

2011-10-18 06:26 UTC #9 lore raw

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

Quoted 4 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>
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

Borislav Petkov

author

2011-10-19 14:21 UTC #10 lore raw

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

Quoted 8 lines · Ingo Molnar

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.

:-)

Quoted 1 line

Please fix the changelog for the first commit as well.

Done, git branch at

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

updated.

Thanks.

Signature · 8 lines

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

Ingo Molnar

2011-10-23 11:49 UTC #11 lore raw

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

Quoted 7 lines · Borislav Petkov

Quoted 6 lines · Ingo Molnar
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.

Quoted 7 lines

Quoted 2 lines
... 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 ;-)

Quoted 7 lines

Quoted 1 line
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