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 <dpsm...@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
