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/2/24 09:47, Alejandro Vallejo wrote:
> I haven't read the entire series yet, but here's my .02 so far
>
> On Fri Aug 30, 2024 at 10:46 PM BST, 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
> > boot_info structure initially containing a count of the number of boot
> > modules.
> >
> > No functional change intended.
> >
> > Signed-off-by: Christopher Clark <christopher.w.clark@xxxxxxxxx>
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >   xen/arch/x86/include/asm/bootinfo.h | 25 +++++++++++++
> >   xen/arch/x86/setup.c                | 58 +++++++++++++++++------------
> >   2 files changed, 59 insertions(+), 24 deletions(-)
> >   create mode 100644 xen/arch/x86/include/asm/bootinfo.h
> >
> > 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__
> > +
> This struct would benefit from a comment stating what it's for and how it's
> meant to be used. At a glance it seems like it's meant to be serve as a
> boot-protocol agnostic representation of boot-parameters, used as a generic
> means of information handover. Which would imply multiboot_info is parsed onto
> it when booting from multiboot and is synthesised from scratch in other cases
> (e.g: direct EFI?).
Yes, some inline documentation can be added.

> > +struct boot_info {
> > +    unsigned int nr_mods;
> It's imo better to treat this as an ABI. That would allow using this layer as a
> boot protocol in itself (which I'm guessing is the objective? I haven't gotten
> that far in the series). If so, this would need to be a fixed-width uintN_t.
>
> Same with other fields in follow-up patches.
The intent is to provide a clean internal abstraction around the boot 
material provided to Xen and not as an external boot protocol. The 
follow-on series to come will build upon this to introduce a 
representation of domains to be constructed at boot. A side goal with 
later introduced fields is to use proper xen types for the fields, eg. 
paddr_t, and mfn_t.
> > +};
> > +
> > +#endif
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > 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
> > @@ -32,6 +32,7 @@
> >   #include <compat/xen.h>
> >   #endif
> >   #include <xen/bitops.h>
> > +#include <asm/bootinfo.h>
> >   #include <asm/smp.h>
> >   #include <asm/processor.h>
> >   #include <asm/mpspec.h>
> > @@ -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)
> If this function returned boot_info instead and the caller made the
> assignment then it would be possible to unit-test/fuzz it.
>
> It also fits a bit more nicely with the usual implications of that function
> name pattern, I think.
As the larger capability continues to be developed, it is becoming 
necessary to get access to the reference from other areas of the code. 
Currently, I just move boot_info outside the function and exported the 
reference. I still have as an open question if the declaration should be 
static and access is obtained through an accessor function(s). I'm open 
to suggestions here.
> > +{
> > +    static struct boot_info __initdata info;
> > +
> > +    info.nr_mods = mbi->mods_count;
> Shouldn't this be gated on MBI_MODULES being set?
>
>     info.nr_mods = (mbi->flags & MBI_MODULES) ? mbi->mods_count : 0;
Yes.

> > +
> > +    boot_info = &info;
> > +}
> >   
> >   unsigned long __init initial_images_nrpages(nodeid_t node)
> >   {
> > @@ -285,7 +295,7 @@ unsigned long __init initial_images_nrpages(nodeid_t node)
> >       unsigned long nr;
> >       unsigned int i;
> >   
> > -    for ( nr = i = 0; i < nr_initial_images; ++i )
> > +    for ( nr = i = 0; i < boot_info->nr_mods; ++i )
> >       {
> >           unsigned long start = initial_images[i].mod_start;
> >           unsigned long end = start + PFN_UP(initial_images[i].mod_end);
> > @@ -301,7 +311,7 @@ void __init discard_initial_images(void)
> >   {
> >       unsigned int i;
> >   
> > -    for ( i = 0; i < nr_initial_images; ++i )
> > +    for ( i = 0; i < boot_info->nr_mods; ++i )
> >       {
> >           uint64_t start = (uint64_t)initial_images[i].mod_start << PAGE_SHIFT;
> >   
> > @@ -309,7 +319,7 @@ void __init discard_initial_images(void)
> >                              start + PAGE_ALIGN(initial_images[i].mod_end));
> >       }
> >   
> > -    nr_initial_images = 0;
> > +    boot_info->nr_mods = 0;
> Out of curiosity, why is this required?
At this point, the boot modules have been "consumed" and are no longer 
safe to access. This ensures that if for some reason any module access 
code gets invoked that attempts to walk the modules, this will ensure
it will result in no access.

> >       initial_images = NULL;
> >   }
> >   
> > @@ -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 newline removal?
>
> >       /* Parse the command-line options. */
> >       if ( mbi->flags & MBI_CMDLINE )
> >           cmdline = cmdline_cook(__va(mbi->cmdline), loader);
> > @@ -1141,18 +1152,18 @@ void asmlinkage __init noreturn __start_xen(unsigned 
> > long mbi_p)
> >              bootsym(boot_edd_info_nr));
> >   
> >       /* Check that we have at least one Multiboot module. */
> > -    if ( !(mbi->flags & MBI_MODULES) || (mbi->mods_count == 0) )
> > +    if ( !(mbi->flags & MBI_MODULES) || (boot_info->nr_mods == 0) )
> With MBI_MODULES accounted for during conversion, the first part of the
> conditional can be ellided and you could simply do:
>
>      if ( !boot_info->nr_mods )
>          panic(...)
This eventually happens, the goal in this revision was to slowly unhook 
the mbi usage as it is pervasive and so the reviewers can ensure we are 
correctly replacing all of its usage. With that said, I can look to see 
if it is reasonable to add its usage here.
> Also, could we move this to multiboot_to_bootinfo()? It'd contain these sorts
> of boot argument checks to a much more self contained function and help check
> at the point of assignment, preventing misuse.
These checks could be moved up into that function, but then you would 
lose the ability to print messages since the conversion function is 
invoked before serial is initialized.
> >           panic("dom0 kernel not specified. Check bootloader configuration\n");
> >   
> >       /* Check that we don't have a silly number of modules. */
> > -    if ( mbi->mods_count > sizeof(module_map) * 8 )
> > +    if ( boot_info->nr_mods > sizeof(module_map) * 8 )
> Like above, this check would be much more neatly contained where boot_info
> is created, imo.
Again, you would lose the ability to print the error message.

> >       {
> > -        mbi->mods_count = sizeof(module_map) * 8;
> > +        boot_info->nr_mods = sizeof(module_map) * 8;
> >           printk("Excessive multiboot modules - using the first %u only\n",
> Does the comment need adjusting too to make it more general? As in
> s/multiboot/boot.
I don't see why the message couldn't be generalized.

> > -               mbi->mods_count);
> > +               boot_info->nr_mods);
> >       }
> >   
> > -    bitmap_fill(module_map, mbi->mods_count);
> > +    bitmap_fill(module_map, boot_info->nr_mods);
> >       __clear_bit(0, module_map); /* Dom0 kernel is always first */
> >   
> >       if ( pvh_boot )
> Cheers,
> Alejandro
Thanks for the review.

v/r
dps