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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew