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

From: Borislav Petkov <bp@alien8.de>
To: Ashok Raj <ashok.raj@intel.com>
Cc: X86 ML <x86@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH 2/2] x86/microcode: Rework early revisions reporting
Date: Fri, 1 Dec 2023 17:39:28 +0100	[thread overview]
Message-ID: <20231201163928.GCZWoMQNAAQSA9U12D@fat_crate.local> (raw)
In-Reply-To: <ZWjVt5dNRjbcvlzR@a4bf019067fa.jf.intel.com>

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
