From: "Daniel P. Smith" <dpsmith@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 <dpsmith@apertussolutions.com>
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* Manually nullify bd->cmdline before xfree()ing cmdline.
* const-ify arguments of domain_cmdline_size()
* Add cmdline_len to pvh_load_kernel()
---
xen/arch/x86/hvm/dom0_build.c | 31 ++++++++--------
xen/arch/x86/include/asm/boot-domain.h | 1 +
xen/arch/x86/pv/dom0_build.c | 4 +-
xen/arch/x86/setup.c | 51 ++++++++++++++++++++------
4 files changed, 57 insertions(+), 30 deletions(-)
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 2a094b3145..49832f921c 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -653,7 +653,7 @@ 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;
+ unsigned long cmdline_len = bd->cmdline ? strlen(bd->cmdline) + 1 : 0;
const char *initrd_cmdline = NULL;
struct elf_binary elf;
struct elf_dom_parms parms;
@@ -736,8 +736,7 @@ static int __init pvh_load_kernel(
initrd = NULL;
}
- if ( cmdline )
- extra_space += elf_round_up(&elf, strlen(cmdline) + 1);
+ extra_space += elf_round_up(&elf, cmdline_len);
last_addr = find_memory(d, &elf, extra_space);
if ( last_addr == INVALID_PADDR )
@@ -778,21 +777,21 @@ static int __init pvh_load_kernel(
/* Free temporary buffers. */
free_boot_modules();
- if ( cmdline != NULL )
+ rc = hvm_copy_to_guest_phys(last_addr, bd->cmdline, cmdline_len, v);
+ if ( rc )
{
- rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 1, v);
- if ( rc )
- {
- printk("Unable to copy guest command line\n");
- return rc;
- }
- start_info.cmdline_paddr = last_addr;
- /*
- * 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);
+ printk("Unable to copy guest command line\n");
+ return rc;
}
+
+ start_info.cmdline_paddr = cmdline_len ? last_addr : 0;
+
+ /*
+ * 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, cmdline_len);
+
if ( initrd != NULL )
{
rc = hvm_copy_to_guest_phys(last_addr, &mod, sizeof(mod), v);
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 3c257f0bad..4df012460d 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(const struct boot_info *bi,
+ const 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 %pdv0\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,20 @@ 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");
+ bd->cmdline = NULL;
+ xfree(cmdline);
+
return d;
}
--
2.43.0On Thu, Apr 17, 2025 at 01:48:23PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@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 <dpsmith@apertussolutions.com>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
> ---
> v4:
> * Manually nullify bd->cmdline before xfree()ing cmdline.
> * const-ify arguments of domain_cmdline_size()
> * Add cmdline_len to pvh_load_kernel()
> ---
> xen/arch/x86/hvm/dom0_build.c | 31 ++++++++--------
> xen/arch/x86/include/asm/boot-domain.h | 1 +
> xen/arch/x86/pv/dom0_build.c | 4 +-
> xen/arch/x86/setup.c | 51 ++++++++++++++++++++------
> 4 files changed, 57 insertions(+), 30 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 2a094b3145..49832f921c 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -653,7 +653,7 @@ 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;
> + unsigned long cmdline_len = bd->cmdline ? strlen(bd->cmdline) + 1 : 0;
> const char *initrd_cmdline = NULL;
> struct elf_binary elf;
> struct elf_dom_parms parms;
> @@ -736,8 +736,7 @@ static int __init pvh_load_kernel(
> initrd = NULL;
> }
>
> - if ( cmdline )
> - extra_space += elf_round_up(&elf, strlen(cmdline) + 1);
> + extra_space += elf_round_up(&elf, cmdline_len);
>
> last_addr = find_memory(d, &elf, extra_space);
> if ( last_addr == INVALID_PADDR )
> @@ -778,21 +777,21 @@ static int __init pvh_load_kernel(
> /* Free temporary buffers. */
> free_boot_modules();
>
> - if ( cmdline != NULL )
> + rc = hvm_copy_to_guest_phys(last_addr, bd->cmdline, cmdline_len, v);
> + if ( rc )
> {
> - rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 1, v);
> - if ( rc )
> - {
> - printk("Unable to copy guest command line\n");
> - return rc;
> - }
> - start_info.cmdline_paddr = last_addr;
> - /*
> - * 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);
> + printk("Unable to copy guest command line\n");
Side note: I think it makes sense to add domain ID to all printouts in
pvh_load_kernel(). E.g. block under elf_xen_parse() logs domain ID.
> + return rc;
> }
> +
> + start_info.cmdline_paddr = cmdline_len ? last_addr : 0;
> +
> + /*
> + * 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, cmdline_len);
> +
> if ( initrd != NULL )
> {
> rc = hvm_copy_to_guest_phys(last_addr, &mod, sizeof(mod), v);
> 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 3c257f0bad..4df012460d 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(const struct boot_info *bi,
> + const 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 %pdv0\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,20 @@ 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");
>
> + bd->cmdline = NULL;
> + xfree(cmdline);
> +
> return d;
> }
>
> --
> 2.43.0
>
>
On Fri Apr 18, 2025 at 10:16 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:23PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@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 <dpsmith@apertussolutions.com>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>
>
> Reviewed-by: Denis Mukhin <dmukhin@ford.com>
Thanks
>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
>> index 2a094b3145..49832f921c 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -778,21 +777,21 @@ static int __init pvh_load_kernel(
>> /* Free temporary buffers. */
>> free_boot_modules();
>>
>> - if ( cmdline != NULL )
>> + rc = hvm_copy_to_guest_phys(last_addr, bd->cmdline, cmdline_len, v);
>> + if ( rc )
>> {
>> - rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 1, v);
>> - if ( rc )
>> - {
>> - printk("Unable to copy guest command line\n");
>> - return rc;
>> - }
>> - start_info.cmdline_paddr = last_addr;
>> - /*
>> - * 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);
>> + printk("Unable to copy guest command line\n");
>
> Side note: I think it makes sense to add domain ID to all printouts in
> pvh_load_kernel(). E.g. block under elf_xen_parse() logs domain ID.
Sounds sensible and easy enough. Sure.
Cheers,
Alejandro
On 17.04.2025 14:48, Alejandro Vallejo wrote: > From: "Daniel P. Smith" <dpsmith@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 <dpsmith@apertussolutions.com> > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> > Signed-off-by: Alejandro Vallejo <agarciav@amd.com> Acked-by: Jan Beulich <jbeulich@suse.com> preferably with ... > --- a/xen/arch/x86/hvm/dom0_build.c > +++ b/xen/arch/x86/hvm/dom0_build.c > @@ -653,7 +653,7 @@ 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; > + unsigned long cmdline_len = bd->cmdline ? strlen(bd->cmdline) + 1 : 0; ... this becoming either size_t (as you have it elsewhere) or unsigned int. Happy to make the adjustment while committing, so long as you agree with either of the suggested variants. Jan
On Thu Apr 17, 2025 at 3:54 PM BST, Jan Beulich wrote: > On 17.04.2025 14:48, Alejandro Vallejo wrote: >> From: "Daniel P. Smith" <dpsmith@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 <dpsmith@apertussolutions.com> >> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> >> Signed-off-by: Alejandro Vallejo <agarciav@amd.com> > > Acked-by: Jan Beulich <jbeulich@suse.com> Thanks, > preferably with ... > >> --- a/xen/arch/x86/hvm/dom0_build.c >> +++ b/xen/arch/x86/hvm/dom0_build.c >> @@ -653,7 +653,7 @@ 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; >> + unsigned long cmdline_len = bd->cmdline ? strlen(bd->cmdline) + 1 : 0; > > ... this becoming either size_t (as you have it elsewhere) or unsigned int. > Happy to make the adjustment while committing, so long as you agree with > either of the suggested variants. > > Jan Yes, sounds good. I'd rather it be size_t seeing as it's the output type of strlen() Cheers, Alejandro
© 2016 - 2025 Red Hat, Inc.