[PATCH 0/2] x86/microcode: Unify early reporting

[PATCH 0/2] x86/microcode: Unify early reporting

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

Hi,

this has been a pet peeve of mine for a while now and Linus prompted me
to finally do it.

So, this reworks the early microcode revision reporting on both sides of
the loader and unifies them into issuing only two lines max, for
example:

microcode: Current revision: 0x0a201025
microcode: Updated early from: 0x0a201016

The per-thread microcode revisions are in /proc/cpuinfo anyway so
grepping that is what people who are really interested, should do while
dmesg remains *not* flooded with the same revision number over and over
again.

Thx.

Borislav Petkov (AMD) (2):
  x86/microcode: Remove the driver announcement and version
  x86/microcode: Rework early revisions reporting

 arch/x86/kernel/cpu/microcode/amd.c      | 39 +++++++-----------------
 arch/x86/kernel/cpu/microcode/core.c     | 16 ++++++----
 arch/x86/kernel/cpu/microcode/intel.c    | 17 +++++------
 arch/x86/kernel/cpu/microcode/internal.h | 14 ++++++---
 4 files changed, 38 insertions(+), 48 deletions(-)


base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
-- 
2.42.0.rc0.25.ga82fb66fed25

[PATCH 1/2] x86/microcode: Remove the driver announcement and version

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

First of all, the print is useless. The driver will either load and say
which microcode revision the machine has or issue an error.

Then, the version number is meaningless and actively confusing, as Yazen
mentioned recently: when a subset of patches are backported to a distro
kernel, one can't assume the driver version is the same as the upstream
one. And besides, the version number of the loader hasn't been used and
incremented for a long time. So drop it.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 arch/x86/kernel/cpu/microcode/core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 666d25bbc5ad..b4be3a2c79df 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -41,8 +41,6 @@
 
 #include "internal.h"
 
-#define DRIVER_VERSION	"2.2"
-
 static struct microcode_ops	*microcode_ops;
 bool dis_ucode_ldr = true;
 
@@ -846,8 +844,6 @@ static int __init microcode_init(void)
 	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "x86/microcode:online",
 			  mc_cpu_online, mc_cpu_down_prep);
 
-	pr_info("Microcode Update Driver: v%s.", DRIVER_VERSION);
-
 	return 0;
 
  out_pdev:
-- 
2.42.0.rc0.25.ga82fb66fed25

[tip: x86/urgent] x86/microcode: Remove the driver announcement and version

[tip-bot2 for Borislav Petkov (AMD)]

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

Commit-ID:     2e569ada424c40ce27c99bfab4f9780619061c83
Gitweb:        https://git.kernel.org/tip/2e569ada424c40ce27c99bfab4f9780619061c83
Author:        Borislav Petkov (AMD) <bp@alien8.de>
AuthorDate:    Wed, 15 Nov 2023 22:02:11 +01:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 21 Nov 2023 16:20:49 +01:00

x86/microcode: Remove the driver announcement and version

First of all, the print is useless. The driver will either load and say
which microcode revision the machine has or issue an error.

Then, the version number is meaningless and actively confusing, as Yazen
mentioned recently: when a subset of patches are backported to a distro
kernel, one can't assume the driver version is the same as the upstream
one. And besides, the version number of the loader hasn't been used and
incremented for a long time. So drop it.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20231115210212.9981-2-bp@alien8.de
---
 arch/x86/kernel/cpu/microcode/core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 666d25b..b4be3a2 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -41,8 +41,6 @@
 
 #include "internal.h"
 
-#define DRIVER_VERSION	"2.2"
-
 static struct microcode_ops	*microcode_ops;
 bool dis_ucode_ldr = true;
 
@@ -846,8 +844,6 @@ static int __init microcode_init(void)
 	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "x86/microcode:online",
 			  mc_cpu_online, mc_cpu_down_prep);
 
-	pr_info("Microcode Update Driver: v%s.", DRIVER_VERSION);
-
 	return 0;
 
  out_pdev:

[PATCH 2/2] x86/microcode: Rework early revisions reporting

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

The AMD side of the loader issues the microcode revision for each
logical thread on the system, which can become really noisy on huge
machines. And doing that doesn't make a whole lot of sense - the
microcode revision is already in /proc/cpuinfo.

So in case one is interested in the theoretical support of mixed silicon
steppings on AMD, one can check there.

What is also missing on the AMD side - something which people have
requested before - is showing the microcode revision the CPU had
*before* the early update.

So abstract that up in the main code and have the BSP on each vendor
provide those revision numbers.

Then, dump them only once on driver init.

On Intel, do not dump the patch date - it is not needed.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://lore.kernel.org/r/CAHk-=wg=%2B8rceshMkB4VnKxmRccVLtBLPBawnewZuuqyx5U=3A@mail.gmail.com
---
 arch/x86/kernel/cpu/microcode/amd.c      | 39 +++++++-----------------
 arch/x86/kernel/cpu/microcode/core.c     | 12 ++++++--
 arch/x86/kernel/cpu/microcode/intel.c    | 17 +++++------
 arch/x86/kernel/cpu/microcode/internal.h | 14 ++++++---
 4 files changed, 38 insertions(+), 44 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 9373ec01c5ae..13b45b9c806d 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -104,8 +104,6 @@ struct cont_desc {
 	size_t		     size;
 };
 
-static u32 ucode_new_rev;
-
 /*
  * Microcode patch container file is prepended to the initrd in cpio
  * format. See Documentation/arch/x86/microcode.rst
@@ -442,12 +440,11 @@ static int __apply_microcode_amd(struct microcode_amd *mc)
  *
  * Returns true if container found (sets @desc), false otherwise.
  */
-static bool early_apply_microcode(u32 cpuid_1_eax, void *ucode, size_t size)
+static bool early_apply_microcode(u32 cpuid_1_eax, u32 old_rev, void *ucode, size_t size)
 {
 	struct cont_desc desc = { 0 };
 	struct microcode_amd *mc;
 	bool ret = false;
-	u32 rev, dummy;
 
 	desc.cpuid_1_eax = cpuid_1_eax;
 
@@ -457,22 +454,15 @@ static bool early_apply_microcode(u32 cpuid_1_eax, void *ucode, size_t size)
 	if (!mc)
 		return ret;
 
-	native_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
-
 	/*
 	 * Allow application of the same revision to pick up SMT-specific
 	 * changes even if the revision of the other SMT thread is already
 	 * up-to-date.
 	 */
-	if (rev > mc->hdr.patch_id)
+	if (old_rev > mc->hdr.patch_id)
 		return ret;
 
-	if (!__apply_microcode_amd(mc)) {
-		ucode_new_rev = mc->hdr.patch_id;
-		ret = true;
-	}
-
-	return ret;
+	return !__apply_microcode_amd(mc);
 }
 
 static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family)
@@ -506,9 +496,12 @@ static void __init find_blobs_in_containers(unsigned int cpuid_1_eax, struct cpi
 	*ret = cp;
 }
 
-void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax)
+void __init load_ucode_amd_bsp(struct early_load_data *ed, unsigned int cpuid_1_eax)
 {
 	struct cpio_data cp = { };
+	u32 dummy;
+
+	native_rdmsr(MSR_AMD64_PATCH_LEVEL, ed->old_rev, dummy);
 
 	/* Needed in load_microcode_amd() */
 	ucode_cpu_info[0].cpu_sig.sig = cpuid_1_eax;
@@ -517,7 +510,8 @@ void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax)
 	if (!(cp.data && cp.size))
 		return;
 
-	early_apply_microcode(cpuid_1_eax, cp.data, cp.size);
+	if (early_apply_microcode(cpuid_1_eax, ed->old_rev, cp.data, cp.size))
+		native_rdmsr(MSR_AMD64_PATCH_LEVEL, ed->new_rev, dummy);
 }
 
 static enum ucode_state load_microcode_amd(u8 family, const u8 *data, size_t size);
@@ -625,10 +619,8 @@ void reload_ucode_amd(unsigned int cpu)
 	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
 
 	if (rev < mc->hdr.patch_id) {
-		if (!__apply_microcode_amd(mc)) {
-			ucode_new_rev = mc->hdr.patch_id;
-			pr_info("reload patch_level=0x%08x\n", ucode_new_rev);
-		}
+		if (!__apply_microcode_amd(mc))
+			pr_info_once("reload revision: 0x%08x\n", mc->hdr.patch_id);
 	}
 }
 
@@ -649,8 +641,6 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 	if (p && (p->patch_id == csig->rev))
 		uci->mc = p->data;
 
-	pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
-
 	return 0;
 }
 
@@ -691,8 +681,6 @@ static enum ucode_state apply_microcode_amd(int cpu)
 	rev = mc_amd->hdr.patch_id;
 	ret = UCODE_UPDATED;
 
-	pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
-
 out:
 	uci->cpu_sig.rev = rev;
 	c->microcode	 = rev;
@@ -935,11 +923,6 @@ struct microcode_ops * __init init_amd_microcode(void)
 		pr_warn("AMD CPU family 0x%x not supported\n", c->x86);
 		return NULL;
 	}
-
-	if (ucode_new_rev)
-		pr_info_once("microcode updated early to new patch_level=0x%08x\n",
-			     ucode_new_rev);
-
 	return &microcode_amd_ops;
 }
 
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index b4be3a2c79df..2eab4014ba02 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -75,6 +75,8 @@ static u32 final_levels[] = {
 	0, /* T-101 terminator */
 };
 
+struct early_load_data early_data;
+
 /*
  * Check the current patch level on this CPU.
  *
@@ -153,9 +155,9 @@ void __init load_ucode_bsp(void)
 		return;
 
 	if (intel)
-		load_ucode_intel_bsp();
+		load_ucode_intel_bsp(&early_data);
 	else
-		load_ucode_amd_bsp(cpuid_1_eax);
+		load_ucode_amd_bsp(&early_data, cpuid_1_eax);
 }
 
 void load_ucode_ap(void)
@@ -826,6 +828,12 @@ static int __init microcode_init(void)
 	if (!microcode_ops)
 		return -ENODEV;
 
+	pr_info_once("Current revision: 0x%08x\n", (early_data.new_rev ?: early_data.old_rev));
+
+	if (early_data.new_rev)
+		pr_info_once("Updated early from: 0x%08x\n",
+			     early_data.old_rev);
+
 	microcode_pdev = platform_device_register_simple("microcode", -1, NULL, 0);
 	if (IS_ERR(microcode_pdev))
 		return PTR_ERR(microcode_pdev);
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 6024feb98d29..070426b9895f 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -339,16 +339,9 @@ static enum ucode_state __apply_microcode(struct ucode_cpu_info *uci,
 static enum ucode_state apply_microcode_early(struct ucode_cpu_info *uci)
 {
 	struct microcode_intel *mc = uci->mc;
-	enum ucode_state ret;
-	u32 cur_rev, date;
+	u32 cur_rev;
 
-	ret = __apply_microcode(uci, mc, &cur_rev);
-	if (ret == UCODE_UPDATED) {
-		date = mc->hdr.date;
-		pr_info_once("updated early: 0x%x -> 0x%x, date = %04x-%02x-%02x\n",
-			     cur_rev, mc->hdr.rev, date & 0xffff, date >> 24, (date >> 16) & 0xff);
-	}
-	return ret;
+	return __apply_microcode(uci, mc, &cur_rev);
 }
 
 static __init bool load_builtin_intel_microcode(struct cpio_data *cp)
@@ -413,13 +406,17 @@ static int __init save_builtin_microcode(void)
 early_initcall(save_builtin_microcode);
 
 /* Load microcode on BSP from initrd or builtin blobs */
-void __init load_ucode_intel_bsp(void)
+void __init load_ucode_intel_bsp(struct early_load_data *ed)
 {
 	struct ucode_cpu_info uci;
 
+	ed->old_rev = intel_get_microcode_revision();
+
 	uci.mc = get_microcode_blob(&uci, false);
 	if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED)
 		ucode_patch_va = UCODE_BSP_LOADED;
+
+	ed->new_rev = uci.cpu_sig.rev;
 }
 
 void load_ucode_intel_ap(void)
diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
index f8047b12329a..21776c529fa9 100644
--- a/arch/x86/kernel/cpu/microcode/internal.h
+++ b/arch/x86/kernel/cpu/microcode/internal.h
@@ -37,6 +37,12 @@ struct microcode_ops {
 				use_nmi		: 1;
 };
 
+struct early_load_data {
+	u32 old_rev;
+	u32 new_rev;
+};
+
+extern struct early_load_data early_data;
 extern struct ucode_cpu_info ucode_cpu_info[];
 struct cpio_data find_microcode_in_initrd(const char *path);
 
@@ -92,14 +98,14 @@ extern bool dis_ucode_ldr;
 extern bool force_minrev;
 
 #ifdef CONFIG_CPU_SUP_AMD
-void load_ucode_amd_bsp(unsigned int family);
+void load_ucode_amd_bsp(struct early_load_data *ed, unsigned int family);
 void load_ucode_amd_ap(unsigned int family);
 int save_microcode_in_initrd_amd(unsigned int family);
 void reload_ucode_amd(unsigned int cpu);
 struct microcode_ops *init_amd_microcode(void);
 void exit_amd_microcode(void);
 #else /* CONFIG_CPU_SUP_AMD */
-static inline void load_ucode_amd_bsp(unsigned int family) { }
+static inline void load_ucode_amd_bsp(struct early_load_data *ed, unsigned int family) { }
 static inline void load_ucode_amd_ap(unsigned int family) { }
 static inline int save_microcode_in_initrd_amd(unsigned int family) { return -EINVAL; }
 static inline void reload_ucode_amd(unsigned int cpu) { }
@@ -108,12 +114,12 @@ static inline void exit_amd_microcode(void) { }
 #endif /* !CONFIG_CPU_SUP_AMD */
 
 #ifdef CONFIG_CPU_SUP_INTEL
-void load_ucode_intel_bsp(void);
+void load_ucode_intel_bsp(struct early_load_data *ed);
 void load_ucode_intel_ap(void);
 void reload_ucode_intel(void);
 struct microcode_ops *init_intel_microcode(void);
 #else /* CONFIG_CPU_SUP_INTEL */
-static inline void load_ucode_intel_bsp(void) { }
+static inline void load_ucode_intel_bsp(struct early_load_data *ed) { }
 static inline void load_ucode_intel_ap(void) { }
 static inline void reload_ucode_intel(void) { }
 static inline struct microcode_ops *init_intel_microcode(void) { return NULL; }
-- 
2.42.0.rc0.25.ga82fb66fed25

Re: [PATCH 2/2] x86/microcode: Rework early revisions reporting

[Ashok Raj]

Hi Boris,

On Wed, Nov 15, 2023 at 10:02:12PM +0100, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> The AMD side of the loader issues the microcode revision for each
> logical thread on the system, which can become really noisy on huge
> machines. And doing that doesn't make a whole lot of sense - the
> microcode revision is already in /proc/cpuinfo.
> 
> So in case one is interested in the theoretical support of mixed silicon
> steppings on AMD, one can check there.
> 
> What is also missing on the AMD side - something which people have
> requested before - is showing the microcode revision the CPU had
> *before* the early update.
> 
> So abstract that up in the main code and have the BSP on each vendor
> provide those revision numbers.
> 
> Then, dump them only once on driver init.
> 
> On Intel, do not dump the patch date - it is not needed.
> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Link: https://lore.kernel.org/r/CAHk-=wg=%2B8rceshMkB4VnKxmRccVLtBLPBawnewZuuqyx5U=3A@mail.gmail.com
> ---
>  arch/x86/kernel/cpu/microcode/amd.c      | 39 +++++++-----------------
>  arch/x86/kernel/cpu/microcode/core.c     | 12 ++++++--
>  arch/x86/kernel/cpu/microcode/intel.c    | 17 +++++------
>  arch/x86/kernel/cpu/microcode/internal.h | 14 ++++++---
>  4 files changed, 38 insertions(+), 44 deletions(-)
> 

[snip]

>  void load_ucode_ap(void)
> @@ -826,6 +828,12 @@ static int __init microcode_init(void)
>  	if (!microcode_ops)
>  		return -ENODEV;
>  
> +	pr_info_once("Current revision: 0x%08x\n", (early_data.new_rev ?: early_data.old_rev));
> +
> +	if (early_data.new_rev)
> +		pr_info_once("Updated early from: 0x%08x\n",
> +			     early_data.old_rev);

See below, new_rev is always assigned. The above message appears even when
no new microcode was applied.

> +
>  	microcode_pdev = platform_device_register_simple("microcode", -1, NULL, 0);
>  	if (IS_ERR(microcode_pdev))
>  		return PTR_ERR(microcode_pdev);
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 6024feb98d29..070426b9895f 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -339,16 +339,9 @@ static enum ucode_state __apply_microcode(struct ucode_cpu_info *uci,
>  static enum ucode_state apply_microcode_early(struct ucode_cpu_info *uci)
>  {
>  	struct microcode_intel *mc = uci->mc;
> -	enum ucode_state ret;
> -	u32 cur_rev, date;
> +	u32 cur_rev;
>  
> -	ret = __apply_microcode(uci, mc, &cur_rev);
> -	if (ret == UCODE_UPDATED) {
> -		date = mc->hdr.date;
> -		pr_info_once("updated early: 0x%x -> 0x%x, date = %04x-%02x-%02x\n",
> -			     cur_rev, mc->hdr.rev, date & 0xffff, date >> 24, (date >> 16) & 0xff);
> -	}
> -	return ret;
> +	return __apply_microcode(uci, mc, &cur_rev);
>  }
>  
>  static __init bool load_builtin_intel_microcode(struct cpio_data *cp)
> @@ -413,13 +406,17 @@ static int __init save_builtin_microcode(void)
>  early_initcall(save_builtin_microcode);
>  
>  /* Load microcode on BSP from initrd or builtin blobs */
> -void __init load_ucode_intel_bsp(void)
> +void __init load_ucode_intel_bsp(struct early_load_data *ed)
>  {
>  	struct ucode_cpu_info uci;
>  
> +	ed->old_rev = intel_get_microcode_revision();
> +
>  	uci.mc = get_microcode_blob(&uci, false);
>  	if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED)
>  		ucode_patch_va = UCODE_BSP_LOADED;
> +
> +	ed->new_rev = uci.cpu_sig.rev;

new_rev is always assigned even if there was no microcode to apply.

Feel free to squash this patch if it makes sense.

From: Ashok Raj <ashok.raj@intel.com>
Date: Tue, 28 Nov 2023 15:30:31 -0800
Subject: [PATCH] x86/microcode: Suppress early load message when the revision
 is unchanged

After early loading, early_data.old_rev is always updated, that results in
printing a message even if the revision is unchanged.

Currently, it's displayed as below:

[  113.395868] microcode: Current revision: 0x21000170
[  113.404244] microcode: Updated early from: 0x21000170

This should happen on both AMD and Intel. Although for different reasons.

- On AMD, the ucode is loaded even if the current revision matches what is
  being loaded.
- On Intel, load_ucode_intel_bsp() assigns new_rev unconditionally. So it's
  never 0.

Suppress the "Updated early" message when revision is unchanged.

Fixes: 080990aa3344 ("x86/microcode: Rework early revisions reporting")
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/kernel/cpu/microcode/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 232026a239a6..18e61ecab005 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -830,7 +830,7 @@ static int __init microcode_init(void)
 
 	pr_info_once("Current revision: 0x%08x\n", (early_data.new_rev ?: early_data.old_rev));
 
-	if (early_data.new_rev)
+	if (early_data.new_rev && early_data.new_rev != early_data.old_rev)
 		pr_info_once("Updated early from: 0x%08x\n", early_data.old_rev);
 
 	microcode_pdev = platform_device_register_simple("microcode", -1, NULL, 0);
-- 
2.39.2

Re: [PATCH 2/2] x86/microcode: Rework early revisions reporting

[Borislav Petkov]

On Thu, Nov 30, 2023 at 10:34:31AM -0800, Ashok Raj wrote:
> new_rev is always assigned even if there was no microcode to apply.

That is wrong.

In my defence, I don't have an Intel machine which has up-to-date
microcode in the BIOS and the blob has the same microcode revision. All
my Intel test boxes do update microcode.

> - On AMD, the ucode is loaded even if the current revision matches what is
>   being loaded.

From the commit message of the patch you're replying to:

"What is also missing on the AMD side - something which people have
requested before - is showing the microcode revision the CPU had
*before* the early update."

The logic is, we want to dump before-after everytime there was
a successful early upload.

> Currently, it's displayed as below:
> 
> [  113.395868] microcode: Current revision: 0x21000170
> [  113.404244] microcode: Updated early from: 0x21000170

There's something else weird going on though. I would expect that that
machine should not update microcode if it cannot find newer. Maybe the
scan_microcode() logic is a bit weird still.

Please run this debug + fix patch and send me full dmesg from that
machine.

Thx.

---

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 070426b9895f..41d1a86287e6 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -267,10 +267,15 @@ static __init struct microcode_intel *scan_microcode(void *data, size_t size,
 	u32 cur_rev = uci->cpu_sig.rev;
 	unsigned int mc_size;
 
+	uc_dbg("cur_rev: 0x%x", cur_rev);
+
 	for (; size >= sizeof(struct microcode_header_intel); size -= mc_size, data += mc_size) {
 		mc_header = (struct microcode_header_intel *)data;
 
 		mc_size = get_totalsize(mc_header);
+
+		uc_dbg("mc_size: %d, size: %ld", mc_size, size);
+
 		if (!mc_size || mc_size > size ||
 		    intel_microcode_sanity_check(data, false, MC_HEADER_TYPE_MICROCODE) < 0)
 			break;
@@ -278,6 +283,8 @@ static __init struct microcode_intel *scan_microcode(void *data, size_t size,
 		if (!intel_find_matching_signature(data, &uci->cpu_sig))
 			continue;
 
+		uc_dbg("save: %d, mc_header->rev: 0x%x", save, mc_header->rev);
+
 		/*
 		 * For saving the early microcode, find the matching revision which
 		 * was loaded on the BSP.
@@ -296,6 +303,8 @@ static __init struct microcode_intel *scan_microcode(void *data, size_t size,
 		cur_rev = mc_header->rev;
 	}
 
+	uc_dbg("ret, size: %ld, patch: 0x%px", size, patch);
+
 	return size ? NULL : patch;
 }
 
@@ -370,13 +379,15 @@ static __init struct microcode_intel *get_microcode_blob(struct ucode_cpu_info *
 {
 	struct cpio_data cp;
 
+	intel_collect_cpu_info(&uci->cpu_sig);
+
 	if (!load_builtin_intel_microcode(&cp))
 		cp = find_microcode_in_initrd(ucode_path);
 
 	if (!(cp.data && cp.size))
 		return NULL;
-
-	intel_collect_cpu_info(&uci->cpu_sig);
+	else
+		uc_dbg("found cp");
 
 	return scan_microcode(cp.data, cp.size, uci, save);
 }
@@ -410,13 +421,19 @@ void __init load_ucode_intel_bsp(struct early_load_data *ed)
 {
 	struct ucode_cpu_info uci;
 
-	ed->old_rev = intel_get_microcode_revision();
-
 	uci.mc = get_microcode_blob(&uci, false);
-	if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED)
+	ed->old_rev = uci.cpu_sig.rev;
+
+	uc_dbg("old_rev: 0x%x", ed->old_rev);
+
+	if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED) {
 		ucode_patch_va = UCODE_BSP_LOADED;
+		ed->new_rev = uci.cpu_sig.rev;
+
+		uc_dbg("updated, new_rev: 0x%x", ed->new_rev);
+	}
 
-	ed->new_rev = uci.cpu_sig.rev;
+	uc_dbg("done");
 }
 
 void load_ucode_intel_ap(void)
diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
index 21776c529fa9..338607da51ec 100644
--- a/arch/x86/kernel/cpu/microcode/internal.h
+++ b/arch/x86/kernel/cpu/microcode/internal.h
@@ -42,6 +42,9 @@ struct early_load_data {
 	u32 new_rev;
 };
 
+#define uc_dbg(fmt, args...)	\
+	pr_info("%s: " fmt "\n", __func__, ##args)
+
 extern struct early_load_data early_data;
 extern struct ucode_cpu_info ucode_cpu_info[];
 struct cpio_data find_microcode_in_initrd(const char *path);


-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] x86/microcode: Rework early revisions reporting

[Ashok Raj]

On Fri, Dec 01, 2023 at 05:39:28PM +0100, Borislav Petkov wrote:
> On Thu, Nov 30, 2023 at 10:34:31AM -0800, Ashok Raj wrote:
> > new_rev is always assigned even if there was no microcode to apply.
> 
> That is wrong.
> 
> In my defence, I don't have an Intel machine which has up-to-date
> microcode in the BIOS and the blob has the same microcode revision. All
> my Intel test boxes do update microcode.
> 
> > - On AMD, the ucode is loaded even if the current revision matches what is
> >   being loaded.
> 
> From the commit message of the patch you're replying to:
> 
> "What is also missing on the AMD side - something which people have
> requested before - is showing the microcode revision the CPU had
> *before* the early update."
> 
> The logic is, we want to dump before-after everytime there was
> a successful early upload.
> 
> > Currently, it's displayed as below:
> > 
> > [  113.395868] microcode: Current revision: 0x21000170
> > [  113.404244] microcode: Updated early from: 0x21000170
> 
> There's something else weird going on though. I would expect that that
> machine should not update microcode if it cannot find newer. Maybe the
> scan_microcode() logic is a bit weird still.
> 
> Please run this debug + fix patch and send me full dmesg from that
> machine.
> 

I'll get a dmesg shortly once i get my test system back.

What I meant was 

void __init load_ucode_intel_bsp(struct early_load_data *ed)
{
        struct ucode_cpu_info uci;

        ed->old_rev = intel_get_microcode_revision();

        uci.mc = get_microcode_blob(&uci, false);
        if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED)
                ucode_patch_va = UCODE_BSP_LOADED;

        ed->new_rev = uci.cpu_sig.rev;
}

ed->new_rev is always assigned, just not for the UCODE_UPDATED case. Hence
even if we had no microcode to update ed->new_rev ends up being non-zero.

Re: [PATCH 2/2] x86/microcode: Rework early revisions reporting

[Borislav Petkov]

On Fri, Dec 01, 2023 at 12:33:34PM -0800, Ashok Raj wrote:
> I'll get a dmesg shortly once i get my test system back.

Thanks.

> What I meant was 

I know what you meant. Did you see the diff I sent you?

It has the fix already:

@@ -410,13 +421,19 @@ void __init load_ucode_intel_bsp(struct early_load_data *ed)
 {
        struct ucode_cpu_info uci;

-       ed->old_rev = intel_get_microcode_revision();
-
        uci.mc = get_microcode_blob(&uci, false);
-       if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED)
+       ed->old_rev = uci.cpu_sig.rev;
+
+       uc_dbg("old_rev: 0x%x", ed->old_rev);
+
+       if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED) {
                ucode_patch_va = UCODE_BSP_LOADED;
+               ed->new_rev = uci.cpu_sig.rev;
+
+               uc_dbg("updated, new_rev: 0x%x", ed->new_rev);
+       }
^^^^^^^^^^^^^

The assignment is now inside the UCODE_UPDATED conditional.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] x86/microcode: Rework early revisions reporting

[Ashok Raj]

On Fri, Dec 01, 2023 at 09:41:46PM +0100, Borislav Petkov wrote:
> On Fri, Dec 01, 2023 at 12:33:34PM -0800, Ashok Raj wrote:
> > I'll get a dmesg shortly once i get my test system back.
> 
> Thanks.
> 

dmesg for the microcode part here below:

https://paste.debian.net/hidden/e911dffc/

> > What I meant was 
> 
> I know what you meant. Did you see the diff I sent you?
> 
> It has the fix already:
> 
> @@ -410,13 +421,19 @@ void __init load_ucode_intel_bsp(struct early_load_data *ed)
>  {
>         struct ucode_cpu_info uci;
> 
> -       ed->old_rev = intel_get_microcode_revision();
> -
>         uci.mc = get_microcode_blob(&uci, false);
> -       if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED)
> +       ed->old_rev = uci.cpu_sig.rev;
> +
> +       uc_dbg("old_rev: 0x%x", ed->old_rev);
> +
> +       if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED) {
>                 ucode_patch_va = UCODE_BSP_LOADED;
> +               ed->new_rev = uci.cpu_sig.rev;
> +
> +               uc_dbg("updated, new_rev: 0x%x", ed->new_rev);
> +       }
> ^^^^^^^^^^^^^
> 
> The assignment is now inside the UCODE_UPDATED conditional.

The first patch I tried was exactly this, but assumed having the fix  in
core.c would help both AMD/Intel.

Assuming if the same loaded patch was also present in initrd, 

load_ucode_amd_bsp()
   early_apply_microcode()
      __apply_microcode_amd()

old_rev will still be non-zero. 

Re: [PATCH 2/2] x86/microcode: Rework early revisions reporting

[Borislav Petkov]

On Fri, Dec 01, 2023 at 01:32:37PM -0800, Ashok Raj wrote:
> dmesg for the microcode part here below:
> 
> https://paste.debian.net/hidden/e911dffc/

Thanks, it is all clear now.

This should fix it:

---
From: "Borislav Petkov (AMD)" <bp@alien8.de>
Date: Fri, 1 Dec 2023 14:35:06 +0100
Subject: [PATCH] x86/microcode/intel: Set new revision only after a successful update

This was meant to be done only when early microcode got updated
successfully. Move it into the if-branch.

Also, make sure the current revision is read unconditionally and only
once.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 arch/x86/kernel/cpu/microcode/intel.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 070426b9895f..334972c097d9 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -370,14 +370,14 @@ static __init struct microcode_intel *get_microcode_blob(struct ucode_cpu_info *
 {
 	struct cpio_data cp;
 
+	intel_collect_cpu_info(&uci->cpu_sig);
+
 	if (!load_builtin_intel_microcode(&cp))
 		cp = find_microcode_in_initrd(ucode_path);
 
 	if (!(cp.data && cp.size))
 		return NULL;
 
-	intel_collect_cpu_info(&uci->cpu_sig);
-
 	return scan_microcode(cp.data, cp.size, uci, save);
 }
 
@@ -410,13 +410,13 @@ void __init load_ucode_intel_bsp(struct early_load_data *ed)
 {
 	struct ucode_cpu_info uci;
 
-	ed->old_rev = intel_get_microcode_revision();
-
 	uci.mc = get_microcode_blob(&uci, false);
-	if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED)
-		ucode_patch_va = UCODE_BSP_LOADED;
+	ed->old_rev = uci.cpu_sig.rev;
 
-	ed->new_rev = uci.cpu_sig.rev;
+	if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED) {
+		ucode_patch_va = UCODE_BSP_LOADED;
+		ed->new_rev = uci.cpu_sig.rev;
+	}
 }
 
 void load_ucode_intel_ap(void)
-- 
2.42.0.rc0.25.ga82fb66fed25

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] x86/microcode: Rework early revisions reporting

[Ashok Raj]

On Sat, Dec 02, 2023 at 05:38:50PM +0100, Borislav Petkov wrote:
> On Fri, Dec 01, 2023 at 01:32:37PM -0800, Ashok Raj wrote:
> > dmesg for the microcode part here below:
> > 
> > https://paste.debian.net/hidden/e911dffc/
> 
> Thanks, it is all clear now.
> 
> This should fix it:

Yes, this works as expected.

> 
> ---
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> Date: Fri, 1 Dec 2023 14:35:06 +0100
> Subject: [PATCH] x86/microcode/intel: Set new revision only after a successful update
> 
> This was meant to be done only when early microcode got updated
> successfully. Move it into the if-branch.
> 
> Also, make sure the current revision is read unconditionally and only
> once.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>

Tested-by: Ashok Raj <ashok.raj@intel.com>

[tip: x86/microcode] x86/microcode/intel: Set new revision only after a successful update

[tip-bot2 for Borislav Petkov (AMD)]

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

Commit-ID:     9c21ea53e6bd1104c637b80a0688040f184cc761
Gitweb:        https://git.kernel.org/tip/9c21ea53e6bd1104c637b80a0688040f184cc761
Author:        Borislav Petkov (AMD) <bp@alien8.de>
AuthorDate:    Fri, 01 Dec 2023 14:35:06 +01:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Sun, 03 Dec 2023 11:49:53 +01:00

x86/microcode/intel: Set new revision only after a successful update

This was meant to be done only when early microcode got updated
successfully. Move it into the if-branch.

Also, make sure the current revision is read unconditionally and only
once.

Fixes: 080990aa3344 ("x86/microcode: Rework early revisions reporting")
Reported-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Tested-by: Ashok Raj <ashok.raj@intel.com>
Link: https://lore.kernel.org/r/ZWjVt5dNRjbcvlzR@a4bf019067fa.jf.intel.com
---
 arch/x86/kernel/cpu/microcode/intel.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 5d6ea87..857e608 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -370,14 +370,14 @@ static __init struct microcode_intel *get_microcode_blob(struct ucode_cpu_info *
 {
 	struct cpio_data cp;
 
+	intel_collect_cpu_info(&uci->cpu_sig);
+
 	if (!load_builtin_intel_microcode(&cp))
 		cp = find_microcode_in_initrd(ucode_path);
 
 	if (!(cp.data && cp.size))
 		return NULL;
 
-	intel_collect_cpu_info(&uci->cpu_sig);
-
 	return scan_microcode(cp.data, cp.size, uci, save);
 }
 
@@ -410,13 +410,13 @@ void __init load_ucode_intel_bsp(struct early_load_data *ed)
 {
 	struct ucode_cpu_info uci;
 
-	ed->old_rev = intel_get_microcode_revision();
-
 	uci.mc = get_microcode_blob(&uci, false);
-	if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED)
-		ucode_patch_va = UCODE_BSP_LOADED;
+	ed->old_rev = uci.cpu_sig.rev;
 
-	ed->new_rev = uci.cpu_sig.rev;
+	if (uci.mc && apply_microcode_early(&uci) == UCODE_UPDATED) {
+		ucode_patch_va = UCODE_BSP_LOADED;
+		ed->new_rev = uci.cpu_sig.rev;
+	}
 }
 
 void load_ucode_intel_ap(void)

Re: [PATCH 2/2] x86/microcode: Rework early revisions reporting

[Ashok Raj]

Hi

Since the late load already announces the old and new revision in core.c

Now that early loading doesn't print the 'date' for Intel, does it make
sense to remove those for late-load as well?

Feel free to squash this into any patch you have in progress if that makes
sense.

On Wed, Nov 15, 2023 at 10:02:12PM +0100, Borislav Petkov wrote:

[snip]

> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 6024feb98d29..070426b9895f 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -339,16 +339,9 @@ static enum ucode_state __apply_microcode(struct ucode_cpu_info *uci,
>  static enum ucode_state apply_microcode_early(struct ucode_cpu_info *uci)
>  {
>  	struct microcode_intel *mc = uci->mc;
> -	enum ucode_state ret;
> -	u32 cur_rev, date;
> +	u32 cur_rev;
>  
> -	ret = __apply_microcode(uci, mc, &cur_rev);
> -	if (ret == UCODE_UPDATED) {
> -		date = mc->hdr.date;
> -		pr_info_once("updated early: 0x%x -> 0x%x, date = %04x-%02x-%02x\n",
> -			     cur_rev, mc->hdr.rev, date & 0xffff, date >> 24, (date >> 16) & 0xff);
> -	}
> -	return ret;
> +	return __apply_microcode(uci, mc, &cur_rev);
>  }


From: Ashok Raj <ashok.raj@intel.com>
Date: Wed, 29 Nov 2023 13:56:43 -0800
Subject: [PATCH 2/2] x86/microcode/intel: Remove the redundant microcode
 updated message

After successful update, core.c/late_load_stop_cpus() prints a message as
shown below.

[  215.386472] microcode: load: updated on 128 primary CPUs with 128 siblings
[  215.394370] microcode: revision: 0x21000170 -> 0x21000190

Remove the redundant message.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/kernel/cpu/microcode/intel.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 070426b9895f..5d6ea875b81b 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -457,12 +457,6 @@ static enum ucode_state apply_microcode_late(int cpu)
 	if (ret != UCODE_UPDATED && ret != UCODE_OK)
 		return ret;
 
-	if (!cpu && uci->cpu_sig.rev != cur_rev) {
-		pr_info("Updated to revision 0x%x, date = %04x-%02x-%02x\n",
-			uci->cpu_sig.rev, mc->hdr.date & 0xffff, mc->hdr.date >> 24,
-			(mc->hdr.date >> 16) & 0xff);
-	}
-
 	cpu_data(cpu).microcode	 = uci->cpu_sig.rev;
 	if (!cpu)
 		boot_cpu_data.microcode = uci->cpu_sig.rev;
-- 
2.39.2

[tip: x86/microcode] x86/microcode/intel: Remove redundant microcode late updated message

[tip-bot2 for Ashok Raj]

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

Commit-ID:     1f693ef550f051ef6a95dbfadfa4fbd600769a81
Gitweb:        https://git.kernel.org/tip/1f693ef550f051ef6a95dbfadfa4fbd600769a81
Author:        Ashok Raj <ashok.raj@intel.com>
AuthorDate:    Wed, 29 Nov 2023 13:56:43 -08:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Fri, 01 Dec 2023 18:52:01 +01:00

x86/microcode/intel: Remove redundant microcode late updated message

After successful update, the late loading routine prints an update
summary similar to:

  microcode: load: updated on 128 primary CPUs with 128 siblings
  microcode: revision: 0x21000170 -> 0x21000190

Remove the redundant message in the Intel side of the driver.

  [ bp: Massage commit message. ]

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://lore.kernel.org/r/ZWjYhedNfhAUmt0k@a4bf019067fa.jf.intel.com
---
 arch/x86/kernel/cpu/microcode/intel.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 070426b..5d6ea87 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -457,12 +457,6 @@ static enum ucode_state apply_microcode_late(int cpu)
 	if (ret != UCODE_UPDATED && ret != UCODE_OK)
 		return ret;
 
-	if (!cpu && uci->cpu_sig.rev != cur_rev) {
-		pr_info("Updated to revision 0x%x, date = %04x-%02x-%02x\n",
-			uci->cpu_sig.rev, mc->hdr.date & 0xffff, mc->hdr.date >> 24,
-			(mc->hdr.date >> 16) & 0xff);
-	}
-
 	cpu_data(cpu).microcode	 = uci->cpu_sig.rev;
 	if (!cpu)
 		boot_cpu_data.microcode = uci->cpu_sig.rev;

Re: [PATCH 0/2] x86/microcode: Unify early reporting

[Thomas Gleixner]

On Wed, Nov 15 2023 at 22:02, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> microcode: Current revision: 0x0a201025
> microcode: Updated early from: 0x0a201016

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>