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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-01-30 09:52, Jan Beulich wrote:
> 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)?
Later, all boot-time domain building is handled by 
domain-builder/core.c.  So some of domain-builder/ is always built, and 
Kconfig disables multidomain support.
> > --- /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?
Would you prefer xen/common/domain-builder ?

> > --- /dev/null
> > +++ b/xen/arch/x86/domain-builder/fdt.c
> > +{
> > +    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.
Eventually, there will be some checking to ensure that the DT actually 
contains hyperlaunch device nodes.
> And again - what is it that's x86-specific in here?
> > --- /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.
Generally, if you only need a type, just use a forward decl?  Use an 
include when you need function prototypes?
> > @@ -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?
Yes, it will change.  There will be two paths, and this is part of the 
non-Hyperlaunch path which needs to implicitly select kernel and initrd 
from the module order, the same as today.  For hyperlaunch, the device 
tree explicitly assigns kernel and initrd.
Regards,
Jason