Re: [PATCH v6 02/12] common/hyperlaunch: introduce the domain builder

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Wed May 21, 2025 at 10:54 AM CEST, Jan Beulich wrote:
> On 29.04.2025 14:36, Alejandro Vallejo wrote:
>> @@ -1284,9 +1285,14 @@ 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];
>> +    if ( builder_init(bi) == FDT_KIND_NONE )
>
> With this, can ...
>
>> +    {
>> +        /* Find first unknown boot module to use as dom0 kernel */
>> +        i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
>
> ... i ever be anything else than 0? If not, perhaps keeping the call here is
> still fine (kind of for doc purposes), but an assertion may then want adding.

I don't think so, no. It's there for symmetry with the way the initrd is
discovered later on, as that might be on module1, or 2, or 3 depending
on whether there's microcode, or an XSM policy or anything else. in
other modules.

>
>> +        bi->mods[i].type = BOOTMOD_KERNEL;
>> +        bi->domains[0].kernel = &bi->mods[i];
>> +        bi->hyperlaunch_enabled = false;
>
> Is this necessary, when the field is supposed to be starting out clear?

It isn't necessary, but I think it gave a sense of intent. That said I'm
pondering removing that boolean though in favour of something like

  bi->mods[0].type == BOOTMOD_FDT

At which point that assignment disappears.

>
>> --- /dev/null
>> +++ b/xen/common/domain-builder/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-y += fdt.init.o
>> +obj-y += core.init.o
>
> Any reason for these not both adding to obj-bin-y, like we do elsewhere for
> *.init.o?
>
> Also please sort object lists alphabetically.
>
>> --- /dev/null
>> +++ b/xen/include/xen/domain-builder.h
>> @@ -0,0 +1,29 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef __XEN_DOMAIN_BUILDER_H__
>> +#define __XEN_DOMAIN_BUILDER_H__
>> +
>> +struct boot_info;
>> +
>> +/* Return status of builder_init(). Shows which boot mechanism was detected 
>> */
>> +enum fdt_kind
>> +{
>> +    /* FDT not found. Skipped builder. */
>> +    FDT_KIND_NONE,
>> +    /* Found an FDT that wasn't hyperlaunch. */
>> +    FDT_KIND_UNKNOWN,
>> +};
>> +
>> +/*
>> + * Initialises `bi` if it detects a compatible FDT. Otherwise returns
>> + * FDT_KIND_NONE and leaves initialisation up to the caller.
>> + */
>> +#if IS_ENABLED(CONFIG_DOMAIN_BUILDER)
>
> For the pre-processor it wants to be the simpler #ifdef.

True. Don't know what I was thinking.

>
> Jan

Cheers,
Alejandro