Re: [PATCH v2 2/2] x86/ucode: Add Kconfig option to remove microcode loading

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/01/2026 3:02 pm, Alejandro Vallejo wrote:
>  automation/gitlab-ci/build.yaml    |  2 +-
>  docs/misc/efi.pandoc               |  2 ++
>  docs/misc/xen-command-line.pandoc  |  4 ++--
>  xen/arch/x86/Kconfig               | 14 +++++++++++++-
>  xen/arch/x86/cpu/microcode/amd.c   | 22 +++++++++++++---------
>  xen/arch/x86/cpu/microcode/core.c  | 25 ++++++++++++++++++-------
>  xen/arch/x86/cpu/microcode/intel.c | 16 +++++++++-------
>  xen/arch/x86/efi/efi-boot.h        |  3 ++-
>  xen/arch/x86/platform_hypercall.c  |  2 ++
>  xen/common/Makefile                |  3 ++-
>  10 files changed, 64 insertions(+), 29 deletions(-)

Much nicer in terms of (non) invasiveness.

First, please split the rename of CONFIG_UCODE_SCAN_DEFAULT into an
earlier patch.

> diff --git a/docs/misc/xen-command-line.pandoc 
> b/docs/misc/xen-command-line.pandoc
> index 50d7edb248..866fa2f951 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2748,7 +2748,7 @@ performance.
>  ### ucode
>  > `= List of [ <integer> | scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
>  
> -    Applicability: x86
> +    Applicability: x86 with CONFIG_MICROCODE_LOADING active
>      Default: `scan` is selectable via Kconfig, `nmi,digest-check`

You want to include this too:

diff --git a/docs/admin-guide/microcode-loading.rst 
b/docs/admin-guide/microcode-loading.rst
index a07e25802fab..91668e6f9b3f 100644
--- a/docs/admin-guide/microcode-loading.rst
+++ b/docs/admin-guide/microcode-loading.rst
@@ -23,6 +23,9 @@ TSX errata which necessitated disabling the feature 
entirely), or the addition
 of brand new features (e.g. the Spectre v2 controls to work around speculative
 execution vulnerabilities).
 
+Microcode loading support in Xen is controlled by the
+``CONFIG_MICROCODE_LOADING`` Kconfig option.
+
 
 Boot time microcode loading
 ---------------------------


while changing the docs.

>  
>  Controls for CPU microcode loading. For early loading, this parameter can
> @@ -2773,7 +2773,7 @@ microcode in the cpio name space must be:
>    - on AMD  : kernel/x86/microcode/AuthenticAMD.bin
>  When using xen.efi, the `ucode=<filename>` config file setting takes
>  precedence over `scan`. The default value for `scan` is set with
> -`CONFIG_UCODE_SCAN_DEFAULT`.
> +`CONFIG_MICROCODE_SCAN_DEFAULT`.
>  
>  'nmi' determines late loading is performed in NMI handler or just in
>  stop_machine context. In NMI handler, even NMIs are blocked, which is
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index c808c989fc..1353ec9a3f 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -331,8 +331,20 @@ config REQUIRE_NX
>         was unavailable. However, if enabled, Xen will no longer boot on
>         any CPU which is lacking NX support.
>  
> -config UCODE_SCAN_DEFAULT
> +config MICROCODE_LOADING
> +     bool "Microcode loading"
> +     default y
> +     help
> +       Support updating the microcode revision of available CPUs with a newer
> +       vendor-provided microcode blob. Microcode updates address some 
> classes of
> +       silicon defects. It's a very common delivery mechanism for fixes or
> +       workarounds for speculative execution vulnerabilities.
> +
> +       If unsure, say Y.

Please don't re-iterate the default.  It's a waste.  As to the main
text, that's not great for the target audience of a distro packager /
power user.  How about:

--8<--
Microcode updates for CPUs fix errata and provide new functionality for
software to work around bugs, such as the speculative execution
vulnerabilities.  It is common for OSes to carry updated microcode as
software tends to get updated more frequently than firmware.

For embedded environments where a full firmware/software stack is being
provided, it might not be necessary for Xen to need to load microcode
itself.
--8<--

> diff --git a/xen/arch/x86/cpu/microcode/core.c 
> b/xen/arch/x86/cpu/microcode/core.c
> index fe47c3a6c1..aec99834fd 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -44,6 +44,9 @@
>  
>  #include "private.h"
>  
> +#define can_apply_microcode(ops) (IS_ENABLED(CONFIG_MICROCODE_LOADING) && \
> +                                  (ops).apply_microcode)

I don't think this is a useful macro.  It's used 3 times, despite ...

> @@ -613,6 +618,7 @@ static long cf_check ucode_update_hcall_cont(void *data)
>      return ret;
>  }
>  
> +#ifdef CONFIG_MICROCODE_LOADING
>  int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
>                         unsigned long len, unsigned int flags)
>  {
> @@ -622,7 +628,7 @@ int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
>      if ( flags & ~XENPF_UCODE_FORCE )
>          return -EINVAL;
>  
> -    if ( !ucode_ops.apply_microcode )
> +    if ( !can_apply_microcode(ucode_ops) )
>          return -EINVAL;
>  
>      buffer = xmalloc_flex_struct(struct ucode_buf, buffer, len);
> @@ -645,6 +651,7 @@ int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
>       */
>      return continue_hypercall_on_cpu(0, ucode_update_hcall_cont, buffer);
>  }
> +#endif /* CONFIG_MICROCODE_LOADING */

... this use being redundant.  Just expand it for the two other cases
and consistently use IS_ENABLED() in view.

> @@ -907,10 +916,12 @@ int __init early_microcode_init(struct boot_info *bi)
>       *
>       * Take the hint in either case and ignore the microcode interface.
>       */
> -    if ( !ucode_ops.apply_microcode || this_cpu(cpu_sig).rev == ~0 )
> +    if ( !can_apply_microcode(ucode_ops) || this_cpu(cpu_sig).rev == ~0 )
>      {
>          printk(XENLOG_INFO "Microcode loading disabled due to: %s\n",
> -               ucode_ops.apply_microcode ? "rev = ~0" : "HW toggle");
> +               !IS_ENABLED(CONFIG_MICROCODE_LOADING) ? "not compiled in" :
> +               ucode_ops.apply_microcode             ? "rev = ~0"        :
> +                                                       "HW toggle");
>          ucode_ops.apply_microcode = NULL;
>          return -ENODEV;
>      }

Don't complicate this messy printk() further.  Just exit early; it's not
interesting to read "not loading microcode because it's not compiled in".

This is a rare example of a subsystem where it remains partially
compiled in, and it's even possible to write such a printk().

> diff --git a/xen/arch/x86/cpu/microcode/intel.c 
> b/xen/arch/x86/cpu/microcode/intel.c
> index 281993e725..d9895018b4 100644
> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -404,21 +404,23 @@ static bool __init can_load_microcode(void)
>      return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
>  }
>  
> -static const char __initconst intel_cpio_path[] =
> +static const char __initconst __maybe_unused intel_cpio_path[] =
>      "kernel/x86/microcode/GenuineIntel.bin";
>  
>  static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
> -    .cpu_request_microcode            = cpu_request_microcode,
>      .collect_cpu_info                 = collect_cpu_info,
> +#ifdef CONFIG_MICROCODE_LOADING
> +    .cpu_request_microcode            = cpu_request_microcode,
>      .apply_microcode                  = apply_microcode,
>      .compare                          = intel_compare,
>      .cpio_path                        = intel_cpio_path,
> +#endif /* CONFIG_MICROCODE_LOADING */
>  };
>  
>  void __init ucode_probe_intel(struct microcode_ops *ops)
>  {
>      *ops = intel_ucode_ops;
>  
> -    if ( !can_load_microcode() )
> +    if ( IS_ENABLED(CONFIG_MICROCODE_LOADING) && !can_load_microcode() )
>          ops->apply_microcode = NULL;
>  }

! ||, surely?


> diff --git a/xen/arch/x86/platform_hypercall.c 
> b/xen/arch/x86/platform_hypercall.c
> index 79bb99e0b6..5e83965d21 100644
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -307,6 +307,7 @@ ret_t do_platform_op(
>          break;
>      }
>  
> +#ifdef CONFIG_MICROCODE_LOADING
>      case XENPF_microcode_update:
>      {
>          XEN_GUEST_HANDLE(const_void) data;
> @@ -327,6 +328,7 @@ ret_t do_platform_op(
>                                   op->u.microcode2.flags);
>          break;
>      }
> +#endif /* CONFIG_MICROCODE_LOADING */

You mustn't #ifdef out a case like this, because it causes the op to
fall into the default case, and some of the default chains go a long way
and make unwise assumptions, like hitting a BUG().

Always use this form:

    if ( !IS_ENABLED(CONFIG_MICROCODE_LOADING) )
        // EOPNOTSUPP

and leave the case intact.

>  
>      case XENPF_platform_quirk:
>      {
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 92c97d641e..1e6c92e554 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -65,7 +65,8 @@ obj-y += wait.o
>  obj-bin-y += warning.init.o
>  obj-y += xmalloc_tlsf.o
>  
> -obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo 
> unlzo unlz4 unzstd earlycpio,$(n).init.o)
> +obj-bin-$(CONFIG_MICROCODE_LOADING) += earlycpio.init.o
> +obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo 
> unlzo unlz4 unzstd,$(n).init.o)
>  
>  obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o memory.o multicall.o 
> xlat.o)
>  

In a prereq patch, please move earlycpio out of common/ into xen/lib/. 
It shouldn't be tied to CONFIG_MICROCODE_LOADING like this, and it can
simply be discarded at link time when it's librified and unreferenced.

~Andrew