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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Mar 19, 2025 at 12:12:22PM +0000, Andrew Cooper wrote:
> On 19/03/2025 11:18 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.
> >
> > Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0")
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >
> > --- a/xen/arch/x86/hvm/dom0_build.c
> > +++ b/xen/arch/x86/hvm/dom0_build.c
> > @@ -712,7 +712,15 @@ static int __init pvh_load_kernel(
> >       * simplify it.
> >       */
> >      last_addr = find_memory(d, &elf, sizeof(start_info) +
> > -                            (initrd ? ROUNDUP(initrd_len, PAGE_SIZE) +
> > +                            (initrd ? ROUNDUP(ROUNDUP(initrd_len,
> > +                                                      elf_64bit(&elf) ? 8 
> > : 4) +
> > +                                              (initrd->cmdline_pa &&
> > +                                               strlen(__va(initrd->
> > +                                                           cmdline_pa))
> > +                                               ? strlen(__va(initrd->
> > +                                                             cmdline_pa)) 
> > + 1
> > +                                               : 0),
> > +                                              PAGE_SIZE) +
> >                                        sizeof(mod)
> >                                      : 0) +
> >                              (cmdline ? ROUNDUP(strlen(cmdline) + 1,
> 
> This piece of logic was already bad, but this is rather worse.
> 
> One patch I made while doing the boot module work is:
> 
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 6a4453103a9a..7292ddd07276 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -654,6 +654,7 @@ static int __init pvh_load_kernel(
>      const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) :
> NULL;
>      struct elf_binary elf;
>      struct elf_dom_parms parms;
> +    size_t metadata_len;
>      paddr_t last_addr;
>      struct hvm_start_info start_info = { 0 };
>      struct hvm_modlist_entry mod = { 0 };
> @@ -711,13 +712,16 @@ static int __init pvh_load_kernel(
>       * split into smaller allocations, done as a single region in order to
>       * simplify it.
>       */
> -    last_addr = find_memory(d, &elf, sizeof(start_info) +
> -                            (initrd ? ROUNDUP(initrd_len, PAGE_SIZE) +
> -                                      sizeof(mod)
> -                                    : 0) +
> -                            (cmdline ? ROUNDUP(strlen(cmdline) + 1,
> -                                               elf_64bit(&elf) ? 8 : 4)
> -                                     : 0));
> +    metadata_len = sizeof(start_info);
> +
> +    if ( initrd )
> +        metadata_len += sizeof(mod) + ROUNDUP(initrd_len, PAGE_SIZE);
> +
> +    if ( cmdline )
> +        metadata_len += ROUNDUP(strlen(cmdline) + 1,
> +                                elf_64bit(&elf) ? 8 : 4);
> +
> +    last_addr = find_memory(d, &elf, metadata_len);
>      if ( last_addr == INVALID_PADDR )
>      {
>          printk("Unable to find a memory region to load initrd and
> metadata\n");
> 
> 
> which I think I ought to submit as a prerequisite to this, after which
> your logic squashed on the RHS now becomes an expansion of the `if (
> initrd )`.
> 
> Thoughts?

Yes please, I was going to ask Jan to break that chain of ?: operators
as it's impossible to read.

Thanks, Roger.