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.2On 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
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
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
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
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 )
> {
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
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
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
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
© 2016 - 2024 Red Hat, Inc.