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 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 )
{
+ size_t cmdline_size = domain_cmdline_size(bi, bd);
+
+ 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 )
{
@@ -1028,17 +1055,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;
+ bd->kernel->cmdline_pa = __pa(bd->cmdline);
}
bd->d = d;
if ( construct_dom0(bd) != 0 )
panic("Could not construct domain 0\n");
+ if ( cmdline )
+ xfree(cmdline);
+
return d;
}
--
2.30.2On 2024-11-15 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@apertussolutions.com>
> 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"
option
> + * - 1 char to hold \0
> + */
> + s += 7 + 13 + 1;
Seems a little fragile. Sizing but also depending on code elsewhere.
Interesting - "verbose" wouldn't actually get updated into acpi_param.
Anyway, using sizeof(acpi_param) seems better. Maybe:
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,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 )
From your other email, since you don't need the length, just non-zero:
if ( (bd->kernel->cmdline_pa && __va(bd->kernel->cmdline_pa)[0]) ||
bi->kextra )
> {
> + size_t cmdline_size = domain_cmdline_size(bi, bd);
> +
> + if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
Just xmalloc_array since it'll be overwritten immediately?
> + 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 )
> {
> @@ -1028,17 +1055,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;
> + bd->kernel->cmdline_pa = __pa(bd->cmdline);
Should cmdline_pa go away if we now have a valid cmdline variable?
Regards,
Jason
> }
>
> bd->d = d;
> if ( construct_dom0(bd) != 0 )
> panic("Could not construct domain 0\n");
>
> + if ( cmdline )
> + xfree(cmdline);
> +
> return d;
> }
>
On 11/15/24 13:20, Jason Andryuk wrote:
> On 2024-11-15 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@apertussolutions.com>
>
>
>> 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"
>
> option
>
>> + * - 1 char to hold \0
>> + */
>> + s += 7 + 13 + 1;
>
> Seems a little fragile. Sizing but also depending on code elsewhere.
> Interesting - "verbose" wouldn't actually get updated into acpi_param.
> Anyway, using sizeof(acpi_param) seems better. Maybe:
>
> s += strlen(" noapic") + strlen(" acpi=") + sizeof(acpi_param)
> + 1;
True, the strlen() is more reasonable and self documenting. Yes, I
overlooked the return in the option parser that would not copy verbose,
plus sizeof(acpi_param) will ensure adequate spacing.
>> +
>> + 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 )
>
> From your other email, since you don't need the length, just non-zero:
>
> if ( (bd->kernel->cmdline_pa && __va(bd->kernel->cmdline_pa)[0]) ||
> bi->kextra )
Ack.
>> {
>> + size_t cmdline_size = domain_cmdline_size(bi, bd);
>> +
>> + if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
>
> Just xmalloc_array since it'll be overwritten immediately?
Yes and no, the concern I was worried about is that the allocation may
end up being slight bigger than what is copied in to the buffer. If we
do not zero the buffer, then those trailing bytes will have random data
in them. In a perfect world, nothing should ever reach those bytes based
on the current usage of the buffer. But from my perspective, it would be
safer to zero the buffer than rely on the world being perfect. I am not
fixed on not switching, just providing my 2 cents,
>> + 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 )
>> {
>> @@ -1028,17 +1055,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;
>> + bd->kernel->cmdline_pa = __pa(bd->cmdline);
>
> Should cmdline_pa go away if we now have a valid cmdline variable?
In the PVH dom0 case, we are still relying on cmdline_pa as the
reference to get at the command line in the function pvh_load_kernel().
With the introduction of cmdline to boot_domain, I could convert the
interface of pvh_load_kernel() to take the boot_domain instance,
removing the need to update cmdline_pa. Not sure if you were asking
this, but as for cmdline_pa going completely away, that is not possible.
First the sequence of events do not allow it, and there is an one-off
case for PVH dom0 where the cmdline_pa of the initrd module is copied
into the domain.
v/r,
dps
On 2024-11-20 12:57, Daniel P. Smith wrote:
> On 11/15/24 13:20, Jason Andryuk wrote:
>> On 2024-11-15 08:12, Daniel P. Smith wrote:>>
>> Just xmalloc_array since it'll be overwritten immediately?
>
> Yes and no, the concern I was worried about is that the allocation may
> end up being slight bigger than what is copied in to the buffer. If we
> do not zero the buffer, then those trailing bytes will have random data
> in them. In a perfect world, nothing should ever reach those bytes based
> on the current usage of the buffer. But from my perspective, it would be
> safer to zero the buffer than rely on the world being perfect. I am not
> fixed on not switching, just providing my 2 cents,
I only looked at the PVH case, but strlen() is used there to obtain the
copy length. I think it's unnecessary and the code doesn't require a
zeroed buffer. But I also realize it's safer to start from zeroed.
>>> + 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),
Also I just noticed a missing space: "cmdline_pa),bi"
>>> + 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 )
>>> {
>>> @@ -1028,17 +1055,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;
>>> + bd->kernel->cmdline_pa = __pa(bd->cmdline);
>>
>> Should cmdline_pa go away if we now have a valid cmdline variable?
>
> In the PVH dom0 case, we are still relying on cmdline_pa as the
> reference to get at the command line in the function pvh_load_kernel().
> With the introduction of cmdline to boot_domain, I could convert the
> interface of pvh_load_kernel() to take the boot_domain instance,
> removing the need to update cmdline_pa. Not sure if you were asking
> this, but as for cmdline_pa going completely away, that is not possible.
> First the sequence of events do not allow it, and there is an one-off
> case for PVH dom0 where the cmdline_pa of the initrd module is copied
> into the domain.
I was thinking from this point forward only boot_domain->cmdline is
necessary. Maybe even zero-ing cmdline_pa? With a valid pointer in
cmdline, cmdline_pa shouldn't be necessary anymore.
Regards,
Jason
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@apertussolutions.com>
> ---
> 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 )
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@apertussolutions.com>
> ---
> 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;
Working on the subsequent series and realized this line could/should be
merged with the declaration line;
v/r,
dps
© 2016 - 2025 Red Hat, Inc.