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

[dmkhn@proton.me]

On Thu, Apr 17, 2025 at 01:48:28PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> 
> 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@apertussolutions.com>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> 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 )
              ...


> +        {
> +            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?

> +        return -ENODATA;
> +    }
> +
> +    return 0;
> +}
> +
>  static int __init find_hyperlaunch_node(const void *fdt)
>  {
>      int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
> @@ -237,8 +283,20 @@ int __init walk_hyperlaunch_fdt(struct boot_info *bi)
> 
>      fdt_for_each_subnode(node, fdt, hv_node)
>      {
> +        if ( bi->nr_domains >= MAX_NR_BOOTDOMS )
> +        {
> +            printk(XENLOG_WARNING "warning: only creating first %u domains\n",
> +                   MAX_NR_BOOTDOMS);
> +            break;
> +        }
> +
>          if ( !fdt_node_check_compatible(fdt, node, "xen,domain") )
> +        {
> +            if ( (ret = process_domain_node(bi, fdt, node)) < 0 )
> +                break;
> +
>              bi->nr_domains++;
> +        }
>      }
> 
>      /* Until multi-domain construction is added, throw an error */
> diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
> index ace6b6875b..ac2b84775d 100644
> --- a/xen/include/xen/domain-builder.h
> +++ b/xen/include/xen/domain-builder.h
> @@ -5,8 +5,5 @@
>  struct boot_info;
> 
>  void builder_init(struct boot_info *bi);
> -int fdt_read_multiboot_module(const void *fdt, int node,
> -                              int address_cells, int size_cells,
> -                              struct boot_info *bi)
> 
>  #endif /* __XEN_DOMAIN_BUILDER_H__ */
> --
> 2.43.0
> 
>