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

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

On 11/6/24 19:14, Andrew Cooper wrote:
> On 06/11/2024 11:06 pm, 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?
> You could, but eventually MISRA will say no and I suspect it will then
> be made your problem to fix.
>
> In this case, we ought to have an `unsigned int idx` and not (re)use rc.
I can move to using an unsigned and ...

> Also, we panic far earlier in __start_xen() if there's no dom0 kernel,
> so I think we can just assert that rather than having a logically dead
> panic() path.
convert it to an ASSERT().

> >    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.
> Similar argument.  Except it turns out that i is used for precisely two
> loops in dom0_construct() both of which are from 0 to either 4 or 32.
>
> So it very much can be converted to being an unsigned variable, and then
> this works nicely.
Ack.

> That said, drop the unlikely().   This is an init function run once, and
> all it is doing is reducing legibility.
Ack.

> > > 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.
> I'm surprised that the compiler didn't complain about "idx < 0" being
> tautological here.
As above, unsigned and ASSERT.

v/r,
dps