[PATCH v4 0/6] Some fixes and cleanups for microcode

[PATCH v4 0/6] Some fixes and cleanups for microcode

[Ashok Raj]

Hi Boris,

Here is a followup after v3[1] with all comments addressed.

Please review and apply.

Changes since v3:

Tony, Ingo
	- Display clear message when microcode load fails.

Boris
	- Changed function names microcode_store_cpu_caps() ->
	  store_cpu_caps().
	- Fix commit logs 
	- Document new parameter to microcode_check()

Dave Hansen
	- Fix commit log
	- Change parameter names from generic to something that's
	  meaningful.

[1] https://lore.kernel.org/lkml/20230103180212.333496-1-ashok.raj@intel.com/

Ashok Raj (6):
  x86/microcode: Add a parameter to microcode_check() to store CPU
    capabilities
  x86/microcode/core: Take a snapshot before and after applying
    microcode
  x86/microcode: Display revisions only when update is successful
  x86/microcode/intel: Use a plain revision argument for
    print_ucode_rev()
  x86/microcode/intel: Print old and new rev during early boot
  x86/microcode/intel: Print when early microcode loading fails

 arch/x86/include/asm/processor.h      |  3 +-
 arch/x86/kernel/cpu/common.c          | 48 ++++++++++++++++++-------
 arch/x86/kernel/cpu/microcode/core.c  | 20 ++++++++---
 arch/x86/kernel/cpu/microcode/intel.c | 52 +++++++++++++--------------
 4 files changed, 77 insertions(+), 46 deletions(-)


base-commit: b7bfaa761d760e72a969d116517eaa12e404c262
prerequisite-patch-id: 450e9658e9f802a2f3f784ba18267bc47ee878b8
-- 
2.34.1

[PATCH v4 1/6] x86/microcode: Add a parameter to microcode_check() to store CPU capabilities

[Ashok Raj]

Add a parameter to store CPU capabilities before performing a microcode
update so that the code later can compare CPU capabilities before and after
performing the update.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
Changes since V3

Boris:
- Fix commit log to drop "next patch".
- Add documentation to new parameter to microcode_check()
---
 arch/x86/include/asm/processor.h     |  2 +-
 arch/x86/kernel/cpu/common.c         | 21 +++++++++++++--------
 arch/x86/kernel/cpu/microcode/core.c |  3 ++-
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 4e35c66edeb7..f256a4ddd25d 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -697,7 +697,7 @@ bool xen_set_default_idle(void);
 #endif
 
 void __noreturn stop_this_cpu(void *dummy);
-void microcode_check(void);
+void microcode_check(struct cpuinfo_x86 *prev_info);
 
 enum l1tf_mitigations {
 	L1TF_MITIGATION_OFF,
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 9cfca3d7d0e2..0f5a173d0871 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2297,30 +2297,35 @@ void cpu_init_secondary(void)
 #endif
 
 #ifdef CONFIG_MICROCODE_LATE_LOADING
-/*
+/**
+ * microcode_check() - Check if any CPU capabilities changed after an update.
+ * @prev_info:	CPU capabilities stored before an update.
+ *
  * 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.
+ *
+ * Return: None
  */
-void microcode_check(void)
+void microcode_check(struct cpuinfo_x86 *prev_info)
 {
-	struct cpuinfo_x86 info;
-
 	perf_check_microcode();
 
 	/* Reload CPUID max function as it might've changed. */
-	info.cpuid_level = cpuid_eax(0);
+	prev_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));
+	memcpy(&prev_info->x86_capability, &boot_cpu_data.x86_capability,
+	       sizeof(prev_info->x86_capability));
 
-	get_cpu_cap(&info);
+	get_cpu_cap(prev_info);
 
-	if (!memcmp(&info.x86_capability, &boot_cpu_data.x86_capability, sizeof(info.x86_capability)))
+	if (!memcmp(&prev_info->x86_capability, &boot_cpu_data.x86_capability,
+		    sizeof(prev_info->x86_capability)))
 		return;
 
 	pr_warn("x86/CPU: CPU features have changed after loading microcode, but might not take effect.\n");
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index c4cd7328177b..e39d83be794b 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -439,6 +439,7 @@ static int __reload_late(void *info)
 static int microcode_reload_late(void)
 {
 	int old = boot_cpu_data.microcode, ret;
+	struct cpuinfo_x86 prev_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");
@@ -448,7 +449,7 @@ static int microcode_reload_late(void)
 
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
 	if (ret == 0)
-		microcode_check();
+		microcode_check(&prev_info);
 
 	pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
 		old, boot_cpu_data.microcode);
-- 
2.34.1

[tip: x86/microcode] x86/microcode: Add a parameter to microcode_check() to store CPU capabilities

[tip-bot2 for Ashok Raj]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     ab31c74455c64e69342ddab21fd9426fcbfefde7
Gitweb:        https://git.kernel.org/tip/ab31c74455c64e69342ddab21fd9426fcbfefde7
Author:        Ashok Raj <ashok.raj@intel.com>
AuthorDate:    Mon, 09 Jan 2023 07:35:50 -08:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Fri, 20 Jan 2023 21:45:13 +01:00

x86/microcode: Add a parameter to microcode_check() to store CPU capabilities

Add a parameter to store CPU capabilities before performing a microcode
update so that CPU capabilities can be compared before and after update.

  [ bp: Massage. ]

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://lore.kernel.org/r/20230109153555.4986-2-ashok.raj@intel.com
---
 arch/x86/include/asm/processor.h     |  2 +-
 arch/x86/kernel/cpu/common.c         | 21 +++++++++++++--------
 arch/x86/kernel/cpu/microcode/core.c |  3 ++-
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 4e35c66..f256a4d 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -697,7 +697,7 @@ bool xen_set_default_idle(void);
 #endif
 
 void __noreturn stop_this_cpu(void *dummy);
-void microcode_check(void);
+void microcode_check(struct cpuinfo_x86 *prev_info);
 
 enum l1tf_mitigations {
 	L1TF_MITIGATION_OFF,
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 9cfca3d..0f5a173 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2297,30 +2297,35 @@ void cpu_init_secondary(void)
 #endif
 
 #ifdef CONFIG_MICROCODE_LATE_LOADING
-/*
+/**
+ * microcode_check() - Check if any CPU capabilities changed after an update.
+ * @prev_info:	CPU capabilities stored before an update.
+ *
  * 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.
+ *
+ * Return: None
  */
-void microcode_check(void)
+void microcode_check(struct cpuinfo_x86 *prev_info)
 {
-	struct cpuinfo_x86 info;
-
 	perf_check_microcode();
 
 	/* Reload CPUID max function as it might've changed. */
-	info.cpuid_level = cpuid_eax(0);
+	prev_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));
+	memcpy(&prev_info->x86_capability, &boot_cpu_data.x86_capability,
+	       sizeof(prev_info->x86_capability));
 
-	get_cpu_cap(&info);
+	get_cpu_cap(prev_info);
 
-	if (!memcmp(&info.x86_capability, &boot_cpu_data.x86_capability, sizeof(info.x86_capability)))
+	if (!memcmp(&prev_info->x86_capability, &boot_cpu_data.x86_capability,
+		    sizeof(prev_info->x86_capability)))
 		return;
 
 	pr_warn("x86/CPU: CPU features have changed after loading microcode, but might not take effect.\n");
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 99abb31..dc5dfba 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -438,6 +438,7 @@ wait_for_siblings:
 static int microcode_reload_late(void)
 {
 	int old = boot_cpu_data.microcode, ret;
+	struct cpuinfo_x86 prev_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");
@@ -447,7 +448,7 @@ static int microcode_reload_late(void)
 
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
 	if (ret == 0)
-		microcode_check();
+		microcode_check(&prev_info);
 
 	pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
 		old, boot_cpu_data.microcode);

[PATCH v4 2/6] x86/microcode/core: Take a snapshot before and after applying microcode

[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.

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

store_cpu_caps() basically stores the original CPU reported
values and not the OS modified values. This will avoid giving a false
warning even when no capabilities have changed.

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.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Fixes: 1008c52c09dc ("x86/CPU: Add a microcode loader callback")
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
Changes since V3:
- Boris
	- Change function from microcode_store_cpu_caps -> store_cpu_caps
	- Split comments in store_cpu_caps().

- Dave Hansen
	- Change parameters names to something meaninful.
	- Cleaned up some commit log.

Changes since V2:

- Boris
	- Keep microcode_check() inside cpu/common.c and not bleed
	  get_cpu_caps() outside of core code.

- Thomas
	- Commit log changes.
---
 arch/x86/include/asm/processor.h     |  1 +
 arch/x86/kernel/cpu/common.c         | 39 ++++++++++++++++++++--------
 arch/x86/kernel/cpu/microcode/core.c |  7 +++++
 3 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index f256a4ddd25d..a77dee6a2bf2 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -698,6 +698,7 @@ bool xen_set_default_idle(void);
 
 void __noreturn stop_this_cpu(void *dummy);
 void microcode_check(struct cpuinfo_x86 *prev_info);
+void store_cpu_caps(struct cpuinfo_x86 *info);
 
 enum l1tf_mitigations {
 	L1TF_MITIGATION_OFF,
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 0f5a173d0871..f5c6feed6c26 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2297,6 +2297,29 @@ void cpu_init_secondary(void)
 #endif
 
 #ifdef CONFIG_MICROCODE_LATE_LOADING
+/**
+ * store_cpu_caps() - Store a snapshot of CPU capabilities
+ *
+ * Returns: None
+ */
+void store_cpu_caps(struct cpuinfo_x86 *curr_info)
+{
+	/* Reload CPUID max function as it might've changed. */
+	curr_info->cpuid_level = cpuid_eax(0);
+
+	/*
+	 * Copy all capability leafs to pick up the synthetic ones
+	 */
+	memcpy(&curr_info->x86_capability, &boot_cpu_data.x86_capability,
+	       sizeof(curr_info->x86_capability));
+
+	/*
+	 * Capabilities copied from BSP will get overwritten
+	 * with the snapshot below
+	 */
+	get_cpu_cap(curr_info);
+}
+
 /**
  * microcode_check() - Check if any CPU capabilities changed after an update.
  * @prev_info:	CPU capabilities stored before an update.
@@ -2309,22 +2332,16 @@ void cpu_init_secondary(void)
  */
 void microcode_check(struct cpuinfo_x86 *prev_info)
 {
-	perf_check_microcode();
+	struct cpuinfo_x86 curr_info;
 
-	/* Reload CPUID max function as it might've changed. */
-	prev_info->cpuid_level = cpuid_eax(0);
+	perf_check_microcode();
 
 	/*
-	 * 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().
+	 * Get a snapshot of CPU capabilities
 	 */
-	memcpy(&prev_info->x86_capability, &boot_cpu_data.x86_capability,
-	       sizeof(prev_info->x86_capability));
-
-	get_cpu_cap(prev_info);
+	store_cpu_caps(&curr_info);
 
-	if (!memcmp(&prev_info->x86_capability, &boot_cpu_data.x86_capability,
+	if (!memcmp(&prev_info->x86_capability, &curr_info.x86_capability,
 		    sizeof(prev_info->x86_capability)))
 		return;
 
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index e39d83be794b..bb943a91a364 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -447,6 +447,13 @@ static int microcode_reload_late(void)
 	atomic_set(&late_cpus_in,  0);
 	atomic_set(&late_cpus_out, 0);
 
+	/*
+	 * Take a snapshot before the microcode update, so we can compare
+	 * them after the update is successful to check for any bits
+	 * changed.
+	 */
+	store_cpu_caps(&prev_info);
+
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
 	if (ret == 0)
 		microcode_check(&prev_info);
-- 
2.34.1

[tip: x86/microcode] x86/microcode: Check CPU capabilities after late microcode update correctly

[tip-bot2 for Ashok Raj]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     c0dd9245aa9e25a697181f6085692272c9ec61bc
Gitweb:        https://git.kernel.org/tip/c0dd9245aa9e25a697181f6085692272c9ec61bc
Author:        Ashok Raj <ashok.raj@intel.com>
AuthorDate:    Mon, 09 Jan 2023 07:35:51 -08:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Sat, 21 Jan 2023 14:53:20 +01:00

x86/microcode: Check CPU capabilities after late microcode update correctly

The kernel caches each CPU's feature bits at boot in an x86_capability[]
structure. However, the capabilities in the BSP's copy can be turned off
as a result of certain command line parameters or configuration
restrictions, for example the SGX bit. This can cause a mismatch when
comparing the values before and after the microcode update.

Another example is X86_FEATURE_SRBDS_CTRL which gets added only after
microcode update:

  --- cpuid.before	2023-01-21 14:54:15.652000747 +0100
  +++ cpuid.after	2023-01-21 14:54:26.632001024 +0100
  @@ -10,7 +10,7 @@ CPU:
      0x00000004 0x04: eax=0x00000000 ebx=0x00000000 ecx=0x00000000 edx=0x00000000
      0x00000005 0x00: eax=0x00000040 ebx=0x00000040 ecx=0x00000003 edx=0x11142120
      0x00000006 0x00: eax=0x000027f7 ebx=0x00000002 ecx=0x00000001 edx=0x00000000
  -   0x00000007 0x00: eax=0x00000000 ebx=0x029c6fbf ecx=0x40000000 edx=0xbc002400
  +   0x00000007 0x00: eax=0x00000000 ebx=0x029c6fbf ecx=0x40000000 edx=0xbc002e00
  									     ^^^

and which proves for a gazillionth time that late loading is a bad bad
idea.

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

Therefore, store the cached CPU caps before the update and compare them
with the CPU caps after the microcode update has succeeded.

Thus, the comparison is done between the CPUID *hardware* bits before
and after the upgrade instead of using the cached, possibly runtime
modified values in BSP's boot_cpu_data copy.

As a result, false warnings about CPUID bits changes are avoided.

  [ bp:
  	- Massage.
	- Add SRBDS_CTRL example.
	- Add kernel-doc.
	- Incorporate forgotten review feedback from dhansen.
	]

Fixes: 1008c52c09dc ("x86/CPU: Add a microcode loader callback")
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://lore.kernel.org/r/20230109153555.4986-3-ashok.raj@intel.com
---
 arch/x86/include/asm/processor.h     |  1 +-
 arch/x86/kernel/cpu/common.c         | 36 +++++++++++++++++----------
 arch/x86/kernel/cpu/microcode/core.c |  6 +++++-
 3 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index f256a4d..a77dee6 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -698,6 +698,7 @@ bool xen_set_default_idle(void);
 
 void __noreturn stop_this_cpu(void *dummy);
 void microcode_check(struct cpuinfo_x86 *prev_info);
+void store_cpu_caps(struct cpuinfo_x86 *info);
 
 enum l1tf_mitigations {
 	L1TF_MITIGATION_OFF,
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 0f5a173..5ff73ba 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2298,6 +2298,25 @@ void cpu_init_secondary(void)
 
 #ifdef CONFIG_MICROCODE_LATE_LOADING
 /**
+ * store_cpu_caps() - Store a snapshot of CPU capabilities
+ * @curr_info: Pointer where to store it
+ *
+ * Returns: None
+ */
+void store_cpu_caps(struct cpuinfo_x86 *curr_info)
+{
+	/* Reload CPUID max function as it might've changed. */
+	curr_info->cpuid_level = cpuid_eax(0);
+
+	/* Copy all capability leafs and pick up the synthetic ones. */
+	memcpy(&curr_info->x86_capability, &boot_cpu_data.x86_capability,
+	       sizeof(curr_info->x86_capability));
+
+	/* Get the hardware CPUID leafs */
+	get_cpu_cap(curr_info);
+}
+
+/**
  * microcode_check() - Check if any CPU capabilities changed after an update.
  * @prev_info:	CPU capabilities stored before an update.
  *
@@ -2309,22 +2328,13 @@ void cpu_init_secondary(void)
  */
 void microcode_check(struct cpuinfo_x86 *prev_info)
 {
-	perf_check_microcode();
-
-	/* Reload CPUID max function as it might've changed. */
-	prev_info->cpuid_level = cpuid_eax(0);
+	struct cpuinfo_x86 curr_info;
 
-	/*
-	 * 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(&prev_info->x86_capability, &boot_cpu_data.x86_capability,
-	       sizeof(prev_info->x86_capability));
+	perf_check_microcode();
 
-	get_cpu_cap(prev_info);
+	store_cpu_caps(&curr_info);
 
-	if (!memcmp(&prev_info->x86_capability, &boot_cpu_data.x86_capability,
+	if (!memcmp(&prev_info->x86_capability, &curr_info.x86_capability,
 		    sizeof(prev_info->x86_capability)))
 		return;
 
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index dc5dfba..8ec38c1 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -446,6 +446,12 @@ static int microcode_reload_late(void)
 	atomic_set(&late_cpus_in,  0);
 	atomic_set(&late_cpus_out, 0);
 
+	/*
+	 * Take a snapshot before the microcode update in order to compare and
+	 * check whether any bits changed after an update.
+	 */
+	store_cpu_caps(&prev_info);
+
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
 	if (ret == 0)
 		microcode_check(&prev_info);

[PATCH v4 3/6] x86/microcode: Display revisions only when update is successful

[Ashok Raj]

Right now, microcode loading failures and successes print the same
message "Reloading completed". This is misleading to users.

Display the updated revision number only if an update was successful.
Display "Reload completed" only if the update was successful, otherwise
report the update failed.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Fixes: 9bd681251b7c ("x86/microcode: Announce reload operation's completion")
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Link: https://lore.kernel.org/lkml/874judpqqd.ffs@tglx/
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
Changes since V3:

Tony, Ingo
	- Print clear message if the update was successful or not.
---
 arch/x86/kernel/cpu/microcode/core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index bb943a91a364..d7cbc83df9b6 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -455,11 +455,15 @@ static int microcode_reload_late(void)
 	store_cpu_caps(&prev_info);
 
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
-	if (ret == 0)
-		microcode_check(&prev_info);
 
-	pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
-		old, boot_cpu_data.microcode);
+	if (ret == 0) {
+		pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
+			old, boot_cpu_data.microcode);
+		microcode_check(&prev_info);
+	} else {
+		pr_info("Reload failed, current microcode revision: 0x%x\n",
+			boot_cpu_data.microcode);
+	}
 
 	return ret;
 }
-- 
2.34.1

[PATCH v4 4/6] x86/microcode/intel: Use a plain revision argument for print_ucode_rev()

[Ashok Raj]

print_ucode_rev() takes a struct ucode_cpu_info argument. The sole purpose
of it is to print the microcode revision.

The only available ucode_cpu_info always describes the currently loaded
microcode revision. After a microcode update is successful, this is the new
revision, or on failure it is the original revision.

Subsequent changes need to print both the original and new revision, but
the original version will be cached in a plain integer, which makes the
code inconsistent.

Replace the struct ucode_cpu_info argument with a plain integer which
contains the revision number and adjust the call sites accordingly.

No functional change.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
Changes since V1:

Thomas:
 - Updated commit log as suggested
 - Remove the line break after static void before print_ucode_info
---
 arch/x86/kernel/cpu/microcode/intel.c | 31 ++++++++-------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 6bebc46ad8b1..1d709b72cfd0 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -310,13 +310,10 @@ static bool load_builtin_intel_microcode(struct cpio_data *cp)
 /*
  * Print ucode update info.
  */
-static void
-print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
+static void print_ucode_info(unsigned int new_rev, unsigned int date)
 {
 	pr_info_once("microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
-		     uci->cpu_sig.rev,
-		     date & 0xffff,
-		     date >> 24,
+		     new_rev, date & 0xffff, date >> 24,
 		     (date >> 16) & 0xff);
 }
 
@@ -334,7 +331,7 @@ void show_ucode_info_early(void)
 
 	if (delay_ucode_info) {
 		intel_cpu_collect_info(&uci);
-		print_ucode_info(&uci, current_mc_date);
+		print_ucode_info(uci.cpu_sig.rev. current_mc_date);
 		delay_ucode_info = 0;
 	}
 }
@@ -343,33 +340,23 @@ void show_ucode_info_early(void)
  * At this point, we can not call printk() yet. Delay printing microcode info in
  * show_ucode_info_early() until printk() works.
  */
-static void print_ucode(struct ucode_cpu_info *uci)
+static void print_ucode(int new_rev, int date)
 {
 	struct microcode_intel *mc;
 	int *delay_ucode_info_p;
 	int *current_mc_date_p;
 
-	mc = uci->mc;
-	if (!mc)
-		return;
-
 	delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
 	current_mc_date_p = (int *)__pa_nodebug(&current_mc_date);
 
 	*delay_ucode_info_p = 1;
-	*current_mc_date_p = mc->hdr.date;
+	*current_mc_date_p = date;
 }
 #else
 
-static inline void print_ucode(struct ucode_cpu_info *uci)
+static inline void print_ucode(int new_rev, int date)
 {
-	struct microcode_intel *mc;
-
-	mc = uci->mc;
-	if (!mc)
-		return;
-
-	print_ucode_info(uci, mc->hdr.date);
+	print_ucode_info(new_rev, date);
 }
 #endif
 
@@ -409,9 +396,9 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 	uci->cpu_sig.rev = rev;
 
 	if (early)
-		print_ucode(uci);
+		print_ucode(uci->cpu_sig.rev, mc->hdr.date);
 	else
-		print_ucode_info(uci, mc->hdr.date);
+		print_ucode_info(uci->cpu_sig.rev, mc->hdr.date);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v4 4/6] x86/microcode/intel: Use a plain revision argument for print_ucode_rev()

[Borislav Petkov]

On Mon, Jan 09, 2023 at 07:35:53AM -0800, Ashok Raj wrote:
> @@ -343,33 +340,23 @@ void show_ucode_info_early(void)
>   * At this point, we can not call printk() yet. Delay printing microcode info in
>   * show_ucode_info_early() until printk() works.
>   */
> -static void print_ucode(struct ucode_cpu_info *uci)
> +static void print_ucode(int new_rev, int date)
>  {
>  	struct microcode_intel *mc;
>  	int *delay_ucode_info_p;
>  	int *current_mc_date_p;
>  
> -	mc = uci->mc;
> -	if (!mc)
> -		return;
> -
>  	delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
>  	current_mc_date_p = (int *)__pa_nodebug(&current_mc_date);
>  
>  	*delay_ucode_info_p = 1;
> -	*current_mc_date_p = mc->hdr.date;
> +	*current_mc_date_p = date;

Here's how I know you haven't tested this on 32-bit:

arch/x86/kernel/cpu/microcode/intel.c: In function ‘print_ucode’:
arch/x86/kernel/cpu/microcode/intel.c:344:33: error: unused variable ‘mc’ [-Werror=unused-variable]
  344 |         struct microcode_intel *mc;
      |                                 ^~
cc1: all warnings being treated as errors
make[5]: *** [scripts/Makefile.build:252: arch/x86/kernel/cpu/microcode/intel.o] Error 1
make[4]: *** [scripts/Makefile.build:504: arch/x86/kernel/cpu/microcode] Error 2
make[3]: *** [scripts/Makefile.build:504: arch/x86/kernel/cpu] Error 2
make[2]: *** [scripts/Makefile.build:504: arch/x86/kernel] Error 2
make[1]: *** [scripts/Makefile.build:504: arch/x86] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:2008: .] Error 2

Testing is overrated, right?

The maintainers can do that, ofc.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v4 4/6] x86/microcode/intel: Use a plain revision argument for print_ucode_rev()

[Borislav Petkov]

On Mon, Jan 09, 2023 at 07:35:53AM -0800, Ashok Raj wrote:
> @@ -334,7 +331,7 @@ void show_ucode_info_early(void)
>  
>  	if (delay_ucode_info) {
>  		intel_cpu_collect_info(&uci);
> -		print_ucode_info(&uci, current_mc_date);
> +		print_ucode_info(uci.cpu_sig.rev. current_mc_date);

You must be kidding:

arch/x86/kernel/cpu/microcode/intel.c: In function ‘show_ucode_info_early’:
arch/x86/kernel/cpu/microcode/intel.c:332:49: error: request for member ‘current_mc_date’ in something not a structure or union
  332 |                 print_ucode_info(uci.cpu_sig.rev. current_mc_date);
      |                                                 ^
arch/x86/kernel/cpu/microcode/intel.c:332:17: error: too few arguments to function ‘print_ucode_info’
  332 |                 print_ucode_info(uci.cpu_sig.rev. current_mc_date);
      |                 ^~~~~~~~~~~~~~~~
arch/x86/kernel/cpu/microcode/intel.c:311:13: note: declared here
  311 | static void print_ucode_info(unsigned int new_rev, unsigned int date)
      |             ^~~~~~~~~~~~~~~~
arch/x86/kernel/cpu/microcode/intel.c: In function ‘print_ucode’:
arch/x86/kernel/cpu/microcode/intel.c:343:33: error: unused variable ‘mc’ [-Werror=unused-variable]
  343 |         struct microcode_intel *mc;
      |                                 ^~
cc1: all warnings being treated as errors
make[5]: *** [scripts/Makefile.build:252: arch/x86/kernel/cpu/microcode/intel.o] Error 1
make[4]: *** [scripts/Makefile.build:504: arch/x86/kernel/cpu/microcode] Error 2
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:504: arch/x86/kernel/cpu] Error 2
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [scripts/Makefile.build:504: arch/x86/kernel] Error 2
make[1]: *** [scripts/Makefile.build:504: arch/x86] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:2008: .] Error 2

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v4 4/6] x86/microcode/intel: Use a plain revision argument for print_ucode_rev()

[Ashok Raj]

On Sun, Jan 15, 2023 at 08:39:18PM +0100, Borislav Petkov wrote:
> On Mon, Jan 09, 2023 at 07:35:53AM -0800, Ashok Raj wrote:
> > @@ -334,7 +331,7 @@ void show_ucode_info_early(void)
> >  
> >  	if (delay_ucode_info) {
> >  		intel_cpu_collect_info(&uci);
> > -		print_ucode_info(&uci, current_mc_date);
> > +		print_ucode_info(uci.cpu_sig.rev. current_mc_date);
> 
> You must be kidding:

Oversight completely.. Apologize. I remember seeing them but unfortunately
I ended up fixing the issue in patch5 instead of patch4. 


> 
> arch/x86/kernel/cpu/microcode/intel.c: In function ‘show_ucode_info_early’:
> arch/x86/kernel/cpu/microcode/intel.c:332:49: error: request for member ‘current_mc_date’ in something not a structure or union
>   332 |                 print_ucode_info(uci.cpu_sig.rev. current_mc_date);
>       |                                                 ^
> arch/x86/kernel/cpu/microcode/intel.c:332:17: error: too few arguments to function ‘print_ucode_info’
>   332 |                 print_ucode_info(uci.cpu_sig.rev. current_mc_date);
>       |                 ^~~~~~~~~~~~~~~~
> arch/x86/kernel/cpu/microcode/intel.c:311:13: note: declared here
>   311 | static void print_ucode_info(unsigned int new_rev, unsigned int date)
>       |             ^~~~~~~~~~~~~~~~
> arch/x86/kernel/cpu/microcode/intel.c: In function ‘print_ucode’:
> arch/x86/kernel/cpu/microcode/intel.c:343:33: error: unused variable ‘mc’ [-Werror=unused-variable]
>   343 |         struct microcode_intel *mc;
>       |                                 ^~

I fixed in patch6 instead of this patch, my bad.

I updated my build to have CONFIG_WERROR, i had that accidently turned off
in one of my configs.

I have fixed up the right version and can repost if there are no additional
comments.

Cheers,
Ashok

Re: [PATCH v4 4/6] x86/microcode/intel: Use a plain revision argument for print_ucode_rev()

[Borislav Petkov]

On Tue, Jan 17, 2023 at 08:05:38AM -0800, Ashok Raj wrote:
> I updated my build to have CONFIG_WERROR, i had that accidently turned off
> in one of my configs.

You need to build every patch for bisection purposes, obviously. In addition
CONFIG_ERROR.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v4 5/6] x86/microcode/intel: Print old and new rev during early boot

[Ashok Raj]

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.

New dmesg log is shown below.

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

Cache the early BIOS revision before the microcode update and change the
print_ucode_info() so it prints both the old and new revision in the same
format as microcode_reload_late().

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
Updates since V1:

Thomas: Commit log updates as suggested.
---
 arch/x86/kernel/cpu/microcode/intel.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 1d709b72cfd0..f24300830ed7 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -310,10 +310,10 @@ static bool load_builtin_intel_microcode(struct cpio_data *cp)
 /*
  * Print ucode update info.
  */
-static void print_ucode_info(unsigned int new_rev, unsigned int date)
+static void print_ucode_info(int old_rev, int new_rev, unsigned int date)
 {
-	pr_info_once("microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
-		     new_rev, date & 0xffff, date >> 24,
+	pr_info_once("early update: 0x%x -> 0x%x, date = %04x-%02x-%02x\n",
+		     old_rev, new_rev, date & 0xffff, date >> 24,
 		     (date >> 16) & 0xff);
 }
 
@@ -321,6 +321,7 @@ static void print_ucode_info(unsigned int new_rev, unsigned int date)
 
 static int delay_ucode_info;
 static int current_mc_date;
+static int early_old_rev;
 
 /*
  * Print early updated ucode info after printk works. This is delayed info dump.
@@ -331,7 +332,7 @@ void show_ucode_info_early(void)
 
 	if (delay_ucode_info) {
 		intel_cpu_collect_info(&uci);
-		print_ucode_info(uci.cpu_sig.rev. current_mc_date);
+		print_ucode_info(early_old_rev, uci.cpu_sig.rev, current_mc_date);
 		delay_ucode_info = 0;
 	}
 }
@@ -340,30 +341,33 @@ void show_ucode_info_early(void)
  * At this point, we can not call printk() yet. Delay printing microcode info in
  * show_ucode_info_early() until printk() works.
  */
-static void print_ucode(int new_rev, int date)
+static void print_ucode(int old_rev, int new_rev, int date)
 {
 	struct microcode_intel *mc;
 	int *delay_ucode_info_p;
 	int *current_mc_date_p;
+	int *early_old_rev_p;
 
 	delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
 	current_mc_date_p = (int *)__pa_nodebug(&current_mc_date);
+	early_old_rev_p = (int *)__pa_nodebug(&early_old_rev);
 
 	*delay_ucode_info_p = 1;
 	*current_mc_date_p = date;
+	*early_old_rev_p = old_rev;
 }
 #else
 
-static inline void print_ucode(int new_rev, int date)
+static inline void print_ucode(int old_rev, int new_rev, int date)
 {
-	print_ucode_info(new_rev, date);
+	print_ucode_info(old_rev, new_rev, date);
 }
 #endif
 
 static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 {
 	struct microcode_intel *mc;
-	u32 rev;
+	u32 rev, old_rev;
 
 	mc = uci->mc;
 	if (!mc)
@@ -389,6 +393,7 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 	/* write microcode via MSR 0x79 */
 	native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
 
+	old_rev = rev;
 	rev = intel_get_microcode_revision();
 	if (rev != mc->hdr.rev)
 		return -1;
@@ -396,9 +401,9 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 	uci->cpu_sig.rev = rev;
 
 	if (early)
-		print_ucode(uci->cpu_sig.rev, mc->hdr.date);
+		print_ucode(old_rev, uci->cpu_sig.rev, mc->hdr.date);
 	else
-		print_ucode_info(uci->cpu_sig.rev, mc->hdr.date);
+		print_ucode_info(old_rev, uci->cpu_sig.rev, mc->hdr.date);
 
 	return 0;
 }
-- 
2.34.1

[PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Ashok Raj]

Currently when early microcode loading fails there is no way for the
user to know that the update failed.

Store the failed status and pass it to print_ucode_info() so that early
loading failures are captured in dmesg.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
Changes since V1:

Thomas: Fix commit log as suggested.
---
 arch/x86/kernel/cpu/microcode/intel.c | 28 +++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index f24300830ed7..0cdff9ed2a4e 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -310,11 +310,11 @@ static bool load_builtin_intel_microcode(struct cpio_data *cp)
 /*
  * Print ucode update info.
  */
-static void print_ucode_info(int old_rev, int new_rev, unsigned int date)
+static void print_ucode_info(bool failed, int old_rev, int new_rev, unsigned int date)
 {
-	pr_info_once("early update: 0x%x -> 0x%x, date = %04x-%02x-%02x\n",
+	pr_info_once("early update: 0x%x -> 0x%x, date = %04x-%02x-%02x %s\n",
 		     old_rev, new_rev, date & 0xffff, date >> 24,
-		     (date >> 16) & 0xff);
+		     (date >> 16) & 0xff, failed ? "FAILED" : "");
 }
 
 #ifdef CONFIG_X86_32
@@ -322,6 +322,7 @@ static void print_ucode_info(int old_rev, int new_rev, unsigned int date)
 static int delay_ucode_info;
 static int current_mc_date;
 static int early_old_rev;
+static bool early_failed;
 
 /*
  * Print early updated ucode info after printk works. This is delayed info dump.
@@ -332,7 +333,7 @@ void show_ucode_info_early(void)
 
 	if (delay_ucode_info) {
 		intel_cpu_collect_info(&uci);
-		print_ucode_info(early_old_rev, uci.cpu_sig.rev, current_mc_date);
+		print_ucode_info(early_failed, early_old_rev, uci.cpu_sig.rev, current_mc_date);
 		delay_ucode_info = 0;
 	}
 }
@@ -341,26 +342,28 @@ void show_ucode_info_early(void)
  * At this point, we can not call printk() yet. Delay printing microcode info in
  * show_ucode_info_early() until printk() works.
  */
-static void print_ucode(int old_rev, int new_rev, int date)
+static void print_ucode(bool failed, int old_rev, int new_rev, int date)
 {
-	struct microcode_intel *mc;
 	int *delay_ucode_info_p;
 	int *current_mc_date_p;
 	int *early_old_rev_p;
+	bool *early_failed_p;
 
 	delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
 	current_mc_date_p = (int *)__pa_nodebug(&current_mc_date);
 	early_old_rev_p = (int *)__pa_nodebug(&early_old_rev);
+	early_failed_p = (bool *)__pa_nodebug(&early_failed);
 
 	*delay_ucode_info_p = 1;
 	*current_mc_date_p = date;
 	*early_old_rev_p = old_rev;
+	*early_failed_p = failed;
 }
 #else
 
-static inline void print_ucode(int old_rev, int new_rev, int date)
+static inline void print_ucode(bool failed, int old_rev, int new_rev, int date)
 {
-	print_ucode_info(old_rev, new_rev, date);
+	print_ucode_info(failed, old_rev, new_rev, date);
 }
 #endif
 
@@ -368,6 +371,7 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 {
 	struct microcode_intel *mc;
 	u32 rev, old_rev;
+	int retval = 0;
 
 	mc = uci->mc;
 	if (!mc)
@@ -396,16 +400,16 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 	old_rev = rev;
 	rev = intel_get_microcode_revision();
 	if (rev != mc->hdr.rev)
-		return -1;
+		retval = -1;
 
 	uci->cpu_sig.rev = rev;
 
 	if (early)
-		print_ucode(old_rev, uci->cpu_sig.rev, mc->hdr.date);
+		print_ucode(retval, old_rev, mc->hdr.rev, mc->hdr.date);
 	else
-		print_ucode_info(old_rev, uci->cpu_sig.rev, mc->hdr.date);
+		print_ucode_info(retval, old_rev, uci->cpu_sig.rev, mc->hdr.date);
 
-	return 0;
+	return retval;
 }
 
 int __init save_microcode_in_initrd_intel(void)
-- 
2.34.1

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Borislav Petkov]

On Mon, Jan 09, 2023 at 07:35:55AM -0800, Ashok Raj wrote:
> Currently when early microcode loading fails there is no way for the
> user to know that the update failed.

Of course there is - there's no early update message in dmesg.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Ashok Raj]

On Sun, Jan 15, 2023 at 08:05:14PM +0100, Borislav Petkov wrote:
> On Mon, Jan 09, 2023 at 07:35:55AM -0800, Ashok Raj wrote:
> > Currently when early microcode loading fails there is no way for the
> > user to know that the update failed.
> 
> Of course there is - there's no early update message in dmesg.

Sorry Boris, didn't comprehend. 

Without user making some additional steps to check the revision in the
default location and the current rev reported to find the update failed.

Maybe that's what you meant.

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Dave Hansen]

On 1/17/23 08:12, Ashok Raj wrote:
> On Sun, Jan 15, 2023 at 08:05:14PM +0100, Borislav Petkov wrote:
>> On Mon, Jan 09, 2023 at 07:35:55AM -0800, Ashok Raj wrote:
>>> Currently when early microcode loading fails there is no way for the
>>> user to know that the update failed.
>> Of course there is - there's no early update message in dmesg.
> Sorry Boris, didn't comprehend. 
> 
> Without user making some additional steps to check the revision in the
> default location and the current rev reported to find the update failed.
> 
> Maybe that's what you meant.

I think a better changelog might help here.  The original was a bit too
absolute.  There is, as Boris pointed out, a way to tell if a failure
occurred.  But, that method is a bit unfriendly to our users.

--

When early microcode loading succeeds, the kernel prints a message via
print_ucode_info() that starts with 'early update:'.  If loading fails,
apply_microcode_early() returns before that message is printed.  This
means that users must know to go looking for that message.  They can
infer a early microcode loading failure if they do not see the message.

That's not great for users.  Instead of expecting them to infer this, be
more explicit about it.  Instead of bailing out and returning from
apply_microcode_early(), stash the error code off and plumb it down to
print_ucode_info().

This ensures that a message of some kind is printed on all early loads:
successes *and* failures.  This should make it easier for our hapless
users to figure out when a failure occurred.

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Borislav Petkov]

On Tue, Jan 17, 2023 at 08:29:28AM -0800, Dave Hansen wrote:
> This ensures that a message of some kind is printed on all early loads:
> successes *and* failures.  This should make it easier for our hapless
> users to figure out when a failure occurred.

I'm still not convinced. When something doesn't happen in the kernel, we don't
always say "It didn't happen". We don't say anything.

So I don't like all those talkative drivers for no good reason. If there wasn't
an update message, then no update happened. That's it.

And the current microcode revision is in /proc/cpuinfo.

If you wanna know why the update didn't happen, then you start adding debug
printks and trying things but then you're clearly not a user so you know what
you're doing.

And the log buffer can get overwritten sooner or later depending on its size so
any message can disappear.

So what's the point of this pointless exercise in verbosity?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Dave Hansen]

On 1/17/23 10:21, Borislav Petkov wrote:
> On Tue, Jan 17, 2023 at 08:29:28AM -0800, Dave Hansen wrote:
>> This ensures that a message of some kind is printed on all early loads:
>> successes *and* failures.  This should make it easier for our hapless
>> users to figure out when a failure occurred.
> I'm still not convinced. When something doesn't happen in the kernel, we don't
> always say "It didn't happen". We don't say anything.

Well, we have an awful lot of pr_warn()'s in the kernel that talk about
something that was tried and failed.

> So I don't like all those talkative drivers for no good reason. If there wasn't
> an update message, then no update happened. That's it.

I actually kinda like the inverse.

The common (boring) case where an update was needed and was successful.
It's the one we don't need to tell users about at all.  It barely
deserves a message.  Users expect that if there's an early update
available, it'll get attempted, it will be successful and the kernel
won't say much.

The time we need to spam dmesg is when something upends user
expectations and they might need to go do something.  An early loading
failure is exactly the place where they want to know.  They want to know
if they're running with known CPU bugs that would have been fixed by the
early update, or if they somehow have a botched early loading image.

So, if I had to pick either:

 * Print on failure
or
 * Print on success

I'd pick failure.  But, considering that we're already printing on
success, I'm OK doing both.

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Borislav Petkov]

On Tue, Jan 17, 2023 at 10:32:50AM -0800, Dave Hansen wrote:
> Well, we have an awful lot of pr_warn()'s in the kernel that talk about
> something that was tried and failed.

Well, is microcode loading failure worth to warn about?

What if there's no microcode for that CPU?

Now imagine in that case you issue a warning and then you get all those
bugzillas opened:

"Heeey, is my CPU broken, it says it cannot load microcode"

I sure don't want to be on the receiving end of this.

I don't think you wanna be there either.

> I actually kinda like the inverse.
> 
> The common (boring) case where an update was needed and was successful.
> It's the one we don't need to tell users about at all.  It barely
> deserves a message.  Users expect that if there's an early update
> available, it'll get attempted, it will be successful and the kernel
> won't say much.

No argument there.

> The time we need to spam dmesg is when something upends user
> expectations and they might need to go do something.  An early loading
> failure is exactly the place where they want to know.  They want to know
> if they're running with known CPU bugs that would have been fixed by the
> early update

No, we already warn about that in the CPU mitigations code.

> or if they somehow have a botched early loading image.

If you can pick apart from this here:

        /* write microcode via MSR 0x79 */
        native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);

what type of failure it is, then sure, let's warn.

But we should not warn just for every revision mismatch in there...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Ashok Raj]

On Tue, Jan 17, 2023 at 07:40:31PM +0100, Borislav Petkov wrote:
> On Tue, Jan 17, 2023 at 10:32:50AM -0800, Dave Hansen wrote:
> > Well, we have an awful lot of pr_warn()'s in the kernel that talk about
> > something that was tried and failed.
> 
> Well, is microcode loading failure worth to warn about?

Is it not? 
> 
> What if there's no microcode for that CPU?

If there is no microcode, we don't print anything. So what's loaded in the
CPU is the latest version. When we have something we can always tell if its
successful or not.

Its not a microcode file in initrd, but a matching microcode to load. If
none is found, nothing to worry about.

We just agreed to show both failed and success for late-load. Doing this is
consistent with that isn't it?

https://lore.kernel.org/lkml/Y7iYLbEJSYnVn+dW@zn.tnic/

Ingo's:
https://lore.kernel.org/lkml/Y7k9DNz%2F%2FvqBAvZK@gmail.com/

Should we treat early loading differently?

Cheers,
Ashok

RE: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Luck, Tony]

> If there is no microcode, we don't print anything. So what's loaded in the
> CPU is the latest version. When we have something we can always tell if its
> successful or not.
>
> Its not a microcode file in initrd, but a matching microcode to load. If
> none is found, nothing to worry about.
>
> We just agreed to show both failed and success for late-load. Doing this is
> consistent with that isn't it?
>
> https://lore.kernel.org/lkml/Y7iYLbEJSYnVn+dW@zn.tnic/
>
> Ingo's:
> https://lore.kernel.org/lkml/Y7k9DNz%2F%2FvqBAvZK@gmail.com/
>
> Should we treat early loading differently?

Getting a better set of messages from early microcode update would
be a "nice-to-have" feature. But if there is no agreement on what those
messages should look like, perhaps just skip this part for now.

Then Ashok can move on to the real issue of allowing LATE_LOAD for a
microcode that supports the new "minrev" header field.

-Tony

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Ashok Raj]

Hi Boris,

On Tue, Jan 17, 2023 at 12:58:12PM -0800, Tony Luck wrote:
> > If there is no microcode, we don't print anything. So what's loaded in the
> > CPU is the latest version. When we have something we can always tell if its
> > successful or not.
> >
> > Its not a microcode file in initrd, but a matching microcode to load. If
> > none is found, nothing to worry about.
> >
> > We just agreed to show both failed and success for late-load. Doing this is
> > consistent with that isn't it?
> >
> > https://lore.kernel.org/lkml/Y7iYLbEJSYnVn+dW@zn.tnic/
> >
> > Ingo's:
> > https://lore.kernel.org/lkml/Y7k9DNz%2F%2FvqBAvZK@gmail.com/
> >
> > Should we treat early loading differently?
> 
> Getting a better set of messages from early microcode update would
> be a "nice-to-have" feature. But if there is no agreement on what those
> messages should look like, perhaps just skip this part for now.
> 
> Then Ashok can move on to the real issue of allowing LATE_LOAD for a
> microcode that supports the new "minrev" header field.
> 
If this is the one blocking this series, as Tony proposed, its not worth
loosing sleep over this.

Would you recommend resubmitting a new set with the fixes for the interim
patch compile failures (for 32bit) and drop this last patch?

If you have any other comments would be great to hear now before I repost a
new series to capture everything that's need to be addressed.

As always you can change commit logs to your satisfaction, I try, but its
still not as perfect as how you like to see them.

Cheers,
Ashok

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Borislav Petkov]

On Thu, Jan 19, 2023 at 09:59:33AM -0800, Ashok Raj wrote:
> Would you recommend resubmitting a new set with the fixes for the interim
> patch compile failures (for 32bit) and drop this last patch?

Sure.

> If you have any other comments would be great to hear now before I repost a
> new series to capture everything that's need to be addressed.

No, no more comments.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Ashok Raj]

On Fri, Jan 20, 2023 at 01:03:57PM +0100, Borislav Petkov wrote:
> On Thu, Jan 19, 2023 at 09:59:33AM -0800, Ashok Raj wrote:
> > Would you recommend resubmitting a new set with the fixes for the interim
> > patch compile failures (for 32bit) and drop this last patch?
> 
> Sure.
> 
> > If you have any other comments would be great to hear now before I repost a
> > new series to capture everything that's need to be addressed.
> 
> No, no more comments.
> 
Great, light at the end of the tunnel!

I have posted v5. 

https://lore.kernel.org/lkml/20230120161923.118882-1-ashok.raj@intel.com/

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Dave Hansen]

On 1/17/23 12:40, Ashok Raj wrote:
> We just agreed to show both failed and success for late-load. Doing this is
> consistent with that isn't it?

Well, I kinda proposed that and you agreed.  I don't think Boris ever
agreed, though.

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Borislav Petkov]

On Tue, Jan 17, 2023 at 12:40:30PM -0800, Ashok Raj wrote:
> We just agreed to show both failed and success for late-load.

Did you read the part you snipped about getting false/misleading bug reports? I
doubt it, as usual.

If you all Intel folks want to deal with people asking why is there a warning
message about microcode not loading successfully, I don't mind forwarding you
all those messages. I sure ain't going to deal with them.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Ashok Raj]

On Tue, Jan 17, 2023 at 10:06:29PM +0100, Borislav Petkov wrote:
> On Tue, Jan 17, 2023 at 12:40:30PM -0800, Ashok Raj wrote:
> > We just agreed to show both failed and success for late-load.
> 
> Did you read the part you snipped about getting false/misleading bug reports? I
> doubt it, as usual.
> 
> If you all Intel folks want to deal with people asking why is there a warning
> message about microcode not loading successfully, I don't mind forwarding you
> all those messages. I sure ain't going to deal with them.

TBH, there is nothing to hide from a microcode loading failure. If user
sees that a loading failed, it could be maybe due to corrupted files and
its best to know and update to proper file vs running without the latest
microcode file always.

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Ashok Raj]

On Tue, Jan 17, 2023 at 10:32:50AM -0800, Dave Hansen wrote:
> On 1/17/23 10:21, Borislav Petkov wrote:
> > On Tue, Jan 17, 2023 at 08:29:28AM -0800, Dave Hansen wrote:
> >> This ensures that a message of some kind is printed on all early loads:
> >> successes *and* failures.  This should make it easier for our hapless
> >> users to figure out when a failure occurred.
> > I'm still not convinced. When something doesn't happen in the kernel, we don't
> > always say "It didn't happen". We don't say anything.
> 
> Well, we have an awful lot of pr_warn()'s in the kernel that talk about
> something that was tried and failed.
> 
> > So I don't like all those talkative drivers for no good reason. If there wasn't
> > an update message, then no update happened. That's it.
> 
> I actually kinda like the inverse.
> 
> The common (boring) case where an update was needed and was successful.
> It's the one we don't need to tell users about at all.  It barely

I think its useful to know what microcode was loaded at FIT from BIOS and
what its updated to from initrd. We have been telling about successful
updates, no good reason to nuke that. 

We added successful and failures on late-loads, so its better to be
consistent for both early and late.


> deserves a message.  Users expect that if there's an early update
> available, it'll get attempted, it will be successful and the kernel
> won't say much.
> 
> The time we need to spam dmesg is when something upends user
> expectations and they might need to go do something.  An early loading
> failure is exactly the place where they want to know.  They want to know
> if they're running with known CPU bugs that would have been fixed by the
> early update, or if they somehow have a botched early loading image.
> 
> So, if I had to pick either:
> 
>  * Print on failure
> or
>  * Print on success
> 
> I'd pick failure.  But, considering that we're already printing on
> success, I'm OK doing both.

That sounds good, lets do on both.

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Dave Hansen]

On 1/9/23 07:35, Ashok Raj wrote:
> -static void print_ucode(int old_rev, int new_rev, int date)
> +static void print_ucode(bool failed, int old_rev, int new_rev, int date)
...
>  	if (rev != mc->hdr.rev)
> -		return -1;
> +		retval = -1;
>  
>  	uci->cpu_sig.rev = rev;
>  
>  	if (early)
> -		print_ucode(old_rev, uci->cpu_sig.rev, mc->hdr.date);
> +		print_ucode(retval, old_rev, mc->hdr.rev, mc->hdr.date);
>  	else
> -		print_ucode_info(old_rev, uci->cpu_sig.rev, mc->hdr.date);
> +		print_ucode_info(retval, old_rev, uci->cpu_sig.rev, mc->hdr.date);
>  
> -	return 0;
> +	return retval;
>  }

I'm generally not a _huge_ fan of having an 'int' implicitly cast to a
bool.  The:

	print_ucode_info(retval, ...

Line could be right or wrong based on what the retval is logically.
This, on the other hand:

	bool failed = false;
	...
	if (rev != mc->hdr.rev) {
		retval = -1;
		failed = true;
	}
	...
	print_ucode_info(failed, old_rev, uci->cpu_sig.rev, ...

*Clearly* and unambiguously matches up with:

	static void print_ucode(bool failed, int old_rev, ...

Re: [PATCH v4 6/6] x86/microcode/intel: Print when early microcode loading fails

[Ashok Raj]

On Tue, Jan 17, 2023 at 08:35:33AM -0800, Dave Hansen wrote:
> On 1/9/23 07:35, Ashok Raj wrote:
> > -static void print_ucode(int old_rev, int new_rev, int date)
> > +static void print_ucode(bool failed, int old_rev, int new_rev, int date)
> ...
> >  	if (rev != mc->hdr.rev)
> > -		return -1;
> > +		retval = -1;
> >  
> >  	uci->cpu_sig.rev = rev;
> >  
> >  	if (early)
> > -		print_ucode(old_rev, uci->cpu_sig.rev, mc->hdr.date);
> > +		print_ucode(retval, old_rev, mc->hdr.rev, mc->hdr.date);
> >  	else
> > -		print_ucode_info(old_rev, uci->cpu_sig.rev, mc->hdr.date);
> > +		print_ucode_info(retval, old_rev, uci->cpu_sig.rev, mc->hdr.date);
> >  
> > -	return 0;
> > +	return retval;
> >  }
> 
> I'm generally not a _huge_ fan of having an 'int' implicitly cast to a
> bool.  The:
> 
> 	print_ucode_info(retval, ...
> 
> Line could be right or wrong based on what the retval is logically.
> This, on the other hand:
> 
> 	bool failed = false;
> 	...
> 	if (rev != mc->hdr.rev) {
> 		retval = -1;
> 		failed = true;
> 	}
> 	...
> 	print_ucode_info(failed, old_rev, uci->cpu_sig.rev, ...
> 
> *Clearly* and unambiguously matches up with:
> 
> 	static void print_ucode(bool failed, int old_rev, ...

Yes, it makes good sense.. I'll fix up next update including the commit log
that you called out.