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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-11-23 13:20, 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 v9 boot modules:
> - convert pvh_load_kernel to boot domain to directly use cmdline
> - adjustments to domain_cmdline_size
>    - remove ASSERT and return 0 instead
>    - use strlen() of values instead of hardcoded sizes
> - update cmdline parsing check to inspect multiboot string and not just pointer
> - add goto to skip cmdline processing if domain_cmdline_size returns 0
> - drop updating cmdline_pa with dynamic buffer with change of its last consumer
>    pvh_load_kernel
>
> 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/hvm/dom0_build.c         | 12 +++---
>   xen/arch/x86/include/asm/bootdomain.h |  2 +
>   xen/arch/x86/pv/dom0_build.c          |  4 +-
>   xen/arch/x86/setup.c                  | 54 ++++++++++++++++++++++-----
>   4 files changed, 54 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index a9384af14304..cbc365d678d2 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address(
>   }
>   
>   static int __init pvh_load_kernel(
> -    struct domain *d, struct boot_module *image, struct boot_module *initrd,
> -    paddr_t *entry, paddr_t *start_info_addr)
> +    struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr)
>   {
> +    struct domain *d = bd->d;
> +    struct boot_module *image = bd->kernel;
> +    struct boot_module *initrd = bd->ramdisk;
>       void *image_base = bootstrap_map_bm(image);
>       void *image_start = image_base + image->headroom;
>       unsigned long image_len = image->size;
cmdline_pa is used just outside of view below here.

const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : NULL;

> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index a2178d5e8cc5..e6580382d247 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -965,10 +965,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;
> +
> +    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,19 +1027,30 @@ 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 )
Here you are checking pointers.

>       {
> +        size_t cmdline_size = domain_cmdline_size(bi, bd);
Internally, domain_cmdline_size() checks the pointers.

> +
> +        if ( cmdline_size == 0 )
> +            goto skip_cmdline;
> +
Maybe just use:

cmdline_size = domain_cmdline_size(bi, bd);
if ( cmdline_size )
{

and eliminate the goto?

> +        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 )
>           {
> @@ -1030,17 +1060,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;
As mentioned above, it looks like you still inadvertently use 
bd->kernel->cmdline_pa and not the new bd->cmdline.  I think clearing 
bd->kernel->cmdline_pa would have helped identify that.  Or do you want 
to retain cmdline_pa for some reason?  It's less error prone if only one 
is usable at a time.
>       }
>   
> + skip_cmdline:
>       bd->d = d;
>       if ( construct_dom0(bd) != 0 )
>           panic("Could not construct domain 0\n");
>   
> +    if ( cmdline )
> +        xfree(cmdline);
You can drop the if - xfree() handles NULL.

Regards,
Jason

> +
>       return d;
>   }
>