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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 11/15/24 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@xxxxxxxxxxxxxxxxxxxx>
> ---
> Changes since v8:
> - switch to a dynamically allocated buffer
> - dropped local cmdline var in pv dom0_construct()
>
> Changes since v7:
> - updated commit message to expand on intent and purpose
> ---
>   xen/arch/x86/include/asm/bootdomain.h |  2 ++
>   xen/arch/x86/include/asm/dom0_build.h |  1 +
>   xen/arch/x86/pv/dom0_build.c          |  6 ++--
>   xen/arch/x86/setup.c                  | 49 ++++++++++++++++++++++-----
>   4 files changed, 45 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/bootdomain.h 
> b/xen/arch/x86/include/asm/bootdomain.h
> index 3873f916f854..75e7c706d86e 100644
> --- a/xen/arch/x86/include/asm/bootdomain.h
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -12,6 +12,8 @@ struct boot_module;
>   struct domain;
>   
>   struct boot_domain {
> +    const char *cmdline;
> +
>       domid_t domid;
>   
>       struct boot_module *kernel;
> diff --git a/xen/arch/x86/include/asm/dom0_build.h 
> b/xen/arch/x86/include/asm/dom0_build.h
> index 8c94e87dc576..6ca3ca7c8a43 100644
> --- a/xen/arch/x86/include/asm/dom0_build.h
> +++ b/xen/arch/x86/include/asm/dom0_build.h
> @@ -4,6 +4,7 @@
>   #include <xen/libelf.h>
>   #include <xen/sched.h>
>   
> +#include <asm/bootinfo.h>
>   #include <asm/setup.h>
>   
>   extern unsigned int dom0_memflags;
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index f42aeb031694..91bcce1542bc 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -379,7 +379,6 @@ static int __init dom0_construct(struct boot_domain *bd)
>       unsigned long image_len;
>       void *image_start;
>       unsigned long initrd_len = initrd ? initrd->size : 0;
> -    const char *cmdline;
>       l4_pgentry_t *l4tab = NULL, *l4start = NULL;
>       l3_pgentry_t *l3tab = NULL, *l3start = NULL;
>       l2_pgentry_t *l2tab = NULL, *l2start = NULL;
> @@ -422,7 +421,6 @@ static int __init dom0_construct(struct boot_domain *bd)
>       image_base = bootstrap_map_bm(image);
>       image_len = image->size;
>       image_start = image_base + image->headroom;
> -    cmdline = __va(image->cmdline_pa);
>   
>       d->max_pages = ~0U;
>   
> @@ -972,8 +970,8 @@ static int __init dom0_construct(struct boot_domain *bd)
>       }
>   
>       memset(si->cmd_line, 0, sizeof(si->cmd_line));
> -    if ( cmdline != NULL )
> -        strlcpy((char *)si->cmd_line, cmdline, 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 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"
> +     *  - 1 char to hold \0
> +     */
> +    s += 7 + 13 + 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 )
The logic from which this originally derives mistakenly gives the sense, 
at least for me, that `string` field from module_t would only be a valid 
address if there was a string. I have now discovered this is not the 
case but is in fact the address of a zero length string. It just so 
happens all the previous logic worked out even for a zero length string. 
It also means this block was always being executed, since the check for 
a cmdline_pa will always be true. I am open to other suggestions, but my 
thinking right now is that the check of cmdline_pa should be a twofold 
check, that it is not zero and that it has a string length, e.g.:
    if ( (bd->kernel->cmdline_pa &&
          strlen(__va(bd->kernel->cmdline_pa))) || bi->kextra )