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>
---
Changes since dom0 device tree v1:
- switched over to bd->cmdline in pvh_load_kernel
- moved cmdline processing under if, eliminating goto
- zero-ed cmdline_pa for kernel module after cmdline processing
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 | 14 ++---
xen/arch/x86/include/asm/bootdomain.h | 2 +
xen/arch/x86/pv/dom0_build.c | 4 +-
xen/arch/x86/setup.c | 78 +++++++++++++++++++--------
4 files changed, 66 insertions(+), 32 deletions(-)
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index cbc365d678d2..47bc3e9ce858 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;
struct elf_binary elf;
struct elf_dom_parms parms;
paddr_t last_addr;
@@ -717,9 +716,9 @@ static int __init pvh_load_kernel(
(initrd ? ROUNDUP(initrd_len, PAGE_SIZE) +
sizeof(mod)
: 0) +
- (cmdline ? ROUNDUP(strlen(cmdline) + 1,
- elf_64bit(&elf) ? 8 : 4)
- : 0));
+ (bd->cmdline ? ROUNDUP(strlen(bd->cmdline) + 1,
+ elf_64bit(&elf) ? 8 : 4)
+ : 0));
if ( last_addr == INVALID_PADDR )
{
printk("Unable to find a memory region to load initrd and metadata\n");
@@ -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);
if ( rc )
{
printk("Unable to copy guest command line\n");
@@ -772,7 +772,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 += ROUNDUP(strlen(cmdline) + 1, elf_64bit(&elf) ? 8 : 4);
+ last_addr += ROUNDUP(strlen(bd->cmdline) + 1, elf_64bit(&elf) ? 8 : 4);
}
if ( initrd != NULL )
{
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 67be575fe781..101a0c643d74 100644
--- 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;
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index e0709a1c1a7a..580f2703a154 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(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 27937a7f7aeb..a61131365477 100644
--- 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;
+ 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,
@@ -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);
- if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
- {
- safe_strcat(cmdline, " acpi=");
- safe_strcat(cmdline, acpi_param);
- }
+ if ( (strlen(acpi_param) == 0) && acpi_disabled )
+ {
+ printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
+ safe_strcpy(acpi_param, "off");
+ }
- 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);
+ }
+ 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.30.2On 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
On 2024-12-26 11:57, 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@apertussolutions.com>
> @@ -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);
I guess I wasn't clear last time. Instead of two levels of indent, I
was thinking at the top level:
/* Grab the DOM0 command line. */
cmdline_size = domain_cmdline_size(bi, bd);
if ( cmdline_size )
{
domain_cmdline_size() checks all the pointers, so this removes
duplication and indent.
The rest looks good.
Regards,
Jason
On 1/10/25 14:52, Jason Andryuk wrote:
> On 2024-12-26 11:57, 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@apertussolutions.com>
>
>
>> @@ -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);
>
> I guess I wasn't clear last time. Instead of two levels of indent, I
> was thinking at the top level:
>
> /* Grab the DOM0 command line. */
> cmdline_size = domain_cmdline_size(bi, bd);
> if ( cmdline_size )
> {
>
> domain_cmdline_size() checks all the pointers, so this removes
> duplication and indent.
But it is possible for there to be no command line, thus there is a
legitimate case where cmdline_size will be 0. If it is 0, there is no
reason to go through all of this logic.
v/r,
dps
On 2025-01-15 12:22, Daniel P. Smith wrote:
> On 1/10/25 14:52, Jason Andryuk wrote:
>> On 2024-12-26 11:57, 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@apertussolutions.com>
>>
>>
>>> @@ -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);
>>
>> I guess I wasn't clear last time. Instead of two levels of indent, I
>> was thinking at the top level:
>>
>> /* Grab the DOM0 command line. */
>> cmdline_size = domain_cmdline_size(bi, bd);
>> if ( cmdline_size )
>> {
>>
>> domain_cmdline_size() checks all the pointers, so this removes
>> duplication and indent.
>
> But it is possible for there to be no command line, thus there is a
> legitimate case where cmdline_size will be 0. If it is 0, there is no
> reason to go through all of this logic.
Sure, but domain_cmdline_size() already handles an empty command line
and returns 0 for that. So I think this logic is still skipped in that
case.
Regards,
Jason
© 2016 - 2026 Red Hat, Inc.