Re: [PATCH 1/2] x86/ucode: Move vendor specifics back out of early_microcode_init()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.10.2023 20:06, Andrew Cooper wrote:
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -847,25 +847,19 @@ int __init early_microcode_init(unsigned long 
> *module_map,
>  {
>      const struct cpuinfo_x86 *c = &boot_cpu_data;
>      int rc = 0;
> -    bool can_load = false;
>  
>      switch ( c->x86_vendor )
>      {
>      case X86_VENDOR_AMD:
> -        if ( c->x86 >= 0x10 )
> -        {
> -            ucode_ops = amd_ucode_ops;
> -            can_load = true;
> -        }
> +        ucode_probe_amd(&ucode_ops);
>          break;
>  
>      case X86_VENDOR_INTEL:
> -        ucode_ops = intel_ucode_ops;
> -        can_load = intel_can_load_microcode();
> +        ucode_probe_intel(&ucode_ops);
>          break;
>      }
>  
> -    if ( !ucode_ops.apply_microcode )
> +    if ( !ucode_ops.collect_cpu_info )
>      {
>          printk(XENLOG_INFO "Microcode loading not available\n");
>          return -ENODEV;
> @@ -882,10 +876,10 @@ int __init early_microcode_init(unsigned long 
> *module_map,
>       *
>       * Take the hint in either case and ignore the microcode interface.
>       */
> -    if ( this_cpu(cpu_sig).rev == ~0 || !can_load )
> +    if ( !ucode_ops.apply_microcode || this_cpu(cpu_sig).rev == ~0 )

Here ucode_ops.apply_microcode simply replaces can_load, as expected.

>      {
>          printk(XENLOG_INFO "Microcode loading disabled due to: %s\n",
> -               can_load ? "rev = ~0" : "HW toggle");
> +               ucode_ops.apply_microcode ? "HW toggle" : "rev = ~0");

Here, otoh, you invert sense, which I don't think is right. With 2nd
and 3rd operands swapped back
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> @@ -398,9 +398,17 @@ bool __init intel_can_load_microcode(void)
>      return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD);
>  }
>  
> -const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
> +static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = {
>      .cpu_request_microcode            = cpu_request_microcode,
>      .collect_cpu_info                 = collect_cpu_info,
>      .apply_microcode                  = apply_microcode,
>      .compare_patch                    = compare_patch,
>  };
> +
> +void __init ucode_probe_intel(struct microcode_ops *ops)
> +{
> +    *ops = intel_ucode_ops;
> +
> +    if ( !can_load_microcode() )
> +        ops->apply_microcode = NULL;
> +}

I was wondering why you didn't use the return value to supply the pointer
to the caller, but this override explains it.

Jan