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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

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

... here already.

Jan