| Message ID | 20250408160802.49870-4-agarciav@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Hyperlaunch device tree for dom0 | expand |
On 08.04.2025 18:07, Alejandro Vallejo wrote: > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > 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> > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> > Signed-off-by: Alejandro Vallejo <agarciav@amd.com> > --- > No changes on ACPI cmdline handling on PVH, as it's orthogonal to the > purpose of this patch. > > v3: > * s/xfree/XFREE/ on failed construct_dom0() to avoid a dangling > cmdline ptr. > * Re-flow hvm_copy_to_guest_phys() into a multi-line call. > * s/bd->cmdline != NULL/b->cmdline/ (to homogenise with the previous > cmdline pointer check) > --- > xen/arch/x86/hvm/dom0_build.c | 12 +++---- > xen/arch/x86/include/asm/boot-domain.h | 1 + > xen/arch/x86/pv/dom0_build.c | 4 +-- > xen/arch/x86/setup.c | 50 +++++++++++++++++++------- > 4 files changed, 47 insertions(+), 20 deletions(-) > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > index 2a094b3145..ebad5a49b8 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; > const char *initrd_cmdline = NULL; > struct elf_binary elf; > struct elf_dom_parms parms; > @@ -736,8 +735,8 @@ static int __init pvh_load_kernel( > initrd = NULL; > } > > - if ( cmdline ) > - extra_space += elf_round_up(&elf, strlen(cmdline) + 1); > + if ( bd->cmdline ) > + extra_space += elf_round_up(&elf, strlen(bd->cmdline) + 1); > > last_addr = find_memory(d, &elf, extra_space); > if ( last_addr == INVALID_PADDR ) > @@ -778,9 +777,10 @@ static int __init pvh_load_kernel( > /* Free temporary buffers. */ > free_boot_modules(); > > - if ( cmdline != NULL ) > + if ( bd->cmdline ) > { > - 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"); > @@ -791,7 +791,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 += elf_round_up(&elf, strlen(cmdline) + 1); > + last_addr += elf_round_up(&elf, strlen(bd->cmdline) + 1); > } > if ( initrd != NULL ) > { Perhaps better introduce a local variable cmdline_len? That would allow the first if() to go away (but of course not its body). > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -978,10 +978,30 @@ 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) const for both? And perhaps s/domain/dom0/ in the function name? > { > - 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; While this retains prior behavior, that prior behavior was certainly odd (and pretty likely not meant to be like that). > @@ -1043,17 +1067,19 @@ 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->kernel->cmdline_pa = 0; > + bd->cmdline = cmdline; > } > > bd->d = d; > if ( construct_dom0(bd) != 0 ) > panic("Could not construct domain 0\n"); > > + XFREE(cmdline); While this tidies the local variable, what about bd->cmdline? As it stands this gives the impression that you're freeing a pointer here which may still be used through passing bd elsewhere. Jan
On Wed Apr 9, 2025 at 7:48 AM BST, Jan Beulich wrote: > On 08.04.2025 18:07, Alejandro Vallejo wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> 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> >> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> >> Signed-off-by: Alejandro Vallejo <agarciav@amd.com> >> --- >> No changes on ACPI cmdline handling on PVH, as it's orthogonal to the >> purpose of this patch. >> >> v3: >> * s/xfree/XFREE/ on failed construct_dom0() to avoid a dangling >> cmdline ptr. >> * Re-flow hvm_copy_to_guest_phys() into a multi-line call. >> * s/bd->cmdline != NULL/b->cmdline/ (to homogenise with the previous >> cmdline pointer check) >> --- >> xen/arch/x86/hvm/dom0_build.c | 12 +++---- >> xen/arch/x86/include/asm/boot-domain.h | 1 + >> xen/arch/x86/pv/dom0_build.c | 4 +-- >> xen/arch/x86/setup.c | 50 +++++++++++++++++++------- >> 4 files changed, 47 insertions(+), 20 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c >> index 2a094b3145..ebad5a49b8 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; >> const char *initrd_cmdline = NULL; >> struct elf_binary elf; >> struct elf_dom_parms parms; >> @@ -736,8 +735,8 @@ static int __init pvh_load_kernel( >> initrd = NULL; >> } >> >> - if ( cmdline ) >> - extra_space += elf_round_up(&elf, strlen(cmdline) + 1); >> + if ( bd->cmdline ) >> + extra_space += elf_round_up(&elf, strlen(bd->cmdline) + 1); >> >> last_addr = find_memory(d, &elf, extra_space); >> if ( last_addr == INVALID_PADDR ) >> @@ -778,9 +777,10 @@ static int __init pvh_load_kernel( >> /* Free temporary buffers. */ >> free_boot_modules(); >> >> - if ( cmdline != NULL ) >> + if ( bd->cmdline ) >> { >> - 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"); >> @@ -791,7 +791,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 += elf_round_up(&elf, strlen(cmdline) + 1); >> + last_addr += elf_round_up(&elf, strlen(bd->cmdline) + 1); >> } >> if ( initrd != NULL ) >> { > > Perhaps better introduce a local variable cmdline_len? That would allow the first > if() to go away (but of course not its body). I'd agree if the function body was smaller, but it has 16 locals already. It's already quite hard to know what's going on, so I'd rather not make the situation worse. > >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -978,10 +978,30 @@ 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) > > const for both? And perhaps s/domain/dom0/ in the function name? > >> { >> - 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; > > While this retains prior behavior, that prior behavior was certainly odd (and > pretty likely not meant to be like that). What part of it? How would you propose it to behave? Do you mean that if no cmdline is passed some ought to be allocated in case we want to override it? Either way, such a functional change is better suited for a different patch that does just that, plus properly handling the acpi adjustments for PVH dom0. > >> @@ -1043,17 +1067,19 @@ 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->kernel->cmdline_pa = 0; >> + bd->cmdline = cmdline; >> } >> >> bd->d = d; >> if ( construct_dom0(bd) != 0 ) >> panic("Could not construct domain 0\n"); >> >> + XFREE(cmdline); > > While this tidies the local variable, what about bd->cmdline? As it stands this > gives the impression that you're freeing a pointer here which may still be used > through passing bd elsewhere. That ought to have been bd->cmdline indeed. > > Jan Cheers, Alejandro
On Wed Apr 9, 2025 at 12:11 PM BST, Alejandro Vallejo wrote: > On Wed Apr 9, 2025 at 7:48 AM BST, Jan Beulich wrote: >> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >>> @@ -978,10 +978,30 @@ 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) >> >> const for both? And perhaps s/domain/dom0/ in the function name? (missed this one) Sure to the const pointers. But as the hyperlaunch effort progresses the point is to turn all this into a more generic domain builders rather than having dom0-specific stuff. Changing the name like that here to adjust it in a few patches down the line doesn't seem worth the effort. >> While this tidies the local variable, what about bd->cmdline? As it stands this >> gives the impression that you're freeing a pointer here which may still be used >> through passing bd elsewhere. > > That ought to have been bd->cmdline indeed. > Actually, it can't be. It's a "const char *", so XFREE() chokes on it. I'll turn it into XFREE(cmdline); bd->cmdline = NULL; instead. Cheers, Alejandro
On 09.04.2025 13:11, Alejandro Vallejo wrote: > On Wed Apr 9, 2025 at 7:48 AM BST, Jan Beulich wrote: >> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>> --- 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; >>> const char *initrd_cmdline = NULL; >>> struct elf_binary elf; >>> struct elf_dom_parms parms; >>> @@ -736,8 +735,8 @@ static int __init pvh_load_kernel( >>> initrd = NULL; >>> } >>> >>> - if ( cmdline ) >>> - extra_space += elf_round_up(&elf, strlen(cmdline) + 1); >>> + if ( bd->cmdline ) >>> + extra_space += elf_round_up(&elf, strlen(bd->cmdline) + 1); >>> >>> last_addr = find_memory(d, &elf, extra_space); >>> if ( last_addr == INVALID_PADDR ) >>> @@ -778,9 +777,10 @@ static int __init pvh_load_kernel( >>> /* Free temporary buffers. */ >>> free_boot_modules(); >>> >>> - if ( cmdline != NULL ) >>> + if ( bd->cmdline ) >>> { >>> - 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"); >>> @@ -791,7 +791,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 += elf_round_up(&elf, strlen(cmdline) + 1); >>> + last_addr += elf_round_up(&elf, strlen(bd->cmdline) + 1); >>> } >>> if ( initrd != NULL ) >>> { >> >> Perhaps better introduce a local variable cmdline_len? That would allow the first >> if() to go away (but of course not its body). > > I'd agree if the function body was smaller, but it has 16 locals > already. It's already quite hard to know what's going on, so I'd rather > not make the situation worse. You wouldn't: You'd replace one local var by another. >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >>> @@ -978,10 +978,30 @@ 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) >> >> const for both? And perhaps s/domain/dom0/ in the function name? >> >>> { >>> - 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; >> >> While this retains prior behavior, that prior behavior was certainly odd (and >> pretty likely not meant to be like that). > > What part of it? How would you propose it to behave? Do you mean that if > no cmdline is passed some ought to be allocated in case we want to > override it? "noapic" and "acpi=" want appending (if so intended) irrespective of there being a non-empty command line already. > Either way, such a functional change is better suited for a different > patch that does just that, plus properly handling the acpi adjustments > for PVH dom0. Maybe. It's always odd to see issues live on when changes are made in their area. For backportability, yes, the fix may want to be separate (and first). Jan
On 09.04.2025 13:28, Alejandro Vallejo wrote: > On Wed Apr 9, 2025 at 12:11 PM BST, Alejandro Vallejo wrote: >> On Wed Apr 9, 2025 at 7:48 AM BST, Jan Beulich wrote: >>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>> --- a/xen/arch/x86/setup.c >>>> +++ b/xen/arch/x86/setup.c >>>> @@ -978,10 +978,30 @@ 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) >>> >>> const for both? And perhaps s/domain/dom0/ in the function name? > > (missed this one) > > Sure to the const pointers. But as the hyperlaunch effort progresses the > point is to turn all this into a more generic domain builders rather > than having dom0-specific stuff. Changing the name like that here to > adjust it in a few patches down the line doesn't seem worth the effort. Oh, if the function is going to fine further uses, that's likely okay. ("likely" because we've seen too many abandoned series, where we then ended up with pieces that were never gaining their intended final purpose.) >>> While this tidies the local variable, what about bd->cmdline? As it stands this >>> gives the impression that you're freeing a pointer here which may still be used >>> through passing bd elsewhere. >> >> That ought to have been bd->cmdline indeed. >> > > Actually, it can't be. It's a "const char *", so XFREE() chokes on it. > I'll turn it into > > XFREE(cmdline); > bd->cmdline = NULL; > > instead. At which point it can be xfree() again, seeing that the function ends right afterwards? Jan
On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@amd.com> wrote: > > > From: "Daniel P. Smith" dpsmith@apertussolutions.com > > > 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 > > Signed-off-by: Jason Andryuk jason.andryuk@amd.com > > Signed-off-by: Alejandro Vallejo agarciav@amd.com > > --- > No changes on ACPI cmdline handling on PVH, as it's orthogonal to the > purpose of this patch. > > v3: > * s/xfree/XFREE/ on failed construct_dom0() to avoid a dangling > cmdline ptr. > * Re-flow hvm_copy_to_guest_phys() into a multi-line call. > * s/bd->cmdline != NULL/b->cmdline/ (to homogenise with the previous > > cmdline pointer check) > --- > xen/arch/x86/hvm/dom0_build.c | 12 +++---- > xen/arch/x86/include/asm/boot-domain.h | 1 + > xen/arch/x86/pv/dom0_build.c | 4 +-- > xen/arch/x86/setup.c | 50 +++++++++++++++++++------- > 4 files changed, 47 insertions(+), 20 deletions(-) > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > index 2a094b3145..ebad5a49b8 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; > > const char *initrd_cmdline = NULL; > struct elf_binary elf; > struct elf_dom_parms parms; > @@ -736,8 +735,8 @@ static int __init pvh_load_kernel( > initrd = NULL; > } > > - if ( cmdline ) > - extra_space += elf_round_up(&elf, strlen(cmdline) + 1); > + if ( bd->cmdline ) > > + extra_space += elf_round_up(&elf, strlen(bd->cmdline) + 1); I think it makes sense to use local variable to cache the results of strlen(bd->cmdline) + 1 since the construct is used multiple times within pvh_load_kernel() E.g. cmdline_len similarly to {image,initrd}_len. > > > last_addr = find_memory(d, &elf, extra_space); > if ( last_addr == INVALID_PADDR ) > @@ -778,9 +777,10 @@ static int __init pvh_load_kernel( > /* Free temporary buffers. */ > free_boot_modules(); > > - if ( cmdline != NULL ) > + if ( bd->cmdline ) > > { > - 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"); > @@ -791,7 +791,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 += elf_round_up(&elf, strlen(cmdline) + 1); > + last_addr += elf_round_up(&elf, strlen(bd->cmdline) + 1); > > } > if ( initrd != NULL ) > { > diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h > index fcbedda0f0..d7c6042e25 100644 > --- a/xen/arch/x86/include/asm/boot-domain.h > +++ b/xen/arch/x86/include/asm/boot-domain.h > @@ -15,6 +15,7 @@ struct boot_domain { > > struct boot_module *kernel; > struct boot_module *module; > + const char *cmdline; > > struct domain *d; > }; > diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c > index b485eea05f..e1b78d47c2 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(const 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 585316f1ca..83cb66e309 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -978,10 +978,30 @@ 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; > + size_t cmdline_size; > struct xen_domctl_createdomain dom0_cfg = { > .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0, > .max_evtchn_port = -1, > @@ -1020,20 +1040,24 @@ static struct domain *__init create_dom0(struct boot_info bi) > if ( alloc_dom0_vcpu0(d) == NULL ) > panic("Error creating %pd vcpu 0\n", d); > > - / Grab the DOM0 command line. */ > - if ( bd->kernel->cmdline_pa || bi->kextra ) > > + 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 ( 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 ) > { > @@ -1043,17 +1067,19 @@ 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->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.43.0
On Wed Apr 9, 2025 at 10:05 PM BST, Denis Mukhin wrote: > On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@amd.com> wrote: > >> >> >> From: "Daniel P. Smith" dpsmith@apertussolutions.com >> >> >> 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 >> >> Signed-off-by: Jason Andryuk jason.andryuk@amd.com >> >> Signed-off-by: Alejandro Vallejo agarciav@amd.com >> >> --- >> No changes on ACPI cmdline handling on PVH, as it's orthogonal to the >> purpose of this patch. >> >> v3: >> * s/xfree/XFREE/ on failed construct_dom0() to avoid a dangling >> cmdline ptr. >> * Re-flow hvm_copy_to_guest_phys() into a multi-line call. >> * s/bd->cmdline != NULL/b->cmdline/ (to homogenise with the previous >> >> cmdline pointer check) >> --- >> xen/arch/x86/hvm/dom0_build.c | 12 +++---- >> xen/arch/x86/include/asm/boot-domain.h | 1 + >> xen/arch/x86/pv/dom0_build.c | 4 +-- >> xen/arch/x86/setup.c | 50 +++++++++++++++++++------- >> 4 files changed, 47 insertions(+), 20 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c >> index 2a094b3145..ebad5a49b8 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; >> >> const char *initrd_cmdline = NULL; >> struct elf_binary elf; >> struct elf_dom_parms parms; >> @@ -736,8 +735,8 @@ static int __init pvh_load_kernel( >> initrd = NULL; >> } >> >> - if ( cmdline ) >> - extra_space += elf_round_up(&elf, strlen(cmdline) + 1); >> + if ( bd->cmdline ) >> >> + extra_space += elf_round_up(&elf, strlen(bd->cmdline) + 1); > > I think it makes sense to use local variable to cache the results of > > strlen(bd->cmdline) + 1 > > since the construct is used multiple times within pvh_load_kernel() > > E.g. cmdline_len similarly to {image,initrd}_len. > Seeing how you're the second person to propose the same thing, I'll bite and just do it. I really dislike the wall of locals in pvh_load_kernel(), but oh well... Cheers, Alejandro
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index 2a094b3145..ebad5a49b8 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; const char *initrd_cmdline = NULL; struct elf_binary elf; struct elf_dom_parms parms; @@ -736,8 +735,8 @@ static int __init pvh_load_kernel( initrd = NULL; } - if ( cmdline ) - extra_space += elf_round_up(&elf, strlen(cmdline) + 1); + if ( bd->cmdline ) + extra_space += elf_round_up(&elf, strlen(bd->cmdline) + 1); last_addr = find_memory(d, &elf, extra_space); if ( last_addr == INVALID_PADDR ) @@ -778,9 +777,10 @@ static int __init pvh_load_kernel( /* Free temporary buffers. */ free_boot_modules(); - if ( cmdline != NULL ) + if ( bd->cmdline ) { - 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"); @@ -791,7 +791,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 += elf_round_up(&elf, strlen(cmdline) + 1); + last_addr += elf_round_up(&elf, strlen(bd->cmdline) + 1); } if ( initrd != NULL ) { diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h index fcbedda0f0..d7c6042e25 100644 --- a/xen/arch/x86/include/asm/boot-domain.h +++ b/xen/arch/x86/include/asm/boot-domain.h @@ -15,6 +15,7 @@ struct boot_domain { struct boot_module *kernel; struct boot_module *module; + const char *cmdline; struct domain *d; }; diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c index b485eea05f..e1b78d47c2 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(const 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 585316f1ca..83cb66e309 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -978,10 +978,30 @@ 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; + size_t cmdline_size; struct xen_domctl_createdomain dom0_cfg = { .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0, .max_evtchn_port = -1, @@ -1020,20 +1040,24 @@ static struct domain *__init create_dom0(struct boot_info *bi) if ( alloc_dom0_vcpu0(d) == NULL ) panic("Error creating %pd vcpu 0\n", d); - /* Grab the DOM0 command line. */ - if ( bd->kernel->cmdline_pa || bi->kextra ) + 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 ( 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 ) { @@ -1043,17 +1067,19 @@ 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->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; }