Re: [PATCH v2 2/2] x86/PVH: account for module command line length

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
> ---
> 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