On 08.12.2022 14:26, Sergey Dyasli wrote:
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -198,7 +198,7 @@ void __init microcode_scan_module(
> bootstrap_map(NULL);
> }
> }
> -void __init microcode_grab_module(
> +static void __init microcode_grab_module(
May I ask that you take the opportunity and add the missing blank line
between the two functions?
> @@ -733,9 +733,35 @@ int microcode_update_one(void)
> }
>
> /* BSP calls this function to parse ucode blob and then apply an update. */
> -static int __init early_microcode_update_cpu(void)
I think the comment wants to stay with its function. In fact I think you
simply want to move down ...
> +static int __init early_microcode_update_cache(const void *data, size_t len)
> {
... this function, which strictly is a helper of early_microcode_init_cache()
(and, being a static helper, could imo do with having a shorter name).
> @@ -754,7 +780,9 @@ static int __init early_microcode_update_cpu(void)
> if ( !data )
> return -ENOMEM;
>
> - patch = parse_blob(data, len);
> + alternative_vcall(ucode_ops.collect_cpu_info);
> +
> + patch = alternative_call(ucode_ops.cpu_request_microcode, data, len,
> false);
I realize early_microcode_init() also uses alternative_vcall(), but I
doubt that of the two altcalls here are useful to have - they merely
bloat the binary afaict. Looking at Andrew's 8f473f92e531 ("x86/ucode:
Use altcall, and __initconst_cf_clobber") I also don't see any clear
justification - the __initconst_cf_clobber alone is sufficient for the
stated purpose, I think (as far as __init functions are concerned, of
course).
> @@ -765,15 +793,28 @@ static int __init early_microcode_update_cpu(void)
> if ( !patch )
> return -ENOENT;
>
> - spin_lock(µcode_mutex);
> - rc = microcode_update_cache(patch);
> - spin_unlock(µcode_mutex);
> - ASSERT(rc);
> + return microcode_update_cpu(patch);
> +}
> +
> +int __init early_microcode_init_cache(unsigned long *module_map,
> + const multiboot_info_t *mbi)
> +{
> + int rc = 0;
> +
> + /* Need to rescan the modules because they might have been relocated */
> + microcode_grab_module(module_map, mbi);
I'm afraid the function isn't safe to call twice; the only safe case looks
to be when "ucode=scan" is in use.
> --- a/xen/arch/x86/include/asm/microcode.h
> +++ b/xen/arch/x86/include/asm/microcode.h
> @@ -3,6 +3,7 @@
>
> #include <xen/types.h>
> #include <xen/percpu.h>
> +#include <xen/multiboot.h>
I think dependencies like this are moving us in the wrong direction, wrt
our header dependencies nightmare. Could I talk you into adding a prereq
patch giving a proper tag to the struct which is typedef-ed to
multiboot_info_t, such that a forward declaration of that struct would
suffice ...
> @@ -21,7 +22,10 @@ DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
>
> void microcode_set_module(unsigned int idx);
> int microcode_update(XEN_GUEST_HANDLE(const_void), unsigned long len);
> -int early_microcode_init(void);
> +int early_microcode_init(unsigned long *module_map,
> + const multiboot_info_t *mbi);
> +int early_microcode_init_cache(unsigned long *module_map,
> + const multiboot_info_t *mbi);
... ahead of the two uses here?
Jan
