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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.03.2025 13:12, 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 )`.

Yes please.

Another thing I was considering to put in a local variable is the
repeated "elf_64bit(&elf) ? 8 : 4". If that sounds okay, I can make that
a(nother) pre-patch to mine.

Jan