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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 11/6/24 18:06, Jason Andryuk wrote:
> 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.
Please see my response to Andy's response.


> > @@ -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?
Ugh, correct. Regression, not sure why, from previous review.

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

v/r,
dps