| Message ID | 9bdc10ba-8572-46b3-a39e-e011e97d1e85@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/PVH: initial space allocation | expand |
On 20/03/2025 9:28 am, Jan Beulich wrote: > As per observation in practice, initrd->cmdline_pa is not normally zero. > Hence so far we always appended at least one byte. That alone may > already render insufficient the "allocation" made by find_memory(). > Things would be worse when there's actually a (perhaps long) command > line. > > Skip setup when the command line is empty. Amend the "allocation" size > by padding and actual size of module command line. Along these lines > also skip initrd setup when the initrd is zero size. > > Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Use elf_round_up(). Introduce initrd_cmdline local. Re-base. > > --- a/xen/arch/x86/hvm/dom0_build.c > +++ b/xen/arch/x86/hvm/dom0_build.c > @@ -652,9 +652,10 @@ static int __init pvh_load_kernel( > 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; > - size_t extra_space; > + size_t extra_space = 0; > paddr_t last_addr; > struct hvm_start_info start_info = { 0 }; > struct hvm_modlist_entry mod = { 0 }; > @@ -712,10 +713,23 @@ static int __init pvh_load_kernel( > * split into smaller allocations, done as a single region in order to > * simplify it. > */ > - extra_space = sizeof(start_info); > + if ( initrd_len ) > + { > + extra_space = elf_round_up(&elf, initrd_len); > + > + if ( initrd->cmdline_pa ) > + { > + initrd_cmdline = __va(initrd->cmdline_pa); > + if ( !*initrd_cmdline ) > + initrd_cmdline = NULL; > + } > + if ( initrd_cmdline ) > + extra_space += strlen(initrd_cmdline) + 1; > + > + extra_space = ROUNDUP(extra_space, PAGE_SIZE) + sizeof(mod); > + } > > - if ( initrd ) > - extra_space += sizeof(mod) + ROUNDUP(initrd_len, PAGE_SIZE); > + extra_space += sizeof(start_info); > This is rather ugly. I could rearrange the original patch, but the main issue is "extra_space = elf_round_up(&elf, initrd_len);" and that trick works exactly once in the function, seeing as it clobbers the running total. IMO it would be better to have a local initrd_space variable, with an "extra_space += ROUNDUP(initrd_space ...)", at which point the logic (and therefore the diff) becomes rather more simple. There is a change in behaviour. You mention empty initrds in the commit message, but it is possible to have an empty initrd with a non-empty cmdline. Previously we would have passed that on, whereas now we dont. I suspect we probably don't care. cmdlines on secondary modules are rare to begin with, but I just want to make sure we've considered the possibility. ~Andrew
On 20.03.2025 11:39, Andrew Cooper wrote: > On 20/03/2025 9:28 am, Jan Beulich wrote: >> As per observation in practice, initrd->cmdline_pa is not normally zero. >> Hence so far we always appended at least one byte. That alone may >> already render insufficient the "allocation" made by find_memory(). >> Things would be worse when there's actually a (perhaps long) command >> line. >> >> Skip setup when the command line is empty. Amend the "allocation" size >> by padding and actual size of module command line. Along these lines >> also skip initrd setup when the initrd is zero size. >> >> Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v2: Use elf_round_up(). Introduce initrd_cmdline local. Re-base. >> >> --- a/xen/arch/x86/hvm/dom0_build.c >> +++ b/xen/arch/x86/hvm/dom0_build.c >> @@ -652,9 +652,10 @@ static int __init pvh_load_kernel( >> 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; >> - size_t extra_space; >> + size_t extra_space = 0; >> paddr_t last_addr; >> struct hvm_start_info start_info = { 0 }; >> struct hvm_modlist_entry mod = { 0 }; >> @@ -712,10 +713,23 @@ static int __init pvh_load_kernel( >> * split into smaller allocations, done as a single region in order to >> * simplify it. >> */ >> - extra_space = sizeof(start_info); >> + if ( initrd_len ) >> + { >> + extra_space = elf_round_up(&elf, initrd_len); >> + >> + if ( initrd->cmdline_pa ) >> + { >> + initrd_cmdline = __va(initrd->cmdline_pa); >> + if ( !*initrd_cmdline ) >> + initrd_cmdline = NULL; >> + } >> + if ( initrd_cmdline ) >> + extra_space += strlen(initrd_cmdline) + 1; >> + >> + extra_space = ROUNDUP(extra_space, PAGE_SIZE) + sizeof(mod); >> + } >> >> - if ( initrd ) >> - extra_space += sizeof(mod) + ROUNDUP(initrd_len, PAGE_SIZE); >> + extra_space += sizeof(start_info); >> > > This is rather ugly. I could rearrange the original patch, but the main > issue is "extra_space = elf_round_up(&elf, initrd_len);" and that trick > works exactly once in the function, seeing as it clobbers the running total. My expectation was that quite likely we won't need that "trick" a 2nd time here. > IMO it would be better to have a local initrd_space variable, with an > "extra_space += ROUNDUP(initrd_space ...)", at which point the logic > (and therefore the diff) becomes rather more simple. If you really think we need this, I can do so, but I would prefer to avoid it. > There is a change in behaviour. You mention empty initrds in the commit > message, but it is possible to have an empty initrd with a non-empty > cmdline. Previously we would have passed that on, whereas now we dont. Hmm, yes, I can take care of that (at the expense of yet more churn). Jan > I suspect we probably don't care. cmdlines on secondary modules are > rare to begin with, but I just want to make sure we've considered the > possibility. > > ~Andrew
--- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -652,9 +652,10 @@ static int __init pvh_load_kernel( 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; - size_t extra_space; + size_t extra_space = 0; paddr_t last_addr; struct hvm_start_info start_info = { 0 }; struct hvm_modlist_entry mod = { 0 }; @@ -712,10 +713,23 @@ static int __init pvh_load_kernel( * split into smaller allocations, done as a single region in order to * simplify it. */ - extra_space = sizeof(start_info); + if ( initrd_len ) + { + extra_space = elf_round_up(&elf, initrd_len); + + if ( initrd->cmdline_pa ) + { + initrd_cmdline = __va(initrd->cmdline_pa); + if ( !*initrd_cmdline ) + initrd_cmdline = NULL; + } + if ( initrd_cmdline ) + extra_space += strlen(initrd_cmdline) + 1; + + extra_space = ROUNDUP(extra_space, PAGE_SIZE) + sizeof(mod); + } - if ( initrd ) - extra_space += sizeof(mod) + ROUNDUP(initrd_len, PAGE_SIZE); + extra_space += sizeof(start_info); if ( cmdline ) extra_space += elf_round_up(&elf, strlen(cmdline) + 1); @@ -727,7 +741,7 @@ static int __init pvh_load_kernel( return -ENOMEM; } - if ( initrd != NULL ) + if ( initrd_len ) { rc = hvm_copy_to_guest_phys(last_addr, __va(initrd->start), initrd_len, v); @@ -740,12 +754,11 @@ static int __init pvh_load_kernel( mod.paddr = last_addr; mod.size = initrd_len; last_addr += elf_round_up(&elf, initrd_len); - if ( initrd->cmdline_pa ) + if ( initrd_cmdline ) { - char *str = __va(initrd->cmdline_pa); - size_t len = strlen(str) + 1; + size_t len = strlen(initrd_cmdline) + 1; - rc = hvm_copy_to_guest_phys(last_addr, str, len, v); + rc = hvm_copy_to_guest_phys(last_addr, initrd_cmdline, len, v); if ( rc ) { printk("Unable to copy module command line\n"); @@ -775,7 +788,7 @@ static int __init pvh_load_kernel( */ last_addr += elf_round_up(&elf, strlen(cmdline) + 1); } - if ( initrd != NULL ) + if ( initrd_len ) { rc = hvm_copy_to_guest_phys(last_addr, &mod, sizeof(mod), v); if ( rc )
As per observation in practice, initrd->cmdline_pa is not normally zero. Hence so far we always appended at least one byte. That alone may already render insufficient the "allocation" made by find_memory(). Things would be worse when there's actually a (perhaps long) command line. Skip setup when the command line is empty. Amend the "allocation" size by padding and actual size of module command line. Along these lines also skip initrd setup when the initrd is zero size. Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Use elf_round_up(). Introduce initrd_cmdline local. Re-base.