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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> --- 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?

> --- 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.

> --- 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.

> --- 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?

> @@ -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.

>              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.

> --- 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