arch/x86/include/asm/microcode.h | 2 ++ arch/x86/kernel/cpu/microcode/amd.c | 6 ++-- arch/x86/kernel/cpu/microcode/core.c | 37 ++++++++++++++---------- 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, 29 insertions(+), 23 deletions(-)
From: "Borislav Petkov (AMD)" <bp@alien8.de>
Date: Sat, 5 Apr 2025 12:35:55 +0200
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().
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 | 37 ++++++++++++++----------
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, 29 insertions(+), 23 deletions(-)
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 695e569159c1..d53148fb893a 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 b61028cf5c8a..9b7b725643ef 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -1093,15 +1093,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 b3658d11e7b6..b6125149894b 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -42,7 +42,7 @@
#include "internal.h"
static struct microcode_ops *microcode_ops;
-bool dis_ucode_ldr = true;
+static int dis_ucode_ldr = -1;
bool force_minrev = IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV);
module_param(force_minrev, bool, S_IRUSR | S_IWUSR);
@@ -95,11 +95,20 @@ 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 < 0) {
+ if (cmdline_find_option_bool(boot_command_line, "dis_ucode_ldr") <= 0)
+ dis_ucode_ldr = 0;
+ else
+ goto disable;
+ }
+
+ if (dis_ucode_ldr > 0)
+ return true;
+
+ if (!have_cpuid_p())
+ goto disable;
/*
* CPUID(1).ECX[31]: reserved for hypervisor use. This is still not
@@ -107,17 +116,18 @@ static bool __init check_loader_disabled_bsp(void)
* that's good enough as they don't land on the BSP path anyway.
*/
if (native_cpuid_ecx(1) & BIT(31))
- return true;
+ goto disable;
if (x86_cpuid_vendor() == X86_VENDOR_AMD) {
if (amd_check_current_patch_level())
- return true;
+ goto disable;
}
- if (cmdline_find_option_bool(cmdline, option) <= 0)
- dis_ucode_ldr = false;
+ return (bool)dis_ucode_ldr;
- return dis_ucode_ldr;
+disable:
+ dis_ucode_ldr = 1;
+ return true;
}
void __init load_ucode_bsp(void)
@@ -125,7 +135,7 @@ void __init load_ucode_bsp(void)
unsigned int cpuid_1_eax;
bool intel = true;
- if (!have_cpuid_p())
+ if (microcode_loader_disabled())
return;
cpuid_1_eax = native_cpuid_eax(1);
@@ -146,9 +156,6 @@ void __init load_ucode_bsp(void)
return;
}
- if (check_loader_disabled_bsp())
- return;
-
if (intel)
load_ucode_intel_bsp(&early_data);
else
@@ -810,7 +817,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 819199bc0119..2a397da43923 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 5df621752fef..50a9702ae4e2 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 de001b2146ab..375f2d7f1762 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;
--
2.43.0
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteOn Tue, Apr 08 2025 at 19:22, Borislav Petkov wrote:
> static struct microcode_ops *microcode_ops;
> -bool dis_ucode_ldr = true;
> +static int dis_ucode_ldr = -1;
This tristate muck is disgusting.
> bool force_minrev = IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV);
> module_param(force_minrev, bool, S_IRUSR | S_IWUSR);
> @@ -95,11 +95,20 @@ 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 < 0) {
> + if (cmdline_find_option_bool(boot_command_line, "dis_ucode_ldr") <= 0)
> + dis_ucode_ldr = 0;
> + else
> + goto disable;
> + }
It just exists to make the above a one time operation. What's wrong with
having:
static void __init microcode_check_cmdline(void)
{
if (cmdline_find_option_bool(boot_command_line, "dis_ucode_ldr") <= 0)
dis_ucode_ldr = false;
}
and call that once at the proper place?
Thanks,
tglx
On Thu, Apr 10, 2025 at 01:53:25PM +0200, Thomas Gleixner wrote:
> It just exists to make the above a one time operation. What's wrong with
> having:
>
> static void __init microcode_check_cmdline(void)
> {
> if (cmdline_find_option_bool(boot_command_line, "dis_ucode_ldr") <= 0)
> dis_ucode_ldr = false;
> }
>
> and call that once at the proper place?
Yeah, I had that done for the mk_early_pgtbl_32() to avoid doing __pa gunk in
the loader. But if we're going to do it unconditionally now, I don't need the
tristate anymore.
Lemme zap it.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
From: "Borislav Petkov (AMD)" <bp@alien8.de>
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 | 40 ++++++++++++++----------
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, 31 insertions(+), 24 deletions(-)
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 695e569159c1..d53148fb893a 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 4a10d35e70aa..96cb992d50ef 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 b3658d11e7b6..541a1478ccf0 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 = true;
bool force_minrev = IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV);
module_param(force_minrev, bool, S_IRUSR | S_IWUSR);
@@ -95,11 +95,13 @@ 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;
+
+ if (!have_cpuid_p())
+ goto disable;
/*
* CPUID(1).ECX[31]: reserved for hypervisor use. This is still not
@@ -107,17 +109,18 @@ static bool __init check_loader_disabled_bsp(void)
* that's good enough as they don't land on the BSP path anyway.
*/
if (native_cpuid_ecx(1) & BIT(31))
- return true;
+ goto disable;
if (x86_cpuid_vendor() == X86_VENDOR_AMD) {
if (amd_check_current_patch_level())
- return true;
+ goto disable;
}
- if (cmdline_find_option_bool(cmdline, option) <= 0)
- dis_ucode_ldr = false;
-
return dis_ucode_ldr;
+
+disable:
+ dis_ucode_ldr = true;
+ return true;
}
void __init load_ucode_bsp(void)
@@ -125,7 +128,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 = false;
+
+ if (microcode_loader_disabled())
return;
cpuid_1_eax = native_cpuid_eax(1);
@@ -146,9 +152,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 +162,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 +818,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 819199bc0119..2a397da43923 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 5df621752fef..50a9702ae4e2 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 de001b2146ab..375f2d7f1762 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;
--
2.43.0
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteOn Mon, Apr 14 2025 at 11:59, Borislav Petkov wrote:
> -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;
> +
> + if (!have_cpuid_p())
> + goto disable;
>
> /*
> * CPUID(1).ECX[31]: reserved for hypervisor use. This is still not
> @@ -107,17 +109,18 @@ static bool __init check_loader_disabled_bsp(void)
> * that's good enough as they don't land on the BSP path anyway.
> */
> if (native_cpuid_ecx(1) & BIT(31))
> - return true;
> + goto disable;
>
> if (x86_cpuid_vendor() == X86_VENDOR_AMD) {
> if (amd_check_current_patch_level())
> - return true;
> + goto disable;
> }
>
> - if (cmdline_find_option_bool(cmdline, option) <= 0)
> - dis_ucode_ldr = false;
> -
> return dis_ucode_ldr;
This return here is confusing at best. The only valid return value is
'false' according to the above logic, because nothing modifies
dis_ucode_ldr and that must be false according to the top-most check,
no?
Something like the delta patch below makes it way more obvious and gets
rid of the ugly gotos as well.
Thanks,
tglx
---
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -84,6 +84,9 @@ static bool amd_check_current_patch_leve
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;
@@ -100,27 +103,25 @@ bool __init microcode_loader_disabled(vo
if (dis_ucode_ldr)
return true;
- if (!have_cpuid_p())
- goto disable;
-
/*
- * 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_p
+ *
+ * 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) The AMD specific patch level check succeeds
*/
- if (native_cpuid_ecx(1) & BIT(31))
- goto disable;
-
- if (x86_cpuid_vendor() == X86_VENDOR_AMD) {
- if (amd_check_current_patch_level())
- goto disable;
+ if (!have_cpuid_p() || native_cpuid_ecx(1) & BIT(31) ||
+ amd_check_current_patch_level()) {
+ dis_ucode_ldr = true;
+ return true;
}
-
- return dis_ucode_ldr;
-
-disable:
- dis_ucode_ldr = true;
- return true;
+ return false;
}
void __init load_ucode_bsp(void)
On Wed, Apr 30, 2025 at 09:16:56PM +0200, Thomas Gleixner wrote:
> This return here is confusing at best. The only valid return value is
> 'false' according to the above logic, because nothing modifies
> dis_ucode_ldr and that must be false according to the top-most check,
> no?
You mean the return value is the build-time dis_ucode_ldr value which is true.
Well, *was* true, keep on reading.
I.e., the loader was default-disabled unless we decide it is ok to turn it on.
Now that I look at it, this double-negation looks gross:
disable:
dis_ucode_ldr = true;
"disable the disable loader". Pfff.
>
> Something like the delta patch below makes it way more obvious and gets
> rid of the ugly gotos as well.
Almost. When we *enable* the loader, we must set dis_ucode_ldr to false. IOW,
we must write dis_ucode_ldr to the newly detected value because
load_ucode_ap() checks it because it can't call microcode_loader_disabled()
because of this:
/*
* 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.
*/
IOW, yours a bit modified. Still untested ofc.
---
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 7771755481ed..652198805ee3 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -42,7 +42,7 @@
#include "internal.h"
static struct microcode_ops *microcode_ops;
-static bool dis_ucode_ldr = true;
+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;
@@ -100,27 +103,28 @@ bool __init microcode_loader_disabled(void)
if (dis_ucode_ldr)
return true;
- if (!have_cpuid_p())
- goto disable;
-
/*
- * 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))
- goto disable;
-
- if (x86_cpuid_vendor() == X86_VENDOR_AMD) {
- if (amd_check_current_patch_level())
- goto disable;
- }
+ if (!have_cpuid_p() ||
+ native_cpuid_ecx(1) & BIT(31) ||
+ amd_check_current_patch_level())
+ dis_ucode_ldr = true;
+ else
+ dis_ucode_ldr = false;
return dis_ucode_ldr;
-
-disable:
- dis_ucode_ldr = true;
- return true;
}
void __init load_ucode_bsp(void)
@@ -129,7 +133,7 @@ void __init load_ucode_bsp(void)
bool intel = true;
if (cmdline_find_option_bool(boot_command_line, "dis_ucode_ldr") > 0)
- dis_ucode_ldr = false;
+ dis_ucode_ldr = true;
if (microcode_loader_disabled())
return;
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, May 02 2025 at 18:22, Borislav Petkov wrote:
> On Wed, Apr 30, 2025 at 09:16:56PM +0200, Thomas Gleixner wrote:
>> This return here is confusing at best. The only valid return value is
>> 'false' according to the above logic, because nothing modifies
>> dis_ucode_ldr and that must be false according to the top-most check,
>> no?
>
> You mean the return value is the build-time dis_ucode_ldr value which is true.
> Well, *was* true, keep on reading.
>
> I.e., the loader was default-disabled unless we decide it is ok to turn it on.
>
> Now that I look at it, this double-negation looks gross:
>
> disable:
> dis_ucode_ldr = true;
>
> "disable the disable loader". Pfff.
Indeed and it's all confusing because at the top of the function you
have:
if (dis_ucode_ldr)
return true;
That means, that dis_ucode_ldr must be false when it reaches
return dis_ucode_ldr;
in your original patch, no?
>> Something like the delta patch below makes it way more obvious and gets
>> rid of the ugly gotos as well.
>
> Almost. When we *enable* the loader, we must set dis_ucode_ldr to false. IOW,
> we must write dis_ucode_ldr to the newly detected value because
> load_ucode_ap() checks it because it can't call microcode_loader_disabled()
> because of this:
>
> /*
> * 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.
> */
>
>
> IOW, yours a bit modified. Still untested ofc.
>
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 7771755481ed..652198805ee3 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -42,7 +42,7 @@
> #include "internal.h"
>
> static struct microcode_ops *microcode_ops;
> -static bool dis_ucode_ldr = true;
> +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;
> @@ -100,27 +103,28 @@ bool __init microcode_loader_disabled(void)
> if (dis_ucode_ldr)
> return true;
>
> - if (!have_cpuid_p())
> - goto disable;
> -
> /*
> - * 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))
> - goto disable;
> -
> - if (x86_cpuid_vendor() == X86_VENDOR_AMD) {
> - if (amd_check_current_patch_level())
> - goto disable;
> - }
> + if (!have_cpuid_p() ||
> + native_cpuid_ecx(1) & BIT(31) ||
> + amd_check_current_patch_level())
> + dis_ucode_ldr = true;
> + else
> + dis_ucode_ldr = false;
This still does not make any sense, because if dis_ucode_ldr == true
when this function is called then the first check immediately returns.
So dis_ucode_ldr _IS_ false when this code is reached, no?
Thanks,
tglx
* Borislav Petkov <bp@alien8.de> wrote: > -static struct microcode_ops *microcode_ops; > -bool dis_ucode_ldr = true; > +static struct microcode_ops *microcode_ops; > +static bool dis_ucode_ldr = true; BTW., any objections against: s/dis_ucode_ldr /ucode_loader_disabled or so? I had to look twice to see through the obfuscated name. (Okay, it was 3 times, and I had to grep the code I confess. :) As a separate cleanup. Thanks, Ingo
On Mon, Apr 14, 2025 at 12:48:59PM +0200, Ingo Molnar wrote:
> BTW., any objections against:
>
> s/dis_ucode_ldr
> /ucode_loader_disabled
Well, "dis_ucode_ldr" is the user-visible cmdline option. (I admit, it wasn't
a good choice back then but we had exposed it to luserspace already so there
was no turning back.).
So if I rename the internal var, there'll be a discrepancy:
if (cmdline_find_option_bool(boot_command_line, "dis_ucode_ldr") > 0)
loader_disabled = false;
And yeah, it doesn't need a "ucode_" or "microcode_" prefix as it is internal
var now.
So yeah, I guess that could work because the discrepancy will be at one place
only, at the parsin location.
An additional thing we could do - and since I'm fan of namespaces - we can
start supporting a "microcode=" cmdline in parallel and have it do
microcode=disable
with the same functionality. And "dis_ucode_ldr" will be deprecated and it'll
warn when people use it and will tell them to use "microcode=disable" and we
will phase it out after a loooong grace period (think years).
So yeah, something like that...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, 14 Apr 2025 13:26:33 +0200 Borislav Petkov <bp@alien8.de> wrote: ... > An additional thing we could do - and since I'm fan of namespaces - we can > start supporting a "microcode=" cmdline in parallel and have it do > > microcode=disable You probably want 'loader' in the name as well. David
On Tue, 8 Apr 2025 19:22:50 +0200
Borislav Petkov <bp@alien8.de> wrote:
> Fixes: 4c585af7180c1 ("x86/boot/32: Temporarily map initrd for
> microcode loading")
Boots fine on my AM486DX2-66 and Cx486DX4-100 PCs.
Thank you!
© 2016 - 2025 Red Hat, Inc.