Re: [PATCH v9 6/6] x86/boot: add cmdline to struct boot_domain

[Jason Andryuk <jason.andryuk@amd.com>]

On 2024-11-15 08:12, Daniel P. Smith wrote:
> 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>


> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 533a1e2bbe05..b9ca9c486fe5 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -963,10 +963,31 @@ 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)
>   {
> -    static char __initdata cmdline[MAX_GUEST_CMDLINE];
> +    size_t s = 0;
> +
> +    s += bi->kextra ? strlen(bi->kextra) : 0;
> +    s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
>   
> +    /* Should only be called if one of extra or cmdline_pa are valid */
> +    ASSERT(s > 0);
> +
> +    /*
> +     * Add additional space for the following cases:
> +     *  - 7 chars for " noapic"
> +     *  - 13 chars for longest acpi opiton, " acpi=verbose"

option

> +     *  - 1 char to hold \0
> +     */
> +    s += 7 + 13 + 1;

Seems a little fragile.  Sizing but also depending on code elsewhere. 
Interesting - "verbose" wouldn't actually get updated into acpi_param. 
Anyway, using sizeof(acpi_param) seems better.  Maybe:

         s += strlen(" noapic") + strlen(" acpi=") + sizeof(acpi_param) + 1;

> +
> +    return s;
> +}
> +
> +static struct domain *__init create_dom0(struct boot_info *bi)
> +{
> +    char *cmdline = NULL;
>       struct xen_domctl_createdomain dom0_cfg = {
>           .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
>           .max_evtchn_port = -1,
> @@ -1008,17 +1029,23 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>       /* Grab the DOM0 command line. */
>       if ( bd->kernel->cmdline_pa || bi->kextra )

 From your other email, since you don't need the length, just non-zero:

     if ( (bd->kernel->cmdline_pa && __va(bd->kernel->cmdline_pa)[0]) ||
           bi->kextra )

>       {
> +        size_t cmdline_size = domain_cmdline_size(bi, bd);
> +
> +        if ( !(cmdline = xzalloc_array(char, cmdline_size)) )

Just xmalloc_array since it'll be overwritten immediately?

> +            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 )
>           {
> @@ -1028,17 +1055,21 @@ 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->cmdline = cmdline;
> +        bd->kernel->cmdline_pa = __pa(bd->cmdline);

Should cmdline_pa go away if we now have a valid cmdline variable?

Regards,
Jason

>       }
>   
>       bd->d = d;
>       if ( construct_dom0(bd) != 0 )
>           panic("Could not construct domain 0\n");
>   
> +    if ( cmdline )
> +        xfree(cmdline);
> +
>       return d;
>   }
>