Re: [PATCH 08/12] x86/boot: convert domain construction to use boot info

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-11-02 13:25, Daniel P. Smith wrote:
> With all the components used to construct dom0 encapsulated in struct boot_info
> and struct boot_module, it is no longer necessary to pass all them as
> parameters down the domain construction call chain. Change the parameter list
> to pass the struct boot_info instance and the struct domain reference.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> ---
> Changes since v5:
> - renamed from "x86/boot: convert create_dom0 to use boot info"
>
> Changes since v5:
> - change headroom back to unsigned long
> - make mod_idx unsigned int
> ---
>   xen/arch/x86/dom0_build.c             |  9 ++--
>   xen/arch/x86/hvm/dom0_build.c         | 49 +++++++++++++---------
>   xen/arch/x86/include/asm/dom0_build.h | 13 ++----
>   xen/arch/x86/include/asm/setup.h      |  7 ++--
>   xen/arch/x86/pv/dom0_build.c          | 59 ++++++++++++++++-----------
>   xen/arch/x86/setup.c                  | 33 ++++++++-------
>   6 files changed, 95 insertions(+), 75 deletions(-)
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index a4ac262db463..cd97f94a168a 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -1301,16 +1302,25 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain 
> *d)
>       }
>   }
>   
> -int __init dom0_construct_pvh(struct domain *d, const module_t *image,
> -                              unsigned long image_headroom,
> -                              module_t *initrd,
> -                              const char *cmdline)
> +int __init dom0_construct_pvh(struct boot_info *bi, struct domain *d)
>   {
>       paddr_t entry, start_info;
> +    struct boot_module *image;
> +    struct boot_module *initrd = NULL;
>       int rc;
>   
>       printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
>   
> +    rc = first_boot_module_index(bi, BOOTMOD_KERNEL);
> +    if ( unlikely(rc < 0 || rc > bi->nr_modules) )
Here and ...

> +        panic("Missing kernel boot module for %pd construction\n", d);
> +
> +    image = &bi->mods[rc];
> +
> +    rc = first_boot_module_index(bi, BOOTMOD_RAMDISK);
> +    if ( rc > 0 || rc < bi->nr_modules )
... here.  Can we just check rc < bi->nr_modules for validity?  Valid 
modules are 0...nr_modules and not found is MAX_NR_BOOTMODS + 1.  It 
eliminates these unecessary double checks.  This would apply to 04/12 
"x86/boot: introduce module release" as well.
> +        initrd = &bi->mods[rc];
> +
>       if ( is_hardware_domain(d) )
>       {
>           /*

> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index c1f44502a1ac..594874cd8d85 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -374,10 +371,13 @@ static int __init dom0_construct(struct domain *d,
>       unsigned int flush_flags = 0;
>       start_info_t *si;
>       struct vcpu *v = d->vcpu[0];
> -    void *image_base = bootstrap_map(image);
> -    unsigned long image_len = image->mod_end;
> -    void *image_start = image_base + image_headroom;
> -    unsigned long initrd_len = initrd ? initrd->mod_end : 0;
> +    struct boot_module *image;
> +    struct boot_module *initrd = NULL;
> +    void *image_base;
> +    unsigned long image_len;
> +    void *image_start;
> +    unsigned long initrd_len = 0;
> +    const char *cmdline;
>       l4_pgentry_t *l4tab = NULL, *l4start = NULL;
>       l3_pgentry_t *l3tab = NULL, *l3start = NULL;
>       l2_pgentry_t *l2tab = NULL, *l2start = NULL;
> @@ -414,6 +414,23 @@ static int __init dom0_construct(struct domain *d,
>   
>       printk(XENLOG_INFO "*** Building a PV Dom%d ***\n", d->domain_id);
>   
> +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
> +    if ( unlikely(i < 0 || i > bi->nr_modules) )
Single check here.

> +        panic("Missing kernel boot module for %pd construction\n", d);
> +
> +    image = &bi->mods[i];
> +    image_base = bootstrap_map_bm(image);
> +    image_len = image->mod->mod_end;
> +    image_start = image_base + image->headroom;
> +    cmdline = __va(image->cmdline_pa);
> +
> +    i = first_boot_module_index(bi, BOOTMOD_RAMDISK);
> +    if ( i > 0 || i < bi->nr_modules )
> +    {
> +        initrd = &bi->mods[i];
> +        initrd_len = initrd->mod->mod_end;
> +    }
> +
>       d->max_pages = ~0U;
>   
>       if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 )
> @@ -613,7 +630,7 @@ static int __init dom0_construct(struct domain *d,
>           initrd_pfn = vinitrd_start ?
>                        (vinitrd_start - v_start) >> PAGE_SHIFT :
>                        domain_tot_pages(d);
> -        initrd_mfn = mfn = initrd->mod_start;
> +        initrd_mfn = mfn = initrd->mod->mod_start;
MISRA doesn't like these assignment chains?

>           count = PFN_UP(initrd_len);
>           if ( d->arch.physaddr_bitsize &&
>                ((mfn + count - 1) >> (d->arch.physaddr_bitsize - PAGE_SHIFT)) )
> @@ -628,17 +645,17 @@ static int __init dom0_construct(struct domain *d,
>                       free_domheap_pages(page, order);
>                       page += 1UL << order;
>                   }
> -            memcpy(page_to_virt(page), mfn_to_virt(initrd->mod_start),
> +            memcpy(page_to_virt(page), mfn_to_virt(initrd->mod->mod_start),
>                      initrd_len);
> -            release_module(initrd, true);
> -            initrd->mod_start = initrd_mfn = mfn_x(page_to_mfn(page));
> +            release_boot_module(initrd, true);
> +            initrd->mod->mod_start = initrd_mfn = mfn_x(page_to_mfn(page));
Assignment chain here.

>           }
>           else
>           {
>               while ( count-- )
>                   if ( assign_pages(mfn_to_page(_mfn(mfn++)), 1, d, 0) )
>                       BUG();
> -            release_module(initrd, false);
> +            release_boot_module(initrd, false);
>           }
>   
>           iommu_memory_setup(d, "initrd", mfn_to_page(_mfn(initrd_mfn)),
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index aba9df8620ef..d9785acf89b6 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -977,10 +977,7 @@ static unsigned int __init copy_bios_e820(struct e820entry 
> *map, unsigned int li
>       return n;
>   }
>   
> -static struct domain *__init create_dom0(const module_t *image,
> -                                         unsigned long headroom,
> -                                         module_t *initrd, const char *kextra,
> -                                         const char *loader)
> +static struct domain *__init create_dom0(struct boot_info *bi)
>   {
>       static char __initdata cmdline[MAX_GUEST_CMDLINE];
>   
> @@ -997,6 +994,14 @@ static struct domain *__init create_dom0(const module_t *image,
>       };
>       struct domain *d;
>       domid_t domid;
> +    struct boot_module *image;
> +    unsigned int idx;
> +
> +    idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
> +    if ( unlikely(idx < 0 || idx > bi->nr_modules) )
Single check here please.

Regards,
Jason

> +        panic("Missing kernel boot module for building Dom0\n");
> +
> +    image = &bi->mods[idx];
>   
>       if ( opt_dom0_pvh )
>       {