linux-kernel - Re: [PATCH -v1 2/2] x86/microcode: Add microcode loader debugging functionality

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2cd370e9-6fd8-4aed-8fe8-c8526a1ab97f@intel.com>
Date: Wed, 20 Aug 2025 22:19:11 -0700
From: Sohil Mehta <sohil.mehta@...el.com>
To: Borislav Petkov <bp@...nel.org>, X86 ML <x86@...nel.org>
CC: "Chang S. Bae" <chang.seok.bae@...el.com>, LKML
	<linux-kernel@...r.kernel.org>, "Borislav Petkov (AMD)" <bp@...en8.de>
Subject: Re: [PATCH -v1 2/2] x86/microcode: Add microcode loader debugging
 functionality

On 8/20/2025 6:50 AM, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@...en8.de>
> 
> Instead of adding ad-hoc debugging glue to the microcode loader each
> time I need it, add debugging functionality which is not built by
> default and when built-in, off by default so that it can only be enabled
> explicitly on the command line.
> 

I didn't realize this last time. It's supposed to be compile-time
disabled and runtime disabled by default (which makes sense).


> +static u32 get_patch_level(void)
> +{
> +	u32 rev, dummy __always_unused;
> +
> +	if (IS_ENABLED(CONFIG_MICROCODE_DBG)) {

Does this need to be (IS_ENABLED(CONFIG_MICROCODE_DBG) && dbg)?

The base_rev description says:
base_rev=X - with <X> with format: <u32>
		Set the base microcode revision of each thread when in
		debug mode.

IIUC, the base_rev handling is also supposed to be runtime disabled by
default, right?

You can probably directly check for "if (dbg)" but that would remove the
compile time code optimization.


> +		int cpu = smp_processor_id();
> +
> +		if (!microcode_rev[cpu]) {
> +			if (!base_rev)
> +				base_rev = cpuid_to_ucode_rev(bsp_cpuid_1_eax);
> +
> +			microcode_rev[cpu] = base_rev;
> +
> +			ucode_dbg("CPU%d, base_rev: 0x%x\n", cpu, base_rev);
> +		}
> +
> +		return microcode_rev[cpu];
> +	}
> +
> +	native_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
> +
> +	return rev;
> +}
> +

...

> @@ -619,8 +657,14 @@ static bool __apply_microcode_amd(struct microcode_amd *mc, u32 *cur_rev,
>  			invlpg(p_addr_end);
>  	}
>  
> +	if (IS_ENABLED(CONFIG_MICROCODE_DBG))
> +		microcode_rev[smp_processor_id()] = mc->hdr.patch_id;
> +

Ditto.

>  	/* verify patch application was successful */
>  	*cur_rev = get_patch_level();

...

The rest of the changes look fine to me.