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

[Alejandro Vallejo <alejandro.garciavallejo@amd.com>]

On Mon Jan 12, 2026 at 6:15 PM CET, Andrew Cooper wrote:
> 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.

Sure

>
>> 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.

Ack

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

Sure. I don't mind.

>
>> 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.

Ack.

>
>> @@ -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().

Ack.

>
>> 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?

When !CONFIG_MICROCODE_LOADING apply_microcode is already NULL. It's a needless
assignment. This is strictly so the compiler can avoid assigning anything.

Functionally it's irrelevant.

>
>
>> 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().

It's normally more convenient for us (AMD) to physically remove code where
possible for coverage reasons, but in this case it probably doesn't matter.

That said, I think we can both agree if dom0 can crash the hypervisor requesting
a non existing op the bug is probably in such a BUG() statement and not
elsewhere. Note CONFIG_VIDEO already removes an op in this way in this very
file. The default case returns with ENOSYS, with BUG() being in helpers for
other data, as far as I can see.

>
> Always use this form:
>
>     if ( !IS_ENABLED(CONFIG_MICROCODE_LOADING) )
>         // EOPNOTSUPP
>
> and leave the case intact.

... but sure.

>
>>  
>>      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

That would preclude having it in the init section though, AIUI.

Cheers,
Alejandro