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

Borislav Petkov
2023-12-01T16:40:04Z

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.