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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2025 18:07, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -1,6 +1,7 @@
>  obj-y += acpi/
>  obj-y += boot/
>  obj-y += cpu/
> +obj-y += domain-builder/

I continue to be irritated that this isn't obj-$(DOMAIN_BUILDER). The
sole function in core.c has "if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )" around
its entire body (which is unhelpful at least from an indentation pov), and
the sole function in fdt.c is only used by the one in core.c. builder_init()
clearly could have an inline stub alternative as of this patch. If future
patches change that picture, imo that would need saying here to justify this
unconditional descend into the new subdir.

> --- a/xen/arch/x86/domain-builder/Kconfig
> +++ b/xen/arch/x86/domain-builder/Kconfig
> @@ -3,7 +3,7 @@ menu "Domain Builder Features"
>  
>  config DOMAIN_BUILDER
>       bool "Domain builder (UNSUPPORTED)" if UNSUPPORTED
> -     select LIB_DEVICE_TREE
> +     select LIBFDT

That's what the earlier patch meant to be doing?

> --- /dev/null
> +++ b/xen/arch/x86/domain-builder/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_DOMAIN_BUILDER) += fdt.init.o
> +obj-y += core.init.o

*.init.o want enlisting into obj-bin-y.

> --- /dev/null
> +++ b/xen/arch/x86/domain-builder/core.c
> @@ -0,0 +1,57 @@
> +/* 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;

For people not using hyperlaunch (which for the time being is likely going
to be a majority) this log line will be little more than spam. I'd like it
to be considered to be dropped.

> --- /dev/null
> +++ b/xen/arch/x86/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>

Was xen/errno.h meant here?

> +#include <xen/init.h>
> +#include <xen/lib.h>

I don't see anything used from there.

> +#include <xen/libfdt/libfdt.h>
> +
> +#include <asm/bootinfo.h>
> +#include <asm/page.h>

How does page.h come into play here?

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -36,6 +36,7 @@
>  #include <asm/bzimage.h>
>  #include <asm/cpu-policy.h>
>  #include <asm/desc.h>
> +#include <asm/domain-builder.h>
>  #include <asm/e820.h>
>  #include <asm/edd.h>
>  #include <asm/genapic.h>
> @@ -1281,9 +1282,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);

For this I think it would be quite desirable if in a prereq patch i's type
was (finally) changed to unsigned int, which is how it's used effectively
everywhere, with one loop requiring a little bit of adjustment.

> +    bi->mods[i].type = BOOTMOD_KERNEL;

Overrunning the array if MAX_NR_BOOTMODS + 1 was returned.

Jan