Xen devel - Jan 2013 - [PATCH 0/4 V6] xen: arm: parse modules from DT during early boot.

Most of the early part of this series went in with V5 and only the PoC
DTB based "protocol" remains.

Main change is that the match on /chosen/modules is now done based on
compatible == "xen,multiboot-module" rather than by the path
(Stefano''s
request).

I had to fix device_tree_for_each_node to handle #*-cells properly, this
is at the start of the series.

I have also added a patch which makes the hypervisor insert
the /hypervisor/ node automatically (and drop anything which is there
already). This is a (small) step along the path of being able to use the
unmodified DTS for the platform. I reckon the main remaining thing in
this area is to automatically update /cpus/ based on dom0_vcpus=X (we
already handle RAM).

Ian.

If a node does not have #*-cells then the parent''s value should be
used. Currently we were asssuming zero which is useless.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain_build.c   |    6 ++++--
 xen/common/device_tree.c      |   12 ++++++++----
 xen/include/xen/device_tree.h |    3 ++-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 7403f1a..bfbe7c7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -198,8 +198,10 @@ static int write_nodes(struct domain *d, struct kernel_info
*kinfo,
         while ( last_depth-- >= depth )
             fdt_end_node(kinfo->fdt);
 
-        address_cells[depth] = device_tree_get_u32(fdt, node,
"#address-cells");
-        size_cells[depth] = device_tree_get_u32(fdt, node,
"#size-cells");
+        address_cells[depth] = device_tree_get_u32(fdt, node,
"#address-cells",
+                                    depth > 0 ? address_cells[depth-1] : 0);
+        size_cells[depth] = device_tree_get_u32(fdt, node,
"#size-cells",
+                                    depth > 0 ? size_cells[depth-1] : 0);
 
         fdt_begin_node(kinfo->fdt, name);
 
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 260c2d4..f10ba1b 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -120,13 +120,14 @@ void device_tree_set_reg(u32 **cell, u32 address_cells,
u32 size_cells,
     set_val(cell, size_cells, size);
 }
 
-u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name)
+u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name,
+                        u32 dflt)
 {
     const struct fdt_property *prop;
 
     prop = fdt_get_property(fdt, node, prop_name, NULL);
     if ( !prop || prop->len < sizeof(u32) )
-        return 0; /* default to 0 */
+        return dflt;
 
     return fdt32_to_cpu(*(uint32_t*)prop->data);
 }
@@ -164,8 +165,11 @@ int device_tree_for_each_node(const void *fdt,
             continue;
         }
 
-        address_cells[depth] = device_tree_get_u32(fdt, node,
"#address-cells");
-        size_cells[depth] = device_tree_get_u32(fdt, node,
"#size-cells");
+        address_cells[depth] = device_tree_get_u32(fdt, node,
"#address-cells",
+                                depth > 0 ? address_cells[depth-1] : 0);
+        size_cells[depth] = device_tree_get_u32(fdt, node,
"#size-cells",
+                                depth > 0 ? size_cells[depth-1] : 0);
+
 
         ret = func(fdt, node, name, depth,
                    address_cells[depth-1], size_cells[depth-1], data);
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 52ef258..a8df493 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -66,7 +66,8 @@ void device_tree_get_reg(const u32 **cell, u32 address_cells,
u32 size_cells,
                          u64 *start, u64 *size);
 void device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
                          u64 start, u64 size);
-u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name);
+u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name,
+			u32 dflt);
 bool_t device_tree_node_matches(const void *fdt, int node, const char *match);
 int device_tree_for_each_node(const void *fdt,
                               device_tree_node_func func, void *data);
-- 
1.7.9.1

The bootloader should populate /chosen/modules/module@<N>/ for each
module it wishes to pass to the hypervisor. The content of these nodes
is described in docs/misc/arm/device-tree/booting.txt

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v6: Match based on compatibility instead of path
v5: Moved later in the series
v4: Use /chosen/modules/module@N
    Identify module type by compatible property not number.
v3: Use a reg = < > property for the module address/length.
v2: Reserve the zeroeth module for Xen itself (not used yet)
    Use a more idiomatic DT layout
    Document said layout
---
 docs/misc/arm/device-tree/booting.txt |   25 +++++++++++++++
 xen/common/device_tree.c              |   54 ++++++++++++++++++++++++++++++++-
 2 files changed, 78 insertions(+), 1 deletions(-)
 create mode 100644 docs/misc/arm/device-tree/booting.txt

diff --git a/docs/misc/arm/device-tree/booting.txt
b/docs/misc/arm/device-tree/booting.txt
new file mode 100644
index 0000000..94cd3f1
--- /dev/null
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -0,0 +1,25 @@
+Xen is passed the dom0 kernel and initrd via a reference in the /chosen
+node of the device tree.
+
+Each node has the form /chosen/modules/module@<N> and contains the
following
+properties:
+
+- compatible
+
+	Must be:
+
+		"xen,<type>", "xen,multiboot-module"
+
+	where <type> must be one of:
+
+	- "linux-zimage" -- the dom0 kernel
+	- "linux-initrd" -- the dom0 ramdisk
+
+- reg
+
+	Specifies the physical address of the module in RAM and the
+	length of the module.
+
+- bootargs (optional)
+
+	Command line associated with this module
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index f10ba1b..c92f8ca 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -341,6 +341,48 @@ static void __init process_gic_node(const void *fdt, int
node,
     early_info.gic.gic_vcpu_addr = start;
 }
 
+static void __init process_multiboot_node(const void *fdt, int node,
+                                          const char *name,
+                                          u32 address_cells, u32 size_cells)
+{
+    const struct fdt_property *prop;
+    const u32 *cell;
+    int nr;
+    struct dt_mb_module *mod;
+    int len;
+
+    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") ==
0 )
+        nr = 1;
+    else if ( fdt_node_check_compatible(fdt, node,
"xen,linux-initrd") == 0)
+        nr = 2;
+    else
+        early_panic("%s not a known xen multiboot type\n", name);
+
+    mod = &early_info.modules.module[nr];
+
+    prop = fdt_get_property(fdt, node, "reg", NULL);
+    if ( !prop )
+        early_panic("node %s missing `reg'' property\n",
name);
+
+    cell = (const u32 *)prop->data;
+    device_tree_get_reg(&cell, address_cells, size_cells,
+                        &mod->start, &mod->size);
+
+    prop = fdt_get_property(fdt, node, "bootargs", &len);
+    if ( prop )
+    {
+        if ( len > sizeof(mod->cmdline) )
+            early_panic("module %d command line too long\n", nr);
+
+        safe_strcpy(mod->cmdline, prop->data);
+    }
+    else
+        mod->cmdline[0] = 0;
+
+    if ( nr > early_info.modules.nr_mods )
+        early_info.modules.nr_mods = nr;
+}
+
 static int __init early_scan_node(const void *fdt,
                                   int node, const char *name, int depth,
                                   u32 address_cells, u32 size_cells,
@@ -352,6 +394,8 @@ static int __init early_scan_node(const void *fdt,
         process_cpu_node(fdt, node, name, address_cells, size_cells);
     else if ( device_tree_node_compatible(fdt, node,
"arm,cortex-a15-gic") )
         process_gic_node(fdt, node, name, address_cells, size_cells);
+    else if ( device_tree_node_compatible(fdt, node,
"xen,multiboot-module" ) )
+        process_multiboot_node(fdt, node, name, address_cells, size_cells);
 
     return 0;
 }
@@ -359,12 +403,20 @@ static int __init early_scan_node(const void *fdt,
 static void __init early_print_info(void)
 {
     struct dt_mem_info *mi = &early_info.mem;
+    struct dt_module_info *mods = &early_info.modules;
     int i;
 
     for ( i = 0; i < mi->nr_banks; i++ )
-        early_printk("RAM: %016llx - %016llx\n",
+        early_printk("RAM: %"PRIpaddr" -
%"PRIpaddr"\n",
                      mi->bank[i].start,
                      mi->bank[i].start + mi->bank[i].size - 1);
+    early_printk("\n");
+    for ( i = 1 ; i < mods->nr_mods + 1; i++ )
+        early_printk("MODULE[%d]: %"PRIpaddr" -
%"PRIpaddr" %s\n",
+                     i,
+                     mods->module[i].start,
+                     mods->module[i].start + mods->module[i].size,
+                     mods->module[i].cmdline);
 }
 
 /**
-- 
1.7.9.1

These nodes are used by Xen to find the initial modules.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v6 - filter based on compatibility node name and not path.
v4 - /chosen/modules/modules@N not /chosen/module@N
v3 - use a helper to filter out DT elements which are not for dom0.
     Better than an ad-hoc break in the middle of a loop.
---
 xen/arch/arm/domain_build.c |   32 ++++++++++++++++++++++++++++++--
 1 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bfbe7c7..bb10096 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -172,6 +172,33 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
     return prop;
 }
 
+/* Returns the next node in fdt (starting from offset) which should be
+ * passed through to dom0.
+ */
+static int fdt_next_dom0_node(const void *fdt, int node,
+                              int *depth_out)
+{
+    int depth = *depth_out;
+
+    while ( (node = fdt_next_node(fdt, node, &depth)) &&
+            node >= 0 && depth >= 0 )
+    {
+        if ( depth >= DEVICE_TREE_MAX_DEPTH )
+            break;
+
+        /* Skip multiboot subnodes */
+        if ( fdt_node_check_compatible(fdt, node,
+                                       "xen,multiboot-module" ) == 0
)
+            continue;
+
+        /* We''ve arrived at a node which dom0 is interested in. */
+        break;
+    }
+
+    *depth_out = depth;
+    return node;
+}
+
 static int write_nodes(struct domain *d, struct kernel_info *kinfo,
                        const void *fdt)
 {
@@ -183,7 +210,7 @@ static int write_nodes(struct domain *d, struct kernel_info
*kinfo,
 
     for ( node = 0, depth = 0;
           node >= 0 && depth >= 0;
-          node = fdt_next_node(fdt, node, &depth) )
+          node = fdt_next_dom0_node(fdt, node, &depth) )
     {
         const char *name;
 
@@ -191,7 +218,8 @@ static int write_nodes(struct domain *d, struct kernel_info
*kinfo,
 
         if ( depth >= DEVICE_TREE_MAX_DEPTH )
         {
-            printk("warning: node `%s'' is nested too
deep\n", name);
+            printk("warning: node `%s'' is nested too deep
(%d)\n",
+                   name, depth);
             continue;
         }
 
-- 
1.7.9.1

I initially added hypervisor-new and confirmed via /proc/device-model
that the content is the same before changing it to drop and replace
an existing node.

NB: There is an ambiguity in the compatibility property.
linux/arch/arm/boot/dts/xenvm-4.2.dts says "xen,xen-4.2" while
Documentation/devicetree/bindings/arm/xen.txt says "xen,xen-4.3". I
don''t know which is correct but I''ve erred on the side of the
DTS.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain_build.c |   53 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bb10096..d3ef180 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -34,7 +34,7 @@ custom_param("dom0_mem", parse_dom0_mem);
  * are added (yet) but one terminating reserve map entry (16 bytes) is
  * added.
  */
-#define DOM0_FDT_EXTRA_SIZE (sizeof(struct fdt_reserve_entry))
+#define DOM0_FDT_EXTRA_SIZE (128 + sizeof(struct fdt_reserve_entry))
 
 struct vcpu *__init alloc_dom0_vcpu0(void)
 {
@@ -186,6 +186,13 @@ static int fdt_next_dom0_node(const void *fdt, int node,
         if ( depth >= DEVICE_TREE_MAX_DEPTH )
             break;
 
+        /* Skip /hypervisor/ node. We will inject our own. */
+        if ( fdt_node_check_compatible(fdt, node, "xen,xen" ) == 0 )
+        {
+            printk("Device-tree contains \"xen,xen\" node.
Ignoring.\n");
+            continue;
+        }
+
         /* Skip multiboot subnodes */
         if ( fdt_node_check_compatible(fdt, node,
                                        "xen,multiboot-module" ) == 0
)
@@ -199,6 +206,45 @@ static int fdt_next_dom0_node(const void *fdt, int node,
     return node;
 }
 
+static void make_hypervisor_node(void *fdt, int addrcells, int sizecells)
+{
+    const char compat[] = "xen,xen-4.2\0xen,xen";
+    u32 reg[4];
+    u32 intr[3];
+    u32 *cell;
+
+    /*
+     * Sanity-check address sizes, since addresses and sizes which do not take
+     * up exactly 4 or 8 bytes are not supported.
+     */
+    if ((addrcells != 1 && addrcells != 2) ||
+        (sizecells != 1 && sizecells != 2))
+        panic("Cannot cope with this size");
+
+    /* See linux Documentation/devicetree/bindings/arm/xen.txt */
+    fdt_begin_node(fdt, "hypervisor");
+
+    fdt_property(fdt, "compatible", compat, sizeof(compat) + 1);
+
+    /* reg 0 is grant table space */
+    cell = &reg[0];
+    device_tree_set_reg(&cell, addrcells, sizecells, 0xb0000000, 0x20000);
+    fdt_property(fdt, "reg", reg,
+                 sizeof(reg[0]) * (addrcells + sizecells));
+
+    /*
+     * interrupts is evtchn upcall  <1 15 0xf08>
+     * See linux Documentation/devicetree/bindings/arm/gic.txt
+     */
+    intr[0] = cpu_to_fdt32(1); /* is a PPI */
+    intr[1] = cpu_to_fdt32(VGIC_IRQ_EVTCHN_CALLBACK - 16); /* PPIs start at 16
*/
+    intr[2] = cpu_to_fdt32(0xf08); /* Active-low level-sensitive */
+
+    fdt_property(fdt, "interrupts", intr, sizeof(intr[0]) * 3);
+
+    fdt_end_node(fdt);
+}
+
 static int write_nodes(struct domain *d, struct kernel_info *kinfo,
                        const void *fdt)
 {
@@ -241,9 +287,12 @@ static int write_nodes(struct domain *d, struct kernel_info
*kinfo,
         last_depth = depth;
     }
 
-    while ( last_depth-- >= 0 )
+    while ( last_depth-- >= 1 )
         fdt_end_node(kinfo->fdt);
 
+    make_hypervisor_node(kinfo->fdt, address_cells[0], size_cells[0]);
+
+    fdt_end_node(kinfo->fdt);
     return 0;
 }
 
-- 
1.7.9.1

On 30/01/13 14:26, Ian Campbell wrote:
> I initially added hypervisor-new and confirmed via /proc/device-model
> that the content is the same before changing it to drop and replace
> an existing node.
> 
> NB: There is an ambiguity in the compatibility property.
> linux/arch/arm/boot/dts/xenvm-4.2.dts says "xen,xen-4.2" while
> Documentation/devicetree/bindings/arm/xen.txt says "xen,xen-4.3".
I
> don''t know which is correct but I''ve erred on the side of
the DTS.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/domain_build.c |   53
+++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index bb10096..d3ef180 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -34,7 +34,7 @@ custom_param("dom0_mem", parse_dom0_mem);
>   * are added (yet) but one terminating reserve map entry (16 bytes) is
>   * added.
>   */
> -#define DOM0_FDT_EXTRA_SIZE (sizeof(struct fdt_reserve_entry))
> +#define DOM0_FDT_EXTRA_SIZE (128 + sizeof(struct fdt_reserve_entry))
>  
>  struct vcpu *__init alloc_dom0_vcpu0(void)
>  {
> @@ -186,6 +186,13 @@ static int fdt_next_dom0_node(const void *fdt, int
node,
>          if ( depth >= DEVICE_TREE_MAX_DEPTH )
>              break;
>  
> +        /* Skip /hypervisor/ node. We will inject our own. */
> +        if ( fdt_node_check_compatible(fdt, node, "xen,xen" ) ==
0 )
> +        {
> +            printk("Device-tree contains \"xen,xen\" node.
Ignoring.\n");
> +            continue;
> +        }
> +
>          /* Skip multiboot subnodes */
>          if ( fdt_node_check_compatible(fdt, node,
>                                         "xen,multiboot-module" )
== 0 )
> @@ -199,6 +206,45 @@ static int fdt_next_dom0_node(const void *fdt, int
node,
>      return node;
>  }
>  
> +static void make_hypervisor_node(void *fdt, int addrcells, int sizecells)
> +{
> +    const char compat[] = "xen,xen-4.2\0xen,xen";
> +    u32 reg[4];
> +    u32 intr[3];
> +    u32 *cell;
> +
> +    /*
> +     * Sanity-check address sizes, since addresses and sizes which do not
take
> +     * up exactly 4 or 8 bytes are not supported.
> +     */
> +    if ((addrcells != 1 && addrcells != 2) ||
> +        (sizecells != 1 && sizecells != 2))
> +        panic("Cannot cope with this size");
You could add those two properties in the hypervisor node if they don''t
match what we expect them to be. This would avoid panicking with a
device tree that have different default values.

-- 
Anthony PERARD

On Tue, 2013-02-05 at 11:26 +0000, Anthony PERARD wrote:
> > +static void make_hypervisor_node(void *fdt, int addrcells, int
sizecells)
> > +{
> > +    const char compat[] = "xen,xen-4.2\0xen,xen";
> > +    u32 reg[4];
> > +    u32 intr[3];
> > +    u32 *cell;
> > +
> > +    /*
> > +     * Sanity-check address sizes, since addresses and sizes which do
not take
> > +     * up exactly 4 or 8 bytes are not supported.
> > +     */
> > +    if ((addrcells != 1 && addrcells != 2) ||
> > +        (sizecells != 1 && sizecells != 2))
> > +        panic("Cannot cope with this size");
> 
> You could add those two properties in the hypervisor node if they
don''t
> match what we expect them to be. This would avoid panicking with a
> device tree that have different default values.
Yes, I suppose that ought to work.

Ian.

On Wed, 30 Jan 2013, Ian Campbell wrote:
> If a node does not have #*-cells then the parent''s value should be
> used. Currently we were asssuming zero which is useless.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/domain_build.c   |    6 ++++--
>  xen/common/device_tree.c      |   12 ++++++++----
>  xen/include/xen/device_tree.h |    3 ++-
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7403f1a..bfbe7c7 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -198,8 +198,10 @@ static int write_nodes(struct domain *d, struct
kernel_info *kinfo,
>          while ( last_depth-- >= depth )
>              fdt_end_node(kinfo->fdt);
>  
> -        address_cells[depth] = device_tree_get_u32(fdt, node,
"#address-cells");
> -        size_cells[depth] = device_tree_get_u32(fdt, node,
"#size-cells");
> +        address_cells[depth] = device_tree_get_u32(fdt, node,
"#address-cells",
> +                                    depth > 0 ? address_cells[depth-1]
: 0);
> +        size_cells[depth] = device_tree_get_u32(fdt, node,
"#size-cells",
> +                                    depth > 0 ? size_cells[depth-1] :
0);
>  
>          fdt_begin_node(kinfo->fdt, name);
The depth is always increasing by steps of 1 in this loop, right?
Because retrieving address-cells and size-cells should be recursive: if
n-1 doesn''t have them, let''s look at n-2, etc. Of course if we
start from
depth = 0 and go from there without missing any levels the results will
be the same.

I think I convinced myself that this is correct.

> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 260c2d4..f10ba1b 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -120,13 +120,14 @@ void device_tree_set_reg(u32 **cell, u32
address_cells, u32 size_cells,
>      set_val(cell, size_cells, size);
>  }
>  
> -u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name)
> +u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name,
> +                        u32 dflt)
>  {
>      const struct fdt_property *prop;
>  
>      prop = fdt_get_property(fdt, node, prop_name, NULL);
>      if ( !prop || prop->len < sizeof(u32) )
> -        return 0; /* default to 0 */
> +        return dflt;
>  
>      return fdt32_to_cpu(*(uint32_t*)prop->data);
>  }
where did the vowels go? :)

On Wed, 30 Jan 2013, Ian Campbell wrote:
> I initially added hypervisor-new and confirmed via /proc/device-model
> that the content is the same before changing it to drop and replace
> an existing node.
> 
> NB: There is an ambiguity in the compatibility property.
> linux/arch/arm/boot/dts/xenvm-4.2.dts says "xen,xen-4.2" while
> Documentation/devicetree/bindings/arm/xen.txt says "xen,xen-4.3".
I
> don''t know which is correct but I''ve erred on the side of
the DTS.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>  xen/arch/arm/domain_build.c |   53
+++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index bb10096..d3ef180 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -34,7 +34,7 @@ custom_param("dom0_mem", parse_dom0_mem);
>   * are added (yet) but one terminating reserve map entry (16 bytes) is
>   * added.
>   */
> -#define DOM0_FDT_EXTRA_SIZE (sizeof(struct fdt_reserve_entry))
> +#define DOM0_FDT_EXTRA_SIZE (128 + sizeof(struct fdt_reserve_entry))
>  
>  struct vcpu *__init alloc_dom0_vcpu0(void)
>  {
> @@ -186,6 +186,13 @@ static int fdt_next_dom0_node(const void *fdt, int
node,
>          if ( depth >= DEVICE_TREE_MAX_DEPTH )
>              break;
>  
> +        /* Skip /hypervisor/ node. We will inject our own. */
> +        if ( fdt_node_check_compatible(fdt, node, "xen,xen" ) ==
0 )
> +        {
> +            printk("Device-tree contains \"xen,xen\" node.
Ignoring.\n");
> +            continue;
> +        }
> +
>          /* Skip multiboot subnodes */
>          if ( fdt_node_check_compatible(fdt, node,
>                                         "xen,multiboot-module" )
== 0 )
> @@ -199,6 +206,45 @@ static int fdt_next_dom0_node(const void *fdt, int
node,
>      return node;
>  }
>  
> +static void make_hypervisor_node(void *fdt, int addrcells, int sizecells)
> +{
> +    const char compat[] = "xen,xen-4.2\0xen,xen";
> +    u32 reg[4];
> +    u32 intr[3];
> +    u32 *cell;
> +
> +    /*
> +     * Sanity-check address sizes, since addresses and sizes which do not
take
> +     * up exactly 4 or 8 bytes are not supported.
> +     */
> +    if ((addrcells != 1 && addrcells != 2) ||
> +        (sizecells != 1 && sizecells != 2))
> +        panic("Cannot cope with this size");
> +
> +    /* See linux Documentation/devicetree/bindings/arm/xen.txt */
> +    fdt_begin_node(fdt, "hypervisor");
> +
> +    fdt_property(fdt, "compatible", compat, sizeof(compat) + 1);
> +
> +    /* reg 0 is grant table space */
> +    cell = &reg[0];
> +    device_tree_set_reg(&cell, addrcells, sizecells, 0xb0000000,
0x20000);
> +    fdt_property(fdt, "reg", reg,
> +                 sizeof(reg[0]) * (addrcells + sizecells));
> +
> +    /*
> +     * interrupts is evtchn upcall  <1 15 0xf08>
> +     * See linux Documentation/devicetree/bindings/arm/gic.txt
> +     */
> +    intr[0] = cpu_to_fdt32(1); /* is a PPI */
> +    intr[1] = cpu_to_fdt32(VGIC_IRQ_EVTCHN_CALLBACK - 16); /* PPIs start
at 16 */
> +    intr[2] = cpu_to_fdt32(0xf08); /* Active-low level-sensitive */
> +
> +    fdt_property(fdt, "interrupts", intr, sizeof(intr[0]) * 3);
> +
> +    fdt_end_node(fdt);
> +}
> +
>  static int write_nodes(struct domain *d, struct kernel_info *kinfo,
>                         const void *fdt)
>  {
> @@ -241,9 +287,12 @@ static int write_nodes(struct domain *d, struct
kernel_info *kinfo,
>          last_depth = depth;
>      }
>  
> -    while ( last_depth-- >= 0 )
> +    while ( last_depth-- >= 1 )
>          fdt_end_node(kinfo->fdt);
>  
> +    make_hypervisor_node(kinfo->fdt, address_cells[0], size_cells[0]);
> +
> +    fdt_end_node(kinfo->fdt);
>      return 0;
>  }
>  
> -- 
> 1.7.9.1
>

On Wed, 30 Jan 2013, Ian Campbell wrote:
> These nodes are used by Xen to find the initial modules.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

> v6 - filter based on compatibility node name and not path.
> v4 - /chosen/modules/modules@N not /chosen/module@N
> v3 - use a helper to filter out DT elements which are not for dom0.
>      Better than an ad-hoc break in the middle of a loop.
> ---
>  xen/arch/arm/domain_build.c |   32 ++++++++++++++++++++++++++++++--
>  1 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index bfbe7c7..bb10096 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -172,6 +172,33 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
>      return prop;
>  }
>  
> +/* Returns the next node in fdt (starting from offset) which should be
> + * passed through to dom0.
> + */
> +static int fdt_next_dom0_node(const void *fdt, int node,
> +                              int *depth_out)
> +{
> +    int depth = *depth_out;
> +
> +    while ( (node = fdt_next_node(fdt, node, &depth)) &&
> +            node >= 0 && depth >= 0 )
> +    {
> +        if ( depth >= DEVICE_TREE_MAX_DEPTH )
> +            break;
> +
> +        /* Skip multiboot subnodes */
> +        if ( fdt_node_check_compatible(fdt, node,
> +                                       "xen,multiboot-module" )
== 0 )
> +            continue;
> +
> +        /* We''ve arrived at a node which dom0 is interested in.
*/
> +        break;
> +    }
> +
> +    *depth_out = depth;
> +    return node;
> +}
> +
>  static int write_nodes(struct domain *d, struct kernel_info *kinfo,
>                         const void *fdt)
>  {
> @@ -183,7 +210,7 @@ static int write_nodes(struct domain *d, struct
kernel_info *kinfo,
>  
>      for ( node = 0, depth = 0;
>            node >= 0 && depth >= 0;
> -          node = fdt_next_node(fdt, node, &depth) )
> +          node = fdt_next_dom0_node(fdt, node, &depth) )
>      {
>          const char *name;
>  
> @@ -191,7 +218,8 @@ static int write_nodes(struct domain *d, struct
kernel_info *kinfo,
>  
>          if ( depth >= DEVICE_TREE_MAX_DEPTH )
>          {
> -            printk("warning: node `%s'' is nested too
deep\n", name);
> +            printk("warning: node `%s'' is nested too deep
(%d)\n",
> +                   name, depth);
>              continue;
>          }
>  
> -- 
> 1.7.9.1
>

On Wed, 30 Jan 2013, Ian Campbell wrote:
> The bootloader should populate /chosen/modules/module@<N>/ for each
> module it wishes to pass to the hypervisor. The content of these nodes
> is described in docs/misc/arm/device-tree/booting.txt
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v6: Match based on compatibility instead of path
> v5: Moved later in the series
> v4: Use /chosen/modules/module@N
>     Identify module type by compatible property not number.
> v3: Use a reg = < > property for the module address/length.
> v2: Reserve the zeroeth module for Xen itself (not used yet)
>     Use a more idiomatic DT layout
>     Document said layout
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  docs/misc/arm/device-tree/booting.txt |   25 +++++++++++++++
>  xen/common/device_tree.c              |   54
++++++++++++++++++++++++++++++++-
>  2 files changed, 78 insertions(+), 1 deletions(-)
>  create mode 100644 docs/misc/arm/device-tree/booting.txt
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt
b/docs/misc/arm/device-tree/booting.txt
> new file mode 100644
> index 0000000..94cd3f1
> --- /dev/null
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -0,0 +1,25 @@
> +Xen is passed the dom0 kernel and initrd via a reference in the /chosen
> +node of the device tree.
> +
> +Each node has the form /chosen/modules/module@<N> and contains the
following
> +properties:
> +
> +- compatible
> +
> +	Must be:
> +
> +		"xen,<type>", "xen,multiboot-module"
> +
> +	where <type> must be one of:
> +
> +	- "linux-zimage" -- the dom0 kernel
> +	- "linux-initrd" -- the dom0 ramdisk
> +
> +- reg
> +
> +	Specifies the physical address of the module in RAM and the
> +	length of the module.
> +
> +- bootargs (optional)
> +
> +	Command line associated with this module
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index f10ba1b..c92f8ca 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -341,6 +341,48 @@ static void __init process_gic_node(const void *fdt,
int node,
>      early_info.gic.gic_vcpu_addr = start;
>  }
>  
> +static void __init process_multiboot_node(const void *fdt, int node,
> +                                          const char *name,
> +                                          u32 address_cells, u32
size_cells)
> +{
> +    const struct fdt_property *prop;
> +    const u32 *cell;
> +    int nr;
> +    struct dt_mb_module *mod;
> +    int len;
> +
> +    if ( fdt_node_check_compatible(fdt, node,
"xen,linux-zimage") == 0 )
> +        nr = 1;
> +    else if ( fdt_node_check_compatible(fdt, node,
"xen,linux-initrd") == 0)
> +        nr = 2;
> +    else
> +        early_panic("%s not a known xen multiboot type\n",
name);
> +
> +    mod = &early_info.modules.module[nr];
> +
> +    prop = fdt_get_property(fdt, node, "reg", NULL);
> +    if ( !prop )
> +        early_panic("node %s missing `reg'' property\n",
name);
> +
> +    cell = (const u32 *)prop->data;
> +    device_tree_get_reg(&cell, address_cells, size_cells,
> +                        &mod->start, &mod->size);
> +
> +    prop = fdt_get_property(fdt, node, "bootargs", &len);
> +    if ( prop )
> +    {
> +        if ( len > sizeof(mod->cmdline) )
> +            early_panic("module %d command line too long\n",
nr);
> +
> +        safe_strcpy(mod->cmdline, prop->data);
> +    }
> +    else
> +        mod->cmdline[0] = 0;
> +
> +    if ( nr > early_info.modules.nr_mods )
> +        early_info.modules.nr_mods = nr;
> +}
> +
>  static int __init early_scan_node(const void *fdt,
>                                    int node, const char *name, int depth,
>                                    u32 address_cells, u32 size_cells,
> @@ -352,6 +394,8 @@ static int __init early_scan_node(const void *fdt,
>          process_cpu_node(fdt, node, name, address_cells, size_cells);
>      else if ( device_tree_node_compatible(fdt, node,
"arm,cortex-a15-gic") )
>          process_gic_node(fdt, node, name, address_cells, size_cells);
> +    else if ( device_tree_node_compatible(fdt, node,
"xen,multiboot-module" ) )
> +        process_multiboot_node(fdt, node, name, address_cells,
size_cells);
>  
>      return 0;
>  }
> @@ -359,12 +403,20 @@ static int __init early_scan_node(const void *fdt,
>  static void __init early_print_info(void)
>  {
>      struct dt_mem_info *mi = &early_info.mem;
> +    struct dt_module_info *mods = &early_info.modules;
>      int i;
>  
>      for ( i = 0; i < mi->nr_banks; i++ )
> -        early_printk("RAM: %016llx - %016llx\n",
> +        early_printk("RAM: %"PRIpaddr" -
%"PRIpaddr"\n",
>                       mi->bank[i].start,
>                       mi->bank[i].start + mi->bank[i].size - 1);
> +    early_printk("\n");
> +    for ( i = 1 ; i < mods->nr_mods + 1; i++ )
> +        early_printk("MODULE[%d]: %"PRIpaddr" -
%"PRIpaddr" %s\n",
> +                     i,
> +                     mods->module[i].start,
> +                     mods->module[i].start + mods->module[i].size,
> +                     mods->module[i].cmdline);
>  }
>  
>  /**
> -- 
> 1.7.9.1
>

On Fri, 2013-02-15 at 12:43 +0000, Stefano Stabellini
wrote:
> On Wed, 30 Jan 2013, Ian Campbell wrote:
> > If a node does not have #*-cells then the parent''s value
should be
> > used. Currently we were asssuming zero which is useless.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/domain_build.c   |    6 ++++--
> >  xen/common/device_tree.c      |   12 ++++++++----
> >  xen/include/xen/device_tree.h |    3 ++-
> >  3 files changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 7403f1a..bfbe7c7 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -198,8 +198,10 @@ static int write_nodes(struct domain *d, struct
kernel_info *kinfo,
> >          while ( last_depth-- >= depth )
> >              fdt_end_node(kinfo->fdt);
> >  
> > -        address_cells[depth] = device_tree_get_u32(fdt, node,
"#address-cells");
> > -        size_cells[depth] = device_tree_get_u32(fdt, node,
"#size-cells");
> > +        address_cells[depth] = device_tree_get_u32(fdt, node,
"#address-cells",
> > +                                    depth > 0 ?
address_cells[depth-1] : 0);
> > +        size_cells[depth] = device_tree_get_u32(fdt, node,
"#size-cells",
> > +                                    depth > 0 ?
size_cells[depth-1] : 0);
> >  
> >          fdt_begin_node(kinfo->fdt, name);
> 
> The depth is always increasing by steps of 1 in this loop, right?
> Because retrieving address-cells and size-cells should be recursive: if
> n-1 doesn''t have them, let''s look at n-2, etc. Of course
if we start from
> depth = 0 and go from there without missing any levels the results will
> be the same.
That was what I thought too. Perhaps it is too subtle?

I bet my "xen: strip xen,multiboot-module nodes from dom0 device tree"
patch changes this invariant. Better to make it explicitly walk
backwards now I think. (or maybe set things for level in
last_depth..depth). I''ll change things along these lines.
> I think I convinced myself that this is correct.
> 
> 
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 260c2d4..f10ba1b 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -120,13 +120,14 @@ void device_tree_set_reg(u32 **cell, u32
address_cells, u32 size_cells,
> >      set_val(cell, size_cells, size);
> >  }
> >  
> > -u32 device_tree_get_u32(const void *fdt, int node, const char
*prop_name)
> > +u32 device_tree_get_u32(const void *fdt, int node, const char
*prop_name,
> > +                        u32 dflt)
> >  {
> >      const struct fdt_property *prop;
> >  
> >      prop = fdt_get_property(fdt, node, prop_name, NULL);
> >      if ( !prop || prop->len < sizeof(u32) )
> > -        return 0; /* default to 0 */
> > +        return dflt;
> >  
> >      return fdt32_to_cpu(*(uint32_t*)prop->data);
> >  }
> 
> where did the vowels go? :)
Not sure. Unlike me ;-)

Ian.

> > > -u32 device_tree_get_u32(const void *fdt, int node, const char
*prop_name)
> > > +u32 device_tree_get_u32(const void *fdt, int node, const char
*prop_name,
> > > +                        u32 dflt)
> > >  {
> > >      const struct fdt_property *prop;
> > >  
> > >      prop = fdt_get_property(fdt, node, prop_name, NULL);
> > >      if ( !prop || prop->len < sizeof(u32) )
> > > -        return 0; /* default to 0 */
> > > +        return dflt;
> > >  
> > >      return fdt32_to_cpu(*(uint32_t*)prop->data);
> > >  }
> > 
> > where did the vowels go? :)
> 
> Not sure. Unlike me ;-)
I remembered when I went to change it, default is a C keyword...

Ian.

On Fri, 2013-02-15 at 13:18 +0000, Ian Campbell wrote:
> On Fri, 2013-02-15 at 12:43 +0000, Stefano Stabellini wrote:
> > On Wed, 30 Jan 2013, Ian Campbell wrote:
> > > If a node does not have #*-cells then the parent''s value
should be
> > > used. Currently we were asssuming zero which is useless.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > >  xen/arch/arm/domain_build.c   |    6 ++++--
> > >  xen/common/device_tree.c      |   12 ++++++++----
> > >  xen/include/xen/device_tree.h |    3 ++-
> > >  3 files changed, 14 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/domain_build.c
b/xen/arch/arm/domain_build.c
> > > index 7403f1a..bfbe7c7 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -198,8 +198,10 @@ static int write_nodes(struct domain *d,
struct kernel_info *kinfo,
> > >          while ( last_depth-- >= depth )
> > >              fdt_end_node(kinfo->fdt);
> > >  
> > > -        address_cells[depth] = device_tree_get_u32(fdt, node,
"#address-cells");
> > > -        size_cells[depth] = device_tree_get_u32(fdt, node,
"#size-cells");
> > > +        address_cells[depth] = device_tree_get_u32(fdt, node,
"#address-cells",
> > > +                                    depth > 0 ?
address_cells[depth-1] : 0);
> > > +        size_cells[depth] = device_tree_get_u32(fdt, node,
"#size-cells",
> > > +                                    depth > 0 ?
size_cells[depth-1] : 0);
> > >  
> > >          fdt_begin_node(kinfo->fdt, name);
> > 
> > The depth is always increasing by steps of 1 in this loop, right?
> > Because retrieving address-cells and size-cells should be recursive:
if
> > n-1 doesn''t have them, let''s look at n-2, etc. Of
course if we start from
> > depth = 0 and go from there without missing any levels the results
will
> > be the same.
> 
> That was what I thought too. Perhaps it is too subtle?
> 
> I bet my "xen: strip xen,multiboot-module nodes from dom0 device
tree"
> patch changes this invariant. Better to make it explicitly walk
> backwards now I think. (or maybe set things for level in
> last_depth..depth). I''ll change things along these lines.
I looked into this and even with that patch the invariant that we cannot
skip levels is present. This turns out to be obviously the case because
you have actually open the node and name it... I''ll add an ASSERT here
though just in case.

Ian.

On Fri, 15 Feb 2013, Ian Campbell wrote:
> > > > -u32 device_tree_get_u32(const void *fdt, int node, const
char *prop_name)
> > > > +u32 device_tree_get_u32(const void *fdt, int node, const
char *prop_name,
> > > > +                        u32 dflt)
> > > >  {
> > > >      const struct fdt_property *prop;
> > > >  
> > > >      prop = fdt_get_property(fdt, node, prop_name, NULL);
> > > >      if ( !prop || prop->len < sizeof(u32) )
> > > > -        return 0; /* default to 0 */
> > > > +        return dflt;
> > > >  
> > > >      return fdt32_to_cpu(*(uint32_t*)prop->data);
> > > >  }
> > > 
> > > where did the vowels go? :)
> > 
> > Not sure. Unlike me ;-)
> 
> I remembered when I went to change it, default is a C keyword...
I would have gone for _default, but I am OK with this too.

On Mon, 2013-02-18 at 13:11 +0000, Stefano Stabellini
wrote:
> On Fri, 15 Feb 2013, Ian Campbell wrote:
> > > > > -u32 device_tree_get_u32(const void *fdt, int node,
const char *prop_name)
> > > > > +u32 device_tree_get_u32(const void *fdt, int node,
const char *prop_name,
> > > > > +                        u32 dflt)
> > > > >  {
> > > > >      const struct fdt_property *prop;
> > > > >  
> > > > >      prop = fdt_get_property(fdt, node, prop_name,
NULL);
> > > > >      if ( !prop || prop->len < sizeof(u32) )
> > > > > -        return 0; /* default to 0 */
> > > > > +        return dflt;
> > > > >  
> > > > >      return fdt32_to_cpu(*(uint32_t*)prop->data);
> > > > >  }
> > > > 
> > > > where did the vowels go? :)
> > > 
> > > Not sure. Unlike me ;-)
> > 
> > I remembered when I went to change it, default is a C keyword...
> 
> I would have gone for _default, but I am OK with this too.
Technically _default is reserved for the implementation (compiler or
system libraries, I forget which).

Ian.

On Tue, 2013-02-05 at 11:26 +0000, Anthony PERARD wrote:
> > +    /*
> > +     * Sanity-check address sizes, since addresses and sizes which do
not take
> > +     * up exactly 4 or 8 bytes are not supported.
> > +     */
> > +    if ((addrcells != 1 && addrcells != 2) ||
> > +        (sizecells != 1 && sizecells != 2))
> > +        panic("Cannot cope with this size");
> 
> You could add those two properties in the hypervisor node if they
don''t
> match what we expect them to be. This would avoid panicking with a
> device tree that have different default values.
Unfortunately the #address-cells nodes etc affect only the children of
the current node. The sizes for this node are controlled by the
parent''s
sizes.

In practice #address-cells more than 2 (i.e. using 16 or more bytes)
seem like they are going to be rather rare.

Ian.