On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarc...@amd.com>
wrote:
>
>
> From: "Daniel P. Smith" dpsm...@apertussolutions.com
>
>
> 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 dpsm...@apertussolutions.com
>
> Signed-off-by: Jason Andryuk jason.andr...@amd.com
>
> Signed-off-by: Alejandro Vallejo agarc...@amd.com
>
> ---
> 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);
I think it makes sense to use local variable to cache the results of
strlen(bd->cmdline) + 1
since the construct is used multiple times within pvh_load_kernel()
E.g. cmdline_len similarly to {image,initrd}_len.
>
>
> 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 )
> {
> diff --git a/xen/arch/x86/include/asm/boot-domain.h
> b/xen/arch/x86/include/asm/boot-domain.h
> index fcbedda0f0..d7c6042e25 100644
> --- a/xen/arch/x86/include/asm/boot-domain.h
> +++ b/xen/arch/x86/include/asm/boot-domain.h
> @@ -15,6 +15,7 @@ struct boot_domain {
>
> struct boot_module *kernel;
> struct boot_module *module;
> + const char *cmdline;
>
> struct domain *d;
> };
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index b485eea05f..e1b78d47c2 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -972,8 +972,8 @@ static int __init dom0_construct(const struct boot_domain
> *bd)
> }
>
> memset(si->cmd_line, 0, sizeof(si->cmd_line));
>
> - if ( image->cmdline_pa )
>
> - strlcpy((char *)si->cmd_line, __va(image->cmdline_pa),
> 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 585316f1ca..83cb66e309 100644
> --- 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)
> {
> - 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;
> + size_t cmdline_size;
> struct xen_domctl_createdomain dom0_cfg = {
> .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
> .max_evtchn_port = -1,
> @@ -1020,20 +1040,24 @@ static struct domain *__init create_dom0(struct
> boot_info bi)
> if ( alloc_dom0_vcpu0(d) == NULL )
> panic("Error creating %pd vcpu 0\n", d);
>
> - / Grab the DOM0 command line. */
> - if ( bd->kernel->cmdline_pa || bi->kextra )
>
> + 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 ( 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 )
> {
> @@ -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);
> +
> return d;
> }
>
> --
> 2.43.0
