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 <jbeul...@suse.com>
>> ---
>> 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
