Xen devel - Dec 2012 - [PATCH 00/09 V4] arm: support for initial modules (e.g. dom0) and DTB supplied in RAM

(I appear to have labelled V3 as V2 when I sent it out, so this V4)

The following series implements support for initial images and DTB in
RAM, as opposed to in flash (dom0 kernel) or compiled into the
hypervisor (DTB). It arranges to not clobber these with either the h/v
text on relocation or with the heaps and frees them as appropriate.

Most of this is independent of the specific bootloader protocol which is
used to tell Xen where these modules actually are, but I have included a
simple PoC bootloader protocol based around device tree which is similar
to the protocol used by Linux to find its initrd
(where /chosen/linux,initrd-{start,end} indicate the physical address).

The PoC protocol is documented in docs/misc/arm/device-tree/booting.txt
which is added by this series.

The major change this time is to use /chosen/modules/module@<N> rather
than /chosen/module@<N> plus using the compatible node to differentiate
which node is which (so <N> is now arbitrary).

As ever the bootloader half is at:
http://xenbits.xen.org/gitweb/?p=people/ianc/boot-wrapper.git;a=summary

Ian.

Otherwise gcc complains about variables being used when not
initialised when in fact that point is never reached.

There aren''t any instances of this in tree right now, I noticed this
while developing another patch.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/include/asm-arm/early_printk.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/xen/include/asm-arm/early_printk.h
b/xen/include/asm-arm/early_printk.h
index f45f21e..a770d4a 100644
--- a/xen/include/asm-arm/early_printk.h
+++ b/xen/include/asm-arm/early_printk.h
@@ -15,7 +15,7 @@
 #ifdef EARLY_UART_ADDRESS
 
 void early_printk(const char *fmt, ...);
-void early_panic(const char *fmt, ...);
+void early_panic(const char *fmt, ...) __attribute__((noreturn));
 
 #else
 
-- 
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

The hypervisor parses for 2 types of module, linux zImages and linux
initrds. Currently we don''t do anything with them.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
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              |   86 +++++++++++++++++++++++++++++++++
 xen/include/xen/device_tree.h         |   14 +++++
 3 files changed, 125 insertions(+), 0 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 da0af77..4bb640e 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -270,6 +270,90 @@ static void __init process_cpu_node(const void *fdt, int
node,
     cpumask_set_cpu(start, &cpu_possible_map);
 }
 
+static int __init process_chosen_modules_node(const void *fdt, int node,
+                                              const char *name, int *depth,
+                                              u32 address_cells, u32
size_cells)
+{
+    const struct fdt_property *prop;
+    const u32 *cell;
+    int nr, nr_modules = 0;
+    struct dt_mb_module *mod;
+    int len;
+
+    for ( *depth = 1;
+          *depth >= 1;
+          node = fdt_next_node(fdt, node, depth) )
+    {
+        name = fdt_get_name(fdt, node, NULL);
+        if ( strncmp(name, "module@", strlen("module@")) ==
0 ) {
+
+            if ( fdt_node_check_compatible(fdt, node,
+                                           "xen,multiboot-module" )
!= 0 )
+                early_panic("%s not a compatible module node\n",
name);
+
+            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 byte\n");
+
+            if ( nr > nr_modules )
+                nr_modules = nr;
+
+            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;
+        }
+    }
+
+    for ( nr = 1 ; nr < nr_modules ; nr++ )
+    {
+        mod = &early_info.modules.module[nr];
+        if ( !mod->start || !mod->size )
+            early_panic("module %d  missing / invalid\n", nr);
+    }
+
+    early_info.modules.nr_mods = nr_modules;
+    return node;
+}
+
+static void __init process_chosen_node(const void *fdt, int node,
+                                       const char *name,
+                                       u32 address_cells, u32 size_cells)
+{
+    int depth;
+
+    for ( depth = 0;
+          depth >= 0;
+          node = fdt_next_node(fdt, node, &depth) )
+    {
+        name = fdt_get_name(fdt, node, NULL);
+        if ( depth == 1 && strcmp(name, "modules") == 0 )
+            node = process_chosen_modules_node(fdt, node, name, &depth,
+                                               address_cells, size_cells);
+    }
+}
+
 static int __init early_scan_node(const void *fdt,
                                   int node, const char *name, int depth,
                                   u32 address_cells, u32 size_cells,
@@ -279,6 +363,8 @@ static int __init early_scan_node(const void *fdt,
         process_memory_node(fdt, node, name, address_cells, size_cells);
     else if ( device_tree_type_matches(fdt, node, "cpu") )
         process_cpu_node(fdt, node, name, address_cells, size_cells);
+    else if ( device_tree_node_matches(fdt, node, "chosen") )
+        process_chosen_node(fdt, node, name, address_cells, size_cells);
 
     return 0;
 }
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 4d010c0..c383677 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -15,6 +15,7 @@
 #define DEVICE_TREE_MAX_DEPTH 16
 
 #define NR_MEM_BANKS 8
+#define NR_MODULES 2
 
 struct membank {
     paddr_t start;
@@ -26,8 +27,21 @@ struct dt_mem_info {
     struct membank bank[NR_MEM_BANKS];
 };
 
+struct dt_mb_module {
+    paddr_t start;
+    paddr_t size;
+    char cmdline[1024];
+};
+
+struct dt_module_info {
+    int nr_mods;
+    /* Module 0 is Xen itself, followed by the provided modules-proper */
+    struct dt_mb_module module[NR_MODULES + 1];
+};
+
 struct dt_early_info {
     struct dt_mem_info mem;
+    struct dt_module_info modules;
 };
 
 typedef int (*device_tree_node_func)(const void *fdt,
-- 
1.7.9.1

This will still fail if the modules are such that Xen is pushed out of
the top 32M of RAM since it will then overlap with the domheap (or
possibly xenheap). This will be dealt with later.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
---
v2: Xen is module 0, modules start at 1.
---
 xen/arch/arm/setup.c |   68 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 2076724..a97455e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -68,17 +68,55 @@ static void __init processor_id(void)
            READ_CP32(ID_ISAR3), READ_CP32(ID_ISAR4), READ_CP32(ID_ISAR5));
 }
 
+/*
+ * Returns the end address of the highest region in the range s..e
+ * with required size and alignment that does not conflict with the
+ * modules from first_mod to nr_modules.
+ *
+ * For non-recursive callers first_mod should normally be 1.
+ */
+static paddr_t __init consider_modules(paddr_t s, paddr_t e,
+                                       uint32_t size, paddr_t align,
+                                       int first_mod)
+{
+    const struct dt_module_info *mi = &early_info.modules;
+    int i;
+
+    s = (s+align-1) & ~(align-1);
+    e = e & ~(align-1);
+
+    if ( s > e ||  e - s < size )
+        return 0;
+
+    for ( i = first_mod; i <= mi->nr_mods; i++ )
+    {
+        paddr_t mod_s = mi->module[i].start;
+        paddr_t mod_e = mod_s + mi->module[i].size;
+
+        if ( s < mod_e && mod_s < e )
+        {
+            mod_e = consider_modules(mod_e, e, size, align, i+1);
+            if ( mod_e )
+                return mod_e;
+
+            return consider_modules(s, mod_s, size, align, i+1);
+        }
+    }
+
+    return e;
+}
+
 /**
  * get_xen_paddr - get physical address to relocate Xen to
  *
- * Xen is relocated to the top of RAM and aligned to a XEN_PADDR_ALIGN
- * boundary.
+ * Xen is relocated to as near to the top of RAM as possible and
+ * aligned to a XEN_PADDR_ALIGN boundary.
  */
 static paddr_t __init get_xen_paddr(void)
 {
     struct dt_mem_info *mi = &early_info.mem;
     paddr_t min_size;
-    paddr_t paddr = 0, t;
+    paddr_t paddr = 0;
     int i;
 
     min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) &
~(XEN_PADDR_ALIGN-1);
@@ -86,17 +124,33 @@ static paddr_t __init get_xen_paddr(void)
     /* Find the highest bank with enough space. */
     for ( i = 0; i < mi->nr_banks; i++ )
     {
-        if ( mi->bank[i].size >= min_size )
+        const struct membank *bank = &mi->bank[i];
+        paddr_t s, e;
+
+        if ( bank->size >= min_size )
         {
-            t = mi->bank[i].start + mi->bank[i].size - min_size;
-            if ( t > paddr )
-                paddr = t;
+            e = consider_modules(bank->start, bank->start +
bank->size,
+                                 min_size, XEN_PADDR_ALIGN, 1);
+            if ( !e )
+                continue;
+
+            s = e - min_size;
+
+            if ( s > paddr )
+                paddr = s;
         }
     }
 
     if ( !paddr )
         early_panic("Not enough memory to relocate Xen\n");
 
+    early_printk("Placing Xen at
0x%"PRIpaddr"-0x%"PRIpaddr"\n",
+                 paddr, paddr + min_size);
+
+    /* Xen is module 0 */
+    early_info.modules.module[0].start = paddr;
+    early_info.modules.module[0].size = min_size;
+
     return paddr;
 }
 
-- 
1.7.9.1

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/setup.c |   89 +++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 78 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index a97455e..9f08daf 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -73,7 +73,8 @@ static void __init processor_id(void)
  * with required size and alignment that does not conflict with the
  * modules from first_mod to nr_modules.
  *
- * For non-recursive callers first_mod should normally be 1.
+ * For non-recursive callers first_mod should normally be 0 (all
+ * modules and Xen itself) or 1 (all modules but not Xen).
  */
 static paddr_t __init consider_modules(paddr_t s, paddr_t e,
                                        uint32_t size, paddr_t align,
@@ -106,6 +107,34 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t
e,
     return e;
 }
 
+/*
+ * Return the end of the non-module region starting at s. In other
+ * words return s the start of the next modules after s.
+ *
+ * Also returns the end of that module in *n.
+ */
+static paddr_t __init next_module(paddr_t s, paddr_t *n)
+{
+    struct dt_module_info *mi = &early_info.modules;
+    paddr_t lowest = ~(paddr_t)0;
+    int i;
+
+    for ( i = 0; i <= mi->nr_mods; i++ )
+    {
+        paddr_t mod_s = mi->module[i].start;
+        paddr_t mod_e = mod_s + mi->module[i].size;
+
+        if ( mod_s < s )
+            continue;
+        if ( mod_s > lowest )
+            continue;
+        lowest = mod_s;
+        *n = mod_e;
+    }
+    return lowest;
+}
+
+
 /**
  * get_xen_paddr - get physical address to relocate Xen to
  *
@@ -159,6 +188,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t
dtb_size)
     paddr_t ram_start;
     paddr_t ram_end;
     paddr_t ram_size;
+    paddr_t s, e;
     unsigned long ram_pages;
     unsigned long heap_pages, xenheap_pages, domheap_pages;
     unsigned long dtb_pages;
@@ -176,22 +206,37 @@ static void __init setup_mm(unsigned long dtb_paddr,
size_t dtb_size)
     ram_pages = ram_size >> PAGE_SHIFT;
 
     /*
-     * Calculate the sizes for the heaps using these constraints:
+     * Locate the xenheap using these constraints:
      *
-     *  - heaps must be 32 MiB aligned
-     *  - must not include Xen itself
-     *  - xen heap must be at most 1 GiB
+     *  - must be 32 MiB aligned
+     *  - must not include Xen itself or the boot modules
+     *  - must be at most 1 GiB
+     *  - must be at least 128M
      *
-     * XXX: needs a platform with at least 1GiB of RAM or the dom
-     * heap will be empty and no domains can be created.
+     * We try to allocate the largest xenheap possible within these
+     * constraints.
      */
-    heap_pages = (ram_size >> PAGE_SHIFT) - (32 << (20 -
PAGE_SHIFT));
+    heap_pages = (ram_size >> PAGE_SHIFT);
     xenheap_pages = min(1ul << (30 - PAGE_SHIFT), heap_pages);
+
+    do
+    {
+        e = consider_modules(ram_start, ram_end,
xenheap_pages<<PAGE_SHIFT,
+                             32<<20, 0);
+        if ( e )
+            break;
+
+        xenheap_pages >>= 1;
+    } while ( xenheap_pages > 128<<(20-PAGE_SHIFT) );
+
+    if ( ! e )
+        panic("Not not enough space for xenheap\n");
+
     domheap_pages = heap_pages - xenheap_pages;
 
     printk("Xen heap: %lu pages  Dom heap: %lu pages\n",
xenheap_pages, domheap_pages);
 
-    setup_xenheap_mappings(ram_start >> PAGE_SHIFT, xenheap_pages);
+    setup_xenheap_mappings((e >> PAGE_SHIFT) - xenheap_pages,
xenheap_pages);
 
     /*
      * Need a single mapped page for populating bootmem_region_list
@@ -215,8 +260,30 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t
dtb_size)
     copy_from_paddr(device_tree_flattened, dtb_paddr, dtb_size, BUFFERABLE);
 
     /* Add non-xenheap memory */
-    init_boot_pages(pfn_to_paddr(xenheap_mfn_start + xenheap_pages),
-                    pfn_to_paddr(xenheap_mfn_start + xenheap_pages +
domheap_pages));
+    s = ram_start;
+    while ( s < ram_end )
+    {
+        paddr_t n = ram_end;
+
+        e = next_module(s, &n);
+
+        if ( e == ~(paddr_t)0 )
+        {
+            e = n = ram_end;
+        }
+
+        /* Avoid the xenheap */
+        if ( s < ((xenheap_mfn_start+xenheap_pages) << PAGE_SHIFT)
+             && (xenheap_mfn_start << PAGE_SHIFT) < e )
+        {
+            e = pfn_to_paddr(xenheap_mfn_start);
+            n = pfn_to_paddr(xenheap_mfn_start+xenheap_pages);
+        }
+
+        init_boot_pages(s, e);
+
+        s = n;
+    }
 
     setup_frametable_mappings(ram_start, ram_end);
     max_page = PFN_DOWN(ram_end);
-- 
1.7.9.1

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
---
v3: early_panic instead of BUG_ON
v2: drop unrelated white space fixup
---
 xen/common/device_tree.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 4bb640e..dc28c56 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -58,6 +58,9 @@ static void __init get_val(const u32 **cell, u32 cells, u64
*val)
 {
     *val = 0;
 
+    if ( cells > 2 )
+        early_panic("dtb value contains > 2 cells\n");
+
     while ( cells-- )
     {
         *val <<= 32;
-- 
1.7.9.1

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
---
v3: - correct limit check in try_zimage_prepare
    - copy zimage header to a local bufffer to avoid issues with
      crossing page boundaries.
    - handle non page aligned source and destinations when loading
    - use a BUFFERABLE mapping when loading kernel from RAM.
---
 xen/arch/arm/kernel.c |   91 ++++++++++++++++++++++++++++++++++--------------
 xen/arch/arm/kernel.h |   11 ++++++
 2 files changed, 75 insertions(+), 27 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 2d56130..c9265d7 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -22,6 +22,7 @@
 #define ZIMAGE_MAGIC_OFFSET 0x24
 #define ZIMAGE_START_OFFSET 0x28
 #define ZIMAGE_END_OFFSET   0x2c
+#define ZIMAGE_HEADER_LEN   0x30
 
 #define ZIMAGE_MAGIC 0x016f2818
 
@@ -65,40 +66,42 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long
len, int attrindx)
 static void kernel_zimage_load(struct kernel_info *info)
 {
     paddr_t load_addr = info->zimage.load_addr;
+    paddr_t paddr = info->zimage.kernel_addr;
+    paddr_t attr = info->load_attr;
     paddr_t len = info->zimage.len;
-    paddr_t flash = KERNEL_FLASH_ADDRESS;
-    void *src = (void *)FIXMAP_ADDR(FIXMAP_MISC);
     unsigned long offs;
 
-    printk("Loading %"PRIpaddr" byte zImage from flash
%"PRIpaddr" to %"PRIpaddr"-%"PRIpaddr": [",
-           len, flash, load_addr, load_addr + len);
-    for ( offs = 0; offs < len; offs += PAGE_SIZE )
+    printk("Loading zImage from %"PRIpaddr" to
%"PRIpaddr"-%"PRIpaddr"\n",
+           paddr, load_addr, load_addr + len);
+    for ( offs = 0; offs < len; )
     {
-        paddr_t ma = gvirt_to_maddr(load_addr + offs);
+        paddr_t s, l, ma = gvirt_to_maddr(load_addr + offs);
         void *dst = map_domain_page(ma>>PAGE_SHIFT);
 
-        if ( ( offs % (1<<20) ) == 0 )
-            printk(".");
+        s = offs & ~PAGE_MASK;
+        l = min(PAGE_SIZE - s, len);
 
-        set_fixmap(FIXMAP_MISC, (flash+offs) >> PAGE_SHIFT, DEV_SHARED);
-        memcpy(dst, src, PAGE_SIZE);
-        clear_fixmap(FIXMAP_MISC);
+        copy_from_paddr(dst + s, paddr + offs, l, attr);
 
         unmap_domain_page(dst);
+        offs += l;
     }
-    printk("]\n");
 }
 
 /**
  * Check the image is a zImage and return the load address and length
  */
-static int kernel_try_zimage_prepare(struct kernel_info *info)
+static int kernel_try_zimage_prepare(struct kernel_info *info,
+                                     paddr_t addr, paddr_t size)
 {
-    uint32_t *zimage = (void *)FIXMAP_ADDR(FIXMAP_MISC);
+    uint32_t zimage[ZIMAGE_HEADER_LEN/4];
     uint32_t start, end;
     struct minimal_dtb_header dtb_hdr;
 
-    set_fixmap(FIXMAP_MISC, KERNEL_FLASH_ADDRESS >> PAGE_SHIFT,
DEV_SHARED);
+    if ( size < ZIMAGE_HEADER_LEN )
+        return -EINVAL;
+
+    copy_from_paddr(zimage, addr, sizeof(zimage), DEV_SHARED);
 
     if (zimage[ZIMAGE_MAGIC_OFFSET/4] != ZIMAGE_MAGIC)
         return -EINVAL;
@@ -106,16 +109,26 @@ static int kernel_try_zimage_prepare(struct kernel_info
*info)
     start = zimage[ZIMAGE_START_OFFSET/4];
     end = zimage[ZIMAGE_END_OFFSET/4];
 
-    clear_fixmap(FIXMAP_MISC);
+    if ( (end - start) > size )
+        return -EINVAL;
 
     /*
      * Check for an appended DTB.
      */
-    copy_from_paddr(&dtb_hdr, KERNEL_FLASH_ADDRESS + end - start,
sizeof(dtb_hdr), DEV_SHARED);
-    if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) {
-        end += be32_to_cpu(dtb_hdr.total_size);
+    if ( addr + end - start + sizeof(dtb_hdr) <= size )
+    {
+        copy_from_paddr(&dtb_hdr, addr + end - start,
+                        sizeof(dtb_hdr), DEV_SHARED);
+        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) {
+            end += be32_to_cpu(dtb_hdr.total_size);
+
+            if ( end > addr + size )
+                return -EINVAL;
+        }
     }
 
+    info->zimage.kernel_addr = addr;
+
     /*
      * If start is zero, the zImage is position independent -- load it
      * at 32k from start of RAM.
@@ -142,25 +155,26 @@ static void kernel_elf_load(struct kernel_info *info)
     free_xenheap_pages(info->kernel_img, info->kernel_order);
 }
 
-static int kernel_try_elf_prepare(struct kernel_info *info)
+static int kernel_try_elf_prepare(struct kernel_info *info,
+                                  paddr_t addr, paddr_t size)
 {
     int rc;
 
-    info->kernel_order = get_order_from_bytes(KERNEL_FLASH_SIZE);
+    info->kernel_order = get_order_from_bytes(size);
     info->kernel_img = alloc_xenheap_pages(info->kernel_order, 0);
     if ( info->kernel_img == NULL )
         panic("Cannot allocate temporary buffer for kernel.\n");
 
-    copy_from_paddr(info->kernel_img, KERNEL_FLASH_ADDRESS,
KERNEL_FLASH_SIZE, DEV_SHARED);
+    copy_from_paddr(info->kernel_img, addr, size, info->load_attr);
 
-    if ( (rc = elf_init(&info->elf.elf, info->kernel_img,
KERNEL_FLASH_SIZE )) != 0 )
-        return rc;
+    if ( (rc = elf_init(&info->elf.elf, info->kernel_img, size )) !=
0 )
+        goto err;
 #ifdef VERBOSE
     elf_set_verbose(&info->elf.elf);
 #endif
     elf_parse_binary(&info->elf.elf);
     if ( (rc = elf_xen_parse(&info->elf.elf, &info->elf.parms))
!= 0 )
-        return rc;
+        goto err;
 
     /*
      * TODO: can the ELF header be used to find the physical address
@@ -170,15 +184,38 @@ static int kernel_try_elf_prepare(struct kernel_info
*info)
     info->load = kernel_elf_load;
 
     return 0;
+err:
+    free_xenheap_pages(info->kernel_img, info->kernel_order);
+    return rc;
 }
 
 int kernel_prepare(struct kernel_info *info)
 {
     int rc;
 
-    rc = kernel_try_zimage_prepare(info);
+    paddr_t start, size;
+
+    if ( early_info.modules.nr_mods > 1 )
+        panic("Cannot handle dom0 initrd yet\n");
+
+    if ( early_info.modules.nr_mods < 1 )
+    {
+        printk("No boot modules found, trying flash\n");
+        start = KERNEL_FLASH_ADDRESS;
+        size = KERNEL_FLASH_SIZE;
+        info->load_attr = DEV_SHARED;
+    }
+    else
+    {
+        printk("Loading kernel from boot module 1\n");
+        start = early_info.modules.module[1].start;
+        size = early_info.modules.module[1].size;
+        info->load_attr = BUFFERABLE;
+    }
+
+    rc = kernel_try_zimage_prepare(info, start, size);
     if (rc < 0)
-        rc = kernel_try_elf_prepare(info);
+        rc = kernel_try_elf_prepare(info, start, size);
 
     return rc;
 }
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index 4533568..49fe9da 100644
--- a/xen/arch/arm/kernel.h
+++ b/xen/arch/arm/kernel.h
@@ -22,6 +22,7 @@ struct kernel_info {
 
     union {
         struct {
+            paddr_t kernel_addr;
             paddr_t load_addr;
             paddr_t len;
         } zimage;
@@ -33,9 +34,19 @@ struct kernel_info {
     };
 
     void (*load)(struct kernel_info *info);
+    int load_attr;
 };
 
 int kernel_prepare(struct kernel_info *info);
 void kernel_load(struct kernel_info *info);
 
 #endif /* #ifdef __ARCH_ARM_KERNEL_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-set-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.7.9.1

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/domain_build.c |    3 +++
 xen/arch/arm/setup.c        |   16 ++++++++++++++++
 xen/include/asm-arm/setup.h |    2 ++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index a9e7f43..e96ed10 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -10,6 +10,7 @@
 #include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.h>
 #include <xen/guest_access.h>
+#include <asm/setup.h>
 
 #include "gic.h"
 #include "kernel.h"
@@ -308,6 +309,8 @@ int construct_dom0(struct domain *d)
     dtb_load(&kinfo);
     kernel_load(&kinfo);
 
+    discard_initial_modules();
+
     clear_bit(_VPF_down, &v->pause_flags);
 
     memset(regs, 0, sizeof(*regs));
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 9f08daf..1eb8f77 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -68,6 +68,22 @@ static void __init processor_id(void)
            READ_CP32(ID_ISAR3), READ_CP32(ID_ISAR4), READ_CP32(ID_ISAR5));
 }
 
+void __init discard_initial_modules(void)
+{
+    struct dt_module_info *mi = &early_info.modules;
+    int i;
+
+    for ( i = 1; i <= mi->nr_mods; i++ )
+    {
+        paddr_t s = mi->module[i].start;
+        paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
+
+        init_domheap_pages(s, e);
+    }
+
+    mi->nr_mods = 0;
+}
+
 /*
  * Returns the end address of the highest region in the range s..e
  * with required size and alignment that does not conflict with the
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 8769f66..3267db0 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -9,6 +9,8 @@ void arch_get_xen_caps(xen_capabilities_info_t *info);
 
 int construct_dom0(struct domain *d);
 
+void discard_initial_modules(void);
+
 #endif
 /*
  * Local variables:
-- 
1.7.9.1

Fallback to xen,dom0-bootargs if this isn''t present.

Ideally this would use module1-args iff the kernel came from
module@1/{start,end} and the existing xen,dom0-bootargs if the kernel
came from flash, but this approach is simpler and has the same effect
in practice.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
---
v2: update for new DT layout
---
 xen/arch/arm/domain_build.c |   28 ++++++++++++++++++++++++----
 1 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e96ed10..7a964f7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -86,8 +86,13 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
                             int node, const char *name, int depth,
                             u32 address_cells, u32 size_cells)
 {
+    const char *bootargs = NULL;
     int prop;
 
+    if ( early_info.modules.nr_mods >= 1 &&
+         early_info.modules.module[1].cmdline[0] )
+        bootargs = &early_info.modules.module[1].cmdline[0];
+
     for ( prop = fdt_first_property_offset(fdt, node);
           prop >= 0;
           prop = fdt_next_property_offset(fdt, prop) )
@@ -104,15 +109,22 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
         prop_len  = fdt32_to_cpu(p->len);
 
         /*
-         * In chosen node: replace bootargs with value from
-         * xen,dom0-bootargs.
+         * In chosen node:
+         *
+         * * remember xen,dom0-bootargs if we don''t already have
+         *   bootargs (from module #1, above).
+         * * remove bootargs and xen,dom0-bootargs.
          */
         if ( device_tree_node_matches(fdt, node, "chosen") )
         {
             if ( strcmp(prop_name, "bootargs") == 0 )
                 continue;
-            if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
-                prop_name = "bootargs";
+            else if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
+            {
+                if ( !bootargs )
+                    bootargs = prop_data;
+                continue;
+            }
         }
         /*
          * In a memory node: adjust reg property.
@@ -147,6 +159,14 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
         xfree(new_data);
     }
 
+    if ( device_tree_node_matches(fdt, node, "chosen") &&
bootargs )
+        fdt_property(kinfo->fdt, "bootargs", bootargs,
strlen(bootargs));
+
+    /*
+     * XXX should populate /chosen/linux,initrd-{start,end} here if we
+     * have module[2]
+     */
+
     if ( prop == -FDT_ERR_NOTFOUND )
         return 0;
     return prop;
-- 
1.7.9.1

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

Only drop the "xen,multiboot-module" compatible nodes in case someone
else has a similar idea.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
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 |   40 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 7a964f7..27e02e4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -172,6 +172,40 @@ 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 parents[DEVICE_TREE_MAX_DEPTH])
+{
+    int depth = *depth_out;
+
+    while ( (node = fdt_next_node(fdt, node, &depth)) &&
+            node >= 0 && depth >= 0 )
+    {
+        if ( depth >= DEVICE_TREE_MAX_DEPTH )
+            break;
+
+        parents[depth] = node;
+
+        /* Skip /chosen/modules/module@<N>/ and all subnodes */
+        if ( depth >= 3 &&
+             device_tree_node_matches(fdt, parents[1], "chosen")
&&
+             device_tree_node_matches(fdt, parents[2], "modules")
&&
+             device_tree_node_matches(fdt, parents[3], "module")
&&
+             fdt_node_check_compatible(fdt, parents[3],
+                                       "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)
 {
@@ -179,11 +213,12 @@ static int write_nodes(struct domain *d, struct
kernel_info *kinfo,
     int depth = 0, last_depth = -1;
     u32 address_cells[DEVICE_TREE_MAX_DEPTH];
     u32 size_cells[DEVICE_TREE_MAX_DEPTH];
+    int parents[DEVICE_TREE_MAX_DEPTH];
     int ret;
 
     for ( node = 0, depth = 0;
           node >= 0 && depth >= 0;
-          node = fdt_next_node(fdt, node, &depth) )
+          node = fdt_next_dom0_node(fdt, node, &depth, parents) )
     {
         const char *name;
 
@@ -191,7 +226,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 Thu, 6 Dec 2012, 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
> 
> The hypervisor parses for 2 types of module, linux zImages and linux
> initrds. Currently we don''t do anything with them.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> 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              |   86
+++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h         |   14 +++++
>  3 files changed, 125 insertions(+), 0 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 da0af77..4bb640e 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -270,6 +270,90 @@ static void __init process_cpu_node(const void *fdt,
int node,
>      cpumask_set_cpu(start, &cpu_possible_map);
>  }
>  
> +static int __init process_chosen_modules_node(const void *fdt, int node,
> +                                              const char *name, int
*depth,
> +                                              u32 address_cells, u32
size_cells)
> +{
> +    const struct fdt_property *prop;
> +    const u32 *cell;
> +    int nr, nr_modules = 0;
> +    struct dt_mb_module *mod;
> +    int len;
> +
> +    for ( *depth = 1;
> +          *depth >= 1;
> +          node = fdt_next_node(fdt, node, depth) )
> +    {
> +        name = fdt_get_name(fdt, node, NULL);
> +        if ( strncmp(name, "module@",
strlen("module@")) == 0 ) {
> +
> +            if ( fdt_node_check_compatible(fdt, node,
> +                                          
"xen,multiboot-module" ) != 0 )
> +                early_panic("%s not a compatible module node\n",
name);
> +
> +            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
byte\n");
> +
> +            if ( nr > nr_modules )
> +                nr_modules = nr;
> +
> +            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;
> +        }
> +    }
> +
> +    for ( nr = 1 ; nr < nr_modules ; nr++ )
> +    {
> +        mod = &early_info.modules.module[nr];
> +        if ( !mod->start || !mod->size )
> +            early_panic("module %d  missing / invalid\n", nr);
> +    }
> +
> +    early_info.modules.nr_mods = nr_modules;
> +    return node;
> +}
> +
> +static void __init process_chosen_node(const void *fdt, int node,
> +                                       const char *name,
> +                                       u32 address_cells, u32 size_cells)
> +{
> +    int depth;
> +
> +    for ( depth = 0;
> +          depth >= 0;
> +          node = fdt_next_node(fdt, node, &depth) )
> +    {
> +        name = fdt_get_name(fdt, node, NULL);
> +        if ( depth == 1 && strcmp(name, "modules") == 0
)
> +            node = process_chosen_modules_node(fdt, node, name,
&depth,
> +                                               address_cells, size_cells);
> +    }
> +}
> +
>  static int __init early_scan_node(const void *fdt,
>                                    int node, const char *name, int depth,
>                                    u32 address_cells, u32 size_cells,
> @@ -279,6 +363,8 @@ static int __init early_scan_node(const void *fdt,
>          process_memory_node(fdt, node, name, address_cells, size_cells);
>      else if ( device_tree_type_matches(fdt, node, "cpu") )
>          process_cpu_node(fdt, node, name, address_cells, size_cells);
> +    else if ( device_tree_node_matches(fdt, node, "chosen") )
> +        process_chosen_node(fdt, node, name, address_cells, size_cells);
>  
>      return 0;
>  }
You have really written a lot of code here!
I would have thought that just matching on the compatible string would
be enough:

else if ( device_tree_node_matches(fdt, node, "linux-zimage") )
     process_linuxzimage_node(fdt, node, name, address_cells, size_cells);
else if ( device_tree_node_matches(fdt, node, "linux-initrd") )
     process_linuxinitrd_node(fdt, node, name, address_cells, size_cells);

so that your process_linuxzimage_node and process_linuxinitrd_node start
from the right node and have everything they need to parse it


> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 4d010c0..c383677 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -15,6 +15,7 @@
>  #define DEVICE_TREE_MAX_DEPTH 16
>  
>  #define NR_MEM_BANKS 8
> +#define NR_MODULES 2
>  
>  struct membank {
>      paddr_t start;
> @@ -26,8 +27,21 @@ struct dt_mem_info {
>      struct membank bank[NR_MEM_BANKS];
>  };
>  
> +struct dt_mb_module {
> +    paddr_t start;
> +    paddr_t size;
> +    char cmdline[1024];
> +};
> +
> +struct dt_module_info {
> +    int nr_mods;
> +    /* Module 0 is Xen itself, followed by the provided modules-proper */
> +    struct dt_mb_module module[NR_MODULES + 1];
> +};
> +
>  struct dt_early_info {
>      struct dt_mem_info mem;
> +    struct dt_module_info modules;
>  };
>  
>  typedef int (*device_tree_node_func)(const void *fdt,
> -- 
> 1.7.9.1
>

On Thu, 6 Dec 2012, Ian Campbell wrote:
> These nodes are used by Xen to find the initial modules.
> 
> Only drop the "xen,multiboot-module" compatible nodes in case
someone
> else has a similar idea.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> 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 |   40
++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7a964f7..27e02e4 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -172,6 +172,40 @@ 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 parents[DEVICE_TREE_MAX_DEPTH])
> +{
> +    int depth = *depth_out;
> +
> +    while ( (node = fdt_next_node(fdt, node, &depth)) &&
> +            node >= 0 && depth >= 0 )
> +    {
> +        if ( depth >= DEVICE_TREE_MAX_DEPTH )
> +            break;
> +
> +        parents[depth] = node;
> +
> +        /* Skip /chosen/modules/module@<N>/ and all subnodes */
> +        if ( depth >= 3 &&
> +             device_tree_node_matches(fdt, parents[1], "chosen")
&&
> +             device_tree_node_matches(fdt, parents[2],
"modules") &&
> +             device_tree_node_matches(fdt, parents[3], "module")
&&
> +             fdt_node_check_compatible(fdt, parents[3],
> +                                       "xen,multiboot-module" )
== 0 )
> +            continue;
> +
> +        /* We''ve arrived at a node which dom0 is interested in.
*/
> +        break;
> +    }
> +
> +    *depth_out = depth;
> +    return node;
> +}
Can''t we just skip the node if it is compatible with
"xen,multiboot-module", no matter where it lives?  This should
simplify
this function greatly and you wouldn''t need the parents parameter
anymore.
This way we could have a simple node blacklist based on the compatible
node all in a single function.

>  static int write_nodes(struct domain *d, struct kernel_info *kinfo,
>                         const void *fdt)
>  {
> @@ -179,11 +213,12 @@ static int write_nodes(struct domain *d, struct
kernel_info *kinfo,
>      int depth = 0, last_depth = -1;
>      u32 address_cells[DEVICE_TREE_MAX_DEPTH];
>      u32 size_cells[DEVICE_TREE_MAX_DEPTH];
> +    int parents[DEVICE_TREE_MAX_DEPTH];
>      int ret;
>  
>      for ( node = 0, depth = 0;
>            node >= 0 && depth >= 0;
> -          node = fdt_next_node(fdt, node, &depth) )
> +          node = fdt_next_dom0_node(fdt, node, &depth, parents) )
>      {
>          const char *name;
>  
> @@ -191,7 +226,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 Fri, 2012-12-07 at 17:30 +0000, Stefano Stabellini
wrote:
> On Thu, 6 Dec 2012, 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
> > 
> > The hypervisor parses for 2 types of module, linux zImages and linux
> > initrds. Currently we don''t do anything with them.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > 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              |   86
+++++++++++++++++++++++++++++++++
> >  xen/include/xen/device_tree.h         |   14 +++++
> >  3 files changed, 125 insertions(+), 0 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 da0af77..4bb640e 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -270,6 +270,90 @@ static void __init process_cpu_node(const void
*fdt, int node,
> >      cpumask_set_cpu(start, &cpu_possible_map);
> >  }
> >  
> > +static int __init process_chosen_modules_node(const void *fdt, int
node,
> > +                                              const char *name, int
*depth,
> > +                                              u32 address_cells, u32
size_cells)
> > +{
> > +    const struct fdt_property *prop;
> > +    const u32 *cell;
> > +    int nr, nr_modules = 0;
> > +    struct dt_mb_module *mod;
> > +    int len;
> > +
> > +    for ( *depth = 1;
> > +          *depth >= 1;
> > +          node = fdt_next_node(fdt, node, depth) )
> > +    {
> > +        name = fdt_get_name(fdt, node, NULL);
> > +        if ( strncmp(name, "module@",
strlen("module@")) == 0 ) {
> > +
> > +            if ( fdt_node_check_compatible(fdt, node,
> > +                                          
"xen,multiboot-module" ) != 0 )
> > +                early_panic("%s not a compatible module
node\n", name);
> > +
> > +            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
byte\n");
> > +
> > +            if ( nr > nr_modules )
> > +                nr_modules = nr;
> > +
> > +            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;
> > +        }
> > +    }
> > +
> > +    for ( nr = 1 ; nr < nr_modules ; nr++ )
> > +    {
> > +        mod = &early_info.modules.module[nr];
> > +        if ( !mod->start || !mod->size )
> > +            early_panic("module %d  missing / invalid\n",
nr);
> > +    }
> > +
> > +    early_info.modules.nr_mods = nr_modules;
> > +    return node;
> > +}
> > +
> > +static void __init process_chosen_node(const void *fdt, int node,
> > +                                       const char *name,
> > +                                       u32 address_cells, u32
size_cells)
> > +{
> > +    int depth;
> > +
> > +    for ( depth = 0;
> > +          depth >= 0;
> > +          node = fdt_next_node(fdt, node, &depth) )
> > +    {
> > +        name = fdt_get_name(fdt, node, NULL);
> > +        if ( depth == 1 && strcmp(name, "modules")
== 0 )
> > +            node = process_chosen_modules_node(fdt, node, name,
&depth,
> > +                                               address_cells,
size_cells);
> > +    }
> > +}
> > +
> >  static int __init early_scan_node(const void *fdt,
> >                                    int node, const char *name, int
depth,
> >                                    u32 address_cells, u32 size_cells,
> > @@ -279,6 +363,8 @@ static int __init early_scan_node(const void *fdt,
> >          process_memory_node(fdt, node, name, address_cells,
size_cells);
> >      else if ( device_tree_type_matches(fdt, node, "cpu") )
> >          process_cpu_node(fdt, node, name, address_cells, size_cells);
> > +    else if ( device_tree_node_matches(fdt, node, "chosen")
)
> > +        process_chosen_node(fdt, node, name, address_cells,
size_cells);
> >  
> >      return 0;
> >  }
> 
> You have really written a lot of code here!
> I would have thought that just matching on the compatible string would
> be enough:
> 
> else if ( device_tree_node_matches(fdt, node, "linux-zimage") )
>      process_linuxzimage_node(fdt, node, name, address_cells, size_cells);
> else if ( device_tree_node_matches(fdt, node, "linux-initrd") )
>      process_linuxinitrd_node(fdt, node, name, address_cells, size_cells);
> 
> so that your process_linuxzimage_node and process_linuxinitrd_node start
> from the right node and have everything they need to parse it
Is the tree structure of Device Tree meaningless? I''d have thought that
a compatible node would only have meaning at a particular place in the
tree. Granted compatible nodes are often pretty specific and precise,
but is that inherent enough in DT that we can rely on it?

> 
> 
> 
> > diff --git a/xen/include/xen/device_tree.h
b/xen/include/xen/device_tree.h
> > index 4d010c0..c383677 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -15,6 +15,7 @@
> >  #define DEVICE_TREE_MAX_DEPTH 16
> >  
> >  #define NR_MEM_BANKS 8
> > +#define NR_MODULES 2
> >  
> >  struct membank {
> >      paddr_t start;
> > @@ -26,8 +27,21 @@ struct dt_mem_info {
> >      struct membank bank[NR_MEM_BANKS];
> >  };
> >  
> > +struct dt_mb_module {
> > +    paddr_t start;
> > +    paddr_t size;
> > +    char cmdline[1024];
> > +};
> > +
> > +struct dt_module_info {
> > +    int nr_mods;
> > +    /* Module 0 is Xen itself, followed by the provided
modules-proper */
> > +    struct dt_mb_module module[NR_MODULES + 1];
> > +};
> > +
> >  struct dt_early_info {
> >      struct dt_mem_info mem;
> > +    struct dt_module_info modules;
> >  };
> >  
> >  typedef int (*device_tree_node_func)(const void *fdt,
> > -- 
> > 1.7.9.1
> >

On Fri, 2012-12-07 at 17:35 +0000, Stefano Stabellini
wrote:
> On Thu, 6 Dec 2012, Ian Campbell wrote:
> > These nodes are used by Xen to find the initial modules.
> > 
> > Only drop the "xen,multiboot-module" compatible nodes in
case someone
> > else has a similar idea.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > 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 |   40
++++++++++++++++++++++++++++++++++++++--
> >  1 files changed, 38 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 7a964f7..27e02e4 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -172,6 +172,40 @@ 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 parents[DEVICE_TREE_MAX_DEPTH])
> > +{
> > +    int depth = *depth_out;
> > +
> > +    while ( (node = fdt_next_node(fdt, node, &depth)) &&
> > +            node >= 0 && depth >= 0 )
> > +    {
> > +        if ( depth >= DEVICE_TREE_MAX_DEPTH )
> > +            break;
> > +
> > +        parents[depth] = node;
> > +
> > +        /* Skip /chosen/modules/module@<N>/ and all subnodes */
> > +        if ( depth >= 3 &&
> > +             device_tree_node_matches(fdt, parents[1],
"chosen") &&
> > +             device_tree_node_matches(fdt, parents[2],
"modules") &&
> > +             device_tree_node_matches(fdt, parents[3],
"module") &&
> > +             fdt_node_check_compatible(fdt, parents[3],
> > +                                      
"xen,multiboot-module" ) == 0 )
> > +            continue;
> > +
> > +        /* We''ve arrived at a node which dom0 is interested
in. */
> > +        break;
> > +    }
> > +
> > +    *depth_out = depth;
> > +    return node;
> > +}
> 
> Can''t we just skip the node if it is compatible with
> "xen,multiboot-module", no matter where it lives?  This should
simplify
> this function greatly and you wouldn''t need the parents parameter
> anymore.
As well as my previous query about the meaning of the tree structure I
think that even if we could get away with this in this particular case
we are going to need this sort of infrastructure once we start doing
proper filtering of dom0 vs xen nodes in the tree.
> This way we could have a simple node blacklist based on the compatible
> node all in a single function.

On Mon, 10 Dec 2012, Ian Campbell wrote:
> On Fri, 2012-12-07 at 17:30 +0000, Stefano Stabellini wrote:
> > On Thu, 6 Dec 2012, 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
> > > 
> > > The hypervisor parses for 2 types of module, linux zImages and
linux
> > > initrds. Currently we don''t do anything with them.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > > 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              |   86
+++++++++++++++++++++++++++++++++
> > >  xen/include/xen/device_tree.h         |   14 +++++
> > >  3 files changed, 125 insertions(+), 0 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 da0af77..4bb640e 100644
> > > --- a/xen/common/device_tree.c
> > > +++ b/xen/common/device_tree.c
> > > @@ -270,6 +270,90 @@ static void __init process_cpu_node(const
void *fdt, int node,
> > >      cpumask_set_cpu(start, &cpu_possible_map);
> > >  }
> > >  
> > > +static int __init process_chosen_modules_node(const void *fdt,
int node,
> > > +                                              const char *name,
int *depth,
> > > +                                              u32 address_cells,
u32 size_cells)
> > > +{
> > > +    const struct fdt_property *prop;
> > > +    const u32 *cell;
> > > +    int nr, nr_modules = 0;
> > > +    struct dt_mb_module *mod;
> > > +    int len;
> > > +
> > > +    for ( *depth = 1;
> > > +          *depth >= 1;
> > > +          node = fdt_next_node(fdt, node, depth) )
> > > +    {
> > > +        name = fdt_get_name(fdt, node, NULL);
> > > +        if ( strncmp(name, "module@",
strlen("module@")) == 0 ) {
> > > +
> > > +            if ( fdt_node_check_compatible(fdt, node,
> > > +                                          
"xen,multiboot-module" ) != 0 )
> > > +                early_panic("%s not a compatible module
node\n", name);
> > > +
> > > +            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
byte\n");
> > > +
> > > +            if ( nr > nr_modules )
> > > +                nr_modules = nr;
> > > +
> > > +            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;
> > > +        }
> > > +    }
> > > +
> > > +    for ( nr = 1 ; nr < nr_modules ; nr++ )
> > > +    {
> > > +        mod = &early_info.modules.module[nr];
> > > +        if ( !mod->start || !mod->size )
> > > +            early_panic("module %d  missing /
invalid\n", nr);
> > > +    }
> > > +
> > > +    early_info.modules.nr_mods = nr_modules;
> > > +    return node;
> > > +}
> > > +
> > > +static void __init process_chosen_node(const void *fdt, int
node,
> > > +                                       const char *name,
> > > +                                       u32 address_cells, u32
size_cells)
> > > +{
> > > +    int depth;
> > > +
> > > +    for ( depth = 0;
> > > +          depth >= 0;
> > > +          node = fdt_next_node(fdt, node, &depth) )
> > > +    {
> > > +        name = fdt_get_name(fdt, node, NULL);
> > > +        if ( depth == 1 && strcmp(name,
"modules") == 0 )
> > > +            node = process_chosen_modules_node(fdt, node, name,
&depth,
> > > +                                               address_cells,
size_cells);
> > > +    }
> > > +}
> > > +
> > >  static int __init early_scan_node(const void *fdt,
> > >                                    int node, const char *name,
int depth,
> > >                                    u32 address_cells, u32
size_cells,
> > > @@ -279,6 +363,8 @@ static int __init early_scan_node(const void
*fdt,
> > >          process_memory_node(fdt, node, name, address_cells,
size_cells);
> > >      else if ( device_tree_type_matches(fdt, node,
"cpu") )
> > >          process_cpu_node(fdt, node, name, address_cells,
size_cells);
> > > +    else if ( device_tree_node_matches(fdt, node,
"chosen") )
> > > +        process_chosen_node(fdt, node, name, address_cells,
size_cells);
> > >  
> > >      return 0;
> > >  }
> > 
> > You have really written a lot of code here!
> > I would have thought that just matching on the compatible string would
> > be enough:
> > 
> > else if ( device_tree_node_matches(fdt, node,
"linux-zimage") )
> >      process_linuxzimage_node(fdt, node, name, address_cells,
size_cells);
> > else if ( device_tree_node_matches(fdt, node,
"linux-initrd") )
> >      process_linuxinitrd_node(fdt, node, name, address_cells,
size_cells);
> > 
> > so that your process_linuxzimage_node and process_linuxinitrd_node
start
> > from the right node and have everything they need to parse it
> 
> Is the tree structure of Device Tree meaningless? I''d have thought
that
> a compatible node would only have meaning at a particular place in the
> tree. Granted compatible nodes are often pretty specific and precise,
> but is that inherent enough in DT that we can rely on it?
I don''t know if it is entirely meaningless, but surely the compatible
string is regarded as a much more reliable way to identify a node AFAIK.
More often than not Linux drivers just use of_find_compatible_node to
find their DT node.

On Mon, 10 Dec 2012, Ian Campbell wrote:
> On Fri, 2012-12-07 at 17:35 +0000, Stefano Stabellini wrote:
> > On Thu, 6 Dec 2012, Ian Campbell wrote:
> > > These nodes are used by Xen to find the initial modules.
> > > 
> > > Only drop the "xen,multiboot-module" compatible nodes
in case someone
> > > else has a similar idea.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > > 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 |   40
++++++++++++++++++++++++++++++++++++++--
> > >  1 files changed, 38 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/domain_build.c
b/xen/arch/arm/domain_build.c
> > > index 7a964f7..27e02e4 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -172,6 +172,40 @@ 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
parents[DEVICE_TREE_MAX_DEPTH])
> > > +{
> > > +    int depth = *depth_out;
> > > +
> > > +    while ( (node = fdt_next_node(fdt, node, &depth))
&&
> > > +            node >= 0 && depth >= 0 )
> > > +    {
> > > +        if ( depth >= DEVICE_TREE_MAX_DEPTH )
> > > +            break;
> > > +
> > > +        parents[depth] = node;
> > > +
> > > +        /* Skip /chosen/modules/module@<N>/ and all
subnodes */
> > > +        if ( depth >= 3 &&
> > > +             device_tree_node_matches(fdt, parents[1],
"chosen") &&
> > > +             device_tree_node_matches(fdt, parents[2],
"modules") &&
> > > +             device_tree_node_matches(fdt, parents[3],
"module") &&
> > > +             fdt_node_check_compatible(fdt, parents[3],
> > > +                                      
"xen,multiboot-module" ) == 0 )
> > > +            continue;
> > > +
> > > +        /* We''ve arrived at a node which dom0 is
interested in. */
> > > +        break;
> > > +    }
> > > +
> > > +    *depth_out = depth;
> > > +    return node;
> > > +}
> > 
> > Can''t we just skip the node if it is compatible with
> > "xen,multiboot-module", no matter where it lives?  This
should simplify
> > this function greatly and you wouldn''t need the parents
parameter
> > anymore.
> 
> As well as my previous query about the meaning of the tree structure I
> think that even if we could get away with this in this particular case
> we are going to need this sort of infrastructure once we start doing
> proper filtering of dom0 vs xen nodes in the tree.
Maybe. However in that case we could just have a generic
filter_device_tree_nodes function that takes an array of strings
(compatible string? device tree paths? I would go for both in a struct,
but the former would probably suffice), and filter the DT based on
those. That would be very useful in the long run. This is very ad-hoc
for the /chosen/modules/module@<N> path.

On Mon, 2012-12-10 at 13:04 +0000, Stefano Stabellini wrote:
> > > You have really written a lot of code here!
> > > I would have thought that just matching on the compatible string
would
> > > be enough:
> > > 
> > > else if ( device_tree_node_matches(fdt, node,
"linux-zimage") )
> > >      process_linuxzimage_node(fdt, node, name, address_cells,
size_cells);
> > > else if ( device_tree_node_matches(fdt, node,
"linux-initrd") )
> > >      process_linuxinitrd_node(fdt, node, name, address_cells,
size_cells);
> > > 
> > > so that your process_linuxzimage_node and
process_linuxinitrd_node start
> > > from the right node and have everything they need to parse it
> > 
> > Is the tree structure of Device Tree meaningless? I''d have
thought that
> > a compatible node would only have meaning at a particular place in the
> > tree. Granted compatible nodes are often pretty specific and precise,
> > but is that inherent enough in DT that we can rely on it?
> 
> I don''t know if it is entirely meaningless, but surely the
compatible
> string is regarded as a much more reliable way to identify a node AFAIK.
> More often than not Linux drivers just use of_find_compatible_node to
> find their DT node.
Hrm, that sounds rather odd to me.

Anyway, there isn''t much code needed to do it right -- there''s
only ~ a
dozen lines to do with actually walking the tree. The rest (i.e. the
majority of this patch) is the internals of process_chosen_modules_node,
the docs and the data structure which would be pretty much the same
regardless of walking the tree. 

Ian.

On Mon, 2012-12-10 at 13:10 +0000, Stefano Stabellini
wrote:
> On Mon, 10 Dec 2012, Ian Campbell wrote:
> > On Fri, 2012-12-07 at 17:35 +0000, Stefano Stabellini wrote:
> > > On Thu, 6 Dec 2012, Ian Campbell wrote:
> > > > These nodes are used by Xen to find the initial modules.
> > > > 
> > > > Only drop the "xen,multiboot-module" compatible
nodes in case someone
> > > > else has a similar idea.
> > > > 
> > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > > ---
> > > > 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 |   40
++++++++++++++++++++++++++++++++++++++--
> > > >  1 files changed, 38 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/arm/domain_build.c
b/xen/arch/arm/domain_build.c
> > > > index 7a964f7..27e02e4 100644
> > > > --- a/xen/arch/arm/domain_build.c
> > > > +++ b/xen/arch/arm/domain_build.c
> > > > @@ -172,6 +172,40 @@ 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
parents[DEVICE_TREE_MAX_DEPTH])
> > > > +{
> > > > +    int depth = *depth_out;
> > > > +
> > > > +    while ( (node = fdt_next_node(fdt, node, &depth))
&&
> > > > +            node >= 0 && depth >= 0 )
> > > > +    {
> > > > +        if ( depth >= DEVICE_TREE_MAX_DEPTH )
> > > > +            break;
> > > > +
> > > > +        parents[depth] = node;
> > > > +
> > > > +        /* Skip /chosen/modules/module@<N>/ and all
subnodes */
> > > > +        if ( depth >= 3 &&
> > > > +             device_tree_node_matches(fdt, parents[1],
"chosen") &&
> > > > +             device_tree_node_matches(fdt, parents[2],
"modules") &&
> > > > +             device_tree_node_matches(fdt, parents[3],
"module") &&
> > > > +             fdt_node_check_compatible(fdt, parents[3],
> > > > +                                      
"xen,multiboot-module" ) == 0 )
> > > > +            continue;
> > > > +
> > > > +        /* We''ve arrived at a node which dom0 is
interested in. */
> > > > +        break;
> > > > +    }
> > > > +
> > > > +    *depth_out = depth;
> > > > +    return node;
> > > > +}
> > > 
> > > Can''t we just skip the node if it is compatible with
> > > "xen,multiboot-module", no matter where it lives?  This
should simplify
> > > this function greatly and you wouldn''t need the parents
parameter
> > > anymore.
> > 
> > As well as my previous query about the meaning of the tree structure I
> > think that even if we could get away with this in this particular case
> > we are going to need this sort of infrastructure once we start doing
> > proper filtering of dom0 vs xen nodes in the tree.
> 
> Maybe. However in that case we could just have a generic
> filter_device_tree_nodes function that takes an array of strings
> (compatible string? device tree paths? I would go for both in a struct,
> but the former would probably suffice), and filter the DT based on
> those. That would be very useful in the long run. This is very ad-hoc
> for the /chosen/modules/module@<N> path.
This function is precisely a filtering function as you are suggesting.
The only difference is that it does the filtering in code instead of
using a string/struct. There is nothing ad-hoc about it it just happens
that the only user right now is the module@N stuff.

I think you''d find that the code to filter based on a struct containing
a path would be more complicated and be a superset of this function,
since you would have to check the path against the same sort of parent
array.

Ian.

At 13:10 +0000 on 06 Dec (1354799442), Ian Campbell
wrote:
> Otherwise gcc complains about variables being used when not
> initialised when in fact that point is never reached.
> 
> There aren''t any instances of this in tree right now, I noticed
this
> while developing another patch.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>

On Thu, 2012-12-13 at 12:36 +0000, Tim Deegan wrote:
> At 13:10 +0000 on 06 Dec (1354799442), Ian Campbell wrote:
> > Otherwise gcc complains about variables being used when not
> > initialised when in fact that point is never reached.
> > 
> > There aren''t any instances of this in tree right now, I
noticed this
> > while developing another patch.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Tim Deegan <tim@xen.org>
Applied, thanks.