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

[dmkhn@xxxxxxxxx]

On Wed, Apr 23, 2025 at 12:52:58PM +0100, Alejandro Vallejo wrote:
> On Fri Apr 18, 2025 at 10:55 PM BST, dmkhn wrote:
> > 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)?
> 
> Inside the boot_info? As in separate aliasing pointers into the arrays?


I was thinking about local variables inside the function pointing to the
bi->mods[0] and bi->domains[0].


> I'd rather not. It'd be dangerous on systems without an actual dom0.
> 
> The PV shim comes to mind, but other configurations might arise in the
> future where no domain holds the id of 0.
> 
> >
> >>
> >>      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?
> 
> In this patch it sounds good, but a later patch adds more stuff at the
> tail of the function that must not be skipped, so it wouldn't work
> as-is.
> 
> Another matter is whether this function could be skipped in the "no-fdt"
> case, and it probably could. But I do know the longer series (big RFC
> from Daniel) adds more common logic present when !CONFIG_DOMAIN_BUILDER,
> so I'm reticent to deviate too much from it to avoid rebasing headaches.


I see, thanks for clarification.


> 
> >
> >> +    {
> >> +        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?
> 
> The function is a misnomer and it ought to change to return an
> enumerated type instead where it returns FDT_HYPERLAUNCH, FDT_DOM0LESS,
> FDT_UNKNOWN or NO_FDT. Using error codes for identification is a tad too
> hacky.
> 
> Cheers,
> Alejandro