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

[Jason Andryuk <jason.andryuk@amd.com>]

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