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

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Wed Apr 9, 2025 at 11:04 PM BST, Denis Mukhin wrote:
> On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@xxxxxxx> 
> 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.
>> 
>> Signed-off-by: Daniel P. Smith dpsmith@xxxxxxxxxxxxxxxxxxxx
>> 
>> Signed-off-by: Jason Andryuk jason.andryuk@xxxxxxx
>> 
>> ---
>> v3:
>> * Rename to fdt_get_prop_offset()
>> * Remove size_t cast from builder_get_cmdline_size()
>> * fdt_get_prop_offset() use strcmp()
>> * fdt_get_prop_offset() return bool
>> ---
>> xen/arch/x86/domain-builder/core.c | 28 +++++++++++++++++++++++
>> xen/arch/x86/domain-builder/fdt.c | 6 +++++
>> xen/arch/x86/domain-builder/fdt.h | 25 ++++++++++++++++++++
>> xen/arch/x86/include/asm/bootinfo.h | 6 +++--
>> xen/arch/x86/include/asm/domain-builder.h | 4 ++++
>> xen/arch/x86/setup.c | 12 +++++++---
>> xen/include/xen/libfdt/libfdt-xen.h | 23 +++++++++++++++++++
>> 7 files changed, 99 insertions(+), 5 deletions(-)
>> 
>> diff --git a/xen/arch/x86/domain-builder/core.c 
>> b/xen/arch/x86/domain-builder/core.c
>> index eda7fa7a8f..510a74a675 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;
>> +#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 a037c8b6cb..bc9903a9de 100644
>> --- 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);
>> 
>> }
>> }
>> 
>> diff --git a/xen/arch/x86/domain-builder/fdt.h 
>> b/xen/arch/x86/domain-builder/fdt.h
>> index e8769dc51c..91f665c8fd 100644
>> --- 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);
>> 
>> +}
>> +
>> int has_hyperlaunch_fdt(const 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 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/include/asm/domain-builder.h 
>> b/xen/arch/x86/include/asm/domain-builder.h
>> index b6d9ba94de..7518b6ddf3 100644
>> --- 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);
>> +
>> void builder_init(struct boot_info *bi);
>> 
>> #endif
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 00e8c8a2a3..ca4415d020 100644
>> --- 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));
>> 
>> 
>> if ( s == 0 )
>> return s;
>> @@ -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
>> strlcpy(cmdline,
>> - cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
>> 
>> + cmdline_cook(__va(bd->kernel->cmdline_pa),bi->loader),
>
> Add extra space before bi->loader?

Yes, was a spurious diff.

>
>> 
>> cmdline_size);
>> 
>> if ( bi->kextra )
>> 
>> diff --git a/xen/include/xen/libfdt/libfdt-xen.h 
>> b/xen/include/xen/libfdt/libfdt-xen.h
>> index 2259c09a6a..e473fbaf0c 100644
>> --- 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);
>
> Return value is not checked.

The pointer itself is ignored, but the error code is placed in "ret"
(when there is an error). Hence the ret < 0 that follows this.

>
>> +
>> + if ( ret < 0 )
>> + continue;
>> +
>> + if ( strcmp(pname, name) == 0 )
>
> I got an impression that the preferred form is 
>    if ( !strcmp(pname, name) )

It varies per file. Doesn't matter much, but sure. Will change.

>
>> + {
>> + offset = poffset;
>> + return true;
>> + }
>> + }
>> +
>> + return false;
>> +}
>> +
>> /
>> * Property: reg
>> *
>> --
>> 2.43.0

Cheers,
Alejandro