[PATCH tip] x86/microcode/AMD: Read from MSR_AMD64_PATCH_LEVEL to get base_rev if not defined

[PATCH tip] x86/microcode/AMD: Read from MSR_AMD64_PATCH_LEVEL to get base_rev if not defined

[Waiman Long]

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.1

Re: [PATCH tip] x86/microcode/AMD: Read from MSR_AMD64_PATCH_LEVEL to get base_rev if not defined

[Borislav Petkov]

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?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH tip] x86/microcode/AMD: Read from MSR_AMD64_PATCH_LEVEL to get base_rev if not defined

[Waiman Long]

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

Re: [PATCH tip] x86/microcode/AMD: Read from MSR_AMD64_PATCH_LEVEL to get base_rev if not defined

[Borislav Petkov]

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

Re: [PATCH tip] x86/microcode/AMD: Read from MSR_AMD64_PATCH_LEVEL to get base_rev if not defined

[Waiman Long]

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);
>   
>