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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.12.2024 17:57, Daniel P. Smith wrote:
> --- 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/

The set of subdirs needed in $(obj-y) is specified at the top of the file.
Also shouldn't this be obj-$(CONFIG_DOMAIN_BUILDER)?

> --- /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;
> +
> +        case -ENOENT:
> +        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;
> +            break;
> +        }
> +    }
> +}

What is it that's x86-specific in here?

> --- /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>
> +#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(struct boot_info *bi)

Pointer-to-const?

> +{
> +    int ret = 0;
> +    const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +
> +    if ( fdt_check_header(fdt) < 0 )
> +        ret = -EINVAL;
> +
> +    bootstrap_unmap();
> +
> +    return ret;
> +}

Is this function intended to later be extended? Aiui anything fitting
the hyperlaunch-agnostic fdt_check_header() will do here, despite the
name of the function.

And again - what is it that's x86-specific in here?

> --- /dev/null
> +++ b/xen/arch/x86/domain-builder/fdt.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) */
> +#ifndef __XEN_X86_FDT_H__
> +#define __XEN_X86_FDT_H__
> +
> +#include <xen/init.h>
> +
> +#include <asm/bootinfo.h>

This isn't needed here, nor ...

> --- /dev/null
> +++ b/xen/arch/x86/include/asm/domainbuilder.h
> @@ -0,0 +1,8 @@
> +#ifndef __XEN_X86_DOMBUILDER_H__
> +#define __XEN_X86_DOMBUILDER_H__
> +
> +#include <asm/bootinfo.h>

... here, is it? Forward decls of struct boot_info are going to do.

> @@ -1285,9 +1286,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];

This is going to change again later? Or else what about there already
being a module marked BOOTMOD_KERNEL?

Jan