Re: [PATCH v6 06/12] x86/hyperlaunch: obtain cmdline from device tree

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.06.2025 19:39, Jason Andryuk wrote:
> 
> 
> On 2025-06-10 02:56, Jan Beulich wrote:
>> On 09.06.2025 19:07, Jason Andryuk wrote:
>>> On 2025-04-29 08:36, Alejandro Vallejo wrote:
>>>> From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>>>>
>>>> Add support to read the command line from the hyperlaunch device tree.
>>>> The device tree command line is located in the "bootargs" property of the
>>>> "multiboot,kernel" node.
>>>>
>>>> A boot loader command line, e.g. a grub module string field, takes
>>>> precendence over the device tree one since it is easier to modify.
>>>>
>>>> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>>>> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
>>>> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
>>>> Reviewed-by: Denis Mukhin <dmukhin@xxxxxxxx>
>>>> ---
>>>
>>>> diff --git a/xen/common/domain-builder/fdt.c 
>>>> b/xen/common/domain-builder/fdt.c
>>>> index cbb0ed30a2..dabe201b04 100644
>>>> --- a/xen/common/domain-builder/fdt.c
>>>> +++ b/xen/common/domain-builder/fdt.c
>>>> @@ -219,6 +219,12 @@ static int __init fdt_process_domain_node(
>>>>                printk(XENLOG_INFO "  kernel: multiboot-index=%d\n", idx);
>>>>                bi->mods[idx].type = BOOTMOD_KERNEL;
>>>>                bd->kernel = &bi->mods[idx];
>>>> +
>>>> +            /* If bootloader didn't set cmdline, see if FDT provides one. 
>>>> */
>>>> +            if ( bd->kernel->cmdline_pa &&
>>>> +                 !((char *)__va(bd->kernel->cmdline_pa))[0] )
>>>
>>> The logic is incorrect - it should be:
>>>
>>>              if ( !bd->kernel->cmdline_pa ||
>>>                   !((char *)__va(bd->kernel->cmdline_pa))[0] )
>>>
>>> If there is no cmdline_pa (which happens with the "reg" property) or the if 
>>> there is a 0-length string, then check the DT for bootargs.
>>
>> Even that sounds bogus to me: There's a difference between "no command line"
>> and "empty command line".
> 
> Yes, you have a point.  The difficulty is grub always provides a NUL 
> terminated string, so Xen can't differentiate the two.

Which may call for either special-casing GrUB, or at least calling out that
behavior in the comment. (Ideally we'd still have a way to distinguish
between both cases, but likely we'll need to resort to documenting that some
dummy option will need adding to tell "none" from [intended to be] empty.)

Jan