arch/x86/include/asm/microcode.h | 2 +- arch/x86/kernel/cpu/microcode/amd.c | 6 +- arch/x86/kernel/cpu/microcode/core.c | 58 +++++++++++++---------- arch/x86/kernel/cpu/microcode/intel.c | 2 +- arch/x86/kernel/cpu/microcode/internal.h | 1 +- arch/x86/kernel/head32.c | 4 +-- 6 files changed, 41 insertions(+), 32 deletions(-)
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: eb72bdfbd0a757f30ebe4f9ec161cb246d19e5ed
Gitweb: https://git.kernel.org/tip/eb72bdfbd0a757f30ebe4f9ec161cb246d19e5ed
Author: Borislav Petkov (AMD) <bp@alien8.de>
AuthorDate: Mon, 14 Apr 2025 11:59:33 +02:00
Committer: Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Sat, 03 May 2025 16:40:56 +02:00
x86/microcode: Consolidate the loader enablement checking
Consolidate the whole logic which determines whether the microcode loader
should be enabled or not into a single function and call it everywhere.
Well, almost everywhere - not in mk_early_pgtbl_32() because there the kernel
is running without paging enabled and checking dis_ucode_ldr et al would
require physical addresses and uglification of the code.
But since this is 32-bit, the easier thing to do is to simply map the initrd
unconditionally especially since that mapping is getting removed later anyway
by zap_early_initrd_mapping() and avoid the uglification.
Fixes: 4c585af7180c1 ("x86/boot/32: Temporarily map initrd for microcode loading")
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Cc: <stable@kernel.org>
Link: https://lore.kernel.org/r/CANpbe9Wm3z8fy9HbgS8cuhoj0TREYEEkBipDuhgkWFvqX0UoVQ@mail.gmail.com
---
arch/x86/include/asm/microcode.h | 2 +-
arch/x86/kernel/cpu/microcode/amd.c | 6 +-
arch/x86/kernel/cpu/microcode/core.c | 58 +++++++++++++----------
arch/x86/kernel/cpu/microcode/intel.c | 2 +-
arch/x86/kernel/cpu/microcode/internal.h | 1 +-
arch/x86/kernel/head32.c | 4 +--
6 files changed, 41 insertions(+), 32 deletions(-)
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 695e569..d53148f 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -17,10 +17,12 @@ struct ucode_cpu_info {
void load_ucode_bsp(void);
void load_ucode_ap(void);
void microcode_bsp_resume(void);
+bool __init microcode_loader_disabled(void);
#else
static inline void load_ucode_bsp(void) { }
static inline void load_ucode_ap(void) { }
static inline void microcode_bsp_resume(void) { }
+bool __init microcode_loader_disabled(void) { return false; }
#endif
extern unsigned long initrd_start_early;
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 4a10d35..96cb992 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -1098,15 +1098,17 @@ static enum ucode_state load_microcode_amd(u8 family, const u8 *data, size_t siz
static int __init save_microcode_in_initrd(void)
{
- unsigned int cpuid_1_eax = native_cpuid_eax(1);
struct cpuinfo_x86 *c = &boot_cpu_data;
struct cont_desc desc = { 0 };
+ unsigned int cpuid_1_eax;
enum ucode_state ret;
struct cpio_data cp;
- if (dis_ucode_ldr || c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10)
+ if (microcode_loader_disabled() || c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10)
return 0;
+ cpuid_1_eax = native_cpuid_eax(1);
+
if (!find_blobs_in_containers(&cp))
return -EINVAL;
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index b3658d1..079f046 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -41,8 +41,8 @@
#include "internal.h"
-static struct microcode_ops *microcode_ops;
-bool dis_ucode_ldr = true;
+static struct microcode_ops *microcode_ops;
+static bool dis_ucode_ldr = false;
bool force_minrev = IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV);
module_param(force_minrev, bool, S_IRUSR | S_IWUSR);
@@ -84,6 +84,9 @@ static bool amd_check_current_patch_level(void)
u32 lvl, dummy, i;
u32 *levels;
+ if (x86_cpuid_vendor() != X86_VENDOR_AMD)
+ return false;
+
native_rdmsr(MSR_AMD64_PATCH_LEVEL, lvl, dummy);
levels = final_levels;
@@ -95,27 +98,29 @@ static bool amd_check_current_patch_level(void)
return false;
}
-static bool __init check_loader_disabled_bsp(void)
+bool __init microcode_loader_disabled(void)
{
- static const char *__dis_opt_str = "dis_ucode_ldr";
- const char *cmdline = boot_command_line;
- const char *option = __dis_opt_str;
+ if (dis_ucode_ldr)
+ return true;
/*
- * CPUID(1).ECX[31]: reserved for hypervisor use. This is still not
- * completely accurate as xen pv guests don't see that CPUID bit set but
- * that's good enough as they don't land on the BSP path anyway.
+ * Disable when:
+ *
+ * 1) The CPU does not support CPUID.
+ *
+ * 2) Bit 31 in CPUID[1]:ECX is clear
+ * The bit is reserved for hypervisor use. This is still not
+ * completely accurate as XEN PV guests don't see that CPUID bit
+ * set, but that's good enough as they don't land on the BSP
+ * path anyway.
+ *
+ * 3) Certain AMD patch levels are not allowed to be
+ * overwritten.
*/
- if (native_cpuid_ecx(1) & BIT(31))
- return true;
-
- if (x86_cpuid_vendor() == X86_VENDOR_AMD) {
- if (amd_check_current_patch_level())
- return true;
- }
-
- if (cmdline_find_option_bool(cmdline, option) <= 0)
- dis_ucode_ldr = false;
+ if (!have_cpuid_p() ||
+ native_cpuid_ecx(1) & BIT(31) ||
+ amd_check_current_patch_level())
+ dis_ucode_ldr = true;
return dis_ucode_ldr;
}
@@ -125,7 +130,10 @@ void __init load_ucode_bsp(void)
unsigned int cpuid_1_eax;
bool intel = true;
- if (!have_cpuid_p())
+ if (cmdline_find_option_bool(boot_command_line, "dis_ucode_ldr") > 0)
+ dis_ucode_ldr = true;
+
+ if (microcode_loader_disabled())
return;
cpuid_1_eax = native_cpuid_eax(1);
@@ -146,9 +154,6 @@ void __init load_ucode_bsp(void)
return;
}
- if (check_loader_disabled_bsp())
- return;
-
if (intel)
load_ucode_intel_bsp(&early_data);
else
@@ -159,6 +164,11 @@ void load_ucode_ap(void)
{
unsigned int cpuid_1_eax;
+ /*
+ * Can't use microcode_loader_disabled() here - .init section
+ * hell. It doesn't have to either - the BSP variant must've
+ * parsed cmdline already anyway.
+ */
if (dis_ucode_ldr)
return;
@@ -810,7 +820,7 @@ static int __init microcode_init(void)
struct cpuinfo_x86 *c = &boot_cpu_data;
int error;
- if (dis_ucode_ldr)
+ if (microcode_loader_disabled())
return -EINVAL;
if (c->x86_vendor == X86_VENDOR_INTEL)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 819199b..2a397da 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -389,7 +389,7 @@ static int __init save_builtin_microcode(void)
if (xchg(&ucode_patch_va, NULL) != UCODE_BSP_LOADED)
return 0;
- if (dis_ucode_ldr || boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
+ if (microcode_loader_disabled() || boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
return 0;
uci.mc = get_microcode_blob(&uci, true);
diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
index 5df6217..50a9702 100644
--- a/arch/x86/kernel/cpu/microcode/internal.h
+++ b/arch/x86/kernel/cpu/microcode/internal.h
@@ -94,7 +94,6 @@ static inline unsigned int x86_cpuid_family(void)
return x86_family(eax);
}
-extern bool dis_ucode_ldr;
extern bool force_minrev;
#ifdef CONFIG_CPU_SUP_AMD
diff --git a/arch/x86/kernel/head32.c b/arch/x86/kernel/head32.c
index de001b2..375f2d7 100644
--- a/arch/x86/kernel/head32.c
+++ b/arch/x86/kernel/head32.c
@@ -145,10 +145,6 @@ void __init __no_stack_protector mk_early_pgtbl_32(void)
*ptr = (unsigned long)ptep + PAGE_OFFSET;
#ifdef CONFIG_MICROCODE_INITRD32
- /* Running on a hypervisor? */
- if (native_cpuid_ecx(1) & BIT(31))
- return;
-
params = (struct boot_params *)__pa_nodebug(&boot_params);
if (!params->hdr.ramdisk_size || !params->hdr.ramdisk_image)
return;
* tip-bot2 for Borislav Petkov (AMD) <tip-bot2@linutronix.de> wrote:
> The following commit has been merged into the x86/urgent branch of tip:
>
> Commit-ID: eb72bdfbd0a757f30ebe4f9ec161cb246d19e5ed
> Gitweb: https://git.kernel.org/tip/eb72bdfbd0a757f30ebe4f9ec161cb246d19e5ed
> Author: Borislav Petkov (AMD) <bp@alien8.de>
> AuthorDate: Mon, 14 Apr 2025 11:59:33 +02:00
> Committer: Borislav Petkov (AMD) <bp@alien8.de>
> CommitterDate: Sat, 03 May 2025 16:40:56 +02:00
>
> x86/microcode: Consolidate the loader enablement checking
>
> Consolidate the whole logic which determines whether the microcode loader
> should be enabled or not into a single function and call it everywhere.
>
> Well, almost everywhere - not in mk_early_pgtbl_32() because there the kernel
> is running without paging enabled and checking dis_ucode_ldr et al would
> require physical addresses and uglification of the code.
>
> But since this is 32-bit, the easier thing to do is to simply map the initrd
> unconditionally especially since that mapping is getting removed later anyway
> by zap_early_initrd_mapping() and avoid the uglification.
>
> Fixes: 4c585af7180c1 ("x86/boot/32: Temporarily map initrd for microcode loading")
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Cc: <stable@kernel.org>
> Link: https://lore.kernel.org/r/CANpbe9Wm3z8fy9HbgS8cuhoj0TREYEEkBipDuhgkWFvqX0UoVQ@mail.gmail.com
So the title claims this 'consolidates' microcode loader enablement,
which is basically a code refactoring, but then there's a Fixes tag -
what exactly does it fix? There's no explanation in the changelog that
I can see that justifies that tag and why it's in x86/urgent.
Thanks,
Ingo
Fix this build bug:
./arch/x86/include/asm/microcode.h:27:13: warning: no previous prototype for ‘microcode_loader_disabled’ [-Wmissing-prototypes]
by adding the 'static' storage class to the !CONFIG_MICROCODE
prototype.
Also, while at it, add all the other storage classes as well for this
block of prototypes, 'extern' and 'static', respectively.
( Omitting 'extern' just because it's technically not needed
is a bad habit for header prototypes and leads to bugs like
this one. )
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/include/asm/microcode.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 6aa12aecbc95..c23849246d0a 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -16,15 +16,15 @@ struct ucode_cpu_info {
};
#ifdef CONFIG_MICROCODE
-void load_ucode_bsp(void);
-void load_ucode_ap(void);
-void microcode_bsp_resume(void);
-bool __init microcode_loader_disabled(void);
+extern void load_ucode_bsp(void);
+extern void load_ucode_ap(void);
+extern void microcode_bsp_resume(void);
+extern bool __init microcode_loader_disabled(void);
#else
static inline void load_ucode_bsp(void) { }
static inline void load_ucode_ap(void) { }
static inline void microcode_bsp_resume(void) { }
-bool __init microcode_loader_disabled(void) { return false; }
+static inline bool __init microcode_loader_disabled(void) { return false; }
#endif
extern unsigned long initrd_start_early;
On Mon, May 05, 2025 at 07:15:00AM +0200, Ingo Molnar wrote:
>
> Fix this build bug:
>
> ./arch/x86/include/asm/microcode.h:27:13: warning: no previous prototype for ‘microcode_loader_disabled’ [-Wmissing-prototypes]
>
> by adding the 'static' storage class to the !CONFIG_MICROCODE
> prototype.
Thanks, I'll merge it with my patch.
> Also, while at it, add all the other storage classes as well for this
> block of prototypes, 'extern' and 'static', respectively.
This I won't add as it is unnecessary.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
* Borislav Petkov <bp@alien8.de> wrote:
> On Mon, May 05, 2025 at 07:15:00AM +0200, Ingo Molnar wrote:
> >
> > Fix this build bug:
> >
> > ./arch/x86/include/asm/microcode.h:27:13: warning: no previous prototype for ‘microcode_loader_disabled’ [-Wmissing-prototypes]
> >
> > by adding the 'static' storage class to the !CONFIG_MICROCODE
> > prototype.
>
> Thanks, I'll merge it with my patch.
Please don't forcibly rebase trees like that, because you've just
reintroduced a bug this way in tip:x86/urgent:
# 5214a9f6c0f5 x86/microcode: Consolidate the loader enablement checking
+static inline bool __init microcode_loader_disabled(void) { return false; }
static inlines don't need an __init tag ...
This was done correctly in the commit you removed:
59e820c6de60 ("x86/microcode: Add microcode_loader_disabled() storage class for the !CONFIG_MICROCODE case")
+static inline bool microcode_loader_disabled(void) { return false; }
> > Also, while at it, add all the other storage classes as well for this
> > block of prototypes, 'extern' and 'static', respectively.
>
> This I won't add as it is unnecessary.
Technically the 'int' in 'unsigned int' is 'unnecessary' and could be
skipped as well with 'unsigned', right? Yet clean kernel code uses it
because it's easier to read and more consistent. Likewise, the 'extern'
storage class is a well-known and widespread way in the kernel to
document public APIs, a counterpart to 'static'. Most of our major
headers use that style.
Your change is a doubly poor choice in this particular case of
<asm/microcode.h> as well, because the header continues with an
'extern':
extern unsigned long initrd_start_early;
So by dropping that cleanup you reintroduced an inconsistency as
well...
Thanks,
Ingo
On Tue, May 06, 2025 at 12:57:08PM +0200, Ingo Molnar wrote:
> Please don't forcibly rebase trees like that, because you've just
> reintroduced a bug this way in tip:x86/urgent:
>
> # 5214a9f6c0f5 x86/microcode: Consolidate the loader enablement checking
>
> +static inline bool __init microcode_loader_disabled(void) { return false; }
>
> static inlines don't need an __init tag ...
Oh please do tell what kind of bug that is. Especially if in
a CONFIG_MICROCODE=n build *all* the callsites of that function are gone!
Geez.
> Technically the 'int' in 'unsigned int' is 'unnecessary' and could be
> skipped as well with 'unsigned', right?
And this is the same how exactly?
> Yet clean kernel code uses it because it's easier to read and more
> consistent. Likewise, the 'extern' storage class is a well-known and
> widespread way in the kernel to document public APIs, a counterpart to
> 'static'. Most of our major headers use that style.
Does this header need it?
No. Not really.
If it fixes something, yes, sure, by all means.
Gratuitous cleanups without any point: no.
--
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: 59e820c6de60e81757211fbb846129485e337ee0
Gitweb: https://git.kernel.org/tip/59e820c6de60e81757211fbb846129485e337ee0
Author: Ingo Molnar <mingo@kernel.org>
AuthorDate: Mon, 05 May 2025 07:15:04 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 05 May 2025 09:17:02 +02:00
x86/microcode: Add microcode_loader_disabled() storage class for the !CONFIG_MICROCODE case
Fix this build bug:
./arch/x86/include/asm/microcode.h:27:13: warning: no previous prototype for ‘microcode_loader_disabled’ [-Wmissing-prototypes]
by adding the 'static' storage class to the !CONFIG_MICROCODE
prototype.
Also, while at it, add all the other storage classes as well for this
block of prototypes, 'extern' and 'static', respectively.
( Omitting 'extern' just because it's technically not needed
is a bad habit for header prototypes and leads to bugs like
this one. )
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: "Borislav Petkov (AMD)" <bp@alien8.de>
Link: https://lore.kernel.org/r/aBhJVJDTlw2Y8owu@gmail.com
---
arch/x86/include/asm/microcode.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index d53148f..b68fc9d 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -14,15 +14,15 @@ struct ucode_cpu_info {
};
#ifdef CONFIG_MICROCODE
-void load_ucode_bsp(void);
-void load_ucode_ap(void);
-void microcode_bsp_resume(void);
-bool __init microcode_loader_disabled(void);
+extern void load_ucode_bsp(void);
+extern void load_ucode_ap(void);
+extern void microcode_bsp_resume(void);
+extern bool __init microcode_loader_disabled(void);
#else
static inline void load_ucode_bsp(void) { }
static inline void load_ucode_ap(void) { }
static inline void microcode_bsp_resume(void) { }
-bool __init microcode_loader_disabled(void) { return false; }
+static inline bool microcode_loader_disabled(void) { return false; }
#endif
extern unsigned long initrd_start_early;The following commit has been merged into the x86/microcode branch of tip:
Commit-ID: e26d770a995f6f1ddb7c0c6d24f1aa81fb41eaa4
Gitweb: https://git.kernel.org/tip/e26d770a995f6f1ddb7c0c6d24f1aa81fb41eaa4
Author: Ingo Molnar <mingo@kernel.org>
AuthorDate: Mon, 05 May 2025 07:15:04 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 05 May 2025 07:15:27 +02:00
x86/microcode: Add microcode_loader_disabled() storage class for the !CONFIG_MICROCODE case
Fix this build bug:
./arch/x86/include/asm/microcode.h:27:13: warning: no previous prototype for ‘microcode_loader_disabled’ [-Wmissing-prototypes]
by adding the 'static' storage class to the !CONFIG_MICROCODE
prototype.
Also, while at it, add all the other storage classes as well for this
block of prototypes, 'extern' and 'static', respectively.
( Omitting 'extern' just because it's technically not needed
is a bad habit for header prototypes and leads to bugs like
this one. )
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: "Borislav Petkov (AMD)" <bp@alien8.de>
Link: https://lore.kernel.org/r/aBhJVJDTlw2Y8owu@gmail.com
---
arch/x86/include/asm/microcode.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index d53148f..af33bbf 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -14,15 +14,15 @@ struct ucode_cpu_info {
};
#ifdef CONFIG_MICROCODE
-void load_ucode_bsp(void);
-void load_ucode_ap(void);
-void microcode_bsp_resume(void);
-bool __init microcode_loader_disabled(void);
+extern void load_ucode_bsp(void);
+extern void load_ucode_ap(void);
+extern void microcode_bsp_resume(void);
+extern bool __init microcode_loader_disabled(void);
#else
static inline void load_ucode_bsp(void) { }
static inline void load_ucode_ap(void) { }
static inline void microcode_bsp_resume(void) { }
-bool __init microcode_loader_disabled(void) { return false; }
+static inline bool __init microcode_loader_disabled(void) { return false; }
#endif
extern unsigned long initrd_start_early;© 2016 - 2026 Red Hat, Inc.