Re: [PATCH v9 1/6] x86/boot: convert domain construction to use boot info

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-11-15 12:01, Andrew Cooper wrote:
> On 15/11/2024 4:33 pm, Jason Andryuk wrote:
> > On 2024-11-15 08:11, Daniel P. Smith wrote:
> > > diff --git a/xen/arch/x86/hvm/dom0_build.c
> > > b/xen/arch/x86/hvm/dom0_build.c
> > > index 3dd913bdb029..d1bdf1b14601 100644
> > > --- a/xen/arch/x86/hvm/dom0_build.c
> > > +++ b/xen/arch/x86/hvm/dom0_build.c
> > > @@ -1300,16 +1301,26 @@ 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;
> > > +    unsigned int idx;
> > >        int rc;
> > >          printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n",
> > > d->domain_id);
> > >    +    idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
> > > +    if ( idx >= bi->nr_modules )
> > What do you think about introducing a new define:
> >
> >      #define BOOTMOD_NOT_FOUND (MAX_NR_BOOTMODS + 1)
> >
> > For first_boot_module_index() to return.  And then:
> >
> >      if ( idx == BOOTMOD_NOT_FOUND )
> >
> > ?
> Care would need to be taken vs BOOTMOD_XEN, which could have the same
> numeric value in a big HL configuration.
It's a little subtle that BOOTMOD_XEN could be at MAX_NR_BOOTMODS + 1, 
and first_boot_module_index() will return that for "not found".  Which I 
overlooked when making the suggestion.
>  From a "reading the code" point of view, a range check against any
> invalid value is better seeing as the next thing we do is index an
> array, so I'm marginally on the side of "keep it as it is".
first_boot_module_index() is looking for a specific BOOTMOD_*, so I 
thought it would be a little safer to just return either a valid index 
or BOOTMOD_NOT_FOUND (which might have to become ~0).
> This particular logic can't trip because of earlier checks in
> __start_xen(), and gets rewritten in patch 4 in the conversion to
> boot_domains, so I'm also not overly fussed at extra polish on this
> specific piece of logic.
If maintainers are okay with it as-is, then I have no issue with the 
code as-is and
Reviewed-by: Jason Andryuk <jason.andryuk@xxxxxxx>

Regards,
Jason