Re: [PATCH v4 01/44] x86/boot: move x86 boot module counting into a new boot_info struct

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

On 9/3/24 18:35, Andrew Cooper wrote:
> On 30/08/2024 10:46 pm, Daniel P. Smith wrote:
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index eee20bb1753c..dd94ee2e736b 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1034,9 +1044,10 @@ void asmlinkage __init noreturn __start_xen(unsigned 
> > long mbi_p)
> >           mod = __va(mbi->mods_addr);
> >       }
> >   
> > +    multiboot_to_bootinfo(mbi);
> Actually, peeking ahead to the end of the series, we've got this:
>
> void __start_xen(unsigned long mbi_p)
> {
>      ...
>      multiboot_info_t *mbi;
>      module_t *mod;
>      ...
>
>      if ( pvh_boot )
>      {
>          ASSERT(mbi_p == 0);
>          pvh_init(&mbi, &mod);
>      }
>      else
>      {
>          mbi = __va(mbi_p);
>          mod = __va(mbi->mods_addr);
>      }
>
>      multiboot_to_bootinfo(mbi, mod);
>
>
> which are the sum total of the mbi and mod pointers.  Worse, pvh_init()
> is transforming the PVH into into MB1 info, just to be transformed
> immediately to BI.
>
> I expect this is work for the end of the series (I can't think of a nice
> way to disentangle it earlier), but could we end up with something more
> like:
>
>      if ( pvh_boot )
>      {
>          ASSERT(mbi_p == 0);
>          pvh_fill_boot_info();
>      }
>      else
>      {
>          multiboot_info_t *mbi = __va(mbi_p);
>
>          multiboot_fill_boot_info(mbi, __va(mbi->mods_addr));
>      }
>
> ?
>
> Or perhaps even just pass mbi_p in, and have multiboot_fill_boot_info()
> do the __va()'s itself.
>
> If so, we probably want to make a naming and possibly prototype
> difference in this patch.
Let me combine this with some of Alejandro's comments. What if I were to 
reshape it to look like this,
/*
 * This level of indirection may not be desired, dropping it is not an
 * issue. I am proposing it because there is going to be a need to
 * access the instance from distant unit files.
 */
struct boot_info __init *get_boot_info(void)
{
    static struct boot_info __initdata info;

    return &info;
}

static struct boot_info __init *multiboot_fill_boot_info(
    unsigned long mbi_p)
{
    struct boot_info *bi = get_boot_info();
    multiboot_info_t *mbi = __va(mbi_p);
    module_t mods = __va(mbi->mods_addr);

    ...

    return bi;
}

, then in the PVH boot code

struct boot_info __init *pvh_fill_boot_info(void)
{
    struct boot_info *bi = get_boot_info();

    ...

    return bi;
}

, and then something similar for efi.

What does everyone think?

v/r,
dps