x86/microcode: Some cleanups and fixes for microcode

There is a pr_info() to dump information about newly loaded microcode.

The code intends this pr_info() to be just once, but the check to ensure is racy. Unfortunately this happens quite often in with this new change resulting in multiple redundant prints on the console.

microcode_init()->schedule_on_each_cpu(setup_online_cpu)->collect_cpu_info

[   33.688639] microcode: sig=0x50654, pf=0x80, revision=0x2006e05
[   33.688659] microcode: sig=0x50654, pf=0x80, revision=0x2006e05
[   33.688660] microcode: sig=0x50654, pf=0x80, revision=0x2006e05

There is already a pr_info() in microcode/core.c as shown below:

microcode: Reload completed, microcode revision: 0x2b000041 -> 0x2b000070

The sig and pf aren't that useful to end user, they are available via

/proc/cpuinfo and this never changes between microcode loads.

Remove the redundant pr_info() and the racy single print checks. This removes the race entirely, zap the duplicated pr_info() spam and simplify the code.

Ashok!

Quoted 1 line · Ashok Raj

There is a pr_info() to dump information about newly loaded microcode.

There... Somewhere, right?

Quoted 4 lines

The code intends this pr_info() to be just once, but the check to ensure is racy. Unfortunately this happens quite often in with this new change resulting in multiple redundant prints on the console.

-ENOPARSE. Can you try to express that in coherent sentences please?

Quoted 10 lines

microcode_init()->schedule_on_each_cpu(setup_online_cpu)->collect_cpu_info

[   33.688639] microcode: sig=0x50654, pf=0x80, revision=0x2006e05
[   33.688659] microcode: sig=0x50654, pf=0x80, revision=0x2006e05
[   33.688660] microcode: sig=0x50654, pf=0x80, revision=0x2006e05

There is already a pr_info() in microcode/core.c as shown below:

microcode: Reload completed, microcode revision: 0x2b000041 -> 0x2b000070

There are quite some pr_info()'s in microcode/core.c...

$function_name() prints the new and the previous microcode revision once when the load has completed:

microcode: Reload completed, microcode revision: 0x2b000041 -> 0x2b000070

Hmm?

Quoted 2 lines

The sig and pf aren't that useful to end user, they are available via

The sig and pf ?!? Come on, you really can do better.

Quoted 2 lines

/proc/cpuinfo and this never changes between microcode loads.

Quoted 3 lines

Remove the redundant pr_info() and the racy single print checks. This removes the race entirely, zap the duplicated pr_info() spam and simplify the code.

The last sentence does not qualify as coherent either.

Other than that. Nice cleanup.

This code in collect_cpu_info() simply checks with a static variable "prev", but when multiple CPUs are running this in parallel it is racy and we notice

Microcode loading can fail. This happens today when handling mixed steppings. But it can also happen for other reasons such as corrupted image, Security Version Number (SVN) preventing anti-rollback, dependencies on BIOS loaded microcode image for some capabilities.

When the microcode loading fails, the kernel will quietly hang at boot. This has been observed by end users (Links below) who had to revert their microcode packages in order to boot again.

The hang is due to an infinite retry loop. The retries were in place to support systems with mixed steppings. Now that mixed steppings are no longer supported, there is only one microcode image at a time. Any retries will simply reattempt to apply the same image over and over without making progress.

Some possible past bugs that could be due to this bug are below.

There is no direct evidence that these end user issues were caused by this retry loop. However, the early boot hangs along with reverting the microcode update workaround provide strong circumstantial evidence to support the theory that they are linked.

Remove the retry loop and only attempt to apply microcode once.

Link https://bugs.launchpad.net/ubuntu/+source/intel-microcode/+bug/1911959

Link https://forums.linuxmint.com/viewtopic.php?p=1827032#1827032

Link https://askubuntu.com/questions/1291486/boot-crash-after-latest-update-of-intel-microcode-nov-11-2020

Fixes 06b8534cb728 ("x86/microcode: Rework microcode loading")

Cc [email protected]

Signed-off-by Ashok Raj <[email protected]>

Quoted 15 lines · Ashok Raj

Microcode loading can fail. This happens today when handling mixed steppings. But it can also happen for other reasons such as corrupted image, Security Version Number (SVN) preventing anti-rollback, dependencies on BIOS loaded microcode image for some capabilities.

When the microcode loading fails, the kernel will quietly hang at boot. This has been observed by end users (Links below) who had to revert their microcode packages in order to boot again.

The hang is due to an infinite retry loop. The retries were in place to support systems with mixed steppings. Now that mixed steppings are no longer supported, there is only one microcode image at a time. Any retries will simply reattempt to apply the same image over and over without making progress.

Quoted 8 lines

Some possible past bugs that could be due to this bug are below.

There is no direct evidence that these end user issues were caused by this retry loop. However, the early boot hangs along with reverting the microcode update workaround provide strong circumstantial evidence to support the theory that they are linked.

Remove the retry loop and only attempt to apply microcode once.

Very concise and informative changelog. See, you can do it :)

Quoted 6 lines

Link https://bugs.launchpad.net/ubuntu/+source/intel-microcode/+bug/1911959

Link https://forums.linuxmint.com/viewtopic.php?p=1827032#1827032

Link https://askubuntu.com/questions/1291486/boot-crash-after-latest-update-of-intel-microcode-nov-11-2020

Fixes 06b8534cb728 ("x86/microcode: Rework microcode loading")

Cc [email protected]

Signed-off-by Ashok Raj <[email protected]>

Nit: Can you order the tags according to the tip documentation next time
please?

Reviewed-by Thomas Gleixner <[email protected]>

arch/x86/kernel/cpu/microcode/intel.c void load_ucode_intel_ap(void) · line 507

Quoted 8 lines · Ashok Raj

arch/x86/kernel/cpu/microcode/intel.c +1 -7

-
-	if (apply_microcode_early(&uci, true)) {
-		/* Mixed-silicon system? Try to refetch the proper patch: */
-		*iup = NULL;
-
-		goto reget;
-	}
+	apply_microcode_early(&uci, true);

After this change, none of the callers of apply_microcode_early() check the return code.

In future, do we expect callers to care about the return code? The rest patches in this series don't seem to suggest so. Also, the expected error printing happens in the function itself.

Should the return type for apply_microcode_early() be changed to void (in a follow-up patch)?

There is no direct evidence that these end user issues were caused by this retry loop. However, the early boot hangs along with reverting the microcode update workaround provide strong circumstantial evidence to support the theory that they are linked.

Quoted 4 lines · Ashok Raj

There is no direct evidence that these end user issues were caused by this retry loop. However, the early boot hangs along with reverting the microcode update workaround provide strong circumstantial evidence to support the theory that they are linked.

A "circumstantial" reason for why something "might" be broken has no place in a commit message.

If you still wanna chase this and *actually* give me a sane, comprehensible reason of why this could cause an endless loop with officially released microcode, then I'm willing to listen.

Otherwise, I'll apply this:

microcode_check() is only called from microcode/core.c. Move it and make it static to prepare for upcoming fix of false negative when checking CPU features after a microcode update. Also move get_cpu_cap() to processor.h for general use outside of kernel/cpu.h

No functional change.

Suggested-by Alison Schofield <[email protected]>

Signed-off-by Ashok Raj <[email protected]>

microcode_check() is only called from microcode/core.c. Move it and make it static to prepare for upcoming fix of false negative when checking CPU features after a microcode update.

Quoted 3 lines · Ashok Raj

microcode_check() is only called from microcode/core.c. Move it and make it static to prepare for upcoming fix of false negative when checking CPU features after a microcode update.

Should we use this opportunity to also make the function name a bit more descriptive? microcode_check() seems very ambiguous to a first time reader.

arch/x86/kernel/cpu/microcode/core.c static int __reload_late(void *info) · line 439

Quoted 6 lines

arch/x86/kernel/cpu/microcode/core.c +6 -0

+/*
+ * The microcode loader calls this upon late microcode load to recheck features,
+ * only when microcode has been updated. Caller holds microcode_mutex and CPU
+ * hotplug lock.
+ */
+static void microcode_check(void)

How about, microcode_recheck_features() or simply recheck_features() since it is static now?

microcode_check() is only called from microcode/core.c. Move it and make it static to prepare for upcoming fix of false negative when checking CPU features after a microcode update.

Quoted 3 lines · Ashok Raj

microcode_check() is only called from microcode/core.c. Move it and make it static to prepare for upcoming fix of false negative when checking CPU features after a microcode update.

So this function is there in cpu/common.c because it uses CPU facilities like cpuinfo_x86 and get_cpu_cap() so the logical place was there. So that I don't have to export a bunch of things but rather have the microcode loader call into it only.

Your next patch is using more of those CPU-specific facilities so "bleeding" them into the microcode loader looks like the wrong way around.

get_cpu_cap() deals with all those c->x86_capability arrays and other functions which do that, should be there too.

I was trying to move this similar to how x86_read_arch_cap_msr() moved from x86/kernel/cpu/cpu.h -> asm/cpu.h.

Keeping the usage local since there is just one caller to microcode_check() but there are other users of get_cpu_cap() like in arch/x86/xen/enlighten_pv.c which seems to be reaching out to ../kernel/cpu/cpu.h.

That said, what you say also makes sense. I'm fine with what you decide how this should look.

The kernel caches features about each CPU's features at boot in an x86_capability[] structure. The microcode update takes one snapshot and compares it with the saved copy at boot.

However, the capabilities in the boot copy can be turned off as a result of certain command line parameters or configuration restrictions. This can cause a mismatch when comparing the values before and after the microcode update.

microcode_check() is called after an update to report any previously cached CPUID bits might have changed due to the update.

Ignore the capabilities recorded at boot. Take a new snapshot before the update and compare with a snapshot after the update to eliminate the false warning.

arch/x86/kernel/cpu/microcode/core.c +24 -9

+static void copy_cpu_caps(struct cpuinfo_x86 *info)
+{
+	/* Reload CPUID max function as it might've changed. */
+	info->cpuid_level = cpuid_eax(0);
+
+	/*
+	 * Copy all capability leafs to pick up the synthetic ones so that
+	 * memcmp() below doesn't fail on that. The ones coming from CPUID will
+	 * get overwritten in get_cpu_cap().
+	 */
+	memcpy(info->x86_capability, &boot_cpu_data.x86_capability,
+	       sizeof(info->x86_capability));
+
+	get_cpu_cap(info);
+}
+
 /*
  * The microcode loader calls this upon late microcode load to recheck features,
  * only when microcode has been updated. Caller holds microcode_mutex and CPU
  * hotplug lock.
  */
-static void microcode_check(void)
+static void microcode_check(struct cpuinfo_x86 *orig)
 {
 	struct cpuinfo_x86 info;

@@ -446,15 +462,13 @@ static void microcode_check(void)
 	info.cpuid_level = cpuid_eax(0);
 
 	/*
-	 * Copy all capability leafs to pick up the synthetic ones so that
-	 * memcmp() below doesn't fail on that. The ones coming from CPUID will
-	 * get overwritten in get_cpu_cap().
-	 */
-	memcpy(&info.x86_capability, &boot_cpu_data.x86_capability, sizeof(info.x86_capability));
+	* Copy all capability leafs to pick up the synthetic ones so that
+	* memcmp() below doesn't fail on that. The ones coming from CPUID will
+	* get overwritten in get_cpu_cap().
+	*/
+	copy_cpu_caps(&info);
 
-	get_cpu_cap(&info);
-
-	if (!memcmp(&info.x86_capability, &boot_cpu_data.x86_capability,
+	if (!memcmp(&info.x86_capability, &orig->x86_capability,
 		    sizeof(info.x86_capability)))
 		return;
 

@@ -469,6 +483,7 @@ static void microcode_check(void)
 static int microcode_reload_late(void)
 {
 	int old = boot_cpu_data.microcode, ret;
+	struct cpuinfo_x86 info;
 
 	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
 	pr_err("You should switch to early loading, if possible.\n");

Quoted 9 lines · Ashok Raj

The kernel caches features about each CPU's features at boot in an x86_capability[] structure. The microcode update takes one snapshot and compares it with the saved copy at boot.

However, the capabilities in the boot copy can be turned off as a result of certain command line parameters or configuration restrictions. This can cause a mismatch when comparing the values before and after the microcode update.

Quoted 6 lines

microcode_check() is called after an update to report any previously cached CPUID bits might have changed due to the update.

Ignore the capabilities recorded at boot. Take a new snapshot before the update and compare with a snapshot after the update to eliminate the false warning.

Makes sense.

Quoted 60 lines

(quoted diff) +24 -9

+static void copy_cpu_caps(struct cpuinfo_x86 *info)
+{
+	/* Reload CPUID max function as it might've changed. */
+	info->cpuid_level = cpuid_eax(0);
+
+	/*
+	 * Copy all capability leafs to pick up the synthetic ones so that
+	 * memcmp() below doesn't fail on that. The ones coming from CPUID will
+	 * get overwritten in get_cpu_cap().
+	 */
+	memcpy(info->x86_capability, &boot_cpu_data.x86_capability,
+	       sizeof(info->x86_capability));
+
+	get_cpu_cap(info);
+}
+
 /*
  * The microcode loader calls this upon late microcode load to recheck features,
  * only when microcode has been updated. Caller holds microcode_mutex and CPU
  * hotplug lock.
  */
-static void microcode_check(void)
+static void microcode_check(struct cpuinfo_x86 *orig)
 {
 	struct cpuinfo_x86 info;

@@ -446,15 +462,13 @@ static void microcode_check(void)
 	info.cpuid_level = cpuid_eax(0);
 
 	/*
-	 * Copy all capability leafs to pick up the synthetic ones so that
-	 * memcmp() below doesn't fail on that. The ones coming from CPUID will
-	 * get overwritten in get_cpu_cap().
-	 */
-	memcpy(&info.x86_capability, &boot_cpu_data.x86_capability, sizeof(info.x86_capability));
+	* Copy all capability leafs to pick up the synthetic ones so that
+	* memcmp() below doesn't fail on that. The ones coming from CPUID will
+	* get overwritten in get_cpu_cap().
+	*/
+	copy_cpu_caps(&info);
 
-	get_cpu_cap(&info);
-
-	if (!memcmp(&info.x86_capability, &boot_cpu_data.x86_capability,
+	if (!memcmp(&info.x86_capability, &orig->x86_capability,
 		    sizeof(info.x86_capability)))
 		return;
 

@@ -469,6 +483,7 @@ static void microcode_check(void)
 static int microcode_reload_late(void)
 {
 	int old = boot_cpu_data.microcode, ret;
+	struct cpuinfo_x86 info;
 
 	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
 	pr_err("You should switch to early loading, if possible.\n");

@@ -476,9 +491,10 @@ static int microcode_reload_late(void)
 	atomic_set(&late_cpus_in,  0);
 	atomic_set(&late_cpus_out, 0);
 

Review static int microcode_reload_late(void) · line 495

Quoted 2 lines

(quoted diff) +1 -0

+	copy_cpu_caps(&info);
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);

You clearly ran out of newlines and comments here.

Review static int microcode_reload_late(void) · line 500

Quoted 7 lines

(quoted diff) +1 -1

 	if (ret == 0)
-		microcode_check();
+		microcode_check(&info);
 
 	pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
 		old, boot_cpu_data.microcode);

Unrelated to that patch, but it just caught my attention. Why are we printing this is case of failure?

The kernel caches features about each CPU's features at boot in an x86_capability[] structure. The microcode update takes one snapshot and compares it with the saved copy at boot.

However, the capabilities in the boot copy can be turned off as a result of certain command line parameters or configuration restrictions. This can cause a mismatch when comparing the values before and after the microcode update.

+static void copy_cpu_caps(struct cpuinfo_x86 *info)

Quoted 8 lines · Ashok Raj

The kernel caches features about each CPU's features at boot in an x86_capability[] structure. The microcode update takes one snapshot and compares it with the saved copy at boot.

However, the capabilities in the boot copy can be turned off as a result of certain command line parameters or configuration restrictions. This can cause a mismatch when comparing the values before and after the microcode update.

Hmm, but if that has happened, the capabilities will be turned off in your @orig argument below?

Or are you saying that this copy_cpu_caps() read before the update will overwrite the cleared bits with the their actual values from CPUID so that what you really wanna compare here is *hardware* CPUID bits before and after and not our potentially modified copy?

I.e., some bit in CPUID is 1 and we have cleared it in our copy.

Close?

If so, then this should be specifically called out in the commit message.

Quoted 2 lines

+static void copy_cpu_caps(struct cpuinfo_x86 *info)

If anything, that function should be called

store_cpu_caps()

Instead of passing a struct ucode_cpu_info, just pass the rev to print.

Next patch will permit printing old and new revisions after an early update. A subsequent patch will print a message when early loading fails.

struct ucode_cpu_info is always the current version in the CPU. When loading fails this is the old rev, when its successfully applied its the new rev. That makes the code bit ugly to read.

Remove the struct ucode_cpu_info parameter from print_ucode() and let the caller to pass in the revision number to print.

Make early loading message to match late loading messages. Print both old and new revisions.

This is helpful to know what the BIOS loaded revision is before an early update.

microcode: early update: 0x2b000041 -> 0x2b000070 date = 2000-01-01

Store the early BIOS revision and change the print format to match late loading message from microcode/core.c

Currently when early microcode loading fails there is no way for user to

know that the update failed.

Store the failed status and pass it to print_ucode_info() to let early loading failures to captured in console log.

Quoted 1 line · Ashok Raj

Currently when early microcode loading fails there is no way for user to

the user

Quoted 4 lines

know that the update failed.

Store the failed status and pass it to print_ucode_info() to let early loading failures to captured in console log.

so that early loading failures are captured in dmesg.

Hmm?