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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 30/08/2024 10:46 pm, Daniel P. Smith wrote:
> From: Christopher Clark <christopher.w.clark@gmail.com>
>
> 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.

> 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@gmail.com>
> + * Copyright (c) 2024 Apertus Solutions, LLC
> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com>
> + */
> +
> +#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."

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

> +};
> +
> +#endif

#endif /* __XEN_X86_BOOTINFO_H__ */

It very quickly get to not being in the same few lines as the #ifndef.

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

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

~Andrew