Re: [PATCH v4 01/13] x86/boot: add cmdline to struct boot_domain

[dmkhn@proton.me]

On Thu, Apr 17, 2025 at 01:48:23PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> 
> Add a container for the "cooked" command line for a domain. This
> provides for the backing memory to be directly associated with the
> domain being constructed.  This is done in anticipation that the domain
> construction path may need to be invoked multiple times, thus ensuring
> each instance had a distinct memory allocation.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>


Reviewed-by: Denis Mukhin <dmukhin@ford.com>

> ---
> v4:
>   * Manually nullify bd->cmdline before xfree()ing cmdline.
>   * const-ify arguments of domain_cmdline_size()
>   * Add cmdline_len to pvh_load_kernel()
> ---
>  xen/arch/x86/hvm/dom0_build.c          | 31 ++++++++--------
>  xen/arch/x86/include/asm/boot-domain.h |  1 +
>  xen/arch/x86/pv/dom0_build.c           |  4 +-
>  xen/arch/x86/setup.c                   | 51 ++++++++++++++++++++------
>  4 files changed, 57 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 2a094b3145..49832f921c 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -653,7 +653,7 @@ static int __init pvh_load_kernel(
>      void *image_start = image_base + image->headroom;
>      unsigned long image_len = image->size;
>      unsigned long initrd_len = initrd ? initrd->size : 0;
> -    const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : NULL;
> +    unsigned long cmdline_len = bd->cmdline ? strlen(bd->cmdline) + 1 : 0;
>      const char *initrd_cmdline = NULL;
>      struct elf_binary elf;
>      struct elf_dom_parms parms;
> @@ -736,8 +736,7 @@ static int __init pvh_load_kernel(
>              initrd = NULL;
>      }
> 
> -    if ( cmdline )
> -        extra_space += elf_round_up(&elf, strlen(cmdline) + 1);
> +    extra_space += elf_round_up(&elf, cmdline_len);
> 
>      last_addr = find_memory(d, &elf, extra_space);
>      if ( last_addr == INVALID_PADDR )
> @@ -778,21 +777,21 @@ static int __init pvh_load_kernel(
>      /* Free temporary buffers. */
>      free_boot_modules();
> 
> -    if ( cmdline != NULL )
> +    rc = hvm_copy_to_guest_phys(last_addr, bd->cmdline, cmdline_len, v);
> +    if ( rc )
>      {
> -        rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 1, v);
> -        if ( rc )
> -        {
> -            printk("Unable to copy guest command line\n");
> -            return rc;
> -        }
> -        start_info.cmdline_paddr = last_addr;
> -        /*
> -         * Round up to 32/64 bits (depending on the guest kernel bitness) so
> -         * the modlist/start_info is aligned.
> -         */
> -        last_addr += elf_round_up(&elf, strlen(cmdline) + 1);
> +        printk("Unable to copy guest command line\n");

Side note: I think it makes sense to add domain ID to all printouts in
pvh_load_kernel(). E.g. block under elf_xen_parse() logs domain ID.

> +        return rc;
>      }
> +
> +    start_info.cmdline_paddr = cmdline_len ? last_addr : 0;
> +
> +    /*
> +     * Round up to 32/64 bits (depending on the guest kernel bitness) so
> +     * the modlist/start_info is aligned.
> +     */
> +    last_addr += elf_round_up(&elf, cmdline_len);
> +
>      if ( initrd != NULL )
>      {
>          rc = hvm_copy_to_guest_phys(last_addr, &mod, sizeof(mod), v);
> diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
> index fcbedda0f0..d7c6042e25 100644
> --- a/xen/arch/x86/include/asm/boot-domain.h
> +++ b/xen/arch/x86/include/asm/boot-domain.h
> @@ -15,6 +15,7 @@ struct boot_domain {
> 
>      struct boot_module *kernel;
>      struct boot_module *module;
> +    const char *cmdline;
> 
>      struct domain *d;
>  };
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index b485eea05f..e1b78d47c2 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -972,8 +972,8 @@ static int __init dom0_construct(const struct boot_domain *bd)
>      }
> 
>      memset(si->cmd_line, 0, sizeof(si->cmd_line));
> -    if ( image->cmdline_pa )
> -        strlcpy((char *)si->cmd_line, __va(image->cmdline_pa), sizeof(si->cmd_line));
> +    if ( bd->cmdline )
> +        strlcpy((char *)si->cmd_line, bd->cmdline, sizeof(si->cmd_line));
> 
>  #ifdef CONFIG_VIDEO
>      if ( !pv_shim && fill_console_start_info((void *)(si + 1)) )
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 3c257f0bad..4df012460d 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -978,10 +978,30 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
>      return n;
>  }
> 
> -static struct domain *__init create_dom0(struct boot_info *bi)
> +static size_t __init domain_cmdline_size(const struct boot_info *bi,
> +                                         const struct boot_domain *bd)
>  {
> -    static char __initdata cmdline[MAX_GUEST_CMDLINE];
> +    size_t s = bi->kextra ? strlen(bi->kextra) : 0;
> +
> +    s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
> 
> +    if ( s == 0 )
> +        return s;
> +
> +    /*
> +     * Certain parameters from the Xen command line may be added to the dom0
> +     * command line. Add additional space for the possible cases along with one
> +     * extra char to hold \0.
> +     */
> +    s += strlen(" noapic") + strlen(" acpi=") + sizeof(acpi_param) + 1;
> +
> +    return s;
> +}
> +
> +static struct domain *__init create_dom0(struct boot_info *bi)
> +{
> +    char *cmdline = NULL;
> +    size_t cmdline_size;
>      struct xen_domctl_createdomain dom0_cfg = {
>          .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
>          .max_evtchn_port = -1,
> @@ -1020,20 +1040,24 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>      if ( alloc_dom0_vcpu0(d) == NULL )
>          panic("Error creating %pdv0\n", d);
> 
> -    /* Grab the DOM0 command line. */
> -    if ( bd->kernel->cmdline_pa || bi->kextra )
> +    cmdline_size = domain_cmdline_size(bi, bd);
> +    if ( cmdline_size )
>      {
> +        if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
> +            panic("Error allocating cmdline buffer for %pd\n", d);
> +
>          if ( bd->kernel->cmdline_pa )
> -            safe_strcpy(cmdline,
> -                        cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader));
> +            strlcpy(cmdline,
> +                    cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
> +                    cmdline_size);
> 
>          if ( bi->kextra )
>              /* kextra always includes exactly one leading space. */
> -            safe_strcat(cmdline, bi->kextra);
> +            strlcat(cmdline, bi->kextra, cmdline_size);
> 
>          /* Append any extra parameters. */
>          if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
> -            safe_strcat(cmdline, " noapic");
> +            strlcat(cmdline, " noapic", cmdline_size);
> 
>          if ( (strlen(acpi_param) == 0) && acpi_disabled )
>          {
> @@ -1043,17 +1067,20 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> 
>          if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
>          {
> -            safe_strcat(cmdline, " acpi=");
> -            safe_strcat(cmdline, acpi_param);
> +            strlcat(cmdline, " acpi=", cmdline_size);
> +            strlcat(cmdline, acpi_param, cmdline_size);
>          }
> -
> -        bd->kernel->cmdline_pa = __pa(cmdline);
> +        bd->kernel->cmdline_pa = 0;
> +        bd->cmdline = cmdline;
>      }
> 
>      bd->d = d;
>      if ( construct_dom0(bd) != 0 )
>          panic("Could not construct domain 0\n");
> 
> +    bd->cmdline = NULL;
> +    xfree(cmdline);
> +
>      return d;
>  }
> 
> --
> 2.43.0
> 
>