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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 11/6/24 09:34, Andrew Cooper wrote:
> 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.
Most definitely.

> > ---
> >   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.
If it is considered a for loop, then I would feel obliged to add the 
spacing per the coding style.
> 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?
Yah, as much as it would be nice to use the helper, this is the 
exception where there is a complex match condition to be handled. This 
will be switched over to an explicit for loop.
> >           {
> > +            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_...
Ack.

> > 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.
Ack.

v/r,
dps