[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87frhppihj.ffs@tglx>
Date: Wed, 30 Apr 2025 21:16:56 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Borislav Petkov <bp@...en8.de>
Cc: Kevin Koster <lkml@...ertech.com>, Oerg866 <oerg866@...glemail.com>,
linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>, Dave Hansen
<dave.hansen@...ux.intel.com>, x86@...nel.org, "H. Peter Anvin"
<hpa@...or.com>
Subject: Re: [PATCH -v2] x86/microcode: Consolidate the loader enablement
checking
On 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)
Powered by blists - more mailing lists