[PATCH 00/15] Hyperlaunch device tree for dom0

[PATCH 00/15] Hyperlaunch device tree for dom0

[Daniel P. Smith]

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.

Much thanks to AMD for supporting this work.

Documentation on Hyperlaunch:
https://wiki.xenproject.org/wiki/Hyperlaunch

Original Hyperlaunch v1 patch series:
https://lists.xenproject.org/archives/html/xen-devel/2022-07/msg00345.html

V/r,
Daniel P. Smith

Daniel P. Smith (15):
  x86/boot: introduce boot domain
  x86/boot: introduce domid field to struct boot_domain
  x86/boot: add cmdline to struct boot_domain
  kconfig: introduce option to independently enable libfdt
  kconfig: introduce domain builder config option
  x86/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/Kconfig                     |   2 +
 xen/arch/x86/Makefile                    |   2 +
 xen/arch/x86/dom0_build.c                |  19 +-
 xen/arch/x86/domain_builder/Kconfig      |  15 +
 xen/arch/x86/domain_builder/Makefile     |   3 +
 xen/arch/x86/domain_builder/core.c       | 109 +++++++
 xen/arch/x86/domain_builder/fdt.c        | 395 +++++++++++++++++++++++
 xen/arch/x86/domain_builder/fdt.h        |  94 ++++++
 xen/arch/x86/hvm/dom0_build.c            |  23 +-
 xen/arch/x86/include/asm/bootdomain.h    |  53 +++
 xen/arch/x86/include/asm/bootinfo.h      |  15 +-
 xen/arch/x86/include/asm/dom0_build.h    |   6 +-
 xen/arch/x86/include/asm/domainbuilder.h |  12 +
 xen/arch/x86/include/asm/setup.h         |   4 +-
 xen/arch/x86/pv/dom0_build.c             |  28 +-
 xen/arch/x86/setup.c                     | 140 +++++---
 xen/common/Kconfig                       |   4 +
 xen/common/Makefile                      |   2 +-
 18 files changed, 836 insertions(+), 90 deletions(-)
 create mode 100644 xen/arch/x86/domain_builder/Kconfig
 create mode 100644 xen/arch/x86/domain_builder/Makefile
 create mode 100644 xen/arch/x86/domain_builder/core.c
 create mode 100644 xen/arch/x86/domain_builder/fdt.c
 create mode 100644 xen/arch/x86/domain_builder/fdt.h
 create mode 100644 xen/arch/x86/include/asm/bootdomain.h
 create mode 100644 xen/arch/x86/include/asm/domainbuilder.h

-- 
2.30.2

[PATCH 01/15] x86/boot: introduce boot domain

[Daniel P. Smith]

To begin moving toward allowing the hypervisor to construct more than one
domain at boot, a container is needed for a domain's build information.
Introduce a new header, <xen/asm/bootdomain.h>, that contains the initial
struct boot_domain that encapsulate the build information for a domain.

Add a kernel and ramdisk boot module reference along with a struct domain
reference to the new struct boot_domain. This allows a struct boot_domain
reference to be the only parameter necessary to pass down through the domain
construction call chain.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

---
Changes since boot modules v9
- dropped unlikely

Changes since v8:
- code style correction
---
 xen/arch/x86/dom0_build.c             |  8 ++++---
 xen/arch/x86/hvm/dom0_build.c         | 17 +++++----------
 xen/arch/x86/include/asm/bootdomain.h | 31 +++++++++++++++++++++++++++
 xen/arch/x86/include/asm/bootinfo.h   |  5 +++++
 xen/arch/x86/include/asm/dom0_build.h |  6 +++---
 xen/arch/x86/include/asm/setup.h      |  4 ++--
 xen/arch/x86/pv/dom0_build.c          | 24 +++++++--------------
 xen/arch/x86/setup.c                  | 24 +++++++++------------
 8 files changed, 69 insertions(+), 50 deletions(-)
 create mode 100644 xen/arch/x86/include/asm/bootdomain.h

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index e8f5bf5447bc..c231191faec7 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -13,6 +13,7 @@
 #include <xen/softirq.h>
 
 #include <asm/amd.h>
+#include <asm/bootinfo.h>
 #include <asm/dom0_build.h>
 #include <asm/guest.h>
 #include <asm/hpet.h>
@@ -596,9 +597,10 @@ int __init dom0_setup_permissions(struct domain *d)
     return rc;
 }
 
-int __init construct_dom0(struct boot_info *bi, struct domain *d)
+int __init construct_dom0(struct boot_domain *bd)
 {
     int rc;
+    const struct domain *d = bd->d;
 
     /* Sanity! */
     BUG_ON(!pv_shim && d->domain_id != 0);
@@ -608,9 +610,9 @@ int __init construct_dom0(struct boot_info *bi, struct domain *d)
     process_pending_softirqs();
 
     if ( is_hvm_domain(d) )
-        rc = dom0_construct_pvh(bi, d);
+        rc = dom0_construct_pvh(bd);
     else if ( is_pv_domain(d) )
-        rc = dom0_construct_pv(bi, d);
+        rc = dom0_construct_pv(bd);
     else
         panic("Cannot construct Dom0. No guest interface available\n");
 
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index ce5b5c31b318..a9384af14304 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -1301,26 +1301,19 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
     }
 }
 
-int __init dom0_construct_pvh(struct boot_info *bi, struct domain *d)
+int __init dom0_construct_pvh(struct boot_domain *bd)
 {
     paddr_t entry, start_info;
-    struct boot_module *image;
-    struct boot_module *initrd = NULL;
-    unsigned int idx;
+    struct boot_module *image = bd->kernel;
+    struct boot_module *initrd = bd->ramdisk;
+    struct domain *d = bd->d;
     int rc;
 
     printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
 
-    idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
-    if ( idx >= bi->nr_modules )
+    if ( image == NULL )
         panic("Missing kernel boot module for %pd construction\n", d);
 
-    image = &bi->mods[idx];
-
-    idx = first_boot_module_index(bi, BOOTMOD_RAMDISK);
-    if ( idx < bi->nr_modules )
-        initrd = &bi->mods[idx];
-
     if ( is_hardware_domain(d) )
     {
         /*
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
new file mode 100644
index 000000000000..12c19ab37bd8
--- /dev/null
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2024 Apertus Solutions, LLC
+ * Author: Daniel P. Smith <dpsmith@apertussolutions.com>
+ * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
+ */
+
+#ifndef __XEN_X86_BOOTDOMAIN_H__
+#define __XEN_X86_BOOTDOMAIN_H__
+
+struct boot_module;
+struct domain;
+
+struct boot_domain {
+    struct boot_module *kernel;
+    struct boot_module *ramdisk;
+
+    struct domain *d;
+};
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index f8b422913063..9f65e2c8f62d 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -11,10 +11,14 @@
 #include <xen/init.h>
 #include <xen/multiboot.h>
 #include <xen/types.h>
+#include <asm/bootdomain.h>
 
 /* Max number of boot modules a bootloader can provide in addition to Xen */
 #define MAX_NR_BOOTMODS 63
 
+/* Max number of boot domains that Xen can construct */
+#define MAX_NR_BOOTDOMS 1
+
 /* Boot module binary type / purpose */
 enum bootmod_type {
     BOOTMOD_UNKNOWN,
@@ -78,6 +82,7 @@ struct boot_info {
 
     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/include/asm/dom0_build.h b/xen/arch/x86/include/asm/dom0_build.h
index 2d67b17213dc..8c94e87dc576 100644
--- a/xen/arch/x86/include/asm/dom0_build.h
+++ b/xen/arch/x86/include/asm/dom0_build.h
@@ -13,9 +13,9 @@ unsigned long dom0_compute_nr_pages(struct domain *d,
                                     unsigned long initrd_len);
 int dom0_setup_permissions(struct domain *d);
 
-struct boot_info;
-int dom0_construct_pv(struct boot_info *bi, struct domain *d);
-int dom0_construct_pvh(struct boot_info *bi, struct domain *d);
+struct boot_domain;
+int dom0_construct_pv(struct boot_domain *bd);
+int dom0_construct_pvh(struct boot_domain *bd);
 
 unsigned long dom0_paging_pages(const struct domain *d,
                                 unsigned long nr_pages);
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index 5c2391a8684b..b517da6144de 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -26,8 +26,8 @@ void subarch_init_memory(void);
 
 void init_IRQ(void);
 
-struct boot_info;
-int construct_dom0(struct boot_info *bi, struct domain *d);
+struct boot_domain;
+int construct_dom0(struct boot_domain *bd);
 
 void setup_io_bitmap(struct domain *d);
 
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index f54d1da5c6f4..e0709a1c1a7a 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -355,7 +355,7 @@ static struct page_info * __init alloc_chunk(struct domain *d,
     return page;
 }
 
-static int __init dom0_construct(struct boot_info *bi, struct domain *d)
+static int __init dom0_construct(struct boot_domain *bd)
 {
     unsigned int i;
     int rc, order, machine;
@@ -371,14 +371,15 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
     struct page_info *page = NULL;
     unsigned int flush_flags = 0;
     start_info_t *si;
+    struct domain *d = bd->d;
     struct vcpu *v = d->vcpu[0];
 
-    struct boot_module *image;
-    struct boot_module *initrd = NULL;
+    struct boot_module *image = bd->kernel;
+    struct boot_module *initrd = bd->ramdisk;
     void *image_base;
     unsigned long image_len;
     void *image_start;
-    unsigned long initrd_len = 0;
+    unsigned long initrd_len = initrd ? initrd->size : 0;
 
     l4_pgentry_t *l4tab = NULL, *l4start = NULL;
     l3_pgentry_t *l3tab = NULL, *l3start = NULL;
@@ -416,22 +417,13 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
 
     printk(XENLOG_INFO "*** Building a PV Dom%d ***\n", d->domain_id);
 
-    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
-    if ( i >= bi->nr_modules )
+    if ( !image )
         panic("Missing kernel boot module for %pd construction\n", d);
 
-    image = &bi->mods[i];
     image_base = bootstrap_map_bm(image);
     image_len = image->size;
     image_start = image_base + image->headroom;
 
-    i = first_boot_module_index(bi, BOOTMOD_RAMDISK);
-    if ( i < bi->nr_modules )
-    {
-        initrd = &bi->mods[i];
-        initrd_len = initrd->size;
-    }
-
     d->max_pages = ~0U;
 
     if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 )
@@ -1078,7 +1070,7 @@ out:
     return rc;
 }
 
-int __init dom0_construct_pv(struct boot_info *bi, struct domain *d)
+int __init dom0_construct_pv(struct boot_domain *bd)
 {
     unsigned long cr4 = read_cr4();
     int rc;
@@ -1096,7 +1088,7 @@ int __init dom0_construct_pv(struct boot_info *bi, struct domain *d)
         write_cr4(cr4 & ~X86_CR4_SMAP);
     }
 
-    rc = dom0_construct(bi, d);
+    rc = dom0_construct(bd);
 
     if ( cr4 & X86_CR4_SMAP )
     {
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d8661d7ca699..460157def8ea 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -977,16 +977,9 @@ static struct domain *__init create_dom0(struct boot_info *bi)
             .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
         },
     };
+    struct boot_domain *bd = &bi->domains[0];
     struct domain *d;
     domid_t domid;
-    struct boot_module *image;
-    unsigned int idx;
-
-    idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
-    if ( idx >= bi->nr_modules )
-        panic("Missing kernel boot module for building domain\n");
-
-    image = &bi->mods[idx];
 
     if ( opt_dom0_pvh )
     {
@@ -1013,11 +1006,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
         panic("Error creating d%uv0\n", domid);
 
     /* Grab the DOM0 command line. */
-    if ( image->cmdline_pa || bi->kextra )
+    if ( bd->kernel->cmdline_pa || bi->kextra )
     {
-        if ( image->cmdline_pa )
-            safe_strcpy(
-                cmdline, cmdline_cook(__va(image->cmdline_pa), bi->loader));
+        if ( bd->kernel->cmdline_pa )
+            safe_strcpy(cmdline,
+                        cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader));
 
         if ( bi->kextra )
             /* kextra always includes exactly one leading space. */
@@ -1039,10 +1032,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
             safe_strcat(cmdline, acpi_param);
         }
 
-        image->cmdline_pa = __pa(cmdline);
+        bd->kernel->cmdline_pa = __pa(cmdline);
     }
 
-    if ( construct_dom0(bi, d) != 0 )
+    bd->d = d;
+    if ( construct_dom0(bd) != 0 )
         panic("Could not construct domain 0\n");
 
     return d;
@@ -1249,6 +1243,7 @@ void asmlinkage __init noreturn __start_xen(void)
 
     /* Dom0 kernel is always first */
     bi->mods[0].type = BOOTMOD_KERNEL;
+    bi->domains[0].kernel = &bi->mods[0];
 
     if ( pvh_boot )
     {
@@ -2110,6 +2105,7 @@ void asmlinkage __init noreturn __start_xen(void)
     if ( initrdidx < MAX_NR_BOOTMODS )
     {
         bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
+        bi->domains[0].ramdisk = &bi->mods[initrdidx];
         if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
             printk(XENLOG_WARNING
                    "Multiple initrd candidates, picking module #%u\n",
-- 
2.30.2

Re: [PATCH 01/15] x86/boot: introduce boot domain

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> To begin moving toward allowing the hypervisor to construct more than one
> domain at boot, a container is needed for a domain's build information.
> Introduce a new header, <xen/asm/bootdomain.h>, that contains the initial
> struct boot_domain that encapsulate the build information for a domain.

Why does this need to be a per-arch header? Wasn't one of the goals to unify
things as much as possible?

> --- a/xen/arch/x86/include/asm/dom0_build.h
> +++ b/xen/arch/x86/include/asm/dom0_build.h
> @@ -13,9 +13,9 @@ unsigned long dom0_compute_nr_pages(struct domain *d,
>                                      unsigned long initrd_len);
>  int dom0_setup_permissions(struct domain *d);
>  
> -struct boot_info;
> -int dom0_construct_pv(struct boot_info *bi, struct domain *d);
> -int dom0_construct_pvh(struct boot_info *bi, struct domain *d);
> +struct boot_domain;
> +int dom0_construct_pv(struct boot_domain *bd);
> +int dom0_construct_pvh(struct boot_domain *bd);

I'm wondering: Just a few commits ago you moved these to boot_info. Now you
move them to boot_domain. Why the extra churn, and what further transformations
are to be expected?

Jan

Re: [PATCH 01/15] x86/boot: introduce boot domain

[Daniel P. Smith]

On 12/2/24 04:57, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> To begin moving toward allowing the hypervisor to construct more than one
>> domain at boot, a container is needed for a domain's build information.
>> Introduce a new header, <xen/asm/bootdomain.h>, that contains the initial
>> struct boot_domain that encapsulate the build information for a domain.
> 
> Why does this need to be a per-arch header? Wasn't one of the goals to unify
> things as much as possible?

Correct and at this point the level of unification is this series that 
provides a common device tree definition.

>> --- a/xen/arch/x86/include/asm/dom0_build.h
>> +++ b/xen/arch/x86/include/asm/dom0_build.h
>> @@ -13,9 +13,9 @@ unsigned long dom0_compute_nr_pages(struct domain *d,
>>                                       unsigned long initrd_len);
>>   int dom0_setup_permissions(struct domain *d);
>>   
>> -struct boot_info;
>> -int dom0_construct_pv(struct boot_info *bi, struct domain *d);
>> -int dom0_construct_pvh(struct boot_info *bi, struct domain *d);
>> +struct boot_domain;
>> +int dom0_construct_pv(struct boot_domain *bd);
>> +int dom0_construct_pvh(struct boot_domain *bd);
> 
> I'm wondering: Just a few commits ago you moved these to boot_info. Now you
> move them to boot_domain. Why the extra churn, and what further transformations
> are to be expected?

Introducing all the abstractions and adjusting all interfaces in one go 
was deemed too complex to review and originally done that way because 
doing it incrementally would require making changes to changes. 
Therefore it is an artifact of breaking the larger change into an 
incremental introduction of the changes. When the multi-domain builder 
is incrementally introduced, there will be renaming in place, splitting, 
and renaming as function are moved.

v/r,
dps

[PATCH 02/15] x86/boot: introduce domid field to struct boot_domain

[Daniel P. Smith]

Add a domid field to struct boot_domain to hold the assigned domain id for the
domain. During initialization, ensure all instances of struct boot_domain have
the invalid domid to ensure that the domid must be set either by convention or
configuration.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since v9 boot modules
- missing include for domid_t def
---
 xen/arch/x86/include/asm/bootdomain.h |  4 ++++
 xen/arch/x86/setup.c                  | 12 +++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 12c19ab37bd8..bcbf36b13f25 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -5,6 +5,8 @@
  * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
  */
 
+#include <public/xen.h>
+
 #ifndef __XEN_X86_BOOTDOMAIN_H__
 #define __XEN_X86_BOOTDOMAIN_H__
 
@@ -12,6 +14,8 @@ struct boot_module;
 struct domain;
 
 struct boot_domain {
+    domid_t domid;
+
     struct boot_module *kernel;
     struct boot_module *ramdisk;
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 460157def8ea..a2178d5e8cc5 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -339,6 +339,9 @@ static struct boot_info *__init multiboot_fill_boot_info(
     /* Variable 'i' should be one entry past the last module. */
     bi->mods[i].type = BOOTMOD_XEN;
 
+    for ( i = 0; i < MAX_NR_BOOTDOMS; i++ )
+        bi->domains[i].domid = DOMID_INVALID;
+
     return bi;
 }
 
@@ -979,7 +982,6 @@ static struct domain *__init create_dom0(struct boot_info *bi)
     };
     struct boot_domain *bd = &bi->domains[0];
     struct domain *d;
-    domid_t domid;
 
     if ( opt_dom0_pvh )
     {
@@ -995,15 +997,15 @@ static struct domain *__init create_dom0(struct boot_info *bi)
         dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
 
     /* Create initial domain.  Not d0 for pvshim. */
-    domid = get_initial_domain_id();
-    d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
+    bd->domid = get_initial_domain_id();
+    d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
     if ( IS_ERR(d) )
-        panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
+        panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
 
     init_dom0_cpuid_policy(d);
 
     if ( alloc_dom0_vcpu0(d) == NULL )
-        panic("Error creating d%uv0\n", domid);
+        panic("Error creating d%uv0\n", bd->domid);
 
     /* Grab the DOM0 command line. */
     if ( bd->kernel->cmdline_pa || bi->kextra )
-- 
2.30.2

[PATCH 03/15] x86/boot: add cmdline to struct boot_domain

[Daniel P. Smith]

Add a container for the "cooked" command line for a domain. This provides for
the backing memory to be directly associated with the domain being constructed.
This is done in anticipation that the domain construction path may need to be
invoked multiple times, thus ensuring each instance had a distinct memory
allocation.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
Changes since v9 boot modules:
- convert pvh_load_kernel to boot domain to directly use cmdline
- adjustments to domain_cmdline_size
  - remove ASSERT and return 0 instead
  - use strlen() of values instead of hardcoded sizes
- update cmdline parsing check to inspect multiboot string and not just pointer
- add goto to skip cmdline processing if domain_cmdline_size returns 0
- drop updating cmdline_pa with dynamic buffer with change of its last consumer
  pvh_load_kernel

Changes since v8:
- switch to a dynamically allocated buffer
- dropped local cmdline var in pv dom0_construct()

Changes since v7:
- updated commit message to expand on intent and purpose
---
 xen/arch/x86/hvm/dom0_build.c         | 12 +++---
 xen/arch/x86/include/asm/bootdomain.h |  2 +
 xen/arch/x86/pv/dom0_build.c          |  4 +-
 xen/arch/x86/setup.c                  | 54 ++++++++++++++++++++++-----
 4 files changed, 54 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index a9384af14304..cbc365d678d2 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address(
 }
 
 static int __init pvh_load_kernel(
-    struct domain *d, struct boot_module *image, struct boot_module *initrd,
-    paddr_t *entry, paddr_t *start_info_addr)
+    struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr)
 {
+    struct domain *d = bd->d;
+    struct boot_module *image = bd->kernel;
+    struct boot_module *initrd = bd->ramdisk;
     void *image_base = bootstrap_map_bm(image);
     void *image_start = image_base + image->headroom;
     unsigned long image_len = image->size;
@@ -1304,14 +1306,12 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
 int __init dom0_construct_pvh(struct boot_domain *bd)
 {
     paddr_t entry, start_info;
-    struct boot_module *image = bd->kernel;
-    struct boot_module *initrd = bd->ramdisk;
     struct domain *d = bd->d;
     int rc;
 
     printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
 
-    if ( image == NULL )
+    if ( bd->kernel == NULL )
         panic("Missing kernel boot module for %pd construction\n", d);
 
     if ( is_hardware_domain(d) )
@@ -1351,7 +1351,7 @@ int __init dom0_construct_pvh(struct boot_domain *bd)
         return rc;
     }
 
-    rc = pvh_load_kernel(d, image, initrd, &entry, &start_info);
+    rc = pvh_load_kernel(bd, &entry, &start_info);
     if ( rc )
     {
         printk("Failed to load Dom0 kernel\n");
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index bcbf36b13f25..ffda1509a63f 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -14,6 +14,8 @@ struct boot_module;
 struct domain;
 
 struct boot_domain {
+    const char *cmdline;
+
     domid_t domid;
 
     struct boot_module *kernel;
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index e0709a1c1a7a..580f2703a154 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -972,8 +972,8 @@ static int __init dom0_construct(struct boot_domain *bd)
     }
 
     memset(si->cmd_line, 0, sizeof(si->cmd_line));
-    if ( image->cmdline_pa )
-        strlcpy((char *)si->cmd_line, __va(image->cmdline_pa), sizeof(si->cmd_line));
+    if ( bd->cmdline )
+        strlcpy((char *)si->cmd_line, bd->cmdline, sizeof(si->cmd_line));
 
 #ifdef CONFIG_VIDEO
     if ( !pv_shim && fill_console_start_info((void *)(si + 1)) )
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index a2178d5e8cc5..e6580382d247 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -965,10 +965,29 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
     return n;
 }
 
-static struct domain *__init create_dom0(struct boot_info *bi)
+static size_t __init domain_cmdline_size(
+    struct boot_info *bi, struct boot_domain *bd)
 {
-    static char __initdata cmdline[MAX_GUEST_CMDLINE];
+    size_t s = bi->kextra ? strlen(bi->kextra) : 0;
+
+    s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
+
+    if ( s == 0 )
+        return s;
+
+    /*
+     * Certain parameters from the Xen command line may be added to the dom0
+     * command line. Add additional space for the possible cases along with one
+     * extra char to hold \0.
+     */
+    s += strlen(" noapic") + strlen(" acpi=") + sizeof(acpi_param) + 1;
+
+    return s;
+}
 
+static struct domain *__init create_dom0(struct boot_info *bi)
+{
+    char *cmdline = NULL;
     struct xen_domctl_createdomain dom0_cfg = {
         .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
         .max_evtchn_port = -1,
@@ -1008,19 +1027,30 @@ static struct domain *__init create_dom0(struct boot_info *bi)
         panic("Error creating d%uv0\n", bd->domid);
 
     /* Grab the DOM0 command line. */
-    if ( bd->kernel->cmdline_pa || bi->kextra )
+    if ( (bd->kernel->cmdline_pa &&
+          ((char *)__va(bd->kernel->cmdline_pa))[0]) ||
+         bi->kextra )
     {
+        size_t cmdline_size = domain_cmdline_size(bi, bd);
+
+        if ( cmdline_size == 0 )
+            goto skip_cmdline;
+
+        if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
+            panic("Error allocating cmdline buffer for %pd\n", d);
+
         if ( bd->kernel->cmdline_pa )
-            safe_strcpy(cmdline,
-                        cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader));
+            strlcpy(cmdline,
+                    cmdline_cook(__va(bd->kernel->cmdline_pa),bi->loader),
+                    cmdline_size);
 
         if ( bi->kextra )
             /* kextra always includes exactly one leading space. */
-            safe_strcat(cmdline, bi->kextra);
+            strlcat(cmdline, bi->kextra, cmdline_size);
 
         /* Append any extra parameters. */
         if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
-            safe_strcat(cmdline, " noapic");
+            strlcat(cmdline, " noapic", cmdline_size);
 
         if ( (strlen(acpi_param) == 0) && acpi_disabled )
         {
@@ -1030,17 +1060,21 @@ static struct domain *__init create_dom0(struct boot_info *bi)
 
         if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
         {
-            safe_strcat(cmdline, " acpi=");
-            safe_strcat(cmdline, acpi_param);
+            strlcat(cmdline, " acpi=", cmdline_size);
+            strlcat(cmdline, acpi_param, cmdline_size);
         }
 
-        bd->kernel->cmdline_pa = __pa(cmdline);
+        bd->cmdline = cmdline;
     }
 
+ skip_cmdline:
     bd->d = d;
     if ( construct_dom0(bd) != 0 )
         panic("Could not construct domain 0\n");
 
+    if ( cmdline )
+        xfree(cmdline);
+
     return d;
 }
 
-- 
2.30.2

Re: [PATCH 03/15] x86/boot: add cmdline to struct boot_domain

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> Add a container for the "cooked" command line for a domain. This provides for
> the backing memory to be directly associated with the domain being constructed.
> This is done in anticipation that the domain construction path may need to be
> invoked multiple times, thus ensuring each instance had a distinct memory
> allocation.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
> Changes since v9 boot modules:
> - convert pvh_load_kernel to boot domain to directly use cmdline
> - adjustments to domain_cmdline_size
>    - remove ASSERT and return 0 instead
>    - use strlen() of values instead of hardcoded sizes
> - update cmdline parsing check to inspect multiboot string and not just pointer
> - add goto to skip cmdline processing if domain_cmdline_size returns 0
> - drop updating cmdline_pa with dynamic buffer with change of its last consumer
>    pvh_load_kernel
> 
> Changes since v8:
> - switch to a dynamically allocated buffer
> - dropped local cmdline var in pv dom0_construct()
> 
> Changes since v7:
> - updated commit message to expand on intent and purpose
> ---
>   xen/arch/x86/hvm/dom0_build.c         | 12 +++---
>   xen/arch/x86/include/asm/bootdomain.h |  2 +
>   xen/arch/x86/pv/dom0_build.c          |  4 +-
>   xen/arch/x86/setup.c                  | 54 ++++++++++++++++++++++-----
>   4 files changed, 54 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index a9384af14304..cbc365d678d2 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address(
>   }
>   
>   static int __init pvh_load_kernel(
> -    struct domain *d, struct boot_module *image, struct boot_module *initrd,
> -    paddr_t *entry, paddr_t *start_info_addr)
> +    struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr)
>   {
> +    struct domain *d = bd->d;
> +    struct boot_module *image = bd->kernel;
> +    struct boot_module *initrd = bd->ramdisk;
>       void *image_base = bootstrap_map_bm(image);
>       void *image_start = image_base + image->headroom;
>       unsigned long image_len = image->size;

cmdline_pa is used just outside of view below here.

const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : NULL;

> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index a2178d5e8cc5..e6580382d247 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -965,10 +965,29 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
>       return n;
>   }
>   
> -static struct domain *__init create_dom0(struct boot_info *bi)
> +static size_t __init domain_cmdline_size(
> +    struct boot_info *bi, struct boot_domain *bd)
>   {
> -    static char __initdata cmdline[MAX_GUEST_CMDLINE];
> +    size_t s = bi->kextra ? strlen(bi->kextra) : 0;
> +
> +    s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
> +	
> +    if ( s == 0 )
> +        return s;
> +
> +    /*
> +     * Certain parameters from the Xen command line may be added to the dom0
> +     * command line. Add additional space for the possible cases along with one
> +     * extra char to hold \0.
> +     */
> +    s += strlen(" noapic") + strlen(" acpi=") + sizeof(acpi_param) + 1;
> +
> +    return s;
> +}
>   
> +static struct domain *__init create_dom0(struct boot_info *bi)
> +{
> +    char *cmdline = NULL;
>       struct xen_domctl_createdomain dom0_cfg = {
>           .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
>           .max_evtchn_port = -1,
> @@ -1008,19 +1027,30 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>           panic("Error creating d%uv0\n", bd->domid);
>   
>       /* Grab the DOM0 command line. */
> -    if ( bd->kernel->cmdline_pa || bi->kextra )
> +    if ( (bd->kernel->cmdline_pa &&
> +          ((char *)__va(bd->kernel->cmdline_pa))[0]) ||
> +         bi->kextra )

Here you are checking pointers.

>       {
> +        size_t cmdline_size = domain_cmdline_size(bi, bd);

Internally, domain_cmdline_size() checks the pointers.

> +
> +        if ( cmdline_size == 0 )
> +            goto skip_cmdline;
> +

Maybe just use:

cmdline_size = domain_cmdline_size(bi, bd);
if ( cmdline_size )
{

and eliminate the goto?

> +        if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
> +            panic("Error allocating cmdline buffer for %pd\n", d);
> +
>           if ( bd->kernel->cmdline_pa )
> -            safe_strcpy(cmdline,
> -                        cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader));
> +            strlcpy(cmdline,
> +                    cmdline_cook(__va(bd->kernel->cmdline_pa),bi->loader),
> +                    cmdline_size);
>   
>           if ( bi->kextra )
>               /* kextra always includes exactly one leading space. */
> -            safe_strcat(cmdline, bi->kextra);
> +            strlcat(cmdline, bi->kextra, cmdline_size);
>   
>           /* Append any extra parameters. */
>           if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
> -            safe_strcat(cmdline, " noapic");
> +            strlcat(cmdline, " noapic", cmdline_size);
>   
>           if ( (strlen(acpi_param) == 0) && acpi_disabled )
>           {
> @@ -1030,17 +1060,21 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>   
>           if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
>           {
> -            safe_strcat(cmdline, " acpi=");
> -            safe_strcat(cmdline, acpi_param);
> +            strlcat(cmdline, " acpi=", cmdline_size);
> +            strlcat(cmdline, acpi_param, cmdline_size);
>           }
>   
> -        bd->kernel->cmdline_pa = __pa(cmdline);
> +        bd->cmdline = cmdline;

As mentioned above, it looks like you still inadvertently use 
bd->kernel->cmdline_pa and not the new bd->cmdline.  I think clearing 
bd->kernel->cmdline_pa would have helped identify that.  Or do you want 
to retain cmdline_pa for some reason?  It's less error prone if only one 
is usable at a time.

>       }
>   
> + skip_cmdline:
>       bd->d = d;
>       if ( construct_dom0(bd) != 0 )
>           panic("Could not construct domain 0\n");
>   
> +    if ( cmdline )
> +        xfree(cmdline);

You can drop the if - xfree() handles NULL.

Regards,
Jason

> +
>       return d;
>   }
>   

Re: [PATCH 03/15] x86/boot: add cmdline to struct boot_domain

[Daniel P. Smith]

On 11/25/24 10:36, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> Add a container for the "cooked" command line for a domain. This 
>> provides for
>> the backing memory to be directly associated with the domain being 
>> constructed.
>> This is done in anticipation that the domain construction path may 
>> need to be
>> invoked multiple times, thus ensuring each instance had a distinct memory
>> allocation.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>> Changes since v9 boot modules:
>> - convert pvh_load_kernel to boot domain to directly use cmdline
>> - adjustments to domain_cmdline_size
>>    - remove ASSERT and return 0 instead
>>    - use strlen() of values instead of hardcoded sizes
>> - update cmdline parsing check to inspect multiboot string and not 
>> just pointer
>> - add goto to skip cmdline processing if domain_cmdline_size returns 0
>> - drop updating cmdline_pa with dynamic buffer with change of its last 
>> consumer
>>    pvh_load_kernel
>>
>> Changes since v8:
>> - switch to a dynamically allocated buffer
>> - dropped local cmdline var in pv dom0_construct()
>>
>> Changes since v7:
>> - updated commit message to expand on intent and purpose
>> ---
>>   xen/arch/x86/hvm/dom0_build.c         | 12 +++---
>>   xen/arch/x86/include/asm/bootdomain.h |  2 +
>>   xen/arch/x86/pv/dom0_build.c          |  4 +-
>>   xen/arch/x86/setup.c                  | 54 ++++++++++++++++++++++-----
>>   4 files changed, 54 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/ 
>> dom0_build.c
>> index a9384af14304..cbc365d678d2 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address(
>>   }
>>   static int __init pvh_load_kernel(
>> -    struct domain *d, struct boot_module *image, struct boot_module 
>> *initrd,
>> -    paddr_t *entry, paddr_t *start_info_addr)
>> +    struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr)
>>   {
>> +    struct domain *d = bd->d;
>> +    struct boot_module *image = bd->kernel;
>> +    struct boot_module *initrd = bd->ramdisk;
>>       void *image_base = bootstrap_map_bm(image);
>>       void *image_start = image_base + image->headroom;
>>       unsigned long image_len = image->size;
> 
> cmdline_pa is used just outside of view below here.
> 
> const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : NULL;

As you mentioned below, this should have been converted to bd->cmdline, 
there's no need for the intermediate variable.

>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index a2178d5e8cc5..e6580382d247 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -965,10 +965,29 @@ static unsigned int __init copy_bios_e820(struct 
>> e820entry *map, unsigned int li
>>       return n;
>>   }
>> -static struct domain *__init create_dom0(struct boot_info *bi)
>> +static size_t __init domain_cmdline_size(
>> +    struct boot_info *bi, struct boot_domain *bd)
>>   {
>> -    static char __initdata cmdline[MAX_GUEST_CMDLINE];
>> +    size_t s = bi->kextra ? strlen(bi->kextra) : 0;
>> +
>> +    s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel- 
>> >cmdline_pa)) : 0;
>> +
>> +    if ( s == 0 )
>> +        return s;
>> +
>> +    /*
>> +     * Certain parameters from the Xen command line may be added to 
>> the dom0
>> +     * command line. Add additional space for the possible cases 
>> along with one
>> +     * extra char to hold \0.
>> +     */
>> +    s += strlen(" noapic") + strlen(" acpi=") + sizeof(acpi_param) + 1;
>> +
>> +    return s;
>> +}
>> +static struct domain *__init create_dom0(struct boot_info *bi)
>> +{
>> +    char *cmdline = NULL;
>>       struct xen_domctl_createdomain dom0_cfg = {
>>           .flags = IS_ENABLED(CONFIG_TBOOT) ? 
>> XEN_DOMCTL_CDF_s3_integrity : 0,
>>           .max_evtchn_port = -1,
>> @@ -1008,19 +1027,30 @@ static struct domain *__init 
>> create_dom0(struct boot_info *bi)
>>           panic("Error creating d%uv0\n", bd->domid);
>>       /* Grab the DOM0 command line. */
>> -    if ( bd->kernel->cmdline_pa || bi->kextra )
>> +    if ( (bd->kernel->cmdline_pa &&
>> +          ((char *)__va(bd->kernel->cmdline_pa))[0]) ||
>> +         bi->kextra )
> 
> Here you are checking pointers.
> 
>>       {
>> +        size_t cmdline_size = domain_cmdline_size(bi, bd);
> 
> Internally, domain_cmdline_size() checks the pointers.
> 
>> +
>> +        if ( cmdline_size == 0 )
>> +            goto skip_cmdline;
>> +
> 
> Maybe just use:
> 
> cmdline_size = domain_cmdline_size(bi, bd);
> if ( cmdline_size )
> {
> 
> and eliminate the goto?

Sure.

>> +        if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
>> +            panic("Error allocating cmdline buffer for %pd\n", d);
>> +
>>           if ( bd->kernel->cmdline_pa )
>> -            safe_strcpy(cmdline,
>> -                        cmdline_cook(__va(bd->kernel->cmdline_pa), 
>> bi->loader));
>> +            strlcpy(cmdline,
>> +                    cmdline_cook(__va(bd->kernel->cmdline_pa),bi- 
>> >loader),
>> +                    cmdline_size);
>>           if ( bi->kextra )
>>               /* kextra always includes exactly one leading space. */
>> -            safe_strcat(cmdline, bi->kextra);
>> +            strlcat(cmdline, bi->kextra, cmdline_size);
>>           /* Append any extra parameters. */
>>           if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
>> -            safe_strcat(cmdline, " noapic");
>> +            strlcat(cmdline, " noapic", cmdline_size);
>>           if ( (strlen(acpi_param) == 0) && acpi_disabled )
>>           {
>> @@ -1030,17 +1060,21 @@ static struct domain *__init 
>> create_dom0(struct boot_info *bi)
>>           if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
>>           {
>> -            safe_strcat(cmdline, " acpi=");
>> -            safe_strcat(cmdline, acpi_param);
>> +            strlcat(cmdline, " acpi=", cmdline_size);
>> +            strlcat(cmdline, acpi_param, cmdline_size);
>>           }
>> -        bd->kernel->cmdline_pa = __pa(cmdline);
>> +        bd->cmdline = cmdline;
> 
> As mentioned above, it looks like you still inadvertently use bd- 
>  >kernel->cmdline_pa and not the new bd->cmdline.  I think clearing bd- 
>  >kernel->cmdline_pa would have helped identify that.  Or do you want to 
> retain cmdline_pa for some reason?  It's less error prone if only one is 
> usable at a time.

When it was just the boot module, it was requested that the pa be 
updated to be the cooked command line. I dropped it here, as we begin to 
transition to multi-domain construction. It is a good point that 
zero-ing will stop inadvertent use of the old/original buffer.

>>       }
>> + skip_cmdline:
>>       bd->d = d;
>>       if ( construct_dom0(bd) != 0 )
>>           panic("Could not construct domain 0\n");
>> +    if ( cmdline )
>> +        xfree(cmdline);
> 
> You can drop the if - xfree() handles NULL.

ack.

v/r,
dps

Re: [PATCH 03/15] x86/boot: add cmdline to struct boot_domain

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address(
>  }
>  
>  static int __init pvh_load_kernel(
> -    struct domain *d, struct boot_module *image, struct boot_module *initrd,
> -    paddr_t *entry, paddr_t *start_info_addr)
> +    struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr)
>  {
> +    struct domain *d = bd->d;
> +    struct boot_module *image = bd->kernel;
> +    struct boot_module *initrd = bd->ramdisk;
>      void *image_base = bootstrap_map_bm(image);
>      void *image_start = image_base + image->headroom;
>      unsigned long image_len = image->size;
> @@ -1304,14 +1306,12 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
>  int __init dom0_construct_pvh(struct boot_domain *bd)
>  {
>      paddr_t entry, start_info;
> -    struct boot_module *image = bd->kernel;
> -    struct boot_module *initrd = bd->ramdisk;
>      struct domain *d = bd->d;
>      int rc;
>  
>      printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
>  
> -    if ( image == NULL )
> +    if ( bd->kernel == NULL )
>          panic("Missing kernel boot module for %pd construction\n", d);
>  
>      if ( is_hardware_domain(d) )
> @@ -1351,7 +1351,7 @@ int __init dom0_construct_pvh(struct boot_domain *bd)
>          return rc;
>      }
>  
> -    rc = pvh_load_kernel(d, image, initrd, &entry, &start_info);
> +    rc = pvh_load_kernel(bd, &entry, &start_info);
>      if ( rc )
>      {
>          printk("Failed to load Dom0 kernel\n");

None of this looks command line related - do these changes rather belong into
patch 1?

Jan

Re: [PATCH 03/15] x86/boot: add cmdline to struct boot_domain

[Daniel P. Smith]

On 12/2/24 04:49, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address(
>>   }
>>   
>>   static int __init pvh_load_kernel(
>> -    struct domain *d, struct boot_module *image, struct boot_module *initrd,
>> -    paddr_t *entry, paddr_t *start_info_addr)
>> +    struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr)
>>   {
>> +    struct domain *d = bd->d;
>> +    struct boot_module *image = bd->kernel;
>> +    struct boot_module *initrd = bd->ramdisk;
>>       void *image_base = bootstrap_map_bm(image);
>>       void *image_start = image_base + image->headroom;
>>       unsigned long image_len = image->size;
>> @@ -1304,14 +1306,12 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
>>   int __init dom0_construct_pvh(struct boot_domain *bd)
>>   {
>>       paddr_t entry, start_info;
>> -    struct boot_module *image = bd->kernel;
>> -    struct boot_module *initrd = bd->ramdisk;
>>       struct domain *d = bd->d;
>>       int rc;
>>   
>>       printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
>>   
>> -    if ( image == NULL )
>> +    if ( bd->kernel == NULL )
>>           panic("Missing kernel boot module for %pd construction\n", d);
>>   
>>       if ( is_hardware_domain(d) )
>> @@ -1351,7 +1351,7 @@ int __init dom0_construct_pvh(struct boot_domain *bd)
>>           return rc;
>>       }
>>   
>> -    rc = pvh_load_kernel(d, image, initrd, &entry, &start_info);
>> +    rc = pvh_load_kernel(bd, &entry, &start_info);
>>       if ( rc )
>>       {
>>           printk("Failed to load Dom0 kernel\n");
> 
> None of this looks command line related - do these changes rather belong into
> patch 1?

Hmmm, yah, it looks like it. This must have been a cherry-pick artifact 
that I missed when updating these commits on top of the version from the 
boot module reviews. I will review and move/drop the chunks.

v/r,
dps

[PATCH 04/15] kconfig: introduce option to independently enable libfdt

[Daniel P. Smith]

Currently the inclusion of libfdt is controlled by the CONFIG_HAS_DEVICE_TREE
kconfig flag. This flag also changes behvaior in a few places, such as boot
module processing for XSM. To support the ability to include libfdt without
changing these behaviors, introduce CONFIG_LIB_DEVICE_TREE. The inclusion of
libfdt is then moved under CONFIG_LIB_DEVICE_TREE.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/common/Kconfig  | 4 ++++
 xen/common/Makefile | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 90268d92499a..5c592dbdc703 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -53,8 +53,12 @@ config HAS_ALTERNATIVE
 config HAS_COMPAT
 	bool
 
+config LIB_DEVICE_TREE
+    bool
+
 config HAS_DEVICE_TREE
 	bool
+    select LIB_DEVICE_TREE
 
 config HAS_DIT # Data Independent Timing
 	bool
diff --git a/xen/common/Makefile b/xen/common/Makefile
index b279b09bfb2b..ff1795de5dda 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -78,7 +78,7 @@ obj-y += sched/
 obj-$(CONFIG_UBSAN) += ubsan/
 
 obj-$(CONFIG_NEEDS_LIBELF) += libelf/
-obj-$(CONFIG_HAS_DEVICE_TREE) += libfdt/
+obj-$(CONFIG_LIB_DEVICE_TREE) += libfdt/
 
 CONF_FILE := $(if $(patsubst /%,,$(KCONFIG_CONFIG)),$(objtree)/)$(KCONFIG_CONFIG)
 $(obj)/config.gz: $(CONF_FILE)
-- 
2.30.2

Re: [PATCH 04/15] kconfig: introduce option to independently enable libfdt

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> Currently the inclusion of libfdt is controlled by the CONFIG_HAS_DEVICE_TREE
> kconfig flag. This flag also changes behvaior in a few places, such as boot

behavior

> module processing for XSM. To support the ability to include libfdt without
> changing these behaviors, introduce CONFIG_LIB_DEVICE_TREE. The inclusion of
> libfdt is then moved under CONFIG_LIB_DEVICE_TREE.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Re: [PATCH 04/15] kconfig: introduce option to independently enable libfdt

[Daniel P. Smith]

On 11/25/24 10:42, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> Currently the inclusion of libfdt is controlled by the 
>> CONFIG_HAS_DEVICE_TREE
>> kconfig flag. This flag also changes behvaior in a few places, such as 
>> boot
> 
> behavior

ack.

>> module processing for XSM. To support the ability to include libfdt 
>> without
>> changing these behaviors, introduce CONFIG_LIB_DEVICE_TREE. The 
>> inclusion of
>> libfdt is then moved under CONFIG_LIB_DEVICE_TREE.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

thanks!

v/r,
dps

Re: [PATCH 04/15] kconfig: introduce option to independently enable libfdt

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -53,8 +53,12 @@ config HAS_ALTERNATIVE
>  config HAS_COMPAT
>  	bool
>  
> +config LIB_DEVICE_TREE
> +    bool

Nit: Indentation is wrong here and ...

>  config HAS_DEVICE_TREE
>  	bool
> +    select LIB_DEVICE_TREE

... here.

Jan

Re: [PATCH 04/15] kconfig: introduce option to independently enable libfdt

[Jan Beulich]

On 26.11.2024 11:03, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -53,8 +53,12 @@ config HAS_ALTERNATIVE
>>  config HAS_COMPAT
>>  	bool
>>  
>> +config LIB_DEVICE_TREE
>> +    bool
> 
> Nit: Indentation is wrong here and ...
> 
>>  config HAS_DEVICE_TREE
>>  	bool
>> +    select LIB_DEVICE_TREE
> 
> ... here.

Oh, and - please don't put LIB_DEVICE_TREE in the middle of (sorted) HAS_*.
It wants to move past them, before MEM_*.

Jan

Re: [PATCH 04/15] kconfig: introduce option to independently enable libfdt

[Daniel P. Smith]

On 11/26/24 05:05, Jan Beulich wrote:
> On 26.11.2024 11:03, Jan Beulich wrote:
>> On 23.11.2024 19:20, Daniel P. Smith wrote:
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -53,8 +53,12 @@ config HAS_ALTERNATIVE
>>>   config HAS_COMPAT
>>>   	bool
>>>   
>>> +config LIB_DEVICE_TREE
>>> +    bool
>>
>> Nit: Indentation is wrong here and ...
>>
>>>   config HAS_DEVICE_TREE
>>>   	bool
>>> +    select LIB_DEVICE_TREE
>>
>> ... here.
> 
> Oh, and - please don't put LIB_DEVICE_TREE in the middle of (sorted) HAS_*.
> It wants to move past them, before MEM_*.

ack.

v/r,
dps

Re: [PATCH 04/15] kconfig: introduce option to independently enable libfdt

[Daniel P. Smith]

On 11/26/24 05:03, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -53,8 +53,12 @@ config HAS_ALTERNATIVE
>>   config HAS_COMPAT
>>   	bool
>>   
>> +config LIB_DEVICE_TREE
>> +    bool
> 
> Nit: Indentation is wrong here and ...
> 
>>   config HAS_DEVICE_TREE
>>   	bool
>> +    select LIB_DEVICE_TREE
> 
> ... here.

ack.

v/r,
dps

[PATCH 05/15] kconfig: introduce domain builder config option

[Daniel P. Smith]

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

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/Kconfig                |  2 ++
 xen/arch/x86/domain_builder/Kconfig | 15 +++++++++++++++
 2 files changed, 17 insertions(+)
 create mode 100644 xen/arch/x86/domain_builder/Kconfig

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 9cdd04721afa..25b9b75423c5 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -383,6 +383,8 @@ config ALTP2M
 
 	  If unsure, stay with defaults.
 
+source "arch/x86/domain_builder/Kconfig"
+
 endmenu
 
 source "common/Kconfig"
diff --git a/xen/arch/x86/domain_builder/Kconfig b/xen/arch/x86/domain_builder/Kconfig
new file mode 100644
index 000000000000..7be2ec3ed00f
--- /dev/null
+++ b/xen/arch/x86/domain_builder/Kconfig
@@ -0,0 +1,15 @@
+
+menu "Domain Builder Features"
+
+config DOMAIN_BUILDER
+	bool "Domain builder (UNSUPPORTED)" if UNSUPPORTED
+	select LIB_DEVICE_TREE
+	help
+      Enables the domain builder capability to configure boot domain
+	  construction using a flattened device tree.
+
+	  This feature is currently experimental.
+
+	  If unsure, say N.
+
+endmenu
-- 
2.30.2

Re: [PATCH 05/15] kconfig: introduce domain builder config option

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> Hyperlaunch domain builder will be the consolidated boot time domain building
> logic framework. Introduces the config option to enable this domain builder to
> and turn on the ability to load the domain configuration via a flattened device

"to and"?

> tree.

Maybe:
"Hyperlaunch is the boot time domain building framework where domain 
configuration is loaded via a flattened device tree.  Introduce a 
kconfig variable to control the feature."

> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

> diff --git a/xen/arch/x86/domain_builder/Kconfig b/xen/arch/x86/domain_builder/Kconfig
> new file mode 100644
> index 000000000000..7be2ec3ed00f
> --- /dev/null
> +++ b/xen/arch/x86/domain_builder/Kconfig
> @@ -0,0 +1,15 @@
> +
> +menu "Domain Builder Features"
> +
> +config DOMAIN_BUILDER
> +	bool "Domain builder (UNSUPPORTED)" if UNSUPPORTED
> +	select LIB_DEVICE_TREE
> +	help
> +      Enables the domain builder capability to configure boot domain

Indent is off.

> +	  construction using a flattened device tree.
> +
> +	  This feature is currently experimental.

Does this need to be unsupported and experimental?  What makes this more 
experimental and/or unsupported than any other new feature?

At least with the commit message and indent fixes:

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

> +
> +	  If unsure, say N.
> +
> +endmenu

Re: [PATCH 05/15] kconfig: introduce domain builder config option

[Daniel P. Smith]

On 11/25/24 12:55, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> Hyperlaunch domain builder will be the consolidated boot time domain 
>> building
>> logic framework. Introduces the config option to enable this domain 
>> builder to
>> and turn on the ability to load the domain configuration via a 
>> flattened device
> 
> "to and"?
> 
>> tree.
> 
> Maybe:
> "Hyperlaunch is the boot time domain building framework where domain 
> configuration is loaded via a flattened device tree.  Introduce a 
> kconfig variable to control the feature."

Sure.

>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
>> diff --git a/xen/arch/x86/domain_builder/Kconfig b/xen/arch/x86/ 
>> domain_builder/Kconfig
>> new file mode 100644
>> index 000000000000..7be2ec3ed00f
>> --- /dev/null
>> +++ b/xen/arch/x86/domain_builder/Kconfig
>> @@ -0,0 +1,15 @@
>> +
>> +menu "Domain Builder Features"
>> +
>> +config DOMAIN_BUILDER
>> +    bool "Domain builder (UNSUPPORTED)" if UNSUPPORTED
>> +    select LIB_DEVICE_TREE
>> +    help
>> +      Enables the domain builder capability to configure boot domain
> 
> Indent is off.

ack

>> +      construction using a flattened device tree.
>> +
>> +      This feature is currently experimental.
> 
> Does this need to be unsupported and experimental?  What makes this more 
> experimental and/or unsupported than any other new feature?

I don't believe it is a unilateral decision I get to make. In fact, with 
the directory introduction, a new HYPERLAUNCH section in MAINTAINERS 
might be warranted with this commit. If so, I would think myself, 
Christopher, and the x86 maintainers would all be set as maintainers for 
the feature.

> At least with the commit message and indent fixes:
> 
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

thanks!

v/r,
dps

Re: [PATCH 05/15] kconfig: introduce domain builder config option

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> Hyperlaunch domain builder will be the consolidated boot time domain building
> logic framework. Introduces the config option to enable this domain builder to
> and turn on the ability to load the domain configuration via a flattened device
> tree.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>  xen/arch/x86/Kconfig                |  2 ++
>  xen/arch/x86/domain_builder/Kconfig | 15 +++++++++++++++
>  2 files changed, 17 insertions(+)
>  create mode 100644 xen/arch/x86/domain_builder/Kconfig

I think I mentioned this already back when the much bigger series was first
posted: Please no underscores in new file (or directory) names; dashes are
to be preferred.

Jan

Re: [PATCH 05/15] kconfig: introduce domain builder config option

[Daniel P. Smith]

On 11/26/24 05:09, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> Hyperlaunch domain builder will be the consolidated boot time domain building
>> logic framework. Introduces the config option to enable this domain builder to
>> and turn on the ability to load the domain configuration via a flattened device
>> tree.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>   xen/arch/x86/Kconfig                |  2 ++
>>   xen/arch/x86/domain_builder/Kconfig | 15 +++++++++++++++
>>   2 files changed, 17 insertions(+)
>>   create mode 100644 xen/arch/x86/domain_builder/Kconfig
> 
> I think I mentioned this already back when the much bigger series was first
> posted: Please no underscores in new file (or directory) names; dashes are
> to be preferred.

You are correct, my apologies for dropping that and I will fix it.

v/r,
dps

[PATCH 06/15] x86/hyperlaunch: introduce the domain builder

[Daniel P. Smith]

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>
---
 xen/arch/x86/Makefile                    |  2 +
 xen/arch/x86/domain_builder/Makefile     |  3 ++
 xen/arch/x86/domain_builder/core.c       | 55 ++++++++++++++++++++++++
 xen/arch/x86/domain_builder/fdt.c        | 38 ++++++++++++++++
 xen/arch/x86/domain_builder/fdt.h        | 21 +++++++++
 xen/arch/x86/include/asm/bootinfo.h      |  3 ++
 xen/arch/x86/include/asm/domainbuilder.h |  8 ++++
 xen/arch/x86/setup.c                     | 18 +++++---
 8 files changed, 142 insertions(+), 6 deletions(-)
 create mode 100644 xen/arch/x86/domain_builder/Makefile
 create mode 100644 xen/arch/x86/domain_builder/core.c
 create mode 100644 xen/arch/x86/domain_builder/fdt.c
 create mode 100644 xen/arch/x86/domain_builder/fdt.h
 create mode 100644 xen/arch/x86/include/asm/domainbuilder.h

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index b35fd5196ce2..45e4c963edcd 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -81,6 +81,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
 obj-y += sysctl.o
 endif
 
+obj-y += domain_builder/
+
 extra-y += asm-macros.i
 extra-y += xen.lds
 
diff --git a/xen/arch/x86/domain_builder/Makefile b/xen/arch/x86/domain_builder/Makefile
new file mode 100644
index 000000000000..309a0c4bdd9e
--- /dev/null
+++ b/xen/arch/x86/domain_builder/Makefile
@@ -0,0 +1,3 @@
+obj-$(CONFIG_DOMAIN_BUILDER) += fdt.init.o
+obj-y += core.init.o
+
diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/domain_builder/core.c
new file mode 100644
index 000000000000..211359895d84
--- /dev/null
+++ b/xen/arch/x86/domain_builder/core.c
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024, Apertus Solutions, LLC
+ */
+#include <xen/err.h>
+#include <xen/init.h>
+#include <xen/kconfig.h>
+#include <xen/lib.h>
+
+#include <asm/bootinfo.h>
+
+#include "fdt.h"
+
+void __init builder_init(struct boot_info *bi)
+{
+    if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
+    {
+        int ret;
+
+        switch ( ret = has_hyperlaunch_fdt(bi) )
+        {
+        case 0:
+            printk("Hyperlaunch device tree detected\n");
+            bi->hyperlaunch_enabled = true;
+            bi->mods[0].type = BOOTMOD_FDT;
+            break;
+        case -EINVAL:
+            printk("Hyperlaunch device tree was not detected\n");
+            bi->hyperlaunch_enabled = false;
+            break;
+        case -ENOENT:
+            fallthrough;
+        case -ENODATA:
+            printk("Device tree found, but not hyperlaunch (%d)\n", ret);
+            bi->hyperlaunch_enabled = false;
+            bi->mods[0].type = BOOTMOD_FDT;
+            break;
+        default:
+            printk("Unknown error (%d) occured checking for hyperlaunch device tree\n",
+                   ret);
+            bi->hyperlaunch_enabled = false;
+        }
+
+    }
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
new file mode 100644
index 000000000000..3f9dda8c34c3
--- /dev/null
+++ b/xen/arch/x86/domain_builder/fdt.c
@@ -0,0 +1,38 @@
+/* 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 <xen/rangeset.h> /* required for asm/setup.h */
+
+#include <asm/bootinfo.h>
+#include <asm/page.h>
+#include <asm/setup.h>
+
+#include "fdt.h"
+
+int __init has_hyperlaunch_fdt(struct boot_info *bi)
+{
+    int ret = 0;
+    void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+
+    if ( fdt_check_header(fdt) < 0 )
+        ret = -EINVAL;
+
+    bootstrap_unmap();
+
+    return ret;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/domain_builder/fdt.h
new file mode 100644
index 000000000000..1c1569a9c633
--- /dev/null
+++ b/xen/arch/x86/domain_builder/fdt.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) */
+#ifndef __XEN_X86_FDT_H__
+#define __XEN_X86_FDT_H__
+
+#include <xen/init.h>
+
+#include <asm/bootinfo.h>
+
+/* hyperlaunch fdt is required to be module 0 */
+#define HYPERLAUNCH_MODULE_IDX 0
+
+#ifdef CONFIG_DOMAIN_BUILDER
+int has_hyperlaunch_fdt(struct boot_info *bi);
+#else
+static inline int __init has_hyperlaunch_fdt(struct boot_info *bi)
+{
+    return -EINVAL;
+}
+#endif
+
+#endif /* __XEN_X86_FDT_H__ */
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 9f65e2c8f62d..208bec90913d 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/include/asm/domainbuilder.h b/xen/arch/x86/include/asm/domainbuilder.h
new file mode 100644
index 000000000000..aedc2b49f7c9
--- /dev/null
+++ b/xen/arch/x86/include/asm/domainbuilder.h
@@ -0,0 +1,8 @@
+#ifndef __XEN_X86_DOMBUILDER_H__
+#define __XEN_X86_DOMBUILDER_H__
+
+#include <asm/bootinfo.h>
+
+void builder_init(struct boot_info *bi);
+
+#endif
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index e6580382d247..8041aeb3dcfd 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -33,6 +33,7 @@
 #endif
 #include <xen/bitops.h>
 #include <asm/bootinfo.h>
+#include <asm/domainbuilder.h>
 #include <asm/smp.h>
 #include <asm/processor.h>
 #include <asm/mpspec.h>
@@ -1277,9 +1278,12 @@ 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];
+    builder_init(bi);
+
+    /* 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];
 
     if ( pvh_boot )
     {
@@ -1466,8 +1470,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);
+    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
+    bi->mods[i].headroom =
+        bzimage_headroom(bootstrap_map_bm(&bi->mods[i]), bi->mods[i].size);
     bootstrap_unmap();
 
 #ifndef highmem_start
@@ -1591,7 +1596,8 @@ void asmlinkage __init noreturn __start_xen(void)
 #endif
     }
 
-    if ( bi->mods[0].headroom && !bi->mods[0].relocated )
+    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
+    if ( bi->mods[i].headroom && !bi->mods[0].relocated )
         panic("Not enough memory to relocate the dom0 kernel image\n");
     for ( i = 0; i < bi->nr_modules; ++i )
     {
-- 
2.30.2

Re: [PATCH 06/15] x86/hyperlaunch: introduce the domain builder

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> 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>

> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index b35fd5196ce2..45e4c963edcd 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -81,6 +81,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
>   obj-y += sysctl.o
>   endif
>   
> +obj-y += domain_builder/

I kinda expected:
obj-$(CONFIG_DOMAIN_BUILDER) += domain_builder/

then ...

> +
>   extra-y += asm-macros.i
>   extra-y += xen.lds
>   

> diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/domain_builder/core.c
> new file mode 100644
> index 000000000000..211359895d84
> --- /dev/null
> +++ b/xen/arch/x86/domain_builder/core.c
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024, Apertus Solutions, LLC
> + */
> +#include <xen/err.h>
> +#include <xen/init.h>
> +#include <xen/kconfig.h>
> +#include <xen/lib.h>
> +
> +#include <asm/bootinfo.h>
> +
> +#include "fdt.h"
> +
> +void __init builder_init(struct boot_info *bi)
> +{
> +    if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )

... then this extra level of indent isn't necessary (with an empty 
static inline builder_init()).

I guess this way, this small part is compiled even when 
CONFIG_DOMAIN_BUILDER is disabled.  But it's only a piece, so I'm not 
sure if it's worth it.

> +    {
> +        int ret;
> +
> +        switch ( ret = has_hyperlaunch_fdt(bi) )
> +        {
> +        case 0:
> +            printk("Hyperlaunch device tree detected\n");
> +            bi->hyperlaunch_enabled = true;
> +            bi->mods[0].type = BOOTMOD_FDT;
> +            break;
> +        case -EINVAL:
> +            printk("Hyperlaunch device tree was not detected\n");
> +            bi->hyperlaunch_enabled = false;
> +            break;
> +        case -ENOENT:
> +            fallthrough;
> +        case -ENODATA:
> +            printk("Device tree found, but not hyperlaunch (%d)\n", ret);
> +            bi->hyperlaunch_enabled = false;
> +            bi->mods[0].type = BOOTMOD_FDT;
> +            break;
> +        default:
> +            printk("Unknown error (%d) occured checking for hyperlaunch device tree\n",
> +                   ret);
> +            bi->hyperlaunch_enabled = false;
> +        }
> +

Stray blank line

> +    }
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
> new file mode 100644
> index 000000000000..3f9dda8c34c3
> --- /dev/null
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -0,0 +1,38 @@
> +/* 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 <xen/rangeset.h> /* required for asm/setup.h */

Should asm/setup.h just be changed?

> +
> +#include <asm/bootinfo.h>
> +#include <asm/page.h>
> +#include <asm/setup.h>
> +
> +#include "fdt.h"
> +

> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index e6580382d247..8041aeb3dcfd 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c

> @@ -1591,7 +1596,8 @@ void asmlinkage __init noreturn __start_xen(void)
>   #endif
>       }
>   
> -    if ( bi->mods[0].headroom && !bi->mods[0].relocated )
> +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
> +    if ( bi->mods[i].headroom && !bi->mods[0].relocated )

Switch .relocated index to i?

Regards,
Jason

>           panic("Not enough memory to relocate the dom0 kernel image\n");
>       for ( i = 0; i < bi->nr_modules; ++i )
>       {

Re: [PATCH 06/15] x86/hyperlaunch: introduce the domain builder

[Jason Andryuk]

On 2024-11-25 12:52, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> +void __init builder_init(struct boot_info *bi)
>> +{
>> +    if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
> 
> ... then this extra level of indent isn't necessary (with an empty 
> static inline builder_init()).
> 
> I guess this way, this small part is compiled even when 
> CONFIG_DOMAIN_BUILDER is disabled.  But it's only a piece, so I'm not 
> sure if it's worth it.

Later in the series, I see more code is added here for non-Hyperlaunch. 
Disregard this comment.

Thanks,
Jason

Re: [PATCH 06/15] x86/hyperlaunch: introduce the domain builder

[Jan Beulich]

On 25.11.2024 18:52, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> --- /dev/null
>> +++ b/xen/arch/x86/domain_builder/fdt.c
>> @@ -0,0 +1,38 @@
>> +/* 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 <xen/rangeset.h> /* required for asm/setup.h */
> 
> Should asm/setup.h just be changed?

Why would it need changing (and why's that #include needed)? It has a
proper forward decl of the struct tag, and I can't see why it would need
anything else.

Jan

Re: [PATCH 06/15] x86/hyperlaunch: introduce the domain builder

[Jason Andryuk]

On 2024-12-02 04:55, Jan Beulich wrote:
> On 25.11.2024 18:52, Jason Andryuk wrote:
>> On 2024-11-23 13:20, Daniel P. Smith wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/domain_builder/fdt.c
>>> @@ -0,0 +1,38 @@
>>> +/* 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 <xen/rangeset.h> /* required for asm/setup.h */
>>
>> Should asm/setup.h just be changed?
> 
> Why would it need changing (and why's that #include needed)? It has a
> proper forward decl of the struct tag, and I can't see why it would need
> anything else.

My question was suggesting to just make the changes, but that was 
already done by Frediano in aa4ad424f0 ("x86/setup: Make setup.h header 
self contained").

I think Dan's comment predates aa4ad424f0.  So it is now stale and the 
whole line should be removed.

Regards,
Jason

Re: [PATCH 06/15] x86/hyperlaunch: introduce the domain builder

[Daniel P. Smith]

On 11/25/24 12:52, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> 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>

<snip/>

>> +            bi->hyperlaunch_enabled = false;
>> +            bi->mods[0].type = BOOTMOD_FDT;
>> +            break;
>> +        default:
>> +            printk("Unknown error (%d) occured checking for 
>> hyperlaunch device tree\n",
>> +                   ret);
>> +            bi->hyperlaunch_enabled = false;
>> +        }
>> +
> 
> Stray blank line

ack.

>> +    }
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/ 
>> domain_builder/fdt.c
>> new file mode 100644
>> index 000000000000..3f9dda8c34c3
>> --- /dev/null
>> +++ b/xen/arch/x86/domain_builder/fdt.c
>> @@ -0,0 +1,38 @@
>> +/* 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 <xen/rangeset.h> /* required for asm/setup.h */
> 
> Should asm/setup.h just be changed?

Will drop per the follow-

>> +
>> +#include <asm/bootinfo.h>
>> +#include <asm/page.h>
>> +#include <asm/setup.h>
>> +
>> +#include "fdt.h"
>> +
> 
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index e6580382d247..8041aeb3dcfd 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
> 
>> @@ -1591,7 +1596,8 @@ void asmlinkage __init noreturn __start_xen(void)
>>   #endif
>>       }
>> -    if ( bi->mods[0].headroom && !bi->mods[0].relocated )
>> +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
>> +    if ( bi->mods[i].headroom && !bi->mods[0].relocated )
> 
> Switch .relocated index to i?

ack.

v//r,
dps

Re: [PATCH 06/15] x86/hyperlaunch: introduce the domain builder

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> 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>
> ---
>  xen/arch/x86/Makefile                    |  2 +
>  xen/arch/x86/domain_builder/Makefile     |  3 ++
>  xen/arch/x86/domain_builder/core.c       | 55 ++++++++++++++++++++++++
>  xen/arch/x86/domain_builder/fdt.c        | 38 ++++++++++++++++
>  xen/arch/x86/domain_builder/fdt.h        | 21 +++++++++
>  xen/arch/x86/include/asm/bootinfo.h      |  3 ++
>  xen/arch/x86/include/asm/domainbuilder.h |  8 ++++
>  xen/arch/x86/setup.c                     | 18 +++++---
>  8 files changed, 142 insertions(+), 6 deletions(-)
>  create mode 100644 xen/arch/x86/domain_builder/Makefile
>  create mode 100644 xen/arch/x86/domain_builder/core.c
>  create mode 100644 xen/arch/x86/domain_builder/fdt.c
>  create mode 100644 xen/arch/x86/domain_builder/fdt.h

As I'm sure I indicated before: Dashes instead of underscores please in new
files' names.

>  create mode 100644 xen/arch/x86/include/asm/domainbuilder.h

Why is there no separator in this file's name?

Similar question as on an earlier patch: Why is all of this x86-specific, when
a goal was generalization?

> --- /dev/null
> +++ b/xen/arch/x86/domain_builder/core.c
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024, Apertus Solutions, LLC
> + */
> +#include <xen/err.h>
> +#include <xen/init.h>
> +#include <xen/kconfig.h>
> +#include <xen/lib.h>
> +
> +#include <asm/bootinfo.h>
> +
> +#include "fdt.h"
> +
> +void __init builder_init(struct boot_info *bi)
> +{
> +    if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
> +    {
> +        int ret;
> +
> +        switch ( ret = has_hyperlaunch_fdt(bi) )
> +        {
> +        case 0:
> +            printk("Hyperlaunch device tree detected\n");
> +            bi->hyperlaunch_enabled = true;
> +            bi->mods[0].type = BOOTMOD_FDT;
> +            break;
> +        case -EINVAL:
> +            printk("Hyperlaunch device tree was not detected\n");
> +            bi->hyperlaunch_enabled = false;
> +            break;
> +        case -ENOENT:
> +            fallthrough;

No need for this.

> +        case -ENODATA:
> +            printk("Device tree found, but not hyperlaunch (%d)\n", ret);
> +            bi->hyperlaunch_enabled = false;
> +            bi->mods[0].type = BOOTMOD_FDT;
> +            break;
> +        default:
> +            printk("Unknown error (%d) occured checking for hyperlaunch device tree\n",
> +                   ret);
> +            bi->hyperlaunch_enabled = false;
> +        }

Nit: Misra demands "break" at the end of default as well.

Blank lines between non-fallthrough blocks would also be nice.

> +

Nit: Excess blank line.

> --- /dev/null
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -0,0 +1,38 @@
> +/* 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 <xen/rangeset.h> /* required for asm/setup.h */
> +
> +#include <asm/bootinfo.h>
> +#include <asm/page.h>
> +#include <asm/setup.h>
> +
> +#include "fdt.h"
> +
> +int __init has_hyperlaunch_fdt(struct boot_info *bi)
> +{
> +    int ret = 0;
> +    void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);

const void *?

> @@ -1277,9 +1278,12 @@ 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];
> +    builder_init(bi);
> +
> +    /* 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];

Better latch the result here into a separate local variable, for use ...

> @@ -1466,8 +1470,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);
> +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
> +    bi->mods[i].headroom =
> +        bzimage_headroom(bootstrap_map_bm(&bi->mods[i]), bi->mods[i].size);
>      bootstrap_unmap();
>  
>  #ifndef highmem_start
> @@ -1591,7 +1596,8 @@ void asmlinkage __init noreturn __start_xen(void)
>  #endif
>      }
>  
> -    if ( bi->mods[0].headroom && !bi->mods[0].relocated )
> +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
> +    if ( bi->mods[i].headroom && !bi->mods[0].relocated )
>          panic("Not enough memory to relocate the dom0 kernel image\n");
>      for ( i = 0; i < bi->nr_modules; ++i )
>      {

... in these two places?

Jan

Re: [PATCH 06/15] x86/hyperlaunch: introduce the domain builder

[Daniel P. Smith]

On 12/2/24 05:10, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> 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>
>> ---
>>   xen/arch/x86/Makefile                    |  2 +
>>   xen/arch/x86/domain_builder/Makefile     |  3 ++
>>   xen/arch/x86/domain_builder/core.c       | 55 ++++++++++++++++++++++++
>>   xen/arch/x86/domain_builder/fdt.c        | 38 ++++++++++++++++
>>   xen/arch/x86/domain_builder/fdt.h        | 21 +++++++++
>>   xen/arch/x86/include/asm/bootinfo.h      |  3 ++
>>   xen/arch/x86/include/asm/domainbuilder.h |  8 ++++
>>   xen/arch/x86/setup.c                     | 18 +++++---
>>   8 files changed, 142 insertions(+), 6 deletions(-)
>>   create mode 100644 xen/arch/x86/domain_builder/Makefile
>>   create mode 100644 xen/arch/x86/domain_builder/core.c
>>   create mode 100644 xen/arch/x86/domain_builder/fdt.c
>>   create mode 100644 xen/arch/x86/domain_builder/fdt.h
> 
> As I'm sure I indicated before: Dashes instead of underscores please in new
> files' names.
> 
>>   create mode 100644 xen/arch/x86/include/asm/domainbuilder.h
> 
> Why is there no separator in this file's name?

Name was getting a bit long, but can add separator if desired.

> Similar question as on an earlier patch: Why is all of this x86-specific, when
> a goal was generalization?
> 
>> --- /dev/null
>> +++ b/xen/arch/x86/domain_builder/core.c
>> @@ -0,0 +1,55 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2024, Apertus Solutions, LLC
>> + */
>> +#include <xen/err.h>
>> +#include <xen/init.h>
>> +#include <xen/kconfig.h>
>> +#include <xen/lib.h>
>> +
>> +#include <asm/bootinfo.h>
>> +
>> +#include "fdt.h"
>> +
>> +void __init builder_init(struct boot_info *bi)
>> +{
>> +    if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
>> +    {
>> +        int ret;
>> +
>> +        switch ( ret = has_hyperlaunch_fdt(bi) )
>> +        {
>> +        case 0:
>> +            printk("Hyperlaunch device tree detected\n");
>> +            bi->hyperlaunch_enabled = true;
>> +            bi->mods[0].type = BOOTMOD_FDT;
>> +            break;
>> +        case -EINVAL:
>> +            printk("Hyperlaunch device tree was not detected\n");
>> +            bi->hyperlaunch_enabled = false;
>> +            break;
>> +        case -ENOENT:
>> +            fallthrough;
> 
> No need for this.

I thought MISRA called for explicit fallthrough?

>> +        case -ENODATA:
>> +            printk("Device tree found, but not hyperlaunch (%d)\n", ret);
>> +            bi->hyperlaunch_enabled = false;
>> +            bi->mods[0].type = BOOTMOD_FDT;
>> +            break;
>> +        default:
>> +            printk("Unknown error (%d) occured checking for hyperlaunch device tree\n",
>> +                   ret);
>> +            bi->hyperlaunch_enabled = false;
>> +        }
> 
> Nit: Misra demands "break" at the end of default as well.

ack.

> Blank lines between non-fallthrough blocks would also be nice.

sure.

>> +
> 
> Nit: Excess blank line.

ack.

>> --- /dev/null
>> +++ b/xen/arch/x86/domain_builder/fdt.c
>> @@ -0,0 +1,38 @@
>> +/* 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 <xen/rangeset.h> /* required for asm/setup.h */
>> +
>> +#include <asm/bootinfo.h>
>> +#include <asm/page.h>
>> +#include <asm/setup.h>
>> +
>> +#include "fdt.h"
>> +
>> +int __init has_hyperlaunch_fdt(struct boot_info *bi)
>> +{
>> +    int ret = 0;
>> +    void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> 
> const void *?

Hmm. it should be.

>> @@ -1277,9 +1278,12 @@ 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];
>> +    builder_init(bi);
>> +
>> +    /* 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];
> 
> Better latch the result here into a separate local variable, for use ...
> 
>> @@ -1466,8 +1470,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);
>> +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
>> +    bi->mods[i].headroom =
>> +        bzimage_headroom(bootstrap_map_bm(&bi->mods[i]), bi->mods[i].size);
>>       bootstrap_unmap();
>>   
>>   #ifndef highmem_start
>> @@ -1591,7 +1596,8 @@ void asmlinkage __init noreturn __start_xen(void)
>>   #endif
>>       }
>>   
>> -    if ( bi->mods[0].headroom && !bi->mods[0].relocated )
>> +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
>> +    if ( bi->mods[i].headroom && !bi->mods[0].relocated )
>>           panic("Not enough memory to relocate the dom0 kernel image\n");
>>       for ( i = 0; i < bi->nr_modules; ++i )
>>       {
> 
> ... in these two places?

I don't know if a local variable is need. I assume your suggestion is to 
drop the first_boot_module_index() call, but thinking about it, not sure 
why I kept the walk. A direct use of bi->domains[0].kernel could be used 
without the intermediate variable while removing the call.

v/r,
dps

Re: [PATCH 06/15] x86/hyperlaunch: introduce the domain builder

[Jan Beulich]

On 11.12.2024 13:36, Daniel P. Smith wrote:
> On 12/2/24 05:10, Jan Beulich wrote:
>> On 23.11.2024 19:20, Daniel P. Smith wrote:
>>> 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>
>>> ---
>>>   xen/arch/x86/Makefile                    |  2 +
>>>   xen/arch/x86/domain_builder/Makefile     |  3 ++
>>>   xen/arch/x86/domain_builder/core.c       | 55 ++++++++++++++++++++++++
>>>   xen/arch/x86/domain_builder/fdt.c        | 38 ++++++++++++++++
>>>   xen/arch/x86/domain_builder/fdt.h        | 21 +++++++++
>>>   xen/arch/x86/include/asm/bootinfo.h      |  3 ++
>>>   xen/arch/x86/include/asm/domainbuilder.h |  8 ++++
>>>   xen/arch/x86/setup.c                     | 18 +++++---
>>>   8 files changed, 142 insertions(+), 6 deletions(-)
>>>   create mode 100644 xen/arch/x86/domain_builder/Makefile
>>>   create mode 100644 xen/arch/x86/domain_builder/core.c
>>>   create mode 100644 xen/arch/x86/domain_builder/fdt.c
>>>   create mode 100644 xen/arch/x86/domain_builder/fdt.h
>>
>> As I'm sure I indicated before: Dashes instead of underscores please in new
>> files' names.
>>
>>>   create mode 100644 xen/arch/x86/include/asm/domainbuilder.h
>>
>> Why is there no separator in this file's name?
> 
> Name was getting a bit long, but can add separator if desired.

Well, my desire is for the subdir and the header names to match up.
Personally I think that neater to achieve when both have a dash in the
middle.

>>> --- /dev/null
>>> +++ b/xen/arch/x86/domain_builder/core.c
>>> @@ -0,0 +1,55 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Copyright (C) 2024, Apertus Solutions, LLC
>>> + */
>>> +#include <xen/err.h>
>>> +#include <xen/init.h>
>>> +#include <xen/kconfig.h>
>>> +#include <xen/lib.h>
>>> +
>>> +#include <asm/bootinfo.h>
>>> +
>>> +#include "fdt.h"
>>> +
>>> +void __init builder_init(struct boot_info *bi)
>>> +{
>>> +    if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
>>> +    {
>>> +        int ret;
>>> +
>>> +        switch ( ret = has_hyperlaunch_fdt(bi) )
>>> +        {
>>> +        case 0:
>>> +            printk("Hyperlaunch device tree detected\n");
>>> +            bi->hyperlaunch_enabled = true;
>>> +            bi->mods[0].type = BOOTMOD_FDT;
>>> +            break;
>>> +        case -EINVAL:
>>> +            printk("Hyperlaunch device tree was not detected\n");
>>> +            bi->hyperlaunch_enabled = false;
>>> +            break;
>>> +        case -ENOENT:
>>> +            fallthrough;
>>
>> No need for this.
> 
> I thought MISRA called for explicit fallthrough?

Only when there are statements between two case labels. Which ...

>>> +        case -ENODATA:

... isn't the case here.

>>> @@ -1277,9 +1278,12 @@ 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];
>>> +    builder_init(bi);
>>> +
>>> +    /* 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];
>>
>> Better latch the result here into a separate local variable, for use ...
>>
>>> @@ -1466,8 +1470,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);
>>> +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
>>> +    bi->mods[i].headroom =
>>> +        bzimage_headroom(bootstrap_map_bm(&bi->mods[i]), bi->mods[i].size);
>>>       bootstrap_unmap();
>>>   
>>>   #ifndef highmem_start
>>> @@ -1591,7 +1596,8 @@ void asmlinkage __init noreturn __start_xen(void)
>>>   #endif
>>>       }
>>>   
>>> -    if ( bi->mods[0].headroom && !bi->mods[0].relocated )
>>> +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
>>> +    if ( bi->mods[i].headroom && !bi->mods[0].relocated )
>>>           panic("Not enough memory to relocate the dom0 kernel image\n");
>>>       for ( i = 0; i < bi->nr_modules; ++i )
>>>       {
>>
>> ... in these two places?
> 
> I don't know if a local variable is need. I assume your suggestion is to 
> drop the first_boot_module_index() call,

The latter two of the three, yes.

> but thinking about it, not sure 
> why I kept the walk. A direct use of bi->domains[0].kernel could be used 
> without the intermediate variable while removing the call.

If that's possible, the even better.

Jan

Re: [PATCH 06/15] x86/hyperlaunch: introduce the domain builder

[Daniel P. Smith]

On 12/12/24 06:06, Jan Beulich wrote:
> On 11.12.2024 13:36, Daniel P. Smith wrote:
>> On 12/2/24 05:10, Jan Beulich wrote:
>>> On 23.11.2024 19:20, Daniel P. Smith wrote:
>>>> 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>
>>>> ---
>>>>    xen/arch/x86/Makefile                    |  2 +
>>>>    xen/arch/x86/domain_builder/Makefile     |  3 ++
>>>>    xen/arch/x86/domain_builder/core.c       | 55 ++++++++++++++++++++++++
>>>>    xen/arch/x86/domain_builder/fdt.c        | 38 ++++++++++++++++
>>>>    xen/arch/x86/domain_builder/fdt.h        | 21 +++++++++
>>>>    xen/arch/x86/include/asm/bootinfo.h      |  3 ++
>>>>    xen/arch/x86/include/asm/domainbuilder.h |  8 ++++
>>>>    xen/arch/x86/setup.c                     | 18 +++++---
>>>>    8 files changed, 142 insertions(+), 6 deletions(-)
>>>>    create mode 100644 xen/arch/x86/domain_builder/Makefile
>>>>    create mode 100644 xen/arch/x86/domain_builder/core.c
>>>>    create mode 100644 xen/arch/x86/domain_builder/fdt.c
>>>>    create mode 100644 xen/arch/x86/domain_builder/fdt.h
>>>
>>> As I'm sure I indicated before: Dashes instead of underscores please in new
>>> files' names.
>>>
>>>>    create mode 100644 xen/arch/x86/include/asm/domainbuilder.h
>>>
>>> Why is there no separator in this file's name?
>>
>> Name was getting a bit long, but can add separator if desired.
> 
> Well, my desire is for the subdir and the header names to match up.
> Personally I think that neater to achieve when both have a dash in the
> middle.

Sure.

>>>> --- /dev/null
>>>> +++ b/xen/arch/x86/domain_builder/core.c
>>>> @@ -0,0 +1,55 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +/*
>>>> + * Copyright (C) 2024, Apertus Solutions, LLC
>>>> + */
>>>> +#include <xen/err.h>
>>>> +#include <xen/init.h>
>>>> +#include <xen/kconfig.h>
>>>> +#include <xen/lib.h>
>>>> +
>>>> +#include <asm/bootinfo.h>
>>>> +
>>>> +#include "fdt.h"
>>>> +
>>>> +void __init builder_init(struct boot_info *bi)
>>>> +{
>>>> +    if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
>>>> +    {
>>>> +        int ret;
>>>> +
>>>> +        switch ( ret = has_hyperlaunch_fdt(bi) )
>>>> +        {
>>>> +        case 0:
>>>> +            printk("Hyperlaunch device tree detected\n");
>>>> +            bi->hyperlaunch_enabled = true;
>>>> +            bi->mods[0].type = BOOTMOD_FDT;
>>>> +            break;
>>>> +        case -EINVAL:
>>>> +            printk("Hyperlaunch device tree was not detected\n");
>>>> +            bi->hyperlaunch_enabled = false;
>>>> +            break;
>>>> +        case -ENOENT:
>>>> +            fallthrough;
>>>
>>> No need for this.
>>
>> I thought MISRA called for explicit fallthrough?
> 
> Only when there are statements between two case labels. Which ...
> 
>>>> +        case -ENODATA:
> 
> ... isn't the case here.

Rgr, have already dropped it.

>>>> @@ -1277,9 +1278,12 @@ 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];
>>>> +    builder_init(bi);
>>>> +
>>>> +    /* 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];
>>>
>>> Better latch the result here into a separate local variable, for use ...
>>>
>>>> @@ -1466,8 +1470,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);
>>>> +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
>>>> +    bi->mods[i].headroom =
>>>> +        bzimage_headroom(bootstrap_map_bm(&bi->mods[i]), bi->mods[i].size);
>>>>        bootstrap_unmap();
>>>>    
>>>>    #ifndef highmem_start
>>>> @@ -1591,7 +1596,8 @@ void asmlinkage __init noreturn __start_xen(void)
>>>>    #endif
>>>>        }
>>>>    
>>>> -    if ( bi->mods[0].headroom && !bi->mods[0].relocated )
>>>> +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
>>>> +    if ( bi->mods[i].headroom && !bi->mods[0].relocated )
>>>>            panic("Not enough memory to relocate the dom0 kernel image\n");
>>>>        for ( i = 0; i < bi->nr_modules; ++i )
>>>>        {
>>>
>>> ... in these two places?
>>
>> I don't know if a local variable is need. I assume your suggestion is to
>> drop the first_boot_module_index() call,
> 
> The latter two of the three, yes.
> 
>> but thinking about it, not sure
>> why I kept the walk. A direct use of bi->domains[0].kernel could be used
>> without the intermediate variable while removing the call.
> 
> If that's possible, the even better.

Yep, while it did make the lines a little longer, I was able to use the 
boot_domain reference.

v/r,
dps

[PATCH 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree

[Daniel P. Smith]

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 if more than one is found.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/domain_builder/core.c  | 14 +++++++
 xen/arch/x86/domain_builder/fdt.c   | 64 ++++++++++++++++++++++++++++-
 xen/arch/x86/domain_builder/fdt.h   |  5 +++
 xen/arch/x86/include/asm/bootinfo.h |  1 +
 4 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/domain_builder/core.c
index 211359895d84..a80f3711c306 100644
--- a/xen/arch/x86/domain_builder/core.c
+++ b/xen/arch/x86/domain_builder/core.c
@@ -40,7 +40,21 @@ void __init builder_init(struct boot_info *bi)
                    ret);
             bi->hyperlaunch_enabled = false;
         }
+    }
+
+    if ( bi->hyperlaunch_enabled )
+    {
+        int ret;
+
+        printk(XENLOG_INFO "Hyperlauch configuration:\n");
+        if ( (ret = walk_hyperlaunch_fdt(bi)) < 0 )
+        {
+            printk(XENLOG_INFO "  walk of device tree failed (%d)\n", ret);
+            bi->hyperlaunch_enabled = false;
+            return;
+        }
 
+        printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
     }
 }
 
diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
index 3f9dda8c34c3..ff1ba58b6907 100644
--- a/xen/arch/x86/domain_builder/fdt.c
+++ b/xen/arch/x86/domain_builder/fdt.c
@@ -14,14 +14,76 @@
 
 #include "fdt.h"
 
+static int __init find_hyperlaunch_node(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;
+        else
+            return hv_node;
+    }
+    else
+    {
+        /* Lood 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") == 0 )
+                return chosen_node;
+        }
+    }
+
+    return -ENODATA;
+}
+
 int __init has_hyperlaunch_fdt(struct boot_info *bi)
 {
     int ret = 0;
     void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
 
-    if ( fdt_check_header(fdt) < 0 )
+    if ( !fdt || fdt_check_header(fdt) < 0 )
         ret = -EINVAL;
+    else
+        ret = find_hyperlaunch_node(fdt);
+
+    bootstrap_unmap();
+
+    return ret < 0 ? ret : 0;
+}
+
+int __init walk_hyperlaunch_fdt(struct boot_info *bi)
+{
+    int ret = 0, hv_node, node;
+    void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+
+    if ( unlikely(!fdt) )
+        return -EINVAL;
+
+    hv_node = find_hyperlaunch_node(fdt);
+    if ( hv_node < 0 )
+    {
+        ret = hv_node;
+        goto err_out;
+    }
+
+    fdt_for_each_subnode(node, fdt, hv_node)
+    {
+        ret = fdt_node_check_compatible(fdt, node, "xen,domain");
+        if ( ret == 0 )
+            bi->nr_domains++;
+    }
+
+    /* Until multi-domain construction is added, throw an error */
+    if ( !bi->nr_domains || bi->nr_domains > 1 )
+        printk(XENLOG_ERR "Hyperlaunch only supports dom0 construction\n");
 
+ err_out:
     bootstrap_unmap();
 
     return ret;
diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/domain_builder/fdt.h
index 1c1569a9c633..84126db208ea 100644
--- a/xen/arch/x86/domain_builder/fdt.h
+++ b/xen/arch/x86/domain_builder/fdt.h
@@ -11,11 +11,16 @@
 
 #ifdef CONFIG_DOMAIN_BUILDER
 int has_hyperlaunch_fdt(struct boot_info *bi);
+int walk_hyperlaunch_fdt(struct boot_info *bi);
 #else
 static inline int __init has_hyperlaunch_fdt(struct boot_info *bi)
 {
     return -EINVAL;
 }
+static int __init walk_hyperlaunch_fdt(struct boot_info *bi)
+{
+    return -EINVAL;
+}
 #endif
 
 #endif /* __XEN_X86_FDT_H__ */
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 208bec90913d..683ca9dbe2e0 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];
 };
-- 
2.30.2

Re: [PATCH 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> 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 if more than one is found.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>   xen/arch/x86/domain_builder/core.c  | 14 +++++++
>   xen/arch/x86/domain_builder/fdt.c   | 64 ++++++++++++++++++++++++++++-
>   xen/arch/x86/domain_builder/fdt.h   |  5 +++
>   xen/arch/x86/include/asm/bootinfo.h |  1 +
>   4 files changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/domain_builder/core.c
> index 211359895d84..a80f3711c306 100644
> --- a/xen/arch/x86/domain_builder/core.c
> +++ b/xen/arch/x86/domain_builder/core.c
> @@ -40,7 +40,21 @@ void __init builder_init(struct boot_info *bi)
>                      ret);
>               bi->hyperlaunch_enabled = false;
>           }
> +    }
> +
> +    if ( bi->hyperlaunch_enabled )
> +    {
> +        int ret;
> +
> +        printk(XENLOG_INFO "Hyperlauch configuration:\n");

Hyperlaunch

> +        if ( (ret = walk_hyperlaunch_fdt(bi)) < 0 )
> +        {
> +            printk(XENLOG_INFO "  walk of device tree failed (%d)\n", ret);
> +            bi->hyperlaunch_enabled = false;
> +            return;
> +        }
>   
> +        printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
>       }
>   }
>   
> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
> index 3f9dda8c34c3..ff1ba58b6907 100644
> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c

> +int __init walk_hyperlaunch_fdt(struct boot_info *bi)
> +{
> +    int ret = 0, hv_node, node;
> +    void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +
> +    if ( unlikely(!fdt) )
> +        return -EINVAL;
> +
> +    hv_node = find_hyperlaunch_node(fdt);
> +    if ( hv_node < 0 )
> +    {
> +        ret = hv_node;
> +        goto err_out;
> +    }
> +
> +    fdt_for_each_subnode(node, fdt, hv_node)
> +    {
> +        ret = fdt_node_check_compatible(fdt, node, "xen,domain");
> +        if ( ret == 0 )
> +            bi->nr_domains++;
> +    }
> +
> +    /* Until multi-domain construction is added, throw an error */
> +    if ( !bi->nr_domains || bi->nr_domains > 1 )
> +        printk(XENLOG_ERR "Hyperlaunch only supports dom0 construction\n");

You continue execution - is that intended?  It'll take the next module 
as the kernel and try to boot?  Would you rather panic?

Regards,
Jason

>   
> + err_out:
>       bootstrap_unmap();
>   
>       return ret;

Re: [PATCH 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree

[Daniel P. Smith]

On 11/25/24 15:11, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> 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 if more than one is found.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>   xen/arch/x86/domain_builder/core.c  | 14 +++++++
>>   xen/arch/x86/domain_builder/fdt.c   | 64 ++++++++++++++++++++++++++++-
>>   xen/arch/x86/domain_builder/fdt.h   |  5 +++
>>   xen/arch/x86/include/asm/bootinfo.h |  1 +
>>   4 files changed, 83 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/ 
>> domain_builder/core.c
>> index 211359895d84..a80f3711c306 100644
>> --- a/xen/arch/x86/domain_builder/core.c
>> +++ b/xen/arch/x86/domain_builder/core.c
>> @@ -40,7 +40,21 @@ void __init builder_init(struct boot_info *bi)
>>                      ret);
>>               bi->hyperlaunch_enabled = false;
>>           }
>> +    }
>> +
>> +    if ( bi->hyperlaunch_enabled )
>> +    {
>> +        int ret;
>> +
>> +        printk(XENLOG_INFO "Hyperlauch configuration:\n");
> 
> Hyperlaunch

Ack.

>> +        if ( (ret = walk_hyperlaunch_fdt(bi)) < 0 )
>> +        {
>> +            printk(XENLOG_INFO "  walk of device tree failed (%d)\n", 
>> ret);
>> +            bi->hyperlaunch_enabled = false;
>> +            return;
>> +        }
>> +        printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
>>       }
>>   }
>> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/ 
>> domain_builder/fdt.c
>> index 3f9dda8c34c3..ff1ba58b6907 100644
>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
> 
>> +int __init walk_hyperlaunch_fdt(struct boot_info *bi)
>> +{
>> +    int ret = 0, hv_node, node;
>> +    void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
>> +
>> +    if ( unlikely(!fdt) )
>> +        return -EINVAL;
>> +
>> +    hv_node = find_hyperlaunch_node(fdt);
>> +    if ( hv_node < 0 )
>> +    {
>> +        ret = hv_node;
>> +        goto err_out;
>> +    }
>> +
>> +    fdt_for_each_subnode(node, fdt, hv_node)
>> +    {
>> +        ret = fdt_node_check_compatible(fdt, node, "xen,domain");
>> +        if ( ret == 0 )
>> +            bi->nr_domains++;
>> +    }
>> +
>> +    /* Until multi-domain construction is added, throw an error */
>> +    if ( !bi->nr_domains || bi->nr_domains > 1 )
>> +        printk(XENLOG_ERR "Hyperlaunch only supports dom0 
>> construction\n");
> 
> You continue execution - is that intended?  It'll take the next module 
> as the kernel and try to boot?  Would you rather panic?

Yes, it was intended, and as of this commit, it will use the next module 
as the kernel. That is the boot convention at this point. In this 
scenario, the system was given a valid HL device tree that happen to 
have multiple domains defined in it. At this point in the series, a 
domain definition literally has zero effect on the boot process, so 
there is no reason to panic.

v/r,
dps

Re: [PATCH 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> 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 if more than one is found.

"reporting" reads like informational logging, when comment and printk() in
walk_hyperlaunch_fdt() indicate this is actually an error (for now).

> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -14,14 +14,76 @@
>  
>  #include "fdt.h"
>  
> +static int __init find_hyperlaunch_node(void *fdt)
> +{
> +    int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
> +    if ( hv_node >= 0 )

Nit: Blank line between declaration(s) and statement(s) please (also
elsewhere).

> --- a/xen/arch/x86/domain_builder/fdt.h
> +++ b/xen/arch/x86/domain_builder/fdt.h
> @@ -11,11 +11,16 @@
>  
>  #ifdef CONFIG_DOMAIN_BUILDER
>  int has_hyperlaunch_fdt(struct boot_info *bi);
> +int walk_hyperlaunch_fdt(struct boot_info *bi);
>  #else
>  static inline int __init has_hyperlaunch_fdt(struct boot_info *bi)
>  {
>      return -EINVAL;
>  }
> +static int __init walk_hyperlaunch_fdt(struct boot_info *bi)

inline?

Jan

Re: [PATCH 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree

[Daniel P. Smith]

On 12/2/24 06:37, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> 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 if more than one is found.
> 
> "reporting" reads like informational logging, when comment and printk() in
> walk_hyperlaunch_fdt() indicate this is actually an error (for now).

That is not a shared assumptive reading. It is equally correct to say I 
will report info and I will report an error. With that said, I can make 
it explicit.

>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
>> @@ -14,14 +14,76 @@
>>   
>>   #include "fdt.h"
>>   
>> +static int __init find_hyperlaunch_node(void *fdt)
>> +{
>> +    int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
>> +    if ( hv_node >= 0 )
> 
> Nit: Blank line between declaration(s) and statement(s) please (also
> elsewhere).

ack.

>> --- a/xen/arch/x86/domain_builder/fdt.h
>> +++ b/xen/arch/x86/domain_builder/fdt.h
>> @@ -11,11 +11,16 @@
>>   
>>   #ifdef CONFIG_DOMAIN_BUILDER
>>   int has_hyperlaunch_fdt(struct boot_info *bi);
>> +int walk_hyperlaunch_fdt(struct boot_info *bi);
>>   #else
>>   static inline int __init has_hyperlaunch_fdt(struct boot_info *bi)
>>   {
>>       return -EINVAL;
>>   }
>> +static int __init walk_hyperlaunch_fdt(struct boot_info *bi)
> 
> inline?

Should have been.

v/r,
dps

[PATCH 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch

[Daniel P. Smith]

Look for a subnode of type `multiboot,kernel` within a domain node. If found,
process the reg property for the MB1 module index. 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>
---
 xen/arch/x86/domain_builder/core.c |  12 +++
 xen/arch/x86/domain_builder/fdt.c  | 126 +++++++++++++++++++++++++++++
 xen/arch/x86/domain_builder/fdt.h  |  17 ++++
 xen/arch/x86/setup.c               |   5 --
 4 files changed, 155 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/domain_builder/core.c
index a80f3711c306..9335f3a9ebef 100644
--- a/xen/arch/x86/domain_builder/core.c
+++ b/xen/arch/x86/domain_builder/core.c
@@ -56,6 +56,18 @@ void __init builder_init(struct boot_info *bi)
 
         printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
     }
+    else
+    {
+        int i;
+
+        /* Find first unknown boot module to use as Dom0 kernel */
+        printk("Falling back to using first boot module as dom0\n");
+        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;
+    }
+
 }
 
 /*
diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
index ff1ba58b6907..6bf1c4a297fe 100644
--- a/xen/arch/x86/domain_builder/fdt.c
+++ b/xen/arch/x86/domain_builder/fdt.c
@@ -14,6 +14,122 @@
 
 #include "fdt.h"
 
+static inline int __init fdt_get_prop_as_reg(
+    const void *fdt, int node, const char *name, unsigned int ssize,
+    unsigned int asize, uint64_t *size, uint64_t *addr)
+{
+    int ret;
+    const struct fdt_property *prop;
+    fdt32_t *cell;
+
+    /* FDT spec max size is 4 (128bit int), but largest arch int size is 64 */
+    if ( ssize > 2 || asize > 2 )
+        return -EINVAL;
+
+    prop = fdt_get_property(fdt, node, name, &ret);
+    if ( !prop || ret < sizeof(u32) )
+        return ret < 0 ? ret : -EINVAL;
+
+    /* read address field */
+    cell = (fdt32_t *)prop->data;
+
+    if ( asize == 1 )
+    {
+        uint32_t val;
+        fdt_cell_as_u32(cell, &val);
+        *addr = (uint64_t)val;
+    }
+    else
+        fdt_cell_as_u64(cell, addr);
+
+    /* read size field */
+    cell += asize;
+
+    if ( ssize == 1 )
+    {
+        uint32_t val;
+        fdt_cell_as_u32(cell, &val);
+        *size = (uint64_t)val;
+    }
+    else
+        fdt_cell_as_u64(cell, size);
+
+    return 0;
+}
+
+static int __init dom0less_module_node(
+    void *fdt, int node, int size_size, int address_size)
+{
+    uint64_t size, addr;
+    int ret = fdt_get_prop_as_reg(fdt, node, "reg", size_size, address_size,
+                                  &size, &addr);
+    /* An FDT error value may have been returned, translate to -EINVAL */
+    if ( ret < 0 )
+        return -EINVAL;
+
+    if ( size != 0 )
+        return -EOPNOTSUPP;
+
+    if ( addr > MAX_NR_BOOTMODS )
+        return -ERANGE;
+
+    /*
+     * MAX_NR_BOOTMODS cannot exceed the max for MB1, represented by a u32,
+     * thus the cast down to a u32 will be safe due to the prior check.
+     */
+    return (int)addr;
+}
+
+static int __init process_domain_node(
+    struct boot_info *bi, 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);
+    int address_size = fdt_address_cells(fdt, dom_node);
+    int size_size = fdt_size_cells(fdt, dom_node);
+
+    if ( address_size < 0 || size_size < 0 )
+    {
+        printk("  failed processing #address or #size for domain %s)\n",
+               name == NULL ? "unknown" : name);
+        return -EINVAL;
+    }
+
+    fdt_for_each_subnode(node, fdt, dom_node)
+    {
+        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
+        {
+            int idx = dom0less_module_node(fdt, node, size_size, address_size);
+            if ( idx < 0 )
+            {
+                printk("  failed processing kernel module for domain %s)\n",
+                       name == NULL ? "unknown" : name);
+                return idx;
+            }
+
+            if ( idx > bi->nr_modules )
+            {
+                printk("  invalid kernel module index for domain node (%d)\n",
+                       bi->nr_domains);
+                return -EINVAL;
+            }
+
+            printk("  kernel: boot module %d\n", idx);
+            bi->mods[idx].type = BOOTMOD_KERNEL;
+            bd->kernel = &bi->mods[idx];
+        }
+    }
+
+    if ( !bd->kernel )
+    {
+        printk(XENLOG_ERR "ERR: no kernel assigned to domain\n");
+        return -EFAULT;
+    }
+
+    return 0;
+}
+
 static int __init find_hyperlaunch_node(void *fdt)
 {
     int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
@@ -74,9 +190,19 @@ int __init walk_hyperlaunch_fdt(struct boot_info *bi)
 
     fdt_for_each_subnode(node, fdt, hv_node)
     {
+        if ( bi->nr_domains >= MAX_NR_BOOTDOMS )
+        {
+            printk(XENLOG_WARNING "WARN: more domains defined than max allowed");
+            break;
+        }
+
         ret = fdt_node_check_compatible(fdt, node, "xen,domain");
         if ( ret == 0 )
+        {
+            if ( (ret = process_domain_node(bi, fdt, node)) < 0 )
+                break;
             bi->nr_domains++;
+        }
     }
 
     /* Until multi-domain construction is added, throw an error */
diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/domain_builder/fdt.h
index 84126db208ea..558d00a994fa 100644
--- a/xen/arch/x86/domain_builder/fdt.h
+++ b/xen/arch/x86/domain_builder/fdt.h
@@ -3,6 +3,7 @@
 #define __XEN_X86_FDT_H__
 
 #include <xen/init.h>
+#include <xen/libfdt/libfdt.h>
 
 #include <asm/bootinfo.h>
 
@@ -10,6 +11,22 @@
 #define HYPERLAUNCH_MODULE_IDX 0
 
 #ifdef CONFIG_DOMAIN_BUILDER
+
+static inline int __init fdt_cell_as_u32(const fdt32_t *cell, uint32_t *val)
+{
+    *val = fdt32_to_cpu(*cell);
+
+    return 0;
+}
+
+static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
+{
+    *val = ((uint64_t)fdt32_to_cpu(cell[0]) << 32) |
+           (uint64_t)fdt32_to_cpu(cell[1]);
+
+    return 0;
+}
+
 int has_hyperlaunch_fdt(struct boot_info *bi);
 int walk_hyperlaunch_fdt(struct boot_info *bi);
 #else
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 8041aeb3dcfd..d6e9d4c1769c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1280,11 +1280,6 @@ void asmlinkage __init noreturn __start_xen(void)
 
     builder_init(bi);
 
-    /* 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];
-
     if ( pvh_boot )
     {
         /* pvh_init() already filled in e820_raw */
-- 
2.30.2

Re: [PATCH 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> Look for a subnode of type `multiboot,kernel` within a domain node. If found,
> process the reg property for the MB1 module index. 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>

> diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/domain_builder/core.c
> index a80f3711c306..9335f3a9ebef 100644
> --- a/xen/arch/x86/domain_builder/core.c
> +++ b/xen/arch/x86/domain_builder/core.c
> @@ -56,6 +56,18 @@ void __init builder_init(struct boot_info *bi)
>   
>           printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
>       }
> +    else
> +    {
> +        int i;
> +
> +        /* Find first unknown boot module to use as Dom0 kernel */
> +        printk("Falling back to using first boot module as dom0\n");
> +        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;
> +    }
> +

extra newline.

>   }
>   
>   /*
> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
> index ff1ba58b6907..6bf1c4a297fe 100644
> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c

> +static int __init process_domain_node(
> +    struct boot_info *bi, 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);

const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown";

to avoid...

> +    int address_size = fdt_address_cells(fdt, dom_node);
> +    int size_size = fdt_size_cells(fdt, dom_node);
> +
> +    if ( address_size < 0 || size_size < 0 )
> +    {
> +        printk("  failed processing #address or #size for domain %s)\n",
> +               name == NULL ? "unknown" : name);

...all this duplication in the following patches.

> +        return -EINVAL;
> +    }
> +
> +    fdt_for_each_subnode(node, fdt, dom_node)
> +    {
> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )

I thought you were going to use "module,kernel" and "module,index" as 
u32s for multiboot2?

Regards,
Jason

> +        {
> +            int idx = dom0less_module_node(fdt, node, size_size, address_size);
> +            if ( idx < 0 )
> +            {
> +                printk("  failed processing kernel module for domain %s)\n",
> +                       name == NULL ? "unknown" : name);
> +                return idx;
> +            }
> +
> +            if ( idx > bi->nr_modules )
> +            {
> +                printk("  invalid kernel module index for domain node (%d)\n",
> +                       bi->nr_domains);
> +                return -EINVAL;
> +            }
> +
> +            printk("  kernel: boot module %d\n", idx);
> +            bi->mods[idx].type = BOOTMOD_KERNEL;
> +            bd->kernel = &bi->mods[idx];
> +        }
> +    }
> +
> +    if ( !bd->kernel )
> +    {
> +        printk(XENLOG_ERR "ERR: no kernel assigned to domain\n");
> +        return -EFAULT;
> +    }
> +
> +    return 0;
> +}
> +
>   static int __init find_hyperlaunch_node(void *fdt)
>   {
>       int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");

Re: [PATCH 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch

[Daniel P. Smith]

On 11/25/24 17:54, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> Look for a subnode of type `multiboot,kernel` within a domain node. If 
>> found,
>> process the reg property for the MB1 module index. 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>
> 
>> diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/ 
>> domain_builder/core.c
>> index a80f3711c306..9335f3a9ebef 100644
>> --- a/xen/arch/x86/domain_builder/core.c
>> +++ b/xen/arch/x86/domain_builder/core.c
>> @@ -56,6 +56,18 @@ void __init builder_init(struct boot_info *bi)
>>           printk(XENLOG_INFO "  Number of domains: %d\n", bi- 
>> >nr_domains);
>>       }
>> +    else
>> +    {
>> +        int i;
>> +
>> +        /* Find first unknown boot module to use as Dom0 kernel */
>> +        printk("Falling back to using first boot module as dom0\n");
>> +        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;
>> +    }
>> +
> 
> extra newline.

ack.

>>   }
>>   /*
>> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/ 
>> domain_builder/fdt.c
>> index ff1ba58b6907..6bf1c4a297fe 100644
>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
> 
>> +static int __init process_domain_node(
>> +    struct boot_info *bi, 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);
> 
> const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown";
> 
> to avoid...

Sure.

>> +    int address_size = fdt_address_cells(fdt, dom_node);
>> +    int size_size = fdt_size_cells(fdt, dom_node);
>> +
>> +    if ( address_size < 0 || size_size < 0 )
>> +    {
>> +        printk("  failed processing #address or #size for domain %s)\n",
>> +               name == NULL ? "unknown" : name);
> 
> ...all this duplication in the following patches.
> 
>> +        return -EINVAL;
>> +    }
>> +
>> +    fdt_for_each_subnode(node, fdt, dom_node)
>> +    {
>> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") 
>> == 0 )
> 
> I thought you were going to use "module,kernel" and "module,index" as 
> u32s for multiboot2?

Per our discussion, I will update appropriately.

v/r,
dps

Re: [PATCH 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> Look for a subnode of type `multiboot,kernel` within a domain node. If found,
> process the reg property for the MB1 module index. If the bootargs property is
> present and there was not an MB1 string, then use the command line from the
> device tree definition.

Why specifically MB1?

> --- a/xen/arch/x86/domain_builder/core.c
> +++ b/xen/arch/x86/domain_builder/core.c
> @@ -56,6 +56,18 @@ void __init builder_init(struct boot_info *bi)
>  
>          printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
>      }
> +    else
> +    {
> +        int i;

Plain int when ...

> +        /* Find first unknown boot module to use as Dom0 kernel */
> +        printk("Falling back to using first boot module as dom0\n");
> +        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;
> +    }

... it's used as array index (and there's no check for the function return
value being negative)?

> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -14,6 +14,122 @@
>  
>  #include "fdt.h"
>  
> +static inline int __init fdt_get_prop_as_reg(

What does "reg" stand for here?

> +    const void *fdt, int node, const char *name, unsigned int ssize,
> +    unsigned int asize, uint64_t *size, uint64_t *addr)
> +{
> +    int ret;
> +    const struct fdt_property *prop;
> +    fdt32_t *cell;
> +
> +    /* FDT spec max size is 4 (128bit int), but largest arch int size is 64 */
> +    if ( ssize > 2 || asize > 2 )
> +        return -EINVAL;
> +
> +    prop = fdt_get_property(fdt, node, name, &ret);
> +    if ( !prop || ret < sizeof(u32) )
> +        return ret < 0 ? ret : -EINVAL;
> +
> +    /* read address field */
> +    cell = (fdt32_t *)prop->data;
> +
> +    if ( asize == 1 )
> +    {
> +        uint32_t val;
> +        fdt_cell_as_u32(cell, &val);
> +        *addr = (uint64_t)val;

No need for a cast here nor ...

> +    }
> +    else
> +        fdt_cell_as_u64(cell, addr);
> +
> +    /* read size field */
> +    cell += asize;
> +
> +    if ( ssize == 1 )
> +    {
> +        uint32_t val;
> +        fdt_cell_as_u32(cell, &val);
> +        *size = (uint64_t)val;

... here?

> +    }
> +    else
> +        fdt_cell_as_u64(cell, size);
> +
> +    return 0;
> +}

This whole function reads very much like a library one. Does it really need
adding here, rather than to the FDT library code we already have? In any
event there's nothing x86-specific about it, afaics.

> +static int __init dom0less_module_node(
> +    void *fdt, int node, int size_size, int address_size)

Three times plain int, when ...

> +{
> +    uint64_t size, addr;
> +    int ret = fdt_get_prop_as_reg(fdt, node, "reg", size_size, address_size,

... two get converted to unsigned int in the course of the function call
here?

> +                                  &size, &addr);
> +    /* An FDT error value may have been returned, translate to -EINVAL */
> +    if ( ret < 0 )
> +        return -EINVAL;
> +
> +    if ( size != 0 )
> +        return -EOPNOTSUPP;

Not knowing much about DT: What does 0 represent here?

> +    if ( addr > MAX_NR_BOOTMODS )
> +        return -ERANGE;
> +
> +    /*
> +     * MAX_NR_BOOTMODS cannot exceed the max for MB1, represented by a u32,
> +     * thus the cast down to a u32 will be safe due to the prior check.
> +     */
> +    return (int)addr;

Comment and cast contradict one another. DYM u32 (really: uint32_t), or plain
int? If you mean to return a plain int (for the sake of the -errno values
further up), MAX_NR_BOOTMODS needs to stay below 2**31.

> +static int __init process_domain_node(
> +    struct boot_info *bi, 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);
> +    int address_size = fdt_address_cells(fdt, dom_node);
> +    int size_size = fdt_size_cells(fdt, dom_node);
> +
> +    if ( address_size < 0 || size_size < 0 )
> +    {
> +        printk("  failed processing #address or #size for domain %s)\n",
> +               name == NULL ? "unknown" : name);
> +        return -EINVAL;
> +    }
> +
> +    fdt_for_each_subnode(node, fdt, dom_node)
> +    {
> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
> +        {
> +            int idx = dom0less_module_node(fdt, node, size_size, address_size);
> +            if ( idx < 0 )
> +            {
> +                printk("  failed processing kernel module for domain %s)\n",
> +                       name == NULL ? "unknown" : name);
> +                return idx;
> +            }
> +
> +            if ( idx > bi->nr_modules )
> +            {
> +                printk("  invalid kernel module index for domain node (%d)\n",
> +                       bi->nr_domains);
> +                return -EINVAL;
> +            }
> +
> +            printk("  kernel: boot module %d\n", idx);
> +            bi->mods[idx].type = BOOTMOD_KERNEL;
> +            bd->kernel = &bi->mods[idx];
> +        }
> +    }

What if you find two?

> --- a/xen/arch/x86/domain_builder/fdt.h
> +++ b/xen/arch/x86/domain_builder/fdt.h
> @@ -3,6 +3,7 @@
>  #define __XEN_X86_FDT_H__
>  
>  #include <xen/init.h>
> +#include <xen/libfdt/libfdt.h>
>  
>  #include <asm/bootinfo.h>
>  
> @@ -10,6 +11,22 @@
>  #define HYPERLAUNCH_MODULE_IDX 0
>  
>  #ifdef CONFIG_DOMAIN_BUILDER
> +
> +static inline int __init fdt_cell_as_u32(const fdt32_t *cell, uint32_t *val)
> +{
> +    *val = fdt32_to_cpu(*cell);
> +
> +    return 0;
> +}
> +
> +static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
> +{
> +    *val = ((uint64_t)fdt32_to_cpu(cell[0]) << 32) |
> +           (uint64_t)fdt32_to_cpu(cell[1]);
> +
> +    return 0;
> +}

Basic library routines again?

Jan

Re: [PATCH 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch

[Daniel P. Smith]

On 12/2/24 06:53, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> Look for a subnode of type `multiboot,kernel` within a domain node. If found,
>> process the reg property for the MB1 module index. If the bootargs property is
>> present and there was not an MB1 string, then use the command line from the
>> device tree definition.
> 
> Why specifically MB1?

Because Xen converts MB2 into an MB1 chain very early in the entry 
points that take MB2. By the time HL code is executed, it will only ever 
see a list of MB1 modules.

>> --- a/xen/arch/x86/domain_builder/core.c
>> +++ b/xen/arch/x86/domain_builder/core.c
>> @@ -56,6 +56,18 @@ void __init builder_init(struct boot_info *bi)
>>   
>>           printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
>>       }
>> +    else
>> +    {
>> +        int i;
> 
> Plain int when ...
> 
>> +        /* Find first unknown boot module to use as Dom0 kernel */
>> +        printk("Falling back to using first boot module as dom0\n");
>> +        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;
>> +    }
> 
> ... it's used as array index (and there's no check for the function return
> value being negative)?

Nope, that is an artifact from early version of boot module series in 
which first_boot_module_index() returned a plain int. Will fix.

>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
>> @@ -14,6 +14,122 @@
>>   
>>   #include "fdt.h"
>>   
>> +static inline int __init fdt_get_prop_as_reg(
> 
> What does "reg" stand for here?

Device Tree defines a two field "prop-encoded=array" property type 
called reg.

>> +    const void *fdt, int node, const char *name, unsigned int ssize,
>> +    unsigned int asize, uint64_t *size, uint64_t *addr)
>> +{
>> +    int ret;
>> +    const struct fdt_property *prop;
>> +    fdt32_t *cell;
>> +
>> +    /* FDT spec max size is 4 (128bit int), but largest arch int size is 64 */
>> +    if ( ssize > 2 || asize > 2 )
>> +        return -EINVAL;
>> +
>> +    prop = fdt_get_property(fdt, node, name, &ret);
>> +    if ( !prop || ret < sizeof(u32) )
>> +        return ret < 0 ? ret : -EINVAL;
>> +
>> +    /* read address field */
>> +    cell = (fdt32_t *)prop->data;
>> +
>> +    if ( asize == 1 )
>> +    {
>> +        uint32_t val;
>> +        fdt_cell_as_u32(cell, &val);
>> +        *addr = (uint64_t)val;
> 
> No need for a cast here nor ...
> 
>> +    }
>> +    else
>> +        fdt_cell_as_u64(cell, addr);
>> +
>> +    /* read size field */
>> +    cell += asize;
>> +
>> +    if ( ssize == 1 )
>> +    {
>> +        uint32_t val;
>> +        fdt_cell_as_u32(cell, &val);
>> +        *size = (uint64_t)val;
> 
> ... here?

No the compiler does not need the cast, placed to remind readers what 
was being done. Can/will drop.

>> +    }
>> +    else
>> +        fdt_cell_as_u64(cell, size);
>> +
>> +    return 0;
>> +}
> 
> This whole function reads very much like a library one. Does it really need
> adding here, rather than to the FDT library code we already have? In any
> event there's nothing x86-specific about it, afaics.

This is where it gets complicated. Most of the higher order functions 
exposed by xen/device_tree.h are written to work with FDT indexing 
structures, referred to as the unflattened tree. Deconflicting the mixed 
use of FDT and FDT index in device_tree.h is beyond the scope of this 
series.

>> +static int __init dom0less_module_node(
>> +    void *fdt, int node, int size_size, int address_size)
> 
> Three times plain int, when ...
> 
>> +{
>> +    uint64_t size, addr;
>> +    int ret = fdt_get_prop_as_reg(fdt, node, "reg", size_size, address_size,
> 
> ... two get converted to unsigned int in the course of the function call
> here?

The libfdt function returns signed int for size_size and address_size to 
allow returning an error code, which is checked for at the time of 
invocation. After that point, the value is guaranteed to be >= 0. They 
can be left as int in the function calls or a pair of temporary plain 
ints could be used for the libfdt call and if no error, store the value 
into unsigned.

>> +                                  &size, &addr);
>> +    /* An FDT error value may have been returned, translate to -EINVAL */
>> +    if ( ret < 0 )
>> +        return -EINVAL;
>> +
>> +    if ( size != 0 )
>> +        return -EOPNOTSUPP;
> 
> Not knowing much about DT: What does 0 represent here?

The libfdt code treats 0 as a valid value, whether zero is a valid value 
is driven by the contextual usage of the property.

>> +    if ( addr > MAX_NR_BOOTMODS )
>> +        return -ERANGE;
>> +
>> +    /*
>> +     * MAX_NR_BOOTMODS cannot exceed the max for MB1, represented by a u32,
>> +     * thus the cast down to a u32 will be safe due to the prior check.
>> +     */
>> +    return (int)addr;
> 
> Comment and cast contradict one another. DYM u32 (really: uint32_t), or plain
> int? If you mean to return a plain int (for the sake of the -errno values
> further up), MAX_NR_BOOTMODS needs to stay below 2**31.

Good point, we cannot artificially impose 2^31 limit when 2^32 is the 
legitimate upper bound supported by the MB1 protocol. Even if that value 
is impractical. At the same time, it is beneficial to be able to 
communicate failures along with some delineation of the failure. Let me 
think about this, and in the meantime suggestions are welcomed.

>> +static int __init process_domain_node(
>> +    struct boot_info *bi, 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);
>> +    int address_size = fdt_address_cells(fdt, dom_node);
>> +    int size_size = fdt_size_cells(fdt, dom_node);
>> +
>> +    if ( address_size < 0 || size_size < 0 )
>> +    {
>> +        printk("  failed processing #address or #size for domain %s)\n",
>> +               name == NULL ? "unknown" : name);
>> +        return -EINVAL;
>> +    }
>> +
>> +    fdt_for_each_subnode(node, fdt, dom_node)
>> +    {
>> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
>> +        {
>> +            int idx = dom0less_module_node(fdt, node, size_size, address_size);
>> +            if ( idx < 0 )
>> +            {
>> +                printk("  failed processing kernel module for domain %s)\n",
>> +                       name == NULL ? "unknown" : name);
>> +                return idx;
>> +            }
>> +
>> +            if ( idx > bi->nr_modules )
>> +            {
>> +                printk("  invalid kernel module index for domain node (%d)\n",
>> +                       bi->nr_domains);
>> +                return -EINVAL;
>> +            }
>> +
>> +            printk("  kernel: boot module %d\n", idx);
>> +            bi->mods[idx].type = BOOTMOD_KERNEL;
>> +            bd->kernel = &bi->mods[idx];
>> +        }
>> +    }
> 
> What if you find two?

No different than if someone accidentally duplicated the module line for 
the kernel in grub.cfg. It's a violation of the boot convention with the 
resulting behavior being indeterminate, which may or may not result in 
failure/panic when the domain attempts to boot. It might be worth adding 
a warning if a duplicate kernel entry is detected. It is possible that 
such a configuration would boot if it was a duplicate paste situation. 
So I would not feel right panicking, when there is a possibility that 
the configuration could boot.

>> --- a/xen/arch/x86/domain_builder/fdt.h
>> +++ b/xen/arch/x86/domain_builder/fdt.h
>> @@ -3,6 +3,7 @@
>>   #define __XEN_X86_FDT_H__
>>   
>>   #include <xen/init.h>
>> +#include <xen/libfdt/libfdt.h>
>>   
>>   #include <asm/bootinfo.h>
>>   
>> @@ -10,6 +11,22 @@
>>   #define HYPERLAUNCH_MODULE_IDX 0
>>   
>>   #ifdef CONFIG_DOMAIN_BUILDER
>> +
>> +static inline int __init fdt_cell_as_u32(const fdt32_t *cell, uint32_t *val)
>> +{
>> +    *val = fdt32_to_cpu(*cell);
>> +
>> +    return 0;
>> +}
>> +
>> +static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
>> +{
>> +    *val = ((uint64_t)fdt32_to_cpu(cell[0]) << 32) |
>> +           (uint64_t)fdt32_to_cpu(cell[1]);
>> +
>> +    return 0;
>> +}
> 
> Basic library routines again?

Same as above.

v/r,
dps

Re: [PATCH 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch

[Jan Beulich]

On 11.12.2024 16:41, Daniel P. Smith wrote:
> On 12/2/24 06:53, Jan Beulich wrote:
>> On 23.11.2024 19:20, Daniel P. Smith wrote:
>>> Look for a subnode of type `multiboot,kernel` within a domain node. If found,
>>> process the reg property for the MB1 module index. If the bootargs property is
>>> present and there was not an MB1 string, then use the command line from the
>>> device tree definition.
>>
>> Why specifically MB1?
> 
> Because Xen converts MB2 into an MB1 chain very early in the entry 
> points that take MB2. By the time HL code is executed, it will only ever 
> see a list of MB1 modules.

Yet that's all Xen's internal representation, which merely is kind of
originating from MB1. That origin is, afaict, irrelevant here, and is
instead, imo, confusing.

>>> --- a/xen/arch/x86/domain_builder/fdt.c
>>> +++ b/xen/arch/x86/domain_builder/fdt.c
>>> @@ -14,6 +14,122 @@
>>>   
>>>   #include "fdt.h"
>>>   
>>> +static inline int __init fdt_get_prop_as_reg(
>>
>> What does "reg" stand for here?
> 
> Device Tree defines a two field "prop-encoded=array" property type 
> called reg.

Okay, this explains where you took it from, yet I remain curious what it
actually stands for. Just from the letters I would derive "register", yet
that seems unlikely here.

>>> +    const void *fdt, int node, const char *name, unsigned int ssize,
>>> +    unsigned int asize, uint64_t *size, uint64_t *addr)
>>> +{
>>> +    int ret;
>>> +    const struct fdt_property *prop;
>>> +    fdt32_t *cell;
>>> +
>>> +    /* FDT spec max size is 4 (128bit int), but largest arch int size is 64 */
>>> +    if ( ssize > 2 || asize > 2 )
>>> +        return -EINVAL;
>>> +
>>> +    prop = fdt_get_property(fdt, node, name, &ret);
>>> +    if ( !prop || ret < sizeof(u32) )
>>> +        return ret < 0 ? ret : -EINVAL;
>>> +
>>> +    /* read address field */
>>> +    cell = (fdt32_t *)prop->data;
>>> +
>>> +    if ( asize == 1 )
>>> +    {
>>> +        uint32_t val;
>>> +        fdt_cell_as_u32(cell, &val);
>>> +        *addr = (uint64_t)val;
>>
>> No need for a cast here nor ...
>>
>>> +    }
>>> +    else
>>> +        fdt_cell_as_u64(cell, addr);
>>> +
>>> +    /* read size field */
>>> +    cell += asize;
>>> +
>>> +    if ( ssize == 1 )
>>> +    {
>>> +        uint32_t val;
>>> +        fdt_cell_as_u32(cell, &val);
>>> +        *size = (uint64_t)val;
>>
>> ... here?
> 
> No the compiler does not need the cast, placed to remind readers what 
> was being done. Can/will drop.
> 
>>> +    }
>>> +    else
>>> +        fdt_cell_as_u64(cell, size);
>>> +
>>> +    return 0;
>>> +}
>>
>> This whole function reads very much like a library one. Does it really need
>> adding here, rather than to the FDT library code we already have? In any
>> event there's nothing x86-specific about it, afaics.
> 
> This is where it gets complicated. Most of the higher order functions 
> exposed by xen/device_tree.h are written to work with FDT indexing 
> structures, referred to as the unflattened tree. Deconflicting the mixed 
> use of FDT and FDT index in device_tree.h is beyond the scope of this 
> series.

Going that far also wasn't my request. Yet it's still library-like code,
which seems odd to introduce as x86-specific when later it may very well
want using by other architectures as well. If the FDT library code isn't
a good place, then put it somewhere under xen/lib/?

>>> +                                  &size, &addr);
>>> +    /* An FDT error value may have been returned, translate to -EINVAL */
>>> +    if ( ret < 0 )
>>> +        return -EINVAL;
>>> +
>>> +    if ( size != 0 )
>>> +        return -EOPNOTSUPP;
>>
>> Not knowing much about DT: What does 0 represent here?
> 
> The libfdt code treats 0 as a valid value, whether zero is a valid value 
> is driven by the contextual usage of the property.

And 0 is the _only_ valid value here? Another comment may be on order, to
at least briefly indicate why this is.

>>> +    if ( addr > MAX_NR_BOOTMODS )
>>> +        return -ERANGE;
>>> +
>>> +    /*
>>> +     * MAX_NR_BOOTMODS cannot exceed the max for MB1, represented by a u32,
>>> +     * thus the cast down to a u32 will be safe due to the prior check.
>>> +     */
>>> +    return (int)addr;
>>
>> Comment and cast contradict one another. DYM u32 (really: uint32_t), or plain
>> int? If you mean to return a plain int (for the sake of the -errno values
>> further up), MAX_NR_BOOTMODS needs to stay below 2**31.
> 
> Good point, we cannot artificially impose 2^31 limit when 2^32 is the 
> legitimate upper bound supported by the MB1 protocol. Even if that value 
> is impractical. At the same time, it is beneficial to be able to 
> communicate failures along with some delineation of the failure. Let me 
> think about this, and in the meantime suggestions are welcomed.

    BUILD_BUG_ON(MAX_NR_BOOTMODS > INT_MAX);

deferring the thinking about the "bigger than this" aspect until a (perhaps
much) later time?

>>> +static int __init process_domain_node(
>>> +    struct boot_info *bi, 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);
>>> +    int address_size = fdt_address_cells(fdt, dom_node);
>>> +    int size_size = fdt_size_cells(fdt, dom_node);
>>> +
>>> +    if ( address_size < 0 || size_size < 0 )
>>> +    {
>>> +        printk("  failed processing #address or #size for domain %s)\n",
>>> +               name == NULL ? "unknown" : name);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    fdt_for_each_subnode(node, fdt, dom_node)
>>> +    {
>>> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
>>> +        {
>>> +            int idx = dom0less_module_node(fdt, node, size_size, address_size);
>>> +            if ( idx < 0 )
>>> +            {
>>> +                printk("  failed processing kernel module for domain %s)\n",
>>> +                       name == NULL ? "unknown" : name);
>>> +                return idx;
>>> +            }
>>> +
>>> +            if ( idx > bi->nr_modules )
>>> +            {
>>> +                printk("  invalid kernel module index for domain node (%d)\n",
>>> +                       bi->nr_domains);
>>> +                return -EINVAL;
>>> +            }
>>> +
>>> +            printk("  kernel: boot module %d\n", idx);
>>> +            bi->mods[idx].type = BOOTMOD_KERNEL;
>>> +            bd->kernel = &bi->mods[idx];
>>> +        }
>>> +    }
>>
>> What if you find two?
> 
> No different than if someone accidentally duplicated the module line for 
> the kernel in grub.cfg. It's a violation of the boot convention with the 
> resulting behavior being indeterminate, which may or may not result in 
> failure/panic when the domain attempts to boot. It might be worth adding 
> a warning if a duplicate kernel entry is detected. It is possible that 
> such a configuration would boot if it was a duplicate paste situation. 
> So I would not feel right panicking, when there is a possibility that 
> the configuration could boot.

I agree with not panic()ing, and I didn't mean to ask that you do so. A
warning ought to be enough indeed.

Jan

[PATCH 09/15] x86/hyperlaunch: obtain cmdline from device tree

[Daniel P. Smith]

If a command line is not provided through the bootloader's mechanism, e.g.
muiltboot module string field, then use one from the device tree if present.
The device tree command line is located in the bootargs property of the
`multiboot,kernel` node.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/domain_builder/core.c       | 28 +++++++++++++++++++
 xen/arch/x86/domain_builder/fdt.c        | 34 ++++++++++++++++++++++++
 xen/arch/x86/domain_builder/fdt.h        | 24 +++++++++++++++++
 xen/arch/x86/include/asm/bootinfo.h      |  6 +++--
 xen/arch/x86/include/asm/domainbuilder.h |  4 +++
 xen/arch/x86/setup.c                     | 10 +++++--
 6 files changed, 102 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/domain_builder/core.c
index 9335f3a9ebef..95cab06e6159 100644
--- a/xen/arch/x86/domain_builder/core.c
+++ b/xen/arch/x86/domain_builder/core.c
@@ -8,9 +8,37 @@
 #include <xen/lib.h>
 
 #include <asm/bootinfo.h>
+#include <asm/setup.h>
 
 #include "fdt.h"
 
+size_t __init builder_get_cmdline_size(struct boot_info *bi, int offset)
+{
+#ifdef CONFIG_DOMAIN_BUILDER
+    const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+    int size = fdt_cmdline_prop_size(fdt, offset);
+
+    bootstrap_unmap();
+    return size < 0 ? 0 : (size_t) size;
+#else
+    return 0;
+#endif
+}
+
+int __init builder_get_cmdline(
+    struct boot_info *bi, int offset, char *cmdline, size_t size)
+{
+#ifdef CONFIG_DOMAIN_BUILDER
+    const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+    int ret = fdt_cmdline_prop_copy(fdt, offset, cmdline, size);
+
+    bootstrap_unmap();
+    return ret;
+#else
+    return 0;
+#endif
+}
+
 void __init builder_init(struct boot_info *bi)
 {
     if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
index 6bf1c4a297fe..f8ddb11b339e 100644
--- a/xen/arch/x86/domain_builder/fdt.c
+++ b/xen/arch/x86/domain_builder/fdt.c
@@ -57,6 +57,30 @@ static inline int __init fdt_get_prop_as_reg(
     return 0;
 }
 
+static int __init fdt_get_prop_as_offset(
+    const void *fdt, int node, const char *name, unsigned long *a)
+{
+    int ret, poffset;
+    const char *pname;
+    size_t nsize = strlen(name);
+
+    fdt_for_each_property_offset(poffset, fdt, node)
+    {
+        fdt_getprop_by_offset(fdt, poffset, &pname, &ret);
+
+        if ( ret < 0 || strlen(pname) != nsize )
+            continue;
+
+        if ( !strncmp(pname, name, nsize) )
+        {
+            *a = poffset;
+            return nsize;
+        }
+    }
+
+    return -ENOENT;
+}
+
 static int __init dom0less_module_node(
     void *fdt, int node, int size_size, int address_size)
 {
@@ -118,6 +142,16 @@ static int __init process_domain_node(
             printk("  kernel: boot module %d\n", idx);
             bi->mods[idx].type = BOOTMOD_KERNEL;
             bd->kernel = &bi->mods[idx];
+
+            /* If bootloader didn't set cmdline, see if FDT provides one. */
+            if ( bd->kernel->cmdline_pa &&
+                 !((char *)__va(bd->kernel->cmdline_pa))[0] )
+            {
+                int ret = fdt_get_prop_as_offset(
+                    fdt, node, "bootargs", &bd->kernel->cmdline_pa);
+                if ( ret > 0 )
+                    bd->kernel->fdt_cmdline = true;
+            }
         }
     }
 
diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/domain_builder/fdt.h
index 558d00a994fa..ab2b43872e25 100644
--- a/xen/arch/x86/domain_builder/fdt.h
+++ b/xen/arch/x86/domain_builder/fdt.h
@@ -27,6 +27,30 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
     return 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);
+}
+
 int has_hyperlaunch_fdt(struct boot_info *bi);
 int walk_hyperlaunch_fdt(struct boot_info *bi);
 #else
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 683ca9dbe2e0..433a0e66121b 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -35,11 +35,13 @@ struct boot_module {
 
     /*
      * Module State Flags:
-     *   relocated: indicates module has been relocated in memory.
-     *   released:  indicates module's pages have been freed.
+     *   relocated:   indicates module has been relocated in memory.
+     *   released:    indicates module's pages have been freed.
+     *   fdt_cmdline: indicates module's cmdline is in the FDT.
      */
     bool relocated:1;
     bool released:1;
+    bool fdt_cmdline:1;
 
     /*
      * A boot module may need decompressing by Xen.  Headroom is an estimate of
diff --git a/xen/arch/x86/include/asm/domainbuilder.h b/xen/arch/x86/include/asm/domainbuilder.h
index aedc2b49f7c9..21221d8df8ec 100644
--- a/xen/arch/x86/include/asm/domainbuilder.h
+++ b/xen/arch/x86/include/asm/domainbuilder.h
@@ -3,6 +3,10 @@
 
 #include <asm/bootinfo.h>
 
+size_t __init builder_get_cmdline_size(struct boot_info *bi, int offset);
+int __init builder_get_cmdline(
+    struct boot_info *bi, int offset, char *cmdline, size_t size);
+
 void builder_init(struct boot_info *bi);
 
 #endif
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d6e9d4c1769c..fda4fc71e205 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -971,7 +971,10 @@ static size_t __init domain_cmdline_size(
 {
     size_t s = bi->kextra ? strlen(bi->kextra) : 0;
 
-    s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
+    if ( bd->kernel->fdt_cmdline )
+        s += builder_get_cmdline_size(bi, bd->kernel->cmdline_pa);
+    else
+        s += strlen(__va(bd->kernel->cmdline_pa));
 
     if ( s == 0 )
         return s;
@@ -1040,7 +1043,10 @@ static struct domain *__init create_dom0(struct boot_info *bi)
         if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
             panic("Error allocating cmdline buffer for %pd\n", d);
 
-        if ( bd->kernel->cmdline_pa )
+        if ( bd->kernel->fdt_cmdline )
+            builder_get_cmdline(
+                bi, bd->kernel->cmdline_pa, cmdline, cmdline_size);
+        else
             strlcpy(cmdline,
                     cmdline_cook(__va(bd->kernel->cmdline_pa),bi->loader),
                     cmdline_size);
-- 
2.30.2

Re: [PATCH 09/15] x86/hyperlaunch: obtain cmdline from device tree

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> If a command line is not provided through the bootloader's mechanism, e.g.
> muiltboot module string field, then use one from the device tree if present.
> The device tree command line is located in the bootargs property of the
> `multiboot,kernel` node.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>   xen/arch/x86/domain_builder/core.c       | 28 +++++++++++++++++++
>   xen/arch/x86/domain_builder/fdt.c        | 34 ++++++++++++++++++++++++
>   xen/arch/x86/domain_builder/fdt.h        | 24 +++++++++++++++++
>   xen/arch/x86/include/asm/bootinfo.h      |  6 +++--
>   xen/arch/x86/include/asm/domainbuilder.h |  4 +++
>   xen/arch/x86/setup.c                     | 10 +++++--
>   6 files changed, 102 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/domain_builder/core.c
> index 9335f3a9ebef..95cab06e6159 100644
> --- a/xen/arch/x86/domain_builder/core.c
> +++ b/xen/arch/x86/domain_builder/core.c
> @@ -8,9 +8,37 @@
>   #include <xen/lib.h>
>   
>   #include <asm/bootinfo.h>
> +#include <asm/setup.h>
>   
>   #include "fdt.h"
>   
> +size_t __init builder_get_cmdline_size(struct boot_info *bi, int offset)
> +{
> +#ifdef CONFIG_DOMAIN_BUILDER

I wasnted to suggest:

if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
     return 0;

but that fails to compile for a missing fdt_cmdline_prop_size().

> +    const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +    int size = fdt_cmdline_prop_size(fdt, offset);
> +
> +    bootstrap_unmap();
> +    return size < 0 ? 0 : (size_t) size;
> +#else
> +    return 0;
> +#endif
> +}
> +
> +int __init builder_get_cmdline(
> +    struct boot_info *bi, int offset, char *cmdline, size_t size)
> +{
> +#ifdef CONFIG_DOMAIN_BUILDER

and here fdt_cmdline_prop_copy().  I'm not sure the addition of more 
stubs offsets these ifdefs, so:

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Regards,
Jason

Re: [PATCH 09/15] x86/hyperlaunch: obtain cmdline from device tree

[Daniel P. Smith]

On 11/25/24 18:12, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> If a command line is not provided through the bootloader's mechanism, 
>> e.g.
>> muiltboot module string field, then use one from the device tree if 
>> present.
>> The device tree command line is located in the bootargs property of the
>> `multiboot,kernel` node.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>   xen/arch/x86/domain_builder/core.c       | 28 +++++++++++++++++++
>>   xen/arch/x86/domain_builder/fdt.c        | 34 ++++++++++++++++++++++++
>>   xen/arch/x86/domain_builder/fdt.h        | 24 +++++++++++++++++
>>   xen/arch/x86/include/asm/bootinfo.h      |  6 +++--
>>   xen/arch/x86/include/asm/domainbuilder.h |  4 +++
>>   xen/arch/x86/setup.c                     | 10 +++++--
>>   6 files changed, 102 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/ 
>> domain_builder/core.c
>> index 9335f3a9ebef..95cab06e6159 100644
>> --- a/xen/arch/x86/domain_builder/core.c
>> +++ b/xen/arch/x86/domain_builder/core.c
>> @@ -8,9 +8,37 @@
>>   #include <xen/lib.h>
>>   #include <asm/bootinfo.h>
>> +#include <asm/setup.h>
>>   #include "fdt.h"
>> +size_t __init builder_get_cmdline_size(struct boot_info *bi, int offset)
>> +{
>> +#ifdef CONFIG_DOMAIN_BUILDER
> 
> I wasnted to suggest:
> 
> if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
>      return 0;
> 
> but that fails to compile for a missing fdt_cmdline_prop_size().

Yes, I wanted to do the same as well.

>> +    const void *fdt = bootstrap_map_bm(&bi- 
>> >mods[HYPERLAUNCH_MODULE_IDX]);
>> +    int size = fdt_cmdline_prop_size(fdt, offset);
>> +
>> +    bootstrap_unmap();
>> +    return size < 0 ? 0 : (size_t) size;
>> +#else
>> +    return 0;
>> +#endif
>> +}
>> +
>> +int __init builder_get_cmdline(
>> +    struct boot_info *bi, int offset, char *cmdline, size_t size)
>> +{
>> +#ifdef CONFIG_DOMAIN_BUILDER
> 
> and here fdt_cmdline_prop_copy().  I'm not sure the addition of more 
> stubs offsets these ifdefs, so:
> 
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Thanks!

v/r,
dps

[PATCH 10/15] x86/hyperlaunch: locate dom0 initrd with hyperlaunch

[Daniel P. Smith]

Look for a subnode of type `multiboot,ramdisk` within a domain node. If
found, process the reg property for the MB1 module index.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/domain_builder/fdt.c | 25 ++++++++++++++++++++++
 xen/arch/x86/setup.c              | 35 +++++++++++++++++--------------
 2 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
index f8ddb11b339e..bc8054a80ec1 100644
--- a/xen/arch/x86/domain_builder/fdt.c
+++ b/xen/arch/x86/domain_builder/fdt.c
@@ -152,6 +152,31 @@ static int __init process_domain_node(
                 if ( ret > 0 )
                     bd->kernel->fdt_cmdline = true;
             }
+
+            continue;
+        }
+        if ( fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
+        {
+            int idx = dom0less_module_node(fdt, node, size_size, address_size);
+            if ( idx < 0 )
+            {
+                printk("  failed processing ramdisk module for domain %s\n",
+                       name == NULL ? "unknown" : name);
+                return -EINVAL;
+            }
+
+            if ( idx > bi->nr_modules )
+            {
+                printk("  invalid ramdisk module index for domain node (%d)\n",
+                       bi->nr_domains);
+                return -EINVAL;
+            }
+
+            printk("  ramdisk: boot module %d\n", idx);
+            bi->mods[idx].type = BOOTMOD_RAMDISK;
+            bd->ramdisk = &bi->mods[idx];
+
+            continue;
         }
     }
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index fda4fc71e205..eaac87b02f78 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1094,7 +1094,7 @@ void asmlinkage __init noreturn __start_xen(void)
     char *kextra;
     void *bsp_stack;
     struct cpu_info *info = get_cpu_info(), *bsp_info;
-    unsigned int initrdidx, num_parked = 0;
+    unsigned int num_parked = 0;
     struct boot_info *bi;
     unsigned long nr_pages, raw_max_page;
     int i, j, e820_warn = 0, bytes = 0;
@@ -2137,22 +2137,25 @@ void asmlinkage __init noreturn __start_xen(void)
            cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
            cpu_has_nx ? "" : "not ");
 
-    /*
-     * 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
-     * candidates.
-     */
-    initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
-    if ( initrdidx < MAX_NR_BOOTMODS )
+    if ( !bi->hyperlaunch_enabled )
     {
-        bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
-        bi->domains[0].ramdisk = &bi->mods[initrdidx];
-        if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
-            printk(XENLOG_WARNING
-                   "Multiple initrd candidates, picking module #%u\n",
-                   initrdidx);
+        /*
+         * 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
+         * candidates.
+         */
+        unsigned int initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
+        if ( initrdidx < MAX_NR_BOOTMODS )
+        {
+            bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
+            bi->domains[0].ramdisk = &bi->mods[initrdidx];
+            if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
+                printk(XENLOG_WARNING
+                       "Multiple initrd candidates, picking module #%u\n",
+                       initrdidx);
+        }
     }
 
     /*
-- 
2.30.2

Re: [PATCH 10/15] x86/hyperlaunch: locate dom0 initrd with hyperlaunch

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> Look for a subnode of type `multiboot,ramdisk` within a domain node. If
> found, process the reg property for the MB1 module index.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>   xen/arch/x86/domain_builder/fdt.c | 25 ++++++++++++++++++++++
>   xen/arch/x86/setup.c              | 35 +++++++++++++++++--------------
>   2 files changed, 44 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
> index f8ddb11b339e..bc8054a80ec1 100644
> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -152,6 +152,31 @@ static int __init process_domain_node(
>                   if ( ret > 0 )
>                       bd->kernel->fdt_cmdline = true;
>               }
> +
> +            continue;
> +        }
> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )

I think
         continue;
     }
     if

should change to

     } else if

?

Also "module,ramdisk"/"module,index"?

Regards,
Jason

Re: [PATCH 10/15] x86/hyperlaunch: locate dom0 initrd with hyperlaunch

[Daniel P. Smith]

On 11/25/24 18:34, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> Look for a subnode of type `multiboot,ramdisk` within a domain node. If
>> found, process the reg property for the MB1 module index.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>   xen/arch/x86/domain_builder/fdt.c | 25 ++++++++++++++++++++++
>>   xen/arch/x86/setup.c              | 35 +++++++++++++++++--------------
>>   2 files changed, 44 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/ 
>> domain_builder/fdt.c
>> index f8ddb11b339e..bc8054a80ec1 100644
>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
>> @@ -152,6 +152,31 @@ static int __init process_domain_node(
>>                   if ( ret > 0 )
>>                       bd->kernel->fdt_cmdline = true;
>>               }
>> +
>> +            continue;
>> +        }
>> +        if ( fdt_node_check_compatible(fdt, node, 
>> "multiboot,ramdisk") == 0 )
> 
> I think
>          continue;
>      }
>      if
> 
> should change to
> 
>      } else if
> 
> ?

Yah, I can make it a nested set of ifs.

> Also "module,ramdisk"/"module,index"?

Yes, will be updated to new format.

v/r,
dps

[PATCH 11/15] x86/hyperlaunch: add domain id parsing to domain config

[Daniel P. Smith]

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>
---
 xen/arch/x86/domain_builder/fdt.c | 31 ++++++++++++++++++++++++++++++-
 xen/arch/x86/domain_builder/fdt.h | 18 ++++++++++++++++++
 xen/arch/x86/setup.c              |  3 ++-
 3 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
index bc8054a80ec1..3a6b4fbc09a9 100644
--- a/xen/arch/x86/domain_builder/fdt.c
+++ b/xen/arch/x86/domain_builder/fdt.c
@@ -9,6 +9,7 @@
 #include <xen/rangeset.h> /* required for asm/setup.h */
 
 #include <asm/bootinfo.h>
+#include <asm/guest.h>
 #include <asm/page.h>
 #include <asm/setup.h>
 
@@ -107,7 +108,7 @@ static int __init dom0less_module_node(
 static int __init process_domain_node(
     struct boot_info *bi, 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);
     int address_size = fdt_address_cells(fdt, dom_node);
@@ -120,6 +121,28 @@ static int __init process_domain_node(
         return -EINVAL;
     }
 
+    fdt_for_each_property_offset(property, fdt, dom_node)
+    {
+        const struct fdt_property *prop;
+
+        prop = fdt_get_property_by_offset(fdt, property, NULL);
+        if ( !prop )
+            continue; /* silently skip */
+
+        if ( match_fdt_property(fdt, prop, "domid" ) )
+        {
+            uint32_t val = DOMID_INVALID;
+            if ( fdt_prop_as_u32(prop, &val) != 0 )
+            {
+                printk("  failed processing domain id for domain %s\n",
+                       name == NULL ? "unknown" : name);
+                return -EINVAL;
+            }
+            bd->domid = (domid_t)val;
+            printk("  domid: %d\n", bd->domid);
+        }
+    }
+
     fdt_for_each_subnode(node, fdt, dom_node)
     {
         if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
@@ -186,6 +209,12 @@ static int __init process_domain_node(
         return -EFAULT;
     }
 
+    if ( bd->domid == DOMID_INVALID )
+        bd->domid = get_initial_domain_id();
+    else
+        if ( bd->domid != get_initial_domain_id() )
+            printk(XENLOG_WARNING "WARN: unsuported booting not using initial domid\n");
+
     return 0;
 }
 
diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/domain_builder/fdt.h
index ab2b43872e25..06ead05a2583 100644
--- a/xen/arch/x86/domain_builder/fdt.h
+++ b/xen/arch/x86/domain_builder/fdt.h
@@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
     return 0;
 }
 
+static inline 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;
+
+    return fdt_cell_as_u32((fdt32_t *)prop->data, val);
+}
+
+static inline bool __init match_fdt_property(
+    const void *fdt, const struct fdt_property *prop, const char *s)
+{
+    int slen, len = strlen(s);
+    const char *p = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &slen);
+
+    return p && (slen == len) && (memcmp(p, s, len) == 0);
+}
+
 static inline int __init fdt_cmdline_prop_size(const void *fdt, int offset)
 {
     int ret;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index eaac87b02f78..317349b80ac6 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1020,7 +1020,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
         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 )
+        bd->domid = get_initial_domain_id();
     d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
     if ( IS_ERR(d) )
         panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
-- 
2.30.2

Re: [PATCH 11/15] x86/hyperlaunch: add domain id parsing to domain config

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> 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>
> ---
>   xen/arch/x86/domain_builder/fdt.c | 31 ++++++++++++++++++++++++++++++-
>   xen/arch/x86/domain_builder/fdt.h | 18 ++++++++++++++++++
>   xen/arch/x86/setup.c              |  3 ++-
>   3 files changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
> index bc8054a80ec1..3a6b4fbc09a9 100644
> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c

> @@ -120,6 +121,28 @@ static int __init process_domain_node(
>           return -EINVAL;
>       }
>   
> +    fdt_for_each_property_offset(property, fdt, dom_node)
> +    {
> +        const struct fdt_property *prop;
> +
> +        prop = fdt_get_property_by_offset(fdt, property, NULL);
> +        if ( !prop )
> +            continue; /* silently skip */
> +
> +        if ( match_fdt_property(fdt, prop, "domid" ) )
> +        {
> +            uint32_t val = DOMID_INVALID;
> +            if ( fdt_prop_as_u32(prop, &val) != 0 )
> +            {
> +                printk("  failed processing domain id for domain %s\n",
> +                       name == NULL ? "unknown" : name);
> +                return -EINVAL;
> +            }

Bounds check against DOMID_FIRST_RESERVED?

> +            bd->domid = (domid_t)val;
> +            printk("  domid: %d\n", bd->domid);
> +        }
> +    }
> +
>       fdt_for_each_subnode(node, fdt, dom_node)
>       {
>           if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
> @@ -186,6 +209,12 @@ static int __init process_domain_node(
>           return -EFAULT;
>       }
>   
> +    if ( bd->domid == DOMID_INVALID )
> +        bd->domid = get_initial_domain_id();
> +    else
> +        if ( bd->domid != get_initial_domain_id() )

single line "else if"?

> +            printk(XENLOG_WARNING "WARN: unsuported booting not using initial domid\n");

"unsupported"

Maybe "Booting without initial domid not supported"?

> +
>       return 0;
>   }
>   
> diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/domain_builder/fdt.h
> index ab2b43872e25..06ead05a2583 100644
> --- a/xen/arch/x86/domain_builder/fdt.h
> +++ b/xen/arch/x86/domain_builder/fdt.h
> @@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)

> +static inline bool __init match_fdt_property(
> +    const void *fdt, const struct fdt_property *prop, const char *s)
> +{
> +    int slen, len = strlen(s);
> +    const char *p = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &slen);
> +
> +    return p && (slen == len) && (memcmp(p, s, len) == 0);

match_fdt_property() gets called more in later patches.  I wonder if

     const char *p = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), 
&slen);

should move into process_domain_node, and then the string is just 
compared?  Maybe it already gets optimized?

(Is there a way to disassemble .init.text with symbols?)

> +}
> +
>   static inline int __init fdt_cmdline_prop_size(const void *fdt, int offset)
>   {
>       int ret;
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index eaac87b02f78..317349b80ac6 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1020,7 +1020,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>           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 )
> +        bd->domid = get_initial_domain_id();

This seems redundant with the earlier DOMID_INVALID check & setting.  Or 
does this handle the non-hyperlaunch case?  Maybe it should move to 
builder_init() for other non-hyperlaunch configuration?

>       d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
>       if ( IS_ERR(d) )
>           panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));

Re: [PATCH 11/15] x86/hyperlaunch: add domain id parsing to domain config

[Jan Beulich]

On 26.11.2024 00:45, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> @@ -186,6 +209,12 @@ static int __init process_domain_node(
>>           return -EFAULT;
>>       }
>>   
>> +    if ( bd->domid == DOMID_INVALID )
>> +        bd->domid = get_initial_domain_id();
>> +    else
>> +        if ( bd->domid != get_initial_domain_id() )
> 
> single line "else if"?

Yes.

>> +            printk(XENLOG_WARNING "WARN: unsuported booting not using initial domid\n");
> 
> "unsupported"
> 
> Maybe "Booting without initial domid not supported"?

Plus the line then wants splitting after XENLOG_WARNING.

Jan

Re: [PATCH 11/15] x86/hyperlaunch: add domain id parsing to domain config

[Daniel P. Smith]

On 12/2/24 07:00, Jan Beulich wrote:
> On 26.11.2024 00:45, Jason Andryuk wrote:
>> On 2024-11-23 13:20, Daniel P. Smith wrote:
>>> @@ -186,6 +209,12 @@ static int __init process_domain_node(
>>>            return -EFAULT;
>>>        }
>>>    
>>> +    if ( bd->domid == DOMID_INVALID )
>>> +        bd->domid = get_initial_domain_id();
>>> +    else
>>> +        if ( bd->domid != get_initial_domain_id() )
>>
>> single line "else if"?
> 
> Yes.

Agreed.

>>> +            printk(XENLOG_WARNING "WARN: unsuported booting not using initial domid\n");
>>
>> "unsupported"
>>
>> Maybe "Booting without initial domid not supported"?
> 
> Plus the line then wants splitting after XENLOG_WARNING.

Okay.

v/r,
dps

Re: [PATCH 11/15] x86/hyperlaunch: add domain id parsing to domain config

[Daniel P. Smith]

On 11/25/24 18:45, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> 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>
>> ---
>>   xen/arch/x86/domain_builder/fdt.c | 31 ++++++++++++++++++++++++++++++-
>>   xen/arch/x86/domain_builder/fdt.h | 18 ++++++++++++++++++
>>   xen/arch/x86/setup.c              |  3 ++-
>>   3 files changed, 50 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/ 
>> domain_builder/fdt.c
>> index bc8054a80ec1..3a6b4fbc09a9 100644
>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
> 
>> @@ -120,6 +121,28 @@ static int __init process_domain_node(
>>           return -EINVAL;
>>       }
>> +    fdt_for_each_property_offset(property, fdt, dom_node)
>> +    {
>> +        const struct fdt_property *prop;
>> +
>> +        prop = fdt_get_property_by_offset(fdt, property, NULL);
>> +        if ( !prop )
>> +            continue; /* silently skip */
>> +
>> +        if ( match_fdt_property(fdt, prop, "domid" ) )
>> +        {
>> +            uint32_t val = DOMID_INVALID;
>> +            if ( fdt_prop_as_u32(prop, &val) != 0 )
>> +            {
>> +                printk("  failed processing domain id for domain %s\n",
>> +                       name == NULL ? "unknown" : name);
>> +                return -EINVAL;
>> +            }
> 
> Bounds check against DOMID_FIRST_RESERVED?

Yah, that would be good.

>> +            bd->domid = (domid_t)val;
>> +            printk("  domid: %d\n", bd->domid);
>> +        }
>> +    }
>> +
>>       fdt_for_each_subnode(node, fdt, dom_node)
>>       {
>>           if ( fdt_node_check_compatible(fdt, node, 
>> "multiboot,kernel") == 0 )
>> @@ -186,6 +209,12 @@ static int __init process_domain_node(
>>           return -EFAULT;
>>       }
>> +    if ( bd->domid == DOMID_INVALID )
>> +        bd->domid = get_initial_domain_id();
>> +    else
>> +        if ( bd->domid != get_initial_domain_id() )
> 
> single line "else if"?

Yep.

>> +            printk(XENLOG_WARNING "WARN: unsuported booting not using 
>> initial domid\n");
> 
> "unsupported"
> 
> Maybe "Booting without initial domid not supported"?

I am good with that phrasing.

>> +
>>       return 0;
>>   }
>> diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/ 
>> domain_builder/fdt.h
>> index ab2b43872e25..06ead05a2583 100644
>> --- a/xen/arch/x86/domain_builder/fdt.h
>> +++ b/xen/arch/x86/domain_builder/fdt.h
>> @@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const 
>> fdt32_t *cell, uint64_t *val)
> 
>> +static inline bool __init match_fdt_property(
>> +    const void *fdt, const struct fdt_property *prop, const char *s)
>> +{
>> +    int slen, len = strlen(s);
>> +    const char *p = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), 
>> &slen);
>> +
>> +    return p && (slen == len) && (memcmp(p, s, len) == 0);
> 
> match_fdt_property() gets called more in later patches.  I wonder if
> 
>      const char *p = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), 
> &slen);
> 
> should move into process_domain_node, and then the string is just 
> compared?  Maybe it already gets optimized?

Your approach would explicitly force a single fdt_get_string() call, 
logic would remain readable, all while not hoping the optimizer can 
catch it.

> (Is there a way to disassemble .init.text with symbols?)
> 
>> +}
>> +
>>   static inline int __init fdt_cmdline_prop_size(const void *fdt, int 
>> offset)
>>   {
>>       int ret;
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index eaac87b02f78..317349b80ac6 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1020,7 +1020,8 @@ static struct domain *__init create_dom0(struct 
>> boot_info *bi)
>>           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 )
>> +        bd->domid = get_initial_domain_id();
> 
> This seems redundant with the earlier DOMID_INVALID check & setting.  Or 
> does this handle the non-hyperlaunch case?  Maybe it should move to 
> builder_init() for other non-hyperlaunch configuration?

Even if the call to get_initial_domain_id() is moved to builder_init(), 
I would not feel comfortable with assuming bd->domid is valid when 
getting here. So I would still have the check, but with panic instead. 
I'm open to making the change if the x86 maintainers are comfortable 
with making the assumption.

v/r,
dps

Re: [PATCH 11/15] x86/hyperlaunch: add domain id parsing to domain config

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> --- a/xen/arch/x86/domain_builder/fdt.h
> +++ b/xen/arch/x86/domain_builder/fdt.h
> @@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
>      return 0;
>  }
>  
> +static inline 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;
> +
> +    return fdt_cell_as_u32((fdt32_t *)prop->data, val);
> +}
> +
> +static inline bool __init match_fdt_property(
> +    const void *fdt, const struct fdt_property *prop, const char *s)
> +{
> +    int slen, len = strlen(s);

Plain int isn't quite appropriate for strlen()'s return. It doesn't strictly
need to be size_t, but it should be at least unsigned int.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1020,7 +1020,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>          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 )
> +        bd->domid = get_initial_domain_id();

Looks like the comment then wants to move, too.

Jan

Re: [PATCH 11/15] x86/hyperlaunch: add domain id parsing to domain config

[Daniel P. Smith]

On 12/2/24 07:02, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> --- a/xen/arch/x86/domain_builder/fdt.h
>> +++ b/xen/arch/x86/domain_builder/fdt.h
>> @@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
>>       return 0;
>>   }
>>   
>> +static inline 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;
>> +
>> +    return fdt_cell_as_u32((fdt32_t *)prop->data, val);
>> +}
>> +
>> +static inline bool __init match_fdt_property(
>> +    const void *fdt, const struct fdt_property *prop, const char *s)
>> +{
>> +    int slen, len = strlen(s);
> 
> Plain int isn't quite appropriate for strlen()'s return. It doesn't strictly
> need to be size_t, but it should be at least unsigned int.

Ack.

>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1020,7 +1020,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>>           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 )
>> +        bd->domid = get_initial_domain_id();
> 
> Looks like the comment then wants to move, too.

Okay.

v/r,
dps

Re: [PATCH 11/15] x86/hyperlaunch: add domain id parsing to domain config

[Daniel P. Smith]

On 12/11/24 11:21, Daniel P. Smith wrote:
> On 12/2/24 07:02, Jan Beulich wrote:
>> On 23.11.2024 19:20, Daniel P. Smith wrote:
>>> --- a/xen/arch/x86/domain_builder/fdt.h
>>> +++ b/xen/arch/x86/domain_builder/fdt.h
>>> @@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const 
>>> fdt32_t *cell, uint64_t *val)
>>>       return 0;
>>>   }
>>> +static inline 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;
>>> +
>>> +    return fdt_cell_as_u32((fdt32_t *)prop->data, val);
>>> +}
>>> +
>>> +static inline bool __init match_fdt_property(
>>> +    const void *fdt, const struct fdt_property *prop, const char *s)
>>> +{
>>> +    int slen, len = strlen(s);
>>
>> Plain int isn't quite appropriate for strlen()'s return. It doesn't 
>> strictly
>> need to be size_t, but it should be at least unsigned int.
> 
> Ack.

I wanted to reply back on this one before posting the next version. By 
implementing Jason's recommendation to unroll this function, the 
strlen() call goes away and slen becomes name_len. But name_len remains 
plain int to align with the fdt_get_string() parameter type.

v/r,
dps

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

[Daniel P. Smith]

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>
---
 xen/arch/x86/domain_builder/fdt.c     | 19 +++++++++++++++++++
 xen/arch/x86/include/asm/bootdomain.h |  6 ++++++
 xen/arch/x86/setup.c                  |  3 ++-
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
index 3a6b4fbc09a9..09e72d96a752 100644
--- a/xen/arch/x86/domain_builder/fdt.c
+++ b/xen/arch/x86/domain_builder/fdt.c
@@ -141,6 +141,25 @@ static int __init process_domain_node(
             bd->domid = (domid_t)val;
             printk("  domid: %d\n", bd->domid);
         }
+        if ( match_fdt_property(fdt, prop, "mode" ) )
+        {
+            if ( fdt_prop_as_u32(prop, &bd->mode) != 0 )
+            {
+                printk("  failed processing mode for domain %s\n",
+                       name == NULL ? "unknown" : name);
+                return -EINVAL;
+            }
+
+            printk("  mode: ");
+            if ( !(bd->mode & BUILD_MODE_PARAVIRT) ) {
+                if ( bd->mode & BUILD_MODE_ENABLE_DM )
+                    printk("HVM\n");
+                else
+                    printk("PVH\n");
+            }
+            else
+                printk("PV\n");
+        }
     }
 
     fdt_for_each_subnode(node, fdt, dom_node)
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index ffda1509a63f..50c33d183e07 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -18,6 +18,12 @@ 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     */
+#define BUILD_MODE_LONG          (1 << 2) /* 64 BIT | 32 BIT  */
+    uint32_t mode;
+
     struct boot_module *kernel;
     struct boot_module *ramdisk;
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 317349b80ac6..dae25721994d 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1006,7 +1006,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) ?
-- 
2.30.2

Re: [PATCH 12/15] x86/hyperlaunch: specify dom0 mode with device tree

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> 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>

32/64 is only applicable for PV.  It might be worth mentioning that.

Regards,
Jason

Re: [PATCH 12/15] x86/hyperlaunch: specify dom0 mode with device tree

[Daniel P. Smith]

On 11/25/24 18:52, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> 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>

Thanks!

> 32/64 is only applicable for PV.  It might be worth mentioning that.

Ack.

v/r,
dps

Re: [PATCH 12/15] x86/hyperlaunch: specify dom0 mode with device tree

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -141,6 +141,25 @@ static int __init process_domain_node(
>              bd->domid = (domid_t)val;
>              printk("  domid: %d\n", bd->domid);
>          }
> +        if ( match_fdt_property(fdt, prop, "mode" ) )
> +        {
> +            if ( fdt_prop_as_u32(prop, &bd->mode) != 0 )
> +            {
> +                printk("  failed processing mode for domain %s\n",
> +                       name == NULL ? "unknown" : name);
> +                return -EINVAL;
> +            }
> +
> +            printk("  mode: ");
> +            if ( !(bd->mode & BUILD_MODE_PARAVIRT) ) {

Nit: Brace placement.

> +                if ( bd->mode & BUILD_MODE_ENABLE_DM )
> +                    printk("HVM\n");
> +                else
> +                    printk("PVH\n");
> +            }
> +            else
> +                printk("PV\n");
> +        }
>      }
>  
>      fdt_for_each_subnode(node, fdt, dom_node)
> --- a/xen/arch/x86/include/asm/bootdomain.h
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -18,6 +18,12 @@ 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     */
> +#define BUILD_MODE_LONG          (1 << 2) /* 64 BIT | 32 BIT  */

This last one isn't used anywhere, is it?

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1006,7 +1006,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) ?

What about BUILD_MODE_ENABLE_DM?

Jan

Re: [PATCH 12/15] x86/hyperlaunch: specify dom0 mode with device tree

[Daniel P. Smith]

On 12/2/24 07:05, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
>> @@ -141,6 +141,25 @@ static int __init process_domain_node(
>>               bd->domid = (domid_t)val;
>>               printk("  domid: %d\n", bd->domid);
>>           }
>> +        if ( match_fdt_property(fdt, prop, "mode" ) )
>> +        {
>> +            if ( fdt_prop_as_u32(prop, &bd->mode) != 0 )
>> +            {
>> +                printk("  failed processing mode for domain %s\n",
>> +                       name == NULL ? "unknown" : name);
>> +                return -EINVAL;
>> +            }
>> +
>> +            printk("  mode: ");
>> +            if ( !(bd->mode & BUILD_MODE_PARAVIRT) ) {
> 
> Nit: Brace placement.

Ack.

>> +                if ( bd->mode & BUILD_MODE_ENABLE_DM )
>> +                    printk("HVM\n");
>> +                else
>> +                    printk("PVH\n");
>> +            }
>> +            else
>> +                printk("PV\n");
>> +        }
>>       }
>>   
>>       fdt_for_each_subnode(node, fdt, dom_node)
>> --- a/xen/arch/x86/include/asm/bootdomain.h
>> +++ b/xen/arch/x86/include/asm/bootdomain.h
>> @@ -18,6 +18,12 @@ 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     */
>> +#define BUILD_MODE_LONG          (1 << 2) /* 64 BIT | 32 BIT  */
> 
> This last one isn't used anywhere, is it?

Hmm, I may have lost this when AMD asked for PVH to be done this cycle. 
It should still be used to allow for 32bit PV dom0, will get this added in.

>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1006,7 +1006,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) ?
> 
> What about BUILD_MODE_ENABLE_DM?

Good point, a goal for HL was to enable building and booting with 
separate hwdom and ctldom.

v/r,
dps

Re: [PATCH 12/15] x86/hyperlaunch: specify dom0 mode with device tree

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -141,6 +141,25 @@ static int __init process_domain_node(
>              bd->domid = (domid_t)val;
>              printk("  domid: %d\n", bd->domid);
>          }
> +        if ( match_fdt_property(fdt, prop, "mode" ) )
> +        {
> +            if ( fdt_prop_as_u32(prop, &bd->mode) != 0 )
> +            {
> +                printk("  failed processing mode for domain %s\n",
> +                       name == NULL ? "unknown" : name);
> +                return -EINVAL;
> +            }
> +
> +            printk("  mode: ");
> +            if ( !(bd->mode & BUILD_MODE_PARAVIRT) ) {
> +                if ( bd->mode & BUILD_MODE_ENABLE_DM )
> +                    printk("HVM\n");
> +                else
> +                    printk("PVH\n");
> +            }
> +            else
> +                printk("PV\n");

Oh, and: What about BUILD_MODE_ENABLE_DM also being set here?

Jan

Re: [PATCH 12/15] x86/hyperlaunch: specify dom0 mode with device tree

[Daniel P. Smith]

On 12/2/24 07:06, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
>> @@ -141,6 +141,25 @@ static int __init process_domain_node(
>>               bd->domid = (domid_t)val;
>>               printk("  domid: %d\n", bd->domid);
>>           }
>> +        if ( match_fdt_property(fdt, prop, "mode" ) )
>> +        {
>> +            if ( fdt_prop_as_u32(prop, &bd->mode) != 0 )
>> +            {
>> +                printk("  failed processing mode for domain %s\n",
>> +                       name == NULL ? "unknown" : name);
>> +                return -EINVAL;
>> +            }
>> +
>> +            printk("  mode: ");
>> +            if ( !(bd->mode & BUILD_MODE_PARAVIRT) ) {
>> +                if ( bd->mode & BUILD_MODE_ENABLE_DM )
>> +                    printk("HVM\n");
>> +                else
>> +                    printk("PVH\n");
>> +            }
>> +            else
>> +                printk("PV\n");
> 
> Oh, and: What about BUILD_MODE_ENABLE_DM also being set here?

Are you asking in the sense that the PV domain is being flag as a device 
model domain? Maybe I am missing something, but I am not aware of 
anything specific that must be set for a PV domain to operate as device 
model domain. If flask is in play, then there is a secure label 
requirement but that is separate of a mode that the domain must be 
running in. Please enlighten me if I am over looking something.

v/r,
dps

Re: [PATCH 12/15] x86/hyperlaunch: specify dom0 mode with device tree

[Jan Beulich]

On 11.12.2024 18:48, Daniel P. Smith wrote:
> On 12/2/24 07:06, Jan Beulich wrote:
>> On 23.11.2024 19:20, Daniel P. Smith wrote:
>>> --- a/xen/arch/x86/domain_builder/fdt.c
>>> +++ b/xen/arch/x86/domain_builder/fdt.c
>>> @@ -141,6 +141,25 @@ static int __init process_domain_node(
>>>               bd->domid = (domid_t)val;
>>>               printk("  domid: %d\n", bd->domid);
>>>           }
>>> +        if ( match_fdt_property(fdt, prop, "mode" ) )
>>> +        {
>>> +            if ( fdt_prop_as_u32(prop, &bd->mode) != 0 )
>>> +            {
>>> +                printk("  failed processing mode for domain %s\n",
>>> +                       name == NULL ? "unknown" : name);
>>> +                return -EINVAL;
>>> +            }
>>> +
>>> +            printk("  mode: ");
>>> +            if ( !(bd->mode & BUILD_MODE_PARAVIRT) ) {
>>> +                if ( bd->mode & BUILD_MODE_ENABLE_DM )
>>> +                    printk("HVM\n");
>>> +                else
>>> +                    printk("PVH\n");
>>> +            }
>>> +            else
>>> +                printk("PV\n");
>>
>> Oh, and: What about BUILD_MODE_ENABLE_DM also being set here?
> 
> Are you asking in the sense that the PV domain is being flag as a device 
> model domain? Maybe I am missing something, but I am not aware of 
> anything specific that must be set for a PV domain to operate as device 
> model domain. If flask is in play, then there is a secure label 
> requirement but that is separate of a mode that the domain must be 
> running in. Please enlighten me if I am over looking something.

Rephrasing my question: Is it legitimate for BUILD_MODE_PARAVIRT to be
accompanied with BUILD_MODE_ENABLE_DM. If it is, what is the difference
between BUILD_MODE_PARAVIRT|BUILD_MODE_ENABLE_DM and plain
BUILD_MODE_PARAVIRT? If there's none, perhaps better to reject the flag
(retaining possible use for some future purpose)?

Jan

[PATCH 13/15] x86/hyperlaunch: add memory parsing to domain config

[Daniel P. Smith]

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>
---
 xen/arch/x86/dom0_build.c             |  8 ++++++
 xen/arch/x86/domain_builder/fdt.c     | 37 +++++++++++++++++++++++++++
 xen/arch/x86/domain_builder/fdt.h     |  9 +++++++
 xen/arch/x86/include/asm/bootdomain.h |  4 +++
 4 files changed, 58 insertions(+)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index c231191faec7..1c3b7ff0e658 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -609,6 +609,14 @@ int __init construct_dom0(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, LONG_MAX) && bd->mem_pages )
+        dom0_size.nr_pages = bd->mem_pages;
+    if ( !get_memsize(&dom0_min_size, LONG_MAX) && bd->min_pages )
+        dom0_size.nr_pages = bd->min_pages;
+    if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages )
+        dom0_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/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
index 09e72d96a752..b8ace5c18c6a 100644
--- a/xen/arch/x86/domain_builder/fdt.c
+++ b/xen/arch/x86/domain_builder/fdt.c
@@ -7,6 +7,7 @@
 #include <xen/lib.h>
 #include <xen/libfdt/libfdt.h>
 #include <xen/rangeset.h> /* required for asm/setup.h */
+#include <xen/sizes.h>
 
 #include <asm/bootinfo.h>
 #include <asm/guest.h>
@@ -160,6 +161,42 @@ static int __init process_domain_node(
             else
                 printk("PV\n");
         }
+        if ( match_fdt_property(fdt, prop, "memory" ) )
+        {
+            uint64_t kb;
+            if ( fdt_prop_as_u64(prop, &kb) != 0 )
+            {
+                printk("  failed processing memory for domain %s\n",
+                       name == NULL ? "unknown" : name);
+                return -EINVAL;
+            }
+            bd->mem_pages = PFN_DOWN(kb * SZ_1K);
+            printk("  memory: %ld\n", bd->mem_pages << PAGE_SHIFT);
+        }
+        if ( match_fdt_property(fdt, prop, "mem-min" ) )
+        {
+            uint64_t kb;
+            if ( fdt_prop_as_u64(prop, &kb) != 0 )
+            {
+                printk("  failed processing memory for domain %s\n",
+                       name == NULL ? "unknown" : name);
+                return -EINVAL;
+            }
+            bd->min_pages = PFN_DOWN(kb * SZ_1K);
+            printk("  min memory: %ld\n", bd->min_pages << PAGE_SHIFT);
+        }
+        if ( match_fdt_property(fdt, prop, "mem-max" ) )
+        {
+            uint64_t kb;
+            if ( fdt_prop_as_u64(prop, &kb) != 0 )
+            {
+                printk("  failed processing memory for domain %s\n",
+                       name == NULL ? "unknown" : name);
+                return -EINVAL;
+            }
+            bd->max_pages = PFN_DOWN(kb * SZ_1K);
+            printk("  max memory: %ld\n", bd->max_pages << PAGE_SHIFT);
+        }
     }
 
     fdt_for_each_subnode(node, fdt, dom_node)
diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/domain_builder/fdt.h
index 06ead05a2583..73d02019b3c7 100644
--- a/xen/arch/x86/domain_builder/fdt.h
+++ b/xen/arch/x86/domain_builder/fdt.h
@@ -36,6 +36,15 @@ static inline int __init fdt_prop_as_u32(
     return fdt_cell_as_u32((fdt32_t *)prop->data, val);
 }
 
+static inline int __init fdt_prop_as_u64(
+    const struct fdt_property *prop, uint64_t *val)
+{
+    if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u64) )
+        return -EINVAL;
+
+    return fdt_cell_as_u64((fdt32_t *)prop->data, val);
+}
+
 static inline bool __init match_fdt_property(
     const void *fdt, const struct fdt_property *prop, const char *s)
 {
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 50c33d183e07..9a5ba2931665 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -24,6 +24,10 @@ struct boot_domain {
 #define BUILD_MODE_LONG          (1 << 2) /* 64 BIT | 32 BIT  */
     uint32_t mode;
 
+    unsigned long mem_pages;
+    unsigned long min_pages;
+    unsigned long max_pages;
+
     struct boot_module *kernel;
     struct boot_module *ramdisk;
 
-- 
2.30.2

Re: [PATCH 13/15] x86/hyperlaunch: add memory parsing to domain config

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> 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>
> ---

> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index c231191faec7..1c3b7ff0e658 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -609,6 +609,14 @@ int __init construct_dom0(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, LONG_MAX) && bd->mem_pages )
> +        dom0_size.nr_pages = bd->mem_pages;
> +    if ( !get_memsize(&dom0_min_size, LONG_MAX) && bd->min_pages )
> +        dom0_size.nr_pages = bd->min_pages;
> +    if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages )
> +        dom0_size.nr_pages = bd->max_pages;
> +

This placement seems a little random.  Can this move into 
dom0_compute_nr_pages()?

>       if ( is_hvm_domain(d) )
>           rc = dom0_construct_pvh(bd);
>       else if ( is_pv_domain(d) )

Re: [PATCH 13/15] x86/hyperlaunch: add memory parsing to domain config

[Daniel P. Smith]

On 11/25/24 19:03, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> 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>
>> ---
> 
>> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
>> index c231191faec7..1c3b7ff0e658 100644
>> --- a/xen/arch/x86/dom0_build.c
>> +++ b/xen/arch/x86/dom0_build.c
>> @@ -609,6 +609,14 @@ int __init construct_dom0(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, LONG_MAX) && bd->mem_pages )
>> +        dom0_size.nr_pages = bd->mem_pages;
>> +    if ( !get_memsize(&dom0_min_size, LONG_MAX) && bd->min_pages )
>> +        dom0_size.nr_pages = bd->min_pages;
>> +    if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages )
>> +        dom0_size.nr_pages = bd->max_pages;
>> +
> 
> This placement seems a little random.  Can this move into 
> dom0_compute_nr_pages()?

As I started to rebase the multi-domain code around all the changes that 
happened under the boot module review, dom0_compute_nr_pages() became a 
mess to work with again. The result does see this drop in favor of 
handling during dom_compute_nr_pages(). I will look to back port that 
refactoring to here.

v/r,
dps

Re: [PATCH 13/15] x86/hyperlaunch: add memory parsing to domain config

[Daniel P. Smith]

On 12/11/24 12:59, Daniel P. Smith wrote:
> On 11/25/24 19:03, Jason Andryuk wrote:
>> On 2024-11-23 13:20, Daniel P. Smith wrote:
>>> 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>
>>> ---
>>
>>> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
>>> index c231191faec7..1c3b7ff0e658 100644
>>> --- a/xen/arch/x86/dom0_build.c
>>> +++ b/xen/arch/x86/dom0_build.c
>>> @@ -609,6 +609,14 @@ int __init construct_dom0(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, LONG_MAX) && bd->mem_pages )
>>> +        dom0_size.nr_pages = bd->mem_pages;
>>> +    if ( !get_memsize(&dom0_min_size, LONG_MAX) && bd->min_pages )
>>> +        dom0_size.nr_pages = bd->min_pages;
>>> +    if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages )
>>> +        dom0_size.nr_pages = bd->max_pages;
>>> +
>>
>> This placement seems a little random.  Can this move into 
>> dom0_compute_nr_pages()?
> 
> As I started to rebase the multi-domain code around all the changes that 
> happened under the boot module review, dom0_compute_nr_pages() became a 
> mess to work with again. The result does see this drop in favor of 
> handling during dom_compute_nr_pages(). I will look to back port that 
> refactoring to here.

Before sending out v2, wanted to respond back that I did not see an 
immediate way to move this up to dom0_compute_nr_pages() without bring 
in a series of unrelated changes.

v/r,
dps

Re: [PATCH 13/15] x86/hyperlaunch: add memory parsing to domain config

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> @@ -160,6 +161,42 @@ static int __init process_domain_node(
>              else
>                  printk("PV\n");
>          }
> +        if ( match_fdt_property(fdt, prop, "memory" ) )
> +        {
> +            uint64_t kb;
> +            if ( fdt_prop_as_u64(prop, &kb) != 0 )
> +            {
> +                printk("  failed processing memory for domain %s\n",
> +                       name == NULL ? "unknown" : name);
> +                return -EINVAL;
> +            }
> +            bd->mem_pages = PFN_DOWN(kb * SZ_1K);
> +            printk("  memory: %ld\n", bd->mem_pages << PAGE_SHIFT);
> +        }
> +        if ( match_fdt_property(fdt, prop, "mem-min" ) )
> +        {
> +            uint64_t kb;
> +            if ( fdt_prop_as_u64(prop, &kb) != 0 )
> +            {
> +                printk("  failed processing memory for domain %s\n",
> +                       name == NULL ? "unknown" : name);
> +                return -EINVAL;
> +            }
> +            bd->min_pages = PFN_DOWN(kb * SZ_1K);
> +            printk("  min memory: %ld\n", bd->min_pages << PAGE_SHIFT);
> +        }
> +        if ( match_fdt_property(fdt, prop, "mem-max" ) )
> +        {
> +            uint64_t kb;
> +            if ( fdt_prop_as_u64(prop, &kb) != 0 )
> +            {
> +                printk("  failed processing memory for domain %s\n",
> +                       name == NULL ? "unknown" : name);
> +                return -EINVAL;
> +            }
> +            bd->max_pages = PFN_DOWN(kb * SZ_1K);
> +            printk("  max memory: %ld\n", bd->max_pages << PAGE_SHIFT);
> +        }

Since the values logged are all multiples of 1k, why make reading the logs
more complicated by logging byte-granular values? I instead wonder whether
converting to more coarse grained values (leaving, say, between 4 and 6
significant digits while using kb, Mb, Gb, etc) wouldn't be yet better.

Jan

Re: [PATCH 13/15] x86/hyperlaunch: add memory parsing to domain config

[Daniel P. Smith]

On 12/2/24 07:14, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> @@ -160,6 +161,42 @@ static int __init process_domain_node(
>>               else
>>                   printk("PV\n");
>>           }
>> +        if ( match_fdt_property(fdt, prop, "memory" ) )
>> +        {
>> +            uint64_t kb;
>> +            if ( fdt_prop_as_u64(prop, &kb) != 0 )
>> +            {
>> +                printk("  failed processing memory for domain %s\n",
>> +                       name == NULL ? "unknown" : name);
>> +                return -EINVAL;
>> +            }
>> +            bd->mem_pages = PFN_DOWN(kb * SZ_1K);
>> +            printk("  memory: %ld\n", bd->mem_pages << PAGE_SHIFT);
>> +        }
>> +        if ( match_fdt_property(fdt, prop, "mem-min" ) )
>> +        {
>> +            uint64_t kb;
>> +            if ( fdt_prop_as_u64(prop, &kb) != 0 )
>> +            {
>> +                printk("  failed processing memory for domain %s\n",
>> +                       name == NULL ? "unknown" : name);
>> +                return -EINVAL;
>> +            }
>> +            bd->min_pages = PFN_DOWN(kb * SZ_1K);
>> +            printk("  min memory: %ld\n", bd->min_pages << PAGE_SHIFT);
>> +        }
>> +        if ( match_fdt_property(fdt, prop, "mem-max" ) )
>> +        {
>> +            uint64_t kb;
>> +            if ( fdt_prop_as_u64(prop, &kb) != 0 )
>> +            {
>> +                printk("  failed processing memory for domain %s\n",
>> +                       name == NULL ? "unknown" : name);
>> +                return -EINVAL;
>> +            }
>> +            bd->max_pages = PFN_DOWN(kb * SZ_1K);
>> +            printk("  max memory: %ld\n", bd->max_pages << PAGE_SHIFT);
>> +        }
> 
> Since the values logged are all multiples of 1k, why make reading the logs
> more complicated by logging byte-granular values? I instead wonder whether
> converting to more coarse grained values (leaving, say, between 4 and 6
> significant digits while using kb, Mb, Gb, etc) wouldn't be yet better.

Sure we can make it report in a friendlier format. To support dynamic 
sizing, is there already an existing formatter, I would hate to 
re-invent the wheel on this, or I could just statically report in kb.

v/r,
dps

Re: [PATCH 13/15] x86/hyperlaunch: add memory parsing to domain config

[Jan Beulich]

On 11.12.2024 19:02, Daniel P. Smith wrote:
> On 12/2/24 07:14, Jan Beulich wrote:
>> On 23.11.2024 19:20, Daniel P. Smith wrote:
>>> @@ -160,6 +161,42 @@ static int __init process_domain_node(
>>>               else
>>>                   printk("PV\n");
>>>           }
>>> +        if ( match_fdt_property(fdt, prop, "memory" ) )
>>> +        {
>>> +            uint64_t kb;
>>> +            if ( fdt_prop_as_u64(prop, &kb) != 0 )
>>> +            {
>>> +                printk("  failed processing memory for domain %s\n",
>>> +                       name == NULL ? "unknown" : name);
>>> +                return -EINVAL;
>>> +            }
>>> +            bd->mem_pages = PFN_DOWN(kb * SZ_1K);
>>> +            printk("  memory: %ld\n", bd->mem_pages << PAGE_SHIFT);
>>> +        }
>>> +        if ( match_fdt_property(fdt, prop, "mem-min" ) )
>>> +        {
>>> +            uint64_t kb;
>>> +            if ( fdt_prop_as_u64(prop, &kb) != 0 )
>>> +            {
>>> +                printk("  failed processing memory for domain %s\n",
>>> +                       name == NULL ? "unknown" : name);
>>> +                return -EINVAL;
>>> +            }
>>> +            bd->min_pages = PFN_DOWN(kb * SZ_1K);
>>> +            printk("  min memory: %ld\n", bd->min_pages << PAGE_SHIFT);
>>> +        }
>>> +        if ( match_fdt_property(fdt, prop, "mem-max" ) )
>>> +        {
>>> +            uint64_t kb;
>>> +            if ( fdt_prop_as_u64(prop, &kb) != 0 )
>>> +            {
>>> +                printk("  failed processing memory for domain %s\n",
>>> +                       name == NULL ? "unknown" : name);
>>> +                return -EINVAL;
>>> +            }
>>> +            bd->max_pages = PFN_DOWN(kb * SZ_1K);
>>> +            printk("  max memory: %ld\n", bd->max_pages << PAGE_SHIFT);
>>> +        }
>>
>> Since the values logged are all multiples of 1k, why make reading the logs
>> more complicated by logging byte-granular values? I instead wonder whether
>> converting to more coarse grained values (leaving, say, between 4 and 6
>> significant digits while using kb, Mb, Gb, etc) wouldn't be yet better.
> 
> Sure we can make it report in a friendlier format. To support dynamic 
> sizing, is there already an existing formatter, I would hate to 
> re-invent the wheel on this, or I could just statically report in kb.

I don't recall use having any formatter for this, so for now I'd just report
kb values.

Jan

[PATCH 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree

[Daniel P. Smith]

Introduce the `cpus` property, named as such for dom0less compatibility, that
represents the maximum number of vpcus 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>
---
 xen/arch/x86/dom0_build.c             |  3 +++
 xen/arch/x86/domain_builder/fdt.c     | 12 ++++++++++++
 xen/arch/x86/include/asm/bootdomain.h |  2 ++
 3 files changed, 17 insertions(+)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 1c3b7ff0e658..7ff052016bfd 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -617,6 +617,9 @@ int __init construct_dom0(struct boot_domain *bd)
     if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages )
         dom0_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/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
index b8ace5c18c6a..d24e265f2378 100644
--- a/xen/arch/x86/domain_builder/fdt.c
+++ b/xen/arch/x86/domain_builder/fdt.c
@@ -197,6 +197,18 @@ static int __init process_domain_node(
             bd->max_pages = PFN_DOWN(kb * SZ_1K);
             printk("  max memory: %ld\n", bd->max_pages << PAGE_SHIFT);
         }
+        if ( match_fdt_property(fdt, prop, "cpus" ) )
+        {
+            uint32_t val = UINT_MAX;
+            if ( fdt_prop_as_u32(prop, &val) != 0 )
+            {
+                printk("  failed processing max_vcpus for domain %s\n",
+                       name == NULL ? "unknown" : name);
+                return -EINVAL;
+            }
+            bd->max_vcpus = val;
+            printk("  max vcpus: %d\n", bd->max_vcpus);
+        }
     }
 
     fdt_for_each_subnode(node, fdt, dom_node)
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 9a5ba2931665..d144d6173400 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -28,6 +28,8 @@ struct boot_domain {
     unsigned long min_pages;
     unsigned long max_pages;
 
+    unsigned int max_vcpus;
+
     struct boot_module *kernel;
     struct boot_module *ramdisk;
 
-- 
2.30.2

Re: [PATCH 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> Introduce the `cpus` property, named as such for dom0less compatibility, that
> represents the maximum number of vpcus 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>
> ---

> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
> index b8ace5c18c6a..d24e265f2378 100644
> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -197,6 +197,18 @@ static int __init process_domain_node(
>               bd->max_pages = PFN_DOWN(kb * SZ_1K);
>               printk("  max memory: %ld\n", bd->max_pages << PAGE_SHIFT);
>           }
> +        if ( match_fdt_property(fdt, prop, "cpus" ) )

I think I forgot to mention it on earlier ones, but I think all these 
match_fdt_property() should be chained together with "else if".

With that

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

> +        {
> +            uint32_t val = UINT_MAX;
> +            if ( fdt_prop_as_u32(prop, &val) != 0 )
> +            {
> +                printk("  failed processing max_vcpus for domain %s\n",
> +                       name == NULL ? "unknown" : name);
> +                return -EINVAL;
> +            }
> +            bd->max_vcpus = val;
> +            printk("  max vcpus: %d\n", bd->max_vcpus);
> +        }
>       }
>   
>       fdt_for_each_subnode(node, fdt, dom_node)

Re: [PATCH 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree

[Daniel P. Smith]

On 11/25/24 19:05, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> Introduce the `cpus` property, named as such for dom0less 
>> compatibility, that
>> represents the maximum number of vpcus 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>
>> ---
> 
>> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/ 
>> domain_builder/fdt.c
>> index b8ace5c18c6a..d24e265f2378 100644
>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
>> @@ -197,6 +197,18 @@ static int __init process_domain_node(
>>               bd->max_pages = PFN_DOWN(kb * SZ_1K);
>>               printk("  max memory: %ld\n", bd->max_pages << PAGE_SHIFT);
>>           }
>> +        if ( match_fdt_property(fdt, prop, "cpus" ) )
> 
> I think I forgot to mention it on earlier ones, but I think all these 
> match_fdt_property() should be chained together with "else if".

Ack.

> With that
> 
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Thanks!

v/r,
dps

Re: [PATCH 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -617,6 +617,9 @@ int __init construct_dom0(struct boot_domain *bd)
>      if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages )
>          dom0_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;

Isn't this kind of backwards? I.e. aren't you meaning to move us towards
boot-domains?

Also, what about the counterpart opt_dom0_max_vcpus_min? That wants to be
controllable from DT too, I would think?

Jan

Re: [PATCH 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree

[Daniel P. Smith]

On 12/2/24 07:19, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> --- a/xen/arch/x86/dom0_build.c
>> +++ b/xen/arch/x86/dom0_build.c
>> @@ -617,6 +617,9 @@ int __init construct_dom0(struct boot_domain *bd)
>>       if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages )
>>           dom0_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;
> 
> Isn't this kind of backwards? I.e. aren't you meaning to move us towards
> boot-domains?

Prior to domain builder, available construction parameters for dom0 were 
exposed as command line parameters. This allowed for boot-time 
adjustments to the parameters. With domain builder, there are now two 
sources for dom0 construction parameters. Those coming from the device 
tree and those coming from the command line. For most x86 platforms, the 
device tree parameters can only be constructed prior to booting Xen. 
Whereas the command line parameters allow boot-time adjustments, at 
least for dom0. That is the thinking at least. Now if there is interest 
in being able to retire the command line options, that would definitely 
simplify things.

> Also, what about the counterpart opt_dom0_max_vcpus_min? That wants to be
> controllable from DT too, I would think?

Yes, in theory we will eventually be able to do requested/min/max as 
well as cpu pinning/affinity. For now it was requested we focus on 
implementing only requested vcpus.

v/r,
dps

Re: [PATCH 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree

[Jan Beulich]

On 11.12.2024 20:49, Daniel P. Smith wrote:
> On 12/2/24 07:19, Jan Beulich wrote:
>> On 23.11.2024 19:20, Daniel P. Smith wrote:
>>> --- a/xen/arch/x86/dom0_build.c
>>> +++ b/xen/arch/x86/dom0_build.c
>>> @@ -617,6 +617,9 @@ int __init construct_dom0(struct boot_domain *bd)
>>>       if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages )
>>>           dom0_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;
>>
>> Isn't this kind of backwards? I.e. aren't you meaning to move us towards
>> boot-domains?
> 
> Prior to domain builder, available construction parameters for dom0 were 
> exposed as command line parameters. This allowed for boot-time 
> adjustments to the parameters. With domain builder, there are now two 
> sources for dom0 construction parameters. Those coming from the device 
> tree and those coming from the command line. For most x86 platforms, the 
> device tree parameters can only be constructed prior to booting Xen. 
> Whereas the command line parameters allow boot-time adjustments, at 
> least for dom0. That is the thinking at least. Now if there is interest 
> in being able to retire the command line options, that would definitely 
> simplify things.

No, retiring command line options is out of question imo. Yet that also
wasn't my point. Instead I was wondering why we wouldn't make bd->* the
ultimate source of truth. However, ...

>> Also, what about the counterpart opt_dom0_max_vcpus_min? That wants to be
>> controllable from DT too, I would think?
> 
> Yes, in theory we will eventually be able to do requested/min/max as 
> well as cpu pinning/affinity. For now it was requested we focus on 
> implementing only requested vcpus.

... that's pretty much only a reasonable option if these were converted
at the same time, to avoid becoming inconsistent for perhaps an extended
period of time.

Jan

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

[Daniel P. Smith]

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

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/domain_builder/core.c    |  2 +-
 xen/arch/x86/domain_builder/fdt.c     | 13 +++++++++++++
 xen/arch/x86/include/asm/bootdomain.h |  4 ++++
 xen/arch/x86/setup.c                  |  6 +++++-
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/domain_builder/core.c
index 95cab06e6159..eaa019472724 100644
--- a/xen/arch/x86/domain_builder/core.c
+++ b/xen/arch/x86/domain_builder/core.c
@@ -93,9 +93,9 @@ void __init builder_init(struct boot_info *bi)
         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 |= BUILD_CAPS_CONTROL;
         bi->nr_domains = 1;
     }
-
 }
 
 /*
diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
index d24e265f2378..9499e337938c 100644
--- a/xen/arch/x86/domain_builder/fdt.c
+++ b/xen/arch/x86/domain_builder/fdt.c
@@ -209,6 +209,19 @@ static int __init process_domain_node(
             bd->max_vcpus = val;
             printk("  max vcpus: %d\n", bd->max_vcpus);
         }
+        if ( match_fdt_property(fdt, prop, "capabilities" ) )
+        {
+            if ( fdt_prop_as_u32(prop, &bd->capabilities) != 0 )
+            {
+                printk("  failed processing domain id for domain %s\n",
+                       name == NULL ? "unknown" : name);
+                return -EINVAL;
+            }
+            printk("  caps: ");
+            if ( bd->capabilities & BUILD_CAPS_CONTROL )
+                printk("c");
+            printk("\n");
+        }
     }
 
     fdt_for_each_subnode(node, fdt, dom_node)
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index d144d6173400..51ebf1f68189 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -18,6 +18,10 @@ struct boot_domain {
 
     domid_t domid;
 
+#define BUILD_CAPS_NONE          (0)
+#define BUILD_CAPS_CONTROL       (1 << 0)
+    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 dae25721994d..28e750a420e8 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -992,6 +992,7 @@ static size_t __init domain_cmdline_size(
 static struct domain *__init create_dom0(struct boot_info *bi)
 {
     char *cmdline = NULL;
+    int create_flags = 0;
     struct xen_domctl_createdomain dom0_cfg = {
         .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
         .max_evtchn_port = -1,
@@ -1023,7 +1024,10 @@ static struct domain *__init create_dom0(struct boot_info *bi)
     /* Create initial domain.  Not d0 for pvshim. */
     if ( bd->domid == DOMID_INVALID )
         bd->domid = get_initial_domain_id();
-    d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
+    if ( bd->capabilities & BUILD_CAPS_CONTROL )
+            create_flags |= CDF_privileged;
+    d = domain_create(bd->domid, &dom0_cfg,
+                      pv_shim ? 0 : create_flags);
     if ( IS_ERR(d) )
         panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
 
-- 
2.30.2

Re: [PATCH 15/15] x86/hyperlaunch: add capabilities to boot domain

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> Introduce the ability to assign capabilities to a domain via its definition in
> device tree. The first capability enabled to select is the control domain
> capability. The capability property is a bitfield in both the device tree and
> `struct boot_domain`.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>   xen/arch/x86/domain_builder/core.c    |  2 +-
>   xen/arch/x86/domain_builder/fdt.c     | 13 +++++++++++++
>   xen/arch/x86/include/asm/bootdomain.h |  4 ++++
>   xen/arch/x86/setup.c                  |  6 +++++-
>   4 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/domain_builder/core.c
> index 95cab06e6159..eaa019472724 100644
> --- a/xen/arch/x86/domain_builder/core.c
> +++ b/xen/arch/x86/domain_builder/core.c
> @@ -93,9 +93,9 @@ void __init builder_init(struct boot_info *bi)
>           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 |= BUILD_CAPS_CONTROL;
>           bi->nr_domains = 1;
>       }
> -

This will get cleaned up earlier.

With that:

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Re: [PATCH 15/15] x86/hyperlaunch: add capabilities to boot domain

[Daniel P. Smith]

On 11/25/24 19:09, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> Introduce the ability to assign capabilities to a domain via its 
>> definition in
>> device tree. The first capability enabled to select is the control domain
>> capability. The capability property is a bitfield in both the device 
>> tree and
>> `struct boot_domain`.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>   xen/arch/x86/domain_builder/core.c    |  2 +-
>>   xen/arch/x86/domain_builder/fdt.c     | 13 +++++++++++++
>>   xen/arch/x86/include/asm/bootdomain.h |  4 ++++
>>   xen/arch/x86/setup.c                  |  6 +++++-
>>   4 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/ 
>> domain_builder/core.c
>> index 95cab06e6159..eaa019472724 100644
>> --- a/xen/arch/x86/domain_builder/core.c
>> +++ b/xen/arch/x86/domain_builder/core.c
>> @@ -93,9 +93,9 @@ void __init builder_init(struct boot_info *bi)
>>           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 |= BUILD_CAPS_CONTROL;
>>           bi->nr_domains = 1;
>>       }
>> -
> 
> This will get cleaned up earlier.
> 
> With that:
> 
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Thanks!

Re: [PATCH 15/15] x86/hyperlaunch: add capabilities to boot domain

[Jan Beulich]

On 23.11.2024 19:20, Daniel P. Smith wrote:
> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -209,6 +209,19 @@ static int __init process_domain_node(
>              bd->max_vcpus = val;
>              printk("  max vcpus: %d\n", bd->max_vcpus);
>          }
> +        if ( match_fdt_property(fdt, prop, "capabilities" ) )
> +        {
> +            if ( fdt_prop_as_u32(prop, &bd->capabilities) != 0 )
> +            {
> +                printk("  failed processing domain id for domain %s\n",
> +                       name == NULL ? "unknown" : name);
> +                return -EINVAL;
> +            }
> +            printk("  caps: ");
> +            if ( bd->capabilities & BUILD_CAPS_CONTROL )
> +                printk("c");
> +            printk("\n");
> +        }

What if any of the other bits is set?

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -992,6 +992,7 @@ static size_t __init domain_cmdline_size(
>  static struct domain *__init create_dom0(struct boot_info *bi)
>  {
>      char *cmdline = NULL;
> +    int create_flags = 0;

Once again unsigned int please.

> @@ -1023,7 +1024,10 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>      /* Create initial domain.  Not d0 for pvshim. */
>      if ( bd->domid == DOMID_INVALID )
>          bd->domid = get_initial_domain_id();
> -    d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
> +    if ( bd->capabilities & BUILD_CAPS_CONTROL )
> +            create_flags |= CDF_privileged;

Nit: Indentation.

Jan

Re: [PATCH 15/15] x86/hyperlaunch: add capabilities to boot domain

[Daniel P. Smith]

On 12/2/24 07:23, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
>> @@ -209,6 +209,19 @@ static int __init process_domain_node(
>>               bd->max_vcpus = val;
>>               printk("  max vcpus: %d\n", bd->max_vcpus);
>>           }
>> +        if ( match_fdt_property(fdt, prop, "capabilities" ) )
>> +        {
>> +            if ( fdt_prop_as_u32(prop, &bd->capabilities) != 0 )
>> +            {
>> +                printk("  failed processing domain id for domain %s\n",
>> +                       name == NULL ? "unknown" : name);
>> +                return -EINVAL;
>> +            }
>> +            printk("  caps: ");
>> +            if ( bd->capabilities & BUILD_CAPS_CONTROL )
>> +                printk("c");
>> +            printk("\n");
>> +        }
> 
> What if any of the other bits is set?

I'm not sure what you are getting at, but there is another cap added 
later for HARDWARE and it will print an 'h' next to the 'c' if set.

>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -992,6 +992,7 @@ static size_t __init domain_cmdline_size(
>>   static struct domain *__init create_dom0(struct boot_info *bi)
>>   {
>>       char *cmdline = NULL;
>> +    int create_flags = 0;
> 
> Once again unsigned int please.

ack.

>> @@ -1023,7 +1024,10 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>>       /* Create initial domain.  Not d0 for pvshim. */
>>       if ( bd->domid == DOMID_INVALID )
>>           bd->domid = get_initial_domain_id();
>> -    d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
>> +    if ( bd->capabilities & BUILD_CAPS_CONTROL )
>> +            create_flags |= CDF_privileged;
> 
> Nit: Indentation.

ack.

v/r,
dps

Re: [PATCH 15/15] x86/hyperlaunch: add capabilities to boot domain

[Jan Beulich]

On 11.12.2024 20:56, Daniel P. Smith wrote:
> On 12/2/24 07:23, Jan Beulich wrote:
>> On 23.11.2024 19:20, Daniel P. Smith wrote:
>>> --- a/xen/arch/x86/domain_builder/fdt.c
>>> +++ b/xen/arch/x86/domain_builder/fdt.c
>>> @@ -209,6 +209,19 @@ static int __init process_domain_node(
>>>               bd->max_vcpus = val;
>>>               printk("  max vcpus: %d\n", bd->max_vcpus);
>>>           }
>>> +        if ( match_fdt_property(fdt, prop, "capabilities" ) )
>>> +        {
>>> +            if ( fdt_prop_as_u32(prop, &bd->capabilities) != 0 )
>>> +            {
>>> +                printk("  failed processing domain id for domain %s\n",
>>> +                       name == NULL ? "unknown" : name);
>>> +                return -EINVAL;
>>> +            }
>>> +            printk("  caps: ");
>>> +            if ( bd->capabilities & BUILD_CAPS_CONTROL )
>>> +                printk("c");
>>> +            printk("\n");
>>> +        }
>>
>> What if any of the other bits is set?
> 
> I'm not sure what you are getting at, but there is another cap added 
> later for HARDWARE and it will print an 'h' next to the 'c' if set.

And that bit, when set, will likely have meaning beyond this mere printing?
If so, what's the effect on the domain when the bit is set, but the code
consuming the bit isn't there yet? Will the domain function correctly? IOW
shouldn't you reject any set bits that the code doesn't know how to handle?
And then perhaps report all unknown bits in a numeric value here?

Jan

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

[Jason Andryuk]

On 2024-11-23 13:20, Daniel P. Smith wrote:
> 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.

Having gone through this, I think you want to ensure that 
docs/designs/launch/hyperlaunch-devicetree.rst is updated with each 
patch adding a property to ensure they stay in sync.

Regards,
Jason

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

[Daniel P. Smith]

On 11/25/24 19:11, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> 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.
> 
> Having gone through this, I think you want to ensure that docs/designs/ 
> launch/hyperlaunch-devicetree.rst is updated with each patch adding a 
> property to ensure they stay in sync.

Ack.

v/r,
dps