Re: [PATCH v2 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-12-26 11:57, Daniel P. Smith wrote:
> Add the ability to detect both a formal hyperlaunch device tree or a dom0less
> device tree. If the hyperlaunch device tree is found, then count the number of
> domain entries, reporting an error if more than one is found.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> diff --git a/xen/arch/x86/domain-builder/fdt.c 
> b/xen/arch/x86/domain-builder/fdt.c
> index 4a3f80648f86..5793bdc9fd47 100644
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -13,14 +13,77 @@
>   
>   #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;
> +    }
> +    else
> +    {
> +        /* Lood for dom0less config */
Look

> +        int node, chosen_node = fdt_path_offset(fdt, "/chosen");
> +        if ( chosen_node < 0 )
> +            return -ENOENT;
> +
> +        fdt_for_each_subnode(node, fdt, chosen_node)
> +        {
> +            if ( fdt_node_check_compatible(fdt, node, "xen,domain") == 0 )
> +                return chosen_node;
> +        }
> +    }
> +
> +    return -ENODATA;
> +}
> +
>   int __init has_hyperlaunch_fdt(struct boot_info *bi)
>   {
>       int ret = 0;
>       const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
>   
> -    if ( fdt_check_header(fdt) < 0 )
> +    if ( !fdt || fdt_check_header(fdt) < 0 )
It seems to me the !fdt check should move into the earlier patch.  What 
do you think?
>           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;
> +    void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +
> +    if ( unlikely(!fdt) )
> +        return -EINVAL;
> +
> +    hv_node = find_hyperlaunch_node(fdt);
You call find_hyperlaunch_node() twice.  It seems like you can just have 
has_hyperlaunch_fdt() return the node and pass it into this function.
Regards,
Jason

> +    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 )
> +        printk(XENLOG_ERR "Hyperlaunch only supports dom0 construction\n");
>   
> + err_out:
>       bootstrap_unmap();
>   
>       return ret;