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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

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

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

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

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.

Happy to fix on commit.

~Andrew