Re: [PATCH 2/5] x86/ucode: Fold early_update_cache() into microcode_init_cache()

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

It's error prone, yes, but "not valid" isn't really true imo: As long as
nothing calls bootstrap_map(NULL) all mappings will remain as they are.

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -755,47 +755,51 @@ int microcode_update_one(void)
>      return microcode_update_cpu(NULL);
>  }
>  
> -static int __init early_update_cache(const void *data, size_t len)
> +int __init microcode_init_cache(unsigned long *module_map,
> +                                const struct multiboot_info *mbi)
>  {
>      int rc = 0;
>      struct microcode_patch *patch;
> +    struct ucode_mod_blob blob = {};
>  
> -    if ( !data )
> -        return -ENOMEM;

This is lost afaict. To be in sync with earlier code ) think you want to ...

> +    if ( ucode_scan )
> +        /* Need to rescan the modules because they might have been relocated 
> */
> +        microcode_scan_module(module_map, mbi);
> +
> +    if ( ucode_mod.mod_end )
> +    {
> +        blob.data = bootstrap_map(&ucode_mod);

... check here instead of ...

> +        blob.size = ucode_mod.mod_end;
> +    }
> +    else if ( ucode_blob.size )
> +    {
> +        blob = ucode_blob;
> +    }

(nit: unnecessary braces)

> -    patch = parse_blob(data, len);
> +    if ( !blob.data )
> +        return 0;

... here, making the "return 0" the "else" to the earlier if/else-if.

Alternatively, if you think the -ENOMEM isn't sensible, I'm happy to
consider respective justification for its removal.

Jan