Re: [PATCH v4 03/13] common/hyperlaunch: introduce the domain builder

[dmkhn@xxxxxxxxx]

On Thu, Apr 17, 2025 at 01:48:25PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
> ---
> v4:
>   * Moved from arch/x86/ to common/
>   * gated all of domain-builder/ on CONFIG_BOOT_INFO
>   * Hide the domain builder submenu for !X86
>   * Factor out the "hyperlaunch_enabled = false" toggle core.c
>   * Removed stub inline, as DCE makes it unnecessary
>   * Adjusted printks.
> ---
>  xen/arch/x86/include/asm/bootinfo.h |  3 ++
>  xen/arch/x86/setup.c                | 17 +++++----
>  xen/common/Makefile                 |  1 +
>  xen/common/domain-builder/Makefile  |  2 ++
>  xen/common/domain-builder/core.c    | 56 +++++++++++++++++++++++++++++
>  xen/common/domain-builder/fdt.c     | 37 +++++++++++++++++++
>  xen/common/domain-builder/fdt.h     | 12 +++++++
>  xen/include/xen/domain-builder.h    |  9 +++++
>  8 files changed, 131 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 4df012460d..ccc57cc70a 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -5,6 +5,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>
> @@ -1282,9 +1283,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];

Nit: perhaps add convenience aliases for bi->domains[0] (e.g. dom0) and 
for bi->mods[0] (e.g. mod)?

> 
>      if ( pvh_boot )
>      {
> @@ -1467,8 +1471,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
> @@ -1592,7 +1597,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..565837bc71 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_HAS_BOOT_INFO) += 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..b10cd56b28
> --- /dev/null
> +++ b/xen/common/domain-builder/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_DOMAIN_BUILDER) += 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..a5b21fc179
> --- /dev/null
> +++ b/xen/common/domain-builder/core.c
> @@ -0,0 +1,56 @@
> +/* 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)
> +{
> +    bi->hyperlaunch_enabled = false;
> +
> +    if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )

I would re-organize the code to remove one level of indentation, e.g.:

       if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
            return;

       switch ( ret = has_hyperlaunch_fdt(bi) )
       ...

or even add #ifdef CONFIG_DOMAIN_BUILDER for builder_init() in the header file.

What do you think?

> +    {
> +        int ret;
> +
> +        switch ( ret = has_hyperlaunch_fdt(bi) )
> +        {
> +        case 0:
> +            printk(XENLOG_DEBUG "DT found: hyperlaunch\n");
> +            bi->hyperlaunch_enabled = true;
> +            bi->mods[0].type = BOOTMOD_FDT;
> +            break;
> +
> +        case -EINVAL:
> +            /* No DT found */
> +            break;
> +
> +        case -ENOENT:
> +        case -ENODATA:

Looks like this code accounts for the follow on change: current implementation
only returns -EINVAL or 0.

Is it possible to convert has_hyperlaunch_fdt() to a simple predicate?

> +            printk(XENLOG_DEBUG "DT found: non-hyperlaunch (%d)\n", ret);
> +            bi->mods[0].type = BOOTMOD_FDT;
> +            break;
> +
> +        default:
> +            printk(XENLOG_ERR "unknown error (%d) checking hyperlaunch DT\n",
> +                   ret);
> +            break;
> +        }
> +    }
> +}
> +
> +/*
> + * 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..aaf8c1cc16
> --- /dev/null
> +++ b/xen/common/domain-builder/fdt.c
> @@ -0,0 +1,37 @@
> +/* 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"
> +
> +int __init has_hyperlaunch_fdt(const struct boot_info *bi)

I think the function can return `bool` in current patch.

> +{
> +    int ret = 0;
> +    const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +
> +    if ( !fdt || 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/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
> new file mode 100644
> index 0000000000..97a45a6ec4
> --- /dev/null
> +++ b/xen/common/domain-builder/fdt.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __XEN_DOMAIN_BUILDER_FDT_H__
> +#define __XEN_DOMAIN_BUILDER_FDT_H__
> +
> +struct boot_info;
> +
> +/* hyperlaunch fdt is required to be module 0 */
> +#define HYPERLAUNCH_MODULE_IDX 0
> +
> +int has_hyperlaunch_fdt(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..ac2b84775d
> --- /dev/null
> +++ b/xen/include/xen/domain-builder.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __XEN_DOMAIN_BUILDER_H__
> +#define __XEN_DOMAIN_BUILDER_H__
> +
> +struct boot_info;
> +
> +void builder_init(struct boot_info *bi);
> +
> +#endif /* __XEN_DOMAIN_BUILDER_H__ */
> --
> 2.43.0
> 
>