[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250502162210.GCaBTxMhdUT_Iw3_bj@fat_crate.local>
Date: Fri, 2 May 2025 18:22:10 +0200
From: Borislav Petkov <bp@...en8.de>
To: Thomas Gleixner <tglx@...utronix.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 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
Powered by blists - more mailing lists