From: "Borislav Petkov (AMD)" <[email protected]>
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 <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Link: https://lore.kernel.org/r/CAHk-=wg=%[email protected]
---
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 µcode_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
Hi Boris,
On Wed, Nov 15, 2023 at 10:02:12PM +0100, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Borislav Petkov (AMD) <[email protected]>
> Link: https://lore.kernel.org/r/CAHk-=wg=%[email protected]
> ---
> 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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
---
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
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
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 <[email protected]>
AuthorDate: Wed, 29 Nov 2023 13:56:43 -08:00
Committer: Borislav Petkov (AMD) <[email protected]>
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 <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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;
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.
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.
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
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)" <[email protected]>
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) <[email protected]>
---
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
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)" <[email protected]>
> 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) <[email protected]>
Tested-by: Ashok Raj <[email protected]>
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) <[email protected]>
AuthorDate: Fri, 01 Dec 2023 14:35:06 +01:00
Committer: Borislav Petkov (AMD) <[email protected]>
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 <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Tested-by: Ashok Raj <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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)