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.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since v1:
- moved common fdt functions to libfdt
- rename prop_as_offset to more correct prop_by_offset
---
 xen/arch/x86/domain-builder/core.c       | 28 ++++++++++++++++++++++++
 xen/arch/x86/domain-builder/fdt.c        | 10 +++++++++
 xen/arch/x86/domain-builder/fdt.h        | 24 ++++++++++++++++++++
 xen/arch/x86/include/asm/bootinfo.h      |  6 +++--
 xen/arch/x86/include/asm/domainbuilder.h |  4 ++++
 xen/arch/x86/setup.c                     | 15 +++++++++----
 xen/include/xen/libfdt/libfdt-xen.h      | 24 ++++++++++++++++++++
 7 files changed, 105 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/domain-builder/core.c b/xen/arch/x86/domain-builder/core.c
index eda7fa7a8ffa..91d1b7367e76 100644
--- 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;
+#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
+}
+
 void __init builder_init(struct boot_info *bi)
 {
     if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
index bcaee50689a6..1094c8dc8838 100644
--- 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;
+            }
         }
     }
 
diff --git a/xen/arch/x86/domain-builder/fdt.h b/xen/arch/x86/domain-builder/fdt.h
index 0be4ac771bc4..3938b0d2619b 100644
--- a/xen/arch/x86/domain-builder/fdt.h
+++ b/xen/arch/x86/domain-builder/fdt.h
@@ -13,6 +13,30 @@
 
 #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);
+}
+
 int has_hyperlaunch_fdt(struct boot_info *bi);
 int walk_hyperlaunch_fdt(struct boot_info *bi);
 #else
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 683ca9dbe2e0..433a0e66121b 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/include/asm/domainbuilder.h b/xen/arch/x86/include/asm/domainbuilder.h
index aedc2b49f7c9..21221d8df8ec 100644
--- a/xen/arch/x86/include/asm/domainbuilder.h
+++ b/xen/arch/x86/include/asm/domainbuilder.h
@@ -3,6 +3,10 @@
 
 #include <asm/bootinfo.h>
 
+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);
+
 void builder_init(struct boot_info *bi);
 
 #endif
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 71ce9315d3ac..3dd04b9afca7 100644
--- 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));
 
     if ( s == 0 )
         return s;
@@ -1039,7 +1042,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
 
     /* Grab the DOM0 command line. */
     if ( (bd->kernel->cmdline_pa &&
-          ((char *)__va(bd->kernel->cmdline_pa))[0]) ||
+          (bd->kernel->fdt_cmdline ||
+           ((char *)__va(bd->kernel->cmdline_pa))[0])) ||
          bi->kextra )
     {
         size_t cmdline_size = domain_cmdline_size(bi, bd);
@@ -1049,9 +1053,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
                 strlcpy(cmdline,
-                        cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
+                        cmdline_cook(__va(bd->kernel->cmdline_pa),bi->loader),
                         cmdline_size);
 
             if ( bi->kextra )
diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
index 27d23df03af3..0e54aeeb6cc2 100644
--- 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) )
+        {
+            *offset = poffset;
+            return nsize;
+        }
+    }
+
+    return -ENOENT;
+}
+
 /*
  * Property: reg
  *
-- 
2.30.2

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

On 2024-12-26 11: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.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
> Changes since v1:
> - moved common fdt functions to libfdt
> - rename prop_as_offset to more correct prop_by_offset

> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
> index 27d23df03af3..0e54aeeb6cc2 100644
> --- 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(

I think fdt_get_prop_offset() is a better name.  The point of this 
function is to return the offset in the fdt of the named property.  "by" 
or "as" confuses the purpose, at least to me.

Compare the existing fdt_get_property_by_offset() which is performing a 
property looking by consulting the offset.

Regards,
Jason