Re: [PATCH v2 09/15] x86/hyperlaunch: obtain cmdline from device tree

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.12.2024 17:57, Daniel P. Smith wrote:
> If a command line is not provided through the bootloader's mechanism, e.g.
> muiltboot module string field, then use one from the device tree if present.
> The device tree command line is located in the bootargs property of the
> `multiboot,kernel` node.

This reads as if it can be mix-and-match (and the code looks to confirm
this), which doesn't sound quite right. If you deem it right, please add
justification here.

> --- 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_t) size;

Nit: While I wouldn't insist, we don't normally put blanks after casts.
(The cast also isn't really needed here anyway.)

> +#else
> +    return 0;
> +#endif
> +}
> +
> +int __init builder_get_cmdline(
> +    struct boot_info *bi, int offset, char *cmdline, size_t size)
> +{
> +#ifdef CONFIG_DOMAIN_BUILDER
> +    const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +    int ret = fdt_cmdline_prop_copy(fdt, offset, cmdline, size);
> +
> +    bootstrap_unmap();
> +    return ret;
> +#else
> +    return 0;
> +#endif
> +}

Such #ifdef-ary is better to be avoided by providing stubs in the header.
Which then also works towards not needing to build in domain-builder/ when
COMFIG_DOMAIN_BUILDER=n.

> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -109,6 +109,16 @@ 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] )
> +            {
> +                int ret = fdt_get_prop_by_offset(
> +                    fdt, node, "bootargs", &bd->kernel->cmdline_pa);
> +                if ( ret > 0 )
> +                    bd->kernel->fdt_cmdline = true;

Is there a guarantee that fdt_get_prop_by_offset() won't alter its output
(passed in by indirection) in case of failure? Otherwise ...

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -981,7 +981,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));

... you'll be hosed here (and elsewhere).

> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -28,6 +28,30 @@ static inline int __init fdt_cell_as_u64(const fdt32_t 
> *cell, uint64_t *val)
>      return 0;
>  }
>  
> +static inline int __init fdt_get_prop_by_offset(
> +    const void *fdt, int node, const char *name, unsigned long *offset)
> +{
> +    int ret, poffset;
> +    const char *pname;
> +    size_t nsize = strlen(name);
> +
> +    fdt_for_each_property_offset(poffset, fdt, node)
> +    {
> +        fdt_getprop_by_offset(fdt, poffset, &pname, &ret);
> +
> +        if ( ret < 0 || strlen(pname) != nsize )
> +            continue;
> +
> +        if ( !strncmp(pname, name, nsize) )

I find this slightly odd: By now we know strlen(name) == strlen(pname) == nsize.
You could then use the usually more efficient memcmp() or the easier to invoke
strcmp().

Jan