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

[Alejandro Vallejo <agarciav@xxxxxxx>]

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'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.

>
>> +    {
>> +        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