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 <jbeul...@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
