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

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Fri Apr 18, 2025 at 11:53 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:29PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> 
>> 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

Oops. yes.

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

As in, also state the contents of the commit message in the function
header?

>
>> 
>> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
>> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
>> ---
>> 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?

I vaguely seem to remember doing something like that a few years ago
(because it does show intent and many linters require it) and being told
not to. But maybe I misremember. I'm definitely happy to use that
convention here and later unless someone pushes back for some reason.

Cheers,
Alejandro