| Message ID | 20241115131204.32135-7-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Boot modules for Hyperlaunch | expand |
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
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 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; > } >
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; }
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(-)