Re: [PATCH v3 10/16] x86/hyperlaunch: obtain cmdline from device tree

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Thu Apr 10, 2025 at 12:12 PM BST, Jan Beulich wrote:
> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/domain-builder/core.c
>> +++ b/xen/arch/x86/domain-builder/core.c
>> @@ -8,9 +8,37 @@
>>  #include <xen/lib.h>
>>  
>>  #include <asm/bootinfo.h>
>> +#include <asm/setup.h>
>>  
>>  #include "fdt.h"
>>  
>> +size_t __init builder_get_cmdline_size(struct boot_info *bi, int offset)
>> +{
>> +#ifdef CONFIG_DOMAIN_BUILDER
>> +    const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
>> +    int size = fdt_cmdline_prop_size(fdt, offset);
>> +
>> +    bootstrap_unmap();
>> +    return size < 0 ? 0 : size;
>
> Use max() instead of open-coding it?

Sure. On an unrelated matter, I've also removed the ifdef and relied on
DCE for v4 for this and the next helper.

>
>> --- a/xen/arch/x86/domain-builder/fdt.c
>> +++ b/xen/arch/x86/domain-builder/fdt.c
>> @@ -189,6 +189,12 @@ static int __init process_domain_node(
>>              printk("  kernel: boot module %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] )
>> +                bd->kernel->fdt_cmdline = fdt_get_prop_offset(
>> +                    fdt, node, "bootargs", &bd->kernel->cmdline_pa);
>
> Somewhat orthogonal question: Should there perhaps be a way for the boot 
> loader
> provided cmdline to go at the tail of the DT provided one?

That would preclude the bootloader fully overriding what's on the DT.
One can always just copy the cmdline in the DT to the bootloader and
adjust whatever is necessary there for testing. Adding append behaviour
sounds more like a hindrance rather than helpful. To me at least.

>
>> --- a/xen/arch/x86/domain-builder/fdt.h
>> +++ b/xen/arch/x86/domain-builder/fdt.h
>> @@ -12,6 +12,31 @@ struct boot_info;
>>  #define HYPERLAUNCH_MODULE_IDX 0
>>  
>>  #ifdef CONFIG_DOMAIN_BUILDER
>> +
>> +static inline int __init fdt_cmdline_prop_size(const void *fdt, int offset)
>> +{
>> +    int ret;
>> +
>> +    fdt_get_property_by_offset(fdt, offset, &ret);
>> +
>> +    return ret;
>> +}
>> +
>> +static inline int __init fdt_cmdline_prop_copy(
>> +    const void *fdt, int offset, char *cmdline, size_t size)
>> +{
>> +    int ret;
>> +    const struct fdt_property *prop =
>> +        fdt_get_property_by_offset(fdt, offset, &ret);
>> +
>> +    if ( ret < 0 )
>> +        return ret;
>> +
>> +    ASSERT(size > ret);
>> +
>> +    return strlcpy(cmdline, prop->data, ret);
>> +}
>
> What's the rationale for these to be separate functions, rather then the code
> being integrated into their sole callers? Especially for the former the extra
> layer feels excessive.

I've moved them onto domain-builder/fdt.c (where I believe they were
originally?) for v4.

>
>> --- a/xen/arch/x86/include/asm/domain-builder.h
>> +++ b/xen/arch/x86/include/asm/domain-builder.h
>> @@ -3,6 +3,10 @@
>>  
>>  struct boot_info;
>>  
>> +size_t __init builder_get_cmdline_size(struct boot_info *bi, int offset);
>> +int __init builder_get_cmdline(
>> +    struct boot_info *bi, int offset, char *cmdline, size_t size);
>
> No __init on declarations please.

Ack.

>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -984,7 +984,10 @@ static size_t __init domain_cmdline_size(
>>  {
>>      size_t s = bi->kextra ? strlen(bi->kextra) : 0;
>>  
>> -    s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
>> +    if ( bd->kernel->fdt_cmdline )
>> +        s += builder_get_cmdline_size(bi, bd->kernel->cmdline_pa);
>> +    else
>> +        s += strlen(__va(bd->kernel->cmdline_pa));
>
> Why's the check lost for bd->kernel->cmdline_pa being non-zero?

Shouldn't have been, indeed.
>
>> @@ -1047,9 +1050,12 @@ static struct domain *__init create_dom0(struct 
>> boot_info *bi)
>>          if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
>>              panic("Error allocating cmdline buffer for %pd\n", d);
>>  
>> -        if ( bd->kernel->cmdline_pa )
>> +        if ( bd->kernel->fdt_cmdline )
>> +            builder_get_cmdline(
>> +                bi, bd->kernel->cmdline_pa, cmdline, cmdline_size);
>> +        else
>
> Same here.

Ack.

>
>>              strlcpy(cmdline,
>> -                    cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
>> +                    cmdline_cook(__va(bd->kernel->cmdline_pa),bi->loader),
>
> The change to this line is bogus altogether.

Ugh, yes.

>
>> --- a/xen/include/xen/libfdt/libfdt-xen.h
>> +++ b/xen/include/xen/libfdt/libfdt-xen.h
>> @@ -23,6 +23,29 @@ static inline uint64_t  __init fdt_cell_as_u64(const 
>> fdt32_t *cell)
>>      return ((uint64_t)fdt32_to_cpu(cell[0]) << 32) | fdt32_to_cpu(cell[1]);
>>  }
>>  
>> +static inline bool __init fdt_get_prop_offset(
>> +    const void *fdt, int node, const char *name, unsigned long *offset)
>> +{
>> +    int ret, poffset;
>> +    const char *pname;
>> +
>> +    fdt_for_each_property_offset(poffset, fdt, node)
>> +    {
>> +        fdt_getprop_by_offset(fdt, poffset, &pname, &ret);
>> +
>> +        if ( ret < 0 )
>> +            continue;
>> +
>> +        if ( strcmp(pname, name) == 0 )
>> +        {
>> +            *offset = poffset;
>
> Variable naming looks backwards here.
>
> Jan

I think the leading p stands for "property" (for the sake of giving it a
name). But I see how that might be confusing when interpreted with a
Hungarian notation lens.

I'll invert it. It doesn't matter which is which, after all.

Cheers,
Alejandro