Re: [PATCH v4 07/13] x86/hyperlaunch: obtain cmdline from device tree

[dmkhn@proton.me]

On Thu, Apr 17, 2025 at 01:48:29PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> 
> Add support to read the command line from the hyperlauunch 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 ove the device tree one since it is easier to modify.

              ^^ over

Also, I would explain the command line parsing rules in the code commentary for
domain_cmdline_size() below.

> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> v4:
>   * Removed __init from header declarations.
>   * Removed ifdef guards, as DCE ensures the missing definitions don't
>     matter.
>   * Removed ifdef guards from new helpers in domain-builder/fdt.c
>     * Rely on DCE instead.
>   * Restored checks for cmdline_pa being zero.
>   * swapped offset and poffset varnames in libfdt-xen.h helper.
>   * swapped name and pname varnames in libfdt-xen.h helper.
>   * Turned foo == 0  into !foo in libfdt-xen.h helper.
> ---
>  xen/arch/x86/include/asm/bootinfo.h |  6 ++++--
>  xen/arch/x86/setup.c                | 10 +++++++--
>  xen/common/domain-builder/core.c    | 32 +++++++++++++++++++++++++++++
>  xen/common/domain-builder/fdt.c     |  6 ++++++
>  xen/common/domain-builder/fdt.h     | 24 ++++++++++++++++++++++
>  xen/include/xen/domain-builder.h    |  4 ++++
>  xen/include/xen/libfdt/libfdt-xen.h | 23 +++++++++++++++++++++
>  7 files changed, 101 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> index 1e3d582e45..5b2c93b1ef 100644
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -35,11 +35,13 @@ struct boot_module {
> 
>      /*
>       * Module State Flags:
> -     *   relocated: indicates module has been relocated in memory.
> -     *   released:  indicates module's pages have been freed.
> +     *   relocated:   indicates module has been relocated in memory.
> +     *   released:    indicates module's pages have been freed.
> +     *   fdt_cmdline: indicates module's cmdline is in the FDT.
>       */
>      bool relocated:1;
>      bool released:1;
> +    bool fdt_cmdline:1;
> 
>      /*
>       * A boot module may need decompressing by Xen.  Headroom is an estimate of
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 4f669f3c60..4cae13163b 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -984,7 +984,10 @@ static size_t __init domain_cmdline_size(const struct boot_info *bi,
>  {
>      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 if ( bd->kernel->cmdline_pa )
> +        s += strlen(__va(bd->kernel->cmdline_pa));
> 
>      if ( s == 0 )
>          return s;
> @@ -1047,7 +1050,10 @@ 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 if ( bd->kernel->cmdline_pa )
>              strlcpy(cmdline,
>                      cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
>                      cmdline_size);
> diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
> index 924cb495a3..4b4230f2ff 100644
> --- a/xen/common/domain-builder/core.c
> +++ b/xen/common/domain-builder/core.c
> @@ -8,9 +8,41 @@
>  #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)
> +{
> +    const void *fdt;
> +    int size;
> +
> +    if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
> +        return 0;
> +
> +    fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +    size = fdt_cmdline_prop_size(fdt, offset);
> +    bootstrap_unmap();
> +
> +    return max(0, size);
> +}
> +
> +int __init builder_get_cmdline(
> +    struct boot_info *bi, int offset, char *cmdline, size_t size)
> +{
> +    const void *fdt;
> +    int ret;
> +
> +    if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
> +        return 0;
> +
> +    fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +    ret = fdt_cmdline_prop_copy(fdt, offset, cmdline, size);
> +    bootstrap_unmap();
> +
> +    return ret;
> +}
> +
>  void __init builder_init(struct boot_info *bi)
>  {
>      bi->hyperlaunch_enabled = false;
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index 1fae6add3b..50fde2d007 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -209,6 +209,12 @@ static int __init 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] )
> +                bd->kernel->fdt_cmdline = fdt_get_prop_offset(
> +                    fdt, node, "bootargs", &bd->kernel->cmdline_pa);
>          }
>      }
> 
> diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
> index 8c98a256eb..d2ae7d5c36 100644
> --- a/xen/common/domain-builder/fdt.h
> +++ b/xen/common/domain-builder/fdt.h
> @@ -9,6 +9,30 @@ struct boot_info;
>  /* hyperlaunch fdt is required to be module 0 */
>  #define HYPERLAUNCH_MODULE_IDX 0
> 
> +static inline int __init fdt_cmdline_prop_size(const void *fdt, int offset)
> +{
> +    int ret;
> +
> +    fdt_get_property_by_offset(fdt, offset, &ret);

Perhaps something like

       (void)fdt_get_property_by_offset...

since there's no need to check the return value?

> +
> +    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);
> +}
> +
>  int has_hyperlaunch_fdt(const struct boot_info *bi);
>  int walk_hyperlaunch_fdt(struct boot_info *bi);
> 
> diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
> index ac2b84775d..ac0fd8f60b 100644
> --- a/xen/include/xen/domain-builder.h
> +++ b/xen/include/xen/domain-builder.h
> @@ -4,6 +4,10 @@
> 
>  struct boot_info;
> 
> +size_t builder_get_cmdline_size(const struct boot_info *bi, int offset);
> +int builder_get_cmdline(const struct boot_info *bi, int offset,
> +                               char *cmdline, size_t size);
> +
>  void builder_init(struct boot_info *bi);
> 
>  #endif /* __XEN_DOMAIN_BUILDER_H__ */
> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
> index deafb25d98..afc76e7750 100644
> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -24,6 +24,29 @@ static inline int __init fdt_prop_as_u32(
>      return 0;
>  }
> 
> +static inline bool __init fdt_get_prop_offset(
> +    const void *fdt, int node, const char *pname, unsigned long *poffset)
> +{
> +    int ret, offset;
> +    const char *name;
> +
> +    fdt_for_each_property_offset(offset, fdt, node)
> +    {
> +        fdt_getprop_by_offset(fdt, offset, &name, &ret);

Add

           (void)fdt_get_property_by_offset...

here as well?

> +
> +        if ( ret < 0 )
> +            continue;
> +
> +        if ( !strcmp(name, pname) )
> +        {
> +            *poffset = offset;
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
>  static inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
>                                          paddr_t *address,
>                                          paddr_t *size)
> --
> 2.43.0
> 
>