From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
Add support to read the command line from the hyperlaunch 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 over 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>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
---
xen/arch/x86/include/asm/bootinfo.h | 6 ++++--
xen/arch/x86/setup.c | 22 ++++++++++++++++++++--
xen/common/domain-builder/core.c | 26 ++++++++++++++++++++++++++
xen/common/domain-builder/fdt.c | 6 ++++++
xen/common/domain-builder/fdt.h | 24 ++++++++++++++++++++++++
xen/include/xen/domain-builder.h | 8 +++++---
xen/include/xen/libfdt/libfdt-xen.h | 23 +++++++++++++++++++++++
7 files changed, 108 insertions(+), 7 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 f8193f2c1c..765b690c41 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -985,7 +985,19 @@ 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;
+
+ /*
+ * Bootloader cmdline takes precedence and replaces the DT provided one.
+ *
+ * In that case, fdt_cmdline is not be populated at all.
+ */
+ if ( bd->kernel->fdt_cmdline )
+ {
+ BUG_ON(!IS_ENABLED(CONFIG_DOMAIN_BUILDER));
+ 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;
@@ -1049,7 +1061,13 @@ 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 )
+ {
+ BUG_ON(!IS_ENABLED(CONFIG_DOMAIN_BUILDER));
+ 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 c6917532be..b2f49bb17b 100644
--- a/xen/common/domain-builder/core.c
+++ b/xen/common/domain-builder/core.c
@@ -10,9 +10,35 @@
#include <xen/lib.h>
#include <asm/bootinfo.h>
+#include <asm/setup.h>
#include "fdt.h"
+size_t __init builder_get_cmdline_size(const struct boot_info *bi, int offset)
+{
+ const void *fdt;
+ int size;
+
+ 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(const struct boot_info *bi,
+ int offset, char *cmdline, size_t size)
+{
+ const void *fdt;
+ int ret;
+
+ fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+ ret = fdt_cmdline_prop_copy(fdt, offset, cmdline, size);
+ bootstrap_unmap();
+
+ return ret;
+}
+
enum fdt_kind __init builder_init(struct boot_info *bi)
{
enum fdt_kind kind;
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index cbb0ed30a2..dabe201b04 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -219,6 +219,12 @@ static int __init fdt_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 5570fb7a9c..68cbc42674 100644
--- a/xen/common/domain-builder/fdt.h
+++ b/xen/common/domain-builder/fdt.h
@@ -10,6 +10,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);
+
+ 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);
+}
+
enum fdt_kind fdt_detect_kind(const struct boot_info *bi);
int fdt_walk_hyperlaunch(struct boot_info *bi);
diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
index 3ac3a0ab84..350e2eb2af 100644
--- a/xen/include/xen/domain-builder.h
+++ b/xen/include/xen/domain-builder.h
@@ -2,6 +2,8 @@
#ifndef __XEN_DOMAIN_BUILDER_H__
#define __XEN_DOMAIN_BUILDER_H__
+#include <xen/types.h>
+
struct boot_info;
/* Return status of builder_init(). Shows which boot mechanism was detected */
@@ -28,8 +30,8 @@ static inline enum fdt_kind builder_init(struct boot_info *bi)
}
#endif /* !IS_ENABLED(CONFIG_DOMAIN_BUILDER) */
-int fdt_read_multiboot_module(const void *fdt, int node,
- int address_cells, int size_cells,
- struct boot_info *bi)
+size_t builder_get_cmdline_size(const struct boot_info *bi, int offset);
+int builder_get_cmdline(const struct boot_info *bi, int offset,
+ char *cmdline, size_t size);
#endif /* __XEN_DOMAIN_BUILDER_H__ */
diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
index a5340bc9f4..9c20a26a35 100644
--- a/xen/include/xen/libfdt/libfdt-xen.h
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -13,6 +13,29 @@
#include <xen/libfdt/libfdt.h>
+static inline bool __init fdt_get_prop_offset(
+ const void *fdt, int node, const char *pname, unsigned long *poffset)
+{
+ int ret, offset;
+ const char *name;
+
+ fdt_for_each_property_offset(offset, fdt, node)
+ {
+ fdt_getprop_by_offset(fdt, offset, &name, &ret);
+
+ if ( ret < 0 )
+ continue;
+
+ if ( !strcmp(name, pname) )
+ {
+ *poffset = offset;
+ return true;
+ }
+ }
+
+ return false;
+}
+
static inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
paddr_t *address,
paddr_t *size)
--
2.43.0On 2025-04-29 08:36, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> Add support to read the command line from the hyperlaunch 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 over 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>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> Reviewed-by: Denis Mukhin <dmukhin@ford.com>
> ---
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index cbb0ed30a2..dabe201b04 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -219,6 +219,12 @@ static int __init fdt_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] )
The logic is incorrect - it should be:
if ( !bd->kernel->cmdline_pa ||
!((char *)__va(bd->kernel->cmdline_pa))[0] )
If there is no cmdline_pa (which happens with the "reg" property) or the
if there is a 0-length string, then check the DT for bootargs.
This fixes the "reg" case to read from bootargs.
> + bd->kernel->fdt_cmdline = fdt_get_prop_offset(
> + fdt, node, "bootargs", &bd->kernel->cmdline_pa);
> }
> }
>
Just giving a heads up in case anyone is using it.
Regards,
Jason
On 09.06.2025 19:07, Jason Andryuk wrote: > On 2025-04-29 08:36, Alejandro Vallejo wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> Add support to read the command line from the hyperlaunch 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 over 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> >> Signed-off-by: Alejandro Vallejo <agarciav@amd.com> >> Reviewed-by: Denis Mukhin <dmukhin@ford.com> >> --- > >> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c >> index cbb0ed30a2..dabe201b04 100644 >> --- a/xen/common/domain-builder/fdt.c >> +++ b/xen/common/domain-builder/fdt.c >> @@ -219,6 +219,12 @@ static int __init fdt_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] ) > > The logic is incorrect - it should be: > > if ( !bd->kernel->cmdline_pa || > !((char *)__va(bd->kernel->cmdline_pa))[0] ) > > If there is no cmdline_pa (which happens with the "reg" property) or the if there is a 0-length string, then check the DT for bootargs. Even that sounds bogus to me: There's a difference between "no command line" and "empty command line". Jan
On 2025-06-10 02:56, Jan Beulich wrote: > On 09.06.2025 19:07, Jason Andryuk wrote: >> On 2025-04-29 08:36, Alejandro Vallejo wrote: >>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>> >>> Add support to read the command line from the hyperlaunch 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 over 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> >>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com> >>> Reviewed-by: Denis Mukhin <dmukhin@ford.com> >>> --- >> >>> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c >>> index cbb0ed30a2..dabe201b04 100644 >>> --- a/xen/common/domain-builder/fdt.c >>> +++ b/xen/common/domain-builder/fdt.c >>> @@ -219,6 +219,12 @@ static int __init fdt_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] ) >> >> The logic is incorrect - it should be: >> >> if ( !bd->kernel->cmdline_pa || >> !((char *)__va(bd->kernel->cmdline_pa))[0] ) >> >> If there is no cmdline_pa (which happens with the "reg" property) or the if there is a 0-length string, then check the DT for bootargs. > > Even that sounds bogus to me: There's a difference between "no command line" > and "empty command line". Yes, you have a point. The difficulty is grub always provides a NUL terminated string, so Xen can't differentiate the two. Regards, Jason
On 10.06.2025 19:39, Jason Andryuk wrote: > > > On 2025-06-10 02:56, Jan Beulich wrote: >> On 09.06.2025 19:07, Jason Andryuk wrote: >>> On 2025-04-29 08:36, Alejandro Vallejo wrote: >>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>> >>>> Add support to read the command line from the hyperlaunch 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 over 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> >>>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com> >>>> Reviewed-by: Denis Mukhin <dmukhin@ford.com> >>>> --- >>> >>>> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c >>>> index cbb0ed30a2..dabe201b04 100644 >>>> --- a/xen/common/domain-builder/fdt.c >>>> +++ b/xen/common/domain-builder/fdt.c >>>> @@ -219,6 +219,12 @@ static int __init fdt_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] ) >>> >>> The logic is incorrect - it should be: >>> >>> if ( !bd->kernel->cmdline_pa || >>> !((char *)__va(bd->kernel->cmdline_pa))[0] ) >>> >>> If there is no cmdline_pa (which happens with the "reg" property) or the if there is a 0-length string, then check the DT for bootargs. >> >> Even that sounds bogus to me: There's a difference between "no command line" >> and "empty command line". > > Yes, you have a point. The difficulty is grub always provides a NUL terminated string, so Xen can't differentiate the two. Which may call for either special-casing GrUB, or at least calling out that behavior in the comment. (Ideally we'd still have a way to distinguish between both cases, but likely we'll need to resort to documenting that some dummy option will need adding to tell "none" from [intended to be] empty.) Jan
On Wed Jun 11, 2025 at 7:35 AM CEST, Jan Beulich wrote: > On 10.06.2025 19:39, Jason Andryuk wrote: >> >> >> On 2025-06-10 02:56, Jan Beulich wrote: >>> On 09.06.2025 19:07, Jason Andryuk wrote: >>>> On 2025-04-29 08:36, Alejandro Vallejo wrote: >>>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>>> >>>>> Add support to read the command line from the hyperlaunch 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 over 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> >>>>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com> >>>>> Reviewed-by: Denis Mukhin <dmukhin@ford.com> >>>>> --- >>>> >>>>> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c >>>>> index cbb0ed30a2..dabe201b04 100644 >>>>> --- a/xen/common/domain-builder/fdt.c >>>>> +++ b/xen/common/domain-builder/fdt.c >>>>> @@ -219,6 +219,12 @@ static int __init fdt_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] ) >>>> >>>> The logic is incorrect - it should be: >>>> >>>> if ( !bd->kernel->cmdline_pa || >>>> !((char *)__va(bd->kernel->cmdline_pa))[0] ) >>>> >>>> If there is no cmdline_pa (which happens with the "reg" property) or the if there is a 0-length string, then check the DT for bootargs. >>> >>> Even that sounds bogus to me: There's a difference between "no command line" >>> and "empty command line". >> >> Yes, you have a point. The difficulty is grub always provides a NUL terminated string, so Xen can't differentiate the two. > > Which may call for either special-casing GrUB, or at least calling out that > behavior in the comment. (Ideally we'd still have a way to distinguish > between both cases, but likely we'll need to resort to documenting that some > dummy option will need adding to tell "none" from [intended to be] empty.) > > Jan We can add suitable comments where required, sure. About the dummy option, note that even if we have an option for Xen, that does nothing for the kernel cmdlines. If there's such dummy option there I don't know of it. Cheers, Alejandro
On 12.06.2025 10:20, Alejandro Vallejo wrote: > On Wed Jun 11, 2025 at 7:35 AM CEST, Jan Beulich wrote: >> On 10.06.2025 19:39, Jason Andryuk wrote: >>> >>> >>> On 2025-06-10 02:56, Jan Beulich wrote: >>>> On 09.06.2025 19:07, Jason Andryuk wrote: >>>>> On 2025-04-29 08:36, Alejandro Vallejo wrote: >>>>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>>>> >>>>>> Add support to read the command line from the hyperlaunch 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 over 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> >>>>>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com> >>>>>> Reviewed-by: Denis Mukhin <dmukhin@ford.com> >>>>>> --- >>>>> >>>>>> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c >>>>>> index cbb0ed30a2..dabe201b04 100644 >>>>>> --- a/xen/common/domain-builder/fdt.c >>>>>> +++ b/xen/common/domain-builder/fdt.c >>>>>> @@ -219,6 +219,12 @@ static int __init fdt_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] ) >>>>> >>>>> The logic is incorrect - it should be: >>>>> >>>>> if ( !bd->kernel->cmdline_pa || >>>>> !((char *)__va(bd->kernel->cmdline_pa))[0] ) >>>>> >>>>> If there is no cmdline_pa (which happens with the "reg" property) or the if there is a 0-length string, then check the DT for bootargs. >>>> >>>> Even that sounds bogus to me: There's a difference between "no command line" >>>> and "empty command line". >>> >>> Yes, you have a point. The difficulty is grub always provides a NUL terminated string, so Xen can't differentiate the two. >> >> Which may call for either special-casing GrUB, or at least calling out that >> behavior in the comment. (Ideally we'd still have a way to distinguish >> between both cases, but likely we'll need to resort to documenting that some >> dummy option will need adding to tell "none" from [intended to be] empty.) > > We can add suitable comments where required, sure. > > About the dummy option, note that even if we have an option for Xen, that does > nothing for the kernel cmdlines. If there's such dummy option there I don't know > of it. Indeed I meant we may need to introduce something. Jan
© 2016 - 2025 Red Hat, Inc.