From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
Introduce the domain builder which is capable of consuming a device tree as the
first boot module. If it finds a device tree as the first boot module, it will
set its type to BOOTMOD_FDT. This change only detects the boot module and
continues to boot with slight change to the boot convention that the dom0
kernel is no longer first boot module but is the second.
No functional change intended.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
---
xen/arch/x86/include/asm/bootinfo.h | 3 ++
xen/arch/x86/setup.c | 19 ++++++++----
xen/common/Makefile | 1 +
xen/common/domain-builder/Makefile | 2 ++
xen/common/domain-builder/core.c | 45 +++++++++++++++++++++++++++++
xen/common/domain-builder/fdt.c | 39 +++++++++++++++++++++++++
xen/common/domain-builder/fdt.h | 14 +++++++++
xen/include/xen/domain-builder.h | 29 +++++++++++++++++++
8 files changed, 146 insertions(+), 6 deletions(-)
create mode 100644 xen/common/domain-builder/Makefile
create mode 100644 xen/common/domain-builder/core.c
create mode 100644 xen/common/domain-builder/fdt.c
create mode 100644 xen/common/domain-builder/fdt.h
create mode 100644 xen/include/xen/domain-builder.h
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 3afc214c17..82c2650fcf 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -27,6 +27,7 @@ enum bootmod_type {
BOOTMOD_RAMDISK,
BOOTMOD_MICROCODE,
BOOTMOD_XSM_POLICY,
+ BOOTMOD_FDT,
};
struct boot_module {
@@ -80,6 +81,8 @@ struct boot_info {
paddr_t memmap_addr;
size_t memmap_length;
+ bool hyperlaunch_enabled;
+
unsigned int nr_modules;
struct boot_module mods[MAX_NR_BOOTMODS + 1];
struct boot_domain domains[MAX_NR_BOOTDOMS];
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2518954124..f3b5c83a3c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -6,6 +6,7 @@
#include <xen/cpuidle.h>
#include <xen/dmi.h>
#include <xen/domain.h>
+#include <xen/domain-builder.h>
#include <xen/domain_page.h>
#include <xen/efi.h>
#include <xen/err.h>
@@ -1284,9 +1285,14 @@ void asmlinkage __init noreturn __start_xen(void)
bi->nr_modules);
}
- /* Dom0 kernel is always first */
- bi->mods[0].type = BOOTMOD_KERNEL;
- bi->domains[0].kernel = &bi->mods[0];
+ if ( builder_init(bi) == FDT_KIND_NONE )
+ {
+ /* Find first unknown boot module to use as dom0 kernel */
+ i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
+ bi->mods[i].type = BOOTMOD_KERNEL;
+ bi->domains[0].kernel = &bi->mods[i];
+ bi->hyperlaunch_enabled = false;
+ }
if ( pvh_boot )
{
@@ -1469,8 +1475,9 @@ void asmlinkage __init noreturn __start_xen(void)
xen->size = __2M_rwdata_end - _stext;
}
- bi->mods[0].headroom =
- bzimage_headroom(bootstrap_map_bm(&bi->mods[0]), bi->mods[0].size);
+ bi->domains[0].kernel->headroom =
+ bzimage_headroom(bootstrap_map_bm(bi->domains[0].kernel),
+ bi->domains[0].kernel->size);
bootstrap_unmap();
#ifndef highmem_start
@@ -1594,7 +1601,7 @@ void asmlinkage __init noreturn __start_xen(void)
#endif
}
- if ( bi->mods[0].headroom && !bi->mods[0].relocated )
+ if ( bi->domains[0].kernel->headroom && !bi->domains[0].kernel->relocated )
panic("Not enough memory to relocate the dom0 kernel image\n");
for ( i = 0; i < bi->nr_modules; ++i )
{
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 98f0873056..e42af71e3f 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
obj-$(CONFIG_IOREQ_SERVER) += dm.o
obj-y += domain.o
+obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/
obj-y += event_2l.o
obj-y += event_channel.o
obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
diff --git a/xen/common/domain-builder/Makefile b/xen/common/domain-builder/Makefile
new file mode 100644
index 0000000000..bfd2f6267e
--- /dev/null
+++ b/xen/common/domain-builder/Makefile
@@ -0,0 +1,2 @@
+obj-y += fdt.init.o
+obj-y += core.init.o
diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
new file mode 100644
index 0000000000..97c92db571
--- /dev/null
+++ b/xen/common/domain-builder/core.c
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024, Apertus Solutions, LLC
+ */
+#include <xen/bug.h>
+#include <xen/err.h>
+#include <xen/init.h>
+#include <xen/kconfig.h>
+#include <xen/domain-builder.h>
+#include <xen/lib.h>
+
+#include <asm/bootinfo.h>
+
+#include "fdt.h"
+
+enum fdt_kind __init builder_init(struct boot_info *bi)
+{
+ enum fdt_kind kind;
+
+ bi->hyperlaunch_enabled = false;
+ switch ( (kind = fdt_detect_kind(bi)) )
+ {
+ case FDT_KIND_NONE:
+ /* No DT found */
+ return kind;
+
+ case FDT_KIND_UNKNOWN:
+ printk(XENLOG_DEBUG "DT found: non-hyperlaunch\n");
+ bi->mods[0].type = BOOTMOD_FDT;
+ return kind;
+
+ default:
+ BUG();
+ }
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
new file mode 100644
index 0000000000..4b07bd22c8
--- /dev/null
+++ b/xen/common/domain-builder/fdt.c
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024, Apertus Solutions, LLC
+ */
+#include <xen/err.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/libfdt/libfdt.h>
+
+#include <asm/bootinfo.h>
+#include <asm/page.h>
+#include <asm/setup.h>
+
+#include "fdt.h"
+
+enum fdt_kind __init fdt_detect_kind(const struct boot_info *bi)
+{
+ enum fdt_kind kind;
+ const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+
+ if ( !fdt || fdt_check_header(fdt) < 0 )
+ kind = FDT_KIND_NONE;
+ else
+ kind = FDT_KIND_UNKNOWN;
+
+ bootstrap_unmap();
+
+ return kind;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
new file mode 100644
index 0000000000..ef897fc412
--- /dev/null
+++ b/xen/common/domain-builder/fdt.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __XEN_DOMAIN_BUILDER_FDT_H__
+#define __XEN_DOMAIN_BUILDER_FDT_H__
+
+#include <xen/domain-builder.h>
+
+struct boot_info;
+
+/* hyperlaunch fdt is required to be module 0 */
+#define HYPERLAUNCH_MODULE_IDX 0
+
+enum fdt_kind fdt_detect_kind(const struct boot_info *bi);
+
+#endif /* __XEN_DOMAIN_BUILDER_FDT_H__ */
diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
new file mode 100644
index 0000000000..b9702db735
--- /dev/null
+++ b/xen/include/xen/domain-builder.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __XEN_DOMAIN_BUILDER_H__
+#define __XEN_DOMAIN_BUILDER_H__
+
+struct boot_info;
+
+/* Return status of builder_init(). Shows which boot mechanism was detected */
+enum fdt_kind
+{
+ /* FDT not found. Skipped builder. */
+ FDT_KIND_NONE,
+ /* Found an FDT that wasn't hyperlaunch. */
+ FDT_KIND_UNKNOWN,
+};
+
+/*
+ * Initialises `bi` if it detects a compatible FDT. Otherwise returns
+ * FDT_KIND_NONE and leaves initialisation up to the caller.
+ */
+#if IS_ENABLED(CONFIG_DOMAIN_BUILDER)
+enum fdt_kind builder_init(struct boot_info *bi);
+#else
+static inline enum fdt_kind builder_init(struct boot_info *bi)
+{
+ return FDT_KIND_NONE;
+}
+#endif /* !IS_ENABLED(CONFIG_DOMAIN_BUILDER) */
+
+#endif /* __XEN_DOMAIN_BUILDER_H__ */
--
2.43.0On 29.04.2025 14:36, Alejandro Vallejo wrote:
> @@ -1284,9 +1285,14 @@ void asmlinkage __init noreturn __start_xen(void)
> bi->nr_modules);
> }
>
> - /* Dom0 kernel is always first */
> - bi->mods[0].type = BOOTMOD_KERNEL;
> - bi->domains[0].kernel = &bi->mods[0];
> + if ( builder_init(bi) == FDT_KIND_NONE )
With this, can ...
> + {
> + /* Find first unknown boot module to use as dom0 kernel */
> + i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
... i ever be anything else than 0? If not, perhaps keeping the call here is
still fine (kind of for doc purposes), but an assertion may then want adding.
> + bi->mods[i].type = BOOTMOD_KERNEL;
> + bi->domains[0].kernel = &bi->mods[i];
> + bi->hyperlaunch_enabled = false;
Is this necessary, when the field is supposed to be starting out clear?
> --- /dev/null
> +++ b/xen/common/domain-builder/Makefile
> @@ -0,0 +1,2 @@
> +obj-y += fdt.init.o
> +obj-y += core.init.o
Any reason for these not both adding to obj-bin-y, like we do elsewhere for
*.init.o?
Also please sort object lists alphabetically.
> --- /dev/null
> +++ b/xen/include/xen/domain-builder.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __XEN_DOMAIN_BUILDER_H__
> +#define __XEN_DOMAIN_BUILDER_H__
> +
> +struct boot_info;
> +
> +/* Return status of builder_init(). Shows which boot mechanism was detected */
> +enum fdt_kind
> +{
> + /* FDT not found. Skipped builder. */
> + FDT_KIND_NONE,
> + /* Found an FDT that wasn't hyperlaunch. */
> + FDT_KIND_UNKNOWN,
> +};
> +
> +/*
> + * Initialises `bi` if it detects a compatible FDT. Otherwise returns
> + * FDT_KIND_NONE and leaves initialisation up to the caller.
> + */
> +#if IS_ENABLED(CONFIG_DOMAIN_BUILDER)
For the pre-processor it wants to be the simpler #ifdef.
Jan
On Wed May 21, 2025 at 10:54 AM CEST, Jan Beulich wrote:
> On 29.04.2025 14:36, Alejandro Vallejo wrote:
>> @@ -1284,9 +1285,14 @@ void asmlinkage __init noreturn __start_xen(void)
>> bi->nr_modules);
>> }
>>
>> - /* Dom0 kernel is always first */
>> - bi->mods[0].type = BOOTMOD_KERNEL;
>> - bi->domains[0].kernel = &bi->mods[0];
>> + if ( builder_init(bi) == FDT_KIND_NONE )
>
> With this, can ...
>
>> + {
>> + /* Find first unknown boot module to use as dom0 kernel */
>> + i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
>
> ... i ever be anything else than 0? If not, perhaps keeping the call here is
> still fine (kind of for doc purposes), but an assertion may then want adding.
I don't think so, no. It's there for symmetry with the way the initrd is
discovered later on, as that might be on module1, or 2, or 3 depending
on whether there's microcode, or an XSM policy or anything else. in
other modules.
>
>> + bi->mods[i].type = BOOTMOD_KERNEL;
>> + bi->domains[0].kernel = &bi->mods[i];
>> + bi->hyperlaunch_enabled = false;
>
> Is this necessary, when the field is supposed to be starting out clear?
It isn't necessary, but I think it gave a sense of intent. That said I'm
pondering removing that boolean though in favour of something like
bi->mods[0].type == BOOTMOD_FDT
At which point that assignment disappears.
>
>> --- /dev/null
>> +++ b/xen/common/domain-builder/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-y += fdt.init.o
>> +obj-y += core.init.o
>
> Any reason for these not both adding to obj-bin-y, like we do elsewhere for
> *.init.o?
>
> Also please sort object lists alphabetically.
>
>> --- /dev/null
>> +++ b/xen/include/xen/domain-builder.h
>> @@ -0,0 +1,29 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef __XEN_DOMAIN_BUILDER_H__
>> +#define __XEN_DOMAIN_BUILDER_H__
>> +
>> +struct boot_info;
>> +
>> +/* Return status of builder_init(). Shows which boot mechanism was detected */
>> +enum fdt_kind
>> +{
>> + /* FDT not found. Skipped builder. */
>> + FDT_KIND_NONE,
>> + /* Found an FDT that wasn't hyperlaunch. */
>> + FDT_KIND_UNKNOWN,
>> +};
>> +
>> +/*
>> + * Initialises `bi` if it detects a compatible FDT. Otherwise returns
>> + * FDT_KIND_NONE and leaves initialisation up to the caller.
>> + */
>> +#if IS_ENABLED(CONFIG_DOMAIN_BUILDER)
>
> For the pre-processor it wants to be the simpler #ifdef.
True. Don't know what I was thinking.
>
> Jan
Cheers,
Alejandro
On 4/29/25 08:36, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> Introduce the domain builder which is capable of consuming a device tree as the
> first boot module. If it finds a device tree as the first boot module, it will
> set its type to BOOTMOD_FDT. This change only detects the boot module and
> continues to boot with slight change to the boot convention that the dom0
> kernel is no longer first boot module but is the second.
>
> No functional change intended.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> Reviewed-by: Denis Mukhin <dmukhin@ford.com>
> ---
> xen/arch/x86/include/asm/bootinfo.h | 3 ++
> xen/arch/x86/setup.c | 19 ++++++++----
> xen/common/Makefile | 1 +
> xen/common/domain-builder/Makefile | 2 ++
> xen/common/domain-builder/core.c | 45 +++++++++++++++++++++++++++++
> xen/common/domain-builder/fdt.c | 39 +++++++++++++++++++++++++
> xen/common/domain-builder/fdt.h | 14 +++++++++
> xen/include/xen/domain-builder.h | 29 +++++++++++++++++++
> 8 files changed, 146 insertions(+), 6 deletions(-)
> create mode 100644 xen/common/domain-builder/Makefile
> create mode 100644 xen/common/domain-builder/core.c
> create mode 100644 xen/common/domain-builder/fdt.c
> create mode 100644 xen/common/domain-builder/fdt.h
> create mode 100644 xen/include/xen/domain-builder.h
>
> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> index 3afc214c17..82c2650fcf 100644
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -27,6 +27,7 @@ enum bootmod_type {
> BOOTMOD_RAMDISK,
> BOOTMOD_MICROCODE,
> BOOTMOD_XSM_POLICY,
> + BOOTMOD_FDT,
> };
>
> struct boot_module {
> @@ -80,6 +81,8 @@ struct boot_info {
> paddr_t memmap_addr;
> size_t memmap_length;
>
> + bool hyperlaunch_enabled;
> +
> unsigned int nr_modules;
> struct boot_module mods[MAX_NR_BOOTMODS + 1];
> struct boot_domain domains[MAX_NR_BOOTDOMS];
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 2518954124..f3b5c83a3c 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -6,6 +6,7 @@
> #include <xen/cpuidle.h>
> #include <xen/dmi.h>
> #include <xen/domain.h>
> +#include <xen/domain-builder.h>
> #include <xen/domain_page.h>
> #include <xen/efi.h>
> #include <xen/err.h>
> @@ -1284,9 +1285,14 @@ void asmlinkage __init noreturn __start_xen(void)
> bi->nr_modules);
> }
>
> - /* Dom0 kernel is always first */
> - bi->mods[0].type = BOOTMOD_KERNEL;
> - bi->domains[0].kernel = &bi->mods[0];
> + if ( builder_init(bi) == FDT_KIND_NONE )
> + {
> + /* Find first unknown boot module to use as dom0 kernel */
> + i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
> + bi->mods[i].type = BOOTMOD_KERNEL;
> + bi->domains[0].kernel = &bi->mods[i];
> + bi->hyperlaunch_enabled = false;
> + }
>
> if ( pvh_boot )
> {
> @@ -1469,8 +1475,9 @@ void asmlinkage __init noreturn __start_xen(void)
> xen->size = __2M_rwdata_end - _stext;
> }
>
> - bi->mods[0].headroom =
> - bzimage_headroom(bootstrap_map_bm(&bi->mods[0]), bi->mods[0].size);
> + bi->domains[0].kernel->headroom =
> + bzimage_headroom(bootstrap_map_bm(bi->domains[0].kernel),
> + bi->domains[0].kernel->size);
> bootstrap_unmap();
>
> #ifndef highmem_start
> @@ -1594,7 +1601,7 @@ void asmlinkage __init noreturn __start_xen(void)
> #endif
> }
>
> - if ( bi->mods[0].headroom && !bi->mods[0].relocated )
> + if ( bi->domains[0].kernel->headroom && !bi->domains[0].kernel->relocated )
> panic("Not enough memory to relocate the dom0 kernel image\n");
> for ( i = 0; i < bi->nr_modules; ++i )
> {
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 98f0873056..e42af71e3f 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
> obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
> obj-$(CONFIG_IOREQ_SERVER) += dm.o
> obj-y += domain.o
> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/
Please don't do this, use IF_ENABLED in core.c and then hide the
unnecessary units in domain-builder/Makefile as I originally had it.
This allows for a much easier time incrementally converting the dom0
construction path into a generalized domain construction path.
> obj-y += event_2l.o
> obj-y += event_channel.o
> obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
> diff --git a/xen/common/domain-builder/Makefile b/xen/common/domain-builder/Makefile
> new file mode 100644
> index 0000000000..bfd2f6267e
> --- /dev/null
> +++ b/xen/common/domain-builder/Makefile
> @@ -0,0 +1,2 @@
> +obj-y += fdt.init.o
> +obj-y += core.init.o
> diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
> new file mode 100644
> index 0000000000..97c92db571
> --- /dev/null
> +++ b/xen/common/domain-builder/core.c
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024, Apertus Solutions, LLC
> + */
> +#include <xen/bug.h>
> +#include <xen/err.h>
> +#include <xen/init.h>
> +#include <xen/kconfig.h>
> +#include <xen/domain-builder.h>
> +#include <xen/lib.h>
> +
> +#include <asm/bootinfo.h>
> +
> +#include "fdt.h"
> +
> +enum fdt_kind __init builder_init(struct boot_info *bi)
> +{
> + enum fdt_kind kind;
> +
> + bi->hyperlaunch_enabled = false;
> + switch ( (kind = fdt_detect_kind(bi)) )
> + {
> + case FDT_KIND_NONE:
> + /* No DT found */
> + return kind;
> +
> + case FDT_KIND_UNKNOWN:
> + printk(XENLOG_DEBUG "DT found: non-hyperlaunch\n");
> + bi->mods[0].type = BOOTMOD_FDT;
> + return kind;
> +
> + default:
> + BUG();
> + }
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> new file mode 100644
> index 0000000000..4b07bd22c8
> --- /dev/null
> +++ b/xen/common/domain-builder/fdt.c
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024, Apertus Solutions, LLC
> + */
> +#include <xen/err.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/libfdt/libfdt.h>
> +
> +#include <asm/bootinfo.h>
> +#include <asm/page.h>
> +#include <asm/setup.h>
> +
> +#include "fdt.h"
> +
> +enum fdt_kind __init fdt_detect_kind(const struct boot_info *bi)
> +{
> + enum fdt_kind kind;
> + const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +
> + if ( !fdt || fdt_check_header(fdt) < 0 )
> + kind = FDT_KIND_NONE;
> + else
> + kind = FDT_KIND_UNKNOWN;
> +
> + bootstrap_unmap();
> +
> + return kind;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
> new file mode 100644
> index 0000000000..ef897fc412
> --- /dev/null
> +++ b/xen/common/domain-builder/fdt.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __XEN_DOMAIN_BUILDER_FDT_H__
> +#define __XEN_DOMAIN_BUILDER_FDT_H__
> +
> +#include <xen/domain-builder.h>
> +
> +struct boot_info;
> +
> +/* hyperlaunch fdt is required to be module 0 */
> +#define HYPERLAUNCH_MODULE_IDX 0
> +
> +enum fdt_kind fdt_detect_kind(const struct boot_info *bi);
> +
> +#endif /* __XEN_DOMAIN_BUILDER_FDT_H__ */
> diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
> new file mode 100644
> index 0000000000..b9702db735
> --- /dev/null
> +++ b/xen/include/xen/domain-builder.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __XEN_DOMAIN_BUILDER_H__
> +#define __XEN_DOMAIN_BUILDER_H__
> +
> +struct boot_info;
> +
> +/* Return status of builder_init(). Shows which boot mechanism was detected */
> +enum fdt_kind
> +{
> + /* FDT not found. Skipped builder. */
> + FDT_KIND_NONE,
> + /* Found an FDT that wasn't hyperlaunch. */
> + FDT_KIND_UNKNOWN,
> +};
> +
> +/*
> + * Initialises `bi` if it detects a compatible FDT. Otherwise returns
> + * FDT_KIND_NONE and leaves initialisation up to the caller.
> + */
> +#if IS_ENABLED(CONFIG_DOMAIN_BUILDER)
> +enum fdt_kind builder_init(struct boot_info *bi);
> +#else
> +static inline enum fdt_kind builder_init(struct boot_info *bi)
> +{
> + return FDT_KIND_NONE;
> +}
> +#endif /* !IS_ENABLED(CONFIG_DOMAIN_BUILDER) */
> +
> +#endif /* __XEN_DOMAIN_BUILDER_H__ */
On 30.04.2025 20:56, Daniel P. Smith wrote: > On 4/29/25 08:36, Alejandro Vallejo wrote: >> --- a/xen/common/Makefile >> +++ b/xen/common/Makefile >> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o >> obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/ >> obj-$(CONFIG_IOREQ_SERVER) += dm.o >> obj-y += domain.o >> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/ > > Please don't do this, use IF_ENABLED in core.c and then hide the > unnecessary units in domain-builder/Makefile as I originally had it. > This allows for a much easier time incrementally converting the dom0 > construction path into a generalized domain construction path. That is, are you viewing this as a transitional thing only? If the end goal is to have it as Alejandro has it above, that may be acceptable (even if not nice). Jan
On 5/2/25 03:21, Jan Beulich wrote: > On 30.04.2025 20:56, Daniel P. Smith wrote: >> On 4/29/25 08:36, Alejandro Vallejo wrote: >>> --- a/xen/common/Makefile >>> +++ b/xen/common/Makefile >>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o >>> obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/ >>> obj-$(CONFIG_IOREQ_SERVER) += dm.o >>> obj-y += domain.o >>> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/ >> >> Please don't do this, use IF_ENABLED in core.c and then hide the >> unnecessary units in domain-builder/Makefile as I originally had it. >> This allows for a much easier time incrementally converting the dom0 >> construction path into a generalized domain construction path. > > That is, are you viewing this as a transitional thing only? If the end > goal is to have it as Alejandro has it above, that may be acceptable > (even if not nice). In the end, it became too much of a mess to keep the core builder functions with the fdt code in common. So I left this as is, and kept all the builder logic in x86/domain-builer/core.c. v/r, dps
On 5/2/25 03:21, Jan Beulich wrote: > On 30.04.2025 20:56, Daniel P. Smith wrote: >> On 4/29/25 08:36, Alejandro Vallejo wrote: >>> --- a/xen/common/Makefile >>> +++ b/xen/common/Makefile >>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o >>> obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/ >>> obj-$(CONFIG_IOREQ_SERVER) += dm.o >>> obj-y += domain.o >>> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/ >> >> Please don't do this, use IF_ENABLED in core.c and then hide the >> unnecessary units in domain-builder/Makefile as I originally had it. >> This allows for a much easier time incrementally converting the dom0 >> construction path into a generalized domain construction path. > > That is, are you viewing this as a transitional thing only? If the end > goal is to have it as Alejandro has it above, that may be acceptable > (even if not nice). There is/will be shared domain construction code between dom0-only and multidomain construction, with it will all living under domain builder. So no, the end goal is not what Alejandro just did. The end result will be the way I had it, with a different kconfig option to enable/disable the multidomain construction path. v/r, dps
On 06.05.2025 21:29, Daniel P. Smith wrote: > On 5/2/25 03:21, Jan Beulich wrote: >> On 30.04.2025 20:56, Daniel P. Smith wrote: >>> On 4/29/25 08:36, Alejandro Vallejo wrote: >>>> --- a/xen/common/Makefile >>>> +++ b/xen/common/Makefile >>>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o >>>> obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/ >>>> obj-$(CONFIG_IOREQ_SERVER) += dm.o >>>> obj-y += domain.o >>>> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/ >>> >>> Please don't do this, use IF_ENABLED in core.c and then hide the >>> unnecessary units in domain-builder/Makefile as I originally had it. >>> This allows for a much easier time incrementally converting the dom0 >>> construction path into a generalized domain construction path. >> >> That is, are you viewing this as a transitional thing only? If the end >> goal is to have it as Alejandro has it above, that may be acceptable >> (even if not nice). > > There is/will be shared domain construction code between dom0-only and > multidomain construction, with it will all living under domain builder. > So no, the end goal is not what Alejandro just did. The end result will > be the way I had it, with a different kconfig option to enable/disable > the multidomain construction path. Isn't CONFIG_DOMAIN_BUILDER a misnomer then? Jan
On 5/13/25 04:05, Jan Beulich wrote: > On 06.05.2025 21:29, Daniel P. Smith wrote: >> On 5/2/25 03:21, Jan Beulich wrote: >>> On 30.04.2025 20:56, Daniel P. Smith wrote: >>>> On 4/29/25 08:36, Alejandro Vallejo wrote: >>>>> --- a/xen/common/Makefile >>>>> +++ b/xen/common/Makefile >>>>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o >>>>> obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/ >>>>> obj-$(CONFIG_IOREQ_SERVER) += dm.o >>>>> obj-y += domain.o >>>>> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/ >>>> >>>> Please don't do this, use IF_ENABLED in core.c and then hide the >>>> unnecessary units in domain-builder/Makefile as I originally had it. >>>> This allows for a much easier time incrementally converting the dom0 >>>> construction path into a generalized domain construction path. >>> >>> That is, are you viewing this as a transitional thing only? If the end >>> goal is to have it as Alejandro has it above, that may be acceptable >>> (even if not nice). >> >> There is/will be shared domain construction code between dom0-only and >> multidomain construction, with it will all living under domain builder. >> So no, the end goal is not what Alejandro just did. The end result will >> be the way I had it, with a different kconfig option to enable/disable >> the multidomain construction path. > > Isn't CONFIG_DOMAIN_BUILDER a misnomer then? Which is why I originally had two kconfig flags, one to enable dtb parsing for domain configuration, which allowed dom0 construction from dtb. Then there was the second flag that was to enable multi-domain construction. I have reworked a portion of the approach in the RFC based on feedback, which is based off of this series. In it, I tried to minimize how much went into common, but you will see how I still had to rework the kconfig flags. v/r, dps
On Wed May 14, 2025 at 12:23 AM CEST, Daniel P. Smith wrote:
> On 5/13/25 04:05, Jan Beulich wrote:
>> On 06.05.2025 21:29, Daniel P. Smith wrote:
>>> On 5/2/25 03:21, Jan Beulich wrote:
>>>> On 30.04.2025 20:56, Daniel P. Smith wrote:
>>>>> On 4/29/25 08:36, Alejandro Vallejo wrote:
>>>>>> --- a/xen/common/Makefile
>>>>>> +++ b/xen/common/Makefile
>>>>>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
>>>>>> obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
>>>>>> obj-$(CONFIG_IOREQ_SERVER) += dm.o
>>>>>> obj-y += domain.o
>>>>>> +obj-$(CONFIG_DOMAIN_BUILDER) += domain-builder/
>>>>>
>>>>> Please don't do this, use IF_ENABLED in core.c and then hide the
>>>>> unnecessary units in domain-builder/Makefile as I originally had it.
>>>>> This allows for a much easier time incrementally converting the dom0
>>>>> construction path into a generalized domain construction path.
>>>>
>>>> That is, are you viewing this as a transitional thing only? If the end
>>>> goal is to have it as Alejandro has it above, that may be acceptable
>>>> (even if not nice).
>>>
>>> There is/will be shared domain construction code between dom0-only and
>>> multidomain construction, with it will all living under domain builder.
>>> So no, the end goal is not what Alejandro just did. The end result will
>>> be the way I had it, with a different kconfig option to enable/disable
>>> the multidomain construction path.
>>
>> Isn't CONFIG_DOMAIN_BUILDER a misnomer then?
>
> Which is why I originally had two kconfig flags, one to enable dtb
> parsing for domain configuration, which allowed dom0 construction from
> dtb. Then there was the second flag that was to enable multi-domain
> construction. I have reworked a portion of the approach in the RFC based
> on feedback, which is based off of this series. In it, I tried to
> minimize how much went into common, but you will see how I still had to
> rework the kconfig flags.
>
> v/r,
> dps
Does it really make sense to have a flag to restrict multidomain while
allowing parsing the DTB? There's virtually nothing compiled out in that
case.
If you did it that way because it doesn't initially build several
domains, that's just transitional and to be expected on any feature
tagged as unsupported with (EXPERIMENTAL) in the name.
What if I collapse everything under a single CONFIG_MULTIDOMAIN_BUILDER
that compiles-in support for parsing DTBs while introducing an
unconditional builder as I go? From what I'm seeing, there are no
breaking changes in the series and the end goal is to have the builder
be unconditionally used, after all.
In fact, with the bindings code in common, I can also collapse everything
in core.c (and later domain.c) into a single arch/x86/domain-builder.c
that is unconditionally compiled in. The DTB parsing logic is already
in separate files and that can be compiled out with
CONFIG_MULTIDOMAIN_BUILDER flag.
In retrospect, after looking at dom0less long enough there's bootfdt.c and
bootinfo.c with similar intent, but far more ad-hoc cohesion. While the
builder wants to be in common, no other arch is in a position to take
it. It needs merging with the stuff done in bootfdt.c/bootinfo.c
So, in short. I'm planning to:
1. Collapse domain-builder/{core,domain}.c into domain-builder.c
under arch/x86. There's little reason to have them separate.
2. Remove CONFIG_DOMAIN_BUILDER, and replace it with something that
reflects the intent of using a DTB. Either CONFIG_MULTIDOMAIN_BUILDER
or CONFIG_DTB_BUILDER. Or maybe even CONFIG_DTB_BOOT.
* I specifically want to avoid CONFIG_BOOTFDT, because that'd
create confusion with the already existing bootfdt.c in common.
and, from the discussion in the other thread about code sharing in the
spirit of getting somewhere soon:
3. Do minimal parsing at builder_init() (module identification,
basically), and do the full parsing by the create_dom0() mark,
immediately before constructing all domains. With the unflattened
tree.
Moving forward I for sure want to merge the boot paths in the x86
builder with those of arm/riscv. Having a unified boot frontend can only
bring niceness.
Cheers,
Alejandro
© 2016 - 2025 Red Hat, Inc.