Re: [PATCH 02/12] x86/boot: eliminate module_map

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/11/2024 5:25 pm, Daniel P. Smith wrote:
> With all boot modules now labeled by type, it is no longer necessary to
> track whether a boot module was identified via the module_map bitmap.
>
> Introduce a set of helpers to search the list of boot modules based on type 
> and
> the reference type, pointer or array index, desired. Then drop all uses of
> setting a bit in module_map and replace its use for looping with the helpers.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> ---
> Changes since v7:
> - collapse the three module_map patches into one,
>   - x86/boot: remove module_map usage from microcode loading
>   - x86/boot: remove module_map usage from xsm policy loading
>   - x86/boot: remove module_map usage by ramdisk loading

Definitely nicer for having been collapsed together.

> ---
>  xen/arch/x86/cpu/microcode/core.c   | 12 ++++-----
>  xen/arch/x86/include/asm/bootinfo.h | 42 ++++++++++++++++++++++++++++-
>  xen/arch/x86/setup.c                | 28 +++++++++++--------
>  xen/xsm/xsm_policy.c                | 19 +++++--------
>  4 files changed, 70 insertions(+), 31 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/microcode/core.c 
> b/xen/arch/x86/cpu/microcode/core.c
> index f46464241557..b09cf83249f6 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -790,15 +790,13 @@ static int __init early_microcode_load(struct boot_info 
> *bi)
>  
>      if ( opt_scan ) /* Scan for a CPIO archive */
>      {
> -        for ( idx = 1; idx < bi->nr_modules; ++idx )
> +        for_each_boot_module_by_type(idx, bi, BOOTMOD_UNKNOWN)

Minor, but we treat for_each_* as if they were for loops, so this either
wants to be

for_each_boot_module_by_type ( idx, bi, BOOTMOD_UNKNOWN )

or

for_each_boot_module_by_type (idx, bi, BOOTMOD_UNKNOWN)

spacing wise.  There's no agreement between maintainers on the extra
spaces inside brackets or not.


However, despite looking at this many times, I've only just realised... 
This semantically changes things in a direction that we won't want.

Today, BOOTMOD_RAMDISK only happens a side effect of being "first
BOOTMOD_UNKNOWN standing at the end".

But the EFI boot code ought to set bi->type=RAMDISK explicitly from the
ramdisk= argument (it can probably set type=MICROCODE too), and future
plans with a large HL config probably will be similar.

Anything which sets type=, and type=RAMDISK in particular, prior to
early_microcode_load() excludes it from the search.  This is definitely
not what we want.


It's a latent bug for now, but I'd suggest keeping the plain for loop, with

            /* Search anything unclaimed or likely to be a CPIO archive. */
            if ( bm->type != BOOTMOD_UNKNOWN &&
                 bm->type != BOOTMOD_RAMDISK )
                continue;

as the selection criteria.  Probably also want to start from idx=0 to
remove assumptions about the dom0 kernel.

Thoughts?



>          {
> +            struct boot_module *bm = &bi->mods[idx];
>              struct cpio_data cd;
>  
> -            if ( !test_bit(idx, bi->module_map) )
> -                continue;
> -
> -            size = bi->mods[idx].mod->mod_end;
> -            data = bootstrap_map_bm(&bi->mods[idx]);
> +            size = bm->mod->mod_end;
> +            data = bootstrap_map_bm(bm);
>              if ( !data )
>              {
>                  printk(XENLOG_WARNING "Microcode: Could not map module %d, 
> size %zu\n",
> @@ -840,7 +838,7 @@ static int __init early_microcode_load(struct boot_info 
> *bi)
>              return -ENODEV;
>          }
>  
> -        if ( !__test_and_clear_bit(idx, bi->module_map) )
> +        if ( bi->mods[idx].type != BOOTMOD_UNKNOWN )
>          {
>              printk(XENLOG_WARNING "Microcode: Chosen module %d already 
> used\n", idx);
>              return -ENODEV;
> diff --git a/xen/arch/x86/include/asm/bootinfo.h 
> b/xen/arch/x86/include/asm/bootinfo.h
> index fc74e3b224e7..37dfcc14fa7d 100644
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -43,10 +43,50 @@ struct boot_info {
>      size_t memmap_length;
>  
>      unsigned int nr_modules;
> -    unsigned long *module_map; /* Temporary */
>      struct boot_module mods[MAX_NR_BOOTMODS + 1];
>  };
>  
> +/*
> + * next_boot_module_index:
> + *     Finds the next boot module of type t, starting at array index start.
> + *
> + * Returns:
> + *      Success - index in boot_module array
> + *      Failure - a value greater than MAX_NR_BOOTMODS
> + */
> +static inline unsigned int __init next_boot_module_index(
> +    const struct boot_info *bi, enum bootmod_type t, unsigned int start)
> +{
> +    unsigned int i;
> +
> +    if ( t == BOOTMOD_XEN )
> +        return bi->nr_modules;
> +
> +    for ( i = start; i < bi->nr_modules; i++ )
> +    {
> +        if ( bi->mods[i].type == t )
> +            return i;
> +    }
> +
> +    return MAX_NR_BOOTMODS + 1;
> +}
> +
> +/*
> + * first_boot_module_index:
> + *     Finds the first boot module of type t.
> + *
> + * Returns:
> + *      Success - index in boot_module array
> + *      Failure - a value greater than MAX_NR_BOOTMODS
> + */
> +#define first_boot_module_index(bi, t)              \
> +    next_boot_module_index(bi, t, 0)
> +
> +#define for_each_boot_module_by_type(i, b, t)       \
> +    for ( i = first_boot_module_index(b, t);        \
> +          i <= (b)->nr_modules;                     \
> +          i = next_boot_module_index(b, t, i + 1) )

(i) = first_...


> diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c
> index 4c195411d05b..12c9de5a1fbf 100644
> --- a/xen/xsm/xsm_policy.c
> +++ b/xen/xsm/xsm_policy.c
> @@ -33,22 +33,18 @@
>  int __init xsm_multiboot_policy_init(
>      struct boot_info *bi, void **policy_buffer, size_t *policy_size)
>  {
> -    int i;
> +    unsigned int i;
>      int rc = 0;
>      u32 *_policy_start;
>      unsigned long _policy_len;
>  
> -    /*
> -     * Try all modules and see whichever could be the binary policy.
> -     * Adjust module_map for the module that is the binary policy.
> -     */
> -    for ( i = bi->nr_modules - 1; i >= 1; i-- )
> +    /* Try all unknown modules and see whichever could be the binary policy. 
> */
> +    for_each_boot_module_by_type(i, bi, BOOTMOD_UNKNOWN)
>      {
> -        if ( !test_bit(i, bi->module_map) )
> -            continue;
> +        struct boot_module *bm = &bi->mods[i];
>  
> -        _policy_start = bootstrap_map(bi->mods[i].mod);
> -        _policy_len   = bi->mods[i].mod->mod_end;
> +        _policy_start = bootstrap_map(bm->mod);
> +        _policy_len   = bm->mod->mod_end;

Minor, but you ought to switch to bootstrap_map_bm() here straight away,
which reduces the churn in patch 9.

~Andrew