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
