[PATCH 1/3] x86: microcode: report if CPU has up-to-date microcode

[PATCH 1/3] x86: microcode: report if CPU has up-to-date microcode

[Denys Vlasenko]

Before this change, successful microcode uploads clearly
indicate that it was done:

microcode: CPU1 sig=0x206a7, pf=0x10, revision=0x1a
microcode: CPU1 updated to revision 0x29, date = 2013-06-12

whereas if microcode was not uploaded, it is not clear why:

microcode: CPU1 sig=0x206a7, pf=0x10, revision=0x29
(nothing more)

So, what was it? No microcode file? No microcode for this sig/pf?
CPU already has microcode with this (or newer) revision?

In practice, it means that I need to ask people to provide me
with more information ("do you have microcode package installed?
which version is it?" etc).

This change adds a message which covers "CPU is up-to-date" and
"no microcode for your CPU in this file" cases:

microcode: CPU1 sig=0x206a7, pf=0x10, revision=0x29
microcode: CPU rev 0x29 is same or newer than 0x29 in microcode data

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
---
 arch/x86/include/asm/microcode_intel.h      |  2 +-
 arch/x86/kernel/cpu/microcode/intel.c       |  4 ++--
 arch/x86/kernel/cpu/microcode/intel_early.c |  2 +-
 arch/x86/kernel/cpu/microcode/intel_lib.c   | 18 +++++++++++++++---
 4 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 9067166..f64ccfd 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -57,7 +57,7 @@ struct extended_sigtable {
 #define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)
 
 extern int
-get_matching_microcode(unsigned int csig, int cpf, void *mc, int rev);
+get_matching_microcode(unsigned int csig, int cpf, void *mc, int rev, bool report_old);
 extern int microcode_sanity_check(void *mc, int print_err);
 extern int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev);
 extern int
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index a276fa7..ef8b17c 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -124,7 +124,7 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
 	cpf = cpu_sig.pf;
 	crev = cpu_sig.rev;
 
-	return get_matching_microcode(csig, cpf, mc_intel, crev);
+	return get_matching_microcode(csig, cpf, mc_intel, crev, /*report_old:*/ 1);
 }
 
 int apply_microcode(int cpu)
@@ -221,7 +221,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
 
 		csig = uci->cpu_sig.sig;
 		cpf = uci->cpu_sig.pf;
-		if (get_matching_microcode(csig, cpf, mc, new_rev)) {
+		if (get_matching_microcode(csig, cpf, mc, new_rev, /*report_old:*/ 1)) {
 			vfree(new_mc);
 			new_rev = mc_header.rev;
 			new_mc  = mc;
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 18f7391..3a2ad0a 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -53,7 +53,7 @@ generic_load_microcode_early(struct microcode_intel **mc_saved_p,
 
 		mc_header = (struct microcode_header_intel *)ucode_ptr;
 		mc_size = get_totalsize(mc_header);
-		if (get_matching_microcode(csig, cpf, ucode_ptr, new_rev)) {
+		if (get_matching_microcode(csig, cpf, ucode_ptr, new_rev, /*report_old:*/ 0)) {
 			new_rev = mc_header->rev;
 			new_mc  = ucode_ptr;
 		}
diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index ce69320..e8c02cb 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -162,13 +162,25 @@ int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev)
  * return 0 - no update found
  * return 1 - found update
  */
-int get_matching_microcode(unsigned int csig, int cpf, void *mc, int rev)
+int get_matching_microcode(unsigned int csig, int cpf, void *mc, int rev, bool report_old)
 {
 	struct microcode_header_intel *mc_header = mc;
 
-	if (!update_match_revision(mc_header, rev))
+	if (!update_match_revision(mc_header, rev)) {
+		if (report_old)
+			pr_info("microcode: CPU rev 0x%x is same or newer"
+				" than 0x%x in microcode data\n",
+				rev, mc_header->rev);
 		return 0;
+	}
+
+	if (!get_matching_sig(csig, cpf, mc, rev)) {
+		if (report_old)
+			pr_info("microcode: no microcode with"
+				" sig=0x%x, pf=0x%x found\n", csig, cpf);
+		return 0;
+	}
 
-	return get_matching_sig(csig, cpf, mc, rev);
+	return 1;
 }
 EXPORT_SYMBOL_GPL(get_matching_microcode);
-- 
1.8.1.4

[PATCH 2/3] x86: microcode: report if no microcode file was found

[Denys Vlasenko]

Before this change, successful microcode uploads clearly
indicate that it was done:

microcode: CPU1 sig=0x206a7, pf=0x10, revision=0x1a
microcode: CPU1 updated to revision 0x29, date = 2013-06-12

whereas if microcode was not uploaded, it is not clear why:

microcode: CPU1 sig=0x206a7, pf=0x10, revision=0x29
(nothing more)

So, what was it? No microcode file? No microcode for this sig/pf?
CPU already has microcode with this (or newer) revision?

In practice, it means that I heed to ask people to provide me
with more information ("do you have microcode package installed?
which version is it?").

This change upgrades existing message from pr_debug to pr_info,
which covers "no microcode file" case:

microcode: CPU1 sig=0x206a7, pf=0x10, revision=0x29
microcode: data file intel-ucode/06-2a-07 load failed

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
---
 arch/x86/kernel/cpu/microcode/intel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index ef8b17c..9ce22b0 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -279,7 +279,7 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device,
 		c->x86, c->x86_model, c->x86_mask);
 
 	if (request_firmware_direct(&firmware, name, device)) {
-		pr_debug("data file %s load failed\n", name);
+		pr_info("data file %s load failed\n", name);
 		return UCODE_NFOUND;
 	}
 
-- 
1.8.1.4

[PATCH 3/3] x86: microcode: remove unused parameter and redundant variable

[Denys Vlasenko]

Two trivial cleanups:

get_matching_sig()'s rev parameter is unused and can be removed.

In apply_microcode(), cpu == cpu_num always. Merge them. Move BUG check
before all other operations so that function call does not interfere
with register scheduling, saving a few bytes of code.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
---
 arch/x86/include/asm/microcode_intel.h      |  2 +-
 arch/x86/kernel/cpu/microcode/intel.c       | 16 ++++++++--------
 arch/x86/kernel/cpu/microcode/intel_early.c |  2 +-
 arch/x86/kernel/cpu/microcode/intel_lib.c   |  4 ++--
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index f64ccfd..e27eba4 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -59,7 +59,7 @@ struct extended_sigtable {
 extern int
 get_matching_microcode(unsigned int csig, int cpf, void *mc, int rev, bool report_old);
 extern int microcode_sanity_check(void *mc, int print_err);
-extern int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev);
+extern int get_matching_sig(unsigned int csig, int cpf, void *mc);
 extern int
 update_match_revision(struct microcode_header_intel *mc_header, int rev);
 
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 9ce22b0..a37f179 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -127,19 +127,19 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
 	return get_matching_microcode(csig, cpf, mc_intel, crev, /*report_old:*/ 1);
 }
 
-int apply_microcode(int cpu)
+int apply_microcode(int cpu_num)
 {
 	struct microcode_intel *mc_intel;
 	struct ucode_cpu_info *uci;
 	unsigned int val[2];
-	int cpu_num = raw_smp_processor_id();
-	struct cpuinfo_x86 *c = &cpu_data(cpu_num);
-
-	uci = ucode_cpu_info + cpu;
-	mc_intel = uci->mc;
+	struct cpuinfo_x86 *c;
 
 	/* We should bind the task to the CPU */
-	BUG_ON(cpu_num != cpu);
+	BUG_ON(cpu_num != raw_smp_processor_id());
+
+	c = &cpu_data(cpu_num);
+	uci = ucode_cpu_info + cpu_num;
+	mc_intel = uci->mc;
 
 	if (mc_intel == NULL)
 		return 0;
@@ -149,7 +149,7 @@ int apply_microcode(int cpu)
 	 * microcode patch in mc_intel when it is newer than the one on this
 	 * CPU.
 	 */
-	if (get_matching_mc(mc_intel, cpu) == 0)
+	if (get_matching_mc(mc_intel, cpu_num) == 0)
 		return 0;
 
 	/* write microcode via MSR 0x79 */
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 3a2ad0a..14b120b 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -276,7 +276,7 @@ static void _save_mc(struct microcode_intel **mc_saved, u8 *ucode_ptr,
 		pf = mc_saved_header->pf;
 		new_rev = mc_header->rev;
 
-		if (get_matching_sig(sig, pf, ucode_ptr, new_rev)) {
+		if (get_matching_sig(sig, pf, ucode_ptr)) {
 			found = 1;
 			if (update_match_revision(mc_header, new_rev)) {
 				/*
diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index e8c02cb..eeec94b 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -131,7 +131,7 @@ EXPORT_SYMBOL_GPL(microcode_sanity_check);
  * return 0 - no update found
  * return 1 - found update
  */
-int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev)
+int get_matching_sig(unsigned int csig, int cpf, void *mc)
 {
 	struct microcode_header_intel *mc_header = mc;
 	struct extended_sigtable *ext_header;
@@ -174,7 +174,7 @@ int get_matching_microcode(unsigned int csig, int cpf, void *mc, int rev, bool r
 		return 0;
 	}
 
-	if (!get_matching_sig(csig, cpf, mc, rev)) {
+	if (!get_matching_sig(csig, cpf, mc)) {
 		if (report_old)
 			pr_info("microcode: no microcode with"
 				" sig=0x%x, pf=0x%x found\n", csig, cpf);
-- 
1.8.1.4

Re: [PATCH 1/3] x86: microcode: report if CPU has up-to-date microcode

[Borislav Petkov]

On Sun, Mar 30, 2014 at 04:09:32PM +0200, Denys Vlasenko wrote:
> Before this change, successful microcode uploads clearly
> indicate that it was done:
> 
> microcode: CPU1 sig=0x206a7, pf=0x10, revision=0x1a
> microcode: CPU1 updated to revision 0x29, date = 2013-06-12
> 
> whereas if microcode was not uploaded, it is not clear why:
> 
> microcode: CPU1 sig=0x206a7, pf=0x10, revision=0x29
> (nothing more)
> 
> So, what was it? No microcode file? No microcode for this sig/pf?
> CPU already has microcode with this (or newer) revision?
> 
> In practice, it means that I need to ask people to provide me
> with more information ("do you have microcode package installed?
> which version is it?" etc).

Well, hmm.

First of all, microcode version is in /proc/cpuinfo. Issuing the reason
why microcode wasn't loaded in dmesg and then the dmesg ring buffer
wraps around doesn't make a lot of sense, in my not really too humble
opinion.

So, for debugging cases, you're probably going to have to ask for
/proc/cpuinfo anyway and then check the CPU vendor's site for newer
microcode packages and compare, instead of relying that dmesg still
contains that info.

Besides, experience shows that dmesg messages like those tend to spook
users and we don't want that :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 2/3] x86: microcode: report if no microcode file was found

[Borislav Petkov]

On Sun, Mar 30, 2014 at 04:09:33PM +0200, Denys Vlasenko wrote:
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index ef8b17c..9ce22b0 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -279,7 +279,7 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device,
>  		c->x86, c->x86_model, c->x86_mask);
>  
>  	if (request_firmware_direct(&firmware, name, device)) {

request_firmware_direct() already has dev_dbg() error messages down its
path.

> -		pr_debug("data file %s load failed\n", name);
> +		pr_info("data file %s load failed\n", name);

Also might spook users unnecessarily.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 1/3] x86: microcode: report if CPU has up-to-date microcode

[Denys Vlasenko]

On 03/31/2014 06:23 PM, Borislav Petkov wrote:
> On Sun, Mar 30, 2014 at 04:09:32PM +0200, Denys Vlasenko wrote:
>> Before this change, successful microcode uploads clearly
>> indicate that it was done:
>>
>> microcode: CPU1 sig=0x206a7, pf=0x10, revision=0x1a
>> microcode: CPU1 updated to revision 0x29, date = 2013-06-12
>>
>> whereas if microcode was not uploaded, it is not clear why:
>>
>> microcode: CPU1 sig=0x206a7, pf=0x10, revision=0x29
>> (nothing more)
>>
>> So, what was it? No microcode file? No microcode for this sig/pf?
>> CPU already has microcode with this (or newer) revision?
>>
>> In practice, it means that I need to ask people to provide me
>> with more information ("do you have microcode package installed?
>> which version is it?" etc).
> 
> First of all, microcode version is in /proc/cpuinfo.

What prompted me to create this patch is a bunch of vmcores
from people having mysterious crashes.

Basically, I am in a real-world scenario where I have only vmcore
from somebody else. I have no /proc/cpuinfo.

Every bit of information I don't have at a minimum incurs email
round-trip delay.

Eventually I did manage to figure out what version of microcode
my users had (they did have old one), but it took some time.

> Issuing the reason
> why microcode wasn't loaded in dmesg and then the dmesg ring buffer
> wraps around doesn't make a lot of sense, in my not really too humble
> opinion.

You are correct, the lack of boot-time dmesg is a problem for
post-mortem vmcore analysis in general.

I contemplate creating a patch to optionally save it.

> Besides, experience shows that dmesg messages like those tend to spook
> users and we don't want that :-)

That is not a bad thing: if my users would have been spooked that way,
maybe they'd install a newer microcode. Or newer BIOS with new microcode
- they did not do that either, despite it being available
from the manufacturer for two years already.

-- 
vda

Re: [PATCH 1/3] x86: microcode: report if CPU has up-to-date microcode

[Borislav Petkov]

On Mon, Mar 31, 2014 at 07:08:08PM +0200, Denys Vlasenko wrote:
> What prompted me to create this patch is a bunch of vmcores
> from people having mysterious crashes.
> 
> Basically, I am in a real-world scenario where I have only vmcore
> from somebody else. I have no /proc/cpuinfo.
> 
> Every bit of information I don't have at a minimum incurs email
> round-trip delay.
> 
> Eventually I did manage to figure out what version of microcode
> my users had (they did have old one), but it took some time.

Out of curiosity: the old microcode version wasn't the culprit for the
bug, was it?

> You are correct, the lack of boot-time dmesg is a problem for
> post-mortem vmcore analysis in general.
> 
> I contemplate creating a patch to optionally save it.

Btw, can vmcore stash ucode version somewhere too, as part of the data
dump it saves? Or even the whole /proc/cpuinfo?

> That is not a bad thing: if my users would have been spooked that
> way, maybe they'd install a newer microcode. Or newer BIOS with new
> microcode - they did not do that either, despite it being available
> from the manufacturer for two years already.

Right, ok. So in thinking about this more. How about we drop the "bool
report_old" machinery and add those printks as debug printks? And by
that I mean,

	printk(KERN_DEBUG...

and not all that other pr_debug() gunk?

This way, you need to boot with "debug" or "ignore_loglevel" on the
cmdline and they will get issued.

My concern is still with spooked users who'll come and say, my machine says:

+			pr_info("microcode: CPU rev 0x%x is same or newer"
+				" than 0x%x in microcode data\n",

Does that mean, I need to go and replace my CPU?

The usual case is that microcode gets upgraded with system upgrade as a
intel-ucode or amd-ucode or whatever package, so people upgrading their
distros will always get the latest ucode anyway.

Hmmm.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--