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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/01/2026 6:47 pm, Alejandro Vallejo wrote:
> On Mon Jan 12, 2026 at 6:15 PM CET, Andrew Cooper wrote:
>> On 12/01/2026 3:02 pm, Alejandro Vallejo wrote:
>>> 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.

Oh, that's subtle.

As can_load_microcode() is a local static function anyway, it might be
better to have an early return false in there.

That will get the same DCE, but be easier to follow.


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

The existing bad practice are the ones I haven't had time to fix yet.

As I recall, we did have a guest reachable BUG_ON() at one point caused
by this pattern, hence the "never again" position.


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

There's already lib stuff placed in init.  It works fine.

(What does get complicated is conditionally-init, conditionally-not, but
that's complicated irrespective of lib/)

~Andrew