Re: [PATCH 06/15] x86/hyperlaunch: introduce the domain builder

[Jason Andryuk <jason.andryuk@xxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx>
> 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 )
>       {