[PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Oerg866]

Starting with v6.7-rc1, the kernel was no longer able to boot on early
i486-class CPUs.

To clarify, this was caused by the comprehensive microcode rework, from

0a23fb262d17f("Merge tag 'x86_microcode_for_v6.7_rc1'
of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip").

The breaking changes were introduced with these specific commits:

4c585af7180c1("x86/boot/32: Temporarily map initrd for microcode loading")

...causes immediate reboot.

a7939f0167203("x86/microcode/amd: Cache builtin/initrd microcode early")

...causes kernel panic early on.

They assume the host CPU supports the CPUID instruction, which
the pre-"enhanced" 486 type ones do not.

These changes make every kernel version from 6.7 to 6.11.4 bootable on
them.
---
 arch/x86/kernel/cpu/microcode/amd.c | 8 ++++++--
 arch/x86/kernel/head32.c            | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index c0d56c02b8da..71fa388573ac 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -516,14 +516,18 @@ 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);
+ unsigned int cpuid_1_eax;
  struct cpuinfo_x86 *c = &boot_cpu_data;
  struct cont_desc desc = { 0 };
  enum ucode_state ret;
  struct cpio_data cp;

- if (dis_ucode_ldr || c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10)
+ if (!have_cpuid_p() || dis_ucode_ldr ||
+        c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10) {
         return 0;
+ }
+
+ cpuid_1_eax = native_cpuid_eax(1);

  find_blobs_in_containers(cpuid_1_eax, &cp);
  if (!(cp.data && cp.size))
diff --git a/arch/x86/kernel/head32.c b/arch/x86/kernel/head32.c
index de001b2146ab..79fd9f11dbcb 100644
--- a/arch/x86/kernel/head32.c
+++ b/arch/x86/kernel/head32.c
@@ -146,7 +146,7 @@ void __init __no_stack_protector mk_early_pgtbl_32(void)

 #ifdef CONFIG_MICROCODE_INITRD32
  /* Running on a hypervisor? */
- if (native_cpuid_ecx(1) & BIT(31))
+ if (have_cpuid_p() && (native_cpuid_ecx(1) & BIT(31)))
         return;

  params = (struct boot_params *)__pa_nodebug(&boot_params);
--
2.34.1

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Borislav Petkov]

On Sat, Oct 19, 2024 at 08:29:04AM +0200, Oerg866 wrote:
> They assume the host CPU supports the CPUID instruction, which
> the pre-"enhanced" 486 type ones do not.

"host"?

How exactly are you triggering this?

Are you running some weird guest or is it real hardware?

Any chance you can share details so that I can try to reproduce here in a VM?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Oerg866]

Hello,

apologies, I had forgotten to switch to plain text in my previous reply.

This is my first kernel patch submission, please bear with me :-)

> Are you running some weird guest or is it real hardware?

Host in this case means the CPU the kernel is running on, so real hardware.

As far as I'm aware, common emulators used for kernel testing, such as
QEMU, do not exhibit this problem.

There are however emulators that can somewhat precisely emulate the
affected CPUs, such as 86Box, that can reproduce this behaviour.

> Any chance you can share details so that I can try to reproduce here in a VM?

I have prepared a small archive to help with near effortless reproduction:

wget https://kext.de/dl/486patchtest.tar.gz
tar -zxvf 486patchtest.tar.gz

- Use the included .config file to compile a minimal kernel for 486DX,
  in this example the kernel has been cloned to ./linux (I used tag v6.11):

cp .config linux/.config
pushd linux
make -j16
popd

- Then, proceed with emulation:

wget https://github.com/86Box/86Box/releases/download/v4.2.1/86Box-Linux-x86_64-b6130.AppImage
chmod +x ./86Box-Linux-x86_64-b6130.AppImage
git clone https://github.com/86Box/roms
./86Box-Linux-x86_64-b6130.AppImage --config 86box.cfg

- Click the little CD-ROM Icon on the bottom left
- select "Folder"
- select linux/arch/x86/boot

The machine will then boot into FreeDOS, load the kernel via LOADLIN
and you should see a kernel panic.

The patch is included in the archive (486.patch) for convenience.
After applying it, the kernel will boot all the way to init (which
expectedly fails, as it is missing in this case).

I hope this information is of use to you!

Best regards
Eric Voirin

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Kevin Koster]

On Sat, 19 Oct 2024 08:29:04 +0200
Oerg866 <oerg866@googlemail.com> wrote:

> Starting with v6.7-rc1, the kernel was no longer able to boot on early
> i486-class CPUs.

Thanks for this patch! It solves my problem with kernel 6.12.11
rebooting at start-up on 486 CPUs, which had me puzzled. (tested on
AM486DX2-66 and CX486DX4-100)

Is there a reason why the patch wasn't accepted? I've proposed using it
for the Tiny Core Linux distro's x86 kernel builds, which target 486DX
and later CPUs.

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Borislav Petkov]

On Sat, Apr 05, 2025 at 01:03:06PM +1100, Kevin Koster wrote:
> On Sat, 19 Oct 2024 08:29:04 +0200
> Oerg866 <oerg866@googlemail.com> wrote:
> 
> > Starting with v6.7-rc1, the kernel was no longer able to boot on early
> > i486-class CPUs.
> 
> Thanks for this patch! It solves my problem with kernel 6.12.11
> rebooting at start-up on 486 CPUs, which had me puzzled. (tested on
> AM486DX2-66 and CX486DX4-100)
> 
> Is there a reason why the patch wasn't accepted?

Yes, too many patches, too little time. :-(

Anyway, does the one below - only build-tested - work for both y'all too?

---
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..dda7f0d409e9 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -1099,7 +1099,7 @@ static int __init save_microcode_in_initrd(void)
 	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;
 
 	if (!find_blobs_in_containers(&cp))
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index b3658d11e7b6..972338a2abae 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -95,12 +95,15 @@ 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 (!have_cpuid_p())
+		return true;
+
 	/*
 	 * 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
@@ -146,7 +149,7 @@ void __init load_ucode_bsp(void)
 		return;
 	}
 
-	if (check_loader_disabled_bsp())
+	if (microcode_loader_disabled())
 		return;
 
 	if (intel)
diff --git a/arch/x86/kernel/head32.c b/arch/x86/kernel/head32.c
index de001b2146ab..f29dc9c95c50 100644
--- a/arch/x86/kernel/head32.c
+++ b/arch/x86/kernel/head32.c
@@ -145,8 +145,7 @@ 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))
+	if (microcode_loader_disabled())
 		return;
 
 	params = (struct boot_params *)__pa_nodebug(&boot_params);

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[H. Peter Anvin]

On April 5, 2025 2:32:26 AM PDT, Borislav Petkov <bp@alien8.de> wrote:
>On Sat, Apr 05, 2025 at 01:03:06PM +1100, Kevin Koster wrote:
>> On Sat, 19 Oct 2024 08:29:04 +0200
>> Oerg866 <oerg866@googlemail.com> wrote:
>> 
>> > Starting with v6.7-rc1, the kernel was no longer able to boot on early
>> > i486-class CPUs.
>> 
>> Thanks for this patch! It solves my problem with kernel 6.12.11
>> rebooting at start-up on 486 CPUs, which had me puzzled. (tested on
>> AM486DX2-66 and CX486DX4-100)
>> 
>> Is there a reason why the patch wasn't accepted?
>
>Yes, too many patches, too little time. :-(
>
>Anyway, does the one below - only build-tested - work for both y'all too?
>
>---
>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..dda7f0d409e9 100644
>--- a/arch/x86/kernel/cpu/microcode/amd.c
>+++ b/arch/x86/kernel/cpu/microcode/amd.c
>@@ -1099,7 +1099,7 @@ static int __init save_microcode_in_initrd(void)
> 	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;
> 
> 	if (!find_blobs_in_containers(&cp))
>diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
>index b3658d11e7b6..972338a2abae 100644
>--- a/arch/x86/kernel/cpu/microcode/core.c
>+++ b/arch/x86/kernel/cpu/microcode/core.c
>@@ -95,12 +95,15 @@ 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 (!have_cpuid_p())
>+		return true;
>+
> 	/*
> 	 * 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
>@@ -146,7 +149,7 @@ void __init load_ucode_bsp(void)
> 		return;
> 	}
> 
>-	if (check_loader_disabled_bsp())
>+	if (microcode_loader_disabled())
> 		return;
> 
> 	if (intel)
>diff --git a/arch/x86/kernel/head32.c b/arch/x86/kernel/head32.c
>index de001b2146ab..f29dc9c95c50 100644
>--- a/arch/x86/kernel/head32.c
>+++ b/arch/x86/kernel/head32.c
>@@ -145,8 +145,7 @@ 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))
>+	if (microcode_loader_disabled())
> 		return;
> 
> 	params = (struct boot_params *)__pa_nodebug(&boot_params);
>

How the Hades does c->x86 not get set to 4 (hence < 0x10) on this CPU?

That's the real bug imo...

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Borislav Petkov]

On Sat, Apr 05, 2025 at 01:33:46PM -0700, H. Peter Anvin wrote:
> How the Hades does c->x86 not get set to 4 (hence < 0x10) on this CPU?
> 
> That's the real bug imo...

Go to the first mail in the thread.

4c585af7180c ("x86/boot/32: Temporarily map initrd for microcode loading")

walks straight into  native_cpuid_ecx().

Which reminds me - I should remove that have_cpuid_p() in load_ucode_bsp() now
too and move the disabled check up.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Borislav Petkov]

On Sat, Apr 05, 2025 at 10:50:13PM +0200, Borislav Petkov wrote:
> On Sat, Apr 05, 2025 at 01:33:46PM -0700, H. Peter Anvin wrote:
> > How the Hades does c->x86 not get set to 4 (hence < 0x10) on this CPU?
> > 
> > That's the real bug imo...
> 
> Go to the first mail in the thread.
> 
> 4c585af7180c ("x86/boot/32: Temporarily map initrd for microcode loading")
> 
> walks straight into  native_cpuid_ecx().

... and what you're *really* asking is why that dis_ucode_ldr is there in
save_microcode_in_initrd() - because tglx made it an early_initcall() so it
runs independently of the loader detection logic and thus needs this check.

So I figure I'll stick all the checking logic into a single function which
everything can call now...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Kevin Koster]

On Sat, 5 Apr 2025 11:32:26 +0200
Borislav Petkov <bp@alien8.de> wrote:
> On Sat, Apr 05, 2025 at 01:03:06PM +1100, Kevin Koster wrote:
> > On Sat, 19 Oct 2024 08:29:04 +0200
> > Oerg866 <oerg866@googlemail.com> wrote:
> > 
> > > Starting with v6.7-rc1, the kernel was no longer able to boot on
> > > early i486-class CPUs.
> > 
> > Thanks for this patch! It solves my problem with kernel 6.12.11
> > rebooting at start-up on 486 CPUs, which had me puzzled. (tested on
> > AM486DX2-66 and CX486DX4-100)
> > 
> > Is there a reason why the patch wasn't accepted?
> 
> Yes, too many patches, too little time. :-(
> 
> Anyway, does the one below - only build-tested - work for both y'all
> too?

On my AM486DX2-66 it gets past the immediate reboot problem but gets
an Oops and kernel panic here:

[snip]
smpboot: SMP disabled
Performance Events: no PMU driver, software events only.
signal: max sigframe size: 928
Oops: invalid opcode: 0000 [#1] SMP
CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0 #1
EIP: 0xc0b5e1d7
Code: 7d dc 00 74 0b b9 07 00 00 00 89 df b0 01 f3 a5 83 c4 1c 5b 5e 5f 5d c3 55 b8 01 00 00 00 89 e5 57 56 31 f6 53 89 f1 83 ec 30 <0f> a2 b9 04 00 00 00 89 45 c4 8d 7d c8 89 f0 f3 ab e8 a0 fc ff ff
EAX: 00000001 EBX: c0be6b00 ECX: 00000000 EDX: 00000246
ESI: 00000000 EDI: 00000000 EBP: c1309f08 ESP: c1309ecc
DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
CR0: 80050033 CR2: ffd38000 CR3: 00c16000 CR4: 00000000
Call Trace:
 ? 0xc01231af
 ? 0xc01231c8
 ? 0xc0123222
 ? 0xc0123245
 ? 0xc01215ef
 ? 0xc01217f7
 ? 0xc0b5e1d7
 ? 0xc0848e09
 ? 0xc012186f
 ? 0xc0b5e1d7
 ? 0xc0848e41
 ? 0xc010105d
 ? 0xc01500d8
 ? 0xc0848e09
 ? 0xc0b5e1d7
 ? 0xc01500d8
 ? 0xc0848e09
 ? 0xc0b5e1d7
 ? 0xc085148a
 ? 0xc0524da8
 ? 0xc0b5e1c5
 0xc0102100
 ? 0xc08514d5
 ? 0xc085148a
 ? 0xc0171bde
 ? 0xc0175987
 ? 0xc016d672
 0xc0b4c9e2
 ? 0xc084c9b8
 0xc084c9ca
 0xc0127d3a
 ? 0xc084c9b8
 0xc01027a2
 0xc0100e3d
Modules linked in:
---[ end trace 0000000000000000 ]---
EIP: 0xc0b5e1d7
Code: 7d dc 00 74 0b b9 07 00 00 00 89 df b0 01 f3 a5 83 c4 1c 5b 5e 5f 5d c3 55 b8 01 00 00 00 89 e5 57 56 31 f6 53 89 f1 83 ec 30 <0f> a2 b9 04 00 00 00 89 45 c4 8d 7d c8 89 f0 f3 ab e8 a0 fc ff ff
EAX: 00000001 EBX: c0be6b00 ECX: 00000000 EDX: 00000246
ESI: 00000000 EDI: 00000000 EBP: c1309f08 ESP: c1309ecc
DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
CR0: 80050033 CR2: ffd38000 CR3: 00c16000 CR4: 00000000
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
Rebooting in 60 seconds..

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Kevin Koster]

On Sun, 6 Apr 2025 16:40:49 +1000
Kevin Koster <lkml@ombertech.com> wrote:
> On Sat, 5 Apr 2025 11:32:26 +0200
> Borislav Petkov <bp@alien8.de> wrote:
> > On Sat, Apr 05, 2025 at 01:03:06PM +1100, Kevin Koster wrote:
> > > On Sat, 19 Oct 2024 08:29:04 +0200
> > > Oerg866 <oerg866@googlemail.com> wrote:
> > > > Starting with v6.7-rc1, the kernel was no longer able to boot on
> > > > early i486-class CPUs.
> > > 
> > > Thanks for this patch! It solves my problem with kernel 6.12.11
> > > rebooting at start-up on 486 CPUs, which had me puzzled. (tested
> > > on AM486DX2-66 and CX486DX4-100)
> > > 
> > > Is there a reason why the patch wasn't accepted?
> > 
> > Yes, too many patches, too little time. :-(
> > 
> > Anyway, does the one below - only build-tested - work for both y'all
> > too?
> 
> On my AM486DX2-66 it gets past the immediate reboot problem but gets
> an Oops and kernel panic here:

But if I move "cpuid_1_eax = native_cpuid_eax(1);" to after the
microcode_loader_disabled() check in amd.c, like in the first patch, it
boots fine!

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Borislav Petkov]

On Sun, Apr 06, 2025 at 05:46:33PM +1000, Kevin Koster wrote:
> But if I move "cpuid_1_eax = native_cpuid_eax(1);" to after the
> microcode_loader_disabled() check in amd.c, like in the first patch, it
> boots fine!

Yeah, I noticed that too last night. Here's a hopefully better version after
I ran it on my 32-bit Atom - I don't have your old rust and maybe you should
simply throw it in the garbage - that thing is probably not worth the
electricity it uses to power up... :-)

Anyway, I wanna do this below.

The only hunk I'm not sure about is in mk_early_pgtbl_32() because I don't
want to uglify the loader checking code with __pa* macros just so that initrd
mapping is made conditional on whether the loader is enabled.

After all, zap_early_initrd_mapping() clears it right after there.

But let's see what tglx says as this is his code:

4c585af7180c ("x86/boot/32: Temporarily map initrd for microcode loading")

Thx.

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..dda7f0d409e9 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -1099,7 +1099,7 @@ static int __init save_microcode_in_initrd(void)
 	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;
 
 	if (!find_blobs_in_containers(&cp))
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;


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Kevin Koster]

On Sun, 6 Apr 2025 21:02:53 +0200
Borislav Petkov <bp@alien8.de> wrote:

> On Sun, Apr 06, 2025 at 05:46:33PM +1000, Kevin Koster wrote:
> > But if I move "cpuid_1_eax = native_cpuid_eax(1);" to after the
> > microcode_loader_disabled() check in amd.c, like in the first
> > patch, it boots fine!
> 
> Yeah, I noticed that too last night. Here's a hopefully better
> version after I ran it on my 32-bit Atom - I don't have your old rust

There is the 86box emulator which Eric Voirin suggested before for
reproducing this issue, although that adds the uncertainty of whether
the emulator matches real hardware behaviour.

> and maybe you should simply throw it in the garbage - that thing is
> probably not worth the electricity it uses to power up... :-)

Well my testing modern Linux on 486s was originally prompted by people
on the Tiny Core Linux forums finding compatibility issues with old
PCs. But I like to know Linux really works on the hardware it's built
for, and I'm not much better, writing this now on a Pentium 1.

[snip]
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c
> b/arch/x86/kernel/cpu/microcode/amd.c index
> b61028cf5c8a..dda7f0d409e9 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -1099,7 +1099,7 @@ static int __init save_microcode_in_initrd(void)
>  	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;

Still fails unless the native_cpuid_eax(1) call is moved under here. After that
change, it boots fine.

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Borislav Petkov]

On Mon, Apr 07, 2025 at 09:58:48AM +1000, Kevin Koster wrote:
> But I like to know Linux really works on the hardware it's built for,

I don't know what that means.

> and I'm not much better, writing this now on a Pentium 1.

Lemme guess: this is your main machine you use for daily work?

:-\

> > -	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;
> 
> Still fails unless the native_cpuid_eax(1) call is moved under here. After that
> change, it boots fine.

Please show me with a diff what you're doing because I don't know what you
mean.

I did this:

bool have_cpuid_p(void)
{
        return false;
}

in order to simulate no CPUID support but my 32-bit guest boots fine.

Also, send a full dmesg from that machine so that I can try to reproduce here.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Kevin Koster]

On Mon, 7 Apr 2025 12:29:27 +0200
Borislav Petkov <bp@alien8.de> wrote:

> On Mon, Apr 07, 2025 at 09:58:48AM +1000, Kevin Koster wrote:
> > But I like to know Linux really works on the hardware it's built
> > for,
> 
> I don't know what that means.

To rephrase: I like knowing that "CONFIG_M486=y" works, in the kernel
configuration. If not, then I know to use other OSs if I want to boot a
486.

> > and I'm not much better, writing this now on a Pentium 1.
> 
> Lemme guess: this is your main machine you use for daily work?

For email/news every morning, then a (newer) laptop afterwards.

> > > -	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;
> > 
> > Still fails unless the native_cpuid_eax(1) call is moved under
> > here. After that change, it boots fine.
> 
> Please show me with a diff what you're doing because I don't know
> what you mean.

OK, this is the change I made after applying your patch:

--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -1093,7 +1093,7 @@
 
 static int __init save_microcode_in_initrd(void)
 {
-	unsigned int cpuid_1_eax = native_cpuid_eax(1);
+	unsigned int cpuid_1_eax;
	struct cpuinfo_x86 *c = &boot_cpu_data;
	struct cont_desc desc = { 0 };
	enum ucode_state ret;
@@ -1102,6 +1102,8 @@
	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;

> I did this:
> 
> bool have_cpuid_p(void)
> {
>         return false;
> }
> 
> in order to simulate no CPUID support but my 32-bit guest boots fine.

It's detecting no CPUID support on 486 CPUs OK, however
save_microcode_in_initrd() uses CPUID before checking if it is
supported.

> Also, send a full dmesg from that machine so that I can try to
> reproduce here.

This is with your latest patch applied, without my above change:

No EFI environment detected.
early console in extract_kernel
input_data: 0x007d1094
input_len: 0x005aa603
output: 0x00100000
output_len: 0x00bb42d8
kernel_total_size: 0x00c9a000
needed_size: 0x00c9a000

Decompressing Linux... Parsing ELF... No relocation needed... done.
Booting the kernel (entry_offset: 0x00751e00).
Linux version 6.14.0 (cnk2@ombertech) (gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40) #3 SMP Sun Apr  6 21:54:11 UTC 2025
BIOS-provided physical RAM map:
BIOS-e820: [mem 0x0000000000000000-0x0000000000004c1f] usable
BIOS-e820: [mem 0x0000000000004c20-0x000000000000501f] reserved
BIOS-e820: [mem 0x0000000000005020-0x000000000009ffff] usable
BIOS-e820: [mem 0x00000000000f5a3c-0x00000000000fffff] reserved
BIOS-e820: [mem 0x0000000000100000-0x00000000013fffff] usable
BIOS-e820: [mem 0x00000000ffff5a3c-0x00000000ffffffff] reserved
printk: legacy console [earlyser0] enabled
printk: debug: ignoring loglevel setting.
Notice: NX (Execute Disable) protection missing in CPU!
APIC: Static calls initialized
DMI not present or invalid.
e820: update [mem 0x00000000-0x00000fff] usable ==> reserved
e820: remove [mem 0x000a0000-0x000fffff] usable
last_pfn = 0x1400 max_arch_pfn = 0x100000
MTRRs disabled (not available)
x86/PAT: PAT not supported by the CPU.
x86/PAT: Configuration [0-7]: WB  WT  UC- UC  WB  WT  UC- UC  
initial memory mapped: [mem 0x00000000-0x00ffffff]
0MB HIGHMEM available.
20MB LOWMEM available.
  mapped low ram: 0 - 01400000
  low ram: 0 - 01400000
Zone ranges:
  DMA      [mem 0x0000000000001000-0x0000000000ffffff]
  Normal   [mem 0x0000000001000000-0x00000000013fffff]
  HighMem  empty
Movable zone start for each node
Early memory node ranges
  node   0: [mem 0x0000000000001000-0x0000000000003fff]
  node   0: [mem 0x0000000000006000-0x000000000009ffff]
  node   0: [mem 0x0000000000100000-0x00000000013fffff]
Initmem setup node 0 [mem 0x0000000000001000-0x00000000013fffff]
On node 0, zone DMA: 1 pages in unavailable ranges
On node 0, zone DMA: 2 pages in unavailable ranges
On node 0, zone DMA: 96 pages in unavailable ranges
No local APIC present or hardware disabled
APIC: disable apic facility
APIC: Switched APIC routing to: noop
CPU topo: Max. logical packages:   1
CPU topo: Max. logical dies:       1
CPU topo: Max. dies per package:   1
CPU topo: Max. threads per core:   1
CPU topo: Num. cores per package:     1
CPU topo: Num. threads per package:   1
CPU topo: Allowing 1 present CPUs plus 0 hotplug CPUs
PM: hibernation: Registered nosave memory: [mem 0x00000000-0x00000fff]
PM: hibernation: Registered nosave memory: [mem 0x00004000-0x00004fff]
PM: hibernation: Registered nosave memory: [mem 0x00005000-0x00005fff]
PM: hibernation: Registered nosave memory: [mem 0x000a0000-0x000f5fff]
PM: hibernation: Registered nosave memory: [mem 0x000f6000-0x000fffff]
[mem 0x01400000-0xffff5a3b] available for PCI devices
Booting paravirtualized kernel on bare hardware
clocksource: refined-jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 6370452778343963 ns
setup_percpu: NR_CPUS:8 nr_cpumask_bits:1 nr_cpu_ids:1 nr_node_ids:1
percpu: Embedded 30 pages/cpu s91852 r0 d31028 u122880
pcpu-alloc: s91852 r0 d31028 u122880 alloc=30*4096
pcpu-alloc: [0] 0 
Kernel command line: root=/dev/sda3 rw base udev.children-max=1 acpi=off earlyprintk=ttyS0,115200,keep ignore_loglevel BOOT_IMAGE=614-4864
Unknown kernel command line parameters "base BOOT_IMAGE=614-4864", will be passed to user space.
printk: log buffer data + meta data: 131072 + 409600 = 540672 bytes
Dentry cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
Inode-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
Built 1 zonelists, mobility grouping on.  Total pages: 5021
mem auto-init: stack:off, heap alloc:off, heap free:off
Initializing HighMem for node 0 (00000000:00000000)
Checking if this processor honours the WP bit even in supervisor mode...Ok.
SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
rcu: Hierarchical RCU implementation.
rcu: 	RCU restricting CPUs from NR_CPUS=8 to nr_cpu_ids=1.
	Tracing variant of Tasks RCU enabled.
rcu: RCU calculated value of scheduler-enlistment delay is 30 jiffies.
rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=1
RCU Tasks Trace: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1 rcu_task_cpu_ids=1.
NR_IRQS: 2304, nr_irqs: 32, preallocated irqs: 16
rcu: srcu_init: Setting srcu_struct sizes based on contention.
Console: colour VGA+ 80x60
printk: legacy console [tty0] enabled
APIC: Keep in PIC mode(8259)
Calibrating delay loop... 30.96 BogoMIPS (lpj=51136)
Last level iTLB entries: 4KB 0, 2MB 0, 4MB 0
Last level dTLB entries: 4KB 0, 2MB 0, 4MB 0, 1GB 0
x86/fpu: Probing for FPU: FSW=0x0000 FCW=0x037f
x86/fpu: x87 FPU will use FSAVE
Freeing SMP alternatives memory: 24K
pid_max: default: 32768 minimum: 301
Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
smpboot: SMP disabled
Performance Events: no PMU driver, software events only.
signal: max sigframe size: 928
Oops: invalid opcode: 0000 [#1] SMP
CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0 #3
EIP: 0xc0b5e1da
Code: 7d dc 00 74 0b b9 07 00 00 00 89 df b0 01 f3 a5 83 c4 1c 5b 5e 5f 5d c3 55 b8 01 00 00 00 89 e5 57 56 31 f6 53 89 f1 83 ec 30 <0f> a2 b9 04 00 00 00 89 45 c4 8d 7d c8 89 f0 f3 ab e8 3c fb ff ff
EAX: 00000001 EBX: c0be6b00 ECX: 00000000 EDX: 00000246
ESI: 00000000 EDI: 00000000 EBP: c1309f08 ESP: c1309ecc
DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
CR0: 80050033 CR2: ffd38000 CR3: 00c16000 CR4: 00000000
Call Trace:
 ? 0xc01231af
 ? 0xc01231c8
 ? 0xc0123222
 ? 0xc0123245
 ? 0xc01215ef
 ? 0xc01217f7
 ? 0xc0b5e1da
 ? 0xc0848e09
 ? 0xc012186f
 ? 0xc0b5e1da
 ? 0xc0848e41
 ? 0xc010105d
 ? 0xc01500d8
 ? 0xc0848e09
 ? 0xc0b5e1da
 ? 0xc01500d8
 ? 0xc0848e09
 ? 0xc0b5e1da
 ? 0xc085148a
 ? 0xc0524da8
 ? 0xc0b5e1c8
 0xc0102100
 ? 0xc08514d5
 ? 0xc085148a
 ? 0xc0171bde
 ? 0xc0175987
 ? 0xc016d672
 0xc0b4c9e2
 ? 0xc084c9b8
 0xc084c9ca
 0xc0127d3a
 ? 0xc084c9b8
 0xc01027a2
 0xc0100e3d
Modules linked in:
---[ end trace 0000000000000000 ]---
EIP: 0xc0b5e1da
Code: 7d dc 00 74 0b b9 07 00 00 00 89 df b0 01 f3 a5 83 c4 1c 5b 5e 5f 5d c3 55 b8 01 00 00 00 89 e5 57 56 31 f6 53 89 f1 83 ec 30 <0f> a2 b9 04 00 00 00 89 45 c4 8d 7d c8 89 f0 f3 ab e8 3c fb ff ff
EAX: 00000001 EBX: c0be6b00 ECX: 00000000 EDX: 00000246
ESI: 00000000 EDI: 00000000 EBP: c1309f08 ESP: c1309ecc
DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
CR0: 80050033 CR2: ffd38000 CR3: 00c16000 CR4: 00000000
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
Rebooting in 60 seconds..

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Borislav Petkov]

On Tue, Apr 08, 2025 at 12:21:50AM +1000, Kevin Koster wrote:
> To rephrase: I like knowing that "CONFIG_M486=y" works, in the kernel
> configuration. If not, then I know to use other OSs if I want to boot a
> 486.

Lemme put it this way: off and on the topic about removing 32-bit support
altogether comes up but we never go through with it. One day we might though.

:-)

> For email/news every morning, then a (newer) laptop afterwards.

Oh boy.

> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -1093,7 +1093,7 @@
>  
>  static int __init save_microcode_in_initrd(void)
>  {
> -	unsigned int cpuid_1_eax = native_cpuid_eax(1);
> +	unsigned int cpuid_1_eax;
> 	struct cpuinfo_x86 *c = &boot_cpu_data;
> 	struct cont_desc desc = { 0 };
> 	enum ucode_state ret;
> @@ -1102,6 +1102,8 @@
> 	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;

Yah, thanks. I must be going blind. :-(

It is all clear now - I'll run the fix on my machines some more and then post
a proper patch.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Oerg866]

Thank you for your discussions,
I'm happy that a fix for this is happening after all :)

FYI, the link above containing files to easily reproduce in an emulator
*should* still work, so if anyone else wants to see this virtual "rust",
for some reason, go ahead - Even includes the genuine BIOS startup, haha.

I must say that it's quite extraordinary how well the kernel runs on a
486. Wish the same could be said for userspace...

I know my voice really doesn't matter, but I would love for 486
support to remain intact. It's an invaluable help for people who tinker
with old stuff but have grown accustomed to modern tools and habits...

One of my projects uses a modern Linux kernel to install Windows 9x (sorry!)
onto a legacy (or modern, if that's your thing) systems, including
magical things like VESA Local Bus 486 systems, way faster than the
official MS installer ever could. There's quirks here and there, especially
with regards to libata and obscure IDE/SCSI controllers, but other than
that the kernel does its job and it does it extremely well.

I've been experimenting with kernel versions and I'm happy to report that
since 4.x there hasn't been any sort of noticeable performance regression
on old systems like that.

Also some newly written BIOS / DOS driver code could not have happened
if Linux wasn't such an excellent reference. Well written drivers for
the old stuff can practically compensate for the lack of datasheets for
example.

I think that's something worth applauding, even if some may think it's
better to throw the old stuff "in the garbage" ;)

Sorry I'm rambling a bit :) Anyway - Thanks for your efforts everyone.

See you next patch!

Eric Voirin

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Maciej W. Rozycki]

On Mon, 7 Apr 2025, Oerg866 wrote:

> One of my projects uses a modern Linux kernel to install Windows 9x (sorry!)
> onto a legacy (or modern, if that's your thing) systems, including
> magical things like VESA Local Bus 486 systems, way faster than the
> official MS installer ever could. There's quirks here and there, especially
> with regards to libata and obscure IDE/SCSI controllers, but other than
> that the kernel does its job and it does it extremely well.

 FWIW I run a plain EISA 486 box to verify our defxx driver keeps working 
with an EISA FDDI network interface, one of the host bus attachments the 
driver supports.  I also run a dual Pentium MMX box (i430HX chipset), 
which I recently used for i386 GNU toolchain/C library verification in the 
course of upstreaming some stuff for my day job (it's the very box I used 
for APIC fiddling some 25 years ago, which Ingo may remember).

 Both machines are permanently wired for continuous use and remote control 
in my remote lab and run fairly recent versions of the kernel (as my time 
permits upgrading).

 I note that the lack of support for SW DMA mode in libata has degraded 
performance of a PATA drive (the rest is SCSI) in the MMX box compared to 
old IDE code, forcing the drive to use PIO 1.  Alas, I've found no time so 
far to look into porting that stuff over and said drive is not essential 
for that system.

  Maciej

[PATCH] x86/microcode: Consolidate the loader enablement

[Borislav Petkov]

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-netiquette

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

[Kevin Koster]

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!

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

[Thomas Gleixner]

On 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

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

[Borislav Petkov]

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

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

[Borislav Petkov]

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-netiquette

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

[Ingo Molnar]

* 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

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

[Borislav Petkov]

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

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

[David Laight]

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

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

[Thomas Gleixner]

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)

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

[Borislav Petkov]

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

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

[Thomas Gleixner]

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

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[H. Peter Anvin]

On April 7, 2025 7:21:50 AM PDT, Kevin Koster <lkml@ombertech.com> wrote:
>On Mon, 7 Apr 2025 12:29:27 +0200
>Borislav Petkov <bp@alien8.de> wrote:
>
>> On Mon, Apr 07, 2025 at 09:58:48AM +1000, Kevin Koster wrote:
>> > But I like to know Linux really works on the hardware it's built
>> > for,
>> 
>> I don't know what that means.
>
>To rephrase: I like knowing that "CONFIG_M486=y" works, in the kernel
>configuration. If not, then I know to use other OSs if I want to boot a
>486.
>
>> > and I'm not much better, writing this now on a Pentium 1.
>> 
>> Lemme guess: this is your main machine you use for daily work?
>
>For email/news every morning, then a (newer) laptop afterwards.
>
>> > > -	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;
>> > 
>> > Still fails unless the native_cpuid_eax(1) call is moved under
>> > here. After that change, it boots fine.
>> 
>> Please show me with a diff what you're doing because I don't know
>> what you mean.
>
>OK, this is the change I made after applying your patch:
>
>--- a/arch/x86/kernel/cpu/microcode/amd.c
>+++ b/arch/x86/kernel/cpu/microcode/amd.c
>@@ -1093,7 +1093,7 @@
> 
> static int __init save_microcode_in_initrd(void)
> {
>-	unsigned int cpuid_1_eax = native_cpuid_eax(1);
>+	unsigned int cpuid_1_eax;
>	struct cpuinfo_x86 *c = &boot_cpu_data;
>	struct cont_desc desc = { 0 };
>	enum ucode_state ret;
>@@ -1102,6 +1102,8 @@
>	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;
>
>> I did this:
>> 
>> bool have_cpuid_p(void)
>> {
>>         return false;
>> }
>> 
>> in order to simulate no CPUID support but my 32-bit guest boots fine.
>
>It's detecting no CPUID support on 486 CPUs OK, however
>save_microcode_in_initrd() uses CPUID before checking if it is
>supported.
>
>> Also, send a full dmesg from that machine so that I can try to
>> reproduce here.
>
>This is with your latest patch applied, without my above change:
>
>No EFI environment detected.
>early console in extract_kernel
>input_data: 0x007d1094
>input_len: 0x005aa603
>output: 0x00100000
>output_len: 0x00bb42d8
>kernel_total_size: 0x00c9a000
>needed_size: 0x00c9a000
>
>Decompressing Linux... Parsing ELF... No relocation needed... done.
>Booting the kernel (entry_offset: 0x00751e00).
>Linux version 6.14.0 (cnk2@ombertech) (gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40) #3 SMP Sun Apr  6 21:54:11 UTC 2025
>BIOS-provided physical RAM map:
>BIOS-e820: [mem 0x0000000000000000-0x0000000000004c1f] usable
>BIOS-e820: [mem 0x0000000000004c20-0x000000000000501f] reserved
>BIOS-e820: [mem 0x0000000000005020-0x000000000009ffff] usable
>BIOS-e820: [mem 0x00000000000f5a3c-0x00000000000fffff] reserved
>BIOS-e820: [mem 0x0000000000100000-0x00000000013fffff] usable
>BIOS-e820: [mem 0x00000000ffff5a3c-0x00000000ffffffff] reserved
>printk: legacy console [earlyser0] enabled
>printk: debug: ignoring loglevel setting.
>Notice: NX (Execute Disable) protection missing in CPU!
>APIC: Static calls initialized
>DMI not present or invalid.
>e820: update [mem 0x00000000-0x00000fff] usable ==> reserved
>e820: remove [mem 0x000a0000-0x000fffff] usable
>last_pfn = 0x1400 max_arch_pfn = 0x100000
>MTRRs disabled (not available)
>x86/PAT: PAT not supported by the CPU.
>x86/PAT: Configuration [0-7]: WB  WT  UC- UC  WB  WT  UC- UC  
>initial memory mapped: [mem 0x00000000-0x00ffffff]
>0MB HIGHMEM available.
>20MB LOWMEM available.
>  mapped low ram: 0 - 01400000
>  low ram: 0 - 01400000
>Zone ranges:
>  DMA      [mem 0x0000000000001000-0x0000000000ffffff]
>  Normal   [mem 0x0000000001000000-0x00000000013fffff]
>  HighMem  empty
>Movable zone start for each node
>Early memory node ranges
>  node   0: [mem 0x0000000000001000-0x0000000000003fff]
>  node   0: [mem 0x0000000000006000-0x000000000009ffff]
>  node   0: [mem 0x0000000000100000-0x00000000013fffff]
>Initmem setup node 0 [mem 0x0000000000001000-0x00000000013fffff]
>On node 0, zone DMA: 1 pages in unavailable ranges
>On node 0, zone DMA: 2 pages in unavailable ranges
>On node 0, zone DMA: 96 pages in unavailable ranges
>No local APIC present or hardware disabled
>APIC: disable apic facility
>APIC: Switched APIC routing to: noop
>CPU topo: Max. logical packages:   1
>CPU topo: Max. logical dies:       1
>CPU topo: Max. dies per package:   1
>CPU topo: Max. threads per core:   1
>CPU topo: Num. cores per package:     1
>CPU topo: Num. threads per package:   1
>CPU topo: Allowing 1 present CPUs plus 0 hotplug CPUs
>PM: hibernation: Registered nosave memory: [mem 0x00000000-0x00000fff]
>PM: hibernation: Registered nosave memory: [mem 0x00004000-0x00004fff]
>PM: hibernation: Registered nosave memory: [mem 0x00005000-0x00005fff]
>PM: hibernation: Registered nosave memory: [mem 0x000a0000-0x000f5fff]
>PM: hibernation: Registered nosave memory: [mem 0x000f6000-0x000fffff]
>[mem 0x01400000-0xffff5a3b] available for PCI devices
>Booting paravirtualized kernel on bare hardware
>clocksource: refined-jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 6370452778343963 ns
>setup_percpu: NR_CPUS:8 nr_cpumask_bits:1 nr_cpu_ids:1 nr_node_ids:1
>percpu: Embedded 30 pages/cpu s91852 r0 d31028 u122880
>pcpu-alloc: s91852 r0 d31028 u122880 alloc=30*4096
>pcpu-alloc: [0] 0 
>Kernel command line: root=/dev/sda3 rw base udev.children-max=1 acpi=off earlyprintk=ttyS0,115200,keep ignore_loglevel BOOT_IMAGE=614-4864
>Unknown kernel command line parameters "base BOOT_IMAGE=614-4864", will be passed to user space.
>printk: log buffer data + meta data: 131072 + 409600 = 540672 bytes
>Dentry cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
>Inode-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
>Built 1 zonelists, mobility grouping on.  Total pages: 5021
>mem auto-init: stack:off, heap alloc:off, heap free:off
>Initializing HighMem for node 0 (00000000:00000000)
>Checking if this processor honours the WP bit even in supervisor mode...Ok.
>SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
>rcu: Hierarchical RCU implementation.
>rcu: 	RCU restricting CPUs from NR_CPUS=8 to nr_cpu_ids=1.
>	Tracing variant of Tasks RCU enabled.
>rcu: RCU calculated value of scheduler-enlistment delay is 30 jiffies.
>rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=1
>RCU Tasks Trace: Setting shift to 0 and lim to 1 rcu_task_cb_adjust=1 rcu_task_cpu_ids=1.
>NR_IRQS: 2304, nr_irqs: 32, preallocated irqs: 16
>rcu: srcu_init: Setting srcu_struct sizes based on contention.
>Console: colour VGA+ 80x60
>printk: legacy console [tty0] enabled
>APIC: Keep in PIC mode(8259)
>Calibrating delay loop... 30.96 BogoMIPS (lpj=51136)
>Last level iTLB entries: 4KB 0, 2MB 0, 4MB 0
>Last level dTLB entries: 4KB 0, 2MB 0, 4MB 0, 1GB 0
>x86/fpu: Probing for FPU: FSW=0x0000 FCW=0x037f
>x86/fpu: x87 FPU will use FSAVE
>Freeing SMP alternatives memory: 24K
>pid_max: default: 32768 minimum: 301
>Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
>Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
>smpboot: SMP disabled
>Performance Events: no PMU driver, software events only.
>signal: max sigframe size: 928
>Oops: invalid opcode: 0000 [#1] SMP
>CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0 #3
>EIP: 0xc0b5e1da
>Code: 7d dc 00 74 0b b9 07 00 00 00 89 df b0 01 f3 a5 83 c4 1c 5b 5e 5f 5d c3 55 b8 01 00 00 00 89 e5 57 56 31 f6 53 89 f1 83 ec 30 <0f> a2 b9 04 00 00 00 89 45 c4 8d 7d c8 89 f0 f3 ab e8 3c fb ff ff
>EAX: 00000001 EBX: c0be6b00 ECX: 00000000 EDX: 00000246
>ESI: 00000000 EDI: 00000000 EBP: c1309f08 ESP: c1309ecc
>DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
>CR0: 80050033 CR2: ffd38000 CR3: 00c16000 CR4: 00000000
>Call Trace:
> ? 0xc01231af
> ? 0xc01231c8
> ? 0xc0123222
> ? 0xc0123245
> ? 0xc01215ef
> ? 0xc01217f7
> ? 0xc0b5e1da
> ? 0xc0848e09
> ? 0xc012186f
> ? 0xc0b5e1da
> ? 0xc0848e41
> ? 0xc010105d
> ? 0xc01500d8
> ? 0xc0848e09
> ? 0xc0b5e1da
> ? 0xc01500d8
> ? 0xc0848e09
> ? 0xc0b5e1da
> ? 0xc085148a
> ? 0xc0524da8
> ? 0xc0b5e1c8
> 0xc0102100
> ? 0xc08514d5
> ? 0xc085148a
> ? 0xc0171bde
> ? 0xc0175987
> ? 0xc016d672
> 0xc0b4c9e2
> ? 0xc084c9b8
> 0xc084c9ca
> 0xc0127d3a
> ? 0xc084c9b8
> 0xc01027a2
> 0xc0100e3d
>Modules linked in:
>---[ end trace 0000000000000000 ]---
>EIP: 0xc0b5e1da
>Code: 7d dc 00 74 0b b9 07 00 00 00 89 df b0 01 f3 a5 83 c4 1c 5b 5e 5f 5d c3 55 b8 01 00 00 00 89 e5 57 56 31 f6 53 89 f1 83 ec 30 <0f> a2 b9 04 00 00 00 89 45 c4 8d 7d c8 89 f0 f3 ab e8 3c fb ff ff
>EAX: 00000001 EBX: c0be6b00 ECX: 00000000 EDX: 00000246
>ESI: 00000000 EDI: 00000000 EBP: c1309f08 ESP: c1309ecc
>DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
>CR0: 80050033 CR2: ffd38000 CR3: 00c16000 CR4: 00000000
>Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
>Rebooting in 60 seconds..

Can you please not post stack traces without any symbol information either internal or external? It is just random hex digits in the absence of a System.map or vmlinux file.

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Kevin Koster]

On Mon, 07 Apr 2025 07:38:59 -0700
"H. Peter Anvin" <hpa@zytor.com> wrote:
> Can you please not post stack traces without any symbol information
> either internal or external? It is just random hex digits in the
> absence of a System.map or vmlinux file.

OK, I'll replace them with "[snip stack trace]" in future unless asked
for the System.map file too.

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[David Laight]

On Tue, 8 Apr 2025 00:21:50 +1000
Kevin Koster <lkml@ombertech.com> wrote:

...
> > > and I'm not much better, writing this now on a Pentium 1.  
> > 
> > Lemme guess: this is your main machine you use for daily work?  
> 
> For email/news every morning, then a (newer) laptop afterwards.

Buy yourself a raspberry pi.

	David

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[H. Peter Anvin]

On April 7, 2025 3:29:27 AM PDT, Borislav Petkov <bp@alien8.de> wrote:
>On Mon, Apr 07, 2025 at 09:58:48AM +1000, Kevin Koster wrote:
>> But I like to know Linux really works on the hardware it's built for,
>
>I don't know what that means.
>
>> and I'm not much better, writing this now on a Pentium 1.
>
>Lemme guess: this is your main machine you use for daily work?
>
>:-\
>
>> > -	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;
>> 
>> Still fails unless the native_cpuid_eax(1) call is moved under here. After that
>> change, it boots fine.
>
>Please show me with a diff what you're doing because I don't know what you
>mean.
>
>I did this:
>
>bool have_cpuid_p(void)
>{
>        return false;
>}
>
>in order to simulate no CPUID support but my 32-bit guest boots fine.
>
>Also, send a full dmesg from that machine so that I can try to reproduce here.
>
>Thx.
>

Well, Linus' original box was a 386 (SX, I think?) so that ship sailed already.

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Maciej W. Rozycki]

On Sun, 6 Apr 2025, Borislav Petkov wrote:

> > But if I move "cpuid_1_eax = native_cpuid_eax(1);" to after the
> > microcode_loader_disabled() check in amd.c, like in the first patch, it
> > boots fine!
> 
> Yeah, I noticed that too last night. Here's a hopefully better version after
> I ran it on my 32-bit Atom - I don't have your old rust and maybe you should
> simply throw it in the garbage - that thing is probably not worth the
> electricity it uses to power up... :-)

 C'mon, these are good room heaters with nice extra side effects. ;)

  Maciej

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Borislav Petkov]

On Tue, Apr 08, 2025 at 11:16:26AM +0100, Maciej W. Rozycki wrote:
>  C'mon, these are good room heaters with nice extra side effects. ;)

Maybe we should intentionally prevent booting Linux on such machines and make
this our community's contribution in the fight against global warming!

:-P

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Maciej W. Rozycki]

On Tue, 8 Apr 2025, Borislav Petkov wrote:

> >  C'mon, these are good room heaters with nice extra side effects. ;)
> 
> Maybe we should intentionally prevent booting Linux on such machines and make
> this our community's contribution in the fight against global warming!

 Hehe...  But seriously, consuming energy is not by itself wrong, energy 
is plentiful in the universe and gets used all the time whether humankind 
contributes to it or not.  The issue is using dirty sources.  And that's 
not exactly individual people's problem, it's not us personally who are to 
build clean power stations.  Which does happen already, so we're moving in 
the right direction.  I would be more concerned of the various industrial 
contributors to global warming whose purpose isn't electricity production.

 Plus we still need heating anyway, at least in some places, so why not 
having one that runs Linux? ;)

 FWIW,

  Maciej

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[Borislav Petkov]

On Tue, Apr 08, 2025 at 01:29:40PM +0100, Maciej W. Rozycki wrote:
>  Hehe...  But seriously, consuming energy is not by itself wrong, energy 
> is plentiful in the universe and gets used all the time whether humankind 
> contributes to it or not.  The issue is using dirty sources.  And that's 
> not exactly individual people's problem, it's not us personally who are to 
> build clean power stations.  Which does happen already, so we're moving in 
> the right direction.  I would be more concerned of the various industrial 
> contributors to global warming whose purpose isn't electricity production.

Oh, ofc. But we have to start somewhere. And 0.000...1% here and 0.000...1%
there helps. Considering what hw options we have currently, there's absolutely
0 need and sense to keep such machines alive.

I'm even disassembling my old test boxes because they're simply not worth it.
They almost never catch an issue and testing on them is simply waste of time
and, well, energy.

>  Plus we still need heating anyway, at least in some places, so why not 
> having one that runs Linux? ;)

You can always get a big fat modern box - it heats even more but you'll
actually get something done with it.

:-P

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Fix crashes on early 486 CPUs due to usage of 'cpuid'.

[H. Peter Anvin]

On April 5, 2025 11:40:49 PM PDT, Kevin Koster <lkml@ombertech.com> wrote:
>On Sat, 5 Apr 2025 11:32:26 +0200
>Borislav Petkov <bp@alien8.de> wrote:
>> On Sat, Apr 05, 2025 at 01:03:06PM +1100, Kevin Koster wrote:
>> > On Sat, 19 Oct 2024 08:29:04 +0200
>> > Oerg866 <oerg866@googlemail.com> wrote:
>> > 
>> > > Starting with v6.7-rc1, the kernel was no longer able to boot on
>> > > early i486-class CPUs.
>> > 
>> > Thanks for this patch! It solves my problem with kernel 6.12.11
>> > rebooting at start-up on 486 CPUs, which had me puzzled. (tested on
>> > AM486DX2-66 and CX486DX4-100)
>> > 
>> > Is there a reason why the patch wasn't accepted?
>> 
>> Yes, too many patches, too little time. :-(
>> 
>> Anyway, does the one below - only build-tested - work for both y'all
>> too?
>
>On my AM486DX2-66 it gets past the immediate reboot problem but gets
>an Oops and kernel panic here:
>
>[snip]
>smpboot: SMP disabled
>Performance Events: no PMU driver, software events only.
>signal: max sigframe size: 928
>Oops: invalid opcode: 0000 [#1] SMP
>CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0 #1
>EIP: 0xc0b5e1d7
>Code: 7d dc 00 74 0b b9 07 00 00 00 89 df b0 01 f3 a5 83 c4 1c 5b 5e 5f 5d c3 55 b8 01 00 00 00 89 e5 57 56 31 f6 53 89 f1 83 ec 30 <0f> a2 b9 04 00 00 00 89 45 c4 8d 7d c8 89 f0 f3 ab e8 a0 fc ff ff
>EAX: 00000001 EBX: c0be6b00 ECX: 00000000 EDX: 00000246
>ESI: 00000000 EDI: 00000000 EBP: c1309f08 ESP: c1309ecc
>DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
>CR0: 80050033 CR2: ffd38000 CR3: 00c16000 CR4: 00000000
>Call Trace:
> ? 0xc01231af
> ? 0xc01231c8
> ? 0xc0123222
> ? 0xc0123245
> ? 0xc01215ef
> ? 0xc01217f7
> ? 0xc0b5e1d7
> ? 0xc0848e09
> ? 0xc012186f
> ? 0xc0b5e1d7
> ? 0xc0848e41
> ? 0xc010105d
> ? 0xc01500d8
> ? 0xc0848e09
> ? 0xc0b5e1d7
> ? 0xc01500d8
> ? 0xc0848e09
> ? 0xc0b5e1d7
> ? 0xc085148a
> ? 0xc0524da8
> ? 0xc0b5e1c5
> 0xc0102100
> ? 0xc08514d5
> ? 0xc085148a
> ? 0xc0171bde
> ? 0xc0175987
> ? 0xc016d672
> 0xc0b4c9e2
> ? 0xc084c9b8
> 0xc084c9ca
> 0xc0127d3a
> ? 0xc084c9b8
> 0xc01027a2
> 0xc0100e3d
>Modules linked in:
>---[ end trace 0000000000000000 ]---
>EIP: 0xc0b5e1d7
>Code: 7d dc 00 74 0b b9 07 00 00 00 89 df b0 01 f3 a5 83 c4 1c 5b 5e 5f 5d c3 55 b8 01 00 00 00 89 e5 57 56 31 f6 53 89 f1 83 ec 30 <0f> a2 b9 04 00 00 00 89 45 c4 8d 7d c8 89 f0 f3 ab e8 a0 fc ff ff
>EAX: 00000001 EBX: c0be6b00 ECX: 00000000 EDX: 00000246
>ESI: 00000000 EDI: 00000000 EBP: c1309f08 ESP: c1309ecc
>DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
>CR0: 80050033 CR2: ffd38000 CR3: 00c16000 CR4: 00000000
>Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
>Rebooting in 60 seconds..

Another case of it stumbling into a CPUID instruction. Of course, without a map file that dump is completely useless since it doesn't show any symbols.

[tip: x86/urgent] x86/microcode: Consolidate the loader enablement checking

[tip-bot2 for Borislav Petkov (AMD)]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     eb72bdfbd0a757f30ebe4f9ec161cb246d19e5ed
Gitweb:        https://git.kernel.org/tip/eb72bdfbd0a757f30ebe4f9ec161cb246d19e5ed
Author:        Borislav Petkov (AMD) <bp@alien8.de>
AuthorDate:    Mon, 14 Apr 2025 11:59:33 +02:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Sat, 03 May 2025 16:40:56 +02:00

x86/microcode: Consolidate the loader enablement checking

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     | 58 +++++++++++++----------
 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, 41 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 695e569..d53148f 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 4a10d35..96cb992 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 b3658d1..079f046 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 = 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;
@@ -95,27 +98,29 @@ 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;
 
 	/*
-	 * 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))
-		return true;
-
-	if (x86_cpuid_vendor() == X86_VENDOR_AMD) {
-		if (amd_check_current_patch_level())
-			return true;
-	}
-
-	if (cmdline_find_option_bool(cmdline, option) <= 0)
-		dis_ucode_ldr = false;
+	if (!have_cpuid_p() ||
+	    native_cpuid_ecx(1) & BIT(31) ||
+	    amd_check_current_patch_level())
+		dis_ucode_ldr = true;
 
 	return dis_ucode_ldr;
 }
@@ -125,7 +130,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 = true;
+
+	if (microcode_loader_disabled())
 		return;
 
 	cpuid_1_eax = native_cpuid_eax(1);
@@ -146,9 +154,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 +164,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 +820,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 819199b..2a397da 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 5df6217..50a9702 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 de001b2..375f2d7 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;

Re: [tip: x86/urgent] x86/microcode: Consolidate the loader enablement checking

[Ingo Molnar]

* tip-bot2 for Borislav Petkov (AMD) <tip-bot2@linutronix.de> wrote:

> The following commit has been merged into the x86/urgent branch of tip:
> 
> Commit-ID:     eb72bdfbd0a757f30ebe4f9ec161cb246d19e5ed
> Gitweb:        https://git.kernel.org/tip/eb72bdfbd0a757f30ebe4f9ec161cb246d19e5ed
> Author:        Borislav Petkov (AMD) <bp@alien8.de>
> AuthorDate:    Mon, 14 Apr 2025 11:59:33 +02:00
> Committer:     Borislav Petkov (AMD) <bp@alien8.de>
> CommitterDate: Sat, 03 May 2025 16:40:56 +02:00
> 
> x86/microcode: Consolidate the loader enablement checking
> 
> 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

So the title claims this 'consolidates' microcode loader enablement, 
which is basically a code refactoring, but then there's a Fixes tag - 
what exactly does it fix? There's no explanation in the changelog that 
I can see that justifies that tag and why it's in x86/urgent.

Thanks,

	Ingo

[PATCH] x86/microcode: Add microcode_loader_disabled() storage class for the !CONFIG_MICROCODE case

[Ingo Molnar]

Fix this build bug:

  ./arch/x86/include/asm/microcode.h:27:13: warning: no previous prototype for ‘microcode_loader_disabled’ [-Wmissing-prototypes]

by adding the 'static' storage class to the !CONFIG_MICROCODE 
prototype.

Also, while at it, add all the other storage classes as well for this 
block of prototypes, 'extern' and 'static', respectively.

( Omitting 'extern' just because it's technically not needed
  is a bad habit for header prototypes and leads to bugs like
  this one. )

Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/microcode.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 6aa12aecbc95..c23849246d0a 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -16,15 +16,15 @@ struct ucode_cpu_info {
 };
 
 #ifdef CONFIG_MICROCODE
-void load_ucode_bsp(void);
-void load_ucode_ap(void);
-void microcode_bsp_resume(void);
-bool __init microcode_loader_disabled(void);
+extern void load_ucode_bsp(void);
+extern void load_ucode_ap(void);
+extern void microcode_bsp_resume(void);
+extern 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; }
+static inline bool __init microcode_loader_disabled(void) { return false; }
 #endif
 
 extern unsigned long initrd_start_early;

[tip: x86/microcode] x86/microcode: Add microcode_loader_disabled() storage class for the !CONFIG_MICROCODE case

[tip-bot2 for Ingo Molnar]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     e26d770a995f6f1ddb7c0c6d24f1aa81fb41eaa4
Gitweb:        https://git.kernel.org/tip/e26d770a995f6f1ddb7c0c6d24f1aa81fb41eaa4
Author:        Ingo Molnar <mingo@kernel.org>
AuthorDate:    Mon, 05 May 2025 07:15:04 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 05 May 2025 07:15:27 +02:00

x86/microcode: Add microcode_loader_disabled() storage class for the !CONFIG_MICROCODE case

Fix this build bug:

  ./arch/x86/include/asm/microcode.h:27:13: warning: no previous prototype for ‘microcode_loader_disabled’ [-Wmissing-prototypes]

by adding the 'static' storage class to the !CONFIG_MICROCODE
prototype.

Also, while at it, add all the other storage classes as well for this
block of prototypes, 'extern' and 'static', respectively.

( Omitting 'extern' just because it's technically not needed
  is a bad habit for header prototypes and leads to bugs like
  this one. )

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: "Borislav Petkov (AMD)" <bp@alien8.de>
Link: https://lore.kernel.org/r/aBhJVJDTlw2Y8owu@gmail.com
---
 arch/x86/include/asm/microcode.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index d53148f..af33bbf 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -14,15 +14,15 @@ struct ucode_cpu_info {
 };
 
 #ifdef CONFIG_MICROCODE
-void load_ucode_bsp(void);
-void load_ucode_ap(void);
-void microcode_bsp_resume(void);
-bool __init microcode_loader_disabled(void);
+extern void load_ucode_bsp(void);
+extern void load_ucode_ap(void);
+extern void microcode_bsp_resume(void);
+extern 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; }
+static inline bool __init microcode_loader_disabled(void) { return false; }
 #endif
 
 extern unsigned long initrd_start_early;

[tip: x86/microcode] x86/microcode: Add microcode_loader_disabled() storage class for the !CONFIG_MICROCODE case

[tip-bot2 for Ingo Molnar]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     59e820c6de60e81757211fbb846129485e337ee0
Gitweb:        https://git.kernel.org/tip/59e820c6de60e81757211fbb846129485e337ee0
Author:        Ingo Molnar <mingo@kernel.org>
AuthorDate:    Mon, 05 May 2025 07:15:04 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 05 May 2025 09:17:02 +02:00

x86/microcode: Add microcode_loader_disabled() storage class for the !CONFIG_MICROCODE case

Fix this build bug:

  ./arch/x86/include/asm/microcode.h:27:13: warning: no previous prototype for ‘microcode_loader_disabled’ [-Wmissing-prototypes]

by adding the 'static' storage class to the !CONFIG_MICROCODE
prototype.

Also, while at it, add all the other storage classes as well for this
block of prototypes, 'extern' and 'static', respectively.

( Omitting 'extern' just because it's technically not needed
  is a bad habit for header prototypes and leads to bugs like
  this one. )

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: "Borislav Petkov (AMD)" <bp@alien8.de>
Link: https://lore.kernel.org/r/aBhJVJDTlw2Y8owu@gmail.com
---
 arch/x86/include/asm/microcode.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index d53148f..b68fc9d 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -14,15 +14,15 @@ struct ucode_cpu_info {
 };
 
 #ifdef CONFIG_MICROCODE
-void load_ucode_bsp(void);
-void load_ucode_ap(void);
-void microcode_bsp_resume(void);
-bool __init microcode_loader_disabled(void);
+extern void load_ucode_bsp(void);
+extern void load_ucode_ap(void);
+extern void microcode_bsp_resume(void);
+extern 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; }
+static inline bool microcode_loader_disabled(void) { return false; }
 #endif
 
 extern unsigned long initrd_start_early;

Re: [PATCH] x86/microcode: Add microcode_loader_disabled() storage class for the !CONFIG_MICROCODE case

[Borislav Petkov]

On Mon, May 05, 2025 at 07:15:00AM +0200, Ingo Molnar wrote:
> 
> Fix this build bug:
> 
>   ./arch/x86/include/asm/microcode.h:27:13: warning: no previous prototype for ‘microcode_loader_disabled’ [-Wmissing-prototypes]
> 
> by adding the 'static' storage class to the !CONFIG_MICROCODE 
> prototype.

Thanks, I'll merge it with my patch.

> Also, while at it, add all the other storage classes as well for this 
> block of prototypes, 'extern' and 'static', respectively.

This I won't add as it is unnecessary.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: Add microcode_loader_disabled() storage class for the !CONFIG_MICROCODE case

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Mon, May 05, 2025 at 07:15:00AM +0200, Ingo Molnar wrote:
> > 
> > Fix this build bug:
> > 
> >   ./arch/x86/include/asm/microcode.h:27:13: warning: no previous prototype for ‘microcode_loader_disabled’ [-Wmissing-prototypes]
> > 
> > by adding the 'static' storage class to the !CONFIG_MICROCODE 
> > prototype.
> 
> Thanks, I'll merge it with my patch.

Please don't forcibly rebase trees like that, because you've just 
reintroduced a bug this way in tip:x86/urgent:

  # 5214a9f6c0f5 x86/microcode: Consolidate the loader enablement checking

  +static inline bool __init microcode_loader_disabled(void) { return false; }

static inlines don't need an __init tag ...

This was done correctly in the commit you removed:

  59e820c6de60 ("x86/microcode: Add microcode_loader_disabled() storage class for the !CONFIG_MICROCODE case")

  +static inline bool microcode_loader_disabled(void) { return false; }

> > Also, while at it, add all the other storage classes as well for this 
> > block of prototypes, 'extern' and 'static', respectively.
> 
> This I won't add as it is unnecessary.

Technically the 'int' in 'unsigned int' is 'unnecessary' and could be 
skipped as well with 'unsigned', right? Yet clean kernel code uses it 
because it's easier to read and more consistent. Likewise, the 'extern' 
storage class is a well-known and widespread way in the kernel to 
document public APIs, a counterpart to 'static'. Most of our major 
headers use that style.

Your change is a doubly poor choice in this particular case of 
<asm/microcode.h> as well, because the header continues with an 
'extern':

  extern unsigned long initrd_start_early;

So by dropping that cleanup you reintroduced an inconsistency as 
well...

Thanks,

	Ingo

Re: [PATCH] x86/microcode: Add microcode_loader_disabled() storage class for the !CONFIG_MICROCODE case

[Borislav Petkov]

On Tue, May 06, 2025 at 12:57:08PM +0200, Ingo Molnar wrote:
> Please don't forcibly rebase trees like that, because you've just 
> reintroduced a bug this way in tip:x86/urgent:
> 
>   # 5214a9f6c0f5 x86/microcode: Consolidate the loader enablement checking
> 
>   +static inline bool __init microcode_loader_disabled(void) { return false; }
> 
> static inlines don't need an __init tag ...

Oh please do tell what kind of bug that is. Especially if in
a CONFIG_MICROCODE=n build *all* the callsites of that function are gone!

Geez.

> Technically the 'int' in 'unsigned int' is 'unnecessary' and could be 
> skipped as well with 'unsigned', right?

And this is the same how exactly?

> Yet clean kernel code uses it because it's easier to read and more
> consistent. Likewise, the 'extern' storage class is a well-known and
> widespread way in the kernel to document public APIs, a counterpart to
> 'static'. Most of our major headers use that style.

Does this header need it?

No. Not really.

If it fixes something, yes, sure, by all means.

Gratuitous cleanups without any point: no. 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[tip: x86/urgent] x86/microcode: Consolidate the loader enablement checking

[tip-bot2 for Borislav Petkov (AMD)]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     5214a9f6c0f56644acb9d2cbb58facf1856d322b
Gitweb:        https://git.kernel.org/tip/5214a9f6c0f56644acb9d2cbb58facf1856d322b
Author:        Borislav Petkov (AMD) <bp@alien8.de>
AuthorDate:    Mon, 14 Apr 2025 11:59:33 +02:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 05 May 2025 10:51:00 +02:00

x86/microcode: Consolidate the loader enablement checking

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.

In doing so, address the issue of old 486er machines without CPUID
support, not booting current kernels.

  [ mingo: Fix no previous prototype for ‘microcode_loader_disabled’ [-Wmissing-prototypes] ]

Fixes: 4c585af7180c1 ("x86/boot/32: Temporarily map initrd for microcode loading")
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
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     | 58 +++++++++++++----------
 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, 41 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 695e569..be7cddc 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) { }
+static inline 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 4a10d35..96cb992 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 b3658d1..079f046 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 = 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;
@@ -95,27 +98,29 @@ 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;
 
 	/*
-	 * 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))
-		return true;
-
-	if (x86_cpuid_vendor() == X86_VENDOR_AMD) {
-		if (amd_check_current_patch_level())
-			return true;
-	}
-
-	if (cmdline_find_option_bool(cmdline, option) <= 0)
-		dis_ucode_ldr = false;
+	if (!have_cpuid_p() ||
+	    native_cpuid_ecx(1) & BIT(31) ||
+	    amd_check_current_patch_level())
+		dis_ucode_ldr = true;
 
 	return dis_ucode_ldr;
 }
@@ -125,7 +130,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 = true;
+
+	if (microcode_loader_disabled())
 		return;
 
 	cpuid_1_eax = native_cpuid_eax(1);
@@ -146,9 +154,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 +164,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 +820,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 819199b..2a397da 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 5df6217..50a9702 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 de001b2..375f2d7 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;