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

[Jason Andryuk]

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 <dpsm...@apertussolutions.com>

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;