Re: [PATCH v3 4/4] x86/ucode: Add Kconfig option to remove microcode loading

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Tue Jan 13, 2026 at 4:27 PM CET, Jan Beulich wrote:
> On 13.01.2026 13:21, Alejandro Vallejo wrote:
>> @@ -469,7 +471,7 @@ struct ucode_buf {
>>      char buffer[];
>>  };
>>  
>> -static long cf_check ucode_update_hcall_cont(void *data)
>> +static long cf_check __maybe_unused ucode_update_hcall_cont(void *data)
>>  {
>>      struct microcode_patch *patch = NULL;
>>      int ret, result;
>
> Why this change when ...
>
>> @@ -613,6 +615,7 @@ static long cf_check ucode_update_hcall_cont(void *data)
>>      return ret;
>>  }
>>  
>> +#ifdef CONFIG_MICROCODE_LOADING
>
> ... this can simply be moved up accordingly? After all ...
>
>>  int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
>>                         unsigned long len, unsigned int flags)
>>  {
>> @@ -645,6 +648,7 @@ int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
>>       */
>>      return continue_hypercall_on_cpu(0, ucode_update_hcall_cont, buffer);
>
> ... this is the only user of that other function.

To minimise the scope of the ifdef. It's hard to know where things start/end
when they cover several functions. This way it's (imo) clearer.

I don't mind much though.

>
>> --- a/xen/arch/x86/cpu/microcode/intel.c
>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>> @@ -408,17 +408,20 @@ static const char __initconst 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,
>> +    .cpu_request_microcode            = MICROCODE_OP(cpu_request_microcode),
>>      .collect_cpu_info                 = collect_cpu_info,
>> -    .apply_microcode                  = apply_microcode,
>> -    .compare                          = intel_compare,
>> -    .cpio_path                        = intel_cpio_path,
>> +    .apply_microcode                  = MICROCODE_OP(apply_microcode),
>> +    .compare                          = MICROCODE_OP(intel_compare),
>> +    .cpio_path                        = MICROCODE_OP(intel_cpio_path),
>>  };
>
> While I appreciate the intention with MICROCODE_OP(), I'm not really happy
> with function pointer members left in place just for them to be NULL
> everywhere. What if a call site remains unguarded? With PV guests that
> would be a privilege escalation XSA.

I see where you're coming from, but these are already NULL if microcode
loading is not exposed by the underlying hypervisor (if any), or is blocked by
hardware in Intel, so arguably that worry is orthogonal to this.

Also, only a privileged domain can perform late microcode loading, so the XSM
check would prevent any such XSA from coming to pass. dom0 crashing the system
on a bad hypercall (while wrong) would just be unfortunate, not a security
issue, as far as I can tell.

I could indeed get rid of it all. But that'd cause a sea a ifdefs anywhere the
fields are accessed, which is contrary to the current intent of relying on DCE
for readability.

Cheers,
Alejandro