| Message ID | 20250408160802.49870-11-agarciav@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Hyperlaunch device tree for dom0 | expand |
On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@amd.com> 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. > > Signed-off-by: Daniel P. Smith dpsmith@apertussolutions.com > > Signed-off-by: Jason Andryuk jason.andryuk@amd.com > > --- > 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? > > 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. > + > + if ( ret < 0 ) > + continue; > + > + if ( strcmp(pname, name) == 0 ) I got an impression that the preferred form is if ( !strcmp(pname, name) ) > + { > + offset = poffset; > + return true; > + } > + } > + > + return false; > +} > + > / > * Property: reg > * > -- > 2.43.0
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
On Thu Apr 10, 2025 at 12:12 PM BST, Jan Beulich wrote: > 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? Sure. On an unrelated matter, I've also removed the ifdef and relied on DCE for v4 for this and the next helper. > >> --- 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? That would preclude the bootloader fully overriding what's on the DT. One can always just copy the cmdline in the DT to the bootloader and adjust whatever is necessary there for testing. Adding append behaviour sounds more like a hindrance rather than helpful. To me at least. > >> --- 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. I've moved them onto domain-builder/fdt.c (where I believe they were originally?) for v4. > >> --- 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. Ack. > >> --- 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? Shouldn't have been, indeed. > >> @@ -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. Ack. > >> 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. Ugh, yes. > >> --- 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 I think the leading p stands for "property" (for the sake of giving it a name). But I see how that might be confusing when interpreted with a Hungarian notation lens. I'll invert it. It doesn't matter which is which, after all. Cheers, Alejandro
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@amd.com> 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. >> >> Signed-off-by: Daniel P. Smith dpsmith@apertussolutions.com >> >> Signed-off-by: Jason Andryuk jason.andryuk@amd.com >> >> --- >> 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
On 14.04.2025 16:23, Alejandro Vallejo wrote: > On Thu Apr 10, 2025 at 12:12 PM BST, Jan Beulich wrote: >> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>> --- 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? > > That would preclude the bootloader fully overriding what's on the DT. > One can always just copy the cmdline in the DT to the bootloader and > adjust whatever is necessary there for testing. Adding append behaviour > sounds more like a hindrance rather than helpful. To me at least. Well. This is why I have been pushing for all options to also have a "negative" form. This way you can override whatever specifically you need to override, without re-typing the entire (perhaps long) cmdline from DT. Also, I didn't mean that to necessarily be the one-and-only behavior. Jan
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), 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); + + if ( ret < 0 ) + continue; + + if ( strcmp(pname, name) == 0 ) + { + *offset = poffset; + return true; + } + } + + return false; +} + /* * Property: reg *