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

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Wed Jun 11, 2025 at 7:35 AM CEST, Jan Beulich wrote:
> 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

We can add suitable comments where required, sure.

About the dummy option, note that even if we have an option for Xen, that does
nothing for the kernel cmdlines. If there's such dummy option there I don't know
of it.

Cheers,
Alejandro