Re: [PATCH v2 03/15] x86/boot: add cmdline to struct boot_domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.12.2024 17:57, Daniel P. Smith wrote:
> @@ -759,9 +758,10 @@ static int __init pvh_load_kernel(
>      /* Free temporary buffers. */
>      free_boot_modules();
>  
> -    if ( cmdline != NULL )
> +    if ( bd->cmdline != NULL )
>      {
> -        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);

Nit: Indentation. The anchor point for this kind of increased indentation
is the function name being called, so you want to add one more blank. (It
is not N times the usual indentation of 4, until "it looks okay".)

> --- a/xen/arch/x86/include/asm/bootdomain.h
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -11,6 +11,8 @@
>  #define __XEN_X86_BOOTDOMAIN_H__
>  
>  struct boot_domain {
> +    const char *cmdline;
> +
>      domid_t domid;
>  
>      struct boot_module *kernel;

I can see why in the earlier patch you added domid at the top. But cmdline?

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -975,10 +975,29 @@ 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 = 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;

See below; I question this all being necessary for PVH Dom0.

> @@ -1018,39 +1037,52 @@ static struct domain *__init create_dom0(struct 
> boot_info *bi)
>          panic("Error creating d%uv0\n", bd->domid);
>  
>      /* Grab the DOM0 command line. */
> -    if ( bd->kernel->cmdline_pa || bi->kextra )
> +    if ( (bd->kernel->cmdline_pa &&
> +          ((char *)__va(bd->kernel->cmdline_pa))[0]) ||
> +         bi->kextra )
>      {
> -        if ( bd->kernel->cmdline_pa )
> -            safe_strcpy(cmdline,
> -                        cmdline_cook(__va(bd->kernel->cmdline_pa), 
> bi->loader));
> +        size_t 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 ( bi->kextra )
> -            /* kextra always includes exactly one leading space. */
> -            safe_strcat(cmdline, bi->kextra);
> +            if ( bd->kernel->cmdline_pa )
> +                strlcpy(cmdline,
> +                        cmdline_cook(__va(bd->kernel->cmdline_pa), 
> bi->loader),
> +                        cmdline_size);
>  
> -        /* Append any extra parameters. */
> -        if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
> -            safe_strcat(cmdline, " noapic");
> +            if ( bi->kextra )
> +                /* kextra always includes exactly one leading space. */
> +                strlcat(cmdline, bi->kextra, cmdline_size);
>  
> -        if ( (strlen(acpi_param) == 0) && acpi_disabled )
> -        {
> -            printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
> -            safe_strcpy(acpi_param, "off");
> -        }
> +            /* Append any extra parameters. */
> +            if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
> +                strlcat(cmdline, " noapic", cmdline_size);

Roger - this isn't going to work very well with PVH Dom0, is it?

> -        if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
> -        {
> -            safe_strcat(cmdline, " acpi=");
> -            safe_strcat(cmdline, acpi_param);
> -        }
> +            if ( (strlen(acpi_param) == 0) && acpi_disabled )

Not sure whether the compiler will do that transformation anyway, but this
check looks odd to me. Why not simply check whether the first char is the
nul one?

> +            {
> +                printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
> +                safe_strcpy(acpi_param, "off");
> +            }

Here I'm doubtful, too, when it comes to PVH Dom0. If Xen's even works in
this mode anymore at all, I don't think we can sensibly start a PVH Dom0
then.

(This is leaving aside that all of this is Linux-centric anyway.)

> -        bd->kernel->cmdline_pa = __pa(cmdline);
> +            if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
> +            {
> +                strlcat(cmdline, " acpi=", cmdline_size);
> +                strlcat(cmdline, acpi_param, cmdline_size);
> +            }

(This, btw, won't work quite well when acpi= is specified more than once
on the command line.)

> +            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);

Leaving bd->cmdline dangling?

Jan