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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.03.2025 19:24, Andrew Cooper wrote:
> On 24/03/2025 12:03 pm, 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>
>> ---
>> v3: Permit empty initrd with non-empty module command line again.
>> v2: Use elf_round_up(). Introduce initrd_cmdline local. Re-base.
> 
> I wasn't making a request to support an empty module with a non-empty
> cmdline.
> 
> That's a nonsense configuration; it's almost certainly an error
> elsewhere, rather than an intended configuration.

Who are we to judge whether some configuration is nonsense?

>> --- 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,26 @@ 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 )
>> -        extra_space += sizeof(mod) + ROUNDUP(initrd_len, PAGE_SIZE);
>> +    {
>> +        extra_space = elf_round_up(&elf, initrd_len);
> 
> I'm going to insist on not doing this.  For the sake of anyone (else)
> trying to follow this logic.
> 
> You're trading increased cognitive complexity of the logic to avoid
> using one local variable.

Well, okay, I'll make a change here then. Your earlier reply didn't sound
like "I'm going to insist", though. Instead I thought I could assume that
my reply there having been left un-responded to would mean you're okay
with the justification I gave.

Jan