Xen devel - Aug 2013 - [RFC 00/24] Allow Xen to boot with a raw Device Tree

Hi,

This is the first patch series to allow Xen to boot with a raw device tree, ie
without any specific modification for Xen.

Few months ago, the patch series "Support multiple ARM platform in
XEN", has
added a tree structure to handle easier the device tree. Each node can carry
metadata to specify if the node can be pass-through to Dom0. Therefore, this
patch series take advantage of the "new" Device Tree API and get a rid
of
the FDT API that is currently used to build Dom0 dts and in some drivers.

There is few items that was not added in this series:
    * Set the correct number of CPUs for Dom0 by editing CPUs node
    * Remove and create the GIC node
    * Remove and create the timer node

I hope to add support for previous items in the next patch series.

At the beginning of the mail, I a bit lied about Xen is able to raw DTS :).
The latter, still need to carry Xen and Linux command line until U-Boot/Grub
will support multiboot2 protocol.
I have noticed that U-boot has some fdt commands to change on the fly the device
tree. I think we can use this solution as a workaround for the beginning.
I will send an email later to explain the procedure.

Cheers.

Julien Grall (24):
  xen/char: dt-uart: Allow the user to give a path to the node
  xen: Introduce __initconst to store initial const data
  xen/dts: Don''t check the number of address and size cells in
    process_cpu_node
  xen/dts: Constify device_tree_flattened
  xen/arm: Move __PSCI* from traps.c to the header
  xen: Add new string functions
  xen: Use the right string comparison function in device tree
  xen/dts: Don''t add a fake property "name" in the device
tree
  xen/dts: Add new helpers to use the device tree
  xen/dts: Remove device_get_reg call in process_memory_node
  xen/dts: Remove device_get_reg call in process_cpu_node
  xen/dts: Remove device_get_reg call in process_multiboot_node
  xen/dts: Check the CPU ID is not greater than NR_CPUS
  xen/video: hdlcd: Convert the driver to the new device tree API
  xen/arm: Use dt_device_match to avoid multiple if conditions
  xen/arm: Build DOM0 FDT by browsing the device tree structure
  xen/arm: Mark each device used by Xen as disabled in DOM0 FDT
  xen/arm: Don''t map disabled device in DOM0
  xen/arm: Create a fake PSCI node in dom0 device tree
  xen/arm: Add new platform specific callback device_is_blacklist
  xen/arm: vexpress: Blacklist a list of board specific devices
  xen/arm: exynos5: Blacklist MCT device
  xen/dts: Clean up the exported API for device tree
  xen/arm: Check if the device is available before using it

 xen/arch/arm/device.c             |    3 +
 xen/arch/arm/domain_build.c       |  313 ++++++++++++++++++-------------------
 xen/arch/arm/platform.c           |   10 ++
 xen/arch/arm/platforms/exynos5.c  |   11 ++
 xen/arch/arm/platforms/vexpress.c |   17 ++
 xen/arch/arm/setup.c              |    6 +-
 xen/arch/arm/time.c               |   11 +-
 xen/arch/arm/traps.c              |    5 -
 xen/common/device_tree.c          |  273 ++++++++++++++++----------------
 xen/common/string.c               |   15 ++
 xen/drivers/char/dt-uart.c        |   16 +-
 xen/drivers/video/arm_hdlcd.c     |   58 ++++---
 xen/include/asm-arm/platform.h    |    7 +
 xen/include/asm-arm/psci.h        |    5 +
 xen/include/xen/device_tree.h     |  156 +++++++++++++++---
 xen/include/xen/init.h            |    1 +
 xen/include/xen/string.h          |   14 ++
 17 files changed, 565 insertions(+), 356 deletions(-)

-- 
1.7.10.4

On some board, there is no alias to the UART. To avoid modification in
the device tree, dt-uart should also search device by path.

To distinguish an alias from a path, dt-uart will check the first character.
If it''s a / then it''s path, otherwise it''s an alias.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/drivers/char/dt-uart.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
index 93bb0f5..d7204fb 100644
--- a/xen/drivers/char/dt-uart.c
+++ b/xen/drivers/char/dt-uart.c
@@ -26,9 +26,10 @@
 
 /*
  * Configure UART port with a string:
- * alias,options
+ * path,options
  *
- * @alias: alias used in the device tree for the UART
+ * @path: full path used in the device tree for the UART. If the path
+ * doesn''t start with ''/'', we assuming that
it''s an alias.
  * @options: UART speficic options (see in each UART driver)
  */
 static char __initdata opt_dtuart[30] = "";
@@ -38,7 +39,7 @@ void __init dt_uart_init(void)
 {
     struct dt_device_node *dev;
     int ret;
-    const char *devalias = opt_dtuart;
+    const char *devpath = opt_dtuart;
     char *options;
 
     if ( !console_has("dtuart") || !strcmp(opt_dtuart, "")
)
@@ -53,12 +54,15 @@ void __init dt_uart_init(void)
     else
         options = "";
 
-    early_printk("Looking for UART console %s\n", devalias);
-    dev = dt_find_node_by_alias(devalias);
+    early_printk("Looking for UART console %s\n", devpath);
+    if ( *devpath == ''/'' )
+        dev = dt_find_node_by_path(devpath);
+    else
+        dev = dt_find_node_by_alias(devpath);
 
     if ( !dev )
     {
-        early_printk("Unable to find device \"%s\"\n",
devalias);
+        early_printk("Unable to find device \"%s\"\n",
devpath);
         return;
     }
 
-- 
1.7.10.4

It''s possible to have 2 type (const and non-const) of data in the same
compilation unit. Using only __initdata will result to a compilation error:

    error: $variablename causes as section tupe conflict with $variablename2

because a section containing const variables is marked read only and so cannot
contain non-const variables.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/include/xen/init.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index b602577..9d481b3 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -10,6 +10,7 @@
 #define __init            __text_section(".init.text")
 #define __exit            __text_section(".exit.text")
 #define __initdata        __section(".init.data")
+#define __initconst       __section(".init.rodata")
 #define __exitdata        __used_section(".exit.data")
 #define __initsetup       __used_section(".init.setup")
 #define __init_call(lvl)  __used_section(".initcall" lvl
".init")
-- 
1.7.10.4

The properties #address-cells and #size-cells is not required in /cpus node in
the device tree (see Linux Documentation/devicetree/booting-without-of.txt
Section III.5.a).

In the OMAP5 device tree, these 2 properties are not defined. Therefore, Xen
will only able to handle 1 CPU.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
CC: andrii.anisov@globallogic.com
CC: baozich@gmail.com
---
 xen/common/device_tree.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 84d704d..51f23eb 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -414,12 +414,6 @@ static void __init process_cpu_node(const void *fdt, int
node,
     const u32 *cell;
     paddr_t start, size;
 
-    if ( address_cells != 1 || size_cells != 0 )
-    {
-        early_printk("fdt: node `%s'': invalid #address-cells or
#size-cells",
-                     name);
-        return;
-    }
 
     prop = fdt_get_property(fdt, node, "reg", NULL);
     if ( !prop )
-- 
1.7.10.4

The Flat Device Tree is given by the bootloader. Xen doesn''t need to
modify it.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain_build.c   |    2 +-
 xen/arch/arm/setup.c          |    6 ++++--
 xen/common/device_tree.c      |    2 +-
 xen/include/xen/device_tree.h |    2 +-
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 01492bb..a324c3b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -475,7 +475,7 @@ static int map_devices_from_device_tree(struct domain *d)
 
 static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
 {
-    void *fdt;
+    const void *fdt;
     int new_size;
     int ret;
     paddr_t end;
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 1ec5e38..c9534fd 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -289,6 +289,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t
dtb_size)
     unsigned long dtb_pages;
     unsigned long boot_mfn_start, boot_mfn_end;
     int i = 0;
+    void *fdt;
 
     /* TODO: Handle non-contiguous memory bank */
     if ( !early_info.mem.nr_banks )
@@ -363,8 +364,9 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t
dtb_size)
      *
      * TODO: handle other payloads too.
      */
-    device_tree_flattened = mfn_to_virt(alloc_boot_pages(dtb_pages, 1));
-    copy_from_paddr(device_tree_flattened, dtb_paddr, dtb_size, BUFFERABLE);
+    fdt = mfn_to_virt(alloc_boot_pages(dtb_pages, 1));
+    copy_from_paddr(fdt, dtb_paddr, dtb_size, BUFFERABLE);
+    device_tree_flattened = fdt;
 
     /* Add non-xenheap memory */
     s = ram_start;
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 51f23eb..b96ae33 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -26,7 +26,7 @@
 #include <asm/early_printk.h>
 
 struct dt_early_info __initdata early_info;
-void *device_tree_flattened;
+const void *device_tree_flattened;
 dt_irq_xlate_func dt_irq_xlate;
 /* Host device tree */
 struct dt_device_node *dt_host;
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 5a2a5c6..7cd9e19 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -158,7 +158,7 @@ typedef int (*device_tree_node_func)(const void *fdt,
                                      void *data);
 
 extern struct dt_early_info early_info;
-extern void *device_tree_flattened;
+extern const void *device_tree_flattened;
 
 size_t __init device_tree_early_init(const void *fdt);
 
-- 
1.7.10.4

These defines will be used to create the fake PSCI node in dom0 device tree.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/traps.c       |    5 -----
 xen/include/asm-arm/psci.h |    5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 6b5fa51..26b3c24 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -847,11 +847,6 @@ static arm_hypercall_t arm_hypercall_table[] = {
     HYPERCALL_ARM(vcpu_op, 3),
 };
 
-#define __PSCI_cpu_suspend 0
-#define __PSCI_cpu_off     1
-#define __PSCI_cpu_on      2
-#define __PSCI_migrate     3
-
 typedef int (*arm_psci_fn_t)(uint32_t, register_t);
 
 typedef struct {
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index 67d4c35..fdba636 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -6,6 +6,11 @@
 #define PSCI_EINVAL  -2
 #define PSCI_DENIED  -3
 
+#define __PSCI_cpu_suspend 0
+#define __PSCI_cpu_off     1
+#define __PSCI_cpu_on      2
+#define __PSCI_migrate     3
+
 int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
 int do_psci_cpu_off(uint32_t power_state);
 int do_psci_cpu_suspend(uint32_t power_state, register_t entry_point);
-- 
1.7.10.4

Add kbasename and strcasecmp. The code is copied from Linux.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/common/string.c      |   15 +++++++++++++++
 xen/include/xen/string.h |   14 ++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/xen/common/string.c b/xen/common/string.c
index db9d9d5..9a5a4ba 100644
--- a/xen/common/string.c
+++ b/xen/common/string.c
@@ -41,6 +41,21 @@ int strnicmp(const char *s1, const char *s2, size_t len)
 }
 #endif
 
+#ifndef __HAVE_ARCH_STRCASECMP
+int strcasecmp(const char *s1, const char *s2)
+{
+    int c1, c2;
+
+    do
+    {
+        c1 = tolower(*s1++);
+        c2 = tolower(*s2++);
+    } while ( c1 == c2 && c1 != 0 );
+
+    return c1 - c2;
+}
+#endif
+
 #ifndef __HAVE_ARCH_STRLCPY
 /**
  * strlcpy - Copy a %NUL terminated string into a sized buffer
diff --git a/xen/include/xen/string.h b/xen/include/xen/string.h
index f26b994..9b7511e 100644
--- a/xen/include/xen/string.h
+++ b/xen/include/xen/string.h
@@ -43,6 +43,9 @@ extern int strncmp(const char *,const char *,__kernel_size_t);
 #ifndef __HAVE_ARCH_STRNICMP
 extern int strnicmp(const char *, const char *, __kernel_size_t);
 #endif
+#ifndef __HAVE_ARCH_STRCASECMP
+extern int strcasecmp(const char *, const char *);
+#endif
 #ifndef __HAVE_ARCH_STRCHR
 extern char * strchr(const char *,int);
 #endif
@@ -94,4 +97,15 @@ extern void * memchr(const void *,int,__kernel_size_t);
     (strlcat(d, s, sizeof(d)) >= sizeof(d));    \
 })
 
+/**
+ * kbasename - return the last part of a pathname.
+ *
+ * @path: path to extract the filename from.
+ */
+static inline const char *kbasename(const char *path)
+{
+    const char *tail = strrchr(path, ''/'');
+    return tail ? tail + 1 : path;
+}
+
 #endif /* _LINUX_STRING_H_ */
-- 
1.7.10.4

When of_node_cmp and of_compat_cmp was introduced in commit fb97eb6
"xen/arm: Create a hierarchical device tree", they were copied from
the wrong
Linux header.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/common/device_tree.c      |    6 +++---
 xen/include/xen/device_tree.h |    4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index b96ae33..362dd66 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -144,7 +144,7 @@ bool_t __init device_tree_node_compatible(const void *fdt,
int node,
         return 0;
 
     while ( len > 0 ) {
-        if ( !dt_compat_cmp(prop, match, mlen) )
+        if ( !dt_compat_cmp(prop, match) )
             return 1;
         l = strlen(prop) + 1;
         prop += l;
@@ -549,7 +549,7 @@ dt_find_property(const struct dt_device_node *np,
 
     for ( pp = np->properties; pp; pp = pp->next )
     {
-        if ( strcmp(pp->name, name) == 0 )
+        if ( dt_prop_cmp(pp->name, name) == 0 )
         {
             if ( lenp )
                 *lenp = pp->length;
@@ -579,7 +579,7 @@ bool_t dt_device_is_compatible(const struct dt_device_node
*device,
         return 0;
     while ( cplen > 0 )
     {
-        if ( dt_compat_cmp(cp, compat, strlen(compat)) == 0 )
+        if ( dt_compat_cmp(cp, compat) == 0 )
             return 1;
         l = strlen(cp) + 1;
         cp += l;
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 7cd9e19..e6a82cb 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -223,8 +223,8 @@ extern const struct dt_device_node *dt_interrupt_controller;
  */
 struct dt_device_node * __init dt_find_interrupt_controller(const char
*compat);
 
-#define dt_node_cmp(s1, s2) strcmp((s1), (s2))
-#define dt_compat_cmp(s1, s2, l) strnicmp((s1), (s2), l)
+#define dt_node_cmp(s1, s2) strcasecmp((s1), (s2))
+#define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2))
 
 /* Default #address and #size cells */
 #define DT_ROOT_NODE_ADDR_CELLS_DEFAULT 1
-- 
1.7.10.4

On new Flat Device Tree version, the property "name" may not exist.
The property is never used in Xen code except to set the field "name"
of
dt_device_node.

For convenience, remove the fake property. It will save space during the
creation of the dom0 FDT.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/common/device_tree.c |   21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 362dd66..315b284 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -1528,6 +1528,7 @@ static unsigned long __init unflatten_dt_node(const void
*fdt,
     if ( !has_name )
     {
         char *p1 = pathp, *ps = pathp, *pa = NULL;
+        char *tmp = NULL;
         int sz;
 
         while ( *p1 )
@@ -1541,25 +1542,21 @@ static unsigned long __init unflatten_dt_node(const void
*fdt,
         if ( pa < ps )
             pa = p1;
         sz = (pa - ps) + 1;
-        pp = unflatten_dt_alloc(&mem, sizeof(struct dt_property) + sz,
-                                __alignof__(struct dt_property));
+
+        tmp = unflatten_dt_alloc(&mem, sz, 1);
         if ( allnextpp )
         {
-            pp->name = "name";
-            pp->length = sz;
-            pp->value = pp + 1;
-            *prev_pp = pp;
-            prev_pp = &pp->next;
-            memcpy(pp->value, ps, sz - 1);
-            ((char *)pp->value)[sz - 1] = 0;
-            dt_dprintk("fixed up name for %s -> %s\n", pathp,
-                       (char *)pp->value);
+            memcpy(tmp, ps, sz - 1);
+            np->name = tmp;
+            tmp[sz - 1] = 0;
+            dt_dprintk("fixed up name for %s -> %s\n", pathp,
np->name);
         }
     }
+
     if ( allnextpp )
     {
         *prev_pp = NULL;
-        np->name = dt_get_property(np, "name", NULL);
+        np->name = (np->name) ? : dt_get_property(np, "name",
NULL);
         np->type = dt_get_property(np, "device_type", NULL);
 
         if ( !np->name )
-- 
1.7.10.4

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/common/device_tree.c      |  102 ++++++++++++++++++++++++++++---
 xen/include/xen/device_tree.h |  135 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 228 insertions(+), 9 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 315b284..95635f0 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -182,23 +182,31 @@ void __init device_tree_get_reg(const u32 **cell, u32
address_cells,
     get_val(cell, size_cells, size);
 }
 
-static void __init set_val(u32 **cell, u32 cells, u64 val)
+void dt_set_cell(__be32 **cellp, int size, u64 val)
 {
-    u32 c = cells;
+    int cells = size;
 
-    while ( c-- )
+    while ( size-- )
     {
-        (*cell)[c] = cpu_to_fdt32(val);
+        (*cellp)[size] = cpu_to_fdt32(val);
         val >>= 32;
     }
-    (*cell) += cells;
+
+    (*cellp) += cells;
 }
 
 void __init 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);
+    dt_set_cell(cell, address_cells, start);
+    dt_set_cell(cell, size_cells, size);
+}
+
+void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
+                  u64 address, u64 size)
+{
+    dt_set_cell(cellp, dt_n_addr_cells(np), address);
+    dt_set_cell(cellp, dt_n_size_cells(np), size);
 }
 
 u32 __init device_tree_get_u32(const void *fdt, int node, const char
*prop_name,
@@ -568,6 +576,23 @@ const void *dt_get_property(const struct dt_device_node
*np,
     return pp ? pp->value : NULL;
 }
 
+int dt_property_read_string(const struct dt_device_node *np,
+                            const char *propname, const char **out_string)
+{
+    const struct dt_property *pp = dt_find_property(np, propname, NULL);
+
+    if ( !pp )
+        return -EINVAL;
+    if ( !pp->value )
+        return -ENODATA;
+    if ( strnlen(pp->value, pp->length) >= pp->length )
+        return -EILSEQ;
+
+    *out_string = pp->value;
+
+    return 0;
+}
+
 bool_t dt_device_is_compatible(const struct dt_device_node *device,
                                const char *compat)
 {
@@ -640,6 +665,34 @@ struct dt_device_node *dt_find_node_by_alias(const char
*alias)
     return NULL;
 }
 
+bool_t dt_match_node(const struct dt_device_match *matches,
+                     const struct dt_device_node *node)
+{
+    if ( !matches )
+        return 0;
+
+    while ( matches->path || matches->type || matches->compatible )
+    {
+        bool_t match = 1;
+
+        if ( matches->path )
+            match &= dt_node_path_is_equal(node, matches->path);
+
+        if ( matches->type )
+            match &= dt_device_type_is_equal(node, matches->type);
+
+        if ( matches->compatible )
+            match &= dt_device_is_compatible(node, matches->compatible);
+
+        if ( match )
+            return match;
+
+        matches++;
+    }
+
+    return 0;
+}
+
 const struct dt_device_node *dt_get_parent(const struct dt_device_node *node)
 {
     if ( !node )
@@ -669,6 +722,23 @@ dt_find_compatible_node(struct dt_device_node *from,
     return np;
 }
 
+struct dt_device_node *
+dt_find_matching_node(struct dt_device_node *from,
+                      const struct dt_device_match *matches)
+{
+    struct dt_device_node *np;
+    struct dt_device_node *dt;
+
+    dt = from ? from->allnext : dt_host;
+    for_each_device_node(dt, np)
+    {
+        if ( dt_match_node(matches, np) )
+            return np;
+    }
+
+    return NULL;
+}
+
 int dt_n_addr_cells(const struct dt_device_node *np)
 {
     const __be32 *ip;
@@ -1357,6 +1427,24 @@ int dt_device_get_irq(const struct dt_device_node
*device, int index,
     return dt_irq_translate(&raw, out_irq);
 }
 
+bool_t dt_device_is_available(const struct dt_device_node *device)
+{
+    const char *status;
+    u32 statlen;
+
+    status = dt_get_property(device, "status", &statlen);
+    if ( status == NULL )
+        return 1;
+
+    if ( statlen > 0 )
+    {
+        if ( !strcmp(status, "okay") || !strcmp(status,
"ok") )
+            return 1;
+    }
+
+    return 0;
+}
+
 /**
  * unflatten_dt_node - Alloc and populate a device_node from the flat tree
  * @fdt: The parent device tree blob
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index e6a82cb..22a2973 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -48,6 +48,19 @@ struct dt_early_info {
     struct dt_module_info modules;
 };
 
+/*
+ * Struct used for matching a device
+ */
+struct dt_device_match {
+    const char *path;
+    const char *type;
+    const char *compatible;
+};
+
+#define DT_MATCH_PATH(p)                { .path = p }
+#define DT_MATCH_TYPE(typ)              { .type = typ }
+#define DT_MATCH_COMPATIBLE(compat)     { .compatible = compat }
+
 typedef u32 dt_phandle;
 
 /**
@@ -223,6 +236,7 @@ extern const struct dt_device_node *dt_interrupt_controller;
  */
 struct dt_device_node * __init dt_find_interrupt_controller(const char
*compat);
 
+#define dt_prop_cmp(s1, s2) strcmp((s1), (s2))
 #define dt_node_cmp(s1, s2) strcasecmp((s1), (s2))
 #define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2))
 
@@ -246,6 +260,20 @@ static inline u64 dt_read_number(const __be32 *cell, int
size)
     return r;
 }
 
+/* Helper to convert a number of cells in bytes */
+static inline int dt_cells_to_size(int size)
+{
+    return (size * sizeof (u32));
+}
+
+static inline u64 dt_next_cell(int s, const __be32 **cellp)
+{
+    const __be32 *p = *cellp;
+
+    *cellp = p + s;
+    return dt_read_number(p, s);
+}
+
 static inline const char *dt_node_full_name(const struct dt_device_node *np)
 {
     return (np && np->full_name) ? np->full_name :
"<no-node>";
@@ -256,6 +284,18 @@ static inline const char *dt_node_name(const struct
dt_device_node *np)
     return (np && np->name) ? np->name :
"<no-node>";
 }
 
+static inline bool_t dt_node_name_is_equal(const struct dt_device_node *np,
+                                           const char *name)
+{
+    return !dt_node_cmp(np->name, name);
+}
+
+static inline bool_t dt_node_path_is_equal(const struct dt_device_node *np,
+                                           const char *path)
+{
+    return !dt_node_cmp(np->full_name, path);
+}
+
 static inline bool_t
 dt_device_type_is_equal(const struct dt_device_node *device,
                         const char *type)
@@ -275,6 +315,12 @@ static inline domid_t dt_device_used_by(const struct
dt_device_node *device)
     return device->used_by;
 }
 
+static inline bool_t dt_property_name_is_equal(const struct dt_property *pp,
+                                               const char *name)
+{
+    return !dt_prop_cmp(pp->name, name);
+}
+
 /**
  * dt_find_compatible_node - Find a node based on type and one of the
  *                           tokens in its "compatible" property
@@ -295,11 +341,29 @@ struct dt_device_node *dt_find_compatible_node(struct
dt_device_node *from,
 /**
  * Find a property with a given name for a given node
  * and return the value.
- */
+  */
 const void *dt_get_property(const struct dt_device_node *np,
                             const char *name, u32 *lenp);
 
 /**
+ * dt_property_read_string - Find and read a string from a property
+ * @np:         Device node from which the property value is to be read
+ * @propname:   Name of the property to be searched
+ * @out_string: Pointer to null terminated return string, modified only
+ *              if return value if 0.
+ *
+ * Search for a property in a device tree node and retrieve a null
+ * terminated string value (pointer to data, not a copy). Returns 0 on
+ * sucess, -EINVAL if the property does not exist, -ENODATA if property
+ * doest not have value, and -EILSEQ if the string is not
+ * null-terminated with the length of the property data.
+ *
+ * The out_string pointer is modifed only if a valid string can be decoded.
+ */
+int dt_property_read_string(const struct dt_device_node *np,
+                            const char *propname, const char **out_string);
+
+/**
  * Checks if the given "compat" string matches one of the strings in
  * the device''s "compatible" property
  */
@@ -433,4 +497,71 @@ int dt_n_size_cells(const struct dt_device_node *np);
  */
 int dt_n_addr_cells(const struct dt_device_node *np);
 
-#endif
+/**
+ * dt_device_is_available - Check if a device is available for use
+ *
+ * @device: Node to check for availability
+ *
+ * Returns true if the status property is absent or set to "okay" or
"ok",
+ * false otherwise.
+ */
+bool_t dt_device_is_available(const struct dt_device_node *device);
+
+/**
+ * dt_match_node - Tell if a device_node has a matching of dt_device_match
+ * @matches: array of dt_device_match structures to search in
+ * @node: the dt_device_node structure to match against
+ *
+ * Returns true if the device node match one of dt_device_match.
+ */
+bool_t dt_match_node(const struct dt_device_match *matches,
+                     const struct dt_device_node *node);
+
+/**
+ * dt_find_matching_node - Find a node based on an dt_device_match match table
+ * @from: The node to start searching from or NULL, the node you pass
+ *        will not be searched, only the next one will; typically, you pass
+ *        what the returned call returned
+ * @matches: array of dt_device_match structures to search in
+ *
+ * Returns a node pointer.
+ */
+struct dt_device_node *
+dt_find_matching_node(struct dt_device_node *from,
+                      const struct dt_device_match *matches);
+
+/**
+ * dt_set_cell - Write a value into a serie of cells
+ *
+ * @cellp: Pointer to cells
+ *
+ * Write a value into a series of cells and update cellp to point to the
+ * cell just after.
+ */
+void dt_set_cell(__be32 **cellp, int size, u64 val);
+
+/**
+ * dt_set_range - Write range into a serie of cells
+ *
+ * @cellp: Pointer to cells
+ * @np: Node which contains the encoding for the address and
+ * the size
+ * @address: Start of range
+ * @size: Size of the range
+ *
+ * Write a range into a serie of cells and update cellp to point to the
+ * cell just after.
+ */
+void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
+                  u64 address, u64 size);
+
+#endif /* __XEN_DEVICE_TREE_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.7.10.4

The function device_get_reg will be removed in a future patch.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/common/device_tree.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 95635f0..ea01a5a 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -385,7 +385,7 @@ static void __init process_memory_node(const void *fdt, int
node,
     const struct fdt_property *prop;
     int i;
     int banks;
-    const u32 *cell;
+    const __be32 *cell;
     paddr_t start, size;
 
     if ( address_cells < 1 || size_cells < 1 )
@@ -402,12 +402,13 @@ static void __init process_memory_node(const void *fdt,
int node,
         return;
     }
 
-    cell = (const u32 *)prop->data;
+    cell = (const __be32 *)prop->data;
     banks = device_tree_nr_reg_ranges(prop, address_cells, size_cells);
 
     for ( i = 0; i < banks && early_info.mem.nr_banks <
NR_MEM_BANKS; i++ )
     {
-        device_tree_get_reg(&cell, address_cells, size_cells, &start,
&size);
+        start = dt_next_cell(address_cells, &cell);
+        size = dt_next_cell(size_cells, &cell);
         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++;
-- 
1.7.10.4

The function device_get_reg will be removed in a future patch.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/common/device_tree.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index ea01a5a..433fa37 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -420,21 +420,26 @@ static void __init process_cpu_node(const void *fdt, int
node,
                                     u32 address_cells, u32 size_cells)
 {
     const struct fdt_property *prop;
-    const u32 *cell;
-    paddr_t start, size;
-
+    u32 cpuid;
+    int len;
 
-    prop = fdt_get_property(fdt, node, "reg", NULL);
+    prop = fdt_get_property(fdt, node, "reg", &len);
     if ( !prop )
     {
         early_printk("fdt: node `%s'': missing `reg''
property\n", name);
         return;
     }
 
-    cell = (const u32 *)prop->data;
-    device_tree_get_reg(&cell, address_cells, size_cells, &start,
&size);
+    if ( len < sizeof (cpuid) )
+    {
+        dt_printk("fdt: node `%s'': `reg` property length is too
short\n",
+                  name);
+        return;
+    }
+
+    cpuid = dt_read_number((const __be32 *)prop->data, 1);
 
-    cpumask_set_cpu(start, &cpu_possible_map);
+    cpumask_set_cpu(cpuid, &cpu_possible_map);
 }
 
 static void __init process_multiboot_node(const void *fdt, int node,
-- 
1.7.10.4

The function device_get_reg will be removed in a future patch.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/common/device_tree.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 433fa37..8a5bfc3 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -447,7 +447,7 @@ static void __init process_multiboot_node(const void *fdt,
int node,
                                           u32 address_cells, u32 size_cells)
 {
     const struct fdt_property *prop;
-    const u32 *cell;
+    const __be32 *cell;
     int nr;
     struct dt_mb_module *mod;
     int len;
@@ -465,9 +465,9 @@ static void __init process_multiboot_node(const void *fdt,
int node,
     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);
+    cell = (const __be32 *)prop->data;
+    mod->start = dt_next_cell(address_cells, &cell);
+    mod->size = dt_next_cell(size_cells, &cell);
 
     prop = fdt_get_property(fdt, node, "bootargs", &len);
     if ( prop )
-- 
1.7.10.4

On some board CPU IDs are not contiguous (for instance the Versatile Express
with big.LITTLE supports). If the CPU ID is greater than NR_CPUS Xen will hang
without any message. This is because console driver is not yet initialized and
hypervisor data abort uses printk.

For the moment check the CPU ID and print an warning if an error occured.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/common/device_tree.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 8a5bfc3..6bdab14 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -439,6 +439,13 @@ static void __init process_cpu_node(const void *fdt, int
node,
 
     cpuid = dt_read_number((const __be32 *)prop->data, 1);
 
+    /* TODO: handle non-contiguous CPU ID */
+    if ( cpuid >= NR_CPUS )
+    {
+        dt_printk("fdt: node `%s'': reg(0x%x) >=
NR_CPUS(%d)\n",
+                  name, cpuid, NR_CPUS);
+        return;
+    }
     cpumask_set_cpu(cpuid, &cpu_possible_map);
 }
 
-- 
1.7.10.4

Avoid to use FDT API which will be removed soon.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/drivers/video/arm_hdlcd.c |   58 +++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/xen/drivers/video/arm_hdlcd.c b/xen/drivers/video/arm_hdlcd.c
index 72979ea..0ff8c22 100644
--- a/xen/drivers/video/arm_hdlcd.c
+++ b/xen/drivers/video/arm_hdlcd.c
@@ -25,6 +25,7 @@
 #include <xen/libfdt/libfdt.h>
 #include <xen/init.h>
 #include <xen/mm.h>
+#include <asm/early_printk.h>
 #include "font.h"
 #include "lfb.h"
 #include "modelines.h"
@@ -96,62 +97,67 @@ static int __init get_color_masks(const char* bpp, struct
color_masks **masks)
 
 static void __init set_pixclock(uint32_t pixclock)
 {
-    if ( device_tree_node_compatible(device_tree_flattened, 0,
"arm,vexpress") )
+    if ( dt_find_compatible_node(NULL, NULL, "arm,vexpress") )
             vexpress_syscfg(1, V2M_SYS_CFG_OSC_FUNC,
                             V2M_SYS_CFG_OSC5, &pixclock);
 }
 
 void __init video_init(void)
 {
-    int node, depth;
-    u32 address_cells, size_cells;
     struct lfb_prop lfbp;
     unsigned char *lfb;
-    paddr_t hdlcd_start, hdlcd_size;
+    u64 hdlcd_start, hdlcd_size;
     paddr_t framebuffer_start, framebuffer_size;
-    const struct fdt_property *prop;
-    const u32 *cell;
     const char *mode_string;
     char _mode_string[16];
     int bytes_per_pixel = 4;
     struct color_masks *c = NULL;
     struct modeline *videomode = NULL;
     int i;
+    const struct dt_device_node *dev;
+    const __be32 *cells;
+    u32 lenp;
+    int res;
 
-    if ( find_compatible_node("arm,hdlcd", &node, &depth,
-                &address_cells, &size_cells) <= 0 )
-        return;
+    dev = dt_find_compatible_node(NULL, NULL, "arm,hdlcd");
 
-    prop = fdt_get_property(device_tree_flattened, node, "reg",
NULL);
-    if ( !prop )
+    if ( !dev )
+    {
+        early_printk("hdlcd: Cannot find node compatible with
\"arm,hdcld\"\n");
         return;
+    }
 
-    cell = (const u32 *)prop->data;
-    device_tree_get_reg(&cell, address_cells, size_cells,
-            &hdlcd_start, &hdlcd_size);
+    res = dt_device_get_address(dev, 0, &hdlcd_start, &hdlcd_size);
+    if ( !res )
+    {
+        early_printk("hdlcd: Unable to retrieve MMIO base
address\n");
+        return;
+    }
 
-    prop = fdt_get_property(device_tree_flattened, node,
"framebuffer", NULL);
-    if ( !prop )
+    cells = dt_get_property(dev, "framebuffer", &lenp);
+    if ( !cells )
+    {
+        early_printk("hdlcd: Unable to retrieve framebuffer
property\n");
         return;
+    }
 
-    cell = (const u32 *)prop->data;
-    device_tree_get_reg(&cell, address_cells, size_cells,
-            &framebuffer_start, &framebuffer_size);
+    framebuffer_start = dt_next_cell(dt_n_addr_cells(dev), &cells);
+    framebuffer_size = dt_next_cell(dt_n_size_cells(dev), &cells);
 
     if ( !hdlcd_start )
     {
-        printk(KERN_ERR "HDLCD address missing from device tree, disabling
driver\n");
+        early_printk("hdlcd: address missing from device tree, disabling
driver\n");
         return;
     }
 
     if ( !hdlcd_start || !framebuffer_start )
     {
-        printk(KERN_ERR "HDLCD: framebuffer address missing from device
tree, disabling driver\n");
+        printk("hdlcd: framebuffer address missing from device tree,
disabling driver\n");
         return;
     }
 
-    mode_string = fdt_getprop(device_tree_flattened, node, "mode",
NULL);
-    if ( !mode_string )
+    res = dt_property_read_string(dev, "mode", &mode_string);
+    if ( res )
     {
         get_color_masks("32", &c);
         memcpy(_mode_string, "1280x1024@60",
strlen("1280x1024@60") + 1);
@@ -160,14 +166,14 @@ void __init video_init(void)
     else if ( strlen(mode_string) < strlen("800x600@60") ||
             strlen(mode_string) > sizeof(_mode_string) - 1 )
     {
-        printk(KERN_ERR "HDLCD: invalid modeline=%s\n", mode_string);
+        early_printk("hdlcd: invalid modeline=%s\n", mode_string);
         return;
     } else {
         char *s = strchr(mode_string, ''-'');
         if ( !s )
         {
-            printk(KERN_INFO "HDLCD: bpp not found in modeline %s, assume
32 bpp\n",
-                    mode_string);
+            early_printk("hdcld: bpp not found in modeline %s, assume 32
bpp\n",
+                         mode_string);
             get_color_masks("32", &c);
             memcpy(_mode_string, mode_string, strlen(mode_string) + 1);
             bytes_per_pixel = 4;
-- 
1.7.10.4

There is some place in Xen ARM code where multiple if conditions is used
check the presence of a node or find a node.
These pieces of code can be replace by an array and using proper device tree
helpers.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain_build.c |   12 +++++++++---
 xen/arch/arm/time.c         |   11 ++++++++---
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index a324c3b..604ec1c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -435,6 +435,14 @@ static int map_device(struct domain *d, const struct
dt_device_node *dev)
     return 0;
 }
 
+static const struct dt_device_match skip_matches[] __initconst +{
+    DT_MATCH_COMPATIBLE("xen,xen"),
+    DT_MATCH_TYPE("memory"),
+    DT_MATCH_PATH("/chosen"),
+    { /* sentinel */ },
+};
+
 static int handle_node(struct domain *d, const struct dt_device_node *np)
 {
     const struct dt_device_node *child;
@@ -443,9 +451,7 @@ static int handle_node(struct domain *d, const struct
dt_device_node *np)
     DPRINT("handle %s\n", dt_node_full_name(np));
 
     /* Skip theses nodes and the sub-nodes */
-    if ( dt_device_is_compatible(np, "xen,xen") ||
-         dt_device_type_is_equal(np, "memory") ||
-         !strcmp("/chosen", dt_node_full_name(np)) )
+    if ( dt_match_node(skip_matches, np ) )
         return 0;
 
     if ( dt_device_used_by(np) != DOMID_XEN )
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index b5864ef..05e58ba 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -98,6 +98,13 @@ static uint32_t calibrate_timer(void)
 }
 */
 
+static const struct dt_device_match timer_ids[] __initdata +{
+    DT_MATCH_COMPATIBLE("arm,armv7-timer"),
+    DT_MATCH_COMPATIBLE("arm,armv8-timer"),
+    { /* sentinel */ },
+};
+
 /* Set up the timer on the boot CPU */
 int __init init_xen_time(void)
 {
@@ -105,9 +112,7 @@ int __init init_xen_time(void)
     int res;
     unsigned int i;
 
-    dev = dt_find_compatible_node(NULL, NULL, "arm,armv7-timer");
-    if ( !dev )
-        dev = dt_find_compatible_node(NULL, NULL, "arm,armv8-timer");
+    dev = dt_find_matching_node(NULL, timer_ids);
     if ( !dev )
         panic("Unable to find a compatible timer in the device
tree\n");
 
-- 
1.7.10.4

Remove the usage of the FDT in benefit of the device tree structure.
The latter is easier to use and can embedded meta-data for Xen (ie: is the
device is used by Xen...).

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain_build.c |  270 ++++++++++++++++---------------------------
 1 file changed, 101 insertions(+), 169 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 604ec1c..c8f24ed 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -63,10 +63,10 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
 }
 
 static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo,
-                             const void *fdt, const u32 *cell, int len,
-                             int address_cells, int size_cells, u32 *new_cell)
+                             const struct dt_property *pp,
+                             const struct dt_device_node *np, __be32 *new_cell)
 {
-    int reg_size = (address_cells + size_cells) * sizeof(*cell);
+    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np));
     paddr_t start;
     paddr_t size;
     struct page_info *pg;
@@ -90,7 +90,7 @@ static int set_memory_reg_11(struct domain *d, struct
kernel_info *kinfo,
     if ( res )
         panic("Unable to add pages in DOM0: %d\n", res);
 
-    device_tree_set_reg(&new_cell, address_cells, size_cells, start, size);
+    dt_set_range(&new_cell, np, start, size);
 
     kinfo->mem.bank[0].start = start;
     kinfo->mem.bank[0].size = size;
@@ -100,25 +100,30 @@ static int set_memory_reg_11(struct domain *d, struct
kernel_info *kinfo,
 }
 
 static int set_memory_reg(struct domain *d, struct kernel_info *kinfo,
-                          const void *fdt, const u32 *cell, int len,
-                          int address_cells, int size_cells, u32 *new_cell)
+                          const struct dt_property *pp,
+                          const struct dt_device_node *np, __be32 *new_cell)
 {
-    int reg_size = (address_cells + size_cells) * sizeof(*cell);
+    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np));
     int l = 0;
+    unsigned int bank = 0;
     u64 start;
     u64 size;
+    int ret;
 
     if ( platform_has_quirk(PLATFORM_QUIRK_DOM0_MAPPING_11) )
-        return set_memory_reg_11(d, kinfo, fdt, cell, len, address_cells,
-                                 size_cells, new_cell);
+        return set_memory_reg_11(d, kinfo, pp, np, new_cell);
 
-    while ( kinfo->unassigned_mem > 0 && l + reg_size <= len
+    while ( kinfo->unassigned_mem > 0 && l + reg_size <=
pp->length
             && kinfo->mem.nr_banks < NR_MEM_BANKS )
     {
-        device_tree_get_reg(&cell, address_cells, size_cells, &start,
&size);
+        ret = dt_device_get_address(np, bank, &start, &size);
+        if ( ret )
+            panic("Unable to retrieve the bank %u for %s\n",
+                  bank, dt_node_full_name(np));
+
         if ( size > kinfo->unassigned_mem )
             size = kinfo->unassigned_mem;
-        device_tree_set_reg(&new_cell, address_cells, size_cells, start,
size);
+        dt_set_range(&new_cell, np, start, size);
 
         printk("Populate P2M
%#"PRIx64"->%#"PRIx64"\n", start, start + size);
         if ( p2m_populate_ram(d, start, start + size) < 0 )
@@ -135,31 +140,21 @@ static int set_memory_reg(struct domain *d, struct
kernel_info *kinfo,
 }
 
 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)
+                            const struct dt_device_node *np)
 {
     const char *bootargs = NULL;
-    int prop;
+    const struct dt_property *pp;
+    int res = 0;
 
     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) )
+    for_each_property_of_node (np, pp)
     {
-        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);
+        const void *prop_data = pp->value;
+        void *new_data = NULL;
+        u32 prop_len = pp->length;
 
         /*
          * In chosen node:
@@ -168,105 +163,72 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
          *   bootargs (from module #1, above).
          * * remove bootargs and xen,dom0-bootargs.
          */
-        if ( device_tree_node_matches(fdt, node, "chosen") )
+        if ( dt_node_path_is_equal(np, "/chosen") )
         {
-            if ( strcmp(prop_name, "bootargs") == 0 )
+            if ( dt_property_name_is_equal(pp, "bootargs") )
                 continue;
-            else if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
+            else if ( dt_property_name_is_equal(pp,
"xen,dom0-bootargs") )
             {
                 if ( !bootargs )
-                    bootargs = prop_data;
+                    bootargs = pp->value;
                 continue;
             }
         }
         /*
          * In a memory node: adjust reg property.
+         * TODO: handle properly memory node (ie: device_type =
"memory")
          */
-        else if ( device_tree_node_matches(fdt, node, "memory") )
+        else if ( dt_node_name_is_equal(np, "memory") )
         {
-            if ( strcmp(prop_name, "reg") == 0 )
+            if ( dt_property_name_is_equal(pp, "reg") )
             {
-                new_data = xzalloc_bytes(prop_len);
-                if ( new_data  == NULL )
+                new_data = xzalloc_bytes(pp->length);
+                if ( prop_data  == NULL )
                     return -FDT_ERR_XEN(ENOMEM);
 
-                prop_len = set_memory_reg(d, kinfo, fdt,
-                                          (u32 *)prop_data, prop_len,
-                                          address_cells, size_cells,
-                                          (u32 *)new_data);
+                prop_len = set_memory_reg(d, kinfo, pp, np,
+                                          (__be32 *)new_data);
                 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);
+        res = fdt_property(kinfo->fdt, pp->name, prop_data, prop_len);
 
         xfree(new_data);
+
+        if ( res )
+            return res;
     }
 
-    if ( device_tree_node_matches(fdt, node, "chosen") &&
bootargs )
-        fdt_property(kinfo->fdt, "bootargs", bootargs,
strlen(bootargs) + 1);
+    if ( dt_node_path_is_equal(np, "/chosen") && bootargs )
+    {
+        res = fdt_property(kinfo->fdt, "bootargs", bootargs,
+                           strlen(bootargs) + 1);
+        if ( res )
+            return res;
+    }
 
     /*
      * XXX should populate /chosen/linux,initrd-{start,end} here if we
      * have module[2]
      */
 
-    if ( prop == -FDT_ERR_NOTFOUND )
-        return 0;
-    return prop;
-}
-
-/* Returns the next node in fdt (starting from offset) which should be
- * passed through to dom0.
- */
-static int fdt_next_dom0_node(const void *fdt, int node,
-                              int *depth_out)
-{
-    int depth = *depth_out;
-
-    while ( (node = fdt_next_node(fdt, node, &depth)) &&
-            node >= 0 && depth >= 0 )
-    {
-        if ( depth >= DEVICE_TREE_MAX_DEPTH )
-            break;
-
-        /* Skip /hypervisor/ node. We will inject our own. */
-        if ( fdt_node_check_compatible(fdt, node, "xen,xen" ) == 0 )
-        {
-            printk("Device-tree contains \"xen,xen\" node.
Ignoring.\n");
-            continue;
-        }
-
-        /* Skip multiboot subnodes */
-        if ( fdt_node_check_compatible(fdt, node,
-                                       "xen,multiboot-module" ) == 0
)
-            continue;
-
-        /* We''ve arrived at a node which dom0 is interested in. */
-        break;
-    }
-
-    *depth_out = depth;
-    return node;
+    return 0;
 }
 
-static void make_hypervisor_node(void *fdt, int addrcells, int sizecells)
+static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent)
 {
     const char compat[]         
"xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
         "xen,xen";
-    u32 reg[4];
-    u32 intr[3];
-    u32 *cell;
+    __be32 reg[4];
+    __be32 intr[3];
+    __be32 *cells;
+    int res;
+    int addrcells = dt_n_addr_cells(parent);
+    int sizecells = dt_n_size_cells(parent);
+
+    DPRINT("Create hypervisor node\n");
 
     /*
      * Sanity-check address sizes, since addresses and sizes which do
@@ -277,16 +239,24 @@ static void make_hypervisor_node(void *fdt, int addrcells,
int sizecells)
         panic("Cannot cope with this size");
 
     /* See linux Documentation/devicetree/bindings/arm/xen.txt */
-    fdt_begin_node(fdt, "hypervisor");
+    res = fdt_begin_node(fdt, "hypervisor");
+    if ( res )
+        return res;
 
     /* Cannot use fdt_property_string due to embedded nulls */
-    fdt_property(fdt, "compatible", compat, sizeof(compat));
+    res = fdt_property(fdt, "compatible", compat, sizeof(compat));
+    if ( res )
+        return res;
 
     /* reg 0 is grant table space */
-    cell = &reg[0];
-    device_tree_set_reg(&cell, addrcells, sizecells, 0xb0000000, 0x20000);
-    fdt_property(fdt, "reg", reg,
-                 sizeof(reg[0]) * (addrcells + sizecells));
+    cells = &reg[0];
+    dt_set_cell(&cells, addrcells, 0xb0000000);
+    dt_set_cell(&cells, sizecells, 0x20000);
+
+    res = fdt_property(fdt, "reg", reg,
+                       sizeof(reg[0]) * (addrcells + sizecells));
+    if ( res )
+        return res;
 
     /*
      * interrupts is evtchn upcall  <1 15 0xf08>
@@ -296,63 +266,13 @@ static void make_hypervisor_node(void *fdt, int addrcells,
int sizecells)
     intr[1] = cpu_to_fdt32(VGIC_IRQ_EVTCHN_CALLBACK - 16); /* PPIs start at 16
*/
     intr[2] = cpu_to_fdt32(0xf08); /* Active-low level-sensitive */
 
-    fdt_property(fdt, "interrupts", intr, sizeof(intr[0]) * 3);
-
-    fdt_end_node(fdt);
-}
-
-static int write_nodes(struct domain *d, struct kernel_info *kinfo,
-                       const void *fdt)
-{
-    int node;
-    int depth = 0, last_depth = -1;
-    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_dom0_node(fdt, node, &depth) )
-    {
-        const char *name;
-
-        name = fdt_get_name(fdt, node, NULL);
-
-        if ( depth >= DEVICE_TREE_MAX_DEPTH )
-        {
-            printk("warning: node `%s'' is nested too deep
(%d)\n",
-                   name, depth);
-            continue;
-        }
-
-        /* We cannot handle descending more than one level at a time */
-        ASSERT( depth <= last_depth + 1 );
-
-        while ( last_depth-- >= depth )
-            fdt_end_node(kinfo->fdt);
-
-        address_cells[depth] = device_tree_get_u32(fdt, node,
"#address-cells",
-                                    depth > 0 ? address_cells[depth-1] : 0);
-        size_cells[depth] = device_tree_get_u32(fdt, node,
"#size-cells",
-                                    depth > 0 ? size_cells[depth-1] : 0);
-
-        fdt_begin_node(kinfo->fdt, name);
-
-        ret = write_properties(d, kinfo, fdt, node, name, depth,
-                               address_cells[depth-1], size_cells[depth-1]);
-        if ( ret < 0 )
-            return ret;
-
-        last_depth = depth;
-    }
-
-    while ( last_depth-- >= 1 )
-        fdt_end_node(kinfo->fdt);
+    res = fdt_property(fdt, "interrupts", intr, sizeof(intr[0]) * 3);
+    if ( res )
+        return res;
 
-    make_hypervisor_node(kinfo->fdt, address_cells[0], size_cells[0]);
+    res = fdt_end_node(fdt);
 
-    fdt_end_node(kinfo->fdt);
-    return 0;
+    return res;
 }
 
 /* Map the device in the domain */
@@ -438,12 +358,12 @@ static int map_device(struct domain *d, const struct
dt_device_node *dev)
 static const struct dt_device_match skip_matches[] __initconst  {
     DT_MATCH_COMPATIBLE("xen,xen"),
-    DT_MATCH_TYPE("memory"),
-    DT_MATCH_PATH("/chosen"),
+    DT_MATCH_COMPATIBLE("xen,multiboot-module"),
     { /* sentinel */ },
 };
 
-static int handle_node(struct domain *d, const struct dt_device_node *np)
+static int handle_node(struct domain *d, struct kernel_info *kinfo,
+                       const struct dt_device_node *np)
 {
     const struct dt_device_node *child;
     int res;
@@ -454,7 +374,8 @@ static int handle_node(struct domain *d, const struct
dt_device_node *np)
     if ( dt_match_node(skip_matches, np ) )
         return 0;
 
-    if ( dt_device_used_by(np) != DOMID_XEN )
+    if ( dt_device_used_by(np) != DOMID_XEN &&
+         !dt_device_type_is_equal(np, "memory") )
     {
         res = map_device(d, np);
 
@@ -462,21 +383,31 @@ static int handle_node(struct domain *d, const struct
dt_device_node *np)
             return res;
     }
 
+    res = fdt_begin_node(kinfo->fdt, kbasename(dt_node_full_name(np)));
+    if ( res )
+        return res;
+
+    res = write_properties(d, kinfo, np);
+    if ( res )
+        return res;
+
     for ( child = np->child; child != NULL; child = child->sibling )
     {
-        res = handle_node(d, child);
+        res = handle_node(d, kinfo, child);
         if ( res )
             return res;
     }
 
-    return 0;
-}
+    if ( np == dt_host )
+    {
+        res = make_hypervisor_node(kinfo->fdt, np);
+        if ( res )
+            return res;
+    }
 
-static int map_devices_from_device_tree(struct domain *d)
-{
-    ASSERT(dt_host && (dt_host->sibling == NULL));
+    res = fdt_end_node(kinfo->fdt);
 
-    return handle_node(d, dt_host);
+    return res;
 }
 
 static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
@@ -486,6 +417,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info
*kinfo)
     int ret;
     paddr_t end;
 
+    ASSERT(dt_host && (dt_host->sibling == NULL));
+
     kinfo->unassigned_mem = dom0_mem;
 
     fdt = device_tree_flattened;
@@ -501,8 +434,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info
*kinfo)
 
     fdt_finish_reservemap(kinfo->fdt);
 
-    ret = write_nodes(d, kinfo, fdt);
-    if ( ret < 0 )
+    ret = handle_node(d, kinfo, dt_host);
+    if ( ret )
         goto err;
 
     ret = fdt_finish(kinfo->fdt);
@@ -574,7 +507,6 @@ int construct_dom0(struct domain *d)
     if ( rc < 0 )
         return rc;
 
-    map_devices_from_device_tree(d);
     rc = platform_specific_mapping(d);
     if ( rc < 0 )
         return rc;
-- 
1.7.10.4

When a device has a property status with disabled inside, Linux will not use
the device.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain_build.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c8f24ed..8377610 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -208,6 +208,14 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
             return res;
     }
 
+    /* Disable all devices used by Xen */
+    if ( dt_device_used_by(np) == DOMID_XEN )
+    {
+        res = fdt_property(kinfo->fdt, "status",
"disabled", 8 + 1);
+        if ( res )
+            return res;
+    }
+
     /*
      * XXX should populate /chosen/linux,initrd-{start,end} here if we
      * have module[2]
-- 
1.7.10.4

Linux should cope with ''status = "disabled"'' in the
Device Tree.
This solution can be used later to pass-through device to a specific guest.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain_build.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8377610..567b1fe 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -383,7 +383,8 @@ static int handle_node(struct domain *d, struct kernel_info
*kinfo,
         return 0;
 
     if ( dt_device_used_by(np) != DOMID_XEN &&
-         !dt_device_type_is_equal(np, "memory") )
+         !dt_device_type_is_equal(np, "memory") &&
+         dt_device_is_available(np) )
     {
         res = map_device(d, np);
 
-- 
1.7.10.4

Xen uses PSCI to bring up secondary cpus for the guest.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain_build.c |   44 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 567b1fe..d8d67a6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -13,6 +13,7 @@
 #include <xen/guest_access.h>
 #include <asm/setup.h>
 #include <asm/platform.h>
+#include <asm/psci.h>
 
 #include <asm/gic.h>
 #include <xen/irq.h>
@@ -283,6 +284,44 @@ static int make_hypervisor_node(void *fdt, const struct
dt_device_node *parent)
     return res;
 }
 
+static int make_psci_node(void *fdt, const struct dt_device_node *parent)
+{
+    int res;
+    __be32 reg[0];
+    __be32 *cells;
+
+    DPRINT("Create PSCI node\n");
+
+    /* See linux Documentation/devicetree/bindings/arm/psci.txt */
+    res = fdt_begin_node(fdt, "psci");
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "compatible",
"arm,psci");
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "method", "hvc");
+    if ( res )
+        return res;
+
+    cells = &reg[0];
+    dt_set_cell(&cells, 1, __PSCI_cpu_off);
+    res = fdt_property(fdt, "cpu_off", reg, sizeof(reg[0]));
+    if ( res )
+        return res;
+
+    cells = &reg[0];
+    dt_set_cell(&cells, 1, __PSCI_cpu_on);
+    res = fdt_property(fdt, "cpu_on", reg, sizeof(reg[0]));
+    if ( res )
+        return res;
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
 /* Map the device in the domain */
 static int map_device(struct domain *d, const struct dt_device_node *dev)
 {
@@ -367,6 +406,7 @@ static const struct dt_device_match skip_matches[]
__initconst  {
     DT_MATCH_COMPATIBLE("xen,xen"),
     DT_MATCH_COMPATIBLE("xen,multiboot-module"),
+    DT_MATCH_COMPATIBLE("arm,psci"),
     { /* sentinel */ },
 };
 
@@ -412,6 +452,10 @@ static int handle_node(struct domain *d, struct kernel_info
*kinfo,
         res = make_hypervisor_node(kinfo->fdt, np);
         if ( res )
             return res;
+
+        res = make_psci_node(kinfo->fdt, np);
+        if ( res )
+            return res;
     }
 
     res = fdt_end_node(kinfo->fdt);
-- 
1.7.10.4

Each platform code will list the device that must not pass-through to a guest.
Theses devices are used for: power management, timer,...

When theses devices are given to DOM0, it can controls the hardware and then
break the whole platform.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/domain_build.c    |    2 +-
 xen/arch/arm/platform.c        |   10 ++++++++++
 xen/include/asm-arm/platform.h |    7 +++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d8d67a6..93f77c6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -419,7 +419,7 @@ static int handle_node(struct domain *d, struct kernel_info
*kinfo,
     DPRINT("handle %s\n", dt_node_full_name(np));
 
     /* Skip theses nodes and the sub-nodes */
-    if ( dt_match_node(skip_matches, np ) )
+    if ( dt_match_node(skip_matches, np ) || platform_device_is_blacklist(np) )
         return 0;
 
     if ( dt_device_used_by(np) != DOMID_XEN &&
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index afda302..cae0580 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -127,6 +127,16 @@ bool_t platform_has_quirk(uint32_t quirk)
     return !!(quirks & quirk);
 }
 
+bool_t platform_device_is_blacklist(const struct dt_device_node *node)
+{
+    const struct dt_device_match *blacklist = NULL;
+
+    if ( platform && platform->blacklist_dev )
+        blacklist = platform->blacklist_dev;
+
+    return dt_match_node(blacklist, node);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index f460e9c..4b511ed 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -4,6 +4,7 @@
 #include <xen/init.h>
 #include <xen/sched.h>
 #include <xen/mm.h>
+#include <xen/device_tree.h>
 
 /* Describe specific operation for a board */
 struct platform_desc {
@@ -26,6 +27,11 @@ struct platform_desc {
      * board with different quirk on each
      */
     uint32_t (*quirks)(void);
+    /*
+     * Platform blacklist devices
+     * List of devices which must not pass-through to a guest
+     */
+    const struct dt_device_match *blacklist_dev;
 };
 
 /*
@@ -40,6 +46,7 @@ int __init platform_specific_mapping(struct domain *d);
 void platform_reset(void);
 void platform_poweroff(void);
 bool_t platform_has_quirk(uint32_t quirk);
+bool_t platform_device_is_blacklist(const struct dt_device_node *node);
 
 #define PLATFORM_START(_name, _namestr)                         \
 static const struct platform_desc  __plat_desc_##_name __used   \
-- 
1.7.10.4

On Versatile there is a bunch of devices that must not pass-through to any
guest (power management and cache coherency devices).

This commit also blacklist the HDLCD device because then is unable to correctly
map the framebuffer. Therefore, when Linux will try to access to the
framebuffer,
Xen will receive a non-handled data access.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/platforms/vexpress.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/xen/arch/arm/platforms/vexpress.c
b/xen/arch/arm/platforms/vexpress.c
index 8fc30c4..ece7bd9 100644
--- a/xen/arch/arm/platforms/vexpress.c
+++ b/xen/arch/arm/platforms/vexpress.c
@@ -125,9 +125,26 @@ static const char const *vexpress_dt_compat[] __initdata   
NULL
 };
 
+static const struct dt_device_match vexpress_blacklist_dev[] __initconst +{
+    /* Cache Coherent Interconnect */
+    DT_MATCH_COMPATIBLE("arm,cci-400"),
+    DT_MATCH_COMPATIBLE("arm,cci-400-pmu"),
+    /* Video device
+     * TODO: remove it once memreserve is handled properly by Xen
+     */
+    DT_MATCH_COMPATIBLE("arm,hdlcd"),
+    /* Hardware power management */
+    DT_MATCH_COMPATIBLE("arm,vexpress-reset"),
+    DT_MATCH_COMPATIBLE("arm,vexpress-reboot"),
+    DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"),
+    { /* sentinel */ },
+};
+
 PLATFORM_START(vexpress, "VERSATILE EXPRESS")
     .compatible = vexpress_dt_compat,
     .reset = vexpress_reset,
+    .blacklist_dev = vexpress_blacklist_dev,
 PLATFORM_END
 
 /*
-- 
1.7.10.4

The Multi Core Timer (MCT) is a Samsung specific device.
This device tries to route IRQ in non-boot CPU which is not yet handled by Xen.

The user will see randomly dom0 hang, but I''m not sure that is the real
reason.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/platforms/exynos5.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index 1368a04..036281c 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -92,12 +92,23 @@ static const char const *exynos5_dt_compat[] __initdata     
NULL
 };
 
+static const struct dt_device_match exynos5_blacklist_dev[] __initconst +{
+    /* Multi core Timer
+     * TODO: this device set up IRQ to CPU 1 which is not yet handled by Xen.
+     * This is result to random freeze.
+     */
+    DT_MATCH_COMPATIBLE("samsung,exynos4210-mct"),
+    { /* sentinel */ },
+};
+
 PLATFORM_START(exynos5, "SAMSUNG EXYNOS5")
     .compatible = exynos5_dt_compat,
     .init_time = exynos5_init_time,
     .specific_mapping = exynos5_specific_mapping,
     .reset = exynos5_reset,
     .quirks = exynos5_quirks,
+    .blacklist_dev = exynos5_blacklist_dev,
 PLATFORM_END
 
 /*
-- 
1.7.10.4

All Xen code has been converted to the new device tree API that uses a tree
structure to describe the DTS.

The Flat Device tree is still used by Xen during early boot stage, but only in
internal. Remove entirely unneeded functions or move to a static function.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/common/device_tree.c      |  109 +++++------------------------------------
 xen/include/xen/device_tree.h |   15 ------
 2 files changed, 12 insertions(+), 112 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 6bdab14..d4a821b 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -103,8 +103,8 @@ struct dt_bus
     unsigned int (*get_flags)(const __be32 *addr);
 };
 
-bool_t __init device_tree_node_matches(const void *fdt, int node,
-                                       const char *match)
+static bool_t __init device_tree_node_matches(const void *fdt, int node,
+                                              const char *match)
 {
     const char *name;
     size_t match_len;
@@ -118,7 +118,7 @@ bool_t __init device_tree_node_matches(const void *fdt, int
node,
         && (name[match_len] == ''@'' || name[match_len]
== ''\0'');
 }
 
-bool_t __init device_tree_type_matches(const void *fdt, int node,
+static bool_t __init device_tree_type_matches(const void *fdt, int node,
                                        const char *match)
 {
     const void *prop;
@@ -130,8 +130,8 @@ bool_t __init device_tree_type_matches(const void *fdt, int
node,
     return !dt_node_cmp(prop, match);
 }
 
-bool_t __init device_tree_node_compatible(const void *fdt, int node,
-                                          const char *match)
+static bool_t __init device_tree_node_compatible(const void *fdt, int node,
+                                                 const char *match)
 {
     int len, l;
     int mlen;
@@ -154,34 +154,6 @@ bool_t __init device_tree_node_compatible(const void *fdt,
int node,
     return 0;
 }
 
-static __init int device_tree_nr_reg_ranges(const struct fdt_property *prop,
-        u32 address_cells, u32 size_cells)
-{
-    u32 reg_cells = address_cells + size_cells;
-    return fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32));
-}
-
-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;
-        *val |= fdt32_to_cpu(*(*cell)++);
-    }
-}
-
-void __init 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);
-}
-
 void dt_set_cell(__be32 **cellp, int size, u64 val)
 {
     int cells = size;
@@ -195,13 +167,6 @@ void dt_set_cell(__be32 **cellp, int size, u64 val)
     (*cellp) += cells;
 }
 
-void __init device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
-                                u64 start, u64 size)
-{
-    dt_set_cell(cell, address_cells, start);
-    dt_set_cell(cell, size_cells, size);
-}
-
 void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
                   u64 address, u64 size)
 {
@@ -209,8 +174,8 @@ void dt_set_range(__be32 **cellp, const struct
dt_device_node *np,
     dt_set_cell(cellp, dt_n_size_cells(np), size);
 }
 
-u32 __init device_tree_get_u32(const void *fdt, int node, const char
*prop_name,
-                               u32 dflt)
+static u32 __init device_tree_get_u32(const void *fdt, int node,
+                                      const char *prop_name, u32 dflt)
 {
     const struct fdt_property *prop;
 
@@ -232,8 +197,9 @@ u32 __init device_tree_get_u32(const void *fdt, int node,
const char *prop_name,
  * Returns 0 if all nodes were iterated over successfully.  If @func
  * returns a value different from 0, that value is returned immediately.
  */
-int __init device_tree_for_each_node(const void *fdt,
-                                     device_tree_node_func func, void *data)
+static int __init device_tree_for_each_node(const void *fdt,
+                                            device_tree_node_func func,
+                                            void *data)
 {
     int node;
     int depth;
@@ -268,58 +234,6 @@ int __init device_tree_for_each_node(const void *fdt,
     return 0;
 }
 
-struct find_compat {
-    const char *compatible;
-    int found;
-    int node;
-    int depth;
-    u32 address_cells;
-    u32 size_cells;
-};
-
-static int _find_compatible_node(const void *fdt,
-                             int node, const char *name, int depth,
-                             u32 address_cells, u32 size_cells,
-                             void *data)
-{
-    struct find_compat *c = (struct find_compat *) data;
-
-    if (  c->found  )
-        return 1;
-
-    if ( device_tree_node_compatible(fdt, node, c->compatible) )
-    {
-        c->found = 1;
-        c->node = node;
-        c->depth = depth;
-        c->address_cells = address_cells;
-        c->size_cells = size_cells;
-        return 1;
-    }
-    return 0;
-}
-
-int __init find_compatible_node(const char *compatible, int *node, int *depth,
-                                u32 *address_cells, u32 *size_cells)
-{
-    int ret;
-    struct find_compat c;
-    c.compatible = compatible;
-    c.found = 0;
-
-    ret = device_tree_for_each_node(device_tree_flattened,
_find_compatible_node, &c);
-    if ( !c.found )
-        return ret;
-    else
-    {
-        *node = c.node;
-        *depth = c.depth;
-        *address_cells = c.address_cells;
-        *size_cells = c.size_cells;
-        return 1;
-    }
-}
-
 /**
  * device_tree_bootargs - return the bootargs (the Xen command line)
  * @fdt flat device tree.
@@ -387,6 +301,7 @@ static void __init process_memory_node(const void *fdt, int
node,
     int banks;
     const __be32 *cell;
     paddr_t start, size;
+    u32 reg_cells = address_cells + size_cells;
 
     if ( address_cells < 1 || size_cells < 1 )
     {
@@ -403,7 +318,7 @@ static void __init process_memory_node(const void *fdt, int
node,
     }
 
     cell = (const __be32 *)prop->data;
-    banks = device_tree_nr_reg_ranges(prop, address_cells, size_cells);
+    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
 
     for ( i = 0; i < banks && early_info.mem.nr_banks <
NR_MEM_BANKS; i++ )
     {
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 22a2973..c61c917 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -175,21 +175,6 @@ extern const void *device_tree_flattened;
 
 size_t __init device_tree_early_init(const void *fdt);
 
-void __init device_tree_get_reg(const u32 **cell, u32 address_cells,
-                                u32 size_cells,
-                                u64 *start, u64 *size);
-void __init device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
-                                u64 start, u64 size);
-u32 __init device_tree_get_u32(const void *fdt, int node,
-                               const char *prop_name, u32 dflt);
-bool_t __init device_tree_node_matches(const void *fdt, int node,
-                                       const char *match);
-bool_t __init device_tree_node_compatible(const void *fdt, int node,
-                                          const char *match);
-int __init find_compatible_node(const char *compatible, int *node, int *depth,
-                                u32 *address_cells, u32 *size_cells);
-int __init device_tree_for_each_node(const void *fdt,
-                                     device_tree_node_func func, void *data);
 const char __init *device_tree_bootargs(const void *fdt);
 void __init device_tree_dump(const void *fdt);
 
-- 
1.7.10.4

It''s possible to have a device description in the DTS but the device is
not
wired.

device_init must check if the device is available before doing anything with
it.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/device.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index dc751a9..f86b2e3 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -47,6 +47,9 @@ int __init device_init(struct dt_device_node *dev, enum
device_type type,
 
     ASSERT(dev != NULL);
 
+    if ( !dt_device_is_available(dev) )
+        return  -ENODEV;
+
     for ( desc = _sdevice; desc != _edevice; desc++ )
     {
         if ( desc->type != type )
-- 
1.7.10.4

On 08/16/2013 11:05 PM, Julien Grall wrote:
> On some board, there is no alias to the UART. To avoid modification in
> the device tree, dt-uart should also search device by path.
Funny, it wrote almost the same patch two days ago (including the 
variable renaming, minus the "/" check). Thanks for saving me the 
cleanup and send-out ;-)
It is really useful for Midway!
> To distinguish an alias from a path, dt-uart will check the first
character.
> If it''s a / then it''s path, otherwise it''s an
alias.
Is that really needed? In my patch I just try it as an alias first, if 
there is no match (dev == NULL), I try the full path.
Are there any ambiguities expected between an alias and a full path?

Regards,
Andre.
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>   xen/drivers/char/dt-uart.c |   16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c
> index 93bb0f5..d7204fb 100644
> --- a/xen/drivers/char/dt-uart.c
> +++ b/xen/drivers/char/dt-uart.c
> @@ -26,9 +26,10 @@
>
>   /*
>    * Configure UART port with a string:
> - * alias,options
> + * path,options
>    *
> - * @alias: alias used in the device tree for the UART
> + * @path: full path used in the device tree for the UART. If the path
> + * doesn''t start with ''/'', we assuming that
it''s an alias.
>    * @options: UART speficic options (see in each UART driver)
>    */
>   static char __initdata opt_dtuart[30] = "";
> @@ -38,7 +39,7 @@ void __init dt_uart_init(void)
>   {
>       struct dt_device_node *dev;
>       int ret;
> -    const char *devalias = opt_dtuart;
> +    const char *devpath = opt_dtuart;
>       char *options;
>
>       if ( !console_has("dtuart") || !strcmp(opt_dtuart,
"") )
> @@ -53,12 +54,15 @@ void __init dt_uart_init(void)
>       else
>           options = "";
>
> -    early_printk("Looking for UART console %s\n", devalias);
> -    dev = dt_find_node_by_alias(devalias);
> +    early_printk("Looking for UART console %s\n", devpath);
> +    if ( *devpath == ''/'' )
> +        dev = dt_find_node_by_path(devpath);
> +    else
> +        dev = dt_find_node_by_alias(devpath);
>
>       if ( !dev )
>       {
> -        early_printk("Unable to find device \"%s\"\n",
devalias);
> +        early_printk("Unable to find device \"%s\"\n",
devpath);
>           return;
>       }
>
>

On Aug 17, 2013, at 5:05 AM, Julien Grall <julien.grall@linaro.org> wrote:
> The properties #address-cells and #size-cells is not required in /cpus node
in
> the device tree (see Linux Documentation/devicetree/booting-without-of.txt
> Section III.5.a).
> 
> In the OMAP5 device tree, these 2 properties are not defined. Therefore,
Xen
> will only able to handle 1 CPU.
Well, these 2 properties are defined in the latest upstream codes. They are just
not defined in the kernel tree shipped with TI devboard which version is 3.8.2.

For the others, this patch looks good for me.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> CC: andrii.anisov@globallogic.com
> CC: baozich@gmail.com
Acked-by: Chen Baozi <baozich@gmail.com>
> ---
> xen/common/device_tree.c |    6 ------
> 1 file changed, 6 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 84d704d..51f23eb 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -414,12 +414,6 @@ static void __init process_cpu_node(const void *fdt,
int node,
>     const u32 *cell;
>     paddr_t start, size;
> 
> -    if ( address_cells != 1 || size_cells != 0 )
> -    {
> -        early_printk("fdt: node `%s'': invalid #address-cells
or #size-cells",
> -                     name);
> -        return;
> -    }
> 
>     prop = fdt_get_property(fdt, node, "reg", NULL);
>     if ( !prop )
> -- 
> 1.7.10.4
>

>>> Julien Grall <julien.grall@linaro.org> 08/16/13 11:06 PM
>>>
>It''s possible to have 2 type (const and non-const) of data in the
same
>compilation unit. Using only __initdata will result to a compilation error:
>
>error: $variablename causes as section tupe conflict with $variablename2
>
>because a section containing const variables is marked read only and so
cannot
>contain non-const variables.
I don''t mind this change, but so far we avoided this by simply not
marking
such objects ''const''. I didn''t check whether the ARM
port is different in this
regard, but the x86 port till now didn''t care to mark regular read-only
data
read-only at runtime, so there''s little point in being over-ambitious
for init-
only data.

Jan

>>> Julien Grall <julien.grall@linaro.org> 08/16/13 11:06 PM
>>>
>Add kbasename and strcasecmp. The code is copied from Linux.
For the latter I can see its general purpose, but Xen isn''t dealing
with path names,
so this doesn''t look warranted in the common header (and likely not in
any header
at all - I''d guess this has just a single source file as consumer).

Jan

On Mon, 2013-08-19 at 10:46 +0100, Jan Beulich wrote:
> >>> Julien Grall <julien.grall@linaro.org> 08/16/13 11:06 PM
>>>
> >It''s possible to have 2 type (const and non-const) of data in
the same
> >compilation unit. Using only __initdata will result to a compilation
error:
> >
> >error: $variablename causes as section tupe conflict with
$variablename2
> >
> >because a section containing const variables is marked read only and so
cannot
> >contain non-const variables.
> 
> I don''t mind this change, but so far we avoided this by simply not
marking
> such objects ''const''. I didn''t check whether the
ARM port is different in this
> regard, but the x86 port till now didn''t care to mark regular
read-only data
> read-only at runtime, so there''s little point in being
over-ambitious for init-
> only data.
ARM has some reasonable amount of initdata to describe the device tree
platform drivers etc. It seems reasonable to make those const to me.

(I''m a bit surprised at the gcc behaviour of not allowing const and non
const data in the same section, but whatever)
Ian.

On Mon, 2013-08-19 at 10:54 +0100, Jan Beulich wrote:
> >>> Julien Grall <julien.grall@linaro.org> 08/16/13 11:06 PM
>>>
> >Add kbasename and strcasecmp. The code is copied from Linux.
> 
> For the latter I can see its general purpose, but Xen isn''t
dealing with path names,
I presumed this was something to do with the path like structure of
device tree.
> so this doesn''t look warranted in the common header (and likely
not in any header
> at all - I''d guess this has just a single source file as
consumer).
> 
> Jan
>

On 08/16/2013 10:25 PM, Andre Przywara wrote:
> On 08/16/2013 11:05 PM, Julien Grall wrote:
>> On some board, there is no alias to the UART. To avoid modification in
>> the device tree, dt-uart should also search device by path.
> 
> Funny, it wrote almost the same patch two days ago (including the
> variable renaming, minus the "/" check). Thanks for saving me the
> cleanup and send-out ;-)
> It is really useful for Midway!
> 
>> To distinguish an alias from a path, dt-uart will check the first
>> character.
>> If it''s a / then it''s path, otherwise it''s
an alias.
> 
> Is that really needed? In my patch I just try it as an alias first, if
> there is no match (dev == NULL), I try the full path.
> Are there any ambiguities expected between an alias and a full path?
Following the ePAPR documentation (section 2.2.4), the name property
can''t contain ''/'' in the name.

I choose this solution, because I want to avoid to call the 2 functions
if it''s possible. It''s a waste of time to call both if we know
that the
path always start with a ''/'' and alias not.

Cheers,

-- 
Julien Grall

On 08/19/2013 03:57 PM, Ian Campbell wrote:
> On Mon, 2013-08-19 at 10:54 +0100, Jan Beulich wrote:
>>>>> Julien Grall <julien.grall@linaro.org> 08/16/13 11:06
PM >>>
>>> Add kbasename and strcasecmp. The code is copied from Linux.
>>
>> For the latter I can see its general purpose, but Xen isn''t
dealing with path names,
> 
> I presumed this was something to do with the path like structure of
> device tree.
Right. As this function is generic, I though it was better to let in
string.h. I can move this function to the device tree header.

Cheers,

-- 
Julien Grall

On 16 August 2013 22:05, Julien Grall <julien.grall@linaro.org>
wrote:
> Hi,
>
> This is the first patch series to allow Xen to boot with a raw device tree,
ie
> without any specific modification for Xen.
>
> Few months ago, the patch series "Support multiple ARM platform in
XEN", has
> added a tree structure to handle easier the device tree. Each node can
carry
> metadata to specify if the node can be pass-through to Dom0. Therefore,
this
> patch series take advantage of the "new" Device Tree API and get
a rid of
> the FDT API that is currently used to build Dom0 dts and in some drivers.
>
> There is few items that was not added in this series:
>     * Set the correct number of CPUs for Dom0 by editing CPUs node
>     * Remove and create the GIC node
>     * Remove and create the timer node
>
> I hope to add support for previous items in the next patch series.
>
> At the beginning of the mail, I a bit lied about Xen is able to raw DTS :).
> The latter, still need to carry Xen and Linux command line until
U-Boot/Grub
> will support multiboot2 protocol.
> I have noticed that U-boot has some fdt commands to change on the fly the
device
> tree. I think we can use this solution as a workaround for the beginning.
> I will send an email later to explain the procedure.
>
As promised, I wrote a small U-boot to remove all remaining changes in
the Device Tree (modules and xen/dom0 bootargs). This script requires
Andre''s patch (http://osdir.com/ml/general/2013-05/msg68353.html).

http://xenbits.xensource.com/people/julieng/multiboot-fixup.scr.txt

The script creates:
   - Xen and Dom0 bootargs
   - the modules node with linux address correctly set up

I think, this script can be used as a workaround to avoid manual
Device Tree modification until U-boot/Grub will support correctly
multiboot. If people are insterested by this script, I can create a
wiki page with all the instructions.

Cheers,

-- 
Julien Grall

>>> On 19.08.13 at 16:56, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> (I''m a bit surprised at the gcc behaviour of not allowing const
and non
> const data in the same section, but whatever)
Iirc that got fixed in newer gcc versions, but we continue to support
(on x86 at least) ones that dislike mixing section attributes.

Jan

On Tue, 2013-08-20 at 08:12 +0100, Jan Beulich wrote:
> >>> On 19.08.13 at 16:56, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
> > (I''m a bit surprised at the gcc behaviour of not allowing
const and non
> > const data in the same section, but whatever)
> 
> Iirc that got fixed in newer gcc versions, but we continue to support
> (on x86 at least) ones that dislike mixing section attributes.
Since Julien is running on ARM, I think with linaro cross-compilers, he
is quite likely running gcc 4.7 or 4.8, which are pretty new I think.

Ian.

>>> On 19.08.13 at 17:13, Julien Grall <julien.grall@linaro.org>
wrote:
> On 08/19/2013 03:57 PM, Ian Campbell wrote:
>> On Mon, 2013-08-19 at 10:54 +0100, Jan Beulich wrote:
>>>>>> Julien Grall <julien.grall@linaro.org> 08/16/13
11:06 PM >>>
>>>> Add kbasename and strcasecmp. The code is copied from Linux.
>>>
>>> For the latter I can see its general purpose, but Xen
isn''t dealing with
> path names,
>> 
>> I presumed this was something to do with the path like structure of
>> device tree.
> 
> Right. As this function is generic, I though it was better to let in
> string.h. I can move this function to the device tree header.
Afaic - yes, please do.

Jan

On Mon, 2013-08-19 at 23:11 +0100, Julien Grall wrote:
> On 16 August 2013 22:05, Julien Grall <julien.grall@linaro.org>
wrote:
> > Hi,
> >
> > This is the first patch series to allow Xen to boot with a raw device
tree, ie
> > without any specific modification for Xen.
> >
> > Few months ago, the patch series "Support multiple ARM platform
in XEN", has
> > added a tree structure to handle easier the device tree. Each node can
carry
> > metadata to specify if the node can be pass-through to Dom0.
Therefore, this
> > patch series take advantage of the "new" Device Tree API and
get a rid of
> > the FDT API that is currently used to build Dom0 dts and in some
drivers.
> >
> > There is few items that was not added in this series:
> >     * Set the correct number of CPUs for Dom0 by editing CPUs node
> >     * Remove and create the GIC node
> >     * Remove and create the timer node
> >
> > I hope to add support for previous items in the next patch series.
> >
> > At the beginning of the mail, I a bit lied about Xen is able to raw
DTS :).
> > The latter, still need to carry Xen and Linux command line until
U-Boot/Grub
> > will support multiboot2 protocol.
> > I have noticed that U-boot has some fdt commands to change on the fly
the device
> > tree. I think we can use this solution as a workaround for the
beginning.
> > I will send an email later to explain the procedure.
> >
> 
> As promised, I wrote a small U-boot to remove all remaining changes in
> the Device Tree (modules and xen/dom0 bootargs). This script requires
> Andre''s patch (http://osdir.com/ml/general/2013-05/msg68353.html).
> 
> http://xenbits.xensource.com/people/julieng/multiboot-fixup.scr.txt
> 
> The script creates:
>    - Xen and Dom0 bootargs
>    - the modules node with linux address correctly set up
> 
> I think, this script can be used as a workaround to avoid manual
> Device Tree modification until U-boot/Grub will support correctly
> multiboot. If people are insterested by this script, I can create a
> wiki page with all the instructions.
Yes. I think this is the way to go for now.

BTW:
        # It seems U-boot doesn''t have command to retrieve the size, so
the size is
        # set to a big value (10Mb)
u-boot sets $filesize when it loads something, so you''d just need to
stash that somewhere for use here. That places a requirement on the
script which actually does the loading (which I suppose is out of scope
of your script).

Ian.

On 20 August 2013 09:33, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Mon, 2013-08-19 at 23:11 +0100, Julien Grall wrote:
>> On 16 August 2013 22:05, Julien Grall <julien.grall@linaro.org>
wrote:
>> > Hi,
>> >
>> > This is the first patch series to allow Xen to boot with a raw
device tree, ie
>> > without any specific modification for Xen.
>> >
>> > Few months ago, the patch series "Support multiple ARM
platform in XEN", has
>> > added a tree structure to handle easier the device tree. Each node
can carry
>> > metadata to specify if the node can be pass-through to Dom0.
Therefore, this
>> > patch series take advantage of the "new" Device Tree API
and get a rid of
>> > the FDT API that is currently used to build Dom0 dts and in some
drivers.
>> >
>> > There is few items that was not added in this series:
>> >     * Set the correct number of CPUs for Dom0 by editing CPUs node
>> >     * Remove and create the GIC node
>> >     * Remove and create the timer node
>> >
>> > I hope to add support for previous items in the next patch series.
>> >
>> > At the beginning of the mail, I a bit lied about Xen is able to
raw DTS :).
>> > The latter, still need to carry Xen and Linux command line until
U-Boot/Grub
>> > will support multiboot2 protocol.
>> > I have noticed that U-boot has some fdt commands to change on the
fly the device
>> > tree. I think we can use this solution as a workaround for the
beginning.
>> > I will send an email later to explain the procedure.
>> >
>>
>> As promised, I wrote a small U-boot to remove all remaining changes in
>> the Device Tree (modules and xen/dom0 bootargs). This script requires
>> Andre''s patch
(http://osdir.com/ml/general/2013-05/msg68353.html).
>>
>> http://xenbits.xensource.com/people/julieng/multiboot-fixup.scr.txt
>>
>> The script creates:
>>    - Xen and Dom0 bootargs
>>    - the modules node with linux address correctly set up
>>
>> I think, this script can be used as a workaround to avoid manual
>> Device Tree modification until U-boot/Grub will support correctly
>> multiboot. If people are insterested by this script, I can create a
>> wiki page with all the instructions.
>
> Yes. I think this is the way to go for now.
I will update the wiki page.
> BTW:
>         # It seems U-boot doesn''t have command to retrieve the
size, so the size is
>         # set to a big value (10Mb)
> u-boot sets $filesize when it loads something, so you''d just need
to
> stash that somewhere for use here. That places a requirement on the
> script which actually does the loading (which I suppose is out of scope
> of your script).
Thanks for the hint. I will update my script.

-- 
Julien Grall

>>> On 20.08.13 at 10:31, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Tue, 2013-08-20 at 08:12 +0100, Jan Beulich wrote:
>> >>> On 19.08.13 at 16:56, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
>> > (I''m a bit surprised at the gcc behaviour of not allowing
const and non
>> > const data in the same section, but whatever)
>> 
>> Iirc that got fixed in newer gcc versions, but we continue to support
>> (on x86 at least) ones that dislike mixing section attributes.
> 
> Since Julien is running on ARM, I think with linaro cross-compilers, he
> is quite likely running gcc 4.7 or 4.8, which are pretty new I think.
I was just suspecting that a build problem may have been found
by him when build-checking x86...

Jan

On 20 August 2013 09:31, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Tue, 2013-08-20 at 08:12 +0100, Jan Beulich wrote:
>> >>> On 19.08.13 at 16:56, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
>> > (I''m a bit surprised at the gcc behaviour of not allowing
const and non
>> > const data in the same section, but whatever)
>>
>> Iirc that got fixed in newer gcc versions, but we continue to support
>> (on x86 at least) ones that dislike mixing section attributes.
>
> Since Julien is running on ARM, I think with linaro cross-compilers, he
> is quite likely running gcc 4.7 or 4.8, which are pretty new I think.
I use gcc 4.8
42sh> gcc --version
arm-linux-gnueabihf-gcc (crosstool-NG
linaro-1.13.1-4.8-2013.04-20130417 - Linaro GCC 2013.04) 4.8.1
20130401 (prerelease)

-- 
Julien Grall

On 08/16/2013 10:05 PM, Julien Grall wrote:
> Xen uses PSCI to bring up secondary cpus for the guest.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain_build.c |   44
+++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 567b1fe..d8d67a6 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -13,6 +13,7 @@
>  #include <xen/guest_access.h>
>  #include <asm/setup.h>
>  #include <asm/platform.h>
> +#include <asm/psci.h>
>  
>  #include <asm/gic.h>
>  #include <xen/irq.h>
> @@ -283,6 +284,44 @@ static int make_hypervisor_node(void *fdt, const
struct dt_device_node *parent)
>      return res;
>  }
>  
> +static int make_psci_node(void *fdt, const struct dt_device_node *parent)
> +{
> +    int res;
> +    __be32 reg[0];
I should have written reg[1];

This code was working by luck.

> +    __be32 *cells;
> +
> +    DPRINT("Create PSCI node\n");
> +
> +    /* See linux Documentation/devicetree/bindings/arm/psci.txt */
> +    res = fdt_begin_node(fdt, "psci");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_string(fdt, "compatible",
"arm,psci");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_string(fdt, "method", "hvc");
> +    if ( res )
> +        return res;
> +
> +    cells = &reg[0];
> +    dt_set_cell(&cells, 1, __PSCI_cpu_off);
> +    res = fdt_property(fdt, "cpu_off", reg, sizeof(reg[0]));
> +    if ( res )
> +        return res;
> +
> +    cells = &reg[0];
> +    dt_set_cell(&cells, 1, __PSCI_cpu_on);
> +    res = fdt_property(fdt, "cpu_on", reg, sizeof(reg[0]));
> +    if ( res )
> +        return res;
> +
> +    res = fdt_end_node(fdt);
> +
> +    return res;
> +}
> +
>  /* Map the device in the domain */
>  static int map_device(struct domain *d, const struct dt_device_node *dev)
>  {
> @@ -367,6 +406,7 @@ static const struct dt_device_match skip_matches[]
__initconst >  {
>      DT_MATCH_COMPATIBLE("xen,xen"),
>      DT_MATCH_COMPATIBLE("xen,multiboot-module"),
> +    DT_MATCH_COMPATIBLE("arm,psci"),
>      { /* sentinel */ },
>  };
>  
> @@ -412,6 +452,10 @@ static int handle_node(struct domain *d, struct
kernel_info *kinfo,
>          res = make_hypervisor_node(kinfo->fdt, np);
>          if ( res )
>              return res;
> +
> +        res = make_psci_node(kinfo->fdt, np);
> +        if ( res )
> +            return res;
>      }
>  
>      res = fdt_end_node(kinfo->fdt);
> 

-- 
Julien Grall

On Mon, 2013-08-19 at 16:09 +0100, Julien Grall wrote:
> On 08/16/2013 10:25 PM, Andre Przywara wrote:
> > On 08/16/2013 11:05 PM, Julien Grall wrote:
> >> On some board, there is no alias to the UART. To avoid
modification in
> >> the device tree, dt-uart should also search device by path.
> > 
> > Funny, it wrote almost the same patch two days ago (including the
> > variable renaming, minus the "/" check). Thanks for saving
me the
> > cleanup and send-out ;-)
> > It is really useful for Midway!
> > 
> >> To distinguish an alias from a path, dt-uart will check the first
> >> character.
> >> If it''s a / then it''s path, otherwise
it''s an alias.
> > 
> > Is that really needed? In my patch I just try it as an alias first, if
> > there is no match (dev == NULL), I try the full path.
> > Are there any ambiguities expected between an alias and a full path?
> 
> Following the ePAPR documentation (section 2.2.4), the name property
> can''t contain ''/'' in the name.
> 
> I choose this solution, because I want to avoid to call the 2 functions
> if it''s possible. It''s a waste of time to call both if we
know that the
> path always start with a ''/'' and alias not.
This is hardly a hot path. But in any case:

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

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> The properties #address-cells and #size-cells is not required in /cpus node
in
> the device tree (see Linux Documentation/devicetree/booting-without-of.txt
> Section III.5.a).
This isn''t quite accurate, the properties must exist somewhere, if not
here then in the parent node, recursively, otherwise we cannot interpret
the reg properties etc. What isn''t required is that they have the
particular values we are checking for.
> 
> In the OMAP5 device tree, these 2 properties are not defined. Therefore,
Xen
> will only able to handle 1 CPU.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> CC: andrii.anisov@globallogic.com
> CC: baozich@gmail.com
> ---
>  xen/common/device_tree.c |    6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 84d704d..51f23eb 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -414,12 +414,6 @@ static void __init process_cpu_node(const void *fdt,
int node,
>      const u32 *cell;
>      paddr_t start, size;
>  
> -    if ( address_cells != 1 || size_cells != 0 )
> -    {
> -        early_printk("fdt: node `%s'': invalid #address-cells
or #size-cells",
> -                     name);
> -        return;
> -    }
>  
>      prop = fdt_get_property(fdt, node, "reg", NULL);
>      if ( !prop )

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> The Flat Device Tree is given by the bootloader. Xen doesn''t need
to modify it.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> @@ -363,8 +364,9 @@ static void __init setup_mm(unsigned long dtb_paddr,
size_t dtb_size)
>       *
>       * TODO: handle other payloads too.
>       */
> -    device_tree_flattened = mfn_to_virt(alloc_boot_pages(dtb_pages, 1));
> -    copy_from_paddr(device_tree_flattened, dtb_paddr, dtb_size,
BUFFERABLE);
> +    fdt = mfn_to_virt(alloc_boot_pages(dtb_pages, 1));
> +    copy_from_paddr(fdt, dtb_paddr, dtb_size, BUFFERABLE);
> +    device_tree_flattened = fdt;
Not related to this patch but I wonder if we ought to alloc_boot_pages
(order_of(dtb_size)) or at least BUG_ON(dtb_size > PAGE_SIZE).

Ian.

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> These defines will be used to create the fake PSCI node in dom0 device
tree.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> It''s possible to have 2 type (const and non-const) of data in the
same
> compilation unit. Using only __initdata will result to a compilation error:
> 
>     error: $variablename causes as section tupe conflict with
$variablename2
> 
> because a section containing const variables is marked read only and so
cannot
> contain non-const variables.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

But that is not in itself sufficient, please remember to CC the right
maintainers, especially for non-ARM specific patches. That probably
means Jan and Keir here (I cc''d both, even though Jan has obviously
already seen and commented)
> ---
>  xen/include/xen/init.h |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
> index b602577..9d481b3 100644
> --- a/xen/include/xen/init.h
> +++ b/xen/include/xen/init.h
> @@ -10,6 +10,7 @@
>  #define __init            __text_section(".init.text")
>  #define __exit            __text_section(".exit.text")
>  #define __initdata        __section(".init.data")
> +#define __initconst       __section(".init.rodata")
>  #define __exitdata        __used_section(".exit.data")
>  #define __initsetup       __used_section(".init.setup")
>  #define __init_call(lvl)  __used_section(".initcall" lvl
".init")

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> When of_node_cmp and of_compat_cmp was introduced in commit fb97eb6
> "xen/arm: Create a hierarchical device tree", they were copied
from the wrong
> Linux header.
I probably don''t want to know why Linux defines this more than once
with
a different interface...
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

On 08/22/2013 01:51 PM, Ian Campbell wrote:
> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>> The properties #address-cells and #size-cells is not required in /cpus
node in
>> the device tree (see Linux
Documentation/devicetree/booting-without-of.txt
>> Section III.5.a).
> 
> This isn''t quite accurate, the properties must exist somewhere, if
not
> here then in the parent node, recursively, otherwise we cannot interpret
> the reg properties etc. What isn''t required is that they have the
> particular values we are checking for.
What about:

The properties #address-cells and #size-cells may not correctly defines
for CPUs node (see /Documentation/devicetree/booting-without-of.txt).
>>
>> In the OMAP5 device tree, these 2 properties are not defined.
Therefore, Xen
>> will only able to handle 1 CPU.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> CC: andrii.anisov@globallogic.com
>> CC: baozich@gmail.com
>> ---
>>  xen/common/device_tree.c |    6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 84d704d..51f23eb 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -414,12 +414,6 @@ static void __init process_cpu_node(const void
*fdt, int node,
>>      const u32 *cell;
>>      paddr_t start, size;
>>  
>> -    if ( address_cells != 1 || size_cells != 0 )
>> -    {
>> -        early_printk("fdt: node `%s'': invalid
#address-cells or #size-cells",
>> -                     name);
>> -        return;
>> -    }
>>  
>>      prop = fdt_get_property(fdt, node, "reg", NULL);
>>      if ( !prop )
> 
> 

-- 
Julien Grall

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> On new Flat Device Tree version, the property "name" may not
exist.
> The property is never used in Xen code except to set the field
"name" of
> dt_device_node.
> 
> For convenience, remove the fake property. It will save space during the
> creation of the dom0 FDT.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/common/device_tree.c |   21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 362dd66..315b284 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -1528,6 +1528,7 @@ static unsigned long __init unflatten_dt_node(const
void *fdt,
>      if ( !has_name )
>      {
>          char *p1 = pathp, *ps = pathp, *pa = NULL;
> +        char *tmp = NULL;
>          int sz;
>  
>          while ( *p1 )
> @@ -1541,25 +1542,21 @@ static unsigned long __init unflatten_dt_node(const
void *fdt,
>          if ( pa < ps )
>              pa = p1;
>          sz = (pa - ps) + 1;
> -        pp = unflatten_dt_alloc(&mem, sizeof(struct dt_property) + sz,
> -                                __alignof__(struct dt_property));
pp appears to not be assigned anywhere else now? I''m not sure if
prev_pp
becomes obsolete or not.
> +
> +        tmp = unflatten_dt_alloc(&mem, sz, 1);
>          if ( allnextpp )
>          {
> -            pp->name = "name";
> -            pp->length = sz;
> -            pp->value = pp + 1;
> -            *prev_pp = pp;
> -            prev_pp = &pp->next;
> -            memcpy(pp->value, ps, sz - 1);
> -            ((char *)pp->value)[sz - 1] = 0;
> -            dt_dprintk("fixed up name for %s -> %s\n", pathp,
> -                       (char *)pp->value);
> +            memcpy(tmp, ps, sz - 1);
> +            np->name = tmp;
> +            tmp[sz - 1] = 0;
> +            dt_dprintk("fixed up name for %s -> %s\n", pathp,
np->name);
>          }
>      }
> +
>      if ( allnextpp )
>      {
>          *prev_pp = NULL;
> -        np->name = dt_get_property(np, "name", NULL);
> +        np->name = (np->name) ? : dt_get_property(np,
"name", NULL);
>          np->type = dt_get_property(np, "device_type", NULL);
>  
>          if ( !np->name )

<insert which new helpers here>

I think this is also reimplementing the internals of some existing
dt_match_foo? And changing set_val into dt_set_cell. IOW there seems to
be more changes than the single line changelog would suggest.

Are any of these just sync''d from Linux?

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> @@ -295,11 +341,29 @@ struct dt_device_node *dt_find_compatible_node(struct
dt_device_node *from,
>  /**
>   * Find a property with a given name for a given node
>   * and return the value.
> - */
> +  */
Oops.
>  const void *dt_get_property(const struct dt_device_node *np,
>                              const char *name, u32 *lenp);
>  
>  /**
> + * dt_property_read_string - Find and read a string from a property
> + * @np:         Device node from which the property value is to be read
> + * @propname:   Name of the property to be searched
> + * @out_string: Pointer to null terminated return string, modified only
> + *              if return value if 0.
> + *
> + * Search for a property in a device tree node and retrieve a null
> + * terminated string value (pointer to data, not a copy). Returns 0 on
> + * sucess, -EINVAL if the property does not exist, -ENODATA if property
success
> + * doest not have value, and -EILSEQ if the string is not
> + * null-terminated with the length of the property data.
> + *
> + * The out_string pointer is modifed only if a valid string can be
decoded.
modified
> + */
> +int dt_property_read_string(const struct dt_device_node *np,
> +                            const char *propname, const char
**out_string);
> +
> +/**
>   * Checks if the given "compat" string matches one of the
strings in
>   * the device''s "compatible" property
>   */
> @@ -433,4 +497,71 @@ int dt_n_size_cells(const struct dt_device_node *np);
>   */
>  int dt_n_addr_cells(const struct dt_device_node *np);
>  
> -#endif
> +/**
> + * dt_device_is_available - Check if a device is available for use
> + *
> + * @device: Node to check for availability
> + *
> + * Returns true if the status property is absent or set to
"okay" or "ok",
> + * false otherwise.
> + */
> +bool_t dt_device_is_available(const struct dt_device_node *device);
> +
> +/**
> + * dt_match_node - Tell if a device_node has a matching of dt_device_match
> + * @matches: array of dt_device_match structures to search in
> + * @node: the dt_device_node structure to match against
> + *
> + * Returns true if the device node match one of dt_device_match.
> + */
> +bool_t dt_match_node(const struct dt_device_match *matches,
> +                     const struct dt_device_node *node);
> +
> +/**
> + * dt_find_matching_node - Find a node based on an dt_device_match match
table
> + * @from: The node to start searching from or NULL, the node you pass
> + *        will not be searched, only the next one will; typically, you
pass
> + *        what the returned call returned
> + * @matches: array of dt_device_match structures to search in
> + *
> + * Returns a node pointer.
> + */
> +struct dt_device_node *
> +dt_find_matching_node(struct dt_device_node *from,
> +                      const struct dt_device_match *matches);
> +
> +/**
> + * dt_set_cell - Write a value into a serie of cells
series
> + *
> + * @cellp: Pointer to cells
> + *
> + * Write a value into a series of cells and update cellp to point to the
> + * cell just after.
> + */
> +void dt_set_cell(__be32 **cellp, int size, u64 val);
> +
> +/**
> + * dt_set_range - Write range into a serie of cells
> + *
> + * @cellp: Pointer to cells
> + * @np: Node which contains the encoding for the address and
> + * the size
> + * @address: Start of range
> + * @size: Size of the range
> + *
> + * Write a range into a serie of cells and update cellp to point to the
series
> + * cell just after.
> + */
> +void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
> +                  u64 address, u64 size);
> +
> +#endif /* __XEN_DEVICE_TREE_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> The function device_get_reg will be removed in a future patch.
Why? It appears to be a useful helper.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/common/device_tree.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 95635f0..ea01a5a 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -385,7 +385,7 @@ static void __init process_memory_node(const void *fdt,
int node,
>      const struct fdt_property *prop;
>      int i;
>      int banks;
> -    const u32 *cell;
> +    const __be32 *cell;
>      paddr_t start, size;
>  
>      if ( address_cells < 1 || size_cells < 1 )
> @@ -402,12 +402,13 @@ static void __init process_memory_node(const void
*fdt, int node,
>          return;
>      }
>  
> -    cell = (const u32 *)prop->data;
> +    cell = (const __be32 *)prop->data;
>      banks = device_tree_nr_reg_ranges(prop, address_cells, size_cells);
>  
>      for ( i = 0; i < banks && early_info.mem.nr_banks <
NR_MEM_BANKS; i++ )
>      {
> -        device_tree_get_reg(&cell, address_cells, size_cells,
&start, &size);
> +        start = dt_next_cell(address_cells, &cell);
> +        size = dt_next_cell(size_cells, &cell);
>          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++;

On 08/22/2013 02:11 PM, Ian Campbell wrote:
> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>> When of_node_cmp and of_compat_cmp was introduced in commit fb97eb6
>> "xen/arm: Create a hierarchical device tree", they were
copied from the wrong
>> Linux header.
> 
> I probably don''t want to know why Linux defines this more than
once with
> a different interface...
Sparc architecture uses different string comparaison. I don''t really
understand why.
> 
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> 

-- 
Julien Grall

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> On some board CPU IDs are not contiguous (for instance the Versatile
Express
> with big.LITTLE supports). If the CPU ID is greater than NR_CPUS Xen will
hang
> without any message. This is because console driver is not yet initialized
and
> hypervisor data abort uses printk.
> 
> For the moment check the CPU ID and print an warning if an error occured.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> Avoid to use FDT API which will be removed soon.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/drivers/video/arm_hdlcd.c |   58
+++++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/drivers/video/arm_hdlcd.c b/xen/drivers/video/arm_hdlcd.c
> index 72979ea..0ff8c22 100644
> --- a/xen/drivers/video/arm_hdlcd.c
> +++ b/xen/drivers/video/arm_hdlcd.c
> @@ -25,6 +25,7 @@
>  #include <xen/libfdt/libfdt.h>
>  #include <xen/init.h>
>  #include <xen/mm.h>
> +#include <asm/early_printk.h>
>  #include "font.h"
>  #include "lfb.h"
>  #include "modelines.h"
> @@ -96,62 +97,67 @@ static int __init get_color_masks(const char* bpp,
struct color_masks **masks)
>  
>  static void __init set_pixclock(uint32_t pixclock)
>  {
> -    if ( device_tree_node_compatible(device_tree_flattened, 0,
"arm,vexpress") )
> +    if ( dt_find_compatible_node(NULL, NULL, "arm,vexpress") )
>              vexpress_syscfg(1, V2M_SYS_CFG_OSC_FUNC,
>                              V2M_SYS_CFG_OSC5, &pixclock);
>  }
>  
>  void __init video_init(void)
>  {
> -    int node, depth;
> -    u32 address_cells, size_cells;
>      struct lfb_prop lfbp;
>      unsigned char *lfb;
> -    paddr_t hdlcd_start, hdlcd_size;
> +    u64 hdlcd_start, hdlcd_size;
Why? These are physical addresses.
>      if ( !hdlcd_start )
>      {
> -        printk(KERN_ERR "HDLCD address missing from device tree,
disabling driver\n");
> +        early_printk("hdlcd: address missing from device tree,
disabling driver\n");
I suppose this (and the other instances of this) is for when the console
is on HDLCD? That''s a separate fix really I think.

Why tr /A-Z/ /a-z/?

Ian.

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> There is some place in Xen ARM code where multiple if conditions is used
> check the presence of a node or find a node.
> These pieces of code can be replace by an array and using proper device
tree
> helpers.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain_build.c |   12 +++++++++---
>  xen/arch/arm/time.c         |   11 ++++++++---
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index a324c3b..604ec1c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -435,6 +435,14 @@ static int map_device(struct domain *d, const struct
dt_device_node *dev)
>      return 0;
>  }
>  
> +static const struct dt_device_match skip_matches[] __initconst > +{
> +    DT_MATCH_COMPATIBLE("xen,xen"),
> +    DT_MATCH_TYPE("memory"),
> +    DT_MATCH_PATH("/chosen"),
> +    { /* sentinel */ },
> +};
Can this not be within the scope of the only function which uses it?

If not then perhaps it can be given a more meaningful name.
dom0_skip_matches?
> +
>  static int handle_node(struct domain *d, const struct dt_device_node *np)
>  {
>      const struct dt_device_node *child;
> @@ -443,9 +451,7 @@ static int handle_node(struct domain *d, const struct
dt_device_node *np)
>      DPRINT("handle %s\n", dt_node_full_name(np));
>  
>      /* Skip theses nodes and the sub-nodes */
> -    if ( dt_device_is_compatible(np, "xen,xen") ||
> -         dt_device_type_is_equal(np, "memory") ||
> -         !strcmp("/chosen", dt_node_full_name(np)) )
> +    if ( dt_match_node(skip_matches, np ) )
>          return 0;
>  
>      if ( dt_device_used_by(np) != DOMID_XEN )
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index b5864ef..05e58ba 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -98,6 +98,13 @@ static uint32_t calibrate_timer(void)
>  }
>  */
>  
> +static const struct dt_device_match timer_ids[] __initdata > +{
> +    DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> +    DT_MATCH_COMPATIBLE("arm,armv8-timer"),
> +    { /* sentinel */ },
> +};
> +
>  /* Set up the timer on the boot CPU */
>  int __init init_xen_time(void)
>  {
> @@ -105,9 +112,7 @@ int __init init_xen_time(void)
>      int res;
>      unsigned int i;
>  
> -    dev = dt_find_compatible_node(NULL, NULL,
"arm,armv7-timer");
> -    if ( !dev )
> -        dev = dt_find_compatible_node(NULL, NULL,
"arm,armv8-timer");
> +    dev = dt_find_matching_node(NULL, timer_ids);
>      if ( !dev )
>          panic("Unable to find a compatible timer in the device
tree\n");
>

On 08/22/2013 02:05 PM, Ian Campbell wrote:
> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>> The Flat Device Tree is given by the bootloader. Xen doesn''t
need to modify it.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
>> @@ -363,8 +364,9 @@ static void __init setup_mm(unsigned long
dtb_paddr, size_t dtb_size)
>>       *
>>       * TODO: handle other payloads too.
>>       */
>> -    device_tree_flattened = mfn_to_virt(alloc_boot_pages(dtb_pages,
1));
>> -    copy_from_paddr(device_tree_flattened, dtb_paddr, dtb_size,
BUFFERABLE);
>> +    fdt = mfn_to_virt(alloc_boot_pages(dtb_pages, 1));
>> +    copy_from_paddr(fdt, dtb_paddr, dtb_size, BUFFERABLE);
>> +    device_tree_flattened = fdt;
> 
> Not related to this patch but I wonder if we ought to alloc_boot_pages
> (order_of(dtb_size)) or at least BUG_ON(dtb_size > PAGE_SIZE).
The last solution can''t work. The size of device tree is about 32K, so
greater than one page.

-- 
Julien Grall

On 08/22/2013 02:16 PM, Ian Campbell wrote:
> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>> On new Flat Device Tree version, the property "name" may not
exist.
>> The property is never used in Xen code except to set the field
"name" of
>> dt_device_node.
>>
>> For convenience, remove the fake property. It will save space during
the
>> creation of the dom0 FDT.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/common/device_tree.c |   21 +++++++++------------
>>  1 file changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 362dd66..315b284 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -1528,6 +1528,7 @@ static unsigned long __init
unflatten_dt_node(const void *fdt,
>>      if ( !has_name )
>>      {
>>          char *p1 = pathp, *ps = pathp, *pa = NULL;
>> +        char *tmp = NULL;
>>          int sz;
>>  
>>          while ( *p1 )
>> @@ -1541,25 +1542,21 @@ static unsigned long __init
unflatten_dt_node(const void *fdt,
>>          if ( pa < ps )
>>              pa = p1;
>>          sz = (pa - ps) + 1;
>> -        pp = unflatten_dt_alloc(&mem, sizeof(struct dt_property) +
sz,
>> -                                __alignof__(struct dt_property));
> 
> pp appears to not be assigned anywhere else now? I''m not sure if
prev_pp
> becomes obsolete or not.
pp is also assigned in the loop that create the properties. (see few
lines below in xen/common/device_tree.c). So prev_pp should be kept.
> 
>> +
>> +        tmp = unflatten_dt_alloc(&mem, sz, 1);
>>          if ( allnextpp )
>>          {
>> -            pp->name = "name";
>> -            pp->length = sz;
>> -            pp->value = pp + 1;
>> -            *prev_pp = pp;
>> -            prev_pp = &pp->next;
>> -            memcpy(pp->value, ps, sz - 1);
>> -            ((char *)pp->value)[sz - 1] = 0;
>> -            dt_dprintk("fixed up name for %s -> %s\n",
pathp,
>> -                       (char *)pp->value);
>> +            memcpy(tmp, ps, sz - 1);
>> +            np->name = tmp;
>> +            tmp[sz - 1] = 0;
>> +            dt_dprintk("fixed up name for %s -> %s\n",
pathp, np->name);
>>          }
>>      }
>> +
>>      if ( allnextpp )
>>      {
>>          *prev_pp = NULL;
>> -        np->name = dt_get_property(np, "name", NULL);
>> +        np->name = (np->name) ? : dt_get_property(np,
"name", NULL);
>>          np->type = dt_get_property(np, "device_type",
NULL);
>>  
>>          if ( !np->name )
> 
> 

-- 
Julien Grall

On 08/22/2013 02:21 PM, Ian Campbell wrote:
> <insert which new helpers here>
I will update the commit message on the next patch series.
> I think this is also reimplementing the internals of some existing
> dt_match_foo? And changing set_val into dt_set_cell. IOW there seems to
> be more changes than the single line changelog would suggest.
Except set_val which was rename and exported to __dt_set_cell, nothing
has changed. All the functions are new.
> 
> Are any of these just sync''d from Linux?
Most of them.
> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>> @@ -295,11 +341,29 @@ struct dt_device_node
*dt_find_compatible_node(struct dt_device_node *from,
>>  /**
>>   * Find a property with a given name for a given node
>>   * and return the value.
>> - */
>> +  */
> 
> Oops.
> 
>>  const void *dt_get_property(const struct dt_device_node *np,
>>                              const char *name, u32 *lenp);
>>  
>>  /**
>> + * dt_property_read_string - Find and read a string from a property
>> + * @np:         Device node from which the property value is to be
read
>> + * @propname:   Name of the property to be searched
>> + * @out_string: Pointer to null terminated return string, modified
only
>> + *              if return value if 0.
>> + *
>> + * Search for a property in a device tree node and retrieve a null
>> + * terminated string value (pointer to data, not a copy). Returns 0 on
>> + * sucess, -EINVAL if the property does not exist, -ENODATA if
property
> 
> success
> 
>> + * doest not have value, and -EILSEQ if the string is not
>> + * null-terminated with the length of the property data.
>> + *
>> + * The out_string pointer is modifed only if a valid string can be
decoded.
> 
> modified
> 
>> + */
>> +int dt_property_read_string(const struct dt_device_node *np,
>> +                            const char *propname, const char
**out_string);
>> +
>> +/**
>>   * Checks if the given "compat" string matches one of the
strings in
>>   * the device''s "compatible" property
>>   */
>> @@ -433,4 +497,71 @@ int dt_n_size_cells(const struct dt_device_node
*np);
>>   */
>>  int dt_n_addr_cells(const struct dt_device_node *np);
>>  
>> -#endif
>> +/**
>> + * dt_device_is_available - Check if a device is available for use
>> + *
>> + * @device: Node to check for availability
>> + *
>> + * Returns true if the status property is absent or set to
"okay" or "ok",
>> + * false otherwise.
>> + */
>> +bool_t dt_device_is_available(const struct dt_device_node *device);
>> +
>> +/**
>> + * dt_match_node - Tell if a device_node has a matching of
dt_device_match
>> + * @matches: array of dt_device_match structures to search in
>> + * @node: the dt_device_node structure to match against
>> + *
>> + * Returns true if the device node match one of dt_device_match.
>> + */
>> +bool_t dt_match_node(const struct dt_device_match *matches,
>> +                     const struct dt_device_node *node);
>> +
>> +/**
>> + * dt_find_matching_node - Find a node based on an dt_device_match
match table
>> + * @from: The node to start searching from or NULL, the node you pass
>> + *        will not be searched, only the next one will; typically, you
pass
>> + *        what the returned call returned
>> + * @matches: array of dt_device_match structures to search in
>> + *
>> + * Returns a node pointer.
>> + */
>> +struct dt_device_node *
>> +dt_find_matching_node(struct dt_device_node *from,
>> +                      const struct dt_device_match *matches);
>> +
>> +/**
>> + * dt_set_cell - Write a value into a serie of cells
> 
> series
> 
>> + *
>> + * @cellp: Pointer to cells
>> + *
>> + * Write a value into a series of cells and update cellp to point to
the
>> + * cell just after.
>> + */
>> +void dt_set_cell(__be32 **cellp, int size, u64 val);
>> +
>> +/**
>> + * dt_set_range - Write range into a serie of cells
>> + *
>> + * @cellp: Pointer to cells
>> + * @np: Node which contains the encoding for the address and
>> + * the size
>> + * @address: Start of range
>> + * @size: Size of the range
>> + *
>> + * Write a range into a serie of cells and update cellp to point to
the
> 
> series
> 
>> + * cell just after.
>> + */
>> +void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
>> +                  u64 address, u64 size);
>> +
>> +#endif /* __XEN_DEVICE_TREE_H */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
> 
> 

-- 
Julien Grall

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> Remove the usage of the FDT in benefit of the device tree structure.
"in favour of" is what I think you mean.
> The latter is easier to use and can embedded meta-data for Xen (ie: is the
> device is used by Xen...).
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain_build.c |  270
++++++++++++++++---------------------------
>  1 file changed, 101 insertions(+), 169 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 604ec1c..c8f24ed 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -63,10 +63,10 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
>  }
>  
>  static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo,
> -                             const void *fdt, const u32 *cell, int len,
> -                             int address_cells, int size_cells, u32
*new_cell)
> +                             const struct dt_property *pp,
> +                             const struct dt_device_node *np, __be32
*new_cell)
> {
> -    int reg_size = (address_cells + size_cells) * sizeof(*cell);
> +    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) +
dt_n_size_cells(np));
>      paddr_t start;
>      paddr_t size;
>      struct page_info *pg;
> @@ -90,7 +90,7 @@ static int set_memory_reg_11(struct domain *d, struct
kernel_info *kinfo,
>      if ( res )
>          panic("Unable to add pages in DOM0: %d\n", res);
>  
> -    device_tree_set_reg(&new_cell, address_cells, size_cells, start,
size);
> +    dt_set_range(&new_cell, np, start, size);
>  
>      kinfo->mem.bank[0].start = start;
>      kinfo->mem.bank[0].size = size;
> @@ -100,25 +100,30 @@ static int set_memory_reg_11(struct domain *d, struct
kernel_info *kinfo,
>  }
>  
>  static int set_memory_reg(struct domain *d, struct kernel_info *kinfo,
> -                          const void *fdt, const u32 *cell, int len,
> -                          int address_cells, int size_cells, u32
*new_cell)
> +                          const struct dt_property *pp,
> +                          const struct dt_device_node *np, __be32
*new_cell)
>  {
> -    int reg_size = (address_cells + size_cells) * sizeof(*cell);
> +    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) +
dt_n_size_cells(np));
>      int l = 0;
> +    unsigned int bank = 0;
>      u64 start;
>      u64 size;
> +    int ret;
>  
>      if ( platform_has_quirk(PLATFORM_QUIRK_DOM0_MAPPING_11) )
> -        return set_memory_reg_11(d, kinfo, fdt, cell, len, address_cells,
> -                                 size_cells, new_cell);
> +        return set_memory_reg_11(d, kinfo, pp, np, new_cell);
>  
> -    while ( kinfo->unassigned_mem > 0 && l + reg_size <=
len
> +    while ( kinfo->unassigned_mem > 0 && l + reg_size <=
pp->length
>              && kinfo->mem.nr_banks < NR_MEM_BANKS )
>      {
> -        device_tree_get_reg(&cell, address_cells, size_cells,
&start, &size);
> +        ret = dt_device_get_address(np, bank, &start, &size);
> +        if ( ret )
> +            panic("Unable to retrieve the bank %u for %s\n",
Dropping "the" sounds more natural to me. Perhaps say "memory
bank" too?
 
> -static void make_hypervisor_node(void *fdt, int addrcells, int sizecells)
> +static int make_hypervisor_node(void *fdt, const struct dt_device_node
*parent)
>  {
>      const char compat[] >         
"xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
>          "xen,xen";
> -    u32 reg[4];
> -    u32 intr[3];
> -    u32 *cell;
> +    __be32 reg[4];
> +    __be32 intr[3];
> +    __be32 *cells;
> +    int res;
> +    int addrcells = dt_n_addr_cells(parent);
> +    int sizecells = dt_n_size_cells(parent);
> +
> +    DPRINT("Create hypervisor node\n");
Not sure there is any point in this print unless you also add DPRINT of
the things we put into it.
>  
>      /*
>       * Sanity-check address sizes, since addresses and sizes which do
> [...]
> izecells));
> +    cells = &reg[0];
> +    dt_set_cell(&cells, addrcells, 0xb0000000);
> +    dt_set_cell(&cells, sizecells, 0x20000);
Aside: this really ought to become dynamic, based on finding a hole in
the physical address map...

[...]
> +    res = fdt_property(fdt, "interrupts", intr, sizeof(intr[0])
* 3);
> +    if ( res )
> +        return res;
the * 3 come from the interrupt-controller nodes properties I think?
Should we assert somewhere that they match? Perhaps we would already die
if it weren''t anyway?
> @@ -454,7 +374,8 @@ static int handle_node(struct domain *d, const struct
dt_device_node *np)
>      if ( dt_match_node(skip_matches, np ) )
>          return 0;
>  
> -    if ( dt_device_used_by(np) != DOMID_XEN )
> +    if ( dt_device_used_by(np) != DOMID_XEN &&
> +         !dt_device_type_is_equal(np, "memory") )
Can we get a comment about why memory is special here please?
>      {
>          res = map_device(d, np);
>  
Ian.

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> When a device has a property status with disabled inside, Linux will not
use
> the device.
Is Linux consistent about that?

I must admit I thought we omitted things in use by Xen from the DT
altogether. Is that no longer true?
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain_build.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c8f24ed..8377610 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -208,6 +208,14 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
>              return res;
>      }
>  
> +    /* Disable all devices used by Xen */
> +    if ( dt_device_used_by(np) == DOMID_XEN )
> +    {
> +        res = fdt_property(kinfo->fdt, "status",
"disabled", 8 + 1);
> +        if ( res )
> +            return res;
> +    }
> +
>      /*
>       * XXX should populate /chosen/linux,initrd-{start,end} here if we
>       * have module[2]

On 08/22/2013 02:23 PM, Ian Campbell wrote:
> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>> The function device_get_reg will be removed in a future patch.
> 
> Why? It appears to be a useful helper.
This helper is only used in few places and can be replaced by:
	- dt_read_number because #address-cells and #size-cells have no meaning
        - 2 consecutive call to dt_next_cell
        - dt_get_address
> 
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/common/device_tree.c |    7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 95635f0..ea01a5a 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -385,7 +385,7 @@ static void __init process_memory_node(const void
*fdt, int node,
>>      const struct fdt_property *prop;
>>      int i;
>>      int banks;
>> -    const u32 *cell;
>> +    const __be32 *cell;
>>      paddr_t start, size;
>>  
>>      if ( address_cells < 1 || size_cells < 1 )
>> @@ -402,12 +402,13 @@ static void __init process_memory_node(const void
*fdt, int node,
>>          return;
>>      }
>>  
>> -    cell = (const u32 *)prop->data;
>> +    cell = (const __be32 *)prop->data;
>>      banks = device_tree_nr_reg_ranges(prop, address_cells,
size_cells);
>>  
>>      for ( i = 0; i < banks && early_info.mem.nr_banks <
NR_MEM_BANKS; i++ )
>>      {
>> -        device_tree_get_reg(&cell, address_cells, size_cells,
&start, &size);
>> +        start = dt_next_cell(address_cells, &cell);
>> +        size = dt_next_cell(size_cells, &cell);
>>          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++;
> 
> 

-- 
Julien Grall

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> Each platform code will list the device that must not pass-through to a
guest.
> Theses devices are used for: power management, timer,...
> 
> When theses devices are given to DOM0, it can controls the hardware and
then
> break the whole platform.
This will do for now. Eventually I think we will want to derive this
from the set of devices used by Xen following the links specified in the
clock and PM bindings etc.

We will also eventually want to implement some per-device MMIO filtering
to allow dom0 to continue to control devices which it owns which happen
to share e.g. a clk controller with Xen.

(could expound on this in the commit message for context perhaps?)
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain_build.c    |    2 +-
>  xen/arch/arm/platform.c        |   10 ++++++++++
>  xen/include/asm-arm/platform.h |    7 +++++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d8d67a6..93f77c6 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -419,7 +419,7 @@ static int handle_node(struct domain *d, struct
kernel_info *kinfo,
>      DPRINT("handle %s\n", dt_node_full_name(np));
>  
>      /* Skip theses nodes and the sub-nodes */
> -    if ( dt_match_node(skip_matches, np ) )
> +    if ( dt_match_node(skip_matches, np ) ||
platform_device_is_blacklist(np) )
>          return 0;
>  
>      if ( dt_device_used_by(np) != DOMID_XEN &&
> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> index afda302..cae0580 100644
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -127,6 +127,16 @@ bool_t platform_has_quirk(uint32_t quirk)
>      return !!(quirks & quirk);
>  }
>  
> +bool_t platform_device_is_blacklist(const struct dt_device_node *node)
...._is_blacklisted.
> +{
> +    const struct dt_device_match *blacklist = NULL;
> +
> +    if ( platform && platform->blacklist_dev )
> +        blacklist = platform->blacklist_dev;
> +
> +    return dt_match_node(blacklist, node);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-arm/platform.h
b/xen/include/asm-arm/platform.h
> index f460e9c..4b511ed 100644
> --- a/xen/include/asm-arm/platform.h
> +++ b/xen/include/asm-arm/platform.h
> @@ -4,6 +4,7 @@
>  #include <xen/init.h>
>  #include <xen/sched.h>
>  #include <xen/mm.h>
> +#include <xen/device_tree.h>
>  
>  /* Describe specific operation for a board */
>  struct platform_desc {
> @@ -26,6 +27,11 @@ struct platform_desc {
>       * board with different quirk on each
>       */
>      uint32_t (*quirks)(void);
> +    /*
> +     * Platform blacklist devices
"Blacklisted platform devices"
> +     * List of devices which must not pass-through to a guest
> +     */
> +    const struct dt_device_match *blacklist_dev;
Either "blacklisted_devs" or just "blacklist" I think. The
former sounds
a bit better to me.
>  };
>  
>  /*
> @@ -40,6 +46,7 @@ int __init platform_specific_mapping(struct domain *d);
>  void platform_reset(void);
>  void platform_poweroff(void);
>  bool_t platform_has_quirk(uint32_t quirk);
> +bool_t platform_device_is_blacklist(const struct dt_device_node *node);
>  
>  #define PLATFORM_START(_name, _namestr)                         \
>  static const struct platform_desc  __plat_desc_##_name __used   \

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> On Versatile there is a bunch of devices that must not pass-through to any
"there are a bunch of devices which must not be passed-through"
> guest (power management and cache coherency devices).
> 
> This commit also blacklist the HDLCD device because then is unable to
correctly
                                                          ^Linux?
> map the framebuffer. Therefore, when Linux will try to access to the
framebuffer,
> Xen will receive a non-handled data access.
Can/should this be conditional on whether Xen has console=hdlcd or not?
Or does Xen use the device unconditionally if it exists? TBH I think it
would be normal to prefer that Linux gets this device...

(I''m unclear how this relates to memreserve as mention in the code
comment)
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/platforms/vexpress.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/xen/arch/arm/platforms/vexpress.c
b/xen/arch/arm/platforms/vexpress.c
> index 8fc30c4..ece7bd9 100644
> --- a/xen/arch/arm/platforms/vexpress.c
> +++ b/xen/arch/arm/platforms/vexpress.c
> @@ -125,9 +125,26 @@ static const char const *vexpress_dt_compat[]
__initdata >      NULL
>  };
>  
> +static const struct dt_device_match vexpress_blacklist_dev[] __initconst
> +{
> +    /* Cache Coherent Interconnect */
> +    DT_MATCH_COMPATIBLE("arm,cci-400"),
> +    DT_MATCH_COMPATIBLE("arm,cci-400-pmu"),
> +    /* Video device
> +     * TODO: remove it once memreserve is handled properly by Xen
> +     */
> +    DT_MATCH_COMPATIBLE("arm,hdlcd"),
> +    /* Hardware power management */
> +    DT_MATCH_COMPATIBLE("arm,vexpress-reset"),
> +    DT_MATCH_COMPATIBLE("arm,vexpress-reboot"),
> +    DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"),
> +    { /* sentinel */ },
> +};
> +
>  PLATFORM_START(vexpress, "VERSATILE EXPRESS")
>      .compatible = vexpress_dt_compat,
>      .reset = vexpress_reset,
> +    .blacklist_dev = vexpress_blacklist_dev,
>  PLATFORM_END
>  
>  /*

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> All Xen code has been converted to the new device tree API that uses a tree
> structure to describe the DTS.
> 
> The Flat Device tree is still used by Xen during early boot stage, but only
in
> internal. Remove entirely unneeded functions or move to a static function.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/common/device_tree.c      |  109
+++++------------------------------------
>  xen/include/xen/device_tree.h |   15 ------
>  2 files changed, 12 insertions(+), 112 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 6bdab14..d4a821b 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -103,8 +103,8 @@ struct dt_bus
>      unsigned int (*get_flags)(const __be32 *addr);
>  };
>  
> -bool_t __init device_tree_node_matches(const void *fdt, int node,
> -                                       const char *match)
> +static bool_t __init device_tree_node_matches(const void *fdt, int node,
> +                                              const char *match)
>  {
>      const char *name;
>      size_t match_len;
> @@ -118,7 +118,7 @@ bool_t __init device_tree_node_matches(const void *fdt,
int node,
>          && (name[match_len] == ''@'' ||
name[match_len] == ''\0'');
>  }
>  
> -bool_t __init device_tree_type_matches(const void *fdt, int node,
> +static bool_t __init device_tree_type_matches(const void *fdt, int node,
>                                         const char *match)
>  {
>      const void *prop;
> @@ -130,8 +130,8 @@ bool_t __init device_tree_type_matches(const void *fdt,
int node,
>      return !dt_node_cmp(prop, match);
>  }
>  
> -bool_t __init device_tree_node_compatible(const void *fdt, int node,
> -                                          const char *match)
> +static bool_t __init device_tree_node_compatible(const void *fdt, int
node,
> +                                                 const char *match)
>  {
>      int len, l;
>      int mlen;
> @@ -154,34 +154,6 @@ bool_t __init device_tree_node_compatible(const void
*fdt, int node,
>      return 0;
>  }
>  
> -static __init int device_tree_nr_reg_ranges(const struct fdt_property
*prop,
> -        u32 address_cells, u32 size_cells)
> -{
> -    u32 reg_cells = address_cells + size_cells;
> -    return fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32));
> -}
> -
> -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;
> -        *val |= fdt32_to_cpu(*(*cell)++);
> -    }
> -}
> -
> -void __init 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);
> -}
> -
>  void dt_set_cell(__be32 **cellp, int size, u64 val)
>  {
>      int cells = size;
> @@ -195,13 +167,6 @@ void dt_set_cell(__be32 **cellp, int size, u64 val)
>      (*cellp) += cells;
>  }
>  
> -void __init device_tree_set_reg(u32 **cell, u32 address_cells, u32
size_cells,
> -                                u64 start, u64 size)
> -{
> -    dt_set_cell(cell, address_cells, start);
> -    dt_set_cell(cell, size_cells, size);
> -}
> -
>  void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
>                    u64 address, u64 size)
>  {
> @@ -209,8 +174,8 @@ void dt_set_range(__be32 **cellp, const struct
dt_device_node *np,
>      dt_set_cell(cellp, dt_n_size_cells(np), size);
>  }
>  
> -u32 __init device_tree_get_u32(const void *fdt, int node, const char
*prop_name,
> -                               u32 dflt)
> +static u32 __init device_tree_get_u32(const void *fdt, int node,
> +                                      const char *prop_name, u32 dflt)
>  {
>      const struct fdt_property *prop;
>  
> @@ -232,8 +197,9 @@ u32 __init device_tree_get_u32(const void *fdt, int
node, const char *prop_name,
>   * Returns 0 if all nodes were iterated over successfully.  If @func
>   * returns a value different from 0, that value is returned immediately.
>   */
> -int __init device_tree_for_each_node(const void *fdt,
> -                                     device_tree_node_func func, void
*data)
> +static int __init device_tree_for_each_node(const void *fdt,
> +                                            device_tree_node_func func,
> +                                            void *data)
>  {
>      int node;
>      int depth;
> @@ -268,58 +234,6 @@ int __init device_tree_for_each_node(const void *fdt,
>      return 0;
>  }
>  
> -struct find_compat {
> -    const char *compatible;
> -    int found;
> -    int node;
> -    int depth;
> -    u32 address_cells;
> -    u32 size_cells;
> -};
> -
> -static int _find_compatible_node(const void *fdt,
> -                             int node, const char *name, int depth,
> -                             u32 address_cells, u32 size_cells,
> -                             void *data)
> -{
> -    struct find_compat *c = (struct find_compat *) data;
> -
> -    if (  c->found  )
> -        return 1;
> -
> -    if ( device_tree_node_compatible(fdt, node, c->compatible) )
> -    {
> -        c->found = 1;
> -        c->node = node;
> -        c->depth = depth;
> -        c->address_cells = address_cells;
> -        c->size_cells = size_cells;
> -        return 1;
> -    }
> -    return 0;
> -}
> -
> -int __init find_compatible_node(const char *compatible, int *node, int
*depth,
> -                                u32 *address_cells, u32 *size_cells)
> -{
> -    int ret;
> -    struct find_compat c;
> -    c.compatible = compatible;
> -    c.found = 0;
> -
> -    ret = device_tree_for_each_node(device_tree_flattened,
_find_compatible_node, &c);
> -    if ( !c.found )
> -        return ret;
> -    else
> -    {
> -        *node = c.node;
> -        *depth = c.depth;
> -        *address_cells = c.address_cells;
> -        *size_cells = c.size_cells;
> -        return 1;
> -    }
> -}
> -
>  /**
>   * device_tree_bootargs - return the bootargs (the Xen command line)
>   * @fdt flat device tree.
> @@ -387,6 +301,7 @@ static void __init process_memory_node(const void *fdt,
int node,
>      int banks;
>      const __be32 *cell;
>      paddr_t start, size;
> +    u32 reg_cells = address_cells + size_cells;
>  
>      if ( address_cells < 1 || size_cells < 1 )
>      {
> @@ -403,7 +318,7 @@ static void __init process_memory_node(const void *fdt,
int node,
>      }
>  
>      cell = (const __be32 *)prop->data;
> -    banks = device_tree_nr_reg_ranges(prop, address_cells, size_cells);
> +    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
>  
>      for ( i = 0; i < banks && early_info.mem.nr_banks <
NR_MEM_BANKS; i++ )
>      {
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 22a2973..c61c917 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -175,21 +175,6 @@ extern const void *device_tree_flattened;
>  
>  size_t __init device_tree_early_init(const void *fdt);
>  
> -void __init device_tree_get_reg(const u32 **cell, u32 address_cells,
> -                                u32 size_cells,
> -                                u64 *start, u64 *size);
> -void __init device_tree_set_reg(u32 **cell, u32 address_cells, u32
size_cells,
> -                                u64 start, u64 size);
> -u32 __init device_tree_get_u32(const void *fdt, int node,
> -                               const char *prop_name, u32 dflt);
> -bool_t __init device_tree_node_matches(const void *fdt, int node,
> -                                       const char *match);
> -bool_t __init device_tree_node_compatible(const void *fdt, int node,
> -                                          const char *match);
> -int __init find_compatible_node(const char *compatible, int *node, int
*depth,
> -                                u32 *address_cells, u32 *size_cells);
> -int __init device_tree_for_each_node(const void *fdt,
> -                                     device_tree_node_func func, void
*data);
>  const char __init *device_tree_bootargs(const void *fdt);
>  void __init device_tree_dump(const void *fdt);
>

On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> It''s possible to have a device description in the DTS but the
device is not
> wired.
> 
> device_init must check if the device is available before doing anything
with
> it.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

On 08/22/2013 02:28 PM, Ian Campbell wrote:
> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>> Avoid to use FDT API which will be removed soon.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/drivers/video/arm_hdlcd.c |   58
+++++++++++++++++++++++------------------
>>  1 file changed, 32 insertions(+), 26 deletions(-)
>>
>> diff --git a/xen/drivers/video/arm_hdlcd.c
b/xen/drivers/video/arm_hdlcd.c
>> index 72979ea..0ff8c22 100644
>> --- a/xen/drivers/video/arm_hdlcd.c
>> +++ b/xen/drivers/video/arm_hdlcd.c
>> @@ -25,6 +25,7 @@
>>  #include <xen/libfdt/libfdt.h>
>>  #include <xen/init.h>
>>  #include <xen/mm.h>
>> +#include <asm/early_printk.h>
>>  #include "font.h"
>>  #include "lfb.h"
>>  #include "modelines.h"
>> @@ -96,62 +97,67 @@ static int __init get_color_masks(const char* bpp,
struct color_masks **masks)
>>  
>>  static void __init set_pixclock(uint32_t pixclock)
>>  {
>> -    if ( device_tree_node_compatible(device_tree_flattened, 0,
"arm,vexpress") )
>> +    if ( dt_find_compatible_node(NULL, NULL, "arm,vexpress")
)
>>              vexpress_syscfg(1, V2M_SYS_CFG_OSC_FUNC,
>>                              V2M_SYS_CFG_OSC5, &pixclock);
>>  }
>>  
>>  void __init video_init(void)
>>  {
>> -    int node, depth;
>> -    u32 address_cells, size_cells;
>>      struct lfb_prop lfbp;
>>      unsigned char *lfb;
>> -    paddr_t hdlcd_start, hdlcd_size;
>> +    u64 hdlcd_start, hdlcd_size;
> 
> Why? These are physical addresses.
dt_get_address take a pointer to u64 value (actually paddr_t == u64).
Perhaps dt_get_addres should take a paddr_t.
> 
>>      if ( !hdlcd_start )
>>      {
>> -        printk(KERN_ERR "HDLCD address missing from device tree,
disabling driver\n");
>> +        early_printk("hdlcd: address missing from device tree,
disabling driver\n");
> 
> I suppose this (and the other instances of this) is for when the console
> is on HDLCD? That''s a separate fix really I think.
The console is not yet set up when the video driver is initialized.

I will divide this patch in 2 parts.
> 
> Why tr /A-Z/ /a-z/?
To keep all printks with the same format. I will stick to HDLCD then.

-- 
Julien Grall

On 08/22/2013 02:30 PM, Ian Campbell wrote:
> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>> There is some place in Xen ARM code where multiple if conditions is
used
>> check the presence of a node or find a node.
>> These pieces of code can be replace by an array and using proper device
tree
>> helpers.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/domain_build.c |   12 +++++++++---
>>  xen/arch/arm/time.c         |   11 ++++++++---
>>  2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index a324c3b..604ec1c 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -435,6 +435,14 @@ static int map_device(struct domain *d, const
struct dt_device_node *dev)
>>      return 0;
>>  }
>>  
>> +static const struct dt_device_match skip_matches[] __initconst
>> +{
>> +    DT_MATCH_COMPATIBLE("xen,xen"),
>> +    DT_MATCH_TYPE("memory"),
>> +    DT_MATCH_PATH("/chosen"),
>> +    { /* sentinel */ },
>> +};
> 
> Can this not be within the scope of the only function which uses it?
Indeed. I will update it for the next patch series.
> 
> If not then perhaps it can be given a more meaningful name.
> dom0_skip_matches?
> 
>> +
>>  static int handle_node(struct domain *d, const struct dt_device_node
*np)
>>  {
>>      const struct dt_device_node *child;
>> @@ -443,9 +451,7 @@ static int handle_node(struct domain *d, const
struct dt_device_node *np)
>>      DPRINT("handle %s\n", dt_node_full_name(np));
>>  
>>      /* Skip theses nodes and the sub-nodes */
>> -    if ( dt_device_is_compatible(np, "xen,xen") ||
>> -         dt_device_type_is_equal(np, "memory") ||
>> -         !strcmp("/chosen", dt_node_full_name(np)) )
>> +    if ( dt_match_node(skip_matches, np ) )
>>          return 0;
>>  
>>      if ( dt_device_used_by(np) != DOMID_XEN )
>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>> index b5864ef..05e58ba 100644
>> --- a/xen/arch/arm/time.c
>> +++ b/xen/arch/arm/time.c
>> @@ -98,6 +98,13 @@ static uint32_t calibrate_timer(void)
>>  }
>>  */
>>  
>> +static const struct dt_device_match timer_ids[] __initdata >> +{
>> +    DT_MATCH_COMPATIBLE("arm,armv7-timer"),
>> +    DT_MATCH_COMPATIBLE("arm,armv8-timer"),
>> +    { /* sentinel */ },
>> +};
>> +
>>  /* Set up the timer on the boot CPU */
>>  int __init init_xen_time(void)
>>  {
>> @@ -105,9 +112,7 @@ int __init init_xen_time(void)
>>      int res;
>>      unsigned int i;
>>  
>> -    dev = dt_find_compatible_node(NULL, NULL,
"arm,armv7-timer");
>> -    if ( !dev )
>> -        dev = dt_find_compatible_node(NULL, NULL,
"arm,armv8-timer");
>> +    dev = dt_find_matching_node(NULL, timer_ids);
>>      if ( !dev )
>>          panic("Unable to find a compatible timer in the device
tree\n");
>>  
> 
> 

-- 
Julien Grall

On Thu, 2013-08-22 at 14:14 +0100, Julien Grall wrote:
> On 08/22/2013 01:51 PM, Ian Campbell wrote:
> > On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> >> The properties #address-cells and #size-cells is not required in
/cpus node in
> >> the device tree (see Linux
Documentation/devicetree/booting-without-of.txt
> >> Section III.5.a).
> > 
> > This isn''t quite accurate, the properties must exist
somewhere, if not
> > here then in the parent node, recursively, otherwise we cannot
interpret
> > the reg properties etc. What isn''t required is that they have
the
> > particular values we are checking for.
> 
> What about:
> 
> The properties #address-cells and #size-cells may not correctly defines
> for CPUs node (see /Documentation/devicetree/booting-without-of.txt).
I don''t think that''s quite right either. #address-calls is
still defined
for /cpus/ even if /cpus/ doesn''t itself contain a that property,
because the parent''s #address-cells is used in that case (I think
recursively, so grand-parents if the parent doesn''t have it,
I''m not
100% sure of that though).

I think you just want to say something along the lines of "CPU nodes are
not required to have #address-cells == 1 and #size-cells == 0, so don''t
check for that".
> 
> >>
> >> In the OMAP5 device tree, these 2 properties are not defined.
Therefore, Xen
> >> will only able to handle 1 CPU.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> CC: andrii.anisov@globallogic.com
> >> CC: baozich@gmail.com
> >> ---
> >>  xen/common/device_tree.c |    6 ------
> >>  1 file changed, 6 deletions(-)
> >>
> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> >> index 84d704d..51f23eb 100644
> >> --- a/xen/common/device_tree.c
> >> +++ b/xen/common/device_tree.c
> >> @@ -414,12 +414,6 @@ static void __init process_cpu_node(const
void *fdt, int node,
> >>      const u32 *cell;
> >>      paddr_t start, size;
> >>  
> >> -    if ( address_cells != 1 || size_cells != 0 )
> >> -    {
> >> -        early_printk("fdt: node `%s'': invalid
#address-cells or #size-cells",
> >> -                     name);
> >> -        return;
> >> -    }
> >>  
> >>      prop = fdt_get_property(fdt, node, "reg", NULL);
> >>      if ( !prop )
> > 
> > 
> 
>

On Thu, 2013-08-22 at 14:35 +0100, Julien Grall wrote:
> On 08/22/2013 02:05 PM, Ian Campbell wrote:
> > On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> >> The Flat Device Tree is given by the bootloader. Xen
doesn''t need to modify it.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> >> @@ -363,8 +364,9 @@ static void __init setup_mm(unsigned long
dtb_paddr, size_t dtb_size)
> >>       *
> >>       * TODO: handle other payloads too.
> >>       */
> >> -    device_tree_flattened =
mfn_to_virt(alloc_boot_pages(dtb_pages, 1));
> >> -    copy_from_paddr(device_tree_flattened, dtb_paddr, dtb_size,
BUFFERABLE);
> >> +    fdt = mfn_to_virt(alloc_boot_pages(dtb_pages, 1));
> >> +    copy_from_paddr(fdt, dtb_paddr, dtb_size, BUFFERABLE);
> >> +    device_tree_flattened = fdt;
> > 
> > Not related to this patch but I wonder if we ought to alloc_boot_pages
> > (order_of(dtb_size)) or at least BUG_ON(dtb_size > PAGE_SIZE).
> 
> The last solution can''t work. The size of device tree is about
32K, so
> greater than one page.
I was being thick, I thought the "1" was the allocation size, missing
the fact that "dtb_pages" was right there staring me in the face. (FTR
1
is the alignment...)

Ian

On Thu, 2013-08-22 at 14:43 +0100, Julien Grall wrote:
> On 08/22/2013 02:16 PM, Ian Campbell wrote:
> > On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> >> On new Flat Device Tree version, the property "name" may
not exist.
> >> The property is never used in Xen code except to set the field
"name" of
> >> dt_device_node.
> >>
> >> For convenience, remove the fake property. It will save space
during the
> >> creation of the dom0 FDT.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> ---
> >>  xen/common/device_tree.c |   21 +++++++++------------
> >>  1 file changed, 9 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> >> index 362dd66..315b284 100644
> >> --- a/xen/common/device_tree.c
> >> +++ b/xen/common/device_tree.c
> >> @@ -1528,6 +1528,7 @@ static unsigned long __init
unflatten_dt_node(const void *fdt,
> >>      if ( !has_name )
> >>      {
> >>          char *p1 = pathp, *ps = pathp, *pa = NULL;
> >> +        char *tmp = NULL;
> >>          int sz;
> >>  
> >>          while ( *p1 )
> >> @@ -1541,25 +1542,21 @@ static unsigned long __init
unflatten_dt_node(const void *fdt,
> >>          if ( pa < ps )
> >>              pa = p1;
> >>          sz = (pa - ps) + 1;
> >> -        pp = unflatten_dt_alloc(&mem, sizeof(struct
dt_property) + sz,
> >> -                                __alignof__(struct dt_property));
> > 
> > pp appears to not be assigned anywhere else now? I''m not sure
if prev_pp
> > becomes obsolete or not.
> 
> pp is also assigned in the loop that create the properties. (see few
> lines below in xen/common/device_tree.c). So prev_pp should be kept.
Oh yes, missed that one. thanks.

Ian.
> 
> > 
> >> +
> >> +        tmp = unflatten_dt_alloc(&mem, sz, 1);
> >>          if ( allnextpp )
> >>          {
> >> -            pp->name = "name";
> >> -            pp->length = sz;
> >> -            pp->value = pp + 1;
> >> -            *prev_pp = pp;
> >> -            prev_pp = &pp->next;
> >> -            memcpy(pp->value, ps, sz - 1);
> >> -            ((char *)pp->value)[sz - 1] = 0;
> >> -            dt_dprintk("fixed up name for %s ->
%s\n", pathp,
> >> -                       (char *)pp->value);
> >> +            memcpy(tmp, ps, sz - 1);
> >> +            np->name = tmp;
> >> +            tmp[sz - 1] = 0;
> >> +            dt_dprintk("fixed up name for %s ->
%s\n", pathp, np->name);
> >>          }
> >>      }
> >> +
> >>      if ( allnextpp )
> >>      {
> >>          *prev_pp = NULL;
> >> -        np->name = dt_get_property(np, "name",
NULL);
> >> +        np->name = (np->name) ? : dt_get_property(np,
"name", NULL);
> >>          np->type = dt_get_property(np,
"device_type", NULL);
> >>  
> >>          if ( !np->name )
> > 
> > 
> 
>

On Thu, 2013-08-22 at 14:48 +0100, Julien Grall wrote:
> On 08/22/2013 02:21 PM, Ian Campbell wrote:
> > <insert which new helpers here>
> 
> I will update the commit message on the next patch series.
> 
> > I think this is also reimplementing the internals of some existing
> > dt_match_foo? And changing set_val into dt_set_cell. IOW there seems
to
> > be more changes than the single line changelog would suggest.
> 
> Except set_val which was rename and exported to __dt_set_cell, nothing
> has changed. All the functions are new.
OK.
> > 
> > Are any of these just sync''d from Linux?
> 
> Most of them.
THIs is worth mentioning in the commit message, along with a rough Linux
version number, in case we come back and need to sync bug fixes or
something
> 
> > On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> >> @@ -295,11 +341,29 @@ struct dt_device_node
*dt_find_compatible_node(struct dt_device_node *from,
> >>  /**
> >>   * Find a property with a given name for a given node
> >>   * and return the value.
> >> - */
> >> +  */
> > 
> > Oops.
> > 
> >>  const void *dt_get_property(const struct dt_device_node *np,
> >>                              const char *name, u32 *lenp);
> >>  
> >>  /**
> >> + * dt_property_read_string - Find and read a string from a
property
> >> + * @np:         Device node from which the property value is to
be read
> >> + * @propname:   Name of the property to be searched
> >> + * @out_string: Pointer to null terminated return string,
modified only
> >> + *              if return value if 0.
> >> + *
> >> + * Search for a property in a device tree node and retrieve a
null
> >> + * terminated string value (pointer to data, not a copy). Returns
0 on
> >> + * sucess, -EINVAL if the property does not exist, -ENODATA if
property
> > 
> > success
> > 
> >> + * doest not have value, and -EILSEQ if the string is not
> >> + * null-terminated with the length of the property data.
> >> + *
> >> + * The out_string pointer is modifed only if a valid string can
be decoded.
> > 
> > modified
> > 
> >> + */
> >> +int dt_property_read_string(const struct dt_device_node *np,
> >> +                            const char *propname, const char
**out_string);
> >> +
> >> +/**
> >>   * Checks if the given "compat" string matches one of
the strings in
> >>   * the device''s "compatible" property
> >>   */
> >> @@ -433,4 +497,71 @@ int dt_n_size_cells(const struct
dt_device_node *np);
> >>   */
> >>  int dt_n_addr_cells(const struct dt_device_node *np);
> >>  
> >> -#endif
> >> +/**
> >> + * dt_device_is_available - Check if a device is available for
use
> >> + *
> >> + * @device: Node to check for availability
> >> + *
> >> + * Returns true if the status property is absent or set to
"okay" or "ok",
> >> + * false otherwise.
> >> + */
> >> +bool_t dt_device_is_available(const struct dt_device_node
*device);
> >> +
> >> +/**
> >> + * dt_match_node - Tell if a device_node has a matching of
dt_device_match
> >> + * @matches: array of dt_device_match structures to search in
> >> + * @node: the dt_device_node structure to match against
> >> + *
> >> + * Returns true if the device node match one of dt_device_match.
> >> + */
> >> +bool_t dt_match_node(const struct dt_device_match *matches,
> >> +                     const struct dt_device_node *node);
> >> +
> >> +/**
> >> + * dt_find_matching_node - Find a node based on an
dt_device_match match table
> >> + * @from: The node to start searching from or NULL, the node you
pass
> >> + *        will not be searched, only the next one will;
typically, you pass
> >> + *        what the returned call returned
> >> + * @matches: array of dt_device_match structures to search in
> >> + *
> >> + * Returns a node pointer.
> >> + */
> >> +struct dt_device_node *
> >> +dt_find_matching_node(struct dt_device_node *from,
> >> +                      const struct dt_device_match *matches);
> >> +
> >> +/**
> >> + * dt_set_cell - Write a value into a serie of cells
> > 
> > series
> > 
> >> + *
> >> + * @cellp: Pointer to cells
> >> + *
> >> + * Write a value into a series of cells and update cellp to point
to the
> >> + * cell just after.
> >> + */
> >> +void dt_set_cell(__be32 **cellp, int size, u64 val);
> >> +
> >> +/**
> >> + * dt_set_range - Write range into a serie of cells
> >> + *
> >> + * @cellp: Pointer to cells
> >> + * @np: Node which contains the encoding for the address and
> >> + * the size
> >> + * @address: Start of range
> >> + * @size: Size of the range
> >> + *
> >> + * Write a range into a serie of cells and update cellp to point
to the
> > 
> > series
> > 
> >> + * cell just after.
> >> + */
> >> +void dt_set_range(__be32 **cellp, const struct dt_device_node
*np,
> >> +                  u64 address, u64 size);
> >> +
> >> +#endif /* __XEN_DEVICE_TREE_H */
> >> +
> >> +/*
> >> + * Local variables:
> >> + * mode: C
> >> + * c-file-style: "BSD"
> >> + * c-basic-offset: 4
> >> + * indent-tabs-mode: nil
> >> + * End:
> >> + */
> > 
> > 
> 
>

On Thu, 2013-08-22 at 14:54 +0100, Julien Grall wrote:
> On 08/22/2013 02:23 PM, Ian Campbell wrote:
> > On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> >> The function device_get_reg will be removed in a future patch.
> > 
> > Why? It appears to be a useful helper.
> 
> This helper is only used in few places and can be replaced by:
> 	- dt_read_number because #address-cells and #size-cells have no meaning
>         - 2 consecutive call to dt_next_cell
It seemed like a helper for this case would remain useful, even if it
isn''t exactly device_get_reg. Up to you though.
>         - dt_get_address
> 
> > 
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> ---
> >>  xen/common/device_tree.c |    7 ++++---
> >>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> >> index 95635f0..ea01a5a 100644
> >> --- a/xen/common/device_tree.c
> >> +++ b/xen/common/device_tree.c
> >> @@ -385,7 +385,7 @@ static void __init process_memory_node(const
void *fdt, int node,
> >>      const struct fdt_property *prop;
> >>      int i;
> >>      int banks;
> >> -    const u32 *cell;
> >> +    const __be32 *cell;
> >>      paddr_t start, size;
> >>  
> >>      if ( address_cells < 1 || size_cells < 1 )
> >> @@ -402,12 +402,13 @@ static void __init process_memory_node(const
void *fdt, int node,
> >>          return;
> >>      }
> >>  
> >> -    cell = (const u32 *)prop->data;
> >> +    cell = (const __be32 *)prop->data;
> >>      banks = device_tree_nr_reg_ranges(prop, address_cells,
size_cells);
> >>  
> >>      for ( i = 0; i < banks && early_info.mem.nr_banks
< NR_MEM_BANKS; i++ )
> >>      {
> >> -        device_tree_get_reg(&cell, address_cells, size_cells,
&start, &size);
> >> +        start = dt_next_cell(address_cells, &cell);
> >> +        size = dt_next_cell(size_cells, &cell);
> >>          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++;
> > 
> > 
> 
>

On 08/22/2013 02:49 PM, Ian Campbell wrote:
> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>> Remove the usage of the FDT in benefit of the device tree structure.
> 
> "in favour of" is what I think you mean.
> 
>> The latter is easier to use and can embedded meta-data for Xen (ie: is
the
>> device is used by Xen...).
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/domain_build.c |  270
++++++++++++++++---------------------------
>>  1 file changed, 101 insertions(+), 169 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 604ec1c..c8f24ed 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -63,10 +63,10 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
>>  }
>>  
>>  static int set_memory_reg_11(struct domain *d, struct kernel_info
*kinfo,
>> -                             const void *fdt, const u32 *cell, int
len,
>> -                             int address_cells, int size_cells, u32
*new_cell)
>> +                             const struct dt_property *pp,
>> +                             const struct dt_device_node *np, __be32
*new_cell)
>> {
>> -    int reg_size = (address_cells + size_cells) * sizeof(*cell);
>> +    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) +
dt_n_size_cells(np));
>>      paddr_t start;
>>      paddr_t size;
>>      struct page_info *pg;
>> @@ -90,7 +90,7 @@ static int set_memory_reg_11(struct domain *d, struct
kernel_info *kinfo,
>>      if ( res )
>>          panic("Unable to add pages in DOM0: %d\n", res);
>>  
>> -    device_tree_set_reg(&new_cell, address_cells, size_cells,
start, size);
>> +    dt_set_range(&new_cell, np, start, size);
>>  
>>      kinfo->mem.bank[0].start = start;
>>      kinfo->mem.bank[0].size = size;
>> @@ -100,25 +100,30 @@ static int set_memory_reg_11(struct domain *d,
struct kernel_info *kinfo,
>>  }
>>  
>>  static int set_memory_reg(struct domain *d, struct kernel_info *kinfo,
>> -                          const void *fdt, const u32 *cell, int len,
>> -                          int address_cells, int size_cells, u32
*new_cell)
>> +                          const struct dt_property *pp,
>> +                          const struct dt_device_node *np, __be32
*new_cell)
>>  {
>> -    int reg_size = (address_cells + size_cells) * sizeof(*cell);
>> +    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) +
dt_n_size_cells(np));
>>      int l = 0;
>> +    unsigned int bank = 0;
>>      u64 start;
>>      u64 size;
>> +    int ret;
>>  
>>      if ( platform_has_quirk(PLATFORM_QUIRK_DOM0_MAPPING_11) )
>> -        return set_memory_reg_11(d, kinfo, fdt, cell, len,
address_cells,
>> -                                 size_cells, new_cell);
>> +        return set_memory_reg_11(d, kinfo, pp, np, new_cell);
>>  
>> -    while ( kinfo->unassigned_mem > 0 && l + reg_size
<= len
>> +    while ( kinfo->unassigned_mem > 0 && l + reg_size
<= pp->length
>>              && kinfo->mem.nr_banks < NR_MEM_BANKS )
>>      {
>> -        device_tree_get_reg(&cell, address_cells, size_cells,
&start, &size);
>> +        ret = dt_device_get_address(np, bank, &start, &size);
>> +        if ( ret )
>> +            panic("Unable to retrieve the bank %u for %s\n",
> 
> Dropping "the" sounds more natural to me. Perhaps say
"memory bank" too?
I will fix it.
>  
>> -static void make_hypervisor_node(void *fdt, int addrcells, int
sizecells)
>> +static int make_hypervisor_node(void *fdt, const struct dt_device_node
*parent)
>>  {
>>      const char compat[] >>         
"xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
>>          "xen,xen";
>> -    u32 reg[4];
>> -    u32 intr[3];
>> -    u32 *cell;
>> +    __be32 reg[4];
>> +    __be32 intr[3];
>> +    __be32 *cells;
>> +    int res;
>> +    int addrcells = dt_n_addr_cells(parent);
>> +    int sizecells = dt_n_size_cells(parent);
>> +
>> +    DPRINT("Create hypervisor node\n");
> 
> Not sure there is any point in this print unless you also add DPRINT of
> the things we put into it.
I will add more print. I think it can be usefull for debugging the
device tree.
>>  
>>      /*
>>       * Sanity-check address sizes, since addresses and sizes which do
>> [...]
> 
>> izecells));
>> +    cells = &reg[0];
>> +    dt_set_cell(&cells, addrcells, 0xb0000000);
>> +    dt_set_cell(&cells, sizecells, 0x20000);
> 
> Aside: this really ought to become dynamic, based on finding a hole in
> the physical address map...
Is this address used somewhere in Xen? I didn''t find any place.
> 
> [...]
>> +    res = fdt_property(fdt, "interrupts", intr,
sizeof(intr[0]) * 3);
>> +    if ( res )
>> +        return res;
> 
> the * 3 come from the interrupt-controller nodes properties I think?
> Should we assert somewhere that they match? Perhaps we would already die
> if it weren''t anyway?
The GIC node always has #interrupt-size equals to 3. I don''t think an
assert is necessary.
> 
>> @@ -454,7 +374,8 @@ static int handle_node(struct domain *d, const
struct dt_device_node *np)
>>      if ( dt_match_node(skip_matches, np ) )
>>          return 0;
>>  
>> -    if ( dt_device_used_by(np) != DOMID_XEN )
>> +    if ( dt_device_used_by(np) != DOMID_XEN &&
>> +         !dt_device_type_is_equal(np, "memory") )
> 
> Can we get a comment about why memory is special here please?
I will add it.
> 
>>      {
>>          res = map_device(d, np);
>>  
> 
> Ian.
> 

-- 
Julien Grall

On Thu, 2013-08-22 at 15:02 +0100, Julien Grall wrote:
> On 08/22/2013 02:28 PM, Ian Campbell wrote:
> > On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> >> Avoid to use FDT API which will be removed soon.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> ---
> >>  xen/drivers/video/arm_hdlcd.c |   58
+++++++++++++++++++++++------------------
> >>  1 file changed, 32 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/xen/drivers/video/arm_hdlcd.c
b/xen/drivers/video/arm_hdlcd.c
> >> index 72979ea..0ff8c22 100644
> >> --- a/xen/drivers/video/arm_hdlcd.c
> >> +++ b/xen/drivers/video/arm_hdlcd.c
> >> @@ -25,6 +25,7 @@
> >>  #include <xen/libfdt/libfdt.h>
> >>  #include <xen/init.h>
> >>  #include <xen/mm.h>
> >> +#include <asm/early_printk.h>
> >>  #include "font.h"
> >>  #include "lfb.h"
> >>  #include "modelines.h"
> >> @@ -96,62 +97,67 @@ static int __init get_color_masks(const char*
bpp, struct color_masks **masks)
> >>  
> >>  static void __init set_pixclock(uint32_t pixclock)
> >>  {
> >> -    if ( device_tree_node_compatible(device_tree_flattened, 0,
"arm,vexpress") )
> >> +    if ( dt_find_compatible_node(NULL, NULL,
"arm,vexpress") )
> >>              vexpress_syscfg(1, V2M_SYS_CFG_OSC_FUNC,
> >>                              V2M_SYS_CFG_OSC5, &pixclock);
> >>  }
> >>  
> >>  void __init video_init(void)
> >>  {
> >> -    int node, depth;
> >> -    u32 address_cells, size_cells;
> >>      struct lfb_prop lfbp;
> >>      unsigned char *lfb;
> >> -    paddr_t hdlcd_start, hdlcd_size;
> >> +    u64 hdlcd_start, hdlcd_size;
> > 
> > Why? These are physical addresses.
> 
> dt_get_address take a pointer to u64 value (actually paddr_t == u64).
> Perhaps dt_get_addres should take a paddr_t.
Assuming dt_gt_address always deals in physical addresses then,  yes, I
think so.
> 
> > 
> >>      if ( !hdlcd_start )
> >>      {
> >> -        printk(KERN_ERR "HDLCD address missing from device
tree, disabling driver\n");
> >> +        early_printk("hdlcd: address missing from device
tree, disabling driver\n");
> > 
> > I suppose this (and the other instances of this) is for when the
console
> > is on HDLCD? That''s a separate fix really I think.
> 
> The console is not yet set up when the video driver is initialized.
> 
> I will divide this patch in 2 parts.
Thanks.
> > 
> > Why tr /A-Z/ /a-z/?
> 
> To keep all printks with the same format. I will stick to HDLCD then.
I think they were until you introduced a lower case one ;-)

Ian.

On Thu, 2013-08-22 at 15:10 +0100, Julien Grall wrote:
> >>  
> >>      /*
> >>       * Sanity-check address sizes, since addresses and sizes
which do
> >> [...]
> > 
> >> izecells));
> >> +    cells = &reg[0];
> >> +    dt_set_cell(&cells, addrcells, 0xb0000000);
> >> +    dt_set_cell(&cells, sizecells, 0x20000);
> > 
> > Aside: this really ought to become dynamic, based on finding a hole in
> > the physical address map...
> 
> Is this address used somewhere in Xen? I didn''t find any place.
I don''t think so, it is just used to indicate to the guest where there
is a safe bit of unused address space.

I''m not sure why the guest can''t figure this out for itself,
maybe
Stefano can fill us in.

Ian.

On 08/22/2013 02:50 PM, Ian Campbell wrote:
> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>> When a device has a property status with disabled inside, Linux will
not use
>> the device.
> 
> Is Linux consistent about that?
For general device (ie: not GIC, timer, and malformed driver), yes.

I choose this solution because, I''m not sure how Linux react if, for
example, on a board with 3 UARTs, the second UART is removed.
Set status property to "disabled" works fine for me.
> I must admit I thought we omitted things in use by Xen from the DT
> altogether. Is that no longer true.
> 
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/domain_build.c |    8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index c8f24ed..8377610 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -208,6 +208,14 @@ static int write_properties(struct domain *d,
struct kernel_info *kinfo,
>>              return res;
>>      }
>>  
>> +    /* Disable all devices used by Xen */
>> +    if ( dt_device_used_by(np) == DOMID_XEN )
>> +    {
>> +        res = fdt_property(kinfo->fdt, "status",
"disabled", 8 + 1);
>> +        if ( res )
>> +            return res;
>> +    }
>> +
>>      /*
>>       * XXX should populate /chosen/linux,initrd-{start,end} here if we
>>       * have module[2]
> 
> 

-- 
Julien Grall

On Thu, 2013-08-22 at 15:15 +0100, Julien Grall wrote:
> On 08/22/2013 02:50 PM, Ian Campbell wrote:
> > On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> >> When a device has a property status with disabled inside, Linux
will not use
> >> the device.
> > 
> > Is Linux consistent about that?
> 
> For general device (ie: not GIC, timer, and malformed driver), yes.
Good enough for me.

Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I choose this solution because, I''m not sure how Linux react if,
for
> example, on a board with 3 UARTs, the second UART is removed.
> Set status property to "disabled" works fine for me.
> 
> > I must admit I thought we omitted things in use by Xen from the DT
> > altogether. Is that no longer true.
> 
> > 
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> ---
> >>  xen/arch/arm/domain_build.c |    8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/domain_build.c
b/xen/arch/arm/domain_build.c
> >> index c8f24ed..8377610 100644
> >> --- a/xen/arch/arm/domain_build.c
> >> +++ b/xen/arch/arm/domain_build.c
> >> @@ -208,6 +208,14 @@ static int write_properties(struct domain *d,
struct kernel_info *kinfo,
> >>              return res;
> >>      }
> >>  
> >> +    /* Disable all devices used by Xen */
> >> +    if ( dt_device_used_by(np) == DOMID_XEN )
> >> +    {
> >> +        res = fdt_property(kinfo->fdt, "status",
"disabled", 8 + 1);
> >> +        if ( res )
> >> +            return res;
> >> +    }
> >> +
> >>      /*
> >>       * XXX should populate /chosen/linux,initrd-{start,end} here
if we
> >>       * have module[2]
> > 
> > 
> 
>

On 08/22/2013 03:00 PM, Ian Campbell wrote:
> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>> On Versatile there is a bunch of devices that must not pass-through to
any
> 
> "there are a bunch of devices which must not be passed-through"
> 
>> guest (power management and cache coherency devices).
>>
>> This commit also blacklist the HDLCD device because then is unable to
correctly
>                                                           ^Linux?
> 
>> map the framebuffer. Therefore, when Linux will try to access to the
framebuffer,
>> Xen will receive a non-handled data access.
> 
> Can/should this be conditional on whether Xen has console=hdlcd or not?
> Or does Xen use the device unconditionally if it exists? TBH I think it
> would be normal to prefer that Linux gets this device...
> 
> (I''m unclear how this relates to memreserve as mention in the code
> comment)
This issue is only when the HDLCD is used by Linux (not Xen). To specify
where the framebuffer lives in the memory, there is a property
"framebuffer" which contains address/size.
This regions must be reserved to avoid Linux/u-boot play with it. So the
DTS has a memreserve range with the same value. I don''t really
understand all memreserve things but Xen must cope with it.

If this device is mapped, Xen will receive a data abort because Linux
can''t access to the framebuffer.

This solution is not upstream (only in the Linaro tree). I don''t know
if
this driver works with the vanilla kernel.
> 
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/platforms/vexpress.c |   17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/xen/arch/arm/platforms/vexpress.c
b/xen/arch/arm/platforms/vexpress.c
>> index 8fc30c4..ece7bd9 100644
>> --- a/xen/arch/arm/platforms/vexpress.c
>> +++ b/xen/arch/arm/platforms/vexpress.c
>> @@ -125,9 +125,26 @@ static const char const *vexpress_dt_compat[]
__initdata >>      NULL
>>  };
>>  
>> +static const struct dt_device_match vexpress_blacklist_dev[]
__initconst >> +{
>> +    /* Cache Coherent Interconnect */
>> +    DT_MATCH_COMPATIBLE("arm,cci-400"),
>> +    DT_MATCH_COMPATIBLE("arm,cci-400-pmu"),
>> +    /* Video device
>> +     * TODO: remove it once memreserve is handled properly by Xen
>> +     */
>> +    DT_MATCH_COMPATIBLE("arm,hdlcd"),
>> +    /* Hardware power management */
>> +    DT_MATCH_COMPATIBLE("arm,vexpress-reset"),
>> +    DT_MATCH_COMPATIBLE("arm,vexpress-reboot"),
>> +    DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"),
>> +    { /* sentinel */ },
>> +};
>> +
>>  PLATFORM_START(vexpress, "VERSATILE EXPRESS")
>>      .compatible = vexpress_dt_compat,
>>      .reset = vexpress_reset,
>> +    .blacklist_dev = vexpress_blacklist_dev,
>>  PLATFORM_END
>>  
>>  /*
> 
> 

-- 
Julien Grall

On Thu, 2013-08-22 at 15:24 +0100, Julien Grall wrote:
> On 08/22/2013 03:00 PM, Ian Campbell wrote:
> > On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> >> On Versatile there is a bunch of devices that must not
pass-through to any
> > 
> > "there are a bunch of devices which must not be
passed-through"
> > 
> >> guest (power management and cache coherency devices).
> >>
> >> This commit also blacklist the HDLCD device because then is unable
to correctly
> >                                                           ^Linux?
> > 
> >> map the framebuffer. Therefore, when Linux will try to access to
the framebuffer,
> >> Xen will receive a non-handled data access.
> > 
> > Can/should this be conditional on whether Xen has console=hdlcd or
not?
> > Or does Xen use the device unconditionally if it exists? TBH I think
it
> > would be normal to prefer that Linux gets this device...
> > 
> > (I''m unclear how this relates to memreserve as mention in the
code
> > comment)
> 
> This issue is only when the HDLCD is used by Linux (not Xen). To specify
> where the framebuffer lives in the memory, there is a property
> "framebuffer" which contains address/size.
> This regions must be reserved to avoid Linux/u-boot play with it. So the
> DTS has a memreserve range with the same value. I don''t really
> understand all memreserve things but Xen must cope with it.
Yes, Xen needs to learn memreserve, it''s used on midway too for
example.
> If this device is mapped, Xen will receive a data abort because Linux
> can''t access to the framebuffer.
Because it wasn''t part of the set of memory which we assigned to the
guest?

It seems like it would be hard to link an individual memsreserve to a
particular device, at least not without device specific logic (e.g
looking at the "framebuffer" property in this case).

Sounds like we might need list of compatible -> dom0_init_hook
functions, with the appropriate hook called for each device which we
pass through.

Ian.
> 
> This solution is not upstream (only in the Linaro tree). I don''t
know if
> this driver works with the vanilla kernel.
> 
> > 
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> ---
> >>  xen/arch/arm/platforms/vexpress.c |   17 +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/platforms/vexpress.c
b/xen/arch/arm/platforms/vexpress.c
> >> index 8fc30c4..ece7bd9 100644
> >> --- a/xen/arch/arm/platforms/vexpress.c
> >> +++ b/xen/arch/arm/platforms/vexpress.c
> >> @@ -125,9 +125,26 @@ static const char const *vexpress_dt_compat[]
__initdata > >>      NULL
> >>  };
> >>  
> >> +static const struct dt_device_match vexpress_blacklist_dev[]
__initconst > >> +{
> >> +    /* Cache Coherent Interconnect */
> >> +    DT_MATCH_COMPATIBLE("arm,cci-400"),
> >> +    DT_MATCH_COMPATIBLE("arm,cci-400-pmu"),
> >> +    /* Video device
> >> +     * TODO: remove it once memreserve is handled properly by Xen
> >> +     */
> >> +    DT_MATCH_COMPATIBLE("arm,hdlcd"),
> >> +    /* Hardware power management */
> >> +    DT_MATCH_COMPATIBLE("arm,vexpress-reset"),
> >> +    DT_MATCH_COMPATIBLE("arm,vexpress-reboot"),
> >> +    DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"),
> >> +    { /* sentinel */ },
> >> +};
> >> +
> >>  PLATFORM_START(vexpress, "VERSATILE EXPRESS")
> >>      .compatible = vexpress_dt_compat,
> >>      .reset = vexpress_reset,
> >> +    .blacklist_dev = vexpress_blacklist_dev,
> >>  PLATFORM_END
> >>  
> >>  /*
> > 
> > 
> 
>

On 08/22/2013 03:36 PM, Ian Campbell wrote:
> On Thu, 2013-08-22 at 15:24 +0100, Julien Grall wrote:
>> On 08/22/2013 03:00 PM, Ian Campbell wrote:
>>> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>>>> On Versatile there is a bunch of devices that must not
pass-through to any
>>>
>>> "there are a bunch of devices which must not be
passed-through"
>>>
>>>> guest (power management and cache coherency devices).
>>>>
>>>> This commit also blacklist the HDLCD device because then is
unable to correctly
>>>                                                           ^Linux?
>>>
>>>> map the framebuffer. Therefore, when Linux will try to access
to the framebuffer,
>>>> Xen will receive a non-handled data access.
>>>
>>> Can/should this be conditional on whether Xen has console=hdlcd or
not?
>>> Or does Xen use the device unconditionally if it exists? TBH I
think it
>>> would be normal to prefer that Linux gets this device...
>>>
>>> (I''m unclear how this relates to memreserve as mention in
the code
>>> comment)
>>
>> This issue is only when the HDLCD is used by Linux (not Xen). To
specify
>> where the framebuffer lives in the memory, there is a property
>> "framebuffer" which contains address/size.
>> This regions must be reserved to avoid Linux/u-boot play with it. So
the
>> DTS has a memreserve range with the same value. I don''t really
>> understand all memreserve things but Xen must cope with it.
> 
> Yes, Xen needs to learn memreserve, it''s used on midway too for
example.
As I understand memreserve can only contains RAM region. Right?
>> If this device is mapped, Xen will receive a data abort because Linux
>> can''t access to the framebuffer.
> 
> Because it wasn''t part of the set of memory which we assigned to
the
> guest?
Yes. The framebuffer steals RAM, in this case, the end of it.
> It seems like it would be hard to link an individual memsreserve to a
> particular device, at least not without device specific logic (e.g
> looking at the "framebuffer" property in this case).
> 
> Sounds like we might need list of compatible -> dom0_init_hook
> functions, with the appropriate hook called for each device which we
> pass through.
It could be a solution. In this case, we just need to update the
framebuffer property and the memreserve.
> 
> Ian.
> 
>>
>> This solution is not upstream (only in the Linaro tree). I
don''t know if
>> this driver works with the vanilla kernel.
>>
>>>
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>> ---
>>>>  xen/arch/arm/platforms/vexpress.c |   17 +++++++++++++++++
>>>>  1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/platforms/vexpress.c
b/xen/arch/arm/platforms/vexpress.c
>>>> index 8fc30c4..ece7bd9 100644
>>>> --- a/xen/arch/arm/platforms/vexpress.c
>>>> +++ b/xen/arch/arm/platforms/vexpress.c
>>>> @@ -125,9 +125,26 @@ static const char const
*vexpress_dt_compat[] __initdata >>>>      NULL
>>>>  };
>>>>  
>>>> +static const struct dt_device_match vexpress_blacklist_dev[]
__initconst >>>> +{
>>>> +    /* Cache Coherent Interconnect */
>>>> +    DT_MATCH_COMPATIBLE("arm,cci-400"),
>>>> +    DT_MATCH_COMPATIBLE("arm,cci-400-pmu"),
>>>> +    /* Video device
>>>> +     * TODO: remove it once memreserve is handled properly by
Xen
>>>> +     */
>>>> +    DT_MATCH_COMPATIBLE("arm,hdlcd"),
>>>> +    /* Hardware power management */
>>>> +    DT_MATCH_COMPATIBLE("arm,vexpress-reset"),
>>>> +    DT_MATCH_COMPATIBLE("arm,vexpress-reboot"),
>>>> +    DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"),
>>>> +    { /* sentinel */ },
>>>> +};
>>>> +
>>>>  PLATFORM_START(vexpress, "VERSATILE EXPRESS")
>>>>      .compatible = vexpress_dt_compat,
>>>>      .reset = vexpress_reset,
>>>> +    .blacklist_dev = vexpress_blacklist_dev,
>>>>  PLATFORM_END
>>>>  
>>>>  /*
>>>
>>>
>>
>>
> 
> 

-- 
Julien Grall

On Thu, 2013-08-22 at 15:51 +0100, Julien Grall wrote:
> On 08/22/2013 03:36 PM, Ian Campbell wrote:
> > On Thu, 2013-08-22 at 15:24 +0100, Julien Grall wrote:
> >> On 08/22/2013 03:00 PM, Ian Campbell wrote:
> >>> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
> >>>> On Versatile there is a bunch of devices that must not
pass-through to any
> >>>
> >>> "there are a bunch of devices which must not be
passed-through"
> >>>
> >>>> guest (power management and cache coherency devices).
> >>>>
> >>>> This commit also blacklist the HDLCD device because then
is unable to correctly
> >>>                                                          
^Linux?
> >>>
> >>>> map the framebuffer. Therefore, when Linux will try to
access to the framebuffer,
> >>>> Xen will receive a non-handled data access.
> >>>
> >>> Can/should this be conditional on whether Xen has
console=hdlcd or not?
> >>> Or does Xen use the device unconditionally if it exists? TBH I
think it
> >>> would be normal to prefer that Linux gets this device...
> >>>
> >>> (I''m unclear how this relates to memreserve as
mention in the code
> >>> comment)
> >>
> >> This issue is only when the HDLCD is used by Linux (not Xen). To
specify
> >> where the framebuffer lives in the memory, there is a property
> >> "framebuffer" which contains address/size.
> >> This regions must be reserved to avoid Linux/u-boot play with it.
So the
> >> DTS has a memreserve range with the same value. I don''t
really
> >> understand all memreserve things but Xen must cope with it.
> > 
> > Yes, Xen needs to learn memreserve, it''s used on midway too
for example.
> 
> As I understand memreserve can only contains RAM region. Right?
ePAPR seems to suggest it is only for RAM, yes.
> > It seems like it would be hard to link an individual memsreserve to a
> > particular device, at least not without device specific logic (e.g
> > looking at the "framebuffer" property in this case).
> > 
> > Sounds like we might need list of compatible -> dom0_init_hook
> > functions, with the appropriate hook called for each device which we
> > pass through.
> 
> It could be a solution. In this case, we just need to update the
> framebuffer property and the memreserve.
I think in this case we probably want to create a 1:1 mapping of the
video ram? In case e.g. the firmware has setup attributes, or the LCD
cannot use a different address etc...
> 
> > 
> > Ian.
> > 
> >>
> >> This solution is not upstream (only in the Linaro tree). I
don''t know if
> >> this driver works with the vanilla kernel.
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Julien Grall
<julien.grall@linaro.org>
> >>>> ---
> >>>>  xen/arch/arm/platforms/vexpress.c |   17
+++++++++++++++++
> >>>>  1 file changed, 17 insertions(+)
> >>>>
> >>>> diff --git a/xen/arch/arm/platforms/vexpress.c
b/xen/arch/arm/platforms/vexpress.c
> >>>> index 8fc30c4..ece7bd9 100644
> >>>> --- a/xen/arch/arm/platforms/vexpress.c
> >>>> +++ b/xen/arch/arm/platforms/vexpress.c
> >>>> @@ -125,9 +125,26 @@ static const char const
*vexpress_dt_compat[] __initdata > >>>>      NULL
> >>>>  };
> >>>>  
> >>>> +static const struct dt_device_match
vexpress_blacklist_dev[] __initconst > >>>> +{
> >>>> +    /* Cache Coherent Interconnect */
> >>>> +    DT_MATCH_COMPATIBLE("arm,cci-400"),
> >>>> +    DT_MATCH_COMPATIBLE("arm,cci-400-pmu"),
> >>>> +    /* Video device
> >>>> +     * TODO: remove it once memreserve is handled
properly by Xen
> >>>> +     */
> >>>> +    DT_MATCH_COMPATIBLE("arm,hdlcd"),
> >>>> +    /* Hardware power management */
> >>>> +    DT_MATCH_COMPATIBLE("arm,vexpress-reset"),
> >>>> +    DT_MATCH_COMPATIBLE("arm,vexpress-reboot"),
> >>>> +   
DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"),
> >>>> +    { /* sentinel */ },
> >>>> +};
> >>>> +
> >>>>  PLATFORM_START(vexpress, "VERSATILE EXPRESS")
> >>>>      .compatible = vexpress_dt_compat,
> >>>>      .reset = vexpress_reset,
> >>>> +    .blacklist_dev = vexpress_blacklist_dev,
> >>>>  PLATFORM_END
> >>>>  
> >>>>  /*
> >>>
> >>>
> >>
> >>
> > 
> > 
> 
>

On 08/22/2013 04:02 PM, Ian Campbell wrote:
> On Thu, 2013-08-22 at 15:51 +0100, Julien Grall wrote:
>> On 08/22/2013 03:36 PM, Ian Campbell wrote:
>>> On Thu, 2013-08-22 at 15:24 +0100, Julien Grall wrote:
>>>> On 08/22/2013 03:00 PM, Ian Campbell wrote:
>>>>> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote:
>>>>>> On Versatile there is a bunch of devices that must not
pass-through to any
>>>>>
>>>>> "there are a bunch of devices which must not be
passed-through"
>>>>>
>>>>>> guest (power management and cache coherency devices).
>>>>>>
>>>>>> This commit also blacklist the HDLCD device because
then is unable to correctly
>>>>>                                                          
^Linux?
>>>>>
>>>>>> map the framebuffer. Therefore, when Linux will try to
access to the framebuffer,
>>>>>> Xen will receive a non-handled data access.
>>>>>
>>>>> Can/should this be conditional on whether Xen has
console=hdlcd or not?
>>>>> Or does Xen use the device unconditionally if it exists?
TBH I think it
>>>>> would be normal to prefer that Linux gets this device...
>>>>>
>>>>> (I''m unclear how this relates to memreserve as
mention in the code
>>>>> comment)
>>>>
>>>> This issue is only when the HDLCD is used by Linux (not Xen).
To specify
>>>> where the framebuffer lives in the memory, there is a property
>>>> "framebuffer" which contains address/size.
>>>> This regions must be reserved to avoid Linux/u-boot play with
it. So the
>>>> DTS has a memreserve range with the same value. I
don''t really
>>>> understand all memreserve things but Xen must cope with it.
>>>
>>> Yes, Xen needs to learn memreserve, it''s used on midway
too for example.
>>
>> As I understand memreserve can only contains RAM region. Right?
> 
> ePAPR seems to suggest it is only for RAM, yes.
> 
>>> It seems like it would be hard to link an individual memsreserve to
a
>>> particular device, at least not without device specific logic (e.g
>>> looking at the "framebuffer" property in this case).
>>>
>>> Sounds like we might need list of compatible -> dom0_init_hook
>>> functions, with the appropriate hook called for each device which
we
>>> pass through.
>>
>> It could be a solution. In this case, we just need to update the
>> framebuffer property and the memreserve.
> 
> I think in this case we probably want to create a 1:1 mapping of the
> video ram? In case e.g. the firmware has setup attributes, or the LCD
> cannot use a different address etc...
Do we need to take into account in the dom0 memory?

For example, the amount of video RAM memory is 6Mb and the DOM0 memory
is 256Mb.

Does Xen must allocate?
	- 6Mb of 1:1 mapping
        - 250Mb of domain allocate page
>>
>>>
>>> Ian.
>>>
>>>>
>>>> This solution is not upstream (only in the Linaro tree). I
don''t know if
>>>> this driver works with the vanilla kernel.
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Julien Grall
<julien.grall@linaro.org>
>>>>>> ---
>>>>>>  xen/arch/arm/platforms/vexpress.c |   17
+++++++++++++++++
>>>>>>  1 file changed, 17 insertions(+)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/platforms/vexpress.c
b/xen/arch/arm/platforms/vexpress.c
>>>>>> index 8fc30c4..ece7bd9 100644
>>>>>> --- a/xen/arch/arm/platforms/vexpress.c
>>>>>> +++ b/xen/arch/arm/platforms/vexpress.c
>>>>>> @@ -125,9 +125,26 @@ static const char const
*vexpress_dt_compat[] __initdata >>>>>>      NULL
>>>>>>  };
>>>>>>  
>>>>>> +static const struct dt_device_match
vexpress_blacklist_dev[] __initconst >>>>>> +{
>>>>>> +    /* Cache Coherent Interconnect */
>>>>>> +    DT_MATCH_COMPATIBLE("arm,cci-400"),
>>>>>> +    DT_MATCH_COMPATIBLE("arm,cci-400-pmu"),
>>>>>> +    /* Video device
>>>>>> +     * TODO: remove it once memreserve is handled
properly by Xen
>>>>>> +     */
>>>>>> +    DT_MATCH_COMPATIBLE("arm,hdlcd"),
>>>>>> +    /* Hardware power management */
>>>>>> +   
DT_MATCH_COMPATIBLE("arm,vexpress-reset"),
>>>>>> +   
DT_MATCH_COMPATIBLE("arm,vexpress-reboot"),
>>>>>> +   
DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"),
>>>>>> +    { /* sentinel */ },
>>>>>> +};
>>>>>> +
>>>>>>  PLATFORM_START(vexpress, "VERSATILE
EXPRESS")
>>>>>>      .compatible = vexpress_dt_compat,
>>>>>>      .reset = vexpress_reset,
>>>>>> +    .blacklist_dev = vexpress_blacklist_dev,
>>>>>>  PLATFORM_END
>>>>>>  
>>>>>>  /*
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 
> 

-- 
Julien Grall

On Thu, 2013-08-22 at 16:28 +0100, Julien Grall wrote:
> > I think in this case we probably want to create a 1:1 mapping of the
> > video ram? In case e.g. the firmware has setup attributes, or the LCD
> > cannot use a different address etc...
> 
> Do we need to take into account in the dom0 memory?
> 
> For example, the amount of video RAM memory is 6Mb and the DOM0 memory
> is 256Mb.
> 
> Does Xen must allocate?
> 	- 6Mb of 1:1 mapping
>         - 250Mb of domain allocate page
Could do it either way. I''d be inclined to count the framebuffer as
being in addition to dom0''s actual allocation.

Ian.