Re: [PATCH v3 09/16] x86/hyperlaunch: locate dom0 kernel with hyperlaunch

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Thu Apr 10, 2025 at 11:58 AM BST, Jan Beulich wrote:
> On 08.04.2025 18:07, 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>
>> ---
>> v3:
>>     * Add const to fdt
>>     * Remove idx == NULL checks
>>     * Add BUILD_BUG_ON for MAX_NR_BOOTMODS fitting in a uint32_t
>
> At least this one looks to rather belong into patch 08?

Urg, yes. Sorry. There was a lot of code motion when I factored out the
helpers.

>
>> --- a/xen/arch/x86/domain-builder/fdt.c
>> +++ b/xen/arch/x86/domain-builder/fdt.c
>> @@ -155,6 +155,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);
>
> Oh, okay - regarding my earlier comment elsewhere: If the sizes come from DT,
> then of course ASSERT_UNREACHABLE() can't be used at the place where bogus
> ones are rejected.
>
>> +    fdt_for_each_subnode(node, fdt, dom_node)
>> +    {
>> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
>> +        {
>
> When the loop body is merely an if() with a non-negligible body, inverting
> the condition and using "continue" is usually better. Much like you do ...

This becomes a chain of if conditions later on, one per property.

>
>> +            int idx;
>> +
>> +            if ( bd->kernel )
>> +            {
>> +                printk(XENLOG_ERR "Duplicate kernel module for domain %s\n",
>> +                       name);
>> +                continue;
>
> ... here already.
>
> Jan

Cheers,
Alejandro