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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.04.2025 20:01, Jason Andryuk wrote:
> 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.

Then CONFIG_DOMAIN_BUILDER is a misnomer?

>>> --- /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 ?

Whatever isn't arch-specific would better live somewhere under xen/common/, yes.

>>> --- /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?

Yes. The latter also when you need struct/union fields or sizes.

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

Here and elsewhere, for things that aren't quite right yet, mentioning
such aspects specially in the description (or in certain cases even in
FIXME comments) would help avoid questions like the one I had raised.

Jan