arch/x86/kernel/cpu/microcode/amd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
AMD microcode is being handled inconsistenly depending on whether the
CONFIG_MICROCODE_DBG is on or off. Currently, the MSR_AMD64_PATCH_LEVEL
MSR is being read in get_patch_level() to retrieve microcode revision
when CONFIG_MICROCODE_DBG is off, but cpuid_to_ucode_rev(bsp_cpuid_1_eax)
is used to get the revision with CONFIG_MICROCODE_DBG on if
microcode_rev[cpu] hasn't been defined yet.
On a test Genoa system with a relatively new BIOS/firmware,
get_patch_level() returns 0xa101100 with cpuid_to_ucode_rev(), but
0x0a101158 when reading from MSR_AMD64_PATCH_LEVEL. It does look
like bsp_cpuid_1_eax doesn't contain the right microcode revision
for microcode embedded in the BIOS/firmware. This is problematic as
need_sha_check() may return an incorrect result.
To fix the inconsistency and provide a more correct result, always read
from the MSR in get_patch_level() to get the microcode revision number.
Fixes: 43181a47263d ("x86/microcode: Add microcode loader debugging functionality")
Signed-off-by: Waiman Long <longman@redhat.com>
---
arch/x86/kernel/cpu/microcode/amd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 3821a985f4ff..561630c017e2 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -327,7 +327,7 @@ static u32 get_patch_level(void)
if (!microcode_rev[cpu]) {
if (!base_rev)
- base_rev = cpuid_to_ucode_rev(bsp_cpuid_1_eax);
+ native_rdmsr(MSR_AMD64_PATCH_LEVEL, base_rev, dummy);
microcode_rev[cpu] = base_rev;
--
2.51.1On Mon, Nov 17, 2025 at 02:15:27PM -0500, Waiman Long wrote:
> AMD microcode is being handled inconsistenly depending on whether the
> CONFIG_MICROCODE_DBG is on or off.
Did you read the help text to CONFIG_MICROCODE_DBG?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 11/17/25 2:37 PM, Borislav Petkov wrote: > On Mon, Nov 17, 2025 at 02:15:27PM -0500, Waiman Long wrote: >> AMD microcode is being handled inconsistenly depending on whether the >> CONFIG_MICROCODE_DBG is on or off. > Did you read the help text to CONFIG_MICROCODE_DBG? > I am fine if "microcode,base_rev=" is specified in the boot command line to force a base_rev for testing purpose. But if that is not specified, get_patch_level() should return the right value. The problem I have on that particular test machine mentioned in the commit is that the kernel is trying to load an incompatible microcode blob causing error like the following when CONFIG_MICROCODE_DBG is on. [ 0.000000] unchecked MSR access error: WRMSR to 0xc0010020 (tried to write 0xff11000026d3e740) at rIP: 0xffffffffa72cc63b (__apply_microcode_amd+0x3b/0xb0) [ 0.000000] Call Trace: [ 0.000000] <TASK> [ 0.000000] ? show_trace_log_lvl+0x1b0/0x2f0 [ 0.000000] ? show_trace_log_lvl+0x1b0/0x2f0 [ 0.000000] ? ex_handler_msr.isra.0.cold+0x5b/0x60 [ 0.000000] ? fixup_exception+0x8f/0x380 [ 0.000000] ? early_fixup_exception+0x45/0xb0 [ 0.000000] ? early_idt_handler_common+0x2f/0x3a [ 0.000000] ? __apply_microcode_amd+0x3b/0xb0 [ 0.000000] ? load_ucode_amd_bsp+0x10b/0x140 [ 0.000000] ? x86_64_start_kernel+0x84/0xa0 [ 0.000000] ? common_startup_64+0x13e/0x141 [ 0.000000] </TASK> [ 0.000000] microcode: updated rev: 0xa101148 I can add a boot_cpu_has(X86_FEATURE_HYPERVISOR) check and use native_rdmsr() only if there is no hypervisor running. Do you think that is acceptable? Regards, Longman
On Mon, Nov 17, 2025 at 02:58:30PM -0500, Waiman Long wrote:
> when CONFIG_MICROCODE_DBG is on.
Again, CONFIG_MICROCODE_DBG is only to be used in a guest. Like the help text
says. For now at least.
I have tried to extend it to debugging on baremetal - see below - but this is
unfinished.
---
Author: Borislav Petkov (AMD) <bp@alien8.de>
Date: Mon Oct 6 17:50:10 2025 +0200
Host debugging
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index fa3b616af03a..c213e00ea963 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1362,10 +1362,12 @@ config MICROCODE_DBG
default n
depends on MICROCODE
help
- Enable code which allows for debugging the microcode loader in
- a guest. Meaning the patch loading is simulated but everything else
+ Enable code which allows to debug the microcode loader. When running
+ in a guest the patch loading is simulated but everything else
related to patch parsing and handling is done as on baremetal with
- the purpose of debugging solely the software side of things.
+ the purpose of debugging solely the software side of things. On
+ baremetal, it simply dumps additional debugging information as it
+ goes.
You almost certainly want to say n here.
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index a584f9cbf9a3..c25db0d40629 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -301,7 +301,7 @@ static u32 get_patch_level(void)
{
u32 rev, dummy __always_unused;
- if (IS_ENABLED(CONFIG_MICROCODE_DBG)) {
+ if (IS_ENABLED(CONFIG_MICROCODE_DBG) && hypervisor_present) {
int cpu = smp_processor_id();
if (!microcode_rev[cpu]) {
@@ -694,7 +694,7 @@ static bool __apply_microcode_amd(struct microcode_amd *mc, u32 *cur_rev,
invlpg(p_addr_end);
}
- if (IS_ENABLED(CONFIG_MICROCODE_DBG))
+ if (IS_ENABLED(CONFIG_MICROCODE_DBG) && hypervisor_present)
microcode_rev[smp_processor_id()] = mc->hdr.patch_id;
/* verify patch application was successful */
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index f75c140906d0..ae0ba9df501b 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -57,6 +57,8 @@ bool force_minrev = IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV);
u32 base_rev;
u32 microcode_rev[NR_CPUS] = {};
+bool hypervisor_present;
+
/*
* Synchronization.
*
@@ -117,6 +119,13 @@ bool __init microcode_loader_disabled(void)
* Disable when:
*
* 1) The CPU does not support CPUID.
+ */
+ if (!cpuid_feature()) {
+ dis_ucode_ldr = true;
+ return dis_ucode_ldr;
+ }
+
+ /*
*
* 2) Bit 31 in CPUID[1]:ECX is clear
* The bit is reserved for hypervisor use. This is still not
@@ -127,9 +136,9 @@ bool __init microcode_loader_disabled(void)
* 3) Certain AMD patch levels are not allowed to be
* overwritten.
*/
- if (!cpuid_feature() ||
- ((native_cpuid_ecx(1) & BIT(31)) &&
- !IS_ENABLED(CONFIG_MICROCODE_DBG)) ||
+ hypervisor_present = native_cpuid_ecx(1) & BIT(31);
+
+ if ((hypervisor_present && !IS_ENABLED(CONFIG_MICROCODE_DBG)) ||
amd_check_current_patch_level())
dis_ucode_ldr = true;
diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
index ae8dbc2b908d..f084aac6c839 100644
--- a/arch/x86/kernel/cpu/microcode/internal.h
+++ b/arch/x86/kernel/cpu/microcode/internal.h
@@ -46,6 +46,7 @@ extern struct early_load_data early_data;
extern struct ucode_cpu_info ucode_cpu_info[];
extern u32 microcode_rev[NR_CPUS];
extern u32 base_rev;
+extern bool hypervisor_present;
struct cpio_data find_microcode_in_initrd(const char *path);
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 11/17/25 4:11 PM, Borislav Petkov wrote:
> On Mon, Nov 17, 2025 at 02:58:30PM -0500, Waiman Long wrote:
>> when CONFIG_MICROCODE_DBG is on.
> Again, CONFIG_MICROCODE_DBG is only to be used in a guest. Like the help text
> says. For now at least.
>
> I have tried to extend it to debugging on baremetal - see below - but this is
> unfinished.
I see. In that case, I am going to wait for your patch then.
Thanks for the info.
Cheers,
Longman
>
> ---
> Author: Borislav Petkov (AMD) <bp@alien8.de>
> Date: Mon Oct 6 17:50:10 2025 +0200
>
> Host debugging
>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index fa3b616af03a..c213e00ea963 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1362,10 +1362,12 @@ config MICROCODE_DBG
> default n
> depends on MICROCODE
> help
> - Enable code which allows for debugging the microcode loader in
> - a guest. Meaning the patch loading is simulated but everything else
> + Enable code which allows to debug the microcode loader. When running
> + in a guest the patch loading is simulated but everything else
> related to patch parsing and handling is done as on baremetal with
> - the purpose of debugging solely the software side of things.
> + the purpose of debugging solely the software side of things. On
> + baremetal, it simply dumps additional debugging information as it
> + goes.
>
> You almost certainly want to say n here.
>
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index a584f9cbf9a3..c25db0d40629 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -301,7 +301,7 @@ static u32 get_patch_level(void)
> {
> u32 rev, dummy __always_unused;
>
> - if (IS_ENABLED(CONFIG_MICROCODE_DBG)) {
> + if (IS_ENABLED(CONFIG_MICROCODE_DBG) && hypervisor_present) {
> int cpu = smp_processor_id();
>
> if (!microcode_rev[cpu]) {
> @@ -694,7 +694,7 @@ static bool __apply_microcode_amd(struct microcode_amd *mc, u32 *cur_rev,
> invlpg(p_addr_end);
> }
>
> - if (IS_ENABLED(CONFIG_MICROCODE_DBG))
> + if (IS_ENABLED(CONFIG_MICROCODE_DBG) && hypervisor_present)
> microcode_rev[smp_processor_id()] = mc->hdr.patch_id;
>
> /* verify patch application was successful */
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index f75c140906d0..ae0ba9df501b 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -57,6 +57,8 @@ bool force_minrev = IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV);
> u32 base_rev;
> u32 microcode_rev[NR_CPUS] = {};
>
> +bool hypervisor_present;
> +
> /*
> * Synchronization.
> *
> @@ -117,6 +119,13 @@ bool __init microcode_loader_disabled(void)
> * Disable when:
> *
> * 1) The CPU does not support CPUID.
> + */
> + if (!cpuid_feature()) {
> + dis_ucode_ldr = true;
> + return dis_ucode_ldr;
> + }
> +
> + /*
> *
> * 2) Bit 31 in CPUID[1]:ECX is clear
> * The bit is reserved for hypervisor use. This is still not
> @@ -127,9 +136,9 @@ bool __init microcode_loader_disabled(void)
> * 3) Certain AMD patch levels are not allowed to be
> * overwritten.
> */
> - if (!cpuid_feature() ||
> - ((native_cpuid_ecx(1) & BIT(31)) &&
> - !IS_ENABLED(CONFIG_MICROCODE_DBG)) ||
> + hypervisor_present = native_cpuid_ecx(1) & BIT(31);
> +
> + if ((hypervisor_present && !IS_ENABLED(CONFIG_MICROCODE_DBG)) ||
> amd_check_current_patch_level())
> dis_ucode_ldr = true;
>
> diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
> index ae8dbc2b908d..f084aac6c839 100644
> --- a/arch/x86/kernel/cpu/microcode/internal.h
> +++ b/arch/x86/kernel/cpu/microcode/internal.h
> @@ -46,6 +46,7 @@ extern struct early_load_data early_data;
> extern struct ucode_cpu_info ucode_cpu_info[];
> extern u32 microcode_rev[NR_CPUS];
> extern u32 base_rev;
> +extern bool hypervisor_present;
>
> struct cpio_data find_microcode_in_initrd(const char *path);
>
>
© 2016 - 2025 Red Hat, Inc.