[PATCH v6 00/12] Hyperlaunch device tree for dom0

[PATCH v6 00/12] Hyperlaunch device tree for dom0

[Alejandro Vallejo]

Hi,

Not very many changes here. Just:

v5->v6:
  * Denis' suggestion to rename a few helpers to fdt_*
  * Change to last patch to only pass CDF_iommu to domains with
    DOMAIN_CAPS_HARDWARE.

I _think_ this addresses all feedback I got so far and I don't expect
anything major remaining before commit. If there's something I was asked
and I haven't delivered yet, please bring it up again.

v5: https://lore.kernel.org/xen-devel/20250424161027.92942-1-agarciav@amd.com/
v4: https://lore.kernel.org/xen-devel/20250417124844.11143-1-agarciav@amd.com/
v3: https://lore.kernel.org/xen-devel/20250408160802.49870-1-agarciav@amd.com/
v2: https://lore.kernel.org/xen-devel/20241226165740.29812-1-dpsmith@apertussolutions.com/
v1: https://lore.kernel.org/xen-devel/20241123182044.30687-1-dpsmith@apertussolutions.com/

========= Original cover letter:

The Hyperlaunch device tree for dom0 series is the second split out for the
introduction of the Hyperlaunch domain builder logic. These changes focus on
introducing the ability to express a domain configuration that is then used to
populate the struct boot_domain structure for dom0. This ability to express a
domain configuration provides the next step towards a general domain builder.

The splitting of Hyperlaunch into a set of series are twofold, to reduce the
effort in reviewing a much larger series, and to reduce the effort in handling
the knock-on effects to the construction logic from requested review changes.

Alejandro Vallejo (1):
  x86/hyperlaunch: Add helpers to locate multiboot modules

Daniel P. Smith (11):
  kconfig: introduce CONFIG_DOMAIN_BUILDER
  common/hyperlaunch: introduce the domain builder
  x86/hyperlaunch: initial support for hyperlaunch device tree
  x86/hyperlaunch: locate dom0 kernel with hyperlaunch
  x86/hyperlaunch: obtain cmdline from device tree
  x86/hyperlaunch: locate dom0 initrd with hyperlaunch
  x86/hyperlaunch: add domain id parsing to domain config
  x86/hyperlaunch: specify dom0 mode with device tree
  x86/hyperlaunch: add memory parsing to domain config
  x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree
  x86/hyperlaunch: add capabilities to boot domain

 xen/arch/x86/dom0_build.c              |  11 +
 xen/arch/x86/include/asm/boot-domain.h |  14 +
 xen/arch/x86/include/asm/bootinfo.h    |  10 +-
 xen/arch/x86/setup.c                   |  66 +++-
 xen/common/Kconfig                     |   2 +
 xen/common/Makefile                    |   1 +
 xen/common/domain-builder/Kconfig      |  15 +
 xen/common/domain-builder/Makefile     |   2 +
 xen/common/domain-builder/core.c       |  86 +++++
 xen/common/domain-builder/fdt.c        | 512 +++++++++++++++++++++++++
 xen/common/domain-builder/fdt.h        |  40 ++
 xen/include/xen/domain-builder.h       |  37 ++
 xen/include/xen/libfdt/libfdt-xen.h    |  23 ++
 13 files changed, 801 insertions(+), 18 deletions(-)
 create mode 100644 xen/common/domain-builder/Kconfig
 create mode 100644 xen/common/domain-builder/Makefile
 create mode 100644 xen/common/domain-builder/core.c
 create mode 100644 xen/common/domain-builder/fdt.c
 create mode 100644 xen/common/domain-builder/fdt.h
 create mode 100644 xen/include/xen/domain-builder.h

-- 
2.43.0

[PATCH v6 01/12] kconfig: introduce CONFIG_DOMAIN_BUILDER

[Alejandro Vallejo]

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

Hyperlaunch domain builder will be the consolidated boot time domain
building logic framework. Introduce the config option to enable this
domain builder to eventually turn on the ability to load the domain
configuration via a flattened device tree.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
 xen/common/Kconfig                |  2 ++
 xen/common/domain-builder/Kconfig | 15 +++++++++++++++
 2 files changed, 17 insertions(+)
 create mode 100644 xen/common/domain-builder/Kconfig

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index be28060716..e025fbe257 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -144,6 +144,8 @@ config STATIC_MEMORY
 
 	  If unsure, say N.
 
+source "common/domain-builder/Kconfig"
+
 menu "Speculative hardening"
 
 config INDIRECT_THUNK
diff --git a/xen/common/domain-builder/Kconfig b/xen/common/domain-builder/Kconfig
new file mode 100644
index 0000000000..44b8351af8
--- /dev/null
+++ b/xen/common/domain-builder/Kconfig
@@ -0,0 +1,15 @@
+menu "Domain Builder Features"
+
+config DOMAIN_BUILDER
+	bool "Domain builder (UNSUPPORTED)" if UNSUPPORTED && X86
+	select LIBFDT
+	help
+	  Support for constructing predefined domains described by a flattened
+	  device tree. This allows constructing multiple domains at boot time
+	  instead of being limited to a single dom0.
+
+	  This feature is currently experimental.
+
+	  If unsure, say N.
+
+endmenu
-- 
2.43.0

[PATCH v6 02/12] common/hyperlaunch: introduce the domain builder

[Alejandro Vallejo]

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

Introduce the domain builder which is capable of consuming a device tree as the
first boot module. If it finds a device tree as the first boot module, it will
set its type to BOOTMOD_FDT. This change only detects the boot module and
continues to boot with slight change to the boot convention that the dom0
kernel is no longer first boot module but is the second.

No functional change intended.

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 |  3 ++
 xen/arch/x86/setup.c                | 19 ++++++++----
 xen/common/Makefile                 |  1 +
 xen/common/domain-builder/Makefile  |  2 ++
 xen/common/domain-builder/core.c    | 45 +++++++++++++++++++++++++++++
 xen/common/domain-builder/fdt.c     | 39 +++++++++++++++++++++++++
 xen/common/domain-builder/fdt.h     | 14 +++++++++
 xen/include/xen/domain-builder.h    | 29 +++++++++++++++++++
 8 files changed, 146 insertions(+), 6 deletions(-)
 create mode 100644 xen/common/domain-builder/Makefile
 create mode 100644 xen/common/domain-builder/core.c
 create mode 100644 xen/common/domain-builder/fdt.c
 create mode 100644 xen/common/domain-builder/fdt.h
 create mode 100644 xen/include/xen/domain-builder.h

diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 3afc214c17..82c2650fcf 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -27,6 +27,7 @@ enum bootmod_type {
     BOOTMOD_RAMDISK,
     BOOTMOD_MICROCODE,
     BOOTMOD_XSM_POLICY,
+    BOOTMOD_FDT,
 };
 
 struct boot_module {
@@ -80,6 +81,8 @@ struct boot_info {
     paddr_t memmap_addr;
     size_t memmap_length;
 
+    bool hyperlaunch_enabled;
+
     unsigned int nr_modules;
     struct boot_module mods[MAX_NR_BOOTMODS + 1];
     struct boot_domain domains[MAX_NR_BOOTDOMS];
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2518954124..f3b5c83a3c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -6,6 +6,7 @@
 #include <xen/cpuidle.h>
 #include <xen/dmi.h>
 #include <xen/domain.h>
+#include <xen/domain-builder.h>
 #include <xen/domain_page.h>
 #include <xen/efi.h>
 #include <xen/err.h>
@@ -1284,9 +1285,14 @@ void asmlinkage __init noreturn __start_xen(void)
                bi->nr_modules);
     }
 
-    /* Dom0 kernel is always first */
-    bi->mods[0].type = BOOTMOD_KERNEL;
-    bi->domains[0].kernel = &bi->mods[0];
+    if ( builder_init(bi) == FDT_KIND_NONE )
+    {
+        /* Find first unknown boot module to use as dom0 kernel */
+        i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
+        bi->mods[i].type = BOOTMOD_KERNEL;
+        bi->domains[0].kernel = &bi->mods[i];
+        bi->hyperlaunch_enabled = false;
+    }
 
     if ( pvh_boot )
     {
@@ -1469,8 +1475,9 @@ void asmlinkage __init noreturn __start_xen(void)
         xen->size  = __2M_rwdata_end - _stext;
     }
 
-    bi->mods[0].headroom =
-        bzimage_headroom(bootstrap_map_bm(&bi->mods[0]), bi->mods[0].size);
+    bi->domains[0].kernel->headroom =
+        bzimage_headroom(bootstrap_map_bm(bi->domains[0].kernel),
+                         bi->domains[0].kernel->size);
     bootstrap_unmap();
 
 #ifndef highmem_start
@@ -1594,7 +1601,7 @@ void asmlinkage __init noreturn __start_xen(void)
 #endif
     }
 
-    if ( bi->mods[0].headroom && !bi->mods[0].relocated )
+    if ( bi->domains[0].kernel->headroom && !bi->domains[0].kernel->relocated )
         panic("Not enough memory to relocate the dom0 kernel image\n");
     for ( i = 0; i < bi->nr_modules; ++i )
     {
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 98f0873056..e42af71e3f 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
 obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
 obj-$(CONFIG_IOREQ_SERVER) += dm.o
 obj-y += domain.o
+obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/
 obj-y += event_2l.o
 obj-y += event_channel.o
 obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
diff --git a/xen/common/domain-builder/Makefile b/xen/common/domain-builder/Makefile
new file mode 100644
index 0000000000..bfd2f6267e
--- /dev/null
+++ b/xen/common/domain-builder/Makefile
@@ -0,0 +1,2 @@
+obj-y += fdt.init.o
+obj-y += core.init.o
diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
new file mode 100644
index 0000000000..97c92db571
--- /dev/null
+++ b/xen/common/domain-builder/core.c
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024, Apertus Solutions, LLC
+ */
+#include <xen/bug.h>
+#include <xen/err.h>
+#include <xen/init.h>
+#include <xen/kconfig.h>
+#include <xen/domain-builder.h>
+#include <xen/lib.h>
+
+#include <asm/bootinfo.h>
+
+#include "fdt.h"
+
+enum fdt_kind __init builder_init(struct boot_info *bi)
+{
+    enum fdt_kind kind;
+
+    bi->hyperlaunch_enabled = false;
+    switch ( (kind = fdt_detect_kind(bi)) )
+    {
+    case FDT_KIND_NONE:
+        /* No DT found */
+        return kind;
+
+    case FDT_KIND_UNKNOWN:
+        printk(XENLOG_DEBUG "DT found: non-hyperlaunch\n");
+        bi->mods[0].type = BOOTMOD_FDT;
+        return kind;
+
+    default:
+        BUG();
+    }
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
new file mode 100644
index 0000000000..4b07bd22c8
--- /dev/null
+++ b/xen/common/domain-builder/fdt.c
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024, Apertus Solutions, LLC
+ */
+#include <xen/err.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/libfdt/libfdt.h>
+
+#include <asm/bootinfo.h>
+#include <asm/page.h>
+#include <asm/setup.h>
+
+#include "fdt.h"
+
+enum fdt_kind __init fdt_detect_kind(const struct boot_info *bi)
+{
+    enum fdt_kind kind;
+    const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+
+    if ( !fdt || fdt_check_header(fdt) < 0 )
+        kind = FDT_KIND_NONE;
+    else
+        kind = FDT_KIND_UNKNOWN;
+
+    bootstrap_unmap();
+
+    return kind;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
new file mode 100644
index 0000000000..ef897fc412
--- /dev/null
+++ b/xen/common/domain-builder/fdt.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __XEN_DOMAIN_BUILDER_FDT_H__
+#define __XEN_DOMAIN_BUILDER_FDT_H__
+
+#include <xen/domain-builder.h>
+
+struct boot_info;
+
+/* hyperlaunch fdt is required to be module 0 */
+#define HYPERLAUNCH_MODULE_IDX 0
+
+enum fdt_kind fdt_detect_kind(const struct boot_info *bi);
+
+#endif /* __XEN_DOMAIN_BUILDER_FDT_H__ */
diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
new file mode 100644
index 0000000000..b9702db735
--- /dev/null
+++ b/xen/include/xen/domain-builder.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __XEN_DOMAIN_BUILDER_H__
+#define __XEN_DOMAIN_BUILDER_H__
+
+struct boot_info;
+
+/* Return status of builder_init(). Shows which boot mechanism was detected */
+enum fdt_kind
+{
+    /* FDT not found. Skipped builder. */
+    FDT_KIND_NONE,
+    /* Found an FDT that wasn't hyperlaunch. */
+    FDT_KIND_UNKNOWN,
+};
+
+/*
+ * Initialises `bi` if it detects a compatible FDT. Otherwise returns
+ * FDT_KIND_NONE and leaves initialisation up to the caller.
+ */
+#if IS_ENABLED(CONFIG_DOMAIN_BUILDER)
+enum fdt_kind builder_init(struct boot_info *bi);
+#else
+static inline enum fdt_kind builder_init(struct boot_info *bi)
+{
+    return FDT_KIND_NONE;
+}
+#endif /* !IS_ENABLED(CONFIG_DOMAIN_BUILDER) */
+
+#endif /* __XEN_DOMAIN_BUILDER_H__ */
-- 
2.43.0

[PATCH v6 03/12] x86/hyperlaunch: initial support for hyperlaunch device tree

[Alejandro Vallejo]

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

Add the ability to detect both a formal hyperlaunch device tree or a dom0less
device tree. If the hyperlaunch device tree is found, then count the number of
domain entries, reporting an error if more than one is found.

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 |  1 +
 xen/common/domain-builder/core.c    | 15 +++++++
 xen/common/domain-builder/fdt.c     | 62 +++++++++++++++++++++++++++++
 xen/common/domain-builder/fdt.h     |  1 +
 xen/include/xen/domain-builder.h    |  2 +
 5 files changed, 81 insertions(+)

diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 82c2650fcf..1e3d582e45 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -84,6 +84,7 @@ struct boot_info {
     bool hyperlaunch_enabled;
 
     unsigned int nr_modules;
+    unsigned int nr_domains;
     struct boot_module mods[MAX_NR_BOOTMODS + 1];
     struct boot_domain domains[MAX_NR_BOOTDOMS];
 };
diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
index 97c92db571..c6917532be 100644
--- a/xen/common/domain-builder/core.c
+++ b/xen/common/domain-builder/core.c
@@ -16,10 +16,17 @@
 enum fdt_kind __init builder_init(struct boot_info *bi)
 {
     enum fdt_kind kind;
+    int ret;
 
     bi->hyperlaunch_enabled = false;
     switch ( (kind = fdt_detect_kind(bi)) )
     {
+    case FDT_KIND_HYPERLAUNCH:
+        printk(XENLOG_DEBUG "DT found: hyperlaunch\n");
+        bi->hyperlaunch_enabled = true;
+        bi->mods[0].type = BOOTMOD_FDT;
+        break;
+
     case FDT_KIND_NONE:
         /* No DT found */
         return kind;
@@ -32,6 +39,14 @@ enum fdt_kind __init builder_init(struct boot_info *bi)
     default:
         BUG();
     }
+
+    printk(XENLOG_INFO "Hyperlaunch configuration:\n");
+    if ( (ret = fdt_walk_hyperlaunch(bi)) < 0 )
+        panic("Walk of device tree failed (%d)\n", ret);
+
+    printk(XENLOG_INFO "  number of domains: %u\n", bi->nr_domains);
+
+    return FDT_KIND_HYPERLAUNCH;
 }
 
 /*
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 4b07bd22c8..94ccff61e2 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -13,6 +13,36 @@
 
 #include "fdt.h"
 
+static int __init find_hyperlaunch_node(const void *fdt)
+{
+    int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
+
+    if ( hv_node >= 0 )
+    {
+        /* Anything other than zero indicates no match */
+        if ( fdt_node_check_compatible(fdt, hv_node, "hypervisor,xen") )
+            return -ENODATA;
+
+        return hv_node;
+    }
+    else
+    {
+        /* Look for dom0less config */
+        int node, chosen_node = fdt_path_offset(fdt, "/chosen");
+
+        if ( chosen_node < 0 )
+            return -ENOENT;
+
+        fdt_for_each_subnode(node, fdt, chosen_node)
+        {
+            if ( !fdt_node_check_compatible(fdt, node, "xen,domain") )
+                return chosen_node;
+        }
+    }
+
+    return -ENODATA;
+}
+
 enum fdt_kind __init fdt_detect_kind(const struct boot_info *bi)
 {
     enum fdt_kind kind;
@@ -20,6 +50,8 @@ enum fdt_kind __init fdt_detect_kind(const struct boot_info *bi)
 
     if ( !fdt || fdt_check_header(fdt) < 0 )
         kind = FDT_KIND_NONE;
+    else if ( find_hyperlaunch_node(fdt) >= 0 )
+        kind = FDT_KIND_HYPERLAUNCH;
     else
         kind = FDT_KIND_UNKNOWN;
 
@@ -28,6 +60,36 @@ enum fdt_kind __init fdt_detect_kind(const struct boot_info *bi)
     return kind;
 }
 
+int __init fdt_walk_hyperlaunch(struct boot_info *bi)
+{
+    int ret = 0, hv_node, node;
+    const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+
+    BUG_ON(!fdt);
+
+    hv_node = find_hyperlaunch_node(fdt);
+    if ( hv_node < 0 )
+    {
+        ret = hv_node;
+        goto err_out;
+    }
+
+    fdt_for_each_subnode(node, fdt, hv_node)
+    {
+        if ( !fdt_node_check_compatible(fdt, node, "xen,domain") )
+            bi->nr_domains++;
+    }
+
+    /* Until multi-domain construction is added, throw an error */
+    if ( bi->nr_domains != 1 )
+        printk(XENLOG_ERR "hyperlaunch only supports dom0 construction\n");
+
+ err_out:
+    bootstrap_unmap();
+
+    return ret;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
index ef897fc412..d1bcc23fa2 100644
--- a/xen/common/domain-builder/fdt.h
+++ b/xen/common/domain-builder/fdt.h
@@ -10,5 +10,6 @@ struct boot_info;
 #define HYPERLAUNCH_MODULE_IDX 0
 
 enum fdt_kind fdt_detect_kind(const struct boot_info *bi);
+int fdt_walk_hyperlaunch(struct boot_info *bi);
 
 #endif /* __XEN_DOMAIN_BUILDER_FDT_H__ */
diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
index b9702db735..cbb3cbea7c 100644
--- a/xen/include/xen/domain-builder.h
+++ b/xen/include/xen/domain-builder.h
@@ -9,6 +9,8 @@ enum fdt_kind
 {
     /* FDT not found. Skipped builder. */
     FDT_KIND_NONE,
+    /* Found Hyperlaunch FDT */
+    FDT_KIND_HYPERLAUNCH,
     /* Found an FDT that wasn't hyperlaunch. */
     FDT_KIND_UNKNOWN,
 };
-- 
2.43.0

[PATCH v6 04/12] x86/hyperlaunch: Add helpers to locate multiboot modules

[Alejandro Vallejo]

Hyperlaunch mandates either a reg or module-index DT prop on nodes that
contain `multiboot,module" under their "compatible" prop. This patch
introduces a helper to generically find such index, appending the module
to the list of modules if it wasn't already (i.e: because it's given via
the "reg" prop).

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/common/domain-builder/fdt.c  | 172 +++++++++++++++++++++++++++++++
 xen/common/domain-builder/fdt.h  |   1 +
 xen/include/xen/domain-builder.h |   4 +
 3 files changed, 177 insertions(+)

diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 94ccff61e2..05ac3647cf 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -13,6 +13,178 @@
 
 #include "fdt.h"
 
+static int __init fdt_prop_as_u32(const struct fdt_property *prop,
+                                  uint32_t *val)
+{
+    if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u32) )
+        return -EINVAL;
+
+    *val = fdt32_to_cpu(*(const fdt32_t *)prop->data);
+    return 0;
+}
+
+/*
+ * Unpacks a "reg" property into its address and size constituents.
+ *
+ * @param prop          Pointer to an FDT "reg" property.
+ * @param address_cells Number of 4-octet cells that make up an "address".
+ * @param size_cells    Number of 4-octet cells that make up a "size".
+ * @param p_addr[out]   Address encoded in the property.
+ * @param p_size[out]   Size encoded in the property.
+ * @returns             -EINVAL on malformed property, 0 otherwise.
+ */
+static int __init fdt_prop_as_reg(const struct fdt_property *prop,
+                                  int address_cells, int size_cells,
+                                  uint64_t *p_addr, uint64_t *p_size)
+{
+    const fdt32_t *cell = (const fdt32_t *)prop->data;
+    uint64_t addr, size;
+
+    if ( fdt32_to_cpu(prop->len) !=
+         (address_cells + size_cells) * sizeof(*cell) )
+    {
+        printk(XENLOG_ERR "  cannot read reg %lu+%lu from prop len %u\n",
+            address_cells * sizeof(*cell), size_cells * sizeof(*cell),
+            fdt32_to_cpu(prop->len));
+        return -EINVAL;
+    }
+
+    switch ( address_cells )
+    {
+    case 1:
+        addr = fdt32_to_cpu(*cell);
+        break;
+    case 2:
+        addr = fdt64_to_cpu(*(const fdt64_t *)cell);
+        break;
+    default:
+        printk(XENLOG_ERR "  unsupported address_cells=%d\n", address_cells);
+        return -EINVAL;
+    }
+
+    cell += address_cells;
+    switch ( size_cells )
+    {
+    case 1:
+        size = fdt32_to_cpu(*cell);
+        break;
+    case 2:
+        size = fdt64_to_cpu(*(const fdt64_t *)cell);
+        break;
+    default:
+        printk(XENLOG_ERR "  unsupported size_cells=%d\n", size_cells);
+        return -EINVAL;
+    }
+
+    *p_addr = addr;
+    *p_size = size;
+
+    return 0;
+}
+
+/*
+ * Locate a multiboot module given its node offset in the FDT.
+ *
+ * The module location may be given via either FDT property:
+ *     * reg = <address, size>
+ *         * Mutates `bi` to append the module.
+ *     * module-index = <idx>
+ *         * Leaves `bi` unchanged.
+ *
+ * @param fdt           Pointer to the full FDT.
+ * @param node          Offset for the module node.
+ * @param address_cells Number of 4-octet cells that make up an "address".
+ * @param size_cells    Number of 4-octet cells that make up a "size".
+ * @param bi[inout]     Xen's representation of the boot parameters.
+ * @return              -EINVAL on malformed nodes, otherwise
+ *                      index inside `bi->mods`
+ */
+int __init fdt_read_multiboot_module(const void *fdt, int node,
+                                     int address_cells, int size_cells,
+                                     struct boot_info *bi)
+{
+    const struct fdt_property *prop;
+    uint64_t addr, size;
+    int ret;
+    uint32_t idx;
+
+    if ( fdt_node_check_compatible(fdt, node, "multiboot,module") )
+    {
+        printk(XENLOG_ERR "  bad module. multiboot,module not found");
+        return -ENODATA;
+    }
+
+    /* Location given as a `module-index` property. */
+    if ( (prop = fdt_get_property(fdt, node, "module-index", NULL)) )
+    {
+        if ( fdt_get_property(fdt, node, "reg", NULL) )
+        {
+            printk(XENLOG_ERR "  found both reg and module-index for module\n");
+            return -EINVAL;
+        }
+        if ( (ret = fdt_prop_as_u32(prop, &idx)) )
+        {
+            printk(XENLOG_ERR "  bad module-index prop\n");
+            return ret;
+        }
+        if ( idx >= MAX_NR_BOOTMODS )
+        {
+            printk(XENLOG_ERR "  module-index overflow. %s=%u\n",
+                   STR(MAX_NR_BOOTMODS), MAX_NR_BOOTMODS);
+            return -EINVAL;
+        }
+
+        return idx;
+    }
+
+    /* Otherwise location given as a `reg` property. */
+    if ( !(prop = fdt_get_property(fdt, node, "reg", NULL)) )
+    {
+        printk(XENLOG_ERR "  no location for multiboot,module\n");
+        return -EINVAL;
+    }
+    if ( fdt_get_property(fdt, node, "module-index", NULL) )
+    {
+        printk(XENLOG_ERR "  found both reg and module-index for module\n");
+        return -EINVAL;
+    }
+
+    ret = fdt_prop_as_reg(prop, address_cells, size_cells, &addr, &size);
+    if ( ret < 0 )
+    {
+        printk(XENLOG_ERR "  failed reading reg for multiboot,module\n");
+        return -EINVAL;
+    }
+
+    idx = bi->nr_modules;
+    if ( idx > MAX_NR_BOOTMODS )
+    {
+        /*
+         * MAX_NR_BOOTMODS must fit in 31 bits so it's representable in the
+         * positive side of an int; for the return value.
+         */
+        BUILD_BUG_ON(MAX_NR_BOOTMODS > (uint64_t)INT_MAX);
+        printk(XENLOG_ERR "  idx=%u exceeds len=%u\n", idx, MAX_NR_BOOTMODS);
+        return -EINVAL;
+    }
+
+    /*
+     * Append new module to the existing list
+     *
+     * Note that bi->nr_modules points to Xen itself, so we must shift it first
+     */
+    bi->nr_modules++;
+    bi->mods[bi->nr_modules] = bi->mods[idx];
+    bi->mods[idx] = (struct boot_module){
+        .start = addr,
+        .size = size,
+    };
+
+    printk(XENLOG_INFO "  module[%u]: addr %lx size %lx\n", idx, addr, size);
+
+    return idx;
+}
+
 static int __init find_hyperlaunch_node(const void *fdt)
 {
     int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
index d1bcc23fa2..5570fb7a9c 100644
--- a/xen/common/domain-builder/fdt.h
+++ b/xen/common/domain-builder/fdt.h
@@ -3,6 +3,7 @@
 #define __XEN_DOMAIN_BUILDER_FDT_H__
 
 #include <xen/domain-builder.h>
+#include <xen/libfdt/libfdt-xen.h>
 
 struct boot_info;
 
diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
index cbb3cbea7c..3ac3a0ab84 100644
--- a/xen/include/xen/domain-builder.h
+++ b/xen/include/xen/domain-builder.h
@@ -28,4 +28,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)
+
 #endif /* __XEN_DOMAIN_BUILDER_H__ */
-- 
2.43.0

[PATCH v6 05/12] x86/hyperlaunch: locate dom0 kernel with hyperlaunch

[Alejandro Vallejo]

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

Look for a subnode of type `multiboot,kernel` within a domain node. If
found, locate it using the multiboot module helper to generically ensure
it lives in the module list. If the bootargs property is present and
there was not an MB1 string, then use the command line from the device
tree definition.

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/setup.c            |  1 +
 xen/common/domain-builder/fdt.c | 64 +++++++++++++++++++++++++++++++--
 2 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f3b5c83a3c..f8193f2c1c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1291,6 +1291,7 @@ void asmlinkage __init noreturn __start_xen(void)
         i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
         bi->mods[i].type = BOOTMOD_KERNEL;
         bi->domains[0].kernel = &bi->mods[i];
+        bi->nr_domains = 1;
         bi->hyperlaunch_enabled = false;
     }
 
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 05ac3647cf..cbb0ed30a2 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -99,9 +99,9 @@ static int __init fdt_prop_as_reg(const struct fdt_property *prop,
  * @return              -EINVAL on malformed nodes, otherwise
  *                      index inside `bi->mods`
  */
-int __init fdt_read_multiboot_module(const void *fdt, int node,
-                                     int address_cells, int size_cells,
-                                     struct boot_info *bi)
+static int __init fdt_read_multiboot_module(const void *fdt, int node,
+                                            int address_cells, int size_cells,
+                                            struct boot_info *bi)
 {
     const struct fdt_property *prop;
     uint64_t addr, size;
@@ -185,6 +185,52 @@ int __init fdt_read_multiboot_module(const void *fdt, int node,
     return idx;
 }
 
+static int __init fdt_process_domain_node(
+    struct boot_info *bi, const void *fdt, int dom_node)
+{
+    int node;
+    struct boot_domain *bd = &bi->domains[bi->nr_domains];
+    const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown";
+    int address_cells = fdt_address_cells(fdt, dom_node);
+    int size_cells = fdt_size_cells(fdt, dom_node);
+
+    fdt_for_each_subnode(node, fdt, dom_node)
+    {
+        if ( !fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
+        {
+            int idx;
+
+            if ( bd->kernel )
+            {
+                printk(XENLOG_WARNING
+                       "  duplicate kernel for domain %s\n", name);
+                continue;
+            }
+
+            idx = fdt_read_multiboot_module(fdt, node, address_cells,
+                                            size_cells, bi);
+            if ( idx < 0 )
+            {
+                printk(XENLOG_ERR
+                       "  failed processing kernel for domain %s\n", name);
+                return idx;
+            }
+
+            printk(XENLOG_INFO "  kernel: multiboot-index=%d\n", idx);
+            bi->mods[idx].type = BOOTMOD_KERNEL;
+            bd->kernel = &bi->mods[idx];
+        }
+    }
+
+    if ( !bd->kernel )
+    {
+        printk(XENLOG_ERR "error: no kernel assigned to domain %s\n", name);
+        return -ENODATA;
+    }
+
+    return 0;
+}
+
 static int __init find_hyperlaunch_node(const void *fdt)
 {
     int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
@@ -248,8 +294,20 @@ int __init fdt_walk_hyperlaunch(struct boot_info *bi)
 
     fdt_for_each_subnode(node, fdt, hv_node)
     {
+        if ( bi->nr_domains >= MAX_NR_BOOTDOMS )
+        {
+            printk(XENLOG_WARNING "warning: only creating first %u domains\n",
+                   MAX_NR_BOOTDOMS);
+            break;
+        }
+
         if ( !fdt_node_check_compatible(fdt, node, "xen,domain") )
+        {
+            if ( (ret = fdt_process_domain_node(bi, fdt, node)) < 0 )
+                break;
+
             bi->nr_domains++;
+        }
     }
 
     /* Until multi-domain construction is added, throw an error */
-- 
2.43.0

[PATCH v6 06/12] x86/hyperlaunch: obtain cmdline from device tree

[Alejandro Vallejo]

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.0

[PATCH v6 07/12] x86/hyperlaunch: locate dom0 initrd with hyperlaunch

[Alejandro Vallejo]

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

Look for a subnode of type `multiboot,ramdisk` within a domain node and
parse via the fdt_read_multiboot_module() helper. After a successful
helper call, the module index is returned and the module is guaranteed
to be in the module list.

Fix unused typo in adjacent comment.

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/setup.c            |  4 ++--
 xen/common/domain-builder/fdt.c | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 765b690c41..c8de028439 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -2172,11 +2172,11 @@ void asmlinkage __init noreturn __start_xen(void)
      * At this point all capabilities that consume boot modules should have
      * claimed their boot modules. Find the first unclaimed boot module and
      * claim it as the initrd ramdisk. Do a second search to see if there are
-     * any remaining unclaimed boot modules, and report them as unusued initrd
+     * any remaining unclaimed boot modules, and report them as unused initrd
      * candidates.
      */
     initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
-    if ( initrdidx < MAX_NR_BOOTMODS )
+    if ( !bi->hyperlaunch_enabled && initrdidx < MAX_NR_BOOTMODS )
     {
         bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
         bi->domains[0].module = &bi->mods[initrdidx];
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index dabe201b04..507f383f8e 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -226,6 +226,30 @@ static int __init fdt_process_domain_node(
                 bd->kernel->fdt_cmdline = fdt_get_prop_offset(
                     fdt, node, "bootargs", &bd->kernel->cmdline_pa);
         }
+        else if ( !fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") )
+        {
+            int idx;
+
+            if ( bd->module )
+            {
+                printk(XENLOG_WARNING
+                       "Duplicate module for domain %s\n", name);
+                continue;
+            }
+
+            idx = fdt_read_multiboot_module(fdt, node, address_cells,
+                                            size_cells, bi);
+            if ( idx < 0 )
+            {
+                printk(XENLOG_ERR "  failed processing module for domain %s\n",
+                       name);
+                return idx;
+            }
+
+            printk(XENLOG_INFO "  module: multiboot-index=%d\n", idx);
+            bi->mods[idx].type = BOOTMOD_RAMDISK;
+            bd->module = &bi->mods[idx];
+        }
     }
 
     if ( !bd->kernel )
-- 
2.43.0

[PATCH v6 08/12] x86/hyperlaunch: add domain id parsing to domain config

[Alejandro Vallejo]

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

Introduce the ability to specify the desired domain id for the domain
definition. The domain id will be populated in the domid property of the
domain node in the device tree configuration.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
 xen/arch/x86/setup.c            |  5 ++--
 xen/common/domain-builder/fdt.c | 52 ++++++++++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c8de028439..10ff67ac37 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1043,8 +1043,9 @@ static struct domain *__init create_dom0(struct boot_info *bi)
     if ( iommu_enabled )
         dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
 
-    /* Create initial domain.  Not d0 for pvshim. */
-    bd->domid = get_initial_domain_id();
+    if ( bd->domid == DOMID_INVALID )
+        /* Create initial domain.  Not d0 for pvshim. */
+        bd->domid = get_initial_domain_id();
     d = domain_create(bd->domid, &dom0_cfg,
                       pv_shim ? 0 : CDF_privileged | CDF_hardware);
     if ( IS_ERR(d) )
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 507f383f8e..2c05b0a22d 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (C) 2024, Apertus Solutions, LLC
  */
+#include <xen/domain.h>
 #include <xen/err.h>
 #include <xen/init.h>
 #include <xen/lib.h>
@@ -188,12 +189,54 @@ static int __init fdt_read_multiboot_module(const void *fdt, int node,
 static int __init fdt_process_domain_node(
     struct boot_info *bi, const void *fdt, int dom_node)
 {
-    int node;
+    int node, property;
     struct boot_domain *bd = &bi->domains[bi->nr_domains];
     const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown";
     int address_cells = fdt_address_cells(fdt, dom_node);
     int size_cells = fdt_size_cells(fdt, dom_node);
 
+    fdt_for_each_property_offset(property, fdt, dom_node)
+    {
+        const struct fdt_property *prop;
+        const char *prop_name;
+        int name_len, rc;
+
+        prop = fdt_get_property_by_offset(fdt, property, NULL);
+        if ( !prop )
+            continue; /* silently skip */
+
+        prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &name_len);
+        if ( !strncmp(prop_name, "domid", name_len) )
+        {
+            uint32_t val = DOMID_INVALID;
+
+            if ( (rc = fdt_prop_as_u32(prop, &val)) )
+            {
+                printk(XENLOG_ERR
+                       "  failed processing domain id for domain %s\n", name);
+                return rc;
+            }
+
+            if ( val >= DOMID_FIRST_RESERVED )
+            {
+                printk(XENLOG_ERR "  invalid domain id for domain %s\n", name);
+                return -EINVAL;
+            }
+
+            for ( unsigned int i = 0; i < bi->nr_domains; i++ )
+            {
+                if ( bi->domains[i].domid == val )
+                {
+                    printk(XENLOG_ERR "  duplicate id for domain %s\n", name);
+                    return -EINVAL;
+                }
+            }
+
+            bd->domid = val;
+            printk(XENLOG_INFO "  domid: %d\n", bd->domid);
+        }
+    }
+
     fdt_for_each_subnode(node, fdt, dom_node)
     {
         if ( !fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
@@ -258,6 +301,13 @@ static int __init fdt_process_domain_node(
         return -ENODATA;
     }
 
+    if ( bd->domid == DOMID_INVALID )
+        bd->domid = get_initial_domain_id();
+    else if ( bd->domid != get_initial_domain_id() )
+        printk(XENLOG_WARNING
+               "warning: d%u is not the expected initial domain (d%u)\n",
+               bd->domid, get_initial_domain_id());
+
     return 0;
 }
 
-- 
2.43.0

[PATCH v6 09/12] x86/hyperlaunch: specify dom0 mode with device tree

[Alejandro Vallejo]

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

Enable selecting the mode in which the domain will be built and ran. This
includes:

  - whether it will be either a 32/64 bit domain
  - if it will be run as a PV or HVM domain
  - and if it will require a device model (not applicable for dom0)

In the device tree, this will be represented as a bit map that will be carried
through into struct boot_domain.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-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/boot-domain.h |  5 +++++
 xen/arch/x86/setup.c                   |  3 ++-
 xen/common/domain-builder/fdt.c        | 21 +++++++++++++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
index d7c6042e25..e316d4bcde 100644
--- a/xen/arch/x86/include/asm/boot-domain.h
+++ b/xen/arch/x86/include/asm/boot-domain.h
@@ -13,6 +13,11 @@
 struct boot_domain {
     domid_t domid;
 
+                                          /* On     | Off    */
+#define BUILD_MODE_PARAVIRT      (1 << 0) /* PV     | PVH/HVM */
+#define BUILD_MODE_ENABLE_DM     (1 << 1) /* HVM    | PVH     */
+    uint32_t mode;
+
     struct boot_module *kernel;
     struct boot_module *module;
     const char *cmdline;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 10ff67ac37..343f87ee9d 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1030,7 +1030,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
     struct boot_domain *bd = &bi->domains[0];
     struct domain *d;
 
-    if ( opt_dom0_pvh )
+    if ( opt_dom0_pvh ||
+         (bi->hyperlaunch_enabled && !(bd->mode & BUILD_MODE_PARAVIRT)) )
     {
         dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
                            ((hvm_hap_supported() && !opt_dom0_shadow) ?
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 2c05b0a22d..aadca11dfa 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -235,6 +235,27 @@ static int __init fdt_process_domain_node(
             bd->domid = val;
             printk(XENLOG_INFO "  domid: %d\n", bd->domid);
         }
+        else if ( !strncmp(prop_name, "mode", name_len) )
+        {
+            if ( (rc = fdt_prop_as_u32(prop, &bd->mode)) )
+            {
+                printk(XENLOG_ERR
+                       "  failed processing mode for domain %s\n", name);
+                return rc;
+            }
+
+            if ( (bd->mode & BUILD_MODE_PARAVIRT)  &&
+                 (bd->mode & BUILD_MODE_ENABLE_DM) )
+            {
+                printk(XENLOG_ERR "  mode: invalid (pv+hvm)\n");
+                return -EINVAL;
+            }
+
+            printk(XENLOG_INFO "  mode: %s\n",
+                   (bd->mode & BUILD_MODE_PARAVIRT)  ? "pv"  :
+                   (bd->mode & BUILD_MODE_ENABLE_DM) ? "hvm" :
+                                                       "pvh");
+        }
     }
 
     fdt_for_each_subnode(node, fdt, dom_node)
-- 
2.43.0

[PATCH v6 10/12] x86/hyperlaunch: add memory parsing to domain config

[Alejandro Vallejo]

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

Add three properties, memory, mem-min, and mem-max, to the domain node device
tree parsing to define the memory allocation for a domain. All three fields are
expressed in kb and written as a u64 in the device tree entries.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-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/dom0_build.c              |  8 +++++
 xen/arch/x86/include/asm/boot-domain.h |  4 +++
 xen/arch/x86/setup.c                   |  5 ++-
 xen/common/domain-builder/fdt.c        | 46 ++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 0b467fd4a4..8db24dbc0a 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -627,6 +627,14 @@ int __init construct_dom0(const struct boot_domain *bd)
 
     process_pending_softirqs();
 
+    /* If param dom0_size was not set and HL config provided memory size */
+    if ( !get_memsize(&dom0_size, ULONG_MAX) && bd->mem_pages )
+        dom0_size.nr_pages = bd->mem_pages;
+    if ( !get_memsize(&dom0_min_size, ULONG_MAX) && bd->min_pages )
+        dom0_min_size.nr_pages = bd->min_pages;
+    if ( !get_memsize(&dom0_max_size, ULONG_MAX) && bd->max_pages )
+        dom0_max_size.nr_pages = bd->max_pages;
+
     if ( is_hvm_domain(d) )
         rc = dom0_construct_pvh(bd);
     else if ( is_pv_domain(d) )
diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
index e316d4bcde..fa8ea1cc66 100644
--- a/xen/arch/x86/include/asm/boot-domain.h
+++ b/xen/arch/x86/include/asm/boot-domain.h
@@ -18,6 +18,10 @@ struct boot_domain {
 #define BUILD_MODE_ENABLE_DM     (1 << 1) /* HVM    | PVH     */
     uint32_t mode;
 
+    unsigned long mem_pages;
+    unsigned long min_pages;
+    unsigned long max_pages;
+
     struct boot_module *kernel;
     struct boot_module *module;
     const char *cmdline;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 343f87ee9d..4a3c41ad71 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -297,7 +297,10 @@ static const char *cmdline_cook(const char *p, const char *loader_name);
 struct boot_info __initdata xen_boot_info = {
     .loader = "unknown",
     .cmdline = "",
-    .domains = { [0 ... MAX_NR_BOOTDOMS - 1] = { .domid = DOMID_INVALID } },
+    .domains = { [0 ... MAX_NR_BOOTDOMS - 1] = {
+        .domid = DOMID_INVALID,
+        .max_pages = ULONG_MAX,
+    }},
 };
 
 static struct boot_info *__init multiboot_fill_boot_info(
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index aadca11dfa..d9babe9d56 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -7,6 +7,7 @@
 #include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/libfdt/libfdt.h>
+#include <xen/sizes.h>
 
 #include <asm/bootinfo.h>
 #include <asm/page.h>
@@ -24,6 +25,16 @@ static int __init fdt_prop_as_u32(const struct fdt_property *prop,
     return 0;
 }
 
+static int __init fdt_prop_as_u64(const struct fdt_property *prop,
+                                  uint64_t *val)
+{
+    if ( !prop || fdt32_to_cpu(prop->len) < sizeof(uint64_t) )
+        return -EINVAL;
+
+    *val = fdt64_to_cpu(*(const fdt64_t *)prop->data);
+    return 0;
+}
+
 /*
  * Unpacks a "reg" property into its address and size constituents.
  *
@@ -256,6 +267,41 @@ static int __init fdt_process_domain_node(
                    (bd->mode & BUILD_MODE_ENABLE_DM) ? "hvm" :
                                                        "pvh");
         }
+        else if ( !strncmp(prop_name, "memory", name_len) )
+        {
+            uint64_t kb;
+            if ( (rc = fdt_prop_as_u64(prop, &kb)) )
+            {
+                printk(XENLOG_ERR "  bad \"memory\" prop on domain %s\n", name);
+                return rc;
+            }
+            bd->mem_pages = PFN_DOWN(kb * SZ_1K);
+            printk(XENLOG_ERR "  memory: %lu KiB\n", kb);
+        }
+        else if ( !strncmp(prop_name, "mem-min", name_len) )
+        {
+            uint64_t kb;
+            if ( (rc = fdt_prop_as_u64(prop, &kb)) )
+            {
+                printk(XENLOG_ERR
+                       "  bad \"mem-min\" prop on domain %s\n", name);
+                return rc;
+            }
+            bd->min_pages = PFN_DOWN(kb * SZ_1K);
+            printk(XENLOG_INFO "  min memory: %lu kb\n", kb);
+        }
+        else if ( !strncmp(prop_name, "mem-max", name_len) )
+        {
+            uint64_t kb;
+            if ( (rc = fdt_prop_as_u64(prop, &kb)) )
+            {
+                printk(XENLOG_ERR
+                       "  bad \"mem-max\" prop on domain %s\n", name);
+                return rc;
+            }
+            bd->max_pages = PFN_DOWN(kb * SZ_1K);
+            printk(XENLOG_INFO "  max memory: %lu KiB\n", kb);
+        }
     }
 
     fdt_for_each_subnode(node, fdt, dom_node)
-- 
2.43.0

[PATCH v6 11/12] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree

[Alejandro Vallejo]

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

Introduce the `cpus` property, named as such for dom0less compatibility, that
represents the maximum number of vcpus to allocate for a domain. In the device
tree, it will be encoded as a u32 value.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-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/dom0_build.c              |  3 +++
 xen/arch/x86/include/asm/boot-domain.h |  2 ++
 xen/common/domain-builder/fdt.c        | 11 +++++++++++
 3 files changed, 16 insertions(+)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 8db24dbc0a..f734104c74 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -635,6 +635,9 @@ int __init construct_dom0(const struct boot_domain *bd)
     if ( !get_memsize(&dom0_max_size, ULONG_MAX) && bd->max_pages )
         dom0_max_size.nr_pages = bd->max_pages;
 
+    if ( opt_dom0_max_vcpus_max == UINT_MAX && bd->max_vcpus )
+        opt_dom0_max_vcpus_max = bd->max_vcpus;
+
     if ( is_hvm_domain(d) )
         rc = dom0_construct_pvh(bd);
     else if ( is_pv_domain(d) )
diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
index fa8ea1cc66..969c02a6ea 100644
--- a/xen/arch/x86/include/asm/boot-domain.h
+++ b/xen/arch/x86/include/asm/boot-domain.h
@@ -22,6 +22,8 @@ struct boot_domain {
     unsigned long min_pages;
     unsigned long max_pages;
 
+    unsigned int max_vcpus;
+
     struct boot_module *kernel;
     struct boot_module *module;
     const char *cmdline;
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index d9babe9d56..02cfe8085d 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -302,6 +302,17 @@ static int __init fdt_process_domain_node(
             bd->max_pages = PFN_DOWN(kb * SZ_1K);
             printk(XENLOG_INFO "  max memory: %lu KiB\n", kb);
         }
+        else if ( !strncmp(prop_name, "cpus", name_len) )
+        {
+            uint32_t val = UINT32_MAX;
+            if ( (rc = fdt_prop_as_u32(prop, &val)) )
+            {
+                printk(XENLOG_ERR "  bad \"cpus\" prop on domain %s\n", name);
+                return rc;
+            }
+            bd->max_vcpus = val;
+            printk(XENLOG_INFO "  cpus: %d\n", bd->max_vcpus);
+        }
     }
 
     fdt_for_each_subnode(node, fdt, dom_node)
-- 
2.43.0

[PATCH v6 12/12] x86/hyperlaunch: add capabilities to boot domain

[Alejandro Vallejo]

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>

Introduce the ability to assign capabilities to a domain via its definition in
device tree. The capability property is a bitfield in both the device tree and
`struct boot_domain`.

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>
---
v6:
  * Reworded the commit message to reflect the fact that the
    DOMAIN_CAPS_HARDWARE is also done here.
  * Conditionalise setting CDF_iommu to DOMAIN_CAPS_HARDWARE being set.
---
 xen/arch/x86/include/asm/boot-domain.h |  3 +++
 xen/arch/x86/setup.c                   |  7 +++++--
 xen/common/domain-builder/fdt.c        | 23 +++++++++++++++++++++++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
index 969c02a6ea..cb5351241a 100644
--- a/xen/arch/x86/include/asm/boot-domain.h
+++ b/xen/arch/x86/include/asm/boot-domain.h
@@ -13,6 +13,9 @@
 struct boot_domain {
     domid_t domid;
 
+    /* Bitmap. See DOMAIN_CAPS_MASK for a list */
+    uint32_t capabilities;
+
                                           /* On     | Off    */
 #define BUILD_MODE_PARAVIRT      (1 << 0) /* PV     | PVH/HVM */
 #define BUILD_MODE_ENABLE_DM     (1 << 1) /* HVM    | PVH     */
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 4a3c41ad71..6a1e874b2e 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -62,6 +62,7 @@
 
 #include <xsm/xsm.h>
 
+#include <public/bootfdt.h>
 #include <public/version.h>
 #ifdef CONFIG_COMPAT
 #include <compat/platform.h>
@@ -1044,14 +1045,15 @@ static struct domain *__init create_dom0(struct boot_info *bi)
             XEN_X86_EMU_LAPIC | XEN_X86_EMU_IOAPIC | XEN_X86_EMU_VPCI;
     }
 
-    if ( iommu_enabled )
+    if ( iommu_enabled && (bd->capabilities & DOMAIN_CAPS_HARDWARE) )
         dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
 
     if ( bd->domid == DOMID_INVALID )
         /* Create initial domain.  Not d0 for pvshim. */
         bd->domid = get_initial_domain_id();
     d = domain_create(bd->domid, &dom0_cfg,
-                      pv_shim ? 0 : CDF_privileged | CDF_hardware);
+            ((bd->capabilities & DOMAIN_CAPS_CONTROL)  ? CDF_privileged : 0) |
+            ((bd->capabilities & DOMAIN_CAPS_HARDWARE) ? CDF_hardware   : 0));
     if ( IS_ERR(d) )
         panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
 
@@ -1314,6 +1316,7 @@ void asmlinkage __init noreturn __start_xen(void)
         i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
         bi->mods[i].type = BOOTMOD_KERNEL;
         bi->domains[0].kernel = &bi->mods[i];
+        bi->domains[0].capabilities = pv_shim ? 0 : DOMAIN_CAPS_MASK;
         bi->nr_domains = 1;
         bi->hyperlaunch_enabled = false;
     }
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 02cfe8085d..232621ae46 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -7,6 +7,7 @@
 #include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/libfdt/libfdt.h>
+#include <public/bootfdt.h>
 #include <xen/sizes.h>
 
 #include <asm/bootinfo.h>
@@ -313,6 +314,28 @@ static int __init fdt_process_domain_node(
             bd->max_vcpus = val;
             printk(XENLOG_INFO "  cpus: %d\n", bd->max_vcpus);
         }
+        else if ( !strncmp(prop_name, "capabilities", name_len) )
+        {
+            if ( (rc = fdt_prop_as_u32(prop, &bd->capabilities)) )
+            {
+                printk(XENLOG_ERR
+                       "  bad \"capabilities\" on domain %s\n", name);
+                return rc;
+            }
+
+            if ( bd->capabilities & ~DOMAIN_CAPS_MASK )
+            {
+                printk(XENLOG_WARNING "  unknown capabilities: %#x\n",
+                       bd->capabilities & ~DOMAIN_CAPS_MASK);
+
+                bd->capabilities &= DOMAIN_CAPS_MASK;
+            }
+
+            printk(XENLOG_INFO "  caps: %s%s%s\n",
+                   (bd->capabilities & DOMAIN_CAPS_CONTROL)  ? "c" : "",
+                   (bd->capabilities & DOMAIN_CAPS_HARDWARE) ? "h" : "",
+                   (bd->capabilities & DOMAIN_CAPS_XENSTORE) ? "x" : "");
+        }
     }
 
     fdt_for_each_subnode(node, fdt, dom_node)
-- 
2.43.0

Re: [PATCH v6 00/12] Hyperlaunch device tree for dom0

[Jan Beulich]

On 29.04.2025 14:36, Alejandro Vallejo wrote:
> Not very many changes here. Just:
> 
> v5->v6:
>   * Denis' suggestion to rename a few helpers to fdt_*
>   * Change to last patch to only pass CDF_iommu to domains with
>     DOMAIN_CAPS_HARDWARE.
> 
> I _think_ this addresses all feedback I got so far and I don't expect
> anything major remaining before commit.

That's optimistic. I for one didn't even get around looking at v5.

Jan

Re: [PATCH v6 00/12] Hyperlaunch device tree for dom0

[Alejandro Vallejo]

On Tue Apr 29, 2025 at 2:00 PM BST, Jan Beulich wrote:
> On 29.04.2025 14:36, Alejandro Vallejo wrote:
>> Not very many changes here. Just:
>> 
>> v5->v6:
>>   * Denis' suggestion to rename a few helpers to fdt_*
>>   * Change to last patch to only pass CDF_iommu to domains with
>>     DOMAIN_CAPS_HARDWARE.
>> 
>> I _think_ this addresses all feedback I got so far and I don't expect
>> anything major remaining before commit.
>
> That's optimistic. I for one didn't even get around looking at v5.

What can I say? I refill my glass often enough for it never to be half
empty :)

Jokes aside, I did cover most of your large-scope concerns by v5 and v6
isn't a dramatica change. I'm hopeful the bigger thorns ought to be gone
by now.

Cheers,
Alejandro

Re: [PATCH v6 02/12] common/hyperlaunch: introduce the domain builder

[Daniel P. Smith]

On 4/29/25 08:36, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> 
> Introduce the domain builder which is capable of consuming a device tree as the
> first boot module. If it finds a device tree as the first boot module, it will
> set its type to BOOTMOD_FDT. This change only detects the boot module and
> continues to boot with slight change to the boot convention that the dom0
> kernel is no longer first boot module but is the second.
> 
> No functional change intended.
> 
> 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 |  3 ++
>   xen/arch/x86/setup.c                | 19 ++++++++----
>   xen/common/Makefile                 |  1 +
>   xen/common/domain-builder/Makefile  |  2 ++
>   xen/common/domain-builder/core.c    | 45 +++++++++++++++++++++++++++++
>   xen/common/domain-builder/fdt.c     | 39 +++++++++++++++++++++++++
>   xen/common/domain-builder/fdt.h     | 14 +++++++++
>   xen/include/xen/domain-builder.h    | 29 +++++++++++++++++++
>   8 files changed, 146 insertions(+), 6 deletions(-)
>   create mode 100644 xen/common/domain-builder/Makefile
>   create mode 100644 xen/common/domain-builder/core.c
>   create mode 100644 xen/common/domain-builder/fdt.c
>   create mode 100644 xen/common/domain-builder/fdt.h
>   create mode 100644 xen/include/xen/domain-builder.h
> 
> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> index 3afc214c17..82c2650fcf 100644
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -27,6 +27,7 @@ enum bootmod_type {
>       BOOTMOD_RAMDISK,
>       BOOTMOD_MICROCODE,
>       BOOTMOD_XSM_POLICY,
> +    BOOTMOD_FDT,
>   };
>   
>   struct boot_module {
> @@ -80,6 +81,8 @@ struct boot_info {
>       paddr_t memmap_addr;
>       size_t memmap_length;
>   
> +    bool hyperlaunch_enabled;
> +
>       unsigned int nr_modules;
>       struct boot_module mods[MAX_NR_BOOTMODS + 1];
>       struct boot_domain domains[MAX_NR_BOOTDOMS];
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 2518954124..f3b5c83a3c 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -6,6 +6,7 @@
>   #include <xen/cpuidle.h>
>   #include <xen/dmi.h>
>   #include <xen/domain.h>
> +#include <xen/domain-builder.h>
>   #include <xen/domain_page.h>
>   #include <xen/efi.h>
>   #include <xen/err.h>
> @@ -1284,9 +1285,14 @@ void asmlinkage __init noreturn __start_xen(void)
>                  bi->nr_modules);
>       }
>   
> -    /* Dom0 kernel is always first */
> -    bi->mods[0].type = BOOTMOD_KERNEL;
> -    bi->domains[0].kernel = &bi->mods[0];
> +    if ( builder_init(bi) == FDT_KIND_NONE )
> +    {
> +        /* Find first unknown boot module to use as dom0 kernel */
> +        i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
> +        bi->mods[i].type = BOOTMOD_KERNEL;
> +        bi->domains[0].kernel = &bi->mods[i];
> +        bi->hyperlaunch_enabled = false;
> +    }
>   
>       if ( pvh_boot )
>       {
> @@ -1469,8 +1475,9 @@ void asmlinkage __init noreturn __start_xen(void)
>           xen->size  = __2M_rwdata_end - _stext;
>       }
>   
> -    bi->mods[0].headroom =
> -        bzimage_headroom(bootstrap_map_bm(&bi->mods[0]), bi->mods[0].size);
> +    bi->domains[0].kernel->headroom =
> +        bzimage_headroom(bootstrap_map_bm(bi->domains[0].kernel),
> +                         bi->domains[0].kernel->size);
>       bootstrap_unmap();
>   
>   #ifndef highmem_start
> @@ -1594,7 +1601,7 @@ void asmlinkage __init noreturn __start_xen(void)
>   #endif
>       }
>   
> -    if ( bi->mods[0].headroom && !bi->mods[0].relocated )
> +    if ( bi->domains[0].kernel->headroom && !bi->domains[0].kernel->relocated )
>           panic("Not enough memory to relocate the dom0 kernel image\n");
>       for ( i = 0; i < bi->nr_modules; ++i )
>       {
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 98f0873056..e42af71e3f 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
>   obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
>   obj-$(CONFIG_IOREQ_SERVER) += dm.o
>   obj-y += domain.o
> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/

Please don't do this, use IF_ENABLED in core.c and then hide the 
unnecessary units in domain-builder/Makefile as I originally had it. 
This allows for a much easier time incrementally converting the dom0 
construction path into a generalized domain construction path.

>   obj-y += event_2l.o
>   obj-y += event_channel.o
>   obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
> diff --git a/xen/common/domain-builder/Makefile b/xen/common/domain-builder/Makefile
> new file mode 100644
> index 0000000000..bfd2f6267e
> --- /dev/null
> +++ b/xen/common/domain-builder/Makefile
> @@ -0,0 +1,2 @@
> +obj-y += fdt.init.o
> +obj-y += core.init.o
> diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
> new file mode 100644
> index 0000000000..97c92db571
> --- /dev/null
> +++ b/xen/common/domain-builder/core.c
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024, Apertus Solutions, LLC
> + */
> +#include <xen/bug.h>
> +#include <xen/err.h>
> +#include <xen/init.h>
> +#include <xen/kconfig.h>
> +#include <xen/domain-builder.h>
> +#include <xen/lib.h>
> +
> +#include <asm/bootinfo.h>
> +
> +#include "fdt.h"
> +
> +enum fdt_kind __init builder_init(struct boot_info *bi)
> +{
> +    enum fdt_kind kind;
> +
> +    bi->hyperlaunch_enabled = false;
> +    switch ( (kind = fdt_detect_kind(bi)) )
> +    {
> +    case FDT_KIND_NONE:
> +        /* No DT found */
> +        return kind;
> +
> +    case FDT_KIND_UNKNOWN:
> +        printk(XENLOG_DEBUG "DT found: non-hyperlaunch\n");
> +        bi->mods[0].type = BOOTMOD_FDT;
> +        return kind;
> +
> +    default:
> +        BUG();
> +    }
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> new file mode 100644
> index 0000000000..4b07bd22c8
> --- /dev/null
> +++ b/xen/common/domain-builder/fdt.c
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024, Apertus Solutions, LLC
> + */
> +#include <xen/err.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/libfdt/libfdt.h>
> +
> +#include <asm/bootinfo.h>
> +#include <asm/page.h>
> +#include <asm/setup.h>
> +
> +#include "fdt.h"
> +
> +enum fdt_kind __init fdt_detect_kind(const struct boot_info *bi)
> +{
> +    enum fdt_kind kind;
> +    const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +
> +    if ( !fdt || fdt_check_header(fdt) < 0 )
> +        kind = FDT_KIND_NONE;
> +    else
> +        kind = FDT_KIND_UNKNOWN;
> +
> +    bootstrap_unmap();
> +
> +    return kind;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
> new file mode 100644
> index 0000000000..ef897fc412
> --- /dev/null
> +++ b/xen/common/domain-builder/fdt.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __XEN_DOMAIN_BUILDER_FDT_H__
> +#define __XEN_DOMAIN_BUILDER_FDT_H__
> +
> +#include <xen/domain-builder.h>
> +
> +struct boot_info;
> +
> +/* hyperlaunch fdt is required to be module 0 */
> +#define HYPERLAUNCH_MODULE_IDX 0
> +
> +enum fdt_kind fdt_detect_kind(const struct boot_info *bi);
> +
> +#endif /* __XEN_DOMAIN_BUILDER_FDT_H__ */
> diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
> new file mode 100644
> index 0000000000..b9702db735
> --- /dev/null
> +++ b/xen/include/xen/domain-builder.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __XEN_DOMAIN_BUILDER_H__
> +#define __XEN_DOMAIN_BUILDER_H__
> +
> +struct boot_info;
> +
> +/* Return status of builder_init(). Shows which boot mechanism was detected */
> +enum fdt_kind
> +{
> +    /* FDT not found. Skipped builder. */
> +    FDT_KIND_NONE,
> +    /* Found an FDT that wasn't hyperlaunch. */
> +    FDT_KIND_UNKNOWN,
> +};
> +
> +/*
> + * Initialises `bi` if it detects a compatible FDT. Otherwise returns
> + * FDT_KIND_NONE and leaves initialisation up to the caller.
> + */
> +#if IS_ENABLED(CONFIG_DOMAIN_BUILDER)
> +enum fdt_kind builder_init(struct boot_info *bi);
> +#else
> +static inline enum fdt_kind builder_init(struct boot_info *bi)
> +{
> +    return FDT_KIND_NONE;
> +}
> +#endif /* !IS_ENABLED(CONFIG_DOMAIN_BUILDER) */
> +
> +#endif /* __XEN_DOMAIN_BUILDER_H__ */

Re: [PATCH v6 02/12] common/hyperlaunch: introduce the domain builder

[Jan Beulich]

On 30.04.2025 20:56, Daniel P. Smith wrote:
> On 4/29/25 08:36, Alejandro Vallejo wrote:
>> --- a/xen/common/Makefile
>> +++ b/xen/common/Makefile
>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
>>   obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
>>   obj-$(CONFIG_IOREQ_SERVER) += dm.o
>>   obj-y += domain.o
>> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/
> 
> Please don't do this, use IF_ENABLED in core.c and then hide the 
> unnecessary units in domain-builder/Makefile as I originally had it. 
> This allows for a much easier time incrementally converting the dom0 
> construction path into a generalized domain construction path.

That is, are you viewing this as a transitional thing only? If the end
goal is to have it as Alejandro has it above, that may be acceptable
(even if not nice).

Jan

Re: [PATCH v6 02/12] common/hyperlaunch: introduce the domain builder

[Daniel P. Smith]

On 5/2/25 03:21, Jan Beulich wrote:
> On 30.04.2025 20:56, Daniel P. Smith wrote:
>> On 4/29/25 08:36, Alejandro Vallejo wrote:
>>> --- a/xen/common/Makefile
>>> +++ b/xen/common/Makefile
>>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
>>>    obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
>>>    obj-$(CONFIG_IOREQ_SERVER) += dm.o
>>>    obj-y += domain.o
>>> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/
>>
>> Please don't do this, use IF_ENABLED in core.c and then hide the
>> unnecessary units in domain-builder/Makefile as I originally had it.
>> This allows for a much easier time incrementally converting the dom0
>> construction path into a generalized domain construction path.
> 
> That is, are you viewing this as a transitional thing only? If the end
> goal is to have it as Alejandro has it above, that may be acceptable
> (even if not nice).

There is/will be shared domain construction code between dom0-only and 
multidomain construction, with it will all living under domain builder. 
So no, the end goal is not what Alejandro just did. The end result will 
be the way I had it, with a different kconfig option to enable/disable 
the multidomain construction path.

v/r,
dps

Re: [PATCH v6 02/12] common/hyperlaunch: introduce the domain builder

[Jan Beulich]

On 06.05.2025 21:29, Daniel P. Smith wrote:
> On 5/2/25 03:21, Jan Beulich wrote:
>> On 30.04.2025 20:56, Daniel P. Smith wrote:
>>> On 4/29/25 08:36, Alejandro Vallejo wrote:
>>>> --- a/xen/common/Makefile
>>>> +++ b/xen/common/Makefile
>>>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
>>>>    obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
>>>>    obj-$(CONFIG_IOREQ_SERVER) += dm.o
>>>>    obj-y += domain.o
>>>> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/
>>>
>>> Please don't do this, use IF_ENABLED in core.c and then hide the
>>> unnecessary units in domain-builder/Makefile as I originally had it.
>>> This allows for a much easier time incrementally converting the dom0
>>> construction path into a generalized domain construction path.
>>
>> That is, are you viewing this as a transitional thing only? If the end
>> goal is to have it as Alejandro has it above, that may be acceptable
>> (even if not nice).
> 
> There is/will be shared domain construction code between dom0-only and 
> multidomain construction, with it will all living under domain builder. 
> So no, the end goal is not what Alejandro just did. The end result will 
> be the way I had it, with a different kconfig option to enable/disable 
> the multidomain construction path.

Isn't CONFIG_DOMAIN_BUILDER a misnomer then?

Jan

Re: [PATCH v6 02/12] common/hyperlaunch: introduce the domain builder

[Daniel P. Smith]

On 5/13/25 04:05, Jan Beulich wrote:
> On 06.05.2025 21:29, Daniel P. Smith wrote:
>> On 5/2/25 03:21, Jan Beulich wrote:
>>> On 30.04.2025 20:56, Daniel P. Smith wrote:
>>>> On 4/29/25 08:36, Alejandro Vallejo wrote:
>>>>> --- a/xen/common/Makefile
>>>>> +++ b/xen/common/Makefile
>>>>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
>>>>>     obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
>>>>>     obj-$(CONFIG_IOREQ_SERVER) += dm.o
>>>>>     obj-y += domain.o
>>>>> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/
>>>>
>>>> Please don't do this, use IF_ENABLED in core.c and then hide the
>>>> unnecessary units in domain-builder/Makefile as I originally had it.
>>>> This allows for a much easier time incrementally converting the dom0
>>>> construction path into a generalized domain construction path.
>>>
>>> That is, are you viewing this as a transitional thing only? If the end
>>> goal is to have it as Alejandro has it above, that may be acceptable
>>> (even if not nice).
>>
>> There is/will be shared domain construction code between dom0-only and
>> multidomain construction, with it will all living under domain builder.
>> So no, the end goal is not what Alejandro just did. The end result will
>> be the way I had it, with a different kconfig option to enable/disable
>> the multidomain construction path.
> 
> Isn't CONFIG_DOMAIN_BUILDER a misnomer then?

Which is why I originally had two kconfig flags, one to enable dtb 
parsing for domain configuration, which allowed dom0 construction from 
dtb. Then there was the second flag that was to enable multi-domain 
construction. I have reworked a portion of the approach in the RFC based 
on feedback, which is based off of this series. In it, I tried to 
minimize how much went into common, but you will see how I still had to 
rework the kconfig flags.

v/r,
dps

Re: [PATCH v6 02/12] common/hyperlaunch: introduce the domain builder

[Daniel P. Smith]

On 5/2/25 03:21, Jan Beulich wrote:
> On 30.04.2025 20:56, Daniel P. Smith wrote:
>> On 4/29/25 08:36, Alejandro Vallejo wrote:
>>> --- a/xen/common/Makefile
>>> +++ b/xen/common/Makefile
>>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
>>>    obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
>>>    obj-$(CONFIG_IOREQ_SERVER) += dm.o
>>>    obj-y += domain.o
>>> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/
>>
>> Please don't do this, use IF_ENABLED in core.c and then hide the
>> unnecessary units in domain-builder/Makefile as I originally had it.
>> This allows for a much easier time incrementally converting the dom0
>> construction path into a generalized domain construction path.
> 
> That is, are you viewing this as a transitional thing only? If the end
> goal is to have it as Alejandro has it above, that may be acceptable
> (even if not nice).

In the end, it became too much of a mess to keep the core builder 
functions with the fdt code in common. So I left this as is, and kept 
all the builder logic in x86/domain-builer/core.c.

v/r,
dps

Re: [PATCH v6 02/12] common/hyperlaunch: introduce the domain builder

[Jan Beulich]

On 29.04.2025 14:36, Alejandro Vallejo wrote:
> @@ -1284,9 +1285,14 @@ void asmlinkage __init noreturn __start_xen(void)
>                 bi->nr_modules);
>      }
>  
> -    /* Dom0 kernel is always first */
> -    bi->mods[0].type = BOOTMOD_KERNEL;
> -    bi->domains[0].kernel = &bi->mods[0];
> +    if ( builder_init(bi) == FDT_KIND_NONE )

With this, can ...

> +    {
> +        /* Find first unknown boot module to use as dom0 kernel */
> +        i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);

... i ever be anything else than 0? If not, perhaps keeping the call here is
still fine (kind of for doc purposes), but an assertion may then want adding.

> +        bi->mods[i].type = BOOTMOD_KERNEL;
> +        bi->domains[0].kernel = &bi->mods[i];
> +        bi->hyperlaunch_enabled = false;

Is this necessary, when the field is supposed to be starting out clear?

> --- /dev/null
> +++ b/xen/common/domain-builder/Makefile
> @@ -0,0 +1,2 @@
> +obj-y += fdt.init.o
> +obj-y += core.init.o

Any reason for these not both adding to obj-bin-y, like we do elsewhere for
*.init.o?

Also please sort object lists alphabetically.

> --- /dev/null
> +++ b/xen/include/xen/domain-builder.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __XEN_DOMAIN_BUILDER_H__
> +#define __XEN_DOMAIN_BUILDER_H__
> +
> +struct boot_info;
> +
> +/* Return status of builder_init(). Shows which boot mechanism was detected */
> +enum fdt_kind
> +{
> +    /* FDT not found. Skipped builder. */
> +    FDT_KIND_NONE,
> +    /* Found an FDT that wasn't hyperlaunch. */
> +    FDT_KIND_UNKNOWN,
> +};
> +
> +/*
> + * Initialises `bi` if it detects a compatible FDT. Otherwise returns
> + * FDT_KIND_NONE and leaves initialisation up to the caller.
> + */
> +#if IS_ENABLED(CONFIG_DOMAIN_BUILDER)

For the pre-processor it wants to be the simpler #ifdef.

Jan

Re: [PATCH v6 03/12] x86/hyperlaunch: initial support for hyperlaunch device tree

[Jan Beulich]

On 29.04.2025 14:36, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> 
> Add the ability to detect both a formal hyperlaunch device tree or a dom0less
> device tree. If the hyperlaunch device tree is found, then count the number of
> domain entries, reporting an error if more than one is found.
> 
> 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>

First: With your code re-use proposal sent earlier today I wonder how
meaningful it is to further review this series. Much of it would change
if that proposal was followed, I expect?

Then: When you say "hyperlaunch or dom0less" - is it entirely benign
which of the two is found, as to further parsing? I ask because I can't
spot anywhere that you would record which of the two (if any) was found.

> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -13,6 +13,36 @@
>  
>  #include "fdt.h"
>  
> +static int __init find_hyperlaunch_node(const void *fdt)
> +{
> +    int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
> +
> +    if ( hv_node >= 0 )
> +    {
> +        /* Anything other than zero indicates no match */
> +        if ( fdt_node_check_compatible(fdt, hv_node, "hypervisor,xen") )
> +            return -ENODATA;
> +
> +        return hv_node;
> +    }
> +    else

Please can such unnecessary (and potentially misleading) "else" be omitted?
As ...

> +    {
> +        /* Look for dom0less config */
> +        int node, chosen_node = fdt_path_offset(fdt, "/chosen");

... these will need to move to function scope then, one of the two may want
folding with "hv_node" above.

Jan

Re: [PATCH v6 03/12] x86/hyperlaunch: initial support for hyperlaunch device tree

[Alejandro Vallejo]

On Wed May 21, 2025 at 5:00 PM CEST, Jan Beulich wrote:
> On 29.04.2025 14:36, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>> 
>> Add the ability to detect both a formal hyperlaunch device tree or a dom0less
>> device tree. If the hyperlaunch device tree is found, then count the number of
>> domain entries, reporting an error if more than one is found.
>> 
>> 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>
>
> First: With your code re-use proposal sent earlier today I wonder how
> meaningful it is to further review this series. Much of it would change
> if that proposal was followed, I expect?

Should I follow through with that proposal, that would indeed have large
knock-on effects here. Sorry, I took longer than I thought I would
evaluating.

I'll go over your reviews and answer them in case they stay relevant
afterwards. Thanks for that.

>
> Then: When you say "hyperlaunch or dom0less" - is it entirely benign
> which of the two is found, as to further parsing? I ask because I can't
> spot anywhere that you would record which of the two (if any) was found.

Under dom0less everything is /chosen, mixed with other nodes.
Hyperlaunch mandates the initial system configuration to reside in
/chosen/hypervisor, which is meant to be "xen,hypervisor"-compatible.

The function is effectively finding a suitable root for the tree that
contains the initial system config.

>
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/domain-builder/fdt.c
>> @@ -13,6 +13,36 @@
>>  
>>  #include "fdt.h"
>>  
>> +static int __init find_hyperlaunch_node(const void *fdt)
>> +{
>> +    int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
>> +
>> +    if ( hv_node >= 0 )
>> +    {
>> +        /* Anything other than zero indicates no match */
>> +        if ( fdt_node_check_compatible(fdt, hv_node, "hypervisor,xen") )
>> +            return -ENODATA;
>> +
>> +        return hv_node;
>> +    }
>> +    else
>
> Please can such unnecessary (and potentially misleading) "else" be omitted?

Not sure how it could be misleading, but...

> As ...
>
>> +    {
>> +        /* Look for dom0less config */
>> +        int node, chosen_node = fdt_path_offset(fdt, "/chosen");
>
> ... these will need to move to function scope then, one of the two may want
> folding with "hv_node" above.

... there is indeed a more compact form the function could take. Noted.

Cheers,
Alejandro

Re: [PATCH v6 03/12] x86/hyperlaunch: initial support for hyperlaunch device tree

[Jan Beulich]

On 21.05.2025 19:24, Alejandro Vallejo wrote:
> On Wed May 21, 2025 at 5:00 PM CEST, Jan Beulich wrote:
>> On 29.04.2025 14:36, Alejandro Vallejo wrote:
>>> --- a/xen/common/domain-builder/fdt.c
>>> +++ b/xen/common/domain-builder/fdt.c
>>> @@ -13,6 +13,36 @@
>>>  
>>>  #include "fdt.h"
>>>  
>>> +static int __init find_hyperlaunch_node(const void *fdt)
>>> +{
>>> +    int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
>>> +
>>> +    if ( hv_node >= 0 )
>>> +    {
>>> +        /* Anything other than zero indicates no match */
>>> +        if ( fdt_node_check_compatible(fdt, hv_node, "hypervisor,xen") )
>>> +            return -ENODATA;
>>> +
>>> +        return hv_node;
>>> +    }
>>> +    else
>>
>> Please can such unnecessary (and potentially misleading) "else" be omitted?
> 
> Not sure how it could be misleading,

For context, just briefly looking at such a construct may leave one with
the (wrong) impression that both branches of the conditional can fall through
to subsequent code. This may be less of an issue as long as both sides are
reasonably short, but then it is imo better to avoid the pattern altogether.

Jan

> but...
> 
>> As ...
>>
>>> +    {
>>> +        /* Look for dom0less config */
>>> +        int node, chosen_node = fdt_path_offset(fdt, "/chosen");
>>
>> ... these will need to move to function scope then, one of the two may want
>> folding with "hv_node" above.
> 
> ... there is indeed a more compact form the function could take. Noted.
> 
> Cheers,
> Alejandro

Re: [PATCH v6 02/12] common/hyperlaunch: introduce the domain builder

[Alejandro Vallejo]

On Wed May 21, 2025 at 10:54 AM CEST, Jan Beulich wrote:
> On 29.04.2025 14:36, Alejandro Vallejo wrote:
>> @@ -1284,9 +1285,14 @@ void asmlinkage __init noreturn __start_xen(void)
>>                 bi->nr_modules);
>>      }
>>  
>> -    /* Dom0 kernel is always first */
>> -    bi->mods[0].type = BOOTMOD_KERNEL;
>> -    bi->domains[0].kernel = &bi->mods[0];
>> +    if ( builder_init(bi) == FDT_KIND_NONE )
>
> With this, can ...
>
>> +    {
>> +        /* Find first unknown boot module to use as dom0 kernel */
>> +        i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
>
> ... i ever be anything else than 0? If not, perhaps keeping the call here is
> still fine (kind of for doc purposes), but an assertion may then want adding.

I don't think so, no. It's there for symmetry with the way the initrd is
discovered later on, as that might be on module1, or 2, or 3 depending
on whether there's microcode, or an XSM policy or anything else. in
other modules.

>
>> +        bi->mods[i].type = BOOTMOD_KERNEL;
>> +        bi->domains[0].kernel = &bi->mods[i];
>> +        bi->hyperlaunch_enabled = false;
>
> Is this necessary, when the field is supposed to be starting out clear?

It isn't necessary, but I think it gave a sense of intent. That said I'm
pondering removing that boolean though in favour of something like

  bi->mods[0].type == BOOTMOD_FDT

At which point that assignment disappears.

>
>> --- /dev/null
>> +++ b/xen/common/domain-builder/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-y += fdt.init.o
>> +obj-y += core.init.o
>
> Any reason for these not both adding to obj-bin-y, like we do elsewhere for
> *.init.o?
>
> Also please sort object lists alphabetically.
>
>> --- /dev/null
>> +++ b/xen/include/xen/domain-builder.h
>> @@ -0,0 +1,29 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef __XEN_DOMAIN_BUILDER_H__
>> +#define __XEN_DOMAIN_BUILDER_H__
>> +
>> +struct boot_info;
>> +
>> +/* Return status of builder_init(). Shows which boot mechanism was detected */
>> +enum fdt_kind
>> +{
>> +    /* FDT not found. Skipped builder. */
>> +    FDT_KIND_NONE,
>> +    /* Found an FDT that wasn't hyperlaunch. */
>> +    FDT_KIND_UNKNOWN,
>> +};
>> +
>> +/*
>> + * Initialises `bi` if it detects a compatible FDT. Otherwise returns
>> + * FDT_KIND_NONE and leaves initialisation up to the caller.
>> + */
>> +#if IS_ENABLED(CONFIG_DOMAIN_BUILDER)
>
> For the pre-processor it wants to be the simpler #ifdef.

True. Don't know what I was thinking.

>
> Jan

Cheers,
Alejandro

Re: [PATCH v6 02/12] common/hyperlaunch: introduce the domain builder

[Alejandro Vallejo]

On Wed May 14, 2025 at 12:23 AM CEST, Daniel P. Smith wrote:
> On 5/13/25 04:05, Jan Beulich wrote:
>> On 06.05.2025 21:29, Daniel P. Smith wrote:
>>> On 5/2/25 03:21, Jan Beulich wrote:
>>>> On 30.04.2025 20:56, Daniel P. Smith wrote:
>>>>> On 4/29/25 08:36, Alejandro Vallejo wrote:
>>>>>> --- a/xen/common/Makefile
>>>>>> +++ b/xen/common/Makefile
>>>>>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
>>>>>>     obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
>>>>>>     obj-$(CONFIG_IOREQ_SERVER) += dm.o
>>>>>>     obj-y += domain.o
>>>>>> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/
>>>>>
>>>>> Please don't do this, use IF_ENABLED in core.c and then hide the
>>>>> unnecessary units in domain-builder/Makefile as I originally had it.
>>>>> This allows for a much easier time incrementally converting the dom0
>>>>> construction path into a generalized domain construction path.
>>>>
>>>> That is, are you viewing this as a transitional thing only? If the end
>>>> goal is to have it as Alejandro has it above, that may be acceptable
>>>> (even if not nice).
>>>
>>> There is/will be shared domain construction code between dom0-only and
>>> multidomain construction, with it will all living under domain builder.
>>> So no, the end goal is not what Alejandro just did. The end result will
>>> be the way I had it, with a different kconfig option to enable/disable
>>> the multidomain construction path.
>> 
>> Isn't CONFIG_DOMAIN_BUILDER a misnomer then?
>
> Which is why I originally had two kconfig flags, one to enable dtb 
> parsing for domain configuration, which allowed dom0 construction from 
> dtb. Then there was the second flag that was to enable multi-domain 
> construction. I have reworked a portion of the approach in the RFC based 
> on feedback, which is based off of this series. In it, I tried to 
> minimize how much went into common, but you will see how I still had to 
> rework the kconfig flags.
>
> v/r,
> dps

Does it really make sense to have a flag to restrict multidomain while
allowing parsing the DTB? There's virtually nothing compiled out in that
case.

If you did it that way because it doesn't initially build several
domains, that's just transitional and to be expected on any feature
tagged as unsupported with (EXPERIMENTAL) in the name.

What if I collapse everything under a single CONFIG_MULTIDOMAIN_BUILDER
that compiles-in support for parsing DTBs while introducing an
unconditional builder as I go? From what I'm seeing, there are no
breaking changes in the series and the end goal is to have the builder
be unconditionally used, after all.

In fact, with the bindings code in common, I can also collapse everything
in core.c (and later domain.c) into a single arch/x86/domain-builder.c
that is unconditionally compiled in. The DTB parsing logic is already 
in separate files and that can be compiled out with
CONFIG_MULTIDOMAIN_BUILDER flag.

In retrospect, after looking at dom0less long enough there's bootfdt.c and
bootinfo.c with similar intent, but far more ad-hoc cohesion. While the
builder wants to be in common, no other arch is in a position to take
it. It needs merging with the stuff done in bootfdt.c/bootinfo.c

So, in short. I'm planning to:

  1. Collapse domain-builder/{core,domain}.c into domain-builder.c
     under arch/x86. There's little reason to have them separate.
  2. Remove CONFIG_DOMAIN_BUILDER, and replace it with something that
     reflects the intent of using a DTB. Either CONFIG_MULTIDOMAIN_BUILDER
     or CONFIG_DTB_BUILDER. Or maybe even CONFIG_DTB_BOOT.
      * I specifically want to avoid CONFIG_BOOTFDT, because that'd
        create confusion with the already existing bootfdt.c in common.

and, from the discussion in the other thread about code sharing in the
spirit of getting somewhere soon:

  3. Do minimal parsing at builder_init() (module identification,
  basically), and do the full parsing by the create_dom0() mark,
  immediately before constructing all domains. With the unflattened
  tree.

Moving forward I for sure want to merge the boot paths in the x86
builder with those of arm/riscv. Having a unified boot frontend can only
bring niceness.

Cheers,
Alejandro

Re: [PATCH v6 06/12] x86/hyperlaunch: obtain cmdline from device tree

[Jason Andryuk]

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.

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

Re: [PATCH v6 06/12] x86/hyperlaunch: obtain cmdline from device tree

[Jan Beulich]

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

Re: [PATCH v6 06/12] x86/hyperlaunch: obtain cmdline from device tree

[Jason Andryuk]

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

Re: [PATCH v6 06/12] x86/hyperlaunch: obtain cmdline from device tree

[Jan Beulich]

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

Re: [PATCH v6 06/12] x86/hyperlaunch: obtain cmdline from device tree

[Alejandro Vallejo]

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

Re: [PATCH v6 06/12] x86/hyperlaunch: obtain cmdline from device tree

[Jan Beulich]

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