Re: [PATCH 3/5] x86/ucode: Fold early_microcode_update_cpu() into early_microcode_init()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.03.2023 21:41, Andrew Cooper wrote:
> It is not valid to retain a bootstrap_map() across returning back to
> __start_xen(), but various pointers get stashed across calls.

Same comment here, and ...

> Begin to address this by folding early_update_cache() into it's single caller,
> rearranging the exit path to always invalidate the mapping.

... this looks to lack editing after copy-and-paste from the earlier patch.

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -804,45 +804,12 @@ int __init microcode_init_cache(unsigned long 
> *module_map,
>      return rc;
>  }
>  
> -/* BSP calls this function to parse ucode blob and then apply an update. */
> -static int __init early_microcode_update_cpu(void)
> -{
> -    const void *data = NULL;
> -    size_t len;
> -    struct microcode_patch *patch;
> -
> -    if ( ucode_blob.size )
> -    {
> -        len = ucode_blob.size;
> -        data = ucode_blob.data;
> -    }
> -    else if ( ucode_mod.mod_end )
> -    {
> -        len = ucode_mod.mod_end;
> -        data = bootstrap_map(&ucode_mod);
> -    }
> -
> -    if ( !data )
> -        return -ENOMEM;

Like in the earlier patch this looks to be lost.

> -    patch = ucode_ops.cpu_request_microcode(data, len, false);
> -    if ( IS_ERR(patch) )
> -    {
> -        printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
> -               PTR_ERR(patch));
> -        return PTR_ERR(patch);
> -    }
> -
> -    if ( !patch )
> -        return -ENOENT;
> -
> -    return microcode_update_cpu(patch);
> -}
> -
>  int __init early_microcode_init(unsigned long *module_map,
>                                  const struct multiboot_info *mbi)
>  {
>      const struct cpuinfo_x86 *c = &boot_cpu_data;
> +    struct microcode_patch *patch;
> +    struct ucode_mod_blob blob = {};
>      int rc = 0;
>  
>      switch ( c->x86_vendor )
> @@ -868,8 +835,37 @@ int __init early_microcode_init(unsigned long 
> *module_map,
>  
>      ucode_ops.collect_cpu_info();
>  
> -    if ( ucode_mod.mod_end || ucode_blob.size )
> -        rc = early_microcode_update_cpu();
> +    if ( ucode_blob.data )
> +    {
> +        blob = ucode_blob;
> +    }
> +    else if ( ucode_mod.mod_end )
> +    {
> +        blob.data = bootstrap_map(&ucode_mod);
> +        blob.size = ucode_mod.mod_end;
> +    }

I wonder whether the order of if/else-if being different between
microcode_init_cache() and here (also before your change) is meaningful
in any way. I would prefer if the checking was always done in the same
order, if I can talk you into re-arranging here and/or in the earlier
patch.

Jan