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

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

[Ashok Raj]

Hi Boris

This is a followup after earlier post [1]

Sending the rest of the patches after first 2 patches that were merged.

Please review and consider applying.

Changes since v1:

- Updated comments and added reviewed by from Thomas.
- Moved microcode_check() to where it originally was based on your
  feedback. [2]

[1] https://lore.kernel.org/lkml/20221227192340.8358-1-ashok.raj@intel.com/
[2] https://lore.kernel.org/lkml/Y6tMgcU2aGbx%2F6yt@zn.tnic/

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          | 29 ++++++++++-----
 arch/x86/kernel/cpu/microcode/core.c  | 17 ++++++---
 arch/x86/kernel/cpu/microcode/intel.c | 52 +++++++++++++--------------
 4 files changed, 60 insertions(+), 41 deletions(-)


base-commit: 88603b6dc419445847923fcb7fe5080067a30f98
-- 
2.34.1

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

[Ashok Raj]

This is a preparation before the next patch uses this to compare CPU
capabilities after performing an update.

Add a parameter to store CPU capabilities before performing a microcode
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>
---
 arch/x86/include/asm/processor.h     |  2 +-
 arch/x86/kernel/cpu/common.c         | 12 +++++-------
 arch/x86/kernel/cpu/microcode/core.c |  3 ++-
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 4e35c66edeb7..387578049de0 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 *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..b9c7529c920e 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2302,25 +2302,23 @@ void cpu_init_secondary(void)
  * only when microcode has been updated. Caller holds microcode_mutex and CPU
  * hotplug lock.
  */
-void microcode_check(void)
+void microcode_check(struct cpuinfo_x86 *info)
 {
-	struct cpuinfo_x86 info;
-
 	perf_check_microcode();
 
 	/* Reload CPUID max function as it might've changed. */
-	info.cpuid_level = cpuid_eax(0);
+	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(&info->x86_capability, &boot_cpu_data.x86_capability, sizeof(info->x86_capability));
 
-	get_cpu_cap(&info);
+	get_cpu_cap(info);
 
-	if (!memcmp(&info.x86_capability, &boot_cpu_data.x86_capability, sizeof(info.x86_capability)))
+	if (!memcmp(&info->x86_capability, &boot_cpu_data.x86_capability, sizeof(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..d86a4f910a6b 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 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(&info);
 
 	pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
 		old, boot_cpu_data.microcode);
-- 
2.34.1

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

microcode_store_cpu_caps() basically stores the original CPU reported
values and not the OS modified values. This will avoid giving a false
warning even if 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.

Fixes: 1008c52c09dc ("x86/CPU: Add a microcode loader callback")
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>
---
Changes since last post

- Boris
	- Change function from copy_cpu_caps() -> store_cpu_caps()
	- 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         | 31 +++++++++++++++++++++-------
 arch/x86/kernel/cpu/microcode/core.c |  7 +++++++
 3 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 387578049de0..ac2e67156b9b 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -697,6 +697,7 @@ bool xen_set_default_idle(void);
 #endif
 
 void __noreturn stop_this_cpu(void *dummy);
+void microcode_store_cpu_caps(struct cpuinfo_x86 *info);
 void microcode_check(struct cpuinfo_x86 *info);
 
 enum l1tf_mitigations {
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index b9c7529c920e..7c86c6fd07ae 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2297,28 +2297,43 @@ void cpu_init_secondary(void)
 #endif
 
 #ifdef CONFIG_MICROCODE_LATE_LOADING
+
+void microcode_store_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.
  */
-void microcode_check(struct cpuinfo_x86 *info)
+void microcode_check(struct cpuinfo_x86 *orig)
 {
-	perf_check_microcode();
+	struct cpuinfo_x86 info;
 
-	/* Reload CPUID max function as it might've changed. */
-	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().
 	 */
-	memcpy(&info->x86_capability, &boot_cpu_data.x86_capability, sizeof(info->x86_capability));
-
-	get_cpu_cap(info);
+	microcode_store_cpu_caps(&info);
 
-	if (!memcmp(&info->x86_capability, &boot_cpu_data.x86_capability, sizeof(info->x86_capability)))
+	if (!memcmp(&info.x86_capability, &orig->x86_capability,
+		    sizeof(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 d86a4f910a6b..14d9031ed68a 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.
+	 */
+	microcode_store_cpu_caps(&info);
+
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
 	if (ret == 0)
 		microcode_check(&info);
-- 
2.34.1

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

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
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>
---
 arch/x86/kernel/cpu/microcode/core.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 14d9031ed68a..e67f8923f119 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -455,11 +455,12 @@ static int microcode_reload_late(void)
 	microcode_store_cpu_caps(&info);
 
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
-	if (ret == 0)
-		microcode_check(&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(&info);
+	}
 
 	return ret;
 }
-- 
2.34.1

[PATCH v3 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>
---
Changes since earlier post.

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

[PATCH v3 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>
---
Updates since previous post.

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 v3 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>
---
Changes since last post.

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 v3 2/6] x86/microcode/core: Take a snapshot before and after applying microcode

[Dave Hansen]

On 1/3/23 10:02, Ashok Raj wrote:
> 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.
> 
> microcode_store_cpu_caps() basically stores the original CPU reported
> values and not the OS modified values. This will avoid giving a false
> warning even if 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.
...

It took me a moment to square this "Ignore the capabilities recorded at
boot." statement with the continued existence of:

	memcpy(info->x86_capability, &boot_cpu_data.x86_capability, ...

I think just adding "hardware" might help:

	Ignore all hardware capabilities recorded at boot.

Or even adding one more sentence:

	Only consult the synthetic capabilities recorded at boot so that
	a simple memcmp() can be used for comparisons.

> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 387578049de0..ac2e67156b9b 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -697,6 +697,7 @@ bool xen_set_default_idle(void);
>  #endif
>  
>  void __noreturn stop_this_cpu(void *dummy);
> +void microcode_store_cpu_caps(struct cpuinfo_x86 *info);
>  void microcode_check(struct cpuinfo_x86 *info);
>  
>  enum l1tf_mitigations {
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index b9c7529c920e..7c86c6fd07ae 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -2297,28 +2297,43 @@ void cpu_init_secondary(void)
>  #endif
>  
>  #ifdef CONFIG_MICROCODE_LATE_LOADING
> +
> +void microcode_store_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.
>   */
> -void microcode_check(struct cpuinfo_x86 *info)
> +void microcode_check(struct cpuinfo_x86 *orig)
>  {
> -	perf_check_microcode();
> +	struct cpuinfo_x86 info;

'info' is kinda a throwaway name.  would this be better as:

	prev_info / new_info

instead of:

	orig / info

?

> -	/* Reload CPUID max function as it might've changed. */
> -	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().
>  	 */

This comment got copied to microcode_store_cpu_caps().  Does this
instance still need to be here?

> -	memcpy(&info->x86_capability, &boot_cpu_data.x86_capability, sizeof(info->x86_capability));
> -
> -	get_cpu_cap(info);
> +	microcode_store_cpu_caps(&info);
>  
> -	if (!memcmp(&info->x86_capability, &boot_cpu_data.x86_capability, sizeof(info->x86_capability)))
> +	if (!memcmp(&info.x86_capability, &orig->x86_capability,
> +		    sizeof(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 d86a4f910a6b..14d9031ed68a 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.
> +	 */
> +	microcode_store_cpu_caps(&info);

A "we" snuck in there.  How about this?

	/*
	 * Take a snapshot before the microcode update.  This enables
	 * a later comparison to see any bits changed after an update.
	 */

I do think some better naming of 'info' here would be nice too.
'old_info' or 'prev_info' seem like good alternatives.

>  	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
>  	if (ret == 0)
>  		microcode_check(&info);

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

[Ashok Raj]

On Tue, Jan 03, 2023 at 10:46:56AM -0800, Dave Hansen wrote:
> On 1/3/23 10:02, Ashok Raj wrote:
> > 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.
> > 
> > microcode_store_cpu_caps() basically stores the original CPU reported
> > values and not the OS modified values. This will avoid giving a false
> > warning even if 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.
> ...
> 
> It took me a moment to square this "Ignore the capabilities recorded at
> boot." statement with the continued existence of:
> 
> 	memcpy(info->x86_capability, &boot_cpu_data.x86_capability, ...
> 
> I think just adding "hardware" might help:
> 
> 	Ignore all hardware capabilities recorded at boot.
> 
> Or even adding one more sentence:
> 
> 	Only consult the synthetic capabilities recorded at boot so that
> 	a simple memcmp() can be used for comparisons.

Rewritten this multiple times, but it still seems confusing, however hard I
try :-(

Its not consulting just the synthetic capabilties, but all real
capabilities reported by hardware including synthetic ones. For e.g. 
if we turn off SGX, but HW still exposes it, the new snapshot will 
have SGX but previous copy doesn't as it has gone through some more
filtering during enabling. That is what resulted in the miscompare. 

Does this replacement sound better?

Ignore any changes to capabilities applied by the kernel during any
feature enabling and check against raw capabilities exposed by the
hardware


> 
> > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> > index 387578049de0..ac2e67156b9b 100644
> > --- a/arch/x86/include/asm/processor.h
> > +++ b/arch/x86/include/asm/processor.h
> > @@ -697,6 +697,7 @@ bool xen_set_default_idle(void);
> >  #endif
> >  
> >  void __noreturn stop_this_cpu(void *dummy);
> > +void microcode_store_cpu_caps(struct cpuinfo_x86 *info);
> >  void microcode_check(struct cpuinfo_x86 *info);
> >  
> >  enum l1tf_mitigations {
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index b9c7529c920e..7c86c6fd07ae 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -2297,28 +2297,43 @@ void cpu_init_secondary(void)
> >  #endif
> >  
> >  #ifdef CONFIG_MICROCODE_LATE_LOADING
> > +
> > +void microcode_store_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.
> >   */
> > -void microcode_check(struct cpuinfo_x86 *info)
> > +void microcode_check(struct cpuinfo_x86 *orig)
> >  {
> > -	perf_check_microcode();
> > +	struct cpuinfo_x86 info;
> 
> 'info' is kinda a throwaway name.  would this be better as:
> 
> 	prev_info / new_info
> 
> instead of:
> 
> 	orig / info
> 
> ?

Yes, that sounds better.

> 
> > -	/* Reload CPUID max function as it might've changed. */
> > -	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().
> >  	 */
> 
> This comment got copied to microcode_store_cpu_caps().  Does this
> instance still need to be here?

I think it still applies.. get_cpu_cap() copies all reported capabilities
and adds the synthetic ones too.

> 
> > -	memcpy(&info->x86_capability, &boot_cpu_data.x86_capability, sizeof(info->x86_capability));
> > -
> > -	get_cpu_cap(info);
> > +	microcode_store_cpu_caps(&info);
> >  
> > -	if (!memcmp(&info->x86_capability, &boot_cpu_data.x86_capability, sizeof(info->x86_capability)))
> > +	if (!memcmp(&info.x86_capability, &orig->x86_capability,
> > +		    sizeof(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 d86a4f910a6b..14d9031ed68a 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.
> > +	 */
> > +	microcode_store_cpu_caps(&info);
> 
> A "we" snuck in there.  How about this?
> 
> 	/*
> 	 * Take a snapshot before the microcode update.  This enables
> 	 * a later comparison to see any bits changed after an update.
> 	 */
> 
> I do think some better naming of 'info' here would be nice too.
> 'old_info' or 'prev_info' seem like good alternatives.

Sounds good. 

Cheers,
Ashok

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

[Borislav Petkov]

On Tue, Jan 03, 2023 at 10:02:07AM -0800, Ashok Raj wrote:
> This is a preparation before the next patch uses this to compare CPU

Once a patch is in git, the concept of "subsequent" or "next" patch becomes
ambiguous depending on how you're sorting them.

So you should strive for your commit messages to make sense on their own,
without referencing other "subsequent" or "next" patches.

> capabilities after performing an update.
> 
> Add a parameter to store CPU capabilities before performing a microcode
> update.

	" ... so that code later can do X."

And that is enough for an explanation.

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 9cfca3d7d0e2..b9c7529c920e 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -2302,25 +2302,23 @@ void cpu_init_secondary(void)
>   * only when microcode has been updated. Caller holds microcode_mutex and CPU
>   * hotplug lock.

<--- I guess you can document that new parameter here.

>   */
> -void microcode_check(void)
> +void microcode_check(struct cpuinfo_x86 *info)

...

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Tue, Jan 03, 2023 at 10:02:08AM -0800, Ashok Raj wrote:
> Fixes: 1008c52c09dc ("x86/CPU: Add a microcode loader callback")

Why a Fixes tag? Do you have a failure scenario for current kernels?

If so, then it would need stable backporting.

If so, it would need the previous patch too.

> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 387578049de0..ac2e67156b9b 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -697,6 +697,7 @@ bool xen_set_default_idle(void);
>  #endif
>  
>  void __noreturn stop_this_cpu(void *dummy);
> +void microcode_store_cpu_caps(struct cpuinfo_x86 *info);

s/microcode_store_cpu_caps/store_cpu_caps/g

>  void microcode_check(struct cpuinfo_x86 *info);
>  
>  enum l1tf_mitigations {
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index b9c7529c920e..7c86c6fd07ae 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -2297,28 +2297,43 @@ void cpu_init_secondary(void)
>  #endif
>  
>  #ifdef CONFIG_MICROCODE_LATE_LOADING
> +
> +void microcode_store_cpu_caps(struct cpuinfo_x86 *info)
> +{
> +	/* Reload CPUID max function as it might've changed. */

Might've changed how?

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

split that comment and put the second part...

> +	 */
> +	memcpy(info->x86_capability, &boot_cpu_data.x86_capability,
> +	       sizeof(info->x86_capability));
> +

... here:

	/*
	 * ... the ones coming from CPUID will get overwritten here:
	 */

> +	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.
>   */
> -void microcode_check(struct cpuinfo_x86 *info)
> +void microcode_check(struct cpuinfo_x86 *orig)
					   ^^^^^

Yeah, what dhansen said.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Tue, Jan 03, 2023 at 10:02:09AM -0800, Ashok Raj wrote:
> Right now, microcode loading failures and successes print the same
> message "Reloading completed". This is misleading to users.

9bd681251b7c ("x86/microcode: Announce reload operation's completion")

-- 
Regards/Gruss,
    Boris.

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

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

[Ashok Raj]

On Wed, Jan 04, 2023 at 08:00:05PM +0100, Borislav Petkov wrote:
> On Tue, Jan 03, 2023 at 10:02:09AM -0800, Ashok Raj wrote:
> > Right now, microcode loading failures and successes print the same
> > message "Reloading completed". This is misleading to users.
> 
> 9bd681251b7c ("x86/microcode: Announce reload operation's completion")

Thanks.. yes I can add when I resent the series.

Cheers,
Ashok

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

[Borislav Petkov]

On Fri, Jan 06, 2023 at 11:42:32AM -0800, Ashok Raj wrote:
> > 9bd681251b7c ("x86/microcode: Announce reload operation's completion")
> 
> Thanks.. yes I can add when I resent the series.

No, you should read the commit message:

"issue a single line to dmesg after the reload ... to let the user know that a
reload has at least been attempted."

and drop your patch.

We issue that line unconditionally to give feedback to the user that the attempt
was actually tried. Otherwise, you don't get any feedback and you wonder whether
it is doing anything.

The prev and next revision:

	"microcode revision: 0x%x -> 0x%x\n",

will tell you whether something got loaded or not.

-- 
Regards/Gruss,
    Boris.

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

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

[Ashok Raj]

On Fri, Jan 06, 2023 at 08:54:30PM +0100, Borislav Petkov wrote:
> On Fri, Jan 06, 2023 at 11:42:32AM -0800, Ashok Raj wrote:
> > > 9bd681251b7c ("x86/microcode: Announce reload operation's completion")
> > 
> > Thanks.. yes I can add when I resent the series.
> 
> No, you should read the commit message:
> 
> "issue a single line to dmesg after the reload ... to let the user know that a
> reload has at least been attempted."
> 
> and drop your patch.
> 
> We issue that line unconditionally to give feedback to the user that the attempt
> was actually tried. Otherwise, you don't get any feedback and you wonder whether
> it is doing anything.
> 
> The prev and next revision:
> 
> 	"microcode revision: 0x%x -> 0x%x\n",
> 
> will tell you whether something got loaded or not.

Yes, that makes sense, Do you think we can add a note that the loading
failed? since the old -> new, new is coming from new microcode rev.

Reload failed/completed ? 

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

[Ashok Raj]

On Wed, Jan 04, 2023 at 07:56:52PM +0100, Borislav Petkov wrote:
> On Tue, Jan 03, 2023 at 10:02:08AM -0800, Ashok Raj wrote:
> > Fixes: 1008c52c09dc ("x86/CPU: Add a microcode loader callback")
> 
> Why a Fixes tag? Do you have a failure scenario for current kernels?

When we reload the same microcode there is no change in CPUID bits, but if
the kernel has turned off a feature, in my case it was SGX. So bsp copy has
SGX off, but new get_cpu_cap() seems to think SGX is there since this is
unfiltered by the kernel. 

This results in a miscompare. I have noticed even when i load a brand new
patch but has no change in CPUID, report there might have been a change in
CPUID.

> 
> If so, then it would need stable backporting.
> 
> If so, it would need the previous patch too.

I think so, but your call.


> 
> > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> > index 387578049de0..ac2e67156b9b 100644
> > --- a/arch/x86/include/asm/processor.h
> > +++ b/arch/x86/include/asm/processor.h
> > @@ -697,6 +697,7 @@ bool xen_set_default_idle(void);
> >  #endif
> >  
> >  void __noreturn stop_this_cpu(void *dummy);
> > +void microcode_store_cpu_caps(struct cpuinfo_x86 *info);
> 
> s/microcode_store_cpu_caps/store_cpu_caps/g

Yes, i'll change.

> 
> >  void microcode_check(struct cpuinfo_x86 *info);
> >  
> >  enum l1tf_mitigations {
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index b9c7529c920e..7c86c6fd07ae 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -2297,28 +2297,43 @@ void cpu_init_secondary(void)
> >  #endif
> >  
> >  #ifdef CONFIG_MICROCODE_LATE_LOADING
> > +
> > +void microcode_store_cpu_caps(struct cpuinfo_x86 *info)
> > +{
> > +	/* Reload CPUID max function as it might've changed. */
> 
> Might've changed how?

This comment existed in the previous microcode_check(). I just moved it
around during the refactor.

I suppose new microcode can bring new features and this max function
enumeration can also change?

> 
> > +	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...
> 
> split that comment and put the second part...
> 
> > +	 */
> > +	memcpy(info->x86_capability, &boot_cpu_data.x86_capability,
> > +	       sizeof(info->x86_capability));
> > +
> 
> ... here:

Will do.

> 
> 	/*
> 	 * ... the ones coming from CPUID will get overwritten here:
> 	 */
> 
> > +	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.
> >   */
> > -void microcode_check(struct cpuinfo_x86 *info)
> > +void microcode_check(struct cpuinfo_x86 *orig)
> 					   ^^^^^
> 
> Yeah, what dhansen said.

Yes, I'm changing that as well.

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

[Borislav Petkov]

On Fri, Jan 06, 2023 at 12:29:00PM -0800, Ashok Raj wrote:
> Yes, that makes sense, Do you think we can add a note that the loading
> failed? since the old -> new, new is coming from new microcode rev.

It has failed when

old == new.

I.e.,

	"microcode revision: 0x1a -> 0x1a"

when the current revision on the CPU is 0x1a.

-- 
Regards/Gruss,
    Boris.

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

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

[Ashok Raj]

On Fri, Jan 06, 2023 at 09:45:24PM +0100, Borislav Petkov wrote:
> On Fri, Jan 06, 2023 at 12:29:00PM -0800, Ashok Raj wrote:
> > Yes, that makes sense, Do you think we can add a note that the loading
> > failed? since the old -> new, new is coming from new microcode rev.
> 
> It has failed when
> 
> old == new.
> 
> I.e.,
> 
> 	"microcode revision: 0x1a -> 0x1a"
> 
> when the current revision on the CPU is 0x1a.

That's fine too. I'll drop the patch.

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

[Luck, Tony]

> We issue that line unconditionally to give feedback to the user that the attempt
> was actually tried. Otherwise, you don't get any feedback and you wonder whether
> it is doing anything.
>
> The prev and next revision:
>
>	"microcode revision: 0x%x -> 0x%x\n",
>
> will tell you whether something got loaded or not.

Seems like a very subtle indication of a possibly important failure.

E.g. There is some security update with new microcode to mitigate.

User downloads the new microcode. Runs:

# echo 1 > /sys/devices/system/cpu/microcode/reload

	[This fails for some reason]

Looks at console

# dmesg | tail -1
microcode revision: 0x40001a -> 0x4001a

User doesn't notice that the version didn't change and thinks
that all is well.

Is there an earlier message that says something like this?

microcode: initiating update to version 0x5003f

-Tony

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

[Borislav Petkov]

On Fri, Jan 06, 2023 at 09:35:41PM +0000, Luck, Tony wrote:
> # dmesg | tail -1
> microcode revision: 0x40001a -> 0x4001a
> 
> User doesn't notice that the version didn't change and thinks
> that all is well.

The version did change. One '0' less.

Users don't read even if you put in there:

	"Corrected error, no action required."

User still complains about maybe her hardware going bad and "should I replace my
CPU" yadda yadda...

But whatever, since both of you think this is sooo important, then pls do this:

Success: "Reload completed, microcode revision: 0x%x -> 0x%x\n"

Failure: "Reload failed, current microcode revision: 0x%x\n"

Or something along those lines.

-- 
Regards/Gruss,
    Boris.

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

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

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Jan 06, 2023 at 12:29:00PM -0800, Ashok Raj wrote:
> > Yes, that makes sense, Do you think we can add a note that the loading
> > failed? since the old -> new, new is coming from new microcode rev.
> 
> It has failed when
> 
> old == new.
> 
> I.e.,
> 
> 	"microcode revision: 0x1a -> 0x1a"
> 
> when the current revision on the CPU is 0x1a.

So wouldn't it make sense to also display the fact that the microcode 
loading failed?

Seeing '0x1a -> 0x1a' one might naively assume from the wording alone that 
it got "reloaded" or somehow reset, or that there's some sub-revision 
update that isn't visible in the revision version - when in fact nothing 
happened, right?

The kernel usually tries to tell users unambigiously when some requested 
operation didn't succeed - not just hint at it somewhat 
passive-aggressively.

Thanks,

	Ingo