Re: [PATCH 04/10] x86 setup: porting dom0 construction logic to boot module structures

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.07.2023 09:18, Christopher Clark wrote:
> --- a/xen/arch/x86/include/asm/boot.h
> +++ b/xen/arch/x86/include/asm/boot.h
> @@ -19,6 +19,42 @@ static inline void *bootstrap_map_multiboot(const module_t 
> *mod)
>      return bootstrap_map(&bm);
>  }
>  
> +static inline unsigned long bootmodule_index(
> +    const struct boot_info *info, bootmod_type_t bootmod_type,
> +    unsigned long start)

unsigned int?

> +{
> +    for ( ; start < info->nr_mods; start++ )
> +        if ( info->mods[start].bootmod_type == bootmod_type )
> +            return start;
> +
> +    return info->nr_mods + 1;
> +}
> +
> +static inline struct boot_module *bootmodule_next(
> +    const struct boot_info *info, bootmod_type_t bootmod_type)
> +{
> +    unsigned long i;

Again.

> +    for ( i = 0; i < info->nr_mods; i++ )
> +        if ( info->mods[i].bootmod_type == bootmod_type )
> +            return &info->mods[i];
> +
> +    return NULL;
> +}

What is "next" meant to express here? You always return the first module
of the requested type.

> +static inline void bootmodule_update_start(struct boot_module *bm,
> +    paddr_t new_start)
> +{
> +    bm->start = new_start;
> +    bm->mfn = maddr_to_mfn(new_start);
> +}
> +
> +static inline void bootmodule_update_mfn(struct boot_module *bm, mfn_t 
> new_mfn)
> +{
> +    bm->mfn = new_mfn;
> +    bm->start = mfn_to_maddr(new_mfn);
> +}

Why two functions setting the same data and overriding each other?

> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -2,9 +2,33 @@
>  #define __ARCH_X86_BOOTINFO_H__
>  
>  struct arch_bootmodule {
> +#define BOOTMOD_FLAG_X86_RELOCATED     1U << 0
> +    uint32_t flags;
>      unsigned headroom;
>  };
>  
> +struct arch_boot_info {
> +    uint32_t flags;
> +#define BOOTINFO_FLAG_X86_CMDLINE      1U << 2
> +#define BOOTINFO_FLAG_X86_MODULES      1U << 3
> +#define BOOTINFO_FLAG_X86_MEMMAP       1U << 6
> +#define BOOTINFO_FLAG_X86_LOADERNAME   1U << 9

In how far are all of these x86-specific? And are they needed at all?
Can't you internally use respective fields being NULL as good enough
indicator (looking at e.g. the places where LOADERNAME and CMDLINE
are inspected).

Also the values are all missing parentheses.

> +    char *boot_loader_name;

const char * ?

> +    uint32_t mmap_length;
> +    paddr_t mmap_addr;
> +};
> +
> +struct __packed mb_memmap {
> +    uint32_t size;
> +    uint32_t base_addr_low;
> +    uint32_t base_addr_high;
> +    uint32_t length_low;
> +    uint32_t length_high;
> +    uint32_t type;
> +};

This looks to duplicate an existing structure, without removing the
original, and without annotating both places to mention that they need
to remain in sync. Question of course is why you need a 2nd struct
definition in the first place.

> @@ -269,20 +273,44 @@ 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 struct boot_info __initdata *boot_info;
>  
> -static void __init multiboot_to_bootinfo(multiboot_info_t *mbi)
> +static void __init multiboot_to_bootinfo(multiboot_info_t *mbi, module_t 
> *mods)
>  {
>      static struct boot_info       __initdata info;
> -    static struct boot_module     __initdata boot_mods[1];
> -    static struct arch_bootmodule __initdata arch_boot_mods[1];
> +    static struct arch_boot_info  __initdata arch_info;
> +    static struct boot_module     __initdata boot_mods[MAX_NR_BOOTMODS + 1];
> +    static struct arch_bootmodule __initdata arch_boot_mods[
> +                                                       MAX_NR_BOOTMODS + 1];
>  
> +    int i;
> +
> +    info.arch = &arch_info;
>      info.mods = boot_mods;
>  
> +    info.cmdline = __va(mbi->cmdline);
> +
> +    /* The BOOTINFO_FLAG_X86_* flags are a 1-1 map to MBI_* */

I would say the comment should go in the header and here you want to
check the property via a set of BUILD_BUG_ON().

> @@ -1147,19 +1177,22 @@ void __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) || (boot_info->nr_mods == 0) )
> +    if ( !(boot_info->arch->flags & BOOTINFO_FLAG_X86_MODULES) ||
> +         (boot_info->nr_mods == 0) )
>          panic("dom0 kernel not specified. Check bootloader configuration\n");
>  
>      /* Check that we don't have a silly number of modules. */
> -    if ( boot_info->nr_mods > sizeof(module_map) * 8 )
> +    if ( boot_info->nr_mods > MAX_NR_BOOTMODS + 1 )
>      {
> -        boot_info->nr_mods = sizeof(module_map) * 8;
> +        boot_info->nr_mods = MAX_NR_BOOTMODS + 1;

This makes for a transient disconnect between module_map's size and
MAX_NR_BOOTMODS (which only so happens to be 63; it could well be
another value).

> @@ -1459,14 +1486,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>              if ( s < end &&
>                   (headroom ||
> -                  ((end - size) >> PAGE_SHIFT) > mod[j].mod_start) )
> +                  ((end - size) >> PAGE_SHIFT) > mfn_x(boot_mod[j].mfn)) )
>              {
>                  move_memory(end - size + headroom,
> -                            (uint64_t)mod[j].mod_start << PAGE_SHIFT,
> -                            mod[j].mod_end);
> -                mod[j].mod_start = (end - size) >> PAGE_SHIFT;
> -                mod[j].mod_end += headroom;
> -                mod[j].reserved = 1;
> +                            (uint64_t)boot_mod[j].start,

Another case where you no longer need a cast afaict.

> @@ -1492,13 +1520,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  #endif
>      }
>  
> -    if ( boot_info->mods[0].arch->headroom && !mod->reserved )
> +    if ( boot_info->mods[0].arch->headroom &&
> +         !(boot_info->mods[0].arch->flags & BOOTMOD_FLAG_X86_RELOCATED) )
>          panic("Not enough memory to relocate the dom0 kernel image\n");
>      for ( i = 0; i < boot_info->nr_mods; ++i )
>      {
> -        uint64_t s = (uint64_t)mod[i].mod_start << PAGE_SHIFT;
> +        uint64_t s = (uint64_t)boot_info->mods[i].start;

Yet one more.

> --- a/xen/include/xen/bootinfo.h
> +++ b/xen/include/xen/bootinfo.h
> @@ -2,23 +2,50 @@
>  #define __XEN_BOOTINFO_H__
>  
>  #include <xen/types.h>
> +#include <xen/compiler.h>
> +#include <xen/mm-frame.h>
>  
>  #ifdef CONFIG_X86
>  #include <asm/bootinfo.h>
>  #else
>      struct arch_bootmodule { };
> +    struct arch_boot_info { };
>  #endif
>  
> +/* Boot module binary type / purpose */
> +#define BOOTMOD_UNKNOWN     0
> +#define BOOTMOD_XEN         1
> +#define BOOTMOD_FDT         2
> +#define BOOTMOD_KERNEL      3
> +#define BOOTMOD_RAMDISK     4
> +#define BOOTMOD_XSM         5
> +#define BOOTMOD_UCODE       6
> +#define BOOTMOD_GUEST_DTB   7

Not all of the constants are used here (and I don't mean Arm-only ones), so
I wonder how they're reliably assigned. Without assignment ahead of use
(via e.g. bootmodule_next()) it's not entirely clear what the purpose is.

> +typedef unsigned int bootmod_type_t;
> +
> +#define BOOTMOD_STRING_MAX_LEN 1024

Is there any chance of getting away with a hard-coded upper bound? You have
...

> +struct boot_string {
> +    char bytes[BOOTMOD_STRING_MAX_LEN];
> +    size_t len;

... a size field, after all.

> +};
> +
>  struct boot_module {
> +    bootmod_type_t bootmod_type;
>      paddr_t start;
> +    mfn_t mfn;
>      size_t size;
>  
>      struct arch_bootmodule *arch;
> +    struct boot_string string;
>  };
>  
>  struct boot_info {
> +    char *cmdline;
> +
>      unsigned int nr_mods;
>      struct boot_module *mods;
> +
> +    struct arch_boot_info *arch;

Along the lines of the question on the earlier patch: Why a pointer?

Jan