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

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

On 11/15/24 09:25, Andrew Cooper wrote:
> On 15/11/2024 1:11 pm, 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>
> There are two minor things needing noting in the commit message.
>
> 1) dom0_construct() turns i from being signed to unsigned.  This is
> necessary for it's new use, and compatible with all pre-existing uses.
>
> 2) dom0_construct() also splits some 3-way assignments to placate MISRA,
> on lines which are modified.
Ack.

> > 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
> > @@ -642,15 +643,15 @@ static bool __init check_and_adjust_load_address(
> >       return true;
> >   }
> >   
> > -static int __init pvh_load_kernel(struct domain *d, const module_t *image,
> > -                                  unsigned long image_headroom,
> > -                                  module_t *initrd, void *image_base,
> > -                                  const char *cmdline, paddr_t *entry,
> > -                                  paddr_t *start_info_addr)
> > +static int __init pvh_load_kernel(
> > +    struct domain *d, struct boot_module *image, struct boot_module *initrd,
> > +    paddr_t *entry, paddr_t *start_info_addr)
> >   {
> > -    void *image_start = image_base + image_headroom;
> > -    unsigned long image_len = image->mod_end;
> > -    unsigned long initrd_len = initrd ? initrd->mod_end : 0;
> > +    void *image_base = bootstrap_map_bm(image);
> > +    void *image_start = image_base + image->headroom;
> > +    unsigned long image_len = image->mod->mod_end;
> > +    unsigned long initrd_len = initrd ? initrd->mod->mod_end : 0;
> > +    const char *cmdline = __va(image->cmdline_pa);
> This isn't safe.  __va(0) != NULL, so later between ...
Yah, that was careless to assume.

> >       struct elf_binary elf;
> >       struct elf_dom_parms parms;
> >       paddr_t last_addr;
> > @@ -725,8 +726,8 @@ static int __init pvh_load_kernel(struct domain *d, const 
> > module_t *image,
> ... these two hunks in the calculation for last_addr, we have:
>
>      ... cmdline ? ROUNDUP(strlen(cmdline) + 1, ...
>
> which does the wrong thing.  (And includes the 16bit IVT onto the
> guest's cmdline.)
>
>
> I'd suggest doing the same as we do with initrd_len/etc, and having:
>
>      const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) :
> NULL;
>
> to maintain the prior semantics.
Agreed.

> >   
> >       if ( initrd != NULL )
> >       {
> > -        rc = hvm_copy_to_guest_phys(last_addr, mfn_to_virt(initrd->mod_start),
> > -                                    initrd_len, v);
> > +        rc = hvm_copy_to_guest_phys(
> > +            last_addr, mfn_to_virt(initrd->mod->mod_start), initrd_len, v);
> This is a temporary adjustment, ending up shorter than it starts by
> patch 3.  I've tweaked it to reduce the churn overall.  I can live with
> 83 chars width for a commit or two...
Just trying to ensure I don't get dinged, so no objection on my part.

> > diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> > index cc882bee61c3..6be3d7745fab 100644
> > --- a/xen/arch/x86/pv/dom0_build.c
> > +++ b/xen/arch/x86/pv/dom0_build.c
> > @@ -354,13 +355,10 @@ static struct page_info * __init alloc_chunk(struct 
> > domain *d,
> >       return page;
> >   }
> >   
> > -static int __init dom0_construct(struct domain *d,
> > -                                 const module_t *image,
> > -                                 unsigned long image_headroom,
> > -                                 module_t *initrd,
> > -                                 const char *cmdline)
> > +static int __init dom0_construct(struct boot_info *bi, struct domain *d)
> >   {
> > -    int i, rc, order, machine;
> > +    unsigned int i;
> > +    int rc, order, machine;
> >       bool compatible, compat;
> >       struct cpu_user_regs *regs;
> >       unsigned long pfn, mfn;
> > @@ -374,10 +372,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;
> I'm tempted to put in some newlines here, just to break up the giant
> block of variables.
Yes, this is a very long block of declarations.

> This use of cmdline in principle needs a similar adjustment to the pvh
> case, but it's only used once, so I suggest this instead:
>
> @@ -984,8 +982,8 @@ static int __init dom0_construct(struct boot_info
> *bi, struct domain *d)
>       }
>   
>       memset(si->cmd_line, 0, sizeof(si->cmd_line));
> -    if ( cmdline != NULL )
> -        strlcpy((char *)si->cmd_line, cmdline, sizeof(si->cmd_line));
> +    if ( image->cmdline_pa )
> +        strlcpy((char *)si->cmd_line, __va(image->cmdline_pa),
> sizeof(si->cmd_line));
>   
>   #ifdef CONFIG_VIDEO
>       if ( !pv_shim && fill_console_start_info((void *)(si + 1)) )
>
>
> [edit] Turns out you do this in patch 6 anyway, so this way around will
> reduce churn.
Ack.

> Happy to fix on commit.
No objection.

v/r,
dps