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:24, Andrew Cooper wrote:
> On 30/08/2024 10:46 pm, Daniel P. Smith wrote:
> > From: Christopher Clark <christopher.w.clark@xxxxxxxxx>
> >
> > An initial step towards a non-multiboot internal representation of boot
> > modules for common code, starting with x86 setup and converting the fields
> > that are accessed for the startup calculations.
> >
> > Introduce a new header, <xen/asm/bootinfo.h>, and populate it with a new
> Just <asm/bootinfo.h>, which matches the code.
Ack.

> > diff --git a/xen/arch/x86/include/asm/bootinfo.h 
> > b/xen/arch/x86/include/asm/bootinfo.h
> > new file mode 100644
> > index 000000000000..e850f80d26a7
> > --- /dev/null
> > +++ b/xen/arch/x86/include/asm/bootinfo.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@xxxxxxxxx>
> > + * Copyright (c) 2024 Apertus Solutions, LLC
> > + * Author: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > + */
> > +
> > +#ifndef __XEN_X86_BOOTINFO_H__
> > +#define __XEN_X86_BOOTINFO_H__
> > +
> There ought to be a short description of what boot_info is, even if it's
> only "Xen's local representation of information provided by the
> bootloader/environment."
Yes, will be adding a description.

> > +struct boot_info {
> > +    unsigned int nr_mods;
> For the sake of 3 letters, please can this be nr_modules.  I've run sed
> over the top of the v5 branch and it doesn't change line wrapping
> anywhere, but it is a legibility improvement IMO.
I'm okay with that.

> > +};
> > +
> > +#endif
> #endif /* __XEN_X86_BOOTINFO_H__ */
>
> It very quickly get to not being in the same few lines as the #ifndef.
Ack.

> > 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
> > @@ -276,7 +277,16 @@ static int __init cf_check parse_acpi_param(const char *s)
> >   custom_param("acpi", parse_acpi_param);
> >   
> >   static const module_t *__initdata initial_images;
> > -static unsigned int __initdata nr_initial_images;
> > +static struct boot_info __initdata *boot_info;
> > +
> > +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi)
> > +{
> > +    static struct boot_info __initdata info;
> > +
> > +    info.nr_mods = mbi->mods_count;
> > +
> > +    boot_info = &info;
> > +}
> Having a global pointer set only to this private structure is weird.
> Even this:
>
>      static struct boot_info __initdata boot_info[1];
>
> lets you keep -> notation, but removes one level of indirection.
Further work has pushed this into being a global, at least in the 
yet-to-be public work. The question is whether the allocation should 
just move out to the unit level or use a level of in direction with an
accessor function.

> > @@ -1034,9 +1044,10 @@ void asmlinkage __init noreturn __start_xen(unsigned 
> > long mbi_p)
> >           mod = __va(mbi->mods_addr);
> >       }
> >   
> > +    multiboot_to_bootinfo(mbi);
> > +
> >       loader = (mbi->flags & MBI_LOADERNAME) ? __va(mbi->boot_loader_name)
> >                                              : "unknown";
> > -
> Stray line removal.  (should be in patch 2 to minimise churn.)
Ack.

> ~Andrew
Thanks!

v/r,
dps