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
