On 08.04.2025 18:07, Alejandro Vallejo wrote:
> --- 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;
Use max() instead of open-coding it?
> --- 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);
Somewhat orthogonal question: Should there perhaps be a way for the boot loader
provided cmdline to go at the tail of the DT provided one?
> --- 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);
> +}
What's the rationale for these to be separate functions, rather then the code
being integrated into their sole callers? Especially for the former the extra
layer feels excessive.
> --- 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);
No __init on declarations please.
> --- 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));
Why's the check lost for bd->kernel->cmdline_pa being non-zero?
> @@ -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
Same here.
> strlcpy(cmdline,
> - cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
> + cmdline_cook(__va(bd->kernel->cmdline_pa),bi->loader),
The change to this line is bogus altogether.
> --- 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);
> +
> + if ( ret < 0 )
> + continue;
> +
> + if ( strcmp(pname, name) == 0 )
> + {
> + *offset = poffset;
Variable naming looks backwards here.
Jan
