[PATCHv3 00/05] arm: pass a device tree to dom0

[PATCHv3 00/05] arm: pass a device tree to dom0

[David Vrabel]

This series of patches makes Xen pass a (somewhat) valid device tree
to dom0.  The device tree for dom0 is the same as the one supplied to
Xen except the memory and chosen nodes are adjusted appropriately.

We don't yet make use of the device tree to map MMIO regions or setup
interrupts for the guest and we still include the UART used for Xen's
console.

Note the kernel must support device tree (ATAGS are no longer provided
by Xen).

It is also possible to provide the Xen and dom0 command line via the
device tree.  This isn't as useful as it seems as Xen still needs to
be rebuilt to included the updated device tree.

The instructions on the wiki[1] have been updated to reflect these
changes.

Changes since v2:
  - dropped already applied patches
  - fix crash on boot if bootargs is missing
  - allow space in dom0's dtb for the mem reserve entry
  - bail if dom0's dtb couldn't be created
  - new patch to print warning if a node is nested too deep

Changes since v1:
  - coding style
  - move libfdt headers
  - added myself as device tree maintainer
  - fix potential infinite loop when assigning mem
  - check dtb destination more carefully
  - minor updates for clarity

David

[1] http://wiki.xen.org/wiki/Xen_ARMv7_with_Virtualization_Extensions

[PATCH 1/5] Fix test for NULL command line in cmdline_parse()

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Cc: Keir Fraser <keir@xen.org>
---
 xen/common/kernel.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 145b0b6..75e6375 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -53,11 +53,11 @@ void __init cmdline_parse(const char *cmdline)
     struct kernel_param *param;
     int bool_assert;
 
-    safe_strcpy(saved_cmdline, cmdline);
-
-    if ( p == NULL )
+    if ( cmdline == NULL )
         return;
 
+    safe_strcpy(saved_cmdline, cmdline);
+
     for ( ; ; )
     {
         /* Skip whitespace. */
-- 
1.7.2.5

[PATCH 2/5] device tree, arm: supply a flat device tree to dom0

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

Build a flat device tree for dom0 based on the one supplied to Xen.
The following changes are made:

  * In the /chosen node, the xen,dom0-bootargs parameter is renamed to
    bootargs.

  * In all memory nodes, the reg parameters are adjusted to reflect
    the amount of memory dom0 can use.  The p2m is updated using this
    info.

Support for passing ATAGS to dom0 is removed.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/arm/domain_build.c         |  257 +++++++++++++++++++++++++++++------
 xen/arch/arm/kernel.c               |    2 +-
 xen/arch/arm/kernel.h               |    8 +-
 xen/common/device_tree.c            |   47 ++++--
 xen/include/xen/device_tree.h       |    8 +
 xen/include/xen/libfdt/libfdt_env.h |    3 +
 6 files changed, 265 insertions(+), 60 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 15632f7..b4c0452 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -6,6 +6,10 @@
 #include <xen/sched.h>
 #include <asm/irq.h>
 #include <asm/regs.h>
+#include <xen/errno.h>
+#include <xen/device_tree.h>
+#include <xen/libfdt/libfdt.h>
+#include <xen/guest_access.h>
 
 #include "gic.h"
 #include "kernel.h"
@@ -13,6 +17,13 @@
 static unsigned int __initdata opt_dom0_max_vcpus;
 integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
 
+/*
+ * Amount of extra space required to dom0's device tree.  No new nodes
+ * are added (yet) but one terminating reserve map entry (16 bytes) is
+ * added.
+ */
+#define DOM0_FDT_EXTRA_SIZE (sizeof(struct fdt_reserve_entry))
+
 struct vcpu *__init alloc_dom0_vcpu0(void)
 {
     if ( opt_dom0_max_vcpus == 0 )
@@ -28,43 +39,210 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
     return alloc_vcpu(dom0, 0, 0);
 }
 
-extern void guest_mode_entry(void);
+static void set_memory_reg(struct domain *d, struct kernel_info *kinfo,
+                           const void *fdt,
+                           const u32 *cell, int address_cells, int size_cells,
+                           u32 *new_cell, int *len)
+{
+    int reg_size = (address_cells + size_cells) * sizeof(*cell);
+    int l;
+    u64 start;
+    u64 size;
+
+    l = *len;
+
+    while ( kinfo->unassigned_mem > 0 && l >= reg_size
+            && kinfo->mem.nr_banks < NR_MEM_BANKS )
+    {
+        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
+        if ( size > kinfo->unassigned_mem )
+            size = kinfo->unassigned_mem;
+
+        device_tree_set_reg(&new_cell, address_cells, size_cells, start, size);
+
+        printk("Populate P2M %#llx->%#llx\n", start, start + size);
+        p2m_populate_ram(d, start, start + size);
+        kinfo->mem.bank[kinfo->mem.nr_banks].start = start;
+        kinfo->mem.bank[kinfo->mem.nr_banks].size = size;
+        kinfo->mem.nr_banks++;
+        kinfo->unassigned_mem -= size;
+
+        l -= reg_size;
+    }
+
+    *len -= l;
+}
+
+static int write_properties(struct domain *d, struct kernel_info *kinfo,
+                            const void *fdt,
+                            int node, const char *name, int depth,
+                            u32 address_cells, u32 size_cells)
+{
+    int prop;
+
+    for ( prop = fdt_first_property_offset(fdt, node);
+          prop >= 0;
+          prop = fdt_next_property_offset(fdt, prop) )
+    {
+        const struct fdt_property *p;
+        const char *prop_name;
+        const char *prop_data;
+        int prop_len;
+        char *new_data = NULL;
+
+        p = fdt_get_property_by_offset(fdt, prop, NULL);
+        prop_name = fdt_string(fdt, fdt32_to_cpu(p->nameoff));
+        prop_data = p->data;
+        prop_len  = fdt32_to_cpu(p->len);
+
+        /*
+         * In chosen node: replace bootargs with value from
+         * 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";
+        }
+        /*
+         * In a memory node: adjust reg property.
+         */
+        else if ( device_tree_node_matches(fdt, node, "memory") )
+        {
+            if ( strcmp(prop_name, "reg") == 0 )
+            {
+                new_data = xzalloc_bytes(prop_len);
+                if ( new_data  == NULL )
+                    return -FDT_ERR_XEN(ENOMEM);
+
+                set_memory_reg(d, kinfo, fdt,
+                               (u32 *)prop_data, address_cells, size_cells,
+                               (u32 *)new_data, &prop_len);
+                prop_data = new_data;
+            }
+        }
+
+        /*
+         * TODO: Should call map_mmio_regions() for all devices in the
+         * tree that have a "reg" parameter (except cpus).  This
+         * requires looking into the parent node's "ranges" property
+         * to translate the bus address in the "reg" value into
+         * physical addresses.  Regions also need to be rounded up to
+         * whole pages.
+         */
+
+        fdt_property(kinfo->fdt, prop_name, prop_data, prop_len);
+
+        xfree(new_data);
+    }
+
+    if ( prop == -FDT_ERR_NOTFOUND )
+        return 0;
+    return prop;
+}
 
-static void setup_linux_atag(paddr_t tags, paddr_t ram_s, paddr_t ram_e)
+static int write_nodes(struct domain *d, struct kernel_info *kinfo,
+                       const void *fdt)
 {
-    paddr_t ma = gvirt_to_maddr(tags);
-    void *map = map_domain_page(ma>>PAGE_SHIFT);
-    void *p = map + (tags & (PAGE_SIZE - 1));
-    char cmdline[] = "earlyprintk=xenboot console=ttyAMA1 root=/dev/mmcblk0 debug rw";
+    int node;
+    int depth = 0, last_depth = -1;
+    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
+    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
+    int ret;
 
-    /* not enough room on this page for all the tags */
-    BUG_ON(PAGE_SIZE - (tags & (PAGE_SIZE - 1)) < 8 * sizeof(uint32_t));
+    for ( node = 0, depth = 0;
+          node >= 0 && depth >= 0;
+          node = fdt_next_node(fdt, node, &depth) )
+    {
+        const char *name;
 
-#define TAG(type, val) *(type*)p = val; p+= sizeof(type)
+        name = fdt_get_name(fdt, node, NULL);
 
-    /* ATAG_CORE */
-    TAG(uint32_t, 2);
-    TAG(uint32_t, 0x54410001);
+        if ( depth >= DEVICE_TREE_MAX_DEPTH )
+        {
+            printk("warning: node `%s' is nested too deep\n", name);
+            continue;
+        }
 
-    /* ATAG_MEM */
-    TAG(uint32_t, 4);
-    TAG(uint32_t, 0x54410002);
-    TAG(uint32_t, (ram_e - ram_s) & 0xFFFFFFFF);
-    TAG(uint32_t, ram_s & 0xFFFFFFFF);
+        while ( last_depth-- >= depth )
+            fdt_end_node(kinfo->fdt);
 
-    /* ATAG_CMDLINE */
-    TAG(uint32_t, 2 + ((strlen(cmdline) + 4) >> 2));
-    TAG(uint32_t, 0x54410009);
-    memcpy(p, cmdline, strlen(cmdline) + 1);
-    p += ((strlen(cmdline) + 4) >> 2) << 2;
+        address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells");
+        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
 
-    /* ATAG_NONE */
-    TAG(uint32_t, 0);
-    TAG(uint32_t, 0);
+        fdt_begin_node(kinfo->fdt, name);
 
-#undef TAG
+        ret = write_properties(d, kinfo, fdt, node, name, depth,
+                               address_cells[depth-1], size_cells[depth-1]);
+        if ( ret < 0 )
+            return ret;
 
-    unmap_domain_page(map);
+        last_depth = depth;
+    }
+
+    while ( last_depth-- >= 0 )
+        fdt_end_node(kinfo->fdt);
+
+    return 0;
+}
+
+static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
+{
+    void *fdt;
+    int new_size;
+    int ret;
+
+    fdt = device_tree_flattened;
+
+    new_size = fdt_totalsize(fdt) + DOM0_FDT_EXTRA_SIZE;
+    kinfo->fdt = xmalloc_bytes(new_size);
+    if ( kinfo->fdt == NULL )
+        return -ENOMEM;
+
+    ret = fdt_create(kinfo->fdt, new_size);
+    if ( ret < 0 )
+        goto err;
+
+    fdt_finish_reservemap(kinfo->fdt);
+
+    ret = write_nodes(d, kinfo, fdt);
+    if ( ret < 0 )
+        goto err;
+
+    ret = fdt_finish(kinfo->fdt);
+    if ( ret < 0 )
+        goto err;
+
+    device_tree_dump(kinfo->fdt);
+
+    /*
+     * Put the device tree at the beginning of the first bank.  It
+     * must be below 4 GiB.
+     */
+    kinfo->dtb_paddr = kinfo->mem.bank[0].start + 0x100;
+    if ( kinfo->dtb_paddr + fdt_totalsize(kinfo->fdt) > (1ull << 32) )
+    {
+        printk("Not enough memory below 4 GiB for the device tree.");
+        ret = -FDT_ERR_XEN(EINVAL);
+        goto err;
+    }
+
+    return 0;
+
+  err:
+    printk("Device tree generation failed (%d).\n", ret);
+    xfree(kinfo->fdt);
+    return -EINVAL;
+}
+
+static void dtb_load(struct kernel_info *kinfo)
+{
+    void * __user dtb_virt = (void *)(u32)kinfo->dtb_paddr;
+
+    raw_copy_to_guest(dtb_virt, kinfo->fdt, fdt_totalsize(kinfo->fdt));
+    xfree(kinfo->fdt);
 }
 
 int construct_dom0(struct domain *d)
@@ -82,22 +260,20 @@ int construct_dom0(struct domain *d)
 
     printk("*** LOADING DOMAIN 0 ***\n");
 
-    /* 128M at 2G physical */
-    /* TODO size and location from DT. */
-    kinfo.ram_start = 0x80000000;
-    kinfo.ram_end   = 0x88000000;
+    d->max_pages = ~0U;
 
-    rc = kernel_prepare(&kinfo);
-    if (rc < 0)
+    if ( (rc = p2m_alloc_table(d)) != 0 )
         return rc;
 
-    d->max_pages = ~0U;
+    kinfo.unassigned_mem = 0x08000000; /* XXX */
 
-    if ( (rc = p2m_alloc_table(d)) != 0 )
+    rc = prepare_dtb(d, &kinfo);
+    if ( rc < 0 )
         return rc;
 
-    printk("Populate P2M %#llx->%#llx\n", kinfo.ram_start, kinfo.ram_end);
-    p2m_populate_ram(d, kinfo.ram_start, kinfo.ram_end);
+    rc = kernel_prepare(&kinfo);
+    if ( rc < 0 )
+        return rc;
 
     printk("Map CS2 MMIO regions 1:1 in the P2M %#llx->%#llx\n", 0x18000000ULL, 0x1BFFFFFFULL);
     map_mmio_regions(d, 0x18000000, 0x1BFFFFFF, 0x18000000);
@@ -125,13 +301,12 @@ int construct_dom0(struct domain *d)
     /* Enable second stage translation */
     WRITE_CP32(READ_CP32(HCR) | HCR_VM, HCR); isb();
 
-    /* The following load uses domain's p2m */
+    /* The following loads use the domain's p2m */
     p2m_load_VTTBR(d);
 
+    dtb_load(&kinfo);
     kernel_load(&kinfo);
 
-    setup_linux_atag(kinfo.ram_start + 0x100, kinfo.ram_start, kinfo.ram_end);
-
     clear_bit(_VPF_down, &v->pause_flags);
 
     memset(regs, 0, sizeof(*regs));
@@ -153,7 +328,7 @@ int construct_dom0(struct domain *d)
 
     regs->r0 = 0; /* SBZ */
     regs->r1 = 2272; /* Machine NR: Versatile Express */
-    regs->r2 = kinfo.ram_start + 0x100; /* ATAGS */
+    regs->r2 = kinfo.dtb_paddr;
 
     WRITE_CP32(SCTLR_BASE, SCTLR);
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index dd757e5..130d488 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -121,7 +121,7 @@ static int kernel_try_zimage_prepare(struct kernel_info *info)
      * at 32k from start of RAM.
      */
     if (start == 0)
-        info->zimage.load_addr = info->ram_start + 0x8000;
+        info->zimage.load_addr = info->mem.bank[0].start + 0x8000;
     else
         info->zimage.load_addr = start;
     info->zimage.len = end - start;
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index 5caebe5..4533568 100644
--- a/xen/arch/arm/kernel.h
+++ b/xen/arch/arm/kernel.h
@@ -7,11 +7,15 @@
 #define __ARCH_ARM_KERNEL_H__
 
 #include <xen/libelf.h>
+#include <xen/device_tree.h>
 
 struct kernel_info {
+    void *fdt; /* flat device tree */
+    paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
+    struct dt_mem_info mem;
+
+    paddr_t dtb_paddr;
     paddr_t entry;
-    paddr_t ram_start;
-    paddr_t ram_end;
 
     void *kernel_img;
     unsigned kernel_order;
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index d4b1556..715fbf6 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -23,7 +23,7 @@
 struct dt_early_info __initdata early_info;
 void *device_tree_flattened;
 
-static bool_t __init node_matches(const void *fdt, int node, const char *match)
+bool_t device_tree_node_matches(const void *fdt, int node, const char *match)
 {
     const char *name;
     size_t match_len;
@@ -48,16 +48,33 @@ static void __init get_val(const u32 **cell, u32 cells, u64 *val)
     }
 }
 
-static void __init get_register(const u32 **cell,
-                                u32 address_cells, u32 size_cells,
-                                u64 *start, u64 *size)
+void device_tree_get_reg(const u32 **cell, u32 address_cells, u32 size_cells,
+                         u64 *start, u64 *size)
 {
     get_val(cell, address_cells, start);
     get_val(cell, size_cells, size);
 }
 
-static u32 __init prop_by_name_u32(const void *fdt, int node,
-                                   const char *prop_name)
+static void set_val(u32 **cell, u32 cells, u64 val)
+{
+    u32 c = cells;
+
+    while ( c-- )
+    {
+        (*cell)[c] = cpu_to_fdt32(val);
+        val >>= 32;
+    }
+    (*cell) += cells;
+}
+
+void device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
+                         u64 start, u64 size)
+{
+    set_val(cell, address_cells, start);
+    set_val(cell, size_cells, size);
+}
+
+u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name)
 {
     const struct fdt_property *prop;
 
@@ -68,8 +85,6 @@ static u32 __init prop_by_name_u32(const void *fdt, int node,
     return fdt32_to_cpu(*(uint32_t*)prop->data);
 }
 
-#define MAX_DEPTH 16
-
 /**
  * device_tree_for_each_node - iterate over all device tree nodes
  * @fdt: flat device tree.
@@ -81,19 +96,19 @@ int device_tree_for_each_node(const void *fdt,
 {
     int node;
     int depth;
-    u32 address_cells[MAX_DEPTH];
-    u32 size_cells[MAX_DEPTH];
+    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
+    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
     int ret;
 
     for ( node = 0, depth = 0;
           node >=0 && depth >= 0;
           node = fdt_next_node(fdt, node, &depth) )
     {
-        if ( depth >= MAX_DEPTH )
+        if ( depth >= DEVICE_TREE_MAX_DEPTH )
             continue;
 
-        address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
-        size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
+        address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells");
+        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
 
         ret = func(fdt, node, fdt_get_name(fdt, node, NULL), depth,
                    address_cells[depth-1], size_cells[depth-1], data);
@@ -106,7 +121,7 @@ int device_tree_for_each_node(const void *fdt,
 static int dump_node(const void *fdt, int node, const char *name, int depth,
                      u32 address_cells, u32 size_cells, void *data)
 {
-    char prefix[2*MAX_DEPTH + 1] = "";
+    char prefix[2*DEVICE_TREE_MAX_DEPTH + 1] = "";
     int i;
     int prop;
 
@@ -172,7 +187,7 @@ static void __init process_memory_node(const void *fdt, int node,
 
     for ( i = 0; i < banks && early_info.mem.nr_banks < NR_MEM_BANKS; i++ )
     {
-        get_register(&cell, address_cells, size_cells, &start, &size);
+        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
         early_info.mem.bank[early_info.mem.nr_banks].start = start;
         early_info.mem.bank[early_info.mem.nr_banks].size = size;
         early_info.mem.nr_banks++;
@@ -184,7 +199,7 @@ static int __init early_scan_node(const void *fdt,
                                   u32 address_cells, u32 size_cells,
                                   void *data)
 {
-    if ( node_matches(fdt, node, "memory") )
+    if ( device_tree_node_matches(fdt, node, "memory") )
         process_memory_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 b91b39f..510b5b4 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -12,6 +12,8 @@
 
 #include <xen/types.h>
 
+#define DEVICE_TREE_MAX_DEPTH 16
+
 #define NR_MEM_BANKS 8
 
 struct membank {
@@ -39,6 +41,12 @@ extern void *device_tree_flattened;
 size_t device_tree_early_init(const void *fdt);
 paddr_t device_tree_get_xen_paddr(void);
 
+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);
+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);
 void device_tree_dump(const void *fdt);
diff --git a/xen/include/xen/libfdt/libfdt_env.h b/xen/include/xen/libfdt/libfdt_env.h
index 8c0c030..2f1b03c 100644
--- a/xen/include/xen/libfdt/libfdt_env.h
+++ b/xen/include/xen/libfdt/libfdt_env.h
@@ -13,4 +13,7 @@
 #define fdt64_to_cpu(x) be64_to_cpu(x)
 #define cpu_to_fdt64(x) cpu_to_be64(x)
 
+/* Xen-specific libfdt error code. */
+#define FDT_ERR_XEN(err) (FDT_ERR_MAX + (err))
+
 #endif /* _LIBFDT_ENV_H */
-- 
1.7.2.5

[PATCH 3/5] arm: use bootargs for the command line

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

Use the /chosen node's bootargs parameter for the Xen command line.
Parse it early on before the serial console is setup.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/arm/setup.c          |    2 ++
 xen/common/device_tree.c      |   20 ++++++++++++++++++++
 xen/include/xen/device_tree.h |    1 +
 3 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 777f8fb..f89a1f7 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -152,6 +152,8 @@ void __init start_xen(unsigned long boot_phys_offset,
         + (atag_paddr & ((1 << SECOND_SHIFT) - 1));
     fdt_size = device_tree_early_init(fdt);
 
+    cmdline_parse(device_tree_bootargs(fdt));
+
     setup_pagetables(boot_phys_offset);
 
 #ifdef EARLY_UART_ADDRESS
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 715fbf6..8fb6d46 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -118,6 +118,26 @@ int device_tree_for_each_node(const void *fdt,
     return 0;
 }
 
+/**
+ * device_tree_bootargs - return the bootargs (the Xen command line)
+ * @fdt flat device tree.
+ */
+const char *device_tree_bootargs(const void *fdt)
+{
+    int node; 
+    const struct fdt_property *prop;
+
+    node = fdt_path_offset(fdt, "/chosen");
+    if ( node < 0 )
+        return NULL;
+
+    prop = fdt_get_property(fdt, node, "bootargs", NULL);
+    if ( prop == NULL )
+        return NULL;
+
+    return prop->data;
+}
+
 static int dump_node(const void *fdt, int node, const char *name, int depth,
                      u32 address_cells, u32 size_cells, void *data)
 {
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 510b5b4..8e1626c 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -49,6 +49,7 @@ u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name);
 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);
+const char *device_tree_bootargs(const void *fdt);
 void device_tree_dump(const void *fdt);
 
 #endif
-- 
1.7.2.5

[PATCH 4/5] arm: add dom0_mem command line argument

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

Add a simple dom0_mem command line argument.  It's not as flexible as
the x86 equivalent (the 'max' and 'min' prefixes are not supported).

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/arm/domain_build.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b4c0452..5a9bb80 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -17,6 +17,17 @@
 static unsigned int __initdata opt_dom0_max_vcpus;
 integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
 
+#define DOM0_MEM_DEFAULT 0x8000000 /* 128 MiB */
+static u64 __initdata dom0_mem = DOM0_MEM_DEFAULT;
+
+static void __init parse_dom0_mem(const char *s)
+{
+    dom0_mem = parse_size_and_unit(s, &s);
+    if ( dom0_mem == 0 )
+        dom0_mem = DOM0_MEM_DEFAULT;
+}
+custom_param("dom0_mem", parse_dom0_mem);
+
 /*
  * Amount of extra space required to dom0's device tree.  No new nodes
  * are added (yet) but one terminating reserve map entry (16 bytes) is
@@ -194,6 +205,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
     int new_size;
     int ret;
 
+    kinfo->unassigned_mem = dom0_mem;
+
     fdt = device_tree_flattened;
 
     new_size = fdt_totalsize(fdt) + DOM0_FDT_EXTRA_SIZE;
@@ -265,8 +278,6 @@ int construct_dom0(struct domain *d)
     if ( (rc = p2m_alloc_table(d)) != 0 )
         return rc;
 
-    kinfo.unassigned_mem = 0x08000000; /* XXX */
-
     rc = prepare_dtb(d, &kinfo);
     if ( rc < 0 )
         return rc;
-- 
1.7.2.5

[PATCH 5/5] device tree: print a warning if a node is nested too deep

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

Since device_tree_for_each_node() is called before printk() works, a
variable is used to switch between using early_printk() and printk().

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/common/device_tree.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 8fb6d46..04619f4 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -23,6 +23,10 @@
 struct dt_early_info __initdata early_info;
 void *device_tree_flattened;
 
+/* Some device tree functions may be called both before and after the
+   console is initialized. */
+static void (*dt_printk)(const char *fmt, ...) = early_printk;
+
 bool_t device_tree_node_matches(const void *fdt, int node, const char *match)
 {
     const char *name;
@@ -90,6 +94,11 @@ u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name)
  * @fdt: flat device tree.
  * @func: function to call for each node.
  * @data: data to pass to @func.
+ *
+ * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
+ *
+ * Returns 0 if all nodes were iterated over successfully.  If @func
+ * returns a negative value, that value is returned immediately.
  */
 int device_tree_for_each_node(const void *fdt,
                               device_tree_node_func func, void *data)
@@ -104,13 +113,19 @@ int device_tree_for_each_node(const void *fdt,
           node >=0 && depth >= 0;
           node = fdt_next_node(fdt, node, &depth) )
     {
+        const char *name = fdt_get_name(fdt, node, NULL);
+
         if ( depth >= DEVICE_TREE_MAX_DEPTH )
+        {
+            dt_printk("Warning: device tree node `%s' is nested too deep\n",
+                      name);
             continue;
+        }
 
         address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells");
         size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
 
-        ret = func(fdt, node, fdt_get_name(fdt, node, NULL), depth,
+        ret = func(fdt, node, name, depth,
                    address_cells[depth-1], size_cells[depth-1], data);
         if ( ret < 0 )
             return ret;
@@ -253,6 +268,8 @@ size_t __init device_tree_early_init(const void *fdt)
     device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
     early_print_info();
 
+    dt_printk = printk;
+
     return fdt_totalsize(fdt);
 }
 
-- 
1.7.2.5

Re: [PATCH 2/5] device tree, arm: supply a flat device tree to dom0

[Ian Campbell]

On Thu, 2012-03-22 at 19:17 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Build a flat device tree for dom0 based on the one supplied to Xen.
> The following changes are made:
> 
>   * In the /chosen node, the xen,dom0-bootargs parameter is renamed to
>     bootargs.
> 
>   * In all memory nodes, the reg parameters are adjusted to reflect
>     the amount of memory dom0 can use.  The p2m is updated using this
>     info.
> 
> Support for passing ATAGS to dom0 is removed.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  xen/arch/arm/domain_build.c         |  257 +++++++++++++++++++++++++++++------
>  xen/arch/arm/kernel.c               |    2 +-
>  xen/arch/arm/kernel.h               |    8 +-
>  xen/common/device_tree.c            |   47 ++++--
>  xen/include/xen/device_tree.h       |    8 +
>  xen/include/xen/libfdt/libfdt_env.h |    3 +
>  6 files changed, 265 insertions(+), 60 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 15632f7..b4c0452 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -6,6 +6,10 @@
>  #include <xen/sched.h>
>  #include <asm/irq.h>
>  #include <asm/regs.h>
> +#include <xen/errno.h>
> +#include <xen/device_tree.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/guest_access.h>
> 
>  #include "gic.h"
>  #include "kernel.h"
> @@ -13,6 +17,13 @@
>  static unsigned int __initdata opt_dom0_max_vcpus;
>  integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
> 
> +/*
> + * Amount of extra space required to dom0's device tree.  No new nodes
> + * are added (yet) but one terminating reserve map entry (16 bytes) is
> + * added.
> + */
> +#define DOM0_FDT_EXTRA_SIZE (sizeof(struct fdt_reserve_entry))
> +
>  struct vcpu *__init alloc_dom0_vcpu0(void)
>  {
>      if ( opt_dom0_max_vcpus == 0 )
> @@ -28,43 +39,210 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
>      return alloc_vcpu(dom0, 0, 0);
>  }
> 
> -extern void guest_mode_entry(void);
> +static void set_memory_reg(struct domain *d, struct kernel_info *kinfo,
> +                           const void *fdt,
> +                           const u32 *cell, int address_cells, int size_cells,
> +                           u32 *new_cell, int *len)
> +{
> +    int reg_size = (address_cells + size_cells) * sizeof(*cell);
> +    int l;
> +    u64 start;
> +    u64 size;
> +
> +    l = *len;

> +
> +    while ( kinfo->unassigned_mem > 0 && l >= reg_size
> +            && kinfo->mem.nr_banks < NR_MEM_BANKS )
> +    {
> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +        if ( size > kinfo->unassigned_mem )
> +            size = kinfo->unassigned_mem;
> +
> +        device_tree_set_reg(&new_cell, address_cells, size_cells, start, size);

This assumes/requires that address_cells and size_cells a 1, right? If
they end up being 2 or more then device_tree_get_reg will trample on the
stack via &start and &size.

> +
> +        printk("Populate P2M %#llx->%#llx\n", start, start + size);
> +        p2m_populate_ram(d, start, start + size);
> +        kinfo->mem.bank[kinfo->mem.nr_banks].start = start;
> +        kinfo->mem.bank[kinfo->mem.nr_banks].size = size;
> +        kinfo->mem.nr_banks++;
> +        kinfo->unassigned_mem -= size;
> +
> +        l -= reg_size;
> +    }
> +
> +    *len -= l;

I had a bit of trouble following the logic wrt l and the updating of
*len in this function.

AIUI *len is initially the size of the original cell. The loop is
looping over the length of the cell, using l as the iterator, and
duplicating each entry into the new cell, while also populating the p2m
and the bank list as it goes. So far so good.

But then *len is updated by subtracting whatever l remained of the
original cells length. It seems that the caller is using it as the
length of the new cell, and the maths does seem to work out that way but
it seems rather convoluted compared with e.g. just returning the length
of the new cell.

> +}
> +
[...]
> +static void set_val(u32 **cell, u32 cells, u64 val)
> +{
> +    u32 c = cells;
> +
> +    while ( c-- )
> +    {
> +        (*cell)[c] = cpu_to_fdt32(val);
> +        val >>= 32;

I guess cells is never more than 2 then?

> +    }
> +    (*cell) += cells;
> +}
> +
> +void device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
> +                         u64 start, u64 size)
> +{
> +    set_val(cell, address_cells, start);
> +    set_val(cell, size_cells, size);
> +}
> +
> +u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name)
>  {
>      const struct fdt_property *prop;
> 
> @@ -68,8 +85,6 @@ static u32 __init prop_by_name_u32(const void *fdt, int node,
>      return fdt32_to_cpu(*(uint32_t*)prop->data);
>  }
> 
> -#define MAX_DEPTH 16
> -
>  /**
>   * device_tree_for_each_node - iterate over all device tree nodes
>   * @fdt: flat device tree.
> @@ -81,19 +96,19 @@ int device_tree_for_each_node(const void *fdt,
>  {
>      int node;
>      int depth;
> -    u32 address_cells[MAX_DEPTH];
> -    u32 size_cells[MAX_DEPTH];
> +    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
> +    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
>      int ret;
> 
>      for ( node = 0, depth = 0;
>            node >=0 && depth >= 0;
>            node = fdt_next_node(fdt, node, &depth) )
>      {
> -        if ( depth >= MAX_DEPTH )
> +        if ( depth >= DEVICE_TREE_MAX_DEPTH )
>              continue;
> 
> -        address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
> -        size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
> +        address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells");
> +        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
> 
>          ret = func(fdt, node, fdt_get_name(fdt, node, NULL), depth,
>                     address_cells[depth-1], size_cells[depth-1], data);
> @@ -106,7 +121,7 @@ int device_tree_for_each_node(const void *fdt,
>  static int dump_node(const void *fdt, int node, const char *name, int depth,
>                       u32 address_cells, u32 size_cells, void *data)
>  {
> -    char prefix[2*MAX_DEPTH + 1] = "";
> +    char prefix[2*DEVICE_TREE_MAX_DEPTH + 1] = "";
>      int i;
>      int prop;
> 
> @@ -172,7 +187,7 @@ static void __init process_memory_node(const void *fdt, int node,
> 
>      for ( i = 0; i < banks && early_info.mem.nr_banks < NR_MEM_BANKS; i++ )
>      {
> -        get_register(&cell, address_cells, size_cells, &start, &size);
> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>          early_info.mem.bank[early_info.mem.nr_banks].start = start;
>          early_info.mem.bank[early_info.mem.nr_banks].size = size;
>          early_info.mem.nr_banks++;
> @@ -184,7 +199,7 @@ static int __init early_scan_node(const void *fdt,
>                                    u32 address_cells, u32 size_cells,
>                                    void *data)
>  {
> -    if ( node_matches(fdt, node, "memory") )
> +    if ( device_tree_node_matches(fdt, node, "memory") )
>          process_memory_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 b91b39f..510b5b4 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -12,6 +12,8 @@
> 
>  #include <xen/types.h>
> 
> +#define DEVICE_TREE_MAX_DEPTH 16
> +
>  #define NR_MEM_BANKS 8
> 
>  struct membank {
> @@ -39,6 +41,12 @@ extern void *device_tree_flattened;
>  size_t device_tree_early_init(const void *fdt);
>  paddr_t device_tree_get_xen_paddr(void);
> 
> +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);
> +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);
>  void device_tree_dump(const void *fdt);
> diff --git a/xen/include/xen/libfdt/libfdt_env.h b/xen/include/xen/libfdt/libfdt_env.h
> index 8c0c030..2f1b03c 100644
> --- a/xen/include/xen/libfdt/libfdt_env.h
> +++ b/xen/include/xen/libfdt/libfdt_env.h
> @@ -13,4 +13,7 @@
>  #define fdt64_to_cpu(x) be64_to_cpu(x)
>  #define cpu_to_fdt64(x) cpu_to_be64(x)
> 
> +/* Xen-specific libfdt error code. */
> +#define FDT_ERR_XEN(err) (FDT_ERR_MAX + (err))

Looks like the only user is FDT_ERR_XEN(ENOMEM) which could as well be
FDT_ERR_NOSPACE?

FWIW I think adding a new error code would be a fair reason to diverge
in a controlled way from the pristine upstream source.

Ian.

Re: [PATCH 3/5] arm: use bootargs for the command line

[Ian Campbell]

On Thu, 2012-03-22 at 19:17 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Use the /chosen node's bootargs parameter for the Xen command line.
> Parse it early on before the serial console is setup.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  xen/arch/arm/setup.c          |    2 ++
>  xen/common/device_tree.c      |   20 ++++++++++++++++++++
>  xen/include/xen/device_tree.h |    1 +
>  3 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 777f8fb..f89a1f7 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -152,6 +152,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>          + (atag_paddr & ((1 << SECOND_SHIFT) - 1));
>      fdt_size = device_tree_early_init(fdt);
>  
> +    cmdline_parse(device_tree_bootargs(fdt));
> +
>      setup_pagetables(boot_phys_offset);
>  
>  #ifdef EARLY_UART_ADDRESS
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 715fbf6..8fb6d46 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -118,6 +118,26 @@ int device_tree_for_each_node(const void *fdt,
>      return 0;
>  }
>  
> +/**
> + * device_tree_bootargs - return the bootargs (the Xen command line)
> + * @fdt flat device tree.
> + */
> +const char *device_tree_bootargs(const void *fdt)
> +{
> +    int node; 
> +    const struct fdt_property *prop;
> +
> +    node = fdt_path_offset(fdt, "/chosen");
> +    if ( node < 0 )
> +        return NULL;
> +
> +    prop = fdt_get_property(fdt, node, "bootargs", NULL);
> +    if ( prop == NULL )
> +        return NULL;
> +
> +    return prop->data;
> +}
> +
>  static int dump_node(const void *fdt, int node, const char *name, int depth,
>                       u32 address_cells, u32 size_cells, void *data)
>  {
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 510b5b4..8e1626c 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -49,6 +49,7 @@ u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name);
>  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);
> +const char *device_tree_bootargs(const void *fdt);
>  void device_tree_dump(const void *fdt);
>  
>  #endif

Re: [PATCH 4/5] arm: add dom0_mem command line argument

[Ian Campbell]

On Thu, 2012-03-22 at 19:17 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Add a simple dom0_mem command line argument.  It's not as flexible as
> the x86 equivalent (the 'max' and 'min' prefixes are not supported).
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  xen/arch/arm/domain_build.c |   15 +++++++++++++--
>  1 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b4c0452..5a9bb80 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -17,6 +17,17 @@
>  static unsigned int __initdata opt_dom0_max_vcpus;
>  integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
>  
> +#define DOM0_MEM_DEFAULT 0x8000000 /* 128 MiB */
> +static u64 __initdata dom0_mem = DOM0_MEM_DEFAULT;
> +
> +static void __init parse_dom0_mem(const char *s)
> +{
> +    dom0_mem = parse_size_and_unit(s, &s);
> +    if ( dom0_mem == 0 )
> +        dom0_mem = DOM0_MEM_DEFAULT;
> +}
> +custom_param("dom0_mem", parse_dom0_mem);
> +
>  /*
>   * Amount of extra space required to dom0's device tree.  No new nodes
>   * are added (yet) but one terminating reserve map entry (16 bytes) is
> @@ -194,6 +205,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>      int new_size;
>      int ret;
>  
> +    kinfo->unassigned_mem = dom0_mem;
> +
>      fdt = device_tree_flattened;
>  
>      new_size = fdt_totalsize(fdt) + DOM0_FDT_EXTRA_SIZE;
> @@ -265,8 +278,6 @@ int construct_dom0(struct domain *d)
>      if ( (rc = p2m_alloc_table(d)) != 0 )
>          return rc;
>  
> -    kinfo.unassigned_mem = 0x08000000; /* XXX */
> -
>      rc = prepare_dtb(d, &kinfo);
>      if ( rc < 0 )
>          return rc;

Re: [PATCH 2/5] device tree, arm: supply a flat device tree to dom0

[David Vrabel]

On 30/03/12 16:59, Ian Campbell wrote:
> On Thu, 2012-03-22 at 19:17 +0000, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Build a flat device tree for dom0 based on the one supplied to Xen.
>> The following changes are made:
>>
>>   * In the /chosen node, the xen,dom0-bootargs parameter is renamed to
>>     bootargs.
>>
>>   * In all memory nodes, the reg parameters are adjusted to reflect
>>     the amount of memory dom0 can use.  The p2m is updated using this
>>     info.
>>
>> Support for passing ATAGS to dom0 is removed.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>  xen/arch/arm/domain_build.c         |  257 +++++++++++++++++++++++++++++------
>>  xen/arch/arm/kernel.c               |    2 +-
>>  xen/arch/arm/kernel.h               |    8 +-
>>  xen/common/device_tree.c            |   47 ++++--
>>  xen/include/xen/device_tree.h       |    8 +
>>  xen/include/xen/libfdt/libfdt_env.h |    3 +
>>  6 files changed, 265 insertions(+), 60 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 15632f7..b4c0452 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -6,6 +6,10 @@
>>  #include <xen/sched.h>
>>  #include <asm/irq.h>
>>  #include <asm/regs.h>
>> +#include <xen/errno.h>
>> +#include <xen/device_tree.h>
>> +#include <xen/libfdt/libfdt.h>
>> +#include <xen/guest_access.h>
>>
>>  #include "gic.h"
>>  #include "kernel.h"
>> @@ -13,6 +17,13 @@
>>  static unsigned int __initdata opt_dom0_max_vcpus;
>>  integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
>>
>> +/*
>> + * Amount of extra space required to dom0's device tree.  No new nodes
>> + * are added (yet) but one terminating reserve map entry (16 bytes) is
>> + * added.
>> + */
>> +#define DOM0_FDT_EXTRA_SIZE (sizeof(struct fdt_reserve_entry))
>> +
>>  struct vcpu *__init alloc_dom0_vcpu0(void)
>>  {
>>      if ( opt_dom0_max_vcpus == 0 )
>> @@ -28,43 +39,210 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
>>      return alloc_vcpu(dom0, 0, 0);
>>  }
>>
>> -extern void guest_mode_entry(void);
>> +static void set_memory_reg(struct domain *d, struct kernel_info *kinfo,
>> +                           const void *fdt,
>> +                           const u32 *cell, int address_cells, int size_cells,
>> +                           u32 *new_cell, int *len)
>> +{
>> +    int reg_size = (address_cells + size_cells) * sizeof(*cell);
>> +    int l;
>> +    u64 start;
>> +    u64 size;
>> +
>> +    l = *len;
> 
>> +
>> +    while ( kinfo->unassigned_mem > 0 && l >= reg_size
>> +            && kinfo->mem.nr_banks < NR_MEM_BANKS )
>> +    {
>> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>> +        if ( size > kinfo->unassigned_mem )
>> +            size = kinfo->unassigned_mem;
>> +
>> +        device_tree_set_reg(&new_cell, address_cells, size_cells, start, size);
> 
> This assumes/requires that address_cells and size_cells a 1, right? If
> they end up being 2 or more then device_tree_get_reg will trample on the
> stack via &start and &size.

No.  address_cells and size_cells can be 1 or 2.  This is enough for 64
addresses.

>> +
>> +        printk("Populate P2M %#llx->%#llx\n", start, start + size);
>> +        p2m_populate_ram(d, start, start + size);
>> +        kinfo->mem.bank[kinfo->mem.nr_banks].start = start;
>> +        kinfo->mem.bank[kinfo->mem.nr_banks].size = size;
>> +        kinfo->mem.nr_banks++;
>> +        kinfo->unassigned_mem -= size;
>> +
>> +        l -= reg_size;
>> +    }
>> +
>> +    *len -= l;
> 
> I had a bit of trouble following the logic wrt l and the updating of
> *len in this function.

I've updated this as per your suggestion.

>> --- a/xen/include/xen/libfdt/libfdt_env.h
>> +++ b/xen/include/xen/libfdt/libfdt_env.h
>> @@ -13,4 +13,7 @@
>>  #define fdt64_to_cpu(x) be64_to_cpu(x)
>>  #define cpu_to_fdt64(x) cpu_to_be64(x)
>>
>> +/* Xen-specific libfdt error code. */
>> +#define FDT_ERR_XEN(err) (FDT_ERR_MAX + (err))
> 
> Looks like the only user is FDT_ERR_XEN(ENOMEM) which could as well be
> FDT_ERR_NOSPACE?

No.  FDT_ERR_NOSPACE means the buffer isn't large enough to add new
nodes etc.  This needs to be a different error code from a memory
allocation failure.

> FWIW I think adding a new error code would be a fair reason to diverge
> in a controlled way from the pristine upstream source.

I don't know what you're asking for here.

David

Re: [PATCH 2/5] device tree, arm: supply a flat device tree to dom0

[Ian Campbell]

On Mon, 2012-04-02 at 10:21 +0100, David Vrabel wrote:
> On 30/03/12 16:59, Ian Campbell wrote:
> > On Thu, 2012-03-22 at 19:17 +0000, David Vrabel wrote:
> >> From: David Vrabel <david.vrabel@citrix.com>
> >>
> >> Build a flat device tree for dom0 based on the one supplied to Xen.
> >> The following changes are made:
> >>
> >>   * In the /chosen node, the xen,dom0-bootargs parameter is renamed to
> >>     bootargs.
> >>
> >>   * In all memory nodes, the reg parameters are adjusted to reflect
> >>     the amount of memory dom0 can use.  The p2m is updated using this
> >>     info.
> >>
> >> Support for passing ATAGS to dom0 is removed.
> >>
> >> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> >> ---
> >>  xen/arch/arm/domain_build.c         |  257 +++++++++++++++++++++++++++++------
> >>  xen/arch/arm/kernel.c               |    2 +-
> >>  xen/arch/arm/kernel.h               |    8 +-
> >>  xen/common/device_tree.c            |   47 ++++--
> >>  xen/include/xen/device_tree.h       |    8 +
> >>  xen/include/xen/libfdt/libfdt_env.h |    3 +
> >>  6 files changed, 265 insertions(+), 60 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >> index 15632f7..b4c0452 100644
> >> --- a/xen/arch/arm/domain_build.c
> >> +++ b/xen/arch/arm/domain_build.c
> >> @@ -6,6 +6,10 @@
> >>  #include <xen/sched.h>
> >>  #include <asm/irq.h>
> >>  #include <asm/regs.h>
> >> +#include <xen/errno.h>
> >> +#include <xen/device_tree.h>
> >> +#include <xen/libfdt/libfdt.h>
> >> +#include <xen/guest_access.h>
> >>
> >>  #include "gic.h"
> >>  #include "kernel.h"
> >> @@ -13,6 +17,13 @@
> >>  static unsigned int __initdata opt_dom0_max_vcpus;
> >>  integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
> >>
> >> +/*
> >> + * Amount of extra space required to dom0's device tree.  No new nodes
> >> + * are added (yet) but one terminating reserve map entry (16 bytes) is
> >> + * added.
> >> + */
> >> +#define DOM0_FDT_EXTRA_SIZE (sizeof(struct fdt_reserve_entry))
> >> +
> >>  struct vcpu *__init alloc_dom0_vcpu0(void)
> >>  {
> >>      if ( opt_dom0_max_vcpus == 0 )
> >> @@ -28,43 +39,210 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
> >>      return alloc_vcpu(dom0, 0, 0);
> >>  }
> >>
> >> -extern void guest_mode_entry(void);
> >> +static void set_memory_reg(struct domain *d, struct kernel_info *kinfo,
> >> +                           const void *fdt,
> >> +                           const u32 *cell, int address_cells, int size_cells,
> >> +                           u32 *new_cell, int *len)
> >> +{
> >> +    int reg_size = (address_cells + size_cells) * sizeof(*cell);
> >> +    int l;
> >> +    u64 start;
> >> +    u64 size;
> >> +
> >> +    l = *len;
> > 
> >> +
> >> +    while ( kinfo->unassigned_mem > 0 && l >= reg_size
> >> +            && kinfo->mem.nr_banks < NR_MEM_BANKS )
> >> +    {
> >> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> >> +        if ( size > kinfo->unassigned_mem )
> >> +            size = kinfo->unassigned_mem;
> >> +
> >> +        device_tree_set_reg(&new_cell, address_cells, size_cells, start, size);
> > 
> > This assumes/requires that address_cells and size_cells a 1, right? If
> > they end up being 2 or more then device_tree_get_reg will trample on the
> > stack via &start and &size.
> 
> No.  address_cells and size_cells can be 1 or 2.

Where is that checked or enforced? What happens if someone feeds us a
DTB with 3? Or are you saying that this isn't possible (perhaps due to
some constraint in DTB itself)?

> >> --- a/xen/include/xen/libfdt/libfdt_env.h
> >> +++ b/xen/include/xen/libfdt/libfdt_env.h
> >> @@ -13,4 +13,7 @@
> >>  #define fdt64_to_cpu(x) be64_to_cpu(x)
> >>  #define cpu_to_fdt64(x) cpu_to_be64(x)
> >>
> >> +/* Xen-specific libfdt error code. */
> >> +#define FDT_ERR_XEN(err) (FDT_ERR_MAX + (err))
> > 
> > Looks like the only user is FDT_ERR_XEN(ENOMEM) which could as well be
> > FDT_ERR_NOSPACE?
> 
> No.  FDT_ERR_NOSPACE means the buffer isn't large enough to add new
> nodes etc.  This needs to be a different error code from a memory
> allocation failure.

OK.

> > FWIW I think adding a new error code would be a fair reason to diverge
> > in a controlled way from the pristine upstream source.
> 
> I don't know what you're asking for here.

I was suggesting that it would be fine to just add a new error code,
IMHO it's a reasonable reason to diverge from upstream. Could perhaps
even send them the patch?

Ian.

Re: [PATCH 5/5] device tree: print a warning if a node is nested too deep

[Ian Campbell]

On Thu, 2012-03-22 at 19:17 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Since device_tree_for_each_node() is called before printk() works, a
> variable is used to switch between using early_printk() and printk().
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(although I separately wonder if generic printk ought not to redirect to
early_printk early on)

>  xen/common/device_tree.c |   19 ++++++++++++++++++-
>  1 files changed, 18 insertions(+), 1 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 8fb6d46..04619f4 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -23,6 +23,10 @@
>  struct dt_early_info __initdata early_info;
>  void *device_tree_flattened;
>  
> +/* Some device tree functions may be called both before and after the
> +   console is initialized. */
> +static void (*dt_printk)(const char *fmt, ...) = early_printk;
> +
>  bool_t device_tree_node_matches(const void *fdt, int node, const char *match)
>  {
>      const char *name;
> @@ -90,6 +94,11 @@ u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name)
>   * @fdt: flat device tree.
>   * @func: function to call for each node.
>   * @data: data to pass to @func.
> + *
> + * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
> + *
> + * Returns 0 if all nodes were iterated over successfully.  If @func
> + * returns a negative value, that value is returned immediately.
>   */
>  int device_tree_for_each_node(const void *fdt,
>                                device_tree_node_func func, void *data)
> @@ -104,13 +113,19 @@ int device_tree_for_each_node(const void *fdt,
>            node >=0 && depth >= 0;
>            node = fdt_next_node(fdt, node, &depth) )
>      {
> +        const char *name = fdt_get_name(fdt, node, NULL);
> +
>          if ( depth >= DEVICE_TREE_MAX_DEPTH )
> +        {
> +            dt_printk("Warning: device tree node `%s' is nested too deep\n",
> +                      name);
>              continue;
> +        }
>  
>          address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells");
>          size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
>  
> -        ret = func(fdt, node, fdt_get_name(fdt, node, NULL), depth,
> +        ret = func(fdt, node, name, depth,
>                     address_cells[depth-1], size_cells[depth-1], data);
>          if ( ret < 0 )
>              return ret;
> @@ -253,6 +268,8 @@ size_t __init device_tree_early_init(const void *fdt)
>      device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
>      early_print_info();
>  
> +    dt_printk = printk;
> +
>      return fdt_totalsize(fdt);
>  }
>  

Re: [PATCH 2/5] device tree, arm: supply a flat device tree to dom0

[David Vrabel]

On 02/04/12 10:30, Ian Campbell wrote:
> On Mon, 2012-04-02 at 10:21 +0100, David Vrabel wrote:
>> On 30/03/12 16:59, Ian Campbell wrote:
>>> On Thu, 2012-03-22 at 19:17 +0000, David Vrabel wrote:
>>>> From: David Vrabel <david.vrabel@citrix.com>
>>>>
>>>> Build a flat device tree for dom0 based on the one supplied to Xen.
>>>> The following changes are made:
>>>>
>>>>   * In the /chosen node, the xen,dom0-bootargs parameter is renamed to
>>>>     bootargs.
>>>>
>>>>   * In all memory nodes, the reg parameters are adjusted to reflect
>>>>     the amount of memory dom0 can use.  The p2m is updated using this
>>>>     info.
>>>>
>>>> Support for passing ATAGS to dom0 is removed.
>>>>
>>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>>>> ---
>>>>  xen/arch/arm/domain_build.c         |  257 +++++++++++++++++++++++++++++------
>>>>  xen/arch/arm/kernel.c               |    2 +-
>>>>  xen/arch/arm/kernel.h               |    8 +-
>>>>  xen/common/device_tree.c            |   47 ++++--
>>>>  xen/include/xen/device_tree.h       |    8 +
>>>>  xen/include/xen/libfdt/libfdt_env.h |    3 +
>>>>  6 files changed, 265 insertions(+), 60 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>> index 15632f7..b4c0452 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -6,6 +6,10 @@
>>>>  #include <xen/sched.h>
>>>>  #include <asm/irq.h>
>>>>  #include <asm/regs.h>
>>>> +#include <xen/errno.h>
>>>> +#include <xen/device_tree.h>
>>>> +#include <xen/libfdt/libfdt.h>
>>>> +#include <xen/guest_access.h>
>>>>
>>>>  #include "gic.h"
>>>>  #include "kernel.h"
>>>> @@ -13,6 +17,13 @@
>>>>  static unsigned int __initdata opt_dom0_max_vcpus;
>>>>  integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
>>>>
>>>> +/*
>>>> + * Amount of extra space required to dom0's device tree.  No new nodes
>>>> + * are added (yet) but one terminating reserve map entry (16 bytes) is
>>>> + * added.
>>>> + */
>>>> +#define DOM0_FDT_EXTRA_SIZE (sizeof(struct fdt_reserve_entry))
>>>> +
>>>>  struct vcpu *__init alloc_dom0_vcpu0(void)
>>>>  {
>>>>      if ( opt_dom0_max_vcpus == 0 )
>>>> @@ -28,43 +39,210 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
>>>>      return alloc_vcpu(dom0, 0, 0);
>>>>  }
>>>>
>>>> -extern void guest_mode_entry(void);
>>>> +static void set_memory_reg(struct domain *d, struct kernel_info *kinfo,
>>>> +                           const void *fdt,
>>>> +                           const u32 *cell, int address_cells, int size_cells,
>>>> +                           u32 *new_cell, int *len)
>>>> +{
>>>> +    int reg_size = (address_cells + size_cells) * sizeof(*cell);
>>>> +    int l;
>>>> +    u64 start;
>>>> +    u64 size;
>>>> +
>>>> +    l = *len;
>>>
>>>> +
>>>> +    while ( kinfo->unassigned_mem > 0 && l >= reg_size
>>>> +            && kinfo->mem.nr_banks < NR_MEM_BANKS )
>>>> +    {
>>>> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>>>> +        if ( size > kinfo->unassigned_mem )
>>>> +            size = kinfo->unassigned_mem;
>>>> +
>>>> +        device_tree_set_reg(&new_cell, address_cells, size_cells, start, size);
>>>
>>> This assumes/requires that address_cells and size_cells a 1, right? If
>>> they end up being 2 or more then device_tree_get_reg will trample on the
>>> stack via &start and &size.
>>
>> No.  address_cells and size_cells can be 1 or 2.
> 
> Where is that checked or enforced? What happens if someone feeds us a
> DTB with 3? Or are you saying that this isn't possible (perhaps due to
> some constraint in DTB itself)?

I just tested with #address-cells == 3 and it works fine.

We trust the DTB is a valid description of the hardware so assume that
all addresses are below 2^64.

David