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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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