Re: [PATCH v3 07/16] x86/hyperlaunch: initial support for hyperlaunch device tree

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2025 18:07, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/domain-builder/core.c
> +++ b/xen/arch/x86/domain-builder/core.c
> @@ -44,6 +44,21 @@ void __init builder_init(struct boot_info *bi)
>              break;
>          }
>      }
> +
> +    if ( bi->hyperlaunch_enabled )
> +    {

Not knowing what else if going to appear here and in what shape, could the
if() here be avoided by making case blocks in the earlier switch setting
the field to false (which is kind of redundant anyway with it starting out
false) use "return" instead of "break"? The the setting of the field to
true could also be centralized below the switch().

> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -13,6 +13,36 @@
>  
>  #include "fdt.h"
>  
> +static int __init find_hyperlaunch_node(const void *fdt)
> +{
> +    int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
> +
> +    if ( hv_node >= 0 )
> +    {
> +        /* Anything other than zero indicates no match */
> +        if ( fdt_node_check_compatible(fdt, hv_node, "hypervisor,xen") )
> +            return -ENODATA;
> +        else
> +            return hv_node;

Could I talk you into omitting such unnecessary "else"?

> @@ -20,7 +50,41 @@ int __init has_hyperlaunch_fdt(const struct boot_info *bi)
>  
>      if ( !fdt || fdt_check_header(fdt) < 0 )
>          ret = -EINVAL;
> +    else
> +        ret = find_hyperlaunch_node(fdt);
> +
> +    bootstrap_unmap();
> +
> +    return ret < 0 ? ret : 0;
> +}
> +
> +int __init walk_hyperlaunch_fdt(struct boot_info *bi)
> +{
> +    int ret = 0, hv_node, node;
> +    const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +
> +    if ( unlikely(!fdt) )
> +        return -EINVAL;
> +
> +    hv_node = find_hyperlaunch_node(fdt);
> +    if ( hv_node < 0 )
> +    {
> +        ret = hv_node;
> +        goto err_out;
> +    }
> +
> +    fdt_for_each_subnode(node, fdt, hv_node)
> +    {
> +        ret = fdt_node_check_compatible(fdt, node, "xen,domain");
> +        if ( ret == 0 )
> +            bi->nr_domains++;
> +    }
> +
> +    /* Until multi-domain construction is added, throw an error */
> +    if ( !bi->nr_domains || bi->nr_domains > 1 )

Simply "!= 1"?

> --- a/xen/arch/x86/domain-builder/fdt.h
> +++ b/xen/arch/x86/domain-builder/fdt.h
> @@ -11,11 +11,16 @@ struct boot_info;
>  
>  #ifdef CONFIG_DOMAIN_BUILDER
>  int has_hyperlaunch_fdt(const struct boot_info *bi);
> +int walk_hyperlaunch_fdt(struct boot_info *bi);
>  #else
>  static inline int __init has_hyperlaunch_fdt(const struct boot_info *bi)
>  {
>      return -EINVAL;
>  }
> +static inline int __init walk_hyperlaunch_fdt(struct boot_info *bi)
> +{
> +    return -EINVAL;
> +}

There's no need for this stub (nor the has_hyperlaunch_fdt() one, as I
notice only now) - even with present arrangements calling code is guarded
such that DCE will take care of eliminating the call, and hence having a
declaration suffices.

Jan