Re: [PATCH 06/10] x86 setup, microcode: switch to the new bootinfo structures

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.07.2023 09:18, Christopher Clark wrote:
> Next step in incremental work towards adding a non-multiboot internal
> representation of boot modules, converting the fields being accessed for
> the startup calculations.
> 
> Move the per-module scan logic into a dedicated function from the
> iteration loop and set the BOOTMOD_UCODE module type when microcode is found.

It's not really clear to me why the split (and the associated code churn)
is needed.

> @@ -150,75 +149,109 @@ static int __init cf_check parse_ucode(const char *s)
>  }
>  custom_param("ucode", parse_ucode);
>  
> -void __init microcode_scan_module(
> -    unsigned long *module_map,
> -    const multiboot_info_t *mbi)
> +#define MICROCODE_MODULE_MATCH 1
> +#define MICROCODE_MODULE_NONMATCH 0
> +
> +static int __init microcode_check_module(struct boot_module *mod)
>  {
> -    module_t *mod = (module_t *)__va(mbi->mods_addr);
>      uint64_t *_blob_start;
>      unsigned long _blob_size;
> -    struct cpio_data cd;
> +    struct cpio_data cd = { NULL, 0 };

Why? You don't ...

>      long offset;
>      const char *p = NULL;
> -    int i;
> -
> -    ucode_blob.size = 0;
> -    if ( !ucode_scan )
> -        return;
>  
>      if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
>          p = "kernel/x86/microcode/AuthenticAMD.bin";
>      else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
>          p = "kernel/x86/microcode/GenuineIntel.bin";
>      else
> +        return -EFAULT;
> +
> +    _blob_start = bootstrap_map(mod);
> +    _blob_size = mod->size;
> +    if ( !_blob_start )
> +    {
> +        printk("Could not map multiboot module (0x%lx) (size: %ld)\n",
> +               mod->start, _blob_size);
> +        /* Non-fatal error, so just say no match */
> +        return MICROCODE_MODULE_NONMATCH;
> +    }
> +
> +    cd = find_cpio_data(p, _blob_start, _blob_size, &offset /* ignore */);

... use the variable ahead of this assignment.

> +    if ( cd.data )
> +    {
> +        ucode_blob.size = cd.size;
> +        ucode_blob.data = cd.data;
> +
> +        mod->bootmod_type = BOOTMOD_UCODE;
> +        return MICROCODE_MODULE_MATCH;
> +    }
> +
> +    bootstrap_map(NULL);
> +
> +    return MICROCODE_MODULE_NONMATCH;
> +}
> +
> +void __init microcode_scan_module(struct boot_info *bootinfo)
> +{
> +    int i;

No plain int please for variables holding only non-negative values.

> +    if ( !ucode_scan )
>          return;
>  
> -    /*
> -     * Try all modules and see whichever could be the microcode blob.
> -     */
> -    for ( i = 1 /* Ignore dom0 kernel */; i < mbi->mods_count; i++ )
> +    i = bootmodule_index(bootinfo, BOOTMOD_UNKNOWN, 0);
> +    while ( i < bootinfo->nr_mods )

Why is the comment going away?

>      {
> -        if ( !test_bit(i, module_map) )
> -            continue;
> +        int ret = microcode_check_module(&bootinfo->mods[i]);
>  
> -        _blob_start = bootstrap_map_multiboot(&mod[i]);
> -        _blob_size = mod[i].mod_end;
> -        if ( !_blob_start )
> +        switch ( ret )
>          {
> -            printk("Could not map multiboot module #%d (size: %ld)\n",
> -                   i, _blob_size);
> +        case MICROCODE_MODULE_MATCH:
> +            return;
> +        case MICROCODE_MODULE_NONMATCH:
> +            i = bootmodule_index(bootinfo, BOOTMOD_UNKNOWN, ++i);
>              continue;
> +        default:
> +            printk("%s: (err: %d) unable to check microcode\n",
> +                   __func__, ret);
> +            return;
>          }
> -        cd.data = NULL;
> -        cd.size = 0;
> -        cd = find_cpio_data(p, _blob_start, _blob_size, &offset /* ignore 
> */);
> -        if ( cd.data )
> -        {
> -            ucode_blob.size = cd.size;
> -            ucode_blob.data = cd.data;
> -            break;
> -        }
> -        bootstrap_map(NULL);
>      }
>  }
>  
> -static void __init microcode_grab_module(
> -    unsigned long *module_map,
> -    const multiboot_info_t *mbi)
> +static void __init microcode_grab_module(struct boot_info *bootinfo)
>  {
> -    module_t *mod = (module_t *)__va(mbi->mods_addr);
> +    ucode_blob.size = 0;
>  
>      if ( ucode_mod_idx < 0 )
> -        ucode_mod_idx += mbi->mods_count;
> -    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= mbi->mods_count ||
> -         !__test_and_clear_bit(ucode_mod_idx, module_map) )
> -        goto scan;
> -    ucode_mod = mod[ucode_mod_idx];
> -scan:
> +        ucode_mod_idx += bootinfo->nr_mods;
> +    if ( ucode_mod_idx >= 0 &&  ucode_mod_idx <= bootinfo->nr_mods &&
> +         bootinfo->mods[ucode_mod_idx].bootmod_type == BOOTMOD_UNKNOWN )
> +    {
> +        int ret = microcode_check_module(&bootinfo->mods[ucode_mod_idx]);
> +
> +        switch ( ret )
> +        {
> +        case MICROCODE_MODULE_MATCH:
> +            return;
> +        case MICROCODE_MODULE_NONMATCH:
> +            break;
> +        default:
> +            printk("%s: (err: %d) unable to check microcode\n",
> +                   __func__, ret);
> +            return;
> +        }
> +    }
> +
>      if ( ucode_scan )
> -        microcode_scan_module(module_map, mbi);
> +        microcode_scan_module(bootinfo);
>  }
>  
> +/* Undefining as they are not needed anymore */
> +#undef MICROCODE_MODULE_MATCH
> +#undef MICROCODE_MODULE_NONMATCH

No need to comment this; we do so elsewhere as well.

> @@ -738,11 +771,6 @@ static int __init cf_check microcode_init(void)
>          ucode_blob.size = 0;
>          ucode_blob.data = NULL;
>      }
> -    else if ( ucode_mod.mod_end )
> -    {
> -        bootstrap_map(NULL);
> -        ucode_mod.mod_end = 0;
> -    }

I can spot why this and ...

> @@ -786,8 +814,7 @@ 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(struct boot_info *bootinfo)
>  {
>      int rc = 0;
>  
> @@ -796,12 +823,9 @@ int __init microcode_init_cache(unsigned long 
> *module_map,
>  
>      if ( ucode_scan )
>          /* Need to rescan the modules because they might have been relocated 
> */
> -        microcode_scan_module(module_map, mbi);
> +        microcode_scan_module(bootinfo);
>  
> -    if ( ucode_mod.mod_end )
> -        rc = early_update_cache(bootstrap_map_multiboot(&ucode_mod),
> -                                ucode_mod.mod_end);
> -    else if ( ucode_blob.size )
> +    if ( ucode_blob.size )
>          rc = early_update_cache(ucode_blob.data, ucode_blob.size);
>  
>      return rc;
> @@ -819,11 +843,6 @@ static int __init early_microcode_update_cpu(void)
>          len = ucode_blob.size;
>          data = ucode_blob.data;
>      }
> -    else if ( ucode_mod.mod_end )
> -    {
> -        len = ucode_mod.mod_end;
> -        data = bootstrap_map_multiboot(&ucode_mod);
> -    }

... this isn't needed anymore. The code doesn't appear to move elsewhere.

Jan