Re: [PATCH -v2] x86/microcode: Consolidate the loader enablement checking

[Thomas Gleixner <tglx@linutronix.de>]

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)