Re: [PATCH v3 03/16] x86/boot: add cmdline to struct boot_domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.04.2025 13:11, Alejandro Vallejo wrote:
> On Wed Apr 9, 2025 at 7:48 AM BST, Jan Beulich wrote:
>> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>>> --- a/xen/arch/x86/hvm/dom0_build.c
>>> +++ b/xen/arch/x86/hvm/dom0_build.c
>>> @@ -653,7 +653,6 @@ static int __init pvh_load_kernel(
>>>      void *image_start = image_base + image->headroom;
>>>      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;
>>> @@ -736,8 +735,8 @@ static int __init pvh_load_kernel(
>>>              initrd = NULL;
>>>      }
>>>  
>>> -    if ( cmdline )
>>> -        extra_space += elf_round_up(&elf, strlen(cmdline) + 1);
>>> +    if ( bd->cmdline )
>>> +        extra_space += elf_round_up(&elf, strlen(bd->cmdline) + 1);
>>>  
>>>      last_addr = find_memory(d, &elf, extra_space);
>>>      if ( last_addr == INVALID_PADDR )
>>> @@ -778,9 +777,10 @@ static int __init pvh_load_kernel(
>>>      /* Free temporary buffers. */
>>>      free_boot_modules();
>>>  
>>> -    if ( cmdline != NULL )
>>> +    if ( bd->cmdline )
>>>      {
>>> -        rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 
>>> 1, v);
>>> +        rc = hvm_copy_to_guest_phys(last_addr, bd->cmdline,
>>> +                                    strlen(bd->cmdline) + 1, v);
>>>          if ( rc )
>>>          {
>>>              printk("Unable to copy guest command line\n");
>>> @@ -791,7 +791,7 @@ static int __init pvh_load_kernel(
>>>           * Round up to 32/64 bits (depending on the guest kernel bitness) 
>>> so
>>>           * the modlist/start_info is aligned.
>>>           */
>>> -        last_addr += elf_round_up(&elf, strlen(cmdline) + 1);
>>> +        last_addr += elf_round_up(&elf, strlen(bd->cmdline) + 1);
>>>      }
>>>      if ( initrd != NULL )
>>>      {
>>
>> Perhaps better introduce a local variable cmdline_len? That would allow the 
>> first
>> if() to go away (but of course not its body).
> 
> I'd agree if the function body was smaller, but it has 16 locals
> already. It's already quite hard to know what's going on, so I'd rather
> not make the situation worse.

You wouldn't: You'd replace one local var by another.

>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -978,10 +978,30 @@ static unsigned int __init copy_bios_e820(struct 
>>> e820entry *map, unsigned int li
>>>      return n;
>>>  }
>>>  
>>> -static struct domain *__init create_dom0(struct boot_info *bi)
>>> +static size_t __init domain_cmdline_size(
>>> +    struct boot_info *bi, struct boot_domain *bd)
>>
>> const for both? And perhaps s/domain/dom0/ in the function name?
>>
>>>  {
>>> -    static char __initdata cmdline[MAX_GUEST_CMDLINE];
>>> +    size_t s = bi->kextra ? strlen(bi->kextra) : 0;
>>> +
>>> +    s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
>>>  
>>> +    if ( s == 0 )
>>> +        return s;
>>
>> While this retains prior behavior, that prior behavior was certainly odd (and
>> pretty likely not meant to be like that).
> 
> What part of it? How would you propose it to behave? Do you mean that if
> no cmdline is passed some ought to be allocated in case we want to
> override it?

"noapic" and "acpi=" want appending (if so intended) irrespective of there
being a non-empty command line already.

> Either way, such a functional change is better suited for a different
> patch that does just that, plus properly handling the acpi adjustments
> for PVH dom0.

Maybe. It's always odd to see issues live on when changes are made in their
area. For backportability, yes, the fix may want to be separate (and first).

Jan