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

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

Hi,

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

Sure

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

Fine.

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

It handles __init fine, but not "lib-y += foo.init.o", AFAICS. It may be 3 lines
worth of fix, but seeing how earlycpio.c has a single function already tagged
with __init, I'll just drop the .init.o part and let the compiler place that
single function in the right section.

Cheers,
Alejandro