Re: [PATCH 1/2] x86/microcode: Add microcode= cmdline parsing

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

On 8/9/2025 2:42 AM, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> Add a "microcode=" command line argument after which all options can be
> passed in a comma-separated list.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> ---
>  .../admin-guide/kernel-parameters.txt         |  8 +++--
>  arch/x86/kernel/cpu/microcode/core.c          | 30 +++++++++++++++----
>  2 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 747a55abf494..7c095177da85 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3767,10 +3767,12 @@
>  
>  	mga=		[HW,DRM]
>  
> -	microcode.force_minrev=	[X86]
> -			Format: <bool>
> +	microcode=      [X86] Control the behavior of the microcode loader.
> +	                Available options, comma separated:
> +


How about adding something like this to encourage using the new options:


@@ -1211,7 +1211,7 @@

-       dis_ucode_ldr   [X86] Disable the microcode loader.
+       dis_ucode_ldr   [X86] Same as microcode=disable_loader.


@@ -3770,6 +3770,9 @@

+                       disable_loader
+                               Disable the microcode loader.
+

> +			force_minrev
>  			Enable or disable the microcode minimal revision
> -			enforcement for the runtime microcode loader.
> + 			enforcement for the runtime microcode loader.


Unnecessary whitespace added before tab.


>  	mini2440=	[ARM,HW,KNL]
>  			Format:[0..2][b][c][t]
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index b92e09a87c69..351a51861f00 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -43,10 +43,9 @@
>  #include "internal.h"
>  
>  static struct microcode_ops *microcode_ops;
> -static bool dis_ucode_ldr = false;
> +static bool dis_ucode_ldr;
>  
> -bool force_minrev = IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV);
> -module_param(force_minrev, bool, S_IRUSR | S_IWUSR);
> +bool force_minrev = false;
>  

Setting this to "false" is redundant.

>  /*
>   * Synchronization.
> @@ -126,13 +125,34 @@ bool __init microcode_loader_disabled(void)
>  	return dis_ucode_ldr;
>  }
>  
> +static void early_parse_cmdline(void)
> +{
> +	char cmd_buf[64] = {};
> +	char *s, *p = cmd_buf;
> +
> +	if (cmdline_find_option(boot_command_line, "microcode", cmd_buf, 64) > 0) {
> +		while ((s = strsep(&p, ","))) {
> +			if (IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV)) {
> +				if (!strcmp("force_minrev", s))
> +					force_minrev = true;
> +			}
> +
> +			if (!strcmp(s, "dis_ucode_ldr"))

If you agree with the above suggestion,
s/dis_ucode_ldr/disable_loader

> +				dis_ucode_ldr = true;
> +		}
> +	}
> +
> +	/* old, compat option */
> +	if (cmdline_find_option_bool(boot_command_line, "dis_ucode_ldr") > 0)
> +		dis_ucode_ldr = true;
> +}
> +
>  void __init load_ucode_bsp(void)
>  {
>  	unsigned int cpuid_1_eax;
>  	bool intel = true;
>  
> -	if (cmdline_find_option_bool(boot_command_line, "dis_ucode_ldr") > 0)
> -		dis_ucode_ldr = true;
> +	early_parse_cmdline();
>  
>  	if (microcode_loader_disabled())
>  		return;