Re: [PATCH v3 03/16] x86/boot: add cmdline to struct boot_domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2025 18:07, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
> ---
> No changes on ACPI cmdline handling on PVH, as it's orthogonal to the
> purpose of this patch.
> 
> v3:
>   * s/xfree/XFREE/ on failed construct_dom0() to avoid a dangling
> cmdline ptr.
>   * Re-flow hvm_copy_to_guest_phys() into a multi-line call.
>   * s/bd->cmdline != NULL/b->cmdline/ (to homogenise with the previous
>     cmdline pointer check)
> ---
>  xen/arch/x86/hvm/dom0_build.c          | 12 +++----
>  xen/arch/x86/include/asm/boot-domain.h |  1 +
>  xen/arch/x86/pv/dom0_build.c           |  4 +--
>  xen/arch/x86/setup.c                   | 50 +++++++++++++++++++-------
>  4 files changed, 47 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 2a094b3145..ebad5a49b8 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -653,7 +653,6 @@ 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;
>      const char *initrd_cmdline = NULL;
>      struct elf_binary elf;
>      struct elf_dom_parms parms;
> @@ -736,8 +735,8 @@ static int __init pvh_load_kernel(
>              initrd = NULL;
>      }
>  
> -    if ( cmdline )
> -        extra_space += elf_round_up(&elf, strlen(cmdline) + 1);
> +    if ( bd->cmdline )
> +        extra_space += elf_round_up(&elf, strlen(bd->cmdline) + 1);
>  
>      last_addr = find_memory(d, &elf, extra_space);
>      if ( last_addr == INVALID_PADDR )
> @@ -778,9 +777,10 @@ static int __init pvh_load_kernel(
>      /* Free temporary buffers. */
>      free_boot_modules();
>  
> -    if ( cmdline != NULL )
> +    if ( bd->cmdline )
>      {
> -        rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 1, 
> v);
> +        rc = hvm_copy_to_guest_phys(last_addr, bd->cmdline,
> +                                    strlen(bd->cmdline) + 1, v);
>          if ( rc )
>          {
>              printk("Unable to copy guest command line\n");
> @@ -791,7 +791,7 @@ static int __init pvh_load_kernel(
>           * 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);
> +        last_addr += elf_round_up(&elf, strlen(bd->cmdline) + 1);
>      }
>      if ( initrd != NULL )
>      {

Perhaps better introduce a local variable cmdline_len? That would allow the 
first
if() to go away (but of course not its body).

> --- 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(
> +    struct boot_info *bi, struct boot_domain *bd)

const for both? And perhaps s/domain/dom0/ in the function name?

>  {
> -    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;

While this retains prior behavior, that prior behavior was certainly odd (and
pretty likely not meant to be like that).

> @@ -1043,17 +1067,19 @@ 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");
>  
> +    XFREE(cmdline);

While this tidies the local variable, what about bd->cmdline? As it stands this
gives the impression that you're freeing a pointer here which may still be used
through passing bd elsewhere.

Jan