Re: [PATCH 2/2] x86/ucode: load microcode earlier on boot CPU

[Jan Beulich <jbeulich@xxxxxxxx>]

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(&microcode_mutex);
> -    rc = microcode_update_cache(patch);
> -    spin_unlock(&microcode_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