Re: [PATCH v5 17/44] x86/boot: convert microcode loading to consume struct boot_info

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-10-06 17:49, Daniel P. Smith wrote:
> Convert the microcode loading functions to take struct boot_info, and then
> using struct boot_module to map and check for microcode. To keep the changes
> focused, continue using the struct mod to hold the reference to the microcode
> that is used by the late microcode logic.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> ---
>   xen/arch/x86/cpu/microcode/core.c    | 37 +++++++++++++---------------
>   xen/arch/x86/include/asm/bootinfo.h  |  1 +
>   xen/arch/x86/include/asm/microcode.h | 14 ++++++-----
>   xen/arch/x86/setup.c                 |  4 +--
>   4 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/microcode/core.c 
> b/xen/arch/x86/cpu/microcode/core.c
> index 8564e4d2c94c..22fea80bc97e 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -205,20 +204,18 @@ static void __init microcode_scan_module(
>   }
>   
>   static void __init microcode_grab_module(
> -    unsigned long *module_map,
> -    const multiboot_info_t *mbi)
> +    unsigned long *module_map, struct boot_info *bi)
>   {
> -    module_t *mod = (module_t *)__va(mbi->mods_addr);
> -
>       if ( ucode_mod_idx < 0 )
> -        ucode_mod_idx += mbi->mods_count;
> -    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= mbi->mods_count ||
> +        ucode_mod_idx += bi->nr_modules;
> +    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
>            !__test_and_clear_bit(ucode_mod_idx, module_map) )
>           goto scan;
> -    ucode_mod = mod[ucode_mod_idx];
> +    bi->mods[ucode_mod_idx].type = BOOTMOD_MICROCODE;
> +    ucode_mod = *bi->mods[ucode_mod_idx].mod;
Why the dereference: *bi->mods[ucode_mod_idx].mod; ?  I don't think it 
should be there.
>   scan:
>       if ( ucode_scan )
> -        microcode_scan_module(module_map, mbi);
> +        microcode_scan_module(module_map, bi);
>   }
>   
>   static struct microcode_ops __ro_after_init ucode_ops;
> @@ -822,8 +819,8 @@ static int __init early_update_cache(const void *data, 
> size_t len)
>       return rc;
>   }
>   
> -int __init microcode_init_cache(unsigned long *module_map,
> -                                const struct multiboot_info *mbi)
> +int __init microcode_init_cache(
> +    unsigned long *module_map, const struct boot_info *bi)
Why the indent style change?  I prefer the original indent and it 
doesn't seem like the line length matters.  And it looks like you 
restore the indent later.
>   {
>       int rc = 0;
>   
> diff --git a/xen/arch/x86/include/asm/microcode.h 
> b/xen/arch/x86/include/asm/microcode.h
> index 57c08205d475..495c8f7a7cc5 100644
> --- a/xen/arch/x86/include/asm/microcode.h
> +++ b/xen/arch/x86/include/asm/microcode.h
> @@ -4,6 +4,8 @@
>   #include <xen/types.h>
>   #include <xen/percpu.h>
>   
> +#include <asm/bootinfo.h>
> +
>   #include <public/xen.h>
>   
>   struct multiboot_info;
> @@ -22,12 +24,12 @@ struct cpu_signature {
>   DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
>   
>   void microcode_set_module(unsigned int idx);
> -int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
> -                     unsigned long len, unsigned int flags);
> -int early_microcode_init(unsigned long *module_map,
> -                         const struct multiboot_info *mbi);
> -int microcode_init_cache(unsigned long *module_map,
> -                         const struct multiboot_info *mbi);
> +int microcode_update(
> +    XEN_GUEST_HANDLE(const_void) buf, unsigned long len, unsigned int flags);
> +int early_microcode_init(
> +    unsigned long *module_map, struct boot_info *bi);
> +int microcode_init_cache(
> +    unsigned long *module_map, const struct boot_info *bi);
More indent churn.  The diff would be smaller without it.

>   int microcode_update_one(void);
>   
>   #endif /* ASM_X86__MICROCODE_H */