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

[Sohil Mehta <sohil.mehta@intel.com>]

On 8/20/2025 6:50 AM, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.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.