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 v7:
- updated commit message to expand on intent and purpose
---
xen/arch/x86/include/asm/bootdomain.h | 4 ++++
xen/arch/x86/include/asm/dom0_build.h | 1 +
xen/arch/x86/pv/dom0_build.c | 4 ++--
xen/arch/x86/setup.c | 18 ++++++++----------
4 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 3873f916f854..bc51f04a1df6 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -8,10 +8,14 @@
#ifndef __XEN_X86_BOOTDOMAIN_H__
#define __XEN_X86_BOOTDOMAIN_H__
+#include <public/xen.h>
+
struct boot_module;
struct domain;
struct boot_domain {
+ char cmdline[MAX_GUEST_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 4a75fb25a801..982bc1fa9e6b 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 28257bf13127..2c84af52de3e 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -960,8 +960,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 ( cmdline[0] != '\0' )
+ 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 7b78bd9c7c8d..9db8a650ecc2 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -980,8 +980,6 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
static struct domain *__init create_dom0(struct boot_info *bi)
{
- static char __initdata cmdline[MAX_GUEST_CMDLINE];
-
struct xen_domctl_createdomain dom0_cfg = {
.flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
.max_evtchn_port = -1,
@@ -1024,16 +1022,16 @@ static struct domain *__init create_dom0(struct boot_info *bi)
if ( bd->kernel->cmdline_pa || bi->kextra )
{
if ( bd->kernel->cmdline_pa )
- safe_strcpy(cmdline, cmdline_cook(__va(bd->kernel->cmdline_pa),
+ safe_strcpy(bd->cmdline, cmdline_cook(__va(bd->kernel->cmdline_pa),
bi->loader));
if ( bi->kextra )
/* kextra always includes exactly one leading space. */
- safe_strcat(cmdline, bi->kextra);
+ safe_strcat(bd->cmdline, bi->kextra);
/* Append any extra parameters. */
- if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
- safe_strcat(cmdline, " noapic");
+ if ( skip_ioapic_setup && !strstr(bd->cmdline, "noapic") )
+ safe_strcat(bd->cmdline, " noapic");
if ( (strlen(acpi_param) == 0) && acpi_disabled )
{
@@ -1041,13 +1039,13 @@ static struct domain *__init create_dom0(struct boot_info *bi)
safe_strcpy(acpi_param, "off");
}
- if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
+ if ( (strlen(acpi_param) != 0) && !strstr(bd->cmdline, "acpi=") )
{
- safe_strcat(cmdline, " acpi=");
- safe_strcat(cmdline, acpi_param);
+ safe_strcat(bd->cmdline, " acpi=");
+ safe_strcat(bd->cmdline, acpi_param);
}
- bd->kernel->cmdline_pa = __pa(cmdline);
+ bd->kernel->cmdline_pa = __pa(bd->cmdline);
}
bd->d = d;
--
2.30.2On 2024-11-02 13:25, 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.
Is the area only used at domain construction time? If that runs
sequentially, then only a single cmdline buffer would be needed.
cmdline_pa can keep pointing to individual cmdlines. Unless the
multi-domain builder needs to keep multiple? But that could maybe keep
cmdline_pa pointing into the device tree?
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
> Changes since v7:
> - updated commit message to expand on intent and purpose
> ---
> xen/arch/x86/include/asm/bootdomain.h | 4 ++++
> xen/arch/x86/include/asm/dom0_build.h | 1 +
> xen/arch/x86/pv/dom0_build.c | 4 ++--
> xen/arch/x86/setup.c | 18 ++++++++----------
> 4 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
> index 3873f916f854..bc51f04a1df6 100644
> --- a/xen/arch/x86/include/asm/bootdomain.h
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -8,10 +8,14 @@
> #ifndef __XEN_X86_BOOTDOMAIN_H__
> #define __XEN_X86_BOOTDOMAIN_H__
>
> +#include <public/xen.h>
> +
> struct boot_module;
> struct domain;
>
> struct boot_domain {
> + char cmdline[MAX_GUEST_CMDLINE];
> +
You might want to put the 1024 byte chars at the end of the structure.
Having the other pointers at the start is probably more useful.
> 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 28257bf13127..2c84af52de3e 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -960,8 +960,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 ( cmdline[0] != '\0' )
Shouldn't this check bd->cmdline[0] ? There is a const char *cmdline in
this function that should probably be removed.
Regards,
Jason
> + strlcpy((char *)si->cmd_line, bd->cmdline, sizeof(si->cmd_line));
>
> #ifdef CONFIG_VIDEO
> if ( !pv_shim && fill_console_start_info((void *)(si + 1
On 11/7/24 16:12, Jason Andryuk wrote:
> On 2024-11-02 13:25, 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.
>
> Is the area only used at domain construction time? If that runs
> sequentially, then only a single cmdline buffer would be needed.
> cmdline_pa can keep pointing to individual cmdlines. Unless the multi-
> domain builder needs to keep multiple? But that could maybe keep
> cmdline_pa pointing into the device tree?
It turns out that 1024 may not be large enough for all use cases.
Instead of trying to make an educated guess, this is being switched to
dynamic allocation for v9.
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>> Changes since v7:
>> - updated commit message to expand on intent and purpose
>> ---
>> xen/arch/x86/include/asm/bootdomain.h | 4 ++++
>> xen/arch/x86/include/asm/dom0_build.h | 1 +
>> xen/arch/x86/pv/dom0_build.c | 4 ++--
>> xen/arch/x86/setup.c | 18 ++++++++----------
>> 4 files changed, 15 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/
>> include/asm/bootdomain.h
>> index 3873f916f854..bc51f04a1df6 100644
>> --- a/xen/arch/x86/include/asm/bootdomain.h
>> +++ b/xen/arch/x86/include/asm/bootdomain.h
>> @@ -8,10 +8,14 @@
>> #ifndef __XEN_X86_BOOTDOMAIN_H__
>> #define __XEN_X86_BOOTDOMAIN_H__
>> +#include <public/xen.h>
>> +
>> struct boot_module;
>> struct domain;
>> struct boot_domain {
>> + char cmdline[MAX_GUEST_CMDLINE];
>> +
>
> You might want to put the 1024 byte chars at the end of the structure.
> Having the other pointers at the start is probably more useful.
OBE.
>> 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 28257bf13127..2c84af52de3e 100644
>> --- a/xen/arch/x86/pv/dom0_build.c
>> +++ b/xen/arch/x86/pv/dom0_build.c
>> @@ -960,8 +960,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 ( cmdline[0] != '\0' )
>
> Shouldn't this check bd->cmdline[0] ? There is a const char *cmdline in
> this function that should probably be removed.
This was caught in the switch to dynamic allocation and the local var
was dropped.
v/r,
dps
© 2016 - 2024 Red Hat, Inc.