[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87zffuomd4.ffs@tglx>
Date: Fri, 02 May 2025 21:15:19 +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 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
Powered by blists - more mailing lists