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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

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?).

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

> +};
> +
> +#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.

> +{
> +    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;

> +
> +    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?

>      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(...)

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.

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

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

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