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 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 | 12 +++---
xen/arch/x86/include/asm/bootdomain.h | 2 +
xen/arch/x86/pv/dom0_build.c | 4 +-
xen/arch/x86/setup.c | 54 ++++++++++++++++++++++-----
4 files changed, 54 insertions(+), 18 deletions(-)
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index a9384af14304..cbc365d678d2 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address(
}
static int __init pvh_load_kernel(
- struct domain *d, struct boot_module *image, struct boot_module *initrd,
- paddr_t *entry, paddr_t *start_info_addr)
+ struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr)
{
+ struct domain *d = bd->d;
+ struct boot_module *image = bd->kernel;
+ struct boot_module *initrd = bd->ramdisk;
void *image_base = bootstrap_map_bm(image);
void *image_start = image_base + image->headroom;
unsigned long image_len = image->size;
@@ -1304,14 +1306,12 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
int __init dom0_construct_pvh(struct boot_domain *bd)
{
paddr_t entry, start_info;
- struct boot_module *image = bd->kernel;
- struct boot_module *initrd = bd->ramdisk;
struct domain *d = bd->d;
int rc;
printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
- if ( image == NULL )
+ if ( bd->kernel == NULL )
panic("Missing kernel boot module for %pd construction\n", d);
if ( is_hardware_domain(d) )
@@ -1351,7 +1351,7 @@ int __init dom0_construct_pvh(struct boot_domain *bd)
return rc;
}
- rc = pvh_load_kernel(d, image, initrd, &entry, &start_info);
+ rc = pvh_load_kernel(bd, &entry, &start_info);
if ( rc )
{
printk("Failed to load Dom0 kernel\n");
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index bcbf36b13f25..ffda1509a63f 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -14,6 +14,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/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 a2178d5e8cc5..e6580382d247 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -965,10 +965,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,
@@ -1008,19 +1027,30 @@ 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 )
{
+ size_t cmdline_size = domain_cmdline_size(bi, bd);
+
+ if ( cmdline_size == 0 )
+ goto skip_cmdline;
+
+ 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 )
{
@@ -1030,17 +1060,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;
}
+ skip_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 23.11.2024 19:20, Daniel P. Smith wrote:
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address(
> }
>
> static int __init pvh_load_kernel(
> - struct domain *d, struct boot_module *image, struct boot_module *initrd,
> - paddr_t *entry, paddr_t *start_info_addr)
> + struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr)
> {
> + struct domain *d = bd->d;
> + struct boot_module *image = bd->kernel;
> + struct boot_module *initrd = bd->ramdisk;
> void *image_base = bootstrap_map_bm(image);
> void *image_start = image_base + image->headroom;
> unsigned long image_len = image->size;
> @@ -1304,14 +1306,12 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
> int __init dom0_construct_pvh(struct boot_domain *bd)
> {
> paddr_t entry, start_info;
> - struct boot_module *image = bd->kernel;
> - struct boot_module *initrd = bd->ramdisk;
> struct domain *d = bd->d;
> int rc;
>
> printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
>
> - if ( image == NULL )
> + if ( bd->kernel == NULL )
> panic("Missing kernel boot module for %pd construction\n", d);
>
> if ( is_hardware_domain(d) )
> @@ -1351,7 +1351,7 @@ int __init dom0_construct_pvh(struct boot_domain *bd)
> return rc;
> }
>
> - rc = pvh_load_kernel(d, image, initrd, &entry, &start_info);
> + rc = pvh_load_kernel(bd, &entry, &start_info);
> if ( rc )
> {
> printk("Failed to load Dom0 kernel\n");
None of this looks command line related - do these changes rather belong into
patch 1?
Jan
On 12/2/24 04:49, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address(
>> }
>>
>> static int __init pvh_load_kernel(
>> - struct domain *d, struct boot_module *image, struct boot_module *initrd,
>> - paddr_t *entry, paddr_t *start_info_addr)
>> + struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr)
>> {
>> + struct domain *d = bd->d;
>> + struct boot_module *image = bd->kernel;
>> + struct boot_module *initrd = bd->ramdisk;
>> void *image_base = bootstrap_map_bm(image);
>> void *image_start = image_base + image->headroom;
>> unsigned long image_len = image->size;
>> @@ -1304,14 +1306,12 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
>> int __init dom0_construct_pvh(struct boot_domain *bd)
>> {
>> paddr_t entry, start_info;
>> - struct boot_module *image = bd->kernel;
>> - struct boot_module *initrd = bd->ramdisk;
>> struct domain *d = bd->d;
>> int rc;
>>
>> printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
>>
>> - if ( image == NULL )
>> + if ( bd->kernel == NULL )
>> panic("Missing kernel boot module for %pd construction\n", d);
>>
>> if ( is_hardware_domain(d) )
>> @@ -1351,7 +1351,7 @@ int __init dom0_construct_pvh(struct boot_domain *bd)
>> return rc;
>> }
>>
>> - rc = pvh_load_kernel(d, image, initrd, &entry, &start_info);
>> + rc = pvh_load_kernel(bd, &entry, &start_info);
>> if ( rc )
>> {
>> printk("Failed to load Dom0 kernel\n");
>
> None of this looks command line related - do these changes rather belong into
> patch 1?
Hmmm, yah, it looks like it. This must have been a cherry-pick artifact
that I missed when updating these commits on top of the version from the
boot module reviews. I will review and move/drop the chunks.
v/r,
dps
On 2024-11-23 13:20, 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 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 | 12 +++---
> xen/arch/x86/include/asm/bootdomain.h | 2 +
> xen/arch/x86/pv/dom0_build.c | 4 +-
> xen/arch/x86/setup.c | 54 ++++++++++++++++++++++-----
> 4 files changed, 54 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index a9384af14304..cbc365d678d2 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address(
> }
>
> static int __init pvh_load_kernel(
> - struct domain *d, struct boot_module *image, struct boot_module *initrd,
> - paddr_t *entry, paddr_t *start_info_addr)
> + struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr)
> {
> + struct domain *d = bd->d;
> + struct boot_module *image = bd->kernel;
> + struct boot_module *initrd = bd->ramdisk;
> void *image_base = bootstrap_map_bm(image);
> void *image_start = image_base + image->headroom;
> unsigned long image_len = image->size;
cmdline_pa is used just outside of view below here.
const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : NULL;
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index a2178d5e8cc5..e6580382d247 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -965,10 +965,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,
> @@ -1008,19 +1027,30 @@ 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 )
Here you are checking pointers.
> {
> + size_t cmdline_size = domain_cmdline_size(bi, bd);
Internally, domain_cmdline_size() checks the pointers.
> +
> + if ( cmdline_size == 0 )
> + goto skip_cmdline;
> +
Maybe just use:
cmdline_size = domain_cmdline_size(bi, bd);
if ( cmdline_size )
{
and eliminate the goto?
> + 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 )
> {
> @@ -1030,17 +1060,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;
As mentioned above, it looks like you still inadvertently use
bd->kernel->cmdline_pa and not the new bd->cmdline. I think clearing
bd->kernel->cmdline_pa would have helped identify that. Or do you want
to retain cmdline_pa for some reason? It's less error prone if only one
is usable at a time.
> }
>
> + skip_cmdline:
> bd->d = d;
> if ( construct_dom0(bd) != 0 )
> panic("Could not construct domain 0\n");
>
> + if ( cmdline )
> + xfree(cmdline);
You can drop the if - xfree() handles NULL.
Regards,
Jason
> +
> return d;
> }
>
On 11/25/24 10:36, Jason Andryuk wrote:
> On 2024-11-23 13:20, 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 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 | 12 +++---
>> xen/arch/x86/include/asm/bootdomain.h | 2 +
>> xen/arch/x86/pv/dom0_build.c | 4 +-
>> xen/arch/x86/setup.c | 54 ++++++++++++++++++++++-----
>> 4 files changed, 54 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/
>> dom0_build.c
>> index a9384af14304..cbc365d678d2 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address(
>> }
>> static int __init pvh_load_kernel(
>> - struct domain *d, struct boot_module *image, struct boot_module
>> *initrd,
>> - paddr_t *entry, paddr_t *start_info_addr)
>> + struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr)
>> {
>> + struct domain *d = bd->d;
>> + struct boot_module *image = bd->kernel;
>> + struct boot_module *initrd = bd->ramdisk;
>> void *image_base = bootstrap_map_bm(image);
>> void *image_start = image_base + image->headroom;
>> unsigned long image_len = image->size;
>
> cmdline_pa is used just outside of view below here.
>
> const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : NULL;
As you mentioned below, this should have been converted to bd->cmdline,
there's no need for the intermediate variable.
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index a2178d5e8cc5..e6580382d247 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -965,10 +965,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,
>> @@ -1008,19 +1027,30 @@ 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 )
>
> Here you are checking pointers.
>
>> {
>> + size_t cmdline_size = domain_cmdline_size(bi, bd);
>
> Internally, domain_cmdline_size() checks the pointers.
>
>> +
>> + if ( cmdline_size == 0 )
>> + goto skip_cmdline;
>> +
>
> Maybe just use:
>
> cmdline_size = domain_cmdline_size(bi, bd);
> if ( cmdline_size )
> {
>
> and eliminate the goto?
Sure.
>> + 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 )
>> {
>> @@ -1030,17 +1060,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;
>
> As mentioned above, it looks like you still inadvertently use bd-
> >kernel->cmdline_pa and not the new bd->cmdline. I think clearing bd-
> >kernel->cmdline_pa would have helped identify that. Or do you want to
> retain cmdline_pa for some reason? It's less error prone if only one is
> usable at a time.
When it was just the boot module, it was requested that the pa be
updated to be the cooked command line. I dropped it here, as we begin to
transition to multi-domain construction. It is a good point that
zero-ing will stop inadvertent use of the old/original buffer.
>> }
>> + skip_cmdline:
>> bd->d = d;
>> if ( construct_dom0(bd) != 0 )
>> panic("Could not construct domain 0\n");
>> + if ( cmdline )
>> + xfree(cmdline);
>
> You can drop the if - xfree() handles NULL.
ack.
v/r,
dps
© 2016 - 2024 Red Hat, Inc.