[PATCH 0/2] x86/microcode: Add debugging glue

[PATCH 0/2] x86/microcode: Add debugging glue

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

Hi,

this is something I've been meaning to do for a long time: each time when
doing despicable things to the loader, I get to add debugging code too and run
it in a VM to see how those despicable things fare. But then I remove the
debugging glue again when cleaning up the despicable things and turning them
into proper patches.

So make this debugging code permanent but keep it out of reach from production
use and have it build- and boot-disabled  by default.

Thx.

Borislav Petkov (AMD) (2):
  x86/microcode: Add microcode= cmdline parsing
  x86/microcode: Add microcode loader debugging functionality

 .../admin-guide/kernel-parameters.txt         | 15 +++-
 arch/x86/Kconfig                              | 12 +++
 arch/x86/kernel/cpu/microcode/amd.c           | 88 ++++++++++++++-----
 arch/x86/kernel/cpu/microcode/core.c          | 55 ++++++++++--
 arch/x86/kernel/cpu/microcode/internal.h      | 10 +++
 5 files changed, 149 insertions(+), 31 deletions(-)

-- 
2.43.0

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

[Borislav Petkov]

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:
+
+			force_minrev
 			Enable or disable the microcode minimal revision
-			enforcement for the runtime microcode loader.
+ 			enforcement for the runtime microcode loader.
 
 	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;
 
 /*
  * 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"))
+				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;
-- 
2.43.0

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

[Borislav Petkov]

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.

Simulate all patch handling the loader does except the actual loading of
the microcode patch into the hw.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 .../admin-guide/kernel-parameters.txt         |  7 ++
 arch/x86/Kconfig                              | 12 +++
 arch/x86/kernel/cpu/microcode/amd.c           | 88 ++++++++++++++-----
 arch/x86/kernel/cpu/microcode/core.c          | 25 +++++-
 arch/x86/kernel/cpu/microcode/internal.h      | 10 +++
 5 files changed, 119 insertions(+), 23 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 7c095177da85..fc002b1a9f57 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3770,6 +3770,13 @@
 	microcode=      [X86] Control the behavior of the microcode loader.
 	                Available options, comma separated:
 
+			dbg - Format: <bool>
+			enable debugging mode when run in a guest
+
+			base_rev=X - with <X> with format: <u32>
+			Set the base microcode revision of each thread when in
+			debug mode.
+
 			force_minrev
 			Enable or disable the microcode minimal revision
  			enforcement for the runtime microcode loader.
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 58d890fe2100..462bf03aeda5 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1360,6 +1360,18 @@ config MICROCODE_LATE_FORCE_MINREV
 
 	  If unsure say Y.
 
+config MICROCODE_DBG
+	bool "Enable microcode loader debugging"
+	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
+	  related to patch parsing and handling is done as on baremetal with
+	  the purpose of debugging solely the software side of things.
+
+	  You almost certainly want to say n here.
+
 config X86_MSR
 	tristate "/dev/cpu/*/msr - Model-specific register support"
 	help
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 097e39327942..ced499789d64 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -249,15 +249,6 @@ static bool verify_sha256_digest(u32 patch_id, u32 cur_rev, const u8 *data, unsi
 	return true;
 }
 
-static u32 get_patch_level(void)
-{
-	u32 rev, dummy __always_unused;
-
-	native_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
-
-	return rev;
-}
-
 static union cpuid_1_eax ucode_rev_to_cpuid(unsigned int val)
 {
 	union zen_patch_rev p;
@@ -275,6 +266,45 @@ static union cpuid_1_eax ucode_rev_to_cpuid(unsigned int val)
 	return c;
 }
 
+static u32 cpuid_to_ucode_rev(unsigned int val)
+{
+	union zen_patch_rev p = {};
+	union cpuid_1_eax c;
+
+	c.full = val;
+
+	p.stepping  = c.stepping;
+	p.model     = c.model;
+	p.ext_model = c.ext_model;
+	p.ext_fam   = c.ext_fam;
+
+	return p.ucode_rev;
+}
+
+static u32 get_patch_level(void)
+{
+	u32 rev, dummy __always_unused;
+
+	if (IS_ENABLED(CONFIG_MICROCODE_DBG)) {
+		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;
+}
+
 static u16 find_equiv_id(struct equiv_cpu_table *et, u32 sig)
 {
 	unsigned int i;
@@ -304,13 +334,13 @@ static bool verify_container(const u8 *buf, size_t buf_size)
 	u32 cont_magic;
 
 	if (buf_size <= CONTAINER_HDR_SZ) {
-		pr_debug("Truncated microcode container header.\n");
+		ucode_dbg("Truncated microcode container header.\n");
 		return false;
 	}
 
 	cont_magic = *(const u32 *)buf;
 	if (cont_magic != UCODE_MAGIC) {
-		pr_debug("Invalid magic value (0x%08x).\n", cont_magic);
+		ucode_dbg("Invalid magic value (0x%08x).\n", cont_magic);
 		return false;
 	}
 
@@ -335,8 +365,8 @@ static bool verify_equivalence_table(const u8 *buf, size_t buf_size)
 
 	cont_type = hdr[1];
 	if (cont_type != UCODE_EQUIV_CPU_TABLE_TYPE) {
-		pr_debug("Wrong microcode container equivalence table type: %u.\n",
-			 cont_type);
+		ucode_dbg("Wrong microcode container equivalence table type: %u.\n",
+			  cont_type);
 		return false;
 	}
 
@@ -345,7 +375,7 @@ static bool verify_equivalence_table(const u8 *buf, size_t buf_size)
 	equiv_tbl_len = hdr[2];
 	if (equiv_tbl_len < sizeof(struct equiv_cpu_entry) ||
 	    buf_size < equiv_tbl_len) {
-		pr_debug("Truncated equivalence table.\n");
+		ucode_dbg("Truncated equivalence table.\n");
 		return false;
 	}
 
@@ -365,7 +395,7 @@ static bool __verify_patch_section(const u8 *buf, size_t buf_size, u32 *sh_psize
 	const u32 *hdr;
 
 	if (buf_size < SECTION_HDR_SIZE) {
-		pr_debug("Truncated patch section.\n");
+		ucode_dbg("Truncated patch section.\n");
 		return false;
 	}
 
@@ -374,13 +404,13 @@ static bool __verify_patch_section(const u8 *buf, size_t buf_size, u32 *sh_psize
 	p_size = hdr[1];
 
 	if (p_type != UCODE_UCODE_TYPE) {
-		pr_debug("Invalid type field (0x%x) in container file section header.\n",
-			 p_type);
+		ucode_dbg("Invalid type field (0x%x) in container file section header.\n",
+			  p_type);
 		return false;
 	}
 
 	if (p_size < sizeof(struct microcode_header_amd)) {
-		pr_debug("Patch of size %u too short.\n", p_size);
+		ucode_dbg("Patch of size %u too short.\n", p_size);
 		return false;
 	}
 
@@ -457,12 +487,12 @@ static int verify_patch(const u8 *buf, size_t buf_size, u32 *patch_size)
 	 * size sh_psize, as the section claims.
 	 */
 	if (buf_size < sh_psize) {
-		pr_debug("Patch of size %u truncated.\n", sh_psize);
+		ucode_dbg("Patch of size %u truncated.\n", sh_psize);
 		return -1;
 	}
 
 	if (!__verify_patch_size(sh_psize, buf_size)) {
-		pr_debug("Per-family patch size mismatch.\n");
+		ucode_dbg("Per-family patch size mismatch.\n");
 		return -1;
 	}
 
@@ -476,6 +506,9 @@ static int verify_patch(const u8 *buf, size_t buf_size, u32 *patch_size)
 
 	proc_id	= mc_hdr->processor_rev_id;
 	patch_fam = 0xf + (proc_id >> 12);
+
+	ucode_dbg("Patch-ID 0x%08x: family: 0x%x\n", mc_hdr->patch_id, patch_fam);
+
 	if (patch_fam != family)
 		return 1;
 
@@ -546,9 +579,14 @@ static size_t parse_container(u8 *ucode, size_t size, struct cont_desc *desc)
 		}
 
 		mc = (struct microcode_amd *)(buf + SECTION_HDR_SIZE);
+
+		ucode_dbg("patch_id: 0x%x\n", mc->hdr.patch_id);
+
 		if (mc_patch_matches(mc, eq_id)) {
 			desc->psize = patch_size;
 			desc->mc = mc;
+
+			ucode_dbg(" match: size: %d\n", patch_size);
 		}
 
 skip:
@@ -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;
+
 	/* verify patch application was successful */
 	*cur_rev = get_patch_level();
+
+	ucode_dbg("updated rev: 0x%x\n", *cur_rev);
+
 	if (*cur_rev != mc->hdr.patch_id)
 		return false;
 
@@ -1008,7 +1052,7 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover,
 	patch->patch_id  = mc_hdr->patch_id;
 	patch->equiv_cpu = proc_id;
 
-	pr_debug("%s: Adding patch_id: 0x%08x, proc_id: 0x%04x\n",
+	ucode_dbg("%s: Adding patch_id: 0x%08x, proc_id: 0x%04x\n",
 		 __func__, patch->patch_id, proc_id);
 
 	/* ... and add to cache. */
@@ -1151,7 +1195,7 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device)
 		snprintf(fw_name, sizeof(fw_name), "amd-ucode/microcode_amd_fam%.2xh.bin", c->x86);
 
 	if (request_firmware_direct(&fw, (const char *)fw_name, device)) {
-		pr_debug("failed to load file %s\n", fw_name);
+		ucode_dbg("failed to load file %s\n", fw_name);
 		goto out;
 	}
 
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 351a51861f00..3a4e210f6cf3 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -47,6 +47,18 @@ static bool dis_ucode_ldr;
 
 bool force_minrev = false;
 
+/*
+ * Those below should be behind CONFIG_MICROCODE_DBG ifdeffery but in
+ * order to not uglify the code with ifdeffery and use IS_ENABLED()
+ * instead, leave them in. When microcode debugging is not enabled,
+ * those are meaningless anyway.
+ */
+/* enable loader debugging */
+bool dbg;
+/* base microcode revision for debugging */
+u32 base_rev;
+u32 microcode_rev[NR_CPUS] = {};
+
 /*
  * Synchronization.
  *
@@ -118,7 +130,7 @@ bool __init microcode_loader_disabled(void)
 	 *    overwritten.
 	 */
 	if (!cpuid_feature() ||
-	    native_cpuid_ecx(1) & BIT(31) ||
+	    ((native_cpuid_ecx(1) & BIT(31)) && !dbg) ||
 	    amd_check_current_patch_level())
 		dis_ucode_ldr = true;
 
@@ -132,6 +144,17 @@ static void early_parse_cmdline(void)
 
 	if (cmdline_find_option(boot_command_line, "microcode", cmd_buf, 64) > 0) {
 		while ((s = strsep(&p, ","))) {
+			if (IS_ENABLED(CONFIG_MICROCODE_DBG)) {
+				if (!strcmp(s, "dbg"))
+					dbg = true;
+
+				if (strstr(s, "base_rev=")) {
+					/* advance to the option arg */
+					strsep(&s, "=");
+					if (kstrtouint(s, 16, &base_rev)) { ; }
+				}
+			}
+
 			if (IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV)) {
 				if (!strcmp("force_minrev", s))
 					force_minrev = true;
diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
index 50a9702ae4e2..bca806dd1aac 100644
--- a/arch/x86/kernel/cpu/microcode/internal.h
+++ b/arch/x86/kernel/cpu/microcode/internal.h
@@ -44,6 +44,10 @@ struct early_load_data {
 
 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 dbg;
+
 struct cpio_data find_microcode_in_initrd(const char *path);
 
 #define MAX_UCODE_COUNT 128
@@ -122,4 +126,10 @@ static inline void reload_ucode_intel(void) { }
 static inline struct microcode_ops *init_intel_microcode(void) { return NULL; }
 #endif  /* !CONFIG_CPU_SUP_INTEL */
 
+#define ucode_dbg(fmt, ...)					\
+({								\
+	if (dbg)						\
+		pr_info(fmt, ##__VA_ARGS__);			\
+})
+
 #endif /* _X86_MICROCODE_INTERNAL_H */
-- 
2.43.0

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

[Sohil Mehta]

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;

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

[Sohil Mehta]

On 8/9/2025 2:42 AM, Borislav Petkov wrote:

> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 7c095177da85..fc002b1a9f57 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3770,6 +3770,13 @@
>  	microcode=      [X86] Control the behavior of the microcode loader.
>  	                Available options, comma separated:
>  
> +			dbg - Format: <bool>

Since this is all new code, would it be better to use the expanded form
of dbg everywhere?

s/dbg/debug/
s/MICROCODE_DBG/MICROCODE_DEBUG
s/ucode_dbg/ucode_debug

Also, I didn't understand the "Format: <bool>".

Isn't the usage similar to force_minrev? So microcode=debug should be
enough, right?

> +			enable debugging mode when run in a guest
> +
> +			base_rev=X - with <X> with format: <u32>
> +			Set the base microcode revision of each thread when in
> +			debug mode.
> +
>  			force_minrev
>  			Enable or disable the microcode minimal revision
>   			enforcement for the runtime microcode loader.


Slight preference for a tabbed description:

	debug
		enable debugging mode when run in a guest

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

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


> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 58d890fe2100..462bf03aeda5 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1360,6 +1360,18 @@ config MICROCODE_LATE_FORCE_MINREV
>  
>  	  If unsure say Y.
>  
> +config MICROCODE_DBG
> +	bool "Enable microcode loader debugging"
> +	default n

Isn't default n redundant? I am fine with keeping it to make it obvious.

> +	depends on MICROCODE

Should we also include a dependency on DEBUG_KERNEL?

> +	help
> +	  Enable code which allows for debugging the microcode loader in
> +	  a guest. Meaning 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.
> +
> +	  You almost certainly want to say n here.
> +

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

[Borislav Petkov]

On Mon, Aug 11, 2025 at 07:26:59PM -0700, Sohil Mehta wrote:
> 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.

Because adding a new alias of a cmdline parameter doesn't belong in the same
patch.

And if anything, that thing should be

	microcode=off

to save me some silly typing.

> Unnecessary whitespace added before tab.

Pff.

> Setting this to "false" is redundant.

Fixed.

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Mon, Aug 11, 2025 at 08:04:58PM -0700, Sohil Mehta wrote:
> Since this is all new code, would it be better to use the expanded form
> of dbg everywhere?
> 
> s/dbg/debug/
> s/MICROCODE_DBG/MICROCODE_DEBUG
> s/ucode_dbg/ucode_debug

No, because I don't want to type unnecessarily and "dbg" is very clear as it
is.

> Also, I didn't understand the "Format: <bool>".

I think force_minrev was supposed to take a bool but then we changed it. And
that remained here. Whacked.

> Isn't default n redundant? I am fine with keeping it to make it obvious.

Yes, we're making it obvious.

> Should we also include a dependency on DEBUG_KERNEL?

Because?

-- 
Regards/Gruss,
    Boris.

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

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

[Sohil Mehta]

On 8/12/2025 3:26 AM, Borislav Petkov wrote:

>> s/dbg/debug/
>> s/MICROCODE_DBG/MICROCODE_DEBUG
>> s/ucode_dbg/ucode_debug
> 
> No, because I don't want to type unnecessarily and "dbg" is very clear as it
> is.
> 

Yes, in this patch, dbg clearly means debug. But, are some contexts
where dbg is used with other things which might take an extra second to
process. For example,

	if (!cpuid_feature() ||
	    ((native_cpuid_ecx(1) & BIT(31)) && !dbg) ||
	    amd_check_current_patch_level())
		dis_ucode_ldr = true;

In the tradeoff between writing and reading, I am more inclined to make
things easier to read. But obviously your preference supersedes mine :)
> 
>> Should we also include a dependency on DEBUG_KERNEL?
> 
> Because?
> 

To ensure people only use it for debugging purposes similar to
X86_DEBUG_FPU or DEBUG_ENTRY.

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

[Borislav Petkov]

On Tue, Aug 12, 2025 at 11:57:50AM -0700, Sohil Mehta wrote:
> To ensure people only use it for debugging purposes similar to
> X86_DEBUG_FPU or DEBUG_ENTRY.

No, because CONFIG_MICROCODE_DBG is a separate option and no one should enable
it. Not even DEBUG_KERNEL builds.

-- 
Regards/Gruss,
    Boris.

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

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

[Sohil Mehta]

On 8/12/2025 3:23 AM, Borislav Petkov wrote:
>> @@ -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.
> 
> Because adding a new alias of a cmdline parameter doesn't belong in the same
> patch.
> 

But, this patch introduces code to support microcode=dis_ucode_ldr which
would then become ABI.

> And if anything, that thing should be
> 
> 	microcode=off
> 

Sure, a separate patch directly adding support for microcode=off works
as well.

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

[Borislav Petkov]

On Tue, Aug 12, 2025 at 12:14:51PM -0700, Sohil Mehta wrote:
> But, this patch introduces code to support microcode=dis_ucode_ldr which
> would then become ABI.

... and supports the old cmdline argument as well.

> Sure, a separate patch directly adding support for microcode=off works
> as well.

I don't see the need for an alias atm.

-- 
Regards/Gruss,
    Boris.

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

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

[Chang S. Bae]

On 8/9/2025 2:42 AM, Borislav Petkov wrote:
>   
> -	microcode.force_minrev=	[X86]
> -			Format: <bool>
> +	microcode=      [X86] Control the behavior of the microcode loader.
> +	                Available options, comma separated:

It looks like microcode=dis_ucode_ldr is also supported.
This could be added here:
			ldr_ucode_ldr
			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.

I also noticed in arch/x86/Kconfig:

config MICROCODE_LATE_LOADING
	bool "Late microcode loading (DANGEROUS)"
	default n
	depends on MICROCODE && SMP
	help
	  ...
	  the kernel command line with "microcode.minrev=Y".

This outdated has been there already. Perhaps, it might be better to fix 
this typo with the new one while updating the option.
> -bool force_minrev = IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV);
> -module_param(force_minrev, bool, S_IRUSR | S_IWUSR);
> +bool force_minrev = false;

<snip>

> +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) {

nit: s/64/sizeof(cmd_bug)/

> +		while ((s = strsep(&p, ","))) {
> +			if (IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV)) {
> +				if (!strcmp("force_minrev", s))
> +					force_minrev = true;
> +			}

I think the behavior here differs from before:

Previously, the minrev requirement could be enforced by either
   (a) Build with MICROCODE_LATE_FORCE_MINREV=y, or
   (b) microcode.force_minrev with MICROCODE_LATE=y.

Now, this requires both. I don't know this is intentional, but it’s like 
asking for more from the user.

Thanks,
Chang

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

[Borislav Petkov]

On Thu, Aug 14, 2025 at 04:08:57PM -0700, Chang S. Bae wrote:
> It looks like microcode=dis_ucode_ldr is also supported.
> This could be added here:
> 			ldr_ucode_ldr
> 			Disable the microcode loader.		

Done.

> I also noticed in arch/x86/Kconfig:
> 
> config MICROCODE_LATE_LOADING
> 	bool "Late microcode loading (DANGEROUS)"
> 	default n
> 	depends on MICROCODE && SMP
> 	help
> 	  ...
> 	  the kernel command line with "microcode.minrev=Y".
> 
> This outdated has been there already. Perhaps, it might be better to fix
> this typo with the new one while updating the option.

Done. Good catch.

> nit: s/64/sizeof(cmd_bug)/

Done.

> I think the behavior here differs from before:
> 
> Previously, the minrev requirement could be enforced by either
>   (a) Build with MICROCODE_LATE_FORCE_MINREV=y, or
>   (b) microcode.force_minrev with MICROCODE_LATE=y.
> 
> Now, this requires both. I don't know this is intentional, but it’s like
> asking for more from the user.

Yeah, you're right. FORCE_MINREV is not a CONFIG item which enables
force_minrev support. 

Ok, here's a diff ontop with all the changes I've caught up until now:

---

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index fc002b1a9f57..e7badf2aba63 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3770,16 +3770,17 @@
 	microcode=      [X86] Control the behavior of the microcode loader.
 	                Available options, comma separated:
 
-			dbg - Format: <bool>
-			enable debugging mode when run in a guest
-
 			base_rev=X - with <X> with format: <u32>
 			Set the base microcode revision of each thread when in
 			debug mode.
 
-			force_minrev
+			dbg: enable debugging mode when run in a guest
+
+			dis_ucode_ldr: 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.
 
 	mini2440=	[ARM,HW,KNL]
 			Format:[0..2][b][c][t]
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 462bf03aeda5..77f72f075d89 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1340,7 +1340,7 @@ config MICROCODE_LATE_LOADING
 	  use this at your own risk. Late loading taints the kernel unless the
 	  microcode header indicates that it is safe for late loading via the
 	  minimal revision check. This minimal revision check can be enforced on
-	  the kernel command line with "microcode.minrev=Y".
+	  the kernel command line with "microcode=force_minrev".
 
 config MICROCODE_LATE_FORCE_MINREV
 	bool "Enforce late microcode loading minimal revision check"
@@ -1356,7 +1356,7 @@ config MICROCODE_LATE_FORCE_MINREV
 	  revision check fails.
 
 	  This minimal revision check can also be controlled via the
-	  "microcode.minrev" parameter on the kernel command line.
+	  "microcode=force_minrev" parameter on the kernel command line.
 
 	  If unsure say Y.
 
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 3a4e210f6cf3..f045670a1fae 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -45,7 +45,7 @@
 static struct microcode_ops *microcode_ops;
 static bool dis_ucode_ldr;
 
-bool force_minrev = false;
+bool force_minrev = IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV);
 
 /*
  * Those below should be behind CONFIG_MICROCODE_DBG ifdeffery but in
@@ -142,7 +142,7 @@ 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) {
+	if (cmdline_find_option(boot_command_line, "microcode", cmd_buf, sizeof(cmd_buf)) > 0) {
 		while ((s = strsep(&p, ","))) {
 			if (IS_ENABLED(CONFIG_MICROCODE_DBG)) {
 				if (!strcmp(s, "dbg"))
@@ -155,10 +155,8 @@ static void early_parse_cmdline(void)
 				}
 			}
 
-			if (IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV)) {
-				if (!strcmp("force_minrev", s))
-					force_minrev = true;
-			}
+			if (!strcmp("force_minrev", s))
+				force_minrev = true;
 
 			if (!strcmp(s, "dis_ucode_ldr"))
 				dis_ucode_ldr = true;

-- 
Regards/Gruss,
    Boris.

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

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

[Chang S. Bae]

On 8/15/2025 2:20 AM, Borislav Petkov wrote:
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index fc002b1a9f57..e7badf2aba63 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3770,16 +3770,17 @@
>   	microcode=      [X86] Control the behavior of the microcode loader.
>   	                Available options, comma separated:
>   
> -			dbg - Format: <bool>
> -			enable debugging mode when run in a guest
> -
>   			base_rev=X - with <X> with format: <u32>
>   			Set the base microcode revision of each thread when in
>   			debug mode.
>   
> -			force_minrev
> +			dbg: enable debugging mode when run in a guest
> +
> +			dis_ucode_ldr: 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.
>   
>   	mini2440=	[ARM,HW,KNL]
>   			Format:[0..2][b][c][t]
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 462bf03aeda5..77f72f075d89 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1340,7 +1340,7 @@ config MICROCODE_LATE_LOADING
>   	  use this at your own risk. Late loading taints the kernel unless the
>   	  microcode header indicates that it is safe for late loading via the
>   	  minimal revision check. This minimal revision check can be enforced on
> -	  the kernel command line with "microcode.minrev=Y".
> +	  the kernel command line with "microcode=force_minrev".
>   
>   config MICROCODE_LATE_FORCE_MINREV
>   	bool "Enforce late microcode loading minimal revision check"
> @@ -1356,7 +1356,7 @@ config MICROCODE_LATE_FORCE_MINREV
>   	  revision check fails.
>   
>   	  This minimal revision check can also be controlled via the
> -	  "microcode.minrev" parameter on the kernel command line.
> +	  "microcode=force_minrev" parameter on the kernel command line.
>   
>   	  If unsure say Y.
>   
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 3a4e210f6cf3..f045670a1fae 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -45,7 +45,7 @@
>   static struct microcode_ops *microcode_ops;
>   static bool dis_ucode_ldr;
>   
> -bool force_minrev = false;
> +bool force_minrev = IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV);
>   
>   /*
>    * Those below should be behind CONFIG_MICROCODE_DBG ifdeffery but in
> @@ -142,7 +142,7 @@ 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) {
> +	if (cmdline_find_option(boot_command_line, "microcode", cmd_buf, sizeof(cmd_buf)) > 0) {
>   		while ((s = strsep(&p, ","))) {
>   			if (IS_ENABLED(CONFIG_MICROCODE_DBG)) {
>   				if (!strcmp(s, "dbg"))
> @@ -155,10 +155,8 @@ static void early_parse_cmdline(void)
>   				}
>   			}
>   
> -			if (IS_ENABLED(CONFIG_MICROCODE_LATE_FORCE_MINREV)) {
> -				if (!strcmp("force_minrev", s))
> -					force_minrev = true;
> -			}
> +			if (!strcmp("force_minrev", s))
> +				force_minrev = true;
>   
>   			if (!strcmp(s, "dis_ucode_ldr"))
>   				dis_ucode_ldr = true;
> 

Thanks for consolidating those loader options -- this makes them easier 
to find and configure in one place.

The overall changes look good to me, so:
   Reviewed-by: Chang S. Bae <chang.seok.bae@intel.com>

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

[Sohil Mehta]

On 8/15/2025 2:20 AM, Borislav Petkov wrote:

> 
> Ok, here's a diff ontop with all the changes I've caught up until now:
> 

The updated patches look okay to me.

Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>