Re: [PATCH v4 06/13] x86/hyperlaunch: locate dom0 kernel with hyperlaunch

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Fri Apr 18, 2025 at 11:39 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:28PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> 
>> Look for a subnode of type `multiboot,kernel` within a domain node. If
>> found, locate it using the multiboot module helper to generically ensure
>> it lives in the module list. If the bootargs property is present and
>> there was not an MB1 string, then use the command line from the device
>> tree definition.
>> 
>> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
>> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
>> ---
>> v4:
>>   * Stop printing on the fallback path of builder_init().
>>     It's in fact the most common path and just adds noise.
>>   * Add missing XENLOG_X.
>>   * Simplified check to log error on nr_domains != 1.
>>   * s/XENLOG_ERR/XENLOG_WARNING/ on duplicate kernel.
>>   * Turned foo == 0 into !foo in the "multiboot,kernel" check
>> ---
>>  xen/arch/x86/setup.c             |  5 ---
>>  xen/common/domain-builder/core.c |  9 +++++
>>  xen/common/domain-builder/fdt.c  | 64 ++++++++++++++++++++++++++++++--
>>  xen/include/xen/domain-builder.h |  3 --
>>  4 files changed, 70 insertions(+), 11 deletions(-)
>> 
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index ccc57cc70a..4f669f3c60 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1285,11 +1285,6 @@ void asmlinkage __init noreturn __start_xen(void)
>> 
>>      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];
>> -
>>      if ( pvh_boot )
>>      {
>>          /* pvh_init() already filled in e820_raw */
>> diff --git a/xen/common/domain-builder/core.c 
>> b/xen/common/domain-builder/core.c
>> index 3b062e85ec..924cb495a3 100644
>> --- a/xen/common/domain-builder/core.c
>> +++ b/xen/common/domain-builder/core.c
>> @@ -54,6 +54,15 @@ void __init builder_init(struct boot_info *bi)
>> 
>>          printk(XENLOG_INFO "  number of domains: %u\n", bi->nr_domains);
>>      }
>> +    else
>> +    {
>> +        /* Find first unknown boot module to use as dom0 kernel */
>> +        unsigned int i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
>> +
>> +        bi->mods[i].type = BOOTMOD_KERNEL;
>> +        bi->domains[0].kernel = &bi->mods[i];
>> +        bi->nr_domains = 1;
>> +    }
>>  }
>> 
>>  /*
>> diff --git a/xen/common/domain-builder/fdt.c 
>> b/xen/common/domain-builder/fdt.c
>> index d73536fed6..1fae6add3b 100644
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/domain-builder/fdt.c
>> @@ -89,9 +89,9 @@ static int __init read_fdt_prop_as_reg(const struct 
>> fdt_property *prop,
>>   * @return              -EINVAL on malformed nodes, otherwise
>>   *                      index inside `bi->mods`
>>   */
>> -int __init fdt_read_multiboot_module(const void *fdt, int node,
>> -                                     int address_cells, int size_cells,
>> -                                     struct boot_info *bi)
>> +static int __init fdt_read_multiboot_module(const void *fdt, int node,
>> +                                            int address_cells, int 
>> size_cells,
>> +                                            struct boot_info *bi)
>>  {
>>      const struct fdt_property *prop;
>>      uint64_t addr, size;
>> @@ -175,6 +175,52 @@ int __init fdt_read_multiboot_module(const void *fdt, 
>> int node,
>>      return idx;
>>  }
>> 
>> +static int __init process_domain_node(
>> +    struct boot_info *bi, const void *fdt, int dom_node)
>> +{
>> +    int node;
>> +    struct boot_domain *bd = &bi->domains[bi->nr_domains];
>> +    const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown";
>> +    int address_cells = fdt_address_cells(fdt, dom_node);
>> +    int size_cells = fdt_size_cells(fdt, dom_node);
>> +
>> +    fdt_for_each_subnode(node, fdt, dom_node)
>> +    {
>> +        if ( !fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
>
> Suggest to restructure the code to reduce levels of indentation, e.g.:
>
>            int idx;
>
>            if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
>                continue;
>
>            if ( bd->kernel )
>               ...
>

This hunk checking for "multiboot,kernel" is part of an if-elseif in a
later patch that also checks for "multiboot,ramdisk", so we can't just
do an early continue here without forcing a bigger diff later on.

>
>> +        {
>> +            int idx;
>> +
>> +            if ( bd->kernel )
>> +            {
>> +                printk(XENLOG_WARNING
>> +                       "  duplicate kernel for domain %s\n", name);
>> +                continue;
>> +            }
>> +
>> +            idx = fdt_read_multiboot_module(fdt, node, address_cells,
>> +                                            size_cells, bi);
>> +            if ( idx < 0 )
>> +            {
>> +                printk(XENLOG_ERR
>> +                       "  failed processing kernel for domain %s\n", name);
>> +                return idx;
>> +            }
>> +
>> +            printk(XENLOG_INFO "  kernel: multiboot-index=%d\n", idx);
>> +            bi->mods[idx].type = BOOTMOD_KERNEL;
>> +            bd->kernel = &bi->mods[idx];
>> +        }
>> +    }
>> +
>> +    if ( !bd->kernel )
>> +    {
>> +        printk(XENLOG_ERR "error: no kernel assigned to domain\n");
>
> Add domain name printout similar to above logging?

Good point.

Cheers,
Alejandro