[PATCH 0/8] arm: pass a device tree to dom0

[PATCH 0/8] arm: pass a device tree to dom0

[David Vrabel]

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

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

Note that loading Linux using a vmlinux file is no longer supported
and the kernel must support device tree (ATAGS are no longer provided
by Xen).

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

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

David

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

[PATCH 1/8] arm: add generated files to .gitignore and .hgignore

[David Vrabel]

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

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 .gitignore |    2 ++
 .hgignore  |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index 8810b35..ba4b241 100644
--- a/.gitignore
+++ b/.gitignore
@@ -284,6 +284,8 @@ tools/ocaml-xenstored*
 xen/.banner*
 xen/BLOG
 xen/System.map
+xen/arch/arm/asm-offsets.s
+xen/arch/arm/xen.lds
 xen/arch/x86/asm-offsets.s
 xen/arch/x86/boot/mkelf32
 xen/arch/x86/xen.lds
diff --git a/.hgignore b/.hgignore
index 46655ad..dffce07 100644
--- a/.hgignore
+++ b/.hgignore
@@ -312,6 +312,8 @@
 ^xen/\.banner.*$
 ^xen/BLOG$
 ^xen/System.map$
+^xen/arch/arm/asm-offsets\.s$
+^xen/arch/arm/xen\.lds$
 ^xen/arch/x86/asm-offsets\.s$
 ^xen/arch/x86/boot/mkelf32$
 ^xen/arch/x86/xen\.lds$
-- 
1.7.2.5

Re: [PATCH 1/8] arm: add generated files to .gitignore and .hgignore

[Ian Campbell]

On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

I'm going to assume my arch/arm committership extends to arm specific
entries in these files and apply. Thanks.

Ian.

[PATCH 2/8] device tree: correctly ignore unit-address when matching nodes by name

[David Vrabel]

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

When matching node by their name, correctly ignore the unit address
(@...) part of the name.  Previously, a "memory-controller" node would
be incorrectly matched as a "memory" node.

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

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index d50cb9c..e5df748 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -24,6 +24,20 @@
 struct dt_early_info __initdata early_info;
 void *device_tree_flattened;
 
+static bool_t __init node_matches(const void *fdt, int node, const char *match)
+{
+    const char *name;
+    size_t len;
+
+    name = fdt_get_name(fdt, node, NULL);
+    len = strlen(match);
+
+    /* Match both "match" and "match@..." patterns but not
+       "match-foo". */
+    return strncmp(name, match, len) == 0
+        && (name[len] == '@' || name[len] == '\0');
+}
+
 static void __init get_val(const u32 **cell, u32 cells, u64 *val)
 {
     *val = 0;
@@ -93,13 +107,10 @@ static void __init early_scan(const void *fdt)
 {
     int node;
     int depth;
-    const char *name;
     u32 address_cells[MAX_DEPTH];
     u32 size_cells[MAX_DEPTH];
 
     for (node = 0; depth >= 0; node = fdt_next_node(fdt, node, &depth)) {
-        name = fdt_get_name(fdt, node, NULL);
-
         if (depth >= MAX_DEPTH) {
             early_printk("fdt: node '%s': nested too deep\n",
                          fdt_get_name(fdt, node, NULL));
@@ -109,7 +120,7 @@ static void __init early_scan(const void *fdt)
         address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
         size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
 
-        if (strncmp(name, "memory", 6) == 0)
+        if (node_matches(fdt, node, "memory"))
             process_memory_node(fdt, node, address_cells[depth-1], size_cells[depth-1]);
     }
 }
-- 
1.7.2.5

Re: [PATCH 2/8] device tree: correctly ignore unit-address when matching nodes by name

[Ian Campbell]

On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> When matching node by their name, correctly ignore the unit address
> (@...) part of the name.  Previously, a "memory-controller" node would
> be incorrectly matched as a "memory" node.

Are we reinventing this ourselves or is this derived from the Linux (or
other) version of similar code?

> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  xen/common/device_tree.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index d50cb9c..e5df748 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -24,6 +24,20 @@
>  struct dt_early_info __initdata early_info;
>  void *device_tree_flattened;
>  
> +static bool_t __init node_matches(const void *fdt, int node, const char *match)
> +{
> +    const char *name;
> +    size_t len;
> +
> +    name = fdt_get_name(fdt, node, NULL);
> +    len = strlen(match);
> +
> +    /* Match both "match" and "match@..." patterns but not
> +       "match-foo". */
> +    return strncmp(name, match, len) == 0
> +        && (name[len] == '@' || name[len] == '\0');

How can name[len] == '@' when len came from strlen? Wouldn't you need
some sort of strchr construct?

Oh, I see, it's strlen(match) not strlen(name) and the short-curcuiting
behaviour of && protects you from the case where strlen(match) >
strlen(name) because the strncmp will fail so name[len] won't be
evaluated.

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

> +}
> +
>  static void __init get_val(const u32 **cell, u32 cells, u64 *val)
>  {
>      *val = 0;
> @@ -93,13 +107,10 @@ static void __init early_scan(const void *fdt)
>  {
>      int node;
>      int depth;
> -    const char *name;
>      u32 address_cells[MAX_DEPTH];
>      u32 size_cells[MAX_DEPTH];
>  
>      for (node = 0; depth >= 0; node = fdt_next_node(fdt, node, &depth)) {
> -        name = fdt_get_name(fdt, node, NULL);
> -
>          if (depth >= MAX_DEPTH) {
>              early_printk("fdt: node '%s': nested too deep\n",
>                           fdt_get_name(fdt, node, NULL));
> @@ -109,7 +120,7 @@ static void __init early_scan(const void *fdt)
>          address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
>          size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
>  
> -        if (strncmp(name, "memory", 6) == 0)
> +        if (node_matches(fdt, node, "memory"))
>              process_memory_node(fdt, node, address_cells[depth-1], size_cells[depth-1]);
>      }
>  }

Re: [PATCH 2/8] device tree: correctly ignore unit-address when matching nodes by name

[David Vrabel]

On 14/03/12 10:00, Ian Campbell wrote:
> On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> When matching node by their name, correctly ignore the unit address
>> (@...) part of the name.  Previously, a "memory-controller" node would
>> be incorrectly matched as a "memory" node.
> 
> Are we reinventing this ourselves or is this derived from the Linux (or
> other) version of similar code?

Linux doesn't have an equivalent node_matches() function.  Linux could
do with one as it open-codes a bunch of these comparisons.

>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index d50cb9c..e5df748 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -24,6 +24,20 @@
>>  struct dt_early_info __initdata early_info;
>>  void *device_tree_flattened;
>>  
>> +static bool_t __init node_matches(const void *fdt, int node, const char *match)
>> +{
>> +    const char *name;
>> +    size_t len;
>> +
>> +    name = fdt_get_name(fdt, node, NULL);
>> +    len = strlen(match);
>> +
>> +    /* Match both "match" and "match@..." patterns but not
>> +       "match-foo". */
>> +    return strncmp(name, match, len) == 0
>> +        && (name[len] == '@' || name[len] == '\0');
> 
> How can name[len] == '@' when len came from strlen? Wouldn't you need
> some sort of strchr construct?

I can rename len to match_len if you think it makes it clearer.

David

[PATCH 3/8] device tree: add device_tree_for_each_node()

[David Vrabel]

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

Add device_tree_for_each_node() to iterate over all nodes in a flat
device tree.  Use this in device_tree_early_init().

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/common/device_tree.c      |   71 ++++++++++++++++++++++++++---------------
 xen/include/xen/device_tree.h |    8 +++++
 2 files changed, 53 insertions(+), 26 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index e5df748..e95ff3c 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -66,7 +66,42 @@ static u32 __init prop_by_name_u32(const void *fdt, int node, const char *prop_n
     return fdt32_to_cpu(*(uint32_t*)prop->data);
 }
 
-static void __init process_memory_node(const void *fdt, int node,
+#define MAX_DEPTH 16
+
+/**
+ * device_tree_for_each_node - iterate over all device tree nodes
+ * @fdt: flat device tree.
+ * @func: function to call for each node.
+ * @data: data to pass to @func.
+ */
+int device_tree_for_each_node(const void *fdt,
+                              device_tree_node_func func, void *data)
+{
+    int node;
+    int depth;
+    u32 address_cells[MAX_DEPTH];
+    u32 size_cells[MAX_DEPTH];
+    int ret;
+
+    for (node = 0, depth = 0;
+         node >=0 && depth >= 0;
+         node = fdt_next_node(fdt, node, &depth))
+    {
+        if (depth >= MAX_DEPTH)
+            continue;
+
+        address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
+        size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
+
+        ret = func(fdt, node, fdt_get_name(fdt, node, NULL), depth,
+                   address_cells[depth-1], size_cells[depth-1], data);
+        if (ret < 0)
+            return ret;
+    }
+    return 0;
+}
+
+static void __init process_memory_node(const void *fdt, int node, const char *name,
                                        u32 address_cells, u32 size_cells)
 {
     const struct fdt_property *prop;
@@ -77,15 +112,13 @@ static void __init process_memory_node(const void *fdt, int node,
     paddr_t start, size;
 
     if (address_cells < 1 || size_cells < 1) {
-        early_printk("fdt: node `%s': invalid #address-cells or #size-cells",
-                     fdt_get_name(fdt, node, NULL));
+        early_printk("fdt: node `%s': invalid #address-cells or #size-cells", name);
         return;
     }
 
     prop = fdt_get_property(fdt, node, "reg", NULL);
     if (!prop) {
-        early_printk("fdt: node `%s': missing `reg' property\n",
-                     fdt_get_name(fdt, node, NULL));
+        early_printk("fdt: node `%s': missing `reg' property\n", name);
         return;
     }
 
@@ -101,28 +134,14 @@ static void __init process_memory_node(const void *fdt, int node,
     }
 }
 
-#define MAX_DEPTH 16
-
-static void __init early_scan(const void *fdt)
+static int __init early_scan_node(const void *fdt,
+                                  int node, const char *name, int depth,
+                                  u32 address_cells, u32 size_cells, void *data)
 {
-    int node;
-    int depth;
-    u32 address_cells[MAX_DEPTH];
-    u32 size_cells[MAX_DEPTH];
-
-    for (node = 0; depth >= 0; node = fdt_next_node(fdt, node, &depth)) {
-        if (depth >= MAX_DEPTH) {
-            early_printk("fdt: node '%s': nested too deep\n",
-                         fdt_get_name(fdt, node, NULL));
-            continue;
-        }
+    if (node_matches(fdt, node, "memory"))
+        process_memory_node(fdt, node, name, address_cells, size_cells);
 
-        address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
-        size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
-
-        if (node_matches(fdt, node, "memory"))
-            process_memory_node(fdt, node, address_cells[depth-1], size_cells[depth-1]);
-    }
+    return 0;
 }
 
 static void __init early_print_info(void)
@@ -149,7 +168,7 @@ size_t __init device_tree_early_init(const void *fdt)
     if (ret < 0)
         early_panic("No valid device tree\n");
 
-    early_scan(fdt);
+    device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
     early_print_info();
 
     return fdt_totalsize(fdt);
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 28a3dee..505f3e3 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -28,10 +28,18 @@ struct dt_early_info {
     struct dt_mem_info mem;
 };
 
+typedef int (*device_tree_node_func)(const void *fdt,
+                                     int node, const char *name, int depth,
+                                     u32 address_cells, u32 size_cells,
+                                     void *data);
+
 extern struct dt_early_info early_info;
 extern void *device_tree_flattened;
 
 size_t device_tree_early_init(const void *fdt);
 paddr_t device_tree_get_xen_paddr(void);
 
+int device_tree_for_each_node(const void *fdt,
+                              device_tree_node_func func, void *data);
+
 #endif
-- 
1.7.2.5

Re: [PATCH 3/8] device tree: add device_tree_for_each_node()

[Ian Campbell]

On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Add device_tree_for_each_node() to iterate over all nodes in a flat
> device tree.  Use this in device_tree_early_init().
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  xen/common/device_tree.c      |   71 ++++++++++++++++++++++++++---------------
>  xen/include/xen/device_tree.h |    8 +++++
>  2 files changed, 53 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index e5df748..e95ff3c 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -66,7 +66,42 @@ static u32 __init prop_by_name_u32(const void *fdt, int node, const char *prop_n
>      return fdt32_to_cpu(*(uint32_t*)prop->data);
>  }
>  
> -static void __init process_memory_node(const void *fdt, int node,
> +#define MAX_DEPTH 16
> +
> +/**
> + * device_tree_for_each_node - iterate over all device tree nodes
> + * @fdt: flat device tree.
> + * @func: function to call for each node.
> + * @data: data to pass to @func.
> + */
> +int device_tree_for_each_node(const void *fdt,
> +                              device_tree_node_func func, void *data)
> +{
> +    int node;
> +    int depth;
> +    u32 address_cells[MAX_DEPTH];
> +    u32 size_cells[MAX_DEPTH];
> +    int ret;
> +
> +    for (node = 0, depth = 0;
> +         node >=0 && depth >= 0;
> +         node = fdt_next_node(fdt, node, &depth))

Xen coding style has spaces both sides of the outermost "(" and ")" of a
for loop (similarly for if / while etc)

> +    {
> +        if (depth >= MAX_DEPTH)

Some sort of warning or error message would be useful here?

> +            continue;
> +
> +        address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
> +        size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
> +
> +        ret = func(fdt, node, fdt_get_name(fdt, node, NULL), depth,
> +                   address_cells[depth-1], size_cells[depth-1], data);

I suppose this function could have been written recursively and avoided
the arbitrary MAX_DEPTH, but actually in the hypervisor an iterative
version with explicit maximum stack usage seems like a reasonable idea.

> +        if (ret < 0)
> +            return ret;
> +    }
> +    return 0;
> +}
> +
> +static void __init process_memory_node(const void *fdt, int node, const char *name,
>                                         u32 address_cells, u32 size_cells)
>  {
>      const struct fdt_property *prop;
> @@ -77,15 +112,13 @@ static void __init process_memory_node(const void *fdt, int node,
>      paddr_t start, size;
>  
>      if (address_cells < 1 || size_cells < 1) {

I should have spotted this before, coding style needs a space inside the
() and { should be on the next line. There's a bunch of this in both the
context and the new code added by this patch. Obviously the new code
should be fixed, I don't mind if you deal with the existing bits by a
cleanup sweep or by picking it up as you go along.

> -        early_printk("fdt: node `%s': invalid #address-cells or #size-cells",
> -                     fdt_get_name(fdt, node, NULL));
> +        early_printk("fdt: node `%s': invalid #address-cells or #size-cells", name);
>          return;
>      }
>  
>      prop = fdt_get_property(fdt, node, "reg", NULL);
>      if (!prop) {
> -        early_printk("fdt: node `%s': missing `reg' property\n",
> -                     fdt_get_name(fdt, node, NULL));
> +        early_printk("fdt: node `%s': missing `reg' property\n", name);
>          return;
>      }
>  
> @@ -101,28 +134,14 @@ static void __init process_memory_node(const void *fdt, int node,
>      }
>  }
>  
> -#define MAX_DEPTH 16
> -
> -static void __init early_scan(const void *fdt)
> +static int __init early_scan_node(const void *fdt,
> +                                  int node, const char *name, int depth,
> +                                  u32 address_cells, u32 size_cells, void *data)
>  {
> -    int node;
> -    int depth;
> -    u32 address_cells[MAX_DEPTH];
> -    u32 size_cells[MAX_DEPTH];
> -
> -    for (node = 0; depth >= 0; node = fdt_next_node(fdt, node, &depth)) {
> -        if (depth >= MAX_DEPTH) {
> -            early_printk("fdt: node '%s': nested too deep\n",
> -                         fdt_get_name(fdt, node, NULL));
> -            continue;
> -        }
> +    if (node_matches(fdt, node, "memory"))
> +        process_memory_node(fdt, node, name, address_cells, size_cells);
>  
> -        address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
> -        size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
> -
> -        if (node_matches(fdt, node, "memory"))
> -            process_memory_node(fdt, node, address_cells[depth-1], size_cells[depth-1]);
> -    }
> +    return 0;
>  }
>  
>  static void __init early_print_info(void)
> @@ -149,7 +168,7 @@ size_t __init device_tree_early_init(const void *fdt)
>      if (ret < 0)
>          early_panic("No valid device tree\n");
>  
> -    early_scan(fdt);
> +    device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
>      early_print_info();
>  
>      return fdt_totalsize(fdt);
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 28a3dee..505f3e3 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -28,10 +28,18 @@ struct dt_early_info {
>      struct dt_mem_info mem;
>  };
>  
> +typedef int (*device_tree_node_func)(const void *fdt,
> +                                     int node, const char *name, int depth,
> +                                     u32 address_cells, u32 size_cells,
> +                                     void *data);
> +
>  extern struct dt_early_info early_info;
>  extern void *device_tree_flattened;
>  
>  size_t device_tree_early_init(const void *fdt);
>  paddr_t device_tree_get_xen_paddr(void);
>  
> +int device_tree_for_each_node(const void *fdt,
> +                              device_tree_node_func func, void *data);
> +
>  #endif

Re: [PATCH 3/8] device tree: add device_tree_for_each_node()

[David Vrabel]

On 14/03/12 10:08, Ian Campbell wrote:
> On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Add device_tree_for_each_node() to iterate over all nodes in a flat
>> device tree.  Use this in device_tree_early_init().
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>  xen/common/device_tree.c      |   71 ++++++++++++++++++++++++++---------------
>>  xen/include/xen/device_tree.h |    8 +++++
>>  2 files changed, 53 insertions(+), 26 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index e5df748..e95ff3c 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -66,7 +66,42 @@ static u32 __init prop_by_name_u32(const void *fdt, int node, const char *prop_n
>>      return fdt32_to_cpu(*(uint32_t*)prop->data);
>>  }
>>  
>> -static void __init process_memory_node(const void *fdt, int node,
>> +#define MAX_DEPTH 16
>> +
>> +/**
>> + * device_tree_for_each_node - iterate over all device tree nodes
>> + * @fdt: flat device tree.
>> + * @func: function to call for each node.
>> + * @data: data to pass to @func.
>> + */
>> +int device_tree_for_each_node(const void *fdt,
>> +                              device_tree_node_func func, void *data)
>> +{
>> +    int node;
>> +    int depth;
>> +    u32 address_cells[MAX_DEPTH];
>> +    u32 size_cells[MAX_DEPTH];
>> +    int ret;
>> +
>> +    for (node = 0, depth = 0;
>> +         node >=0 && depth >= 0;
>> +         node = fdt_next_node(fdt, node, &depth))
> 
> Xen coding style has spaces both sides of the outermost "(" and ")" of a
> for loop (similarly for if / while etc)

This is a minor difference between the Linux and Xen coding styles.
Given people often work on both can we not aim for more consistency
between the two?

I personally think the additional spaces reduce readability (but this is
probably mostly because I'm more familiar with Linux style rather than
any inherent improvement).

My recommendation would be to allow both styles of spacing withing Xen
but make it consistent within a file.

>> +    {
>> +        if (depth >= MAX_DEPTH)
> 
> Some sort of warning or error message would be useful here?

Tricky as this function is called early before printk() works and later
(when it does).

>> +            continue;
>> +
>> +        address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
>> +        size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
>> +
>> +        ret = func(fdt, node, fdt_get_name(fdt, node, NULL), depth,
>> +                   address_cells[depth-1], size_cells[depth-1], data);
> 
> I suppose this function could have been written recursively and avoided
> the arbitrary MAX_DEPTH, but actually in the hypervisor an iterative
> version with explicit maximum stack usage seems like a reasonable idea.

This is why I used a loop.

> I should have spotted this before, coding style needs a space inside the
> () and { should be on the next line. There's a bunch of this in both the
> context and the new code added by this patch. Obviously the new code
> should be fixed, I don't mind if you deal with the existing bits by a
> cleanup sweep or by picking it up as you go along.

See comment above.

David

Re: [PATCH 3/8] device tree: add device_tree_for_each_node()

[Ian Campbell]

On Wed, 2012-03-14 at 11:01 +0000, David Vrabel wrote:
> On 14/03/12 10:08, Ian Campbell wrote:
> > On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
> >> From: David Vrabel <david.vrabel@citrix.com>
> >>
> >> Add device_tree_for_each_node() to iterate over all nodes in a flat
> >> device tree.  Use this in device_tree_early_init().
> >>
> >> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> >> ---
> >>  xen/common/device_tree.c      |   71 ++++++++++++++++++++++++++---------------
> >>  xen/include/xen/device_tree.h |    8 +++++
> >>  2 files changed, 53 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> >> index e5df748..e95ff3c 100644
> >> --- a/xen/common/device_tree.c
> >> +++ b/xen/common/device_tree.c
> >> @@ -66,7 +66,42 @@ static u32 __init prop_by_name_u32(const void *fdt, int node, const char *prop_n
> >>      return fdt32_to_cpu(*(uint32_t*)prop->data);
> >>  }
> >>  
> >> -static void __init process_memory_node(const void *fdt, int node,
> >> +#define MAX_DEPTH 16
> >> +
> >> +/**
> >> + * device_tree_for_each_node - iterate over all device tree nodes
> >> + * @fdt: flat device tree.
> >> + * @func: function to call for each node.
> >> + * @data: data to pass to @func.
> >> + */
> >> +int device_tree_for_each_node(const void *fdt,
> >> +                              device_tree_node_func func, void *data)
> >> +{
> >> +    int node;
> >> +    int depth;
> >> +    u32 address_cells[MAX_DEPTH];
> >> +    u32 size_cells[MAX_DEPTH];
> >> +    int ret;
> >> +
> >> +    for (node = 0, depth = 0;
> >> +         node >=0 && depth >= 0;
> >> +         node = fdt_next_node(fdt, node, &depth))
> > 
> > Xen coding style has spaces both sides of the outermost "(" and ")" of a
> > for loop (similarly for if / while etc)
> 
> This is a minor difference between the Linux and Xen coding styles.
> Given people often work on both can we not aim for more consistency
> between the two?
> 
> I personally think the additional spaces reduce readability (but this is
> probably mostly because I'm more familiar with Linux style rather than
> any inherent improvement).
> 
> My recommendation would be to allow both styles of spacing withing Xen
> but make it consistent within a file.

This is for Keir to decide. At the moment the Xen coding style is what
it is.

> 
> >> +    {
> >> +        if (depth >= MAX_DEPTH)
> > 
> > Some sort of warning or error message would be useful here?
> 
> Tricky as this function is called early before printk() works and later
> (when it does).
> 
> >> +            continue;
> >> +
> >> +        address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
> >> +        size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
> >> +
> >> +        ret = func(fdt, node, fdt_get_name(fdt, node, NULL), depth,
> >> +                   address_cells[depth-1], size_cells[depth-1], data);
> > 
> > I suppose this function could have been written recursively and avoided
> > the arbitrary MAX_DEPTH, but actually in the hypervisor an iterative
> > version with explicit maximum stack usage seems like a reasonable idea.
> 
> This is why I used a loop.
> 
> > I should have spotted this before, coding style needs a space inside the
> > () and { should be on the next line. There's a bunch of this in both the
> > context and the new code added by this patch. Obviously the new code
> > should be fixed, I don't mind if you deal with the existing bits by a
> > cleanup sweep or by picking it up as you go along.
> 
> See comment above.
> 
> David

[PATCH 4/8] device tree: add device_tree_dump() to print a flat device tree

[David Vrabel]

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

Add a device_tree_dump() function which prints to main structure and
properties names of a flat device tree (but not the properties values
yet).

This will be useful for debugging problems with the device tree
generated for dom0.

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

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index e95ff3c..dbc4f61 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -101,6 +101,44 @@ int device_tree_for_each_node(const void *fdt,
     return 0;
 }
 
+static int dump_node(const void *fdt, int node, const char *name, int depth,
+                     u32 address_cells, u32 size_cells, void *data)
+{
+    char prefix[2*MAX_DEPTH + 1] = "";
+    int i;
+    int prop;
+
+    for (i = 0; i < depth; i++)
+        safe_strcat(prefix, "  ");
+
+    if (name[0] == '\0')
+        name = "/";
+    printk("%s%s:\n", prefix, name);
+
+    for (prop = fdt_first_property_offset(fdt, node);
+         prop >= 0;
+         prop = fdt_next_property_offset(fdt, prop))
+    {
+        const struct fdt_property *p;
+
+        p = fdt_get_property_by_offset(fdt, prop, NULL);
+
+        printk("%s  %s\n", prefix, fdt_string(fdt, fdt32_to_cpu(p->nameoff)));
+    }
+
+    return 0;
+}
+
+/**
+ * device_tree_dump - print a text representation of a device tree
+ * @fdt: flat device tree to print
+ */
+void device_tree_dump(const void *fdt)
+{
+    device_tree_for_each_node(fdt, dump_node, NULL);
+}
+
+
 static void __init process_memory_node(const void *fdt, int node, const char *name,
                                        u32 address_cells, u32 size_cells)
 {
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 505f3e3..b91b39f 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -41,5 +41,6 @@ paddr_t device_tree_get_xen_paddr(void);
 
 int device_tree_for_each_node(const void *fdt,
                               device_tree_node_func func, void *data);
+void device_tree_dump(const void *fdt);
 
 #endif
-- 
1.7.2.5

Re: [PATCH 4/8] device tree: add device_tree_dump() to print a flat device tree

[Ian Campbell]

On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Add a device_tree_dump() function which prints to main structure and
> properties names of a flat device tree (but not the properties values
> yet).
> 
> This will be useful for debugging problems with the device tree
> generated for dom0.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Other than coding style issues (which I'm not going to comment on any
more):
Acked-by: Ian Campbell <ian.campbell@citrix.com>


> ---
>  xen/common/device_tree.c      |   38 ++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h |    1 +
>  2 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index e95ff3c..dbc4f61 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -101,6 +101,44 @@ int device_tree_for_each_node(const void *fdt,
>      return 0;
>  }
>  
> +static int dump_node(const void *fdt, int node, const char *name, int depth,
> +                     u32 address_cells, u32 size_cells, void *data)
> +{
> +    char prefix[2*MAX_DEPTH + 1] = "";
> +    int i;
> +    int prop;
> +
> +    for (i = 0; i < depth; i++)
> +        safe_strcat(prefix, "  ");
> +
> +    if (name[0] == '\0')
> +        name = "/";
> +    printk("%s%s:\n", prefix, name);
> +
> +    for (prop = fdt_first_property_offset(fdt, node);
> +         prop >= 0;
> +         prop = fdt_next_property_offset(fdt, prop))
> +    {
> +        const struct fdt_property *p;
> +
> +        p = fdt_get_property_by_offset(fdt, prop, NULL);
> +
> +        printk("%s  %s\n", prefix, fdt_string(fdt, fdt32_to_cpu(p->nameoff)));
> +    }
> +
> +    return 0;
> +}
> +
> +/**
> + * device_tree_dump - print a text representation of a device tree
> + * @fdt: flat device tree to print
> + */
> +void device_tree_dump(const void *fdt)
> +{
> +    device_tree_for_each_node(fdt, dump_node, NULL);
> +}
> +
> +
>  static void __init process_memory_node(const void *fdt, int node, const char *name,
>                                         u32 address_cells, u32 size_cells)
>  {
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 505f3e3..b91b39f 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -41,5 +41,6 @@ paddr_t device_tree_get_xen_paddr(void);
>  
>  int device_tree_for_each_node(const void *fdt,
>                                device_tree_node_func func, void *data);
> +void device_tree_dump(const void *fdt);
>  
>  #endif

[PATCH 5/8] arm: remove the hack for loading vmlinux images

[David Vrabel]

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

Don't adjust the RAM location/size when loading an ELF for dom0.  It
was vmlinux specific and no longer needed because Linux can be loaded
from a zImage.

This also makes preparing the device tree for dom0 easier.

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

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 71a204d..dd757e5 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -91,7 +91,6 @@ static void kernel_zimage_load(struct kernel_info *info)
 
 /**
  * Check the image is a zImage and return the load address and length
- * (FIXME: including any appended DTB).
  */
 static int kernel_try_zimage_prepare(struct kernel_info *info)
 {
@@ -117,8 +116,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info)
         end += be32_to_cpu(dtb_hdr.total_size);
     }
 
-    /* FIXME: get RAM location from appended DTB (if there is one)? */
-
     /*
      * If start is zero, the zImage is position independent -- load it
      * at 32k from start of RAM.
@@ -166,13 +163,9 @@ static int kernel_try_elf_prepare(struct kernel_info *info)
         return rc;
 
     /*
-     * FIXME: can the ELF header be used to find the physical address
-     * to load the image to?  Instead of making virt == phys by
-     * relocating the guest's RAM.
+     * TODO: can the ELF header be used to find the physical address
+     * to load the image to?  Instead of assuming virt == phys.
      */
-    info->ram_start = 0xc0000000;
-    info->ram_end   = 0xc8000000;
-
     info->entry = info->elf.parms.virt_entry;
     info->load = kernel_elf_load;
 
-- 
1.7.2.5

Re: [PATCH 5/8] arm: remove the hack for loading vmlinux images

[Ian Campbell]

On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Don't adjust the RAM location/size when loading an ELF for dom0.  It
> was vmlinux specific and no longer needed because Linux can be loaded
> from a zImage.

I'm a bit confused, aren't kernel_try_zimage_prepare and
kernel_try_elf_prepare two different methods for loading the kernel? If
the adjustment is needed for vmlinux how does the fact that you could
load via zImage allow you to remove that adjustment in the vmlinux case?

Could we consider removing support for loading an ELF file entirely?

> 
> This also makes preparing the device tree for dom0 easier.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  xen/arch/arm/kernel.c |   11 ++---------
>  1 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 71a204d..dd757e5 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -91,7 +91,6 @@ static void kernel_zimage_load(struct kernel_info *info)
>  
>  /**
>   * Check the image is a zImage and return the load address and length
> - * (FIXME: including any appended DTB).
>   */
>  static int kernel_try_zimage_prepare(struct kernel_info *info)
>  {
> @@ -117,8 +116,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info)
>          end += be32_to_cpu(dtb_hdr.total_size);
>      }
>  
> -    /* FIXME: get RAM location from appended DTB (if there is one)? */
> -
>      /*
>       * If start is zero, the zImage is position independent -- load it
>       * at 32k from start of RAM.
> @@ -166,13 +163,9 @@ static int kernel_try_elf_prepare(struct kernel_info *info)
>          return rc;
>  
>      /*
> -     * FIXME: can the ELF header be used to find the physical address
> -     * to load the image to?  Instead of making virt == phys by
> -     * relocating the guest's RAM.
> +     * TODO: can the ELF header be used to find the physical address
> +     * to load the image to?  Instead of assuming virt == phys.
>       */
> -    info->ram_start = 0xc0000000;
> -    info->ram_end   = 0xc8000000;
> -
>      info->entry = info->elf.parms.virt_entry;
>      info->load = kernel_elf_load;
>  

Re: [PATCH 5/8] arm: remove the hack for loading vmlinux images

[David Vrabel]

On 14/03/12 10:17, Ian Campbell wrote:
> On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Don't adjust the RAM location/size when loading an ELF for dom0.  It
>> was vmlinux specific and no longer needed because Linux can be loaded
>> from a zImage.
> 
> I'm a bit confused, aren't kernel_try_zimage_prepare and
> kernel_try_elf_prepare two different methods for loading the kernel? If
> the adjustment is needed for vmlinux how does the fact that you could
> load via zImage allow you to remove that adjustment in the vmlinux case?
> 
> Could we consider removing support for loading an ELF file entirely?

I left the ELF support in case we need to load some other (non-vmlinux)
ELF image.

David

[PATCH 6/8] device tree, arm: supply a flat device tree to dom0

[David Vrabel]

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

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

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

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

Support for passing ATAGS to dom0 is removed.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/arm/Makefile         |    2 +
 xen/arch/arm/domain_build.c   |  238 ++++++++++++++++++++++++++++++++++-------
 xen/arch/arm/kernel.c         |    4 +-
 xen/arch/arm/kernel.h         |    8 +-
 xen/common/device_tree.c      |   44 +++++---
 xen/include/xen/device_tree.h |    8 ++
 6 files changed, 246 insertions(+), 58 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index da5096a..4c9517c 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -25,6 +25,8 @@ obj-y += vtimer.o
 
 #obj-bin-y += ....o
 
+CFLAGS += -I../../common/libfdt
+
 ifdef CONFIG_DTB_FILE
 obj-y += dtb.o
 AFLAGS += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9240209..ca8c706 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -6,6 +6,11 @@
 #include <xen/sched.h>
 #include <asm/irq.h>
 #include <asm/regs.h>
+#include <xen/errno.h>
+#include <xen/device_tree.h>
+#include <xen/guest_access.h>
+
+#include <libfdt.h>
 
 #include "gic.h"
 #include "kernel.h"
@@ -27,43 +32,190 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
     return alloc_vcpu(dom0, 0, 0);
 }
 
-extern void guest_mode_entry(void);
+static void set_memory_reg(struct domain *d, struct kernel_info *dom0,
+                           const void *fdt,
+                           const u32 *cell, int address_cells, int size_cells,
+                           u32 *new_cell, int *len)
+{
+    int reg_size = (address_cells + size_cells) * sizeof(*cell);
+    int l;
+    u64 start;
+    u64 size;
+
+    l = *len;
+
+    while (dom0->unassigned_mem > 0 && l > reg_size
+           && dom0->mem.nr_banks < NR_MEM_BANKS)
+    {
+        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
+        if (size > dom0->unassigned_mem)
+            size = dom0->unassigned_mem;
+
+        device_tree_set_reg(&new_cell, address_cells, size_cells, start, size);
+
+        printk("Populate P2M %#llx->%#llx\n", start, start + size);
+        p2m_populate_ram(d, start, start + size);
+        dom0->mem.bank[dom0->mem.nr_banks].start = start;
+        dom0->mem.bank[dom0->mem.nr_banks].size = size;
+        dom0->unassigned_mem -= size;
+
+        l -= reg_size;
+    }
+
+    *len -= l;
+}
+
+static int write_properties(struct domain *d, struct kernel_info *dom0,
+                            const void *fdt,
+                            int node, const char *name, int depth,
+                            u32 address_cells, u32 size_cells)
+{
+    int prop;
+
+    for (prop = fdt_first_property_offset(fdt, node);
+         prop >= 0;
+         prop = fdt_next_property_offset(fdt, prop))
+    {
+        const struct fdt_property *p;
+        const char *prop_name;
+        const char *prop_data;
+        int prop_len;
+        char *new_data = NULL;
+
+        p = fdt_get_property_by_offset(fdt, prop, NULL);
+        prop_name = fdt_string(fdt, fdt32_to_cpu(p->nameoff));
+        prop_data = p->data;
+        prop_len  = fdt32_to_cpu(p->len);
+
+        /*
+         * In chosen node: replace bootargs with value from
+         * xen,dom0-bootargs.
+         */
+        if (device_tree_node_matches(fdt, node, "chosen")) {
+            if (strcmp(prop_name, "bootargs") == 0)
+                continue;
+            if (strcmp(prop_name, "xen,dom0-bootargs") == 0)
+                prop_name = "bootargs";
+        }
+
+        /*
+         * In a memory node: adjust reg property.
+         */
+        if (device_tree_node_matches(fdt, node, "memory")) {
+            if (strcmp(prop_name, "reg") == 0) {
+                new_data = xzalloc_bytes(prop_len);
+                if (new_data  == NULL)
+                    return -ENOMEM;
+
+                set_memory_reg(d, dom0, fdt,
+                               (u32 *)prop_data, address_cells, size_cells,
+                               (u32 *)new_data, &prop_len);
+                prop_data = new_data;
+            }
+        }
+
+        /*
+         * TODO: Should call map_mmio_regions() for all devices in the
+         * tree that have a "reg" parameter (except cpus).  This
+         * requires that adjacent regions are coalesced and rounded up
+         * to whole pages.
+         */
+
+        fdt_property(dom0->fdt, prop_name, prop_data, prop_len);
+
+        xfree(new_data);
+    }
+
+    if (prop == -FDT_ERR_NOTFOUND)
+        return 0;
+    return prop;
+}
+
+static int write_nodes(struct domain *d, struct kernel_info *dom0,
+                       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_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\n", name);
+            continue;
+        }
+
+        while (last_depth-- >= depth)
+            fdt_end_node(dom0->fdt);
+
+        address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells");
+        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
+
+        fdt_begin_node(dom0->fdt, name);
 
-static void setup_linux_atag(paddr_t tags, paddr_t ram_s, paddr_t ram_e)
+        ret = write_properties(d, dom0, fdt, node, name, depth,
+                               address_cells[depth-1], size_cells[depth-1]);
+        if (ret < 0)
+            return ret;
+
+        last_depth = depth;
+    }
+
+    while (last_depth-- >= 0)
+        fdt_end_node(dom0->fdt);
+
+    return 0;
+}
+
+static int prepare_dtb(struct domain *d, struct kernel_info *dom0)
 {
-    paddr_t ma = gvirt_to_maddr(tags);
-    void *map = map_domain_page(ma>>PAGE_SHIFT);
-    void *p = map + (tags & (PAGE_SIZE - 1));
-    char cmdline[] = "earlyprintk=xenboot console=ttyAMA1 root=/dev/mmcblk0 debug rw";
+    void *fdt;
+    int new_size;
+    int ret;
 
-    /* not enough room on this page for all the tags */
-    BUG_ON(PAGE_SIZE - (tags & (PAGE_SIZE - 1)) < 8 * sizeof(uint32_t));
+    fdt = device_tree_flattened;
 
-#define TAG(type, val) *(type*)p = val; p+= sizeof(type)
+    new_size = fdt_totalsize(fdt) + 8192;
+    dom0->fdt = xmalloc_bytes(new_size);
+    if (dom0->fdt == NULL)
+        return -ENOMEM;
 
-    /* ATAG_CORE */
-    TAG(uint32_t, 2);
-    TAG(uint32_t, 0x54410001);
+    ret = fdt_create(dom0->fdt, new_size);
+    if (ret < 0)
+        goto err;
 
-    /* ATAG_MEM */
-    TAG(uint32_t, 4);
-    TAG(uint32_t, 0x54410002);
-    TAG(uint32_t, (ram_e - ram_s) & 0xFFFFFFFF);
-    TAG(uint32_t, ram_s & 0xFFFFFFFF);
+    fdt_finish_reservemap(dom0->fdt);
 
-    /* ATAG_CMDLINE */
-    TAG(uint32_t, 2 + ((strlen(cmdline) + 4) >> 2));
-    TAG(uint32_t, 0x54410009);
-    memcpy(p, cmdline, strlen(cmdline) + 1);
-    p += ((strlen(cmdline) + 4) >> 2) << 2;
+    ret = write_nodes(d, dom0, fdt);
+    if (ret < 0)
+        goto err;
 
-    /* ATAG_NONE */
-    TAG(uint32_t, 0);
-    TAG(uint32_t, 0);
+    fdt_finish(dom0->fdt);
 
-#undef TAG
+    device_tree_dump(dom0->fdt);
+
+    dom0->fdt = dom0->fdt;
+    return 0;
+
+  err:
+    xfree(dom0->fdt);
+    return ret;
+}
+
+static void dtb_load(struct kernel_info *dom0)
+{
+    void *dtb_virt = (void *)(u32)dom0->dtb_paddr;
 
-    unmap_domain_page(map);
+    raw_copy_to_guest(dtb_virt, dom0->fdt, fdt_totalsize(dom0->fdt));
+    xfree(dom0->fdt);
 }
 
 int construct_dom0(struct domain *d)
@@ -81,22 +233,27 @@ int construct_dom0(struct domain *d)
 
     printk("*** LOADING DOMAIN 0 ***\n");
 
-    /* 128M at 2G physical */
-    /* TODO size and location from DT. */
-    kinfo.ram_start = 0x80000000;
-    kinfo.ram_end   = 0x88000000;
+    d->max_pages = ~0U;
 
-    rc = kernel_prepare(&kinfo);
-    if (rc < 0)
+    if ( (rc = p2m_alloc_table(d)) != 0 )
         return rc;
 
-    d->max_pages = ~0U;
+    kinfo.unassigned_mem = 0x08000000; /* XXX */
 
-    if ( (rc = p2m_alloc_table(d)) != 0 )
+    rc = prepare_dtb(d, &kinfo);
+    if (rc < 0)
         return rc;
 
-    printk("Populate P2M %#llx->%#llx\n", kinfo.ram_start, kinfo.ram_end);
-    p2m_populate_ram(d, kinfo.ram_start, kinfo.ram_end);
+    /* First RAM bank must be below 4 GiB or a 32-bit dom0 kernel
+       cannot be loaded. */
+    if (kinfo.mem.bank[0].start >= 1ull << 32) {
+        printk("No memory accessible to 32 bit dom0.");
+        return -EINVAL;
+    }
+
+    rc = kernel_prepare(&kinfo);
+    if (rc < 0)
+        return rc;
 
     printk("Map CS2 MMIO regions 1:1 in the P2M %#llx->%#llx\n", 0x18000000ULL, 0x1BFFFFFFULL);
     map_mmio_regions(d, 0x18000000, 0x1BFFFFFF, 0x18000000);
@@ -124,13 +281,12 @@ int construct_dom0(struct domain *d)
     /* Enable second stage translation */
     WRITE_CP32(READ_CP32(HCR) | HCR_VM, HCR); isb();
 
-    /* The following load uses domain's p2m */
+    /* The following loads use the domain's p2m */
     p2m_load_VTTBR(d);
 
+    dtb_load(&kinfo);
     kernel_load(&kinfo);
 
-    setup_linux_atag(kinfo.ram_start + 0x100, kinfo.ram_start, kinfo.ram_end);
-
     clear_bit(_VPF_down, &v->pause_flags);
 
     memset(regs, 0, sizeof(*regs));
@@ -152,7 +308,7 @@ int construct_dom0(struct domain *d)
 
     regs->r0 = 0; /* SBZ */
     regs->r1 = 2272; /* Machine NR: Versatile Express */
-    regs->r2 = kinfo.ram_start + 0x100; /* ATAGS */
+    regs->r2 = kinfo.dtb_paddr;
 
     WRITE_CP32(SCTLR_BASE, SCTLR);
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index dd757e5..b27b20d 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -121,7 +121,7 @@ static int kernel_try_zimage_prepare(struct kernel_info *info)
      * at 32k from start of RAM.
      */
     if (start == 0)
-        info->zimage.load_addr = info->ram_start + 0x8000;
+        info->zimage.load_addr = info->mem.bank[0].start + 0x8000;
     else
         info->zimage.load_addr = start;
     info->zimage.len = end - start;
@@ -176,6 +176,8 @@ int kernel_prepare(struct kernel_info *info)
 {
     int rc;
 
+    info->dtb_paddr = info->mem.bank[0].start + 0x100;
+
     rc = kernel_try_zimage_prepare(info);
     if (rc < 0)
         rc = kernel_try_elf_prepare(info);
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index 5caebe5..4533568 100644
--- a/xen/arch/arm/kernel.h
+++ b/xen/arch/arm/kernel.h
@@ -7,11 +7,15 @@
 #define __ARCH_ARM_KERNEL_H__
 
 #include <xen/libelf.h>
+#include <xen/device_tree.h>
 
 struct kernel_info {
+    void *fdt; /* flat device tree */
+    paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
+    struct dt_mem_info mem;
+
+    paddr_t dtb_paddr;
     paddr_t entry;
-    paddr_t ram_start;
-    paddr_t ram_end;
 
     void *kernel_img;
     unsigned kernel_order;
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index dbc4f61..2422fba 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -24,7 +24,7 @@
 struct dt_early_info __initdata early_info;
 void *device_tree_flattened;
 
-static bool_t __init node_matches(const void *fdt, int node, const char *match)
+bool_t device_tree_node_matches(const void *fdt, int node, const char *match)
 {
     const char *name;
     size_t len;
@@ -48,14 +48,32 @@ static void __init get_val(const u32 **cell, u32 cells, u64 *val)
     }
 }
 
-static void __init get_register(const u32 **cell, u32 address_cells, u32 size_cells,
-                                u64 *start, u64 *size)
+void device_tree_get_reg(const u32 **cell, u32 address_cells, u32 size_cells,
+                         u64 *start, u64 *size)
 {
     get_val(cell, address_cells, start);
     get_val(cell, size_cells, size);
 }
 
-static u32 __init prop_by_name_u32(const void *fdt, int node, const char *prop_name)
+void set_val(u32 **cell, u32 cells, u64 val)
+{
+    u32 c = cells;
+
+    while (c--) {
+        (*cell)[c] = cpu_to_fdt32(val);
+        val >>= 32;
+    }
+    (*cell) += cells;
+}
+
+void device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
+                         u64 start, u64 size)
+{
+    set_val(cell, address_cells, start);
+    set_val(cell, size_cells, size);
+}
+
+u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name)
 {
     const struct fdt_property *prop;
 
@@ -66,8 +84,6 @@ static u32 __init prop_by_name_u32(const void *fdt, int node, const char *prop_n
     return fdt32_to_cpu(*(uint32_t*)prop->data);
 }
 
-#define MAX_DEPTH 16
-
 /**
  * device_tree_for_each_node - iterate over all device tree nodes
  * @fdt: flat device tree.
@@ -79,19 +95,19 @@ int device_tree_for_each_node(const void *fdt,
 {
     int node;
     int depth;
-    u32 address_cells[MAX_DEPTH];
-    u32 size_cells[MAX_DEPTH];
+    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
+    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
     int ret;
 
     for (node = 0, depth = 0;
          node >=0 && depth >= 0;
          node = fdt_next_node(fdt, node, &depth))
     {
-        if (depth >= MAX_DEPTH)
+        if (depth >= DEVICE_TREE_MAX_DEPTH)
             continue;
 
-        address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
-        size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
+        address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells");
+        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
 
         ret = func(fdt, node, fdt_get_name(fdt, node, NULL), depth,
                    address_cells[depth-1], size_cells[depth-1], data);
@@ -104,7 +120,7 @@ int device_tree_for_each_node(const void *fdt,
 static int dump_node(const void *fdt, int node, const char *name, int depth,
                      u32 address_cells, u32 size_cells, void *data)
 {
-    char prefix[2*MAX_DEPTH + 1] = "";
+    char prefix[2*DEVICE_TREE_MAX_DEPTH + 1] = "";
     int i;
     int prop;
 
@@ -165,7 +181,7 @@ static void __init process_memory_node(const void *fdt, int node, const char *na
     banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32));
 
     for (i = 0; i < banks && early_info.mem.nr_banks < NR_MEM_BANKS; i++) {
-        get_register(&cell, address_cells, size_cells, &start, &size);
+        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
         early_info.mem.bank[early_info.mem.nr_banks].start = start;
         early_info.mem.bank[early_info.mem.nr_banks].size = size;
         early_info.mem.nr_banks++;
@@ -176,7 +192,7 @@ static int __init early_scan_node(const void *fdt,
                                   int node, const char *name, int depth,
                                   u32 address_cells, u32 size_cells, void *data)
 {
-    if (node_matches(fdt, node, "memory"))
+    if (device_tree_node_matches(fdt, node, "memory"))
         process_memory_node(fdt, node, name, address_cells, size_cells);
 
     return 0;
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index b91b39f..510b5b4 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -12,6 +12,8 @@
 
 #include <xen/types.h>
 
+#define DEVICE_TREE_MAX_DEPTH 16
+
 #define NR_MEM_BANKS 8
 
 struct membank {
@@ -39,6 +41,12 @@ extern void *device_tree_flattened;
 size_t device_tree_early_init(const void *fdt);
 paddr_t device_tree_get_xen_paddr(void);
 
+void device_tree_get_reg(const u32 **cell, u32 address_cells, u32 size_cells,
+                         u64 *start, u64 *size);
+void device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
+                         u64 start, u64 size);
+u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name);
+bool_t device_tree_node_matches(const void *fdt, int node, const char *match);
 int device_tree_for_each_node(const void *fdt,
                               device_tree_node_func func, void *data);
 void device_tree_dump(const void *fdt);
-- 
1.7.2.5

Re: [PATCH 6/8] device tree, arm: supply a flat device tree to dom0

[Ian Campbell]

On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Build a flat device tree for dom0 based on the one supplied to Xen.
> The following changes are made:
> 
>   * In the /chosen node, the xen,dom0-bootargs parameter is renamed to
>     bootargs.
> 
>   * In all memory nodes, the reg parameters are adjusted to reflect
>     the amount of memory dom0 can use.  The p2m is updated using this
>     info.

Do you also do
    * Hide the xen console UART from dom0

? (it seems not)

> Support for passing ATAGS to dom0 is removed.

> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  xen/arch/arm/Makefile         |    2 +
>  xen/arch/arm/domain_build.c   |  238 ++++++++++++++++++++++++++++++++++-------
>  xen/arch/arm/kernel.c         |    4 +-
>  xen/arch/arm/kernel.h         |    8 +-
>  xen/common/device_tree.c      |   44 +++++---
>  xen/include/xen/device_tree.h |    8 ++
>  6 files changed, 246 insertions(+), 58 deletions(-)
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index da5096a..4c9517c 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -25,6 +25,8 @@ obj-y += vtimer.o
> 
>  #obj-bin-y += ....o
> 
> +CFLAGS += -I../../common/libfdt

This suggests that something ought to be moved to xen/include/xen.

> +
>  ifdef CONFIG_DTB_FILE
>  obj-y += dtb.o
>  AFLAGS += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9240209..ca8c706 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -6,6 +6,11 @@
>  #include <xen/sched.h>
>  #include <asm/irq.h>
>  #include <asm/regs.h>
> +#include <xen/errno.h>
> +#include <xen/device_tree.h>
> +#include <xen/guest_access.h>
> +
> +#include <libfdt.h>
> 
>  #include "gic.h"
>  #include "kernel.h"
> @@ -27,43 +32,190 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
>      return alloc_vcpu(dom0, 0, 0);
>  }
> 
> -extern void guest_mode_entry(void);
> +static void set_memory_reg(struct domain *d, struct kernel_info *dom0,
> +                           const void *fdt,
> +                           const u32 *cell, int address_cells, int size_cells,
> +                           u32 *new_cell, int *len)
> +{
> +    int reg_size = (address_cells + size_cells) * sizeof(*cell);
> +    int l;
> +    u64 start;
> +    u64 size;
> +
> +    l = *len;
> +
> +    while (dom0->unassigned_mem > 0 && l > reg_size
> +           && dom0->mem.nr_banks < NR_MEM_BANKS)
> +    {
> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +        if (size > dom0->unassigned_mem)
> +            size = dom0->unassigned_mem;

I somehow can't find any struct members named unassigned_mem in struct
domain (or in my whole tree). I'm obviously missing something...

... which is that there is a local "struct kernel_info *dom0" shadowing
the global "struct domain *dom0". I think another name for the local var
would be useful ;-)

> +
> +        device_tree_set_reg(&new_cell, address_cells, size_cells, start, size);
> +
> +        printk("Populate P2M %#llx->%#llx\n", start, start + size);
> +        p2m_populate_ram(d, start, start + size);
> +        dom0->mem.bank[dom0->mem.nr_banks].start = start;
> +        dom0->mem.bank[dom0->mem.nr_banks].size = size;
> +        dom0->unassigned_mem -= size;
> +
> +        l -= reg_size;
> +    }
> +
> +    *len -= l;
> +}
> +
> +static int write_properties(struct domain *d, struct kernel_info *dom0,
> +                            const void *fdt,
> +                            int node, const char *name, int depth,
> +                            u32 address_cells, u32 size_cells)
> +{
> +    int prop;
> +
> +    for (prop = fdt_first_property_offset(fdt, node);
> +         prop >= 0;
> +         prop = fdt_next_property_offset(fdt, prop))
> +    {
> +        const struct fdt_property *p;
> +        const char *prop_name;
> +        const char *prop_data;
> +        int prop_len;
> +        char *new_data = NULL;
> +
> +        p = fdt_get_property_by_offset(fdt, prop, NULL);
> +        prop_name = fdt_string(fdt, fdt32_to_cpu(p->nameoff));
> +        prop_data = p->data;
> +        prop_len  = fdt32_to_cpu(p->len);
> +
> +        /*
> +         * In chosen node: replace bootargs with value from
> +         * xen,dom0-bootargs.
> +         */
> +        if (device_tree_node_matches(fdt, node, "chosen")) {
> +            if (strcmp(prop_name, "bootargs") == 0)
> +                continue;
> +            if (strcmp(prop_name, "xen,dom0-bootargs") == 0)
> +                prop_name = "bootargs";
> +        }
> +
> +        /*
> +         * In a memory node: adjust reg property.
> +         */
> +        if (device_tree_node_matches(fdt, node, "memory")) {
> +            if (strcmp(prop_name, "reg") == 0) {
> +                new_data = xzalloc_bytes(prop_len);
> +                if (new_data  == NULL)
> +                    return -ENOMEM;
> +
> +                set_memory_reg(d, dom0, fdt,
> +                               (u32 *)prop_data, address_cells, size_cells,
> +                               (u32 *)new_data, &prop_len);
> +                prop_data = new_data;

Need to free the previous value of prop_data?

> +            }
> +        }
> +
> +        /*
> +         * TODO: Should call map_mmio_regions() for all devices in the
> +         * tree that have a "reg" parameter (except cpus).  This
> +         * requires that adjacent regions are coalesced and rounded up
> +         * to whole pages.

Does map_mmio_regions currently require you to coalesce? If it was smart
enough to use larger mappings for contiguous areas (which it isn't, but
probably should?) then I suppose coalescing would be useful.

> +         */
> +
> +        fdt_property(dom0->fdt, prop_name, prop_data, prop_len);
> +
> +        xfree(new_data);

Does fdt_property duplicate prop_data? (answer: yes)

> +    }
> +
> +    if (prop == -FDT_ERR_NOTFOUND)
> +        return 0;
> +    return prop;
> +}
> +
> +static int write_nodes(struct domain *d, struct kernel_info *dom0,
> +                       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];

In a previous patch you had a local MAX_DEPTH. Shouldn't this be used
instead.

(comes back later: oh, I see it is. good ;-))

> +    int ret;
> +
> +    for (node = 0, depth = 0;
> +         node >= 0 && depth >= 0;
> +         node = fdt_next_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\n", name);
> +            continue;
> +        }
> +
> +        while (last_depth-- >= depth)
> +            fdt_end_node(dom0->fdt);
> +
> +        address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells");
> +        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
> +
> +        fdt_begin_node(dom0->fdt, name);
> 
> -static void setup_linux_atag(paddr_t tags, paddr_t ram_s, paddr_t ram_e)
> +        ret = write_properties(d, dom0, fdt, node, name, depth,
> +                               address_cells[depth-1], size_cells[depth-1]);
> +        if (ret < 0)
> +            return ret;
> +
> +        last_depth = depth;
> +    }
> +
> +    while (last_depth-- >= 0)
> +        fdt_end_node(dom0->fdt);
> +
> +    return 0;
> +}
> +
> +static int prepare_dtb(struct domain *d, struct kernel_info *dom0)
>  {
> -    paddr_t ma = gvirt_to_maddr(tags);
> -    void *map = map_domain_page(ma>>PAGE_SHIFT);
> -    void *p = map + (tags & (PAGE_SIZE - 1));
> -    char cmdline[] = "earlyprintk=xenboot console=ttyAMA1 root=/dev/mmcblk0 debug rw";
> +    void *fdt;
> +    int new_size;
> +    int ret;
> 
> -    /* not enough room on this page for all the tags */
> -    BUG_ON(PAGE_SIZE - (tags & (PAGE_SIZE - 1)) < 8 * sizeof(uint32_t));
> +    fdt = device_tree_flattened;
> 
> -#define TAG(type, val) *(type*)p = val; p+= sizeof(type)
> +    new_size = fdt_totalsize(fdt) + 8192;

2*PAGE_SIZE? Or is this number magic in some other way?

> +    dom0->fdt = xmalloc_bytes(new_size);
> +    if (dom0->fdt == NULL)
> +        return -ENOMEM;
> 
> -    /* ATAG_CORE */
> -    TAG(uint32_t, 2);
> -    TAG(uint32_t, 0x54410001);
> +    ret = fdt_create(dom0->fdt, new_size);
> +    if (ret < 0)
> +        goto err;
> 
> -    /* ATAG_MEM */
> -    TAG(uint32_t, 4);
> -    TAG(uint32_t, 0x54410002);
> -    TAG(uint32_t, (ram_e - ram_s) & 0xFFFFFFFF);
> -    TAG(uint32_t, ram_s & 0xFFFFFFFF);
> +    fdt_finish_reservemap(dom0->fdt);
> 
> -    /* ATAG_CMDLINE */
> -    TAG(uint32_t, 2 + ((strlen(cmdline) + 4) >> 2));
> -    TAG(uint32_t, 0x54410009);
> -    memcpy(p, cmdline, strlen(cmdline) + 1);
> -    p += ((strlen(cmdline) + 4) >> 2) << 2;
> +    ret = write_nodes(d, dom0, fdt);
> +    if (ret < 0)
> +        goto err;
> 
> -    /* ATAG_NONE */
> -    TAG(uint32_t, 0);
> -    TAG(uint32_t, 0);
> +    fdt_finish(dom0->fdt);
> 
> -#undef TAG
> +    device_tree_dump(dom0->fdt);
> +
> +    dom0->fdt = dom0->fdt;
> +    return 0;
> +
> +  err:
> +    xfree(dom0->fdt);
> +    return ret;
> +}
> +
> +static void dtb_load(struct kernel_info *dom0)
> +{
> +    void *dtb_virt = (void *)(u32)dom0->dtb_paddr;

This is part of the 1:1 map? Shuldn't this be either a variant of __va
or some sort of explicit map create function?

> 
> -    unmap_domain_page(map);
> +    raw_copy_to_guest(dtb_virt, dom0->fdt, fdt_totalsize(dom0->fdt));

Oh, so dtb_virt is a _dom0_ virtual address, which because we are
building the domain with the stage 1 MMU disabled == the phys addr.
Might be worthy of a comment? Our use of __user for such functions seems
a little inconsistent or I would suggest
	void * __user dtb_virt = ...
(maybe that's a good idea anyway?)

Do we arrange somewhere that dtb_paddr < 4G ? I suppose we must...

> +    xfree(dom0->fdt);
>  }
> 
>  int construct_dom0(struct domain *d)
> @@ -81,22 +233,27 @@ int construct_dom0(struct domain *d)
> 
>      printk("*** LOADING DOMAIN 0 ***\n");
> 
> -    /* 128M at 2G physical */
> -    /* TODO size and location from DT. */
> -    kinfo.ram_start = 0x80000000;
> -    kinfo.ram_end   = 0x88000000;
> +    d->max_pages = ~0U;
> 
> -    rc = kernel_prepare(&kinfo);
> -    if (rc < 0)
> +    if ( (rc = p2m_alloc_table(d)) != 0 )
>          return rc;
> 
> -    d->max_pages = ~0U;
> +    kinfo.unassigned_mem = 0x08000000; /* XXX */
> 
> -    if ( (rc = p2m_alloc_table(d)) != 0 )
> +    rc = prepare_dtb(d, &kinfo);
> +    if (rc < 0)
>          return rc;
> 
> -    printk("Populate P2M %#llx->%#llx\n", kinfo.ram_start, kinfo.ram_end);
> -    p2m_populate_ram(d, kinfo.ram_start, kinfo.ram_end);
> +    /* First RAM bank must be below 4 GiB or a 32-bit dom0 kernel
> +       cannot be loaded. */

In principal we could relocate a bank to a hole under 4G but I expect
that all machines will meet this constraint, or they'd have trouble
booting native.

BTW, does this DTB stuff mean that we no longer use the 40 bit physical
address alias to access RAM form the hypervisor? Not a big problem, we
just did that to help shake out 32 bitness assumptions.

> +    if (kinfo.mem.bank[0].start >= 1ull << 32) {
> +        printk("No memory accessible to 32 bit dom0.");
> +        return -EINVAL;
> +    }
> +
> +    rc = kernel_prepare(&kinfo);
> +    if (rc < 0)
> +        return rc;
> 
>      printk("Map CS2 MMIO regions 1:1 in the P2M %#llx->%#llx\n", 0x18000000ULL, 0x1BFFFFFFULL);
>      map_mmio_regions(d, 0x18000000, 0x1BFFFFFF, 0x18000000);
> @@ -124,13 +281,12 @@ int construct_dom0(struct domain *d)
>      /* Enable second stage translation */
>      WRITE_CP32(READ_CP32(HCR) | HCR_VM, HCR); isb();
> 
> -    /* The following load uses domain's p2m */
> +    /* The following loads use the domain's p2m */
>      p2m_load_VTTBR(d);
> 
> +    dtb_load(&kinfo);
>      kernel_load(&kinfo);
> 
> -    setup_linux_atag(kinfo.ram_start + 0x100, kinfo.ram_start, kinfo.ram_end);
> -
>      clear_bit(_VPF_down, &v->pause_flags);
> 
>      memset(regs, 0, sizeof(*regs));
> @@ -152,7 +308,7 @@ int construct_dom0(struct domain *d)
> 
>      regs->r0 = 0; /* SBZ */
>      regs->r1 = 2272; /* Machine NR: Versatile Express */
> -    regs->r2 = kinfo.ram_start + 0x100; /* ATAGS */
> +    regs->r2 = kinfo.dtb_paddr;
> 
>      WRITE_CP32(SCTLR_BASE, SCTLR);
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index dd757e5..b27b20d 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -121,7 +121,7 @@ static int kernel_try_zimage_prepare(struct kernel_info *info)
>       * at 32k from start of RAM.
>       */
>      if (start == 0)
> -        info->zimage.load_addr = info->ram_start + 0x8000;
> +        info->zimage.load_addr = info->mem.bank[0].start + 0x8000;
>      else
>          info->zimage.load_addr = start;
>      info->zimage.len = end - start;
> @@ -176,6 +176,8 @@ int kernel_prepare(struct kernel_info *info)
>  {
>      int rc;
> 
> +    info->dtb_paddr = info->mem.bank[0].start + 0x100;

assert(info->dtb_paddr < 0x100000000ULL); ???

I suppose it migth also be useful to check dtb_paddr + dtb_len < 4G too?

> +
>      rc = kernel_try_zimage_prepare(info);
>      if (rc < 0)
>          rc = kernel_try_elf_prepare(info);
> diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
> index 5caebe5..4533568 100644
> --- a/xen/arch/arm/kernel.h
> +++ b/xen/arch/arm/kernel.h
> @@ -7,11 +7,15 @@
>  #define __ARCH_ARM_KERNEL_H__
> 
>  #include <xen/libelf.h>
> +#include <xen/device_tree.h>
> 
>  struct kernel_info {
> +    void *fdt; /* flat device tree */
> +    paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
> +    struct dt_mem_info mem;
> +
> +    paddr_t dtb_paddr;
>      paddr_t entry;
> -    paddr_t ram_start;
> -    paddr_t ram_end;
> 
>      void *kernel_img;
>      unsigned kernel_order;
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index dbc4f61..2422fba 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -24,7 +24,7 @@
>  struct dt_early_info __initdata early_info;
>  void *device_tree_flattened;
> 
> -static bool_t __init node_matches(const void *fdt, int node, const char *match)
> +bool_t device_tree_node_matches(const void *fdt, int node, const char *match)
>  {
>      const char *name;
>      size_t len;
> @@ -48,14 +48,32 @@ static void __init get_val(const u32 **cell, u32 cells, u64 *val)
>      }
>  }
> 
> -static void __init get_register(const u32 **cell, u32 address_cells, u32 size_cells,
> -                                u64 *start, u64 *size)
> +void device_tree_get_reg(const u32 **cell, u32 address_cells, u32 size_cells,
> +                         u64 *start, u64 *size)
>  {
>      get_val(cell, address_cells, start);
>      get_val(cell, size_cells, size);
>  }
> 
> -static u32 __init prop_by_name_u32(const void *fdt, int node, const char *prop_name)
> +void set_val(u32 **cell, u32 cells, u64 val)
> +{
> +    u32 c = cells;
> +
> +    while (c--) {
> +        (*cell)[c] = cpu_to_fdt32(val);
> +        val >>= 32;
> +    }
> +    (*cell) += cells;
> +}
> +
> +void device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
> +                         u64 start, u64 size)
> +{
> +    set_val(cell, address_cells, start);
> +    set_val(cell, size_cells, size);
> +}
> +
> +u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name)
>  {
>      const struct fdt_property *prop;
> 
> @@ -66,8 +84,6 @@ static u32 __init prop_by_name_u32(const void *fdt, int node, const char *prop_n
>      return fdt32_to_cpu(*(uint32_t*)prop->data);
>  }
> 
> -#define MAX_DEPTH 16
> -
>  /**
>   * device_tree_for_each_node - iterate over all device tree nodes
>   * @fdt: flat device tree.
> @@ -79,19 +95,19 @@ int device_tree_for_each_node(const void *fdt,
>  {
>      int node;
>      int depth;
> -    u32 address_cells[MAX_DEPTH];
> -    u32 size_cells[MAX_DEPTH];
> +    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
> +    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
>      int ret;
> 
>      for (node = 0, depth = 0;
>           node >=0 && depth >= 0;
>           node = fdt_next_node(fdt, node, &depth))
>      {
> -        if (depth >= MAX_DEPTH)
> +        if (depth >= DEVICE_TREE_MAX_DEPTH)
>              continue;
> 
> -        address_cells[depth] = prop_by_name_u32(fdt, node, "#address-cells");
> -        size_cells[depth] = prop_by_name_u32(fdt, node, "#size-cells");
> +        address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells");
> +        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
> 
>          ret = func(fdt, node, fdt_get_name(fdt, node, NULL), depth,
>                     address_cells[depth-1], size_cells[depth-1], data);
> @@ -104,7 +120,7 @@ int device_tree_for_each_node(const void *fdt,
>  static int dump_node(const void *fdt, int node, const char *name, int depth,
>                       u32 address_cells, u32 size_cells, void *data)
>  {
> -    char prefix[2*MAX_DEPTH + 1] = "";
> +    char prefix[2*DEVICE_TREE_MAX_DEPTH + 1] = "";
>      int i;
>      int prop;
> 
> @@ -165,7 +181,7 @@ static void __init process_memory_node(const void *fdt, int node, const char *na
>      banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32));
> 
>      for (i = 0; i < banks && early_info.mem.nr_banks < NR_MEM_BANKS; i++) {
> -        get_register(&cell, address_cells, size_cells, &start, &size);
> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>          early_info.mem.bank[early_info.mem.nr_banks].start = start;
>          early_info.mem.bank[early_info.mem.nr_banks].size = size;
>          early_info.mem.nr_banks++;
> @@ -176,7 +192,7 @@ static int __init early_scan_node(const void *fdt,
>                                    int node, const char *name, int depth,
>                                    u32 address_cells, u32 size_cells, void *data)
>  {
> -    if (node_matches(fdt, node, "memory"))
> +    if (device_tree_node_matches(fdt, node, "memory"))
>          process_memory_node(fdt, node, name, address_cells, size_cells);
> 
>      return 0;
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index b91b39f..510b5b4 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -12,6 +12,8 @@
> 
>  #include <xen/types.h>
> 
> +#define DEVICE_TREE_MAX_DEPTH 16
> +
>  #define NR_MEM_BANKS 8
> 
>  struct membank {
> @@ -39,6 +41,12 @@ extern void *device_tree_flattened;
>  size_t device_tree_early_init(const void *fdt);
>  paddr_t device_tree_get_xen_paddr(void);
> 
> +void device_tree_get_reg(const u32 **cell, u32 address_cells, u32 size_cells,
> +                         u64 *start, u64 *size);
> +void device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
> +                         u64 start, u64 size);
> +u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name);
> +bool_t device_tree_node_matches(const void *fdt, int node, const char *match);
>  int device_tree_for_each_node(const void *fdt,
>                                device_tree_node_func func, void *data);
>  void device_tree_dump(const void *fdt);
> --
> 1.7.2.5
> 

Re: [PATCH 6/8] device tree, arm: supply a flat device tree to dom0

[David Vrabel]

On 14/03/12 10:44, Ian Campbell wrote:
> On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Build a flat device tree for dom0 based on the one supplied to Xen.
>> The following changes are made:
>>
>>   * In the /chosen node, the xen,dom0-bootargs parameter is renamed to
>>     bootargs.
>>
>>   * In all memory nodes, the reg parameters are adjusted to reflect
>>     the amount of memory dom0 can use.  The p2m is updated using this
>>     info.
> 
> Do you also do
>     * Hide the xen console UART from dom0
> 
> ? (it seems not)

No.  I don't know which UART Xen uses yet as it isn't discovered via the
device tree.


>> Support for passing ATAGS to dom0 is removed.
> 
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>  xen/arch/arm/Makefile         |    2 +
>>  xen/arch/arm/domain_build.c   |  238 ++++++++++++++++++++++++++++++++++-------
>>  xen/arch/arm/kernel.c         |    4 +-
>>  xen/arch/arm/kernel.h         |    8 +-
>>  xen/common/device_tree.c      |   44 +++++---
>>  xen/include/xen/device_tree.h |    8 ++
>>  6 files changed, 246 insertions(+), 58 deletions(-)
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index da5096a..4c9517c 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -25,6 +25,8 @@ obj-y += vtimer.o
>>
>>  #obj-bin-y += ....o
>>
>> +CFLAGS += -I../../common/libfdt
> 
> This suggests that something ought to be moved to xen/include/xen.

libfdt was imported using the original directory structure.  Would you
prefer header files to be moved about and #include's in
common/libfdt/*.c fixed up?

>> +
>>  ifdef CONFIG_DTB_FILE
>>  obj-y += dtb.o
>>  AFLAGS += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 9240209..ca8c706 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -6,6 +6,11 @@
>>  #include <xen/sched.h>
>>  #include <asm/irq.h>
>>  #include <asm/regs.h>
>> +#include <xen/errno.h>
>> +#include <xen/device_tree.h>
>> +#include <xen/guest_access.h>
>> +
>> +#include <libfdt.h>
>>
>>  #include "gic.h"
>>  #include "kernel.h"
>> @@ -27,43 +32,190 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
>>      return alloc_vcpu(dom0, 0, 0);
>>  }
>>
>> -extern void guest_mode_entry(void);
>> +static void set_memory_reg(struct domain *d, struct kernel_info *dom0,
>> +                           const void *fdt,
>> +                           const u32 *cell, int address_cells, int size_cells,
>> +                           u32 *new_cell, int *len)
>> +{
>> +    int reg_size = (address_cells + size_cells) * sizeof(*cell);
>> +    int l;
>> +    u64 start;
>> +    u64 size;
>> +
>> +    l = *len;
>> +
>> +    while (dom0->unassigned_mem > 0 && l > reg_size
>> +           && dom0->mem.nr_banks < NR_MEM_BANKS)
>> +    {
>> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>> +        if (size > dom0->unassigned_mem)
>> +            size = dom0->unassigned_mem;
> 
> I somehow can't find any struct members named unassigned_mem in struct
> domain (or in my whole tree). I'm obviously missing something...
> 
> ... which is that there is a local "struct kernel_info *dom0" shadowing
> the global "struct domain *dom0". I think another name for the local var
> would be useful ;-)

Oops.  I'll fix up the name.

>> +
>> +        device_tree_set_reg(&new_cell, address_cells, size_cells, start, size);
>> +
>> +        printk("Populate P2M %#llx->%#llx\n", start, start + size);
>> +        p2m_populate_ram(d, start, start + size);
>> +        dom0->mem.bank[dom0->mem.nr_banks].start = start;
>> +        dom0->mem.bank[dom0->mem.nr_banks].size = size;
>> +        dom0->unassigned_mem -= size;
>> +
>> +        l -= reg_size;
>> +    }
>> +
>> +    *len -= l;
>> +}
>> +
>> +static int write_properties(struct domain *d, struct kernel_info *dom0,
>> +                            const void *fdt,
>> +                            int node, const char *name, int depth,
>> +                            u32 address_cells, u32 size_cells)
>> +{
>> +    int prop;
>> +
>> +    for (prop = fdt_first_property_offset(fdt, node);
>> +         prop >= 0;
>> +         prop = fdt_next_property_offset(fdt, prop))
>> +    {
>> +        const struct fdt_property *p;
>> +        const char *prop_name;
>> +        const char *prop_data;
>> +        int prop_len;
>> +        char *new_data = NULL;
>> +
>> +        p = fdt_get_property_by_offset(fdt, prop, NULL);
>> +        prop_name = fdt_string(fdt, fdt32_to_cpu(p->nameoff));
>> +        prop_data = p->data;
>> +        prop_len  = fdt32_to_cpu(p->len);
>> +
>> +        /*
>> +         * In chosen node: replace bootargs with value from
>> +         * xen,dom0-bootargs.
>> +         */
>> +        if (device_tree_node_matches(fdt, node, "chosen")) {
>> +            if (strcmp(prop_name, "bootargs") == 0)
>> +                continue;
>> +            if (strcmp(prop_name, "xen,dom0-bootargs") == 0)
>> +                prop_name = "bootargs";
>> +        }
>> +
>> +        /*
>> +         * In a memory node: adjust reg property.
>> +         */
>> +        if (device_tree_node_matches(fdt, node, "memory")) {
>> +            if (strcmp(prop_name, "reg") == 0) {
>> +                new_data = xzalloc_bytes(prop_len);
>> +                if (new_data  == NULL)
>> +                    return -ENOMEM;
>> +
>> +                set_memory_reg(d, dom0, fdt,
>> +                               (u32 *)prop_data, address_cells, size_cells,
>> +                               (u32 *)new_data, &prop_len);
>> +                prop_data = new_data;
> 
> Need to free the previous value of prop_data?

No. prop_data points into the FDT itself.

>> +            }
>> +        }
>> +
>> +        /*
>> +         * TODO: Should call map_mmio_regions() for all devices in the
>> +         * tree that have a "reg" parameter (except cpus).  This
>> +         * requires that adjacent regions are coalesced and rounded up
>> +         * to whole pages.
> 
> Does map_mmio_regions currently require you to coalesce? If it was smart
> enough to use larger mappings for contiguous areas (which it isn't, but
> probably should?) then I suppose coalescing would be useful.

I had a look after I wrote that comment and coalescing is not required.
Still need to round to whole pages though.

>> +         */
>> +
>> +        fdt_property(dom0->fdt, prop_name, prop_data, prop_len);
>> +
>> +        xfree(new_data);
> 
> Does fdt_property duplicate prop_data? (answer: yes)

Yes.

>> +    }
>> +
>> +    if (prop == -FDT_ERR_NOTFOUND)
>> +        return 0;
>> +    return prop;
>> +}
>> +
>> +static int write_nodes(struct domain *d, struct kernel_info *dom0,
>> +                       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];
> 
> In a previous patch you had a local MAX_DEPTH. Shouldn't this be used
> instead.
> 
> (comes back later: oh, I see it is. good ;-))
> 
>> +    int ret;
>> +
>> +    for (node = 0, depth = 0;
>> +         node >= 0 && depth >= 0;
>> +         node = fdt_next_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\n", name);
>> +            continue;
>> +        }
>> +
>> +        while (last_depth-- >= depth)
>> +            fdt_end_node(dom0->fdt);
>> +
>> +        address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells");
>> +        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
>> +
>> +        fdt_begin_node(dom0->fdt, name);
>>
>> -static void setup_linux_atag(paddr_t tags, paddr_t ram_s, paddr_t ram_e)
>> +        ret = write_properties(d, dom0, fdt, node, name, depth,
>> +                               address_cells[depth-1], size_cells[depth-1]);
>> +        if (ret < 0)
>> +            return ret;
>> +
>> +        last_depth = depth;
>> +    }
>> +
>> +    while (last_depth-- >= 0)
>> +        fdt_end_node(dom0->fdt);
>> +
>> +    return 0;
>> +}
>> +
>> +static int prepare_dtb(struct domain *d, struct kernel_info *dom0)
>>  {
>> -    paddr_t ma = gvirt_to_maddr(tags);
>> -    void *map = map_domain_page(ma>>PAGE_SHIFT);
>> -    void *p = map + (tags & (PAGE_SIZE - 1));
>> -    char cmdline[] = "earlyprintk=xenboot console=ttyAMA1 root=/dev/mmcblk0 debug rw";
>> +    void *fdt;
>> +    int new_size;
>> +    int ret;
>>
>> -    /* not enough room on this page for all the tags */
>> -    BUG_ON(PAGE_SIZE - (tags & (PAGE_SIZE - 1)) < 8 * sizeof(uint32_t));
>> +    fdt = device_tree_flattened;
>>
>> -#define TAG(type, val) *(type*)p = val; p+= sizeof(type)
>> +    new_size = fdt_totalsize(fdt) + 8192;
> 
> 2*PAGE_SIZE? Or is this number magic in some other way?

It's arbitrary.  I meant to have a DOM0_FDT_EXTRA_SIZE #define.

>> +    dom0->fdt = xmalloc_bytes(new_size);
>> +    if (dom0->fdt == NULL)
>> +        return -ENOMEM;
>>
>> -    /* ATAG_CORE */
>> -    TAG(uint32_t, 2);
>> -    TAG(uint32_t, 0x54410001);
>> +    ret = fdt_create(dom0->fdt, new_size);
>> +    if (ret < 0)
>> +        goto err;
>>
>> -    /* ATAG_MEM */
>> -    TAG(uint32_t, 4);
>> -    TAG(uint32_t, 0x54410002);
>> -    TAG(uint32_t, (ram_e - ram_s) & 0xFFFFFFFF);
>> -    TAG(uint32_t, ram_s & 0xFFFFFFFF);
>> +    fdt_finish_reservemap(dom0->fdt);
>>
>> -    /* ATAG_CMDLINE */
>> -    TAG(uint32_t, 2 + ((strlen(cmdline) + 4) >> 2));
>> -    TAG(uint32_t, 0x54410009);
>> -    memcpy(p, cmdline, strlen(cmdline) + 1);
>> -    p += ((strlen(cmdline) + 4) >> 2) << 2;
>> +    ret = write_nodes(d, dom0, fdt);
>> +    if (ret < 0)
>> +        goto err;
>>
>> -    /* ATAG_NONE */
>> -    TAG(uint32_t, 0);
>> -    TAG(uint32_t, 0);
>> +    fdt_finish(dom0->fdt);
>>
>> -#undef TAG
>> +    device_tree_dump(dom0->fdt);
>> +
>> +    dom0->fdt = dom0->fdt;
>> +    return 0;
>> +
>> +  err:
>> +    xfree(dom0->fdt);
>> +    return ret;
>> +}
>> +
>> +static void dtb_load(struct kernel_info *dom0)
>> +{
>> +    void *dtb_virt = (void *)(u32)dom0->dtb_paddr;
> 
> This is part of the 1:1 map? Shuldn't this be either a variant of __va
> or some sort of explicit map create function?
>>
>> -    unmap_domain_page(map);
>> +    raw_copy_to_guest(dtb_virt, dom0->fdt, fdt_totalsize(dom0->fdt));
> 
> Oh, so dtb_virt is a _dom0_ virtual address, which because we are
> building the domain with the stage 1 MMU disabled == the phys addr.
> Might be worthy of a comment? Our use of __user for such functions seems
> a little inconsistent or I would suggest
>         void * __user dtb_virt = ...
> (maybe that's a good idea anyway?)

Ok.

> Do we arrange somewhere that dtb_paddr < 4G ? I suppose we must...

We require that the first memory bank for dom0 is below 4G and dtb_paddr
is placed at the beginning of this bank.

>> +    xfree(dom0->fdt);
>>  }
>>
>>  int construct_dom0(struct domain *d)
>> @@ -81,22 +233,27 @@ int construct_dom0(struct domain *d)
>>
>>      printk("*** LOADING DOMAIN 0 ***\n");
>>
>> -    /* 128M at 2G physical */
>> -    /* TODO size and location from DT. */
>> -    kinfo.ram_start = 0x80000000;
>> -    kinfo.ram_end   = 0x88000000;
>> +    d->max_pages = ~0U;
>>
>> -    rc = kernel_prepare(&kinfo);
>> -    if (rc < 0)
>> +    if ( (rc = p2m_alloc_table(d)) != 0 )
>>          return rc;
>>
>> -    d->max_pages = ~0U;
>> +    kinfo.unassigned_mem = 0x08000000; /* XXX */
>>
>> -    if ( (rc = p2m_alloc_table(d)) != 0 )
>> +    rc = prepare_dtb(d, &kinfo);
>> +    if (rc < 0)
>>          return rc;
>>
>> -    printk("Populate P2M %#llx->%#llx\n", kinfo.ram_start, kinfo.ram_end);
>> -    p2m_populate_ram(d, kinfo.ram_start, kinfo.ram_end);
>> +    /* First RAM bank must be below 4 GiB or a 32-bit dom0 kernel
>> +       cannot be loaded. */
> 
> In principal we could relocate a bank to a hole under 4G but I expect
> that all machines will meet this constraint, or they'd have trouble
> booting native.
> 
> BTW, does this DTB stuff mean that we no longer use the 40 bit physical
> address alias to access RAM form the hypervisor? Not a big problem, we
> just did that to help shake out 32 bitness assumptions.

Yes.  There no way to specify two memory regions are aliases.  The
device tree we're using does report the extra memory as an additional
bank but since we don't support discontiguous memory Xen can't use it.

>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>> index dd757e5..b27b20d 100644
>> --- a/xen/arch/arm/kernel.c
>> +++ b/xen/arch/arm/kernel.c
>> @@ -121,7 +121,7 @@ static int kernel_try_zimage_prepare(struct kernel_info *info)
>>       * at 32k from start of RAM.
>>       */
>>      if (start == 0)
>> -        info->zimage.load_addr = info->ram_start + 0x8000;
>> +        info->zimage.load_addr = info->mem.bank[0].start + 0x8000;
>>      else
>>          info->zimage.load_addr = start;
>>      info->zimage.len = end - start;
>> @@ -176,6 +176,8 @@ int kernel_prepare(struct kernel_info *info)
>>  {
>>      int rc;
>>
>> +    info->dtb_paddr = info->mem.bank[0].start + 0x100;
> 
> assert(info->dtb_paddr < 0x100000000ULL); ???
> 
> I suppose it migth also be useful to check dtb_paddr + dtb_len < 4G too?

Ok.

David

Re: [PATCH 6/8] device tree, arm: supply a flat device tree to dom0

[Ian Campbell]

On Wed, 2012-03-14 at 11:17 +0000, David Vrabel wrote:
> On 14/03/12 10:44, Ian Campbell wrote:
> > On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
> >> From: David Vrabel <david.vrabel@citrix.com>
> >>
> >> Build a flat device tree for dom0 based on the one supplied to Xen.
> >> The following changes are made:
> >>
> >>   * In the /chosen node, the xen,dom0-bootargs parameter is renamed to
> >>     bootargs.
> >>
> >>   * In all memory nodes, the reg parameters are adjusted to reflect
> >>     the amount of memory dom0 can use.  The p2m is updated using this
> >>     info.
> >
> > Do you also do
> >     * Hide the xen console UART from dom0
> >
> > ? (it seems not)
> 
> No.  I don't know which UART Xen uses yet as it isn't discovered via the
> device tree.

Fair enough.

> 
> 
> >> Support for passing ATAGS to dom0 is removed.
> >
> >>
> >> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> >> ---
> >>  xen/arch/arm/Makefile         |    2 +
> >>  xen/arch/arm/domain_build.c   |  238 ++++++++++++++++++++++++++++++++++-------
> >>  xen/arch/arm/kernel.c         |    4 +-
> >>  xen/arch/arm/kernel.h         |    8 +-
> >>  xen/common/device_tree.c      |   44 +++++---
> >>  xen/include/xen/device_tree.h |    8 ++
> >>  6 files changed, 246 insertions(+), 58 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> >> index da5096a..4c9517c 100644
> >> --- a/xen/arch/arm/Makefile
> >> +++ b/xen/arch/arm/Makefile
> >> @@ -25,6 +25,8 @@ obj-y += vtimer.o
> >>
> >>  #obj-bin-y += ....o
> >>
> >> +CFLAGS += -I../../common/libfdt
> >
> > This suggests that something ought to be moved to xen/include/xen.
> 
> libfdt was imported using the original directory structure.  Would you
> prefer header files to be moved about and #include's in
> common/libfdt/*.c fixed up?

Hrm. I see these (multiple times each):
        #include <fdt.h>
        #include "libfdt_env.h"
        #include <libfdt.h>
        #include "libfdt_internal.h"

Presumably only fdt.h and libfdt.h would need to move? Or is the
"libfdt_env.h" actually an error since it is included in libfdt.h?

That would still need fixing up to add the xen/ prefix, or we could put
them in include/libfdt/....h and add that to the CFLAGS. Neither of
which seems any better than the change you made.

Anyone else have any ideas/opinions? I'm inclined to stick with the
CFLAGS change, although I think you should use $(BASEDIR) rather
than ../..

(snipped rest, all seemed reasonable)

[PATCH 7/8] arm: use bootargs for the command line

[David Vrabel]

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

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

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

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 4c0244c..01ead73 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -164,6 +164,8 @@ void __init start_xen(unsigned long boot_phys_offset,
         + (atag_paddr & ((1 << SECOND_SHIFT) - 1));
     fdt_size = device_tree_early_init(fdt);
 
+    cmdline_parse(device_tree_bootargs(fdt));
+
     setup_pagetables(boot_phys_offset);
 
 #ifdef EARLY_UART_ADDRESS
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 2422fba..512297f 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -117,6 +117,26 @@ int device_tree_for_each_node(const void *fdt,
     return 0;
 }
 
+/**
+ * device_tree_bootargs - return the bootargs (the Xen command line)
+ * @fdt flat device tree.
+ */
+const char *device_tree_bootargs(const void *fdt)
+{
+    int node; 
+    const struct fdt_property *prop;
+
+    node = fdt_path_offset(fdt, "/chosen");
+    if (node < 0)
+        return NULL;
+
+    prop = fdt_get_property(fdt, node, "bootargs", NULL);
+    if (prop == NULL)
+        return NULL;
+
+    return prop->data;
+}
+
 static int dump_node(const void *fdt, int node, const char *name, int depth,
                      u32 address_cells, u32 size_cells, void *data)
 {
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 88984d2..145b0b6 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -46,7 +46,7 @@ static void __init assign_integer_param(
     }
 }
 
-void __init cmdline_parse(char *cmdline)
+void __init cmdline_parse(const char *cmdline)
 {
     char opt[100], *optval, *optkey, *q;
     const char *p = cmdline;
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 510b5b4..8e1626c 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -49,6 +49,7 @@ u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name);
 bool_t device_tree_node_matches(const void *fdt, int node, const char *match);
 int device_tree_for_each_node(const void *fdt,
                               device_tree_node_func func, void *data);
+const char *device_tree_bootargs(const void *fdt);
 void device_tree_dump(const void *fdt);
 
 #endif
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index d6f9182..36b4c7f 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -56,7 +56,7 @@ do {                                                            \
 
 struct domain;
 
-void cmdline_parse(char *cmdline);
+void cmdline_parse(const char *cmdline);
 int parse_bool(const char *s);
 
 /*#define DEBUG_TRACE_DUMP*/
-- 
1.7.2.5

Re: [PATCH 7/8] arm: use bootargs for the command line

[Ian Campbell]

On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Use the /chosen node's bootargs parameter for the Xen command line.
> Parse it early on before the serial console is setup.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Looks good to me. Did you build/test the cmdline_parse change on x86 (or
non-ARM) too?

Assuming that changes stands alone it might be useful to split it out
and submit it to Keir separately.

> ---
>  xen/arch/arm/setup.c          |    2 ++
>  xen/common/device_tree.c      |   20 ++++++++++++++++++++
>  xen/common/kernel.c           |    2 +-
>  xen/include/xen/device_tree.h |    1 +
>  xen/include/xen/lib.h         |    2 +-
>  5 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 4c0244c..01ead73 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -164,6 +164,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>          + (atag_paddr & ((1 << SECOND_SHIFT) - 1));
>      fdt_size = device_tree_early_init(fdt);
>  
> +    cmdline_parse(device_tree_bootargs(fdt));
> +
>      setup_pagetables(boot_phys_offset);
>  
>  #ifdef EARLY_UART_ADDRESS
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 2422fba..512297f 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -117,6 +117,26 @@ int device_tree_for_each_node(const void *fdt,
>      return 0;
>  }
>  
> +/**
> + * device_tree_bootargs - return the bootargs (the Xen command line)
> + * @fdt flat device tree.
> + */
> +const char *device_tree_bootargs(const void *fdt)
> +{
> +    int node; 
> +    const struct fdt_property *prop;
> +
> +    node = fdt_path_offset(fdt, "/chosen");
> +    if (node < 0)
> +        return NULL;
> +
> +    prop = fdt_get_property(fdt, node, "bootargs", NULL);
> +    if (prop == NULL)
> +        return NULL;
> +
> +    return prop->data;
> +}
> +
>  static int dump_node(const void *fdt, int node, const char *name, int depth,
>                       u32 address_cells, u32 size_cells, void *data)
>  {
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index 88984d2..145b0b6 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -46,7 +46,7 @@ static void __init assign_integer_param(
>      }
>  }
>  
> -void __init cmdline_parse(char *cmdline)
> +void __init cmdline_parse(const char *cmdline)
>  {
>      char opt[100], *optval, *optkey, *q;
>      const char *p = cmdline;
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 510b5b4..8e1626c 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -49,6 +49,7 @@ u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name);
>  bool_t device_tree_node_matches(const void *fdt, int node, const char *match);
>  int device_tree_for_each_node(const void *fdt,
>                                device_tree_node_func func, void *data);
> +const char *device_tree_bootargs(const void *fdt);
>  void device_tree_dump(const void *fdt);
>  
>  #endif
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index d6f9182..36b4c7f 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -56,7 +56,7 @@ do {                                                            \
>  
>  struct domain;
>  
> -void cmdline_parse(char *cmdline);
> +void cmdline_parse(const char *cmdline);
>  int parse_bool(const char *s);
>  
>  /*#define DEBUG_TRACE_DUMP*/

Re: [PATCH 7/8] arm: use bootargs for the command line

[David Vrabel]

On 14/03/12 10:46, Ian Campbell wrote:
> On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Use the /chosen node's bootargs parameter for the Xen command line.
>> Parse it early on before the serial console is setup.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> 
> Looks good to me. Did you build/test the cmdline_parse change on x86 (or
> non-ARM) too?

I built it for x86, yes.  I also looked quite carefully at
cmdline_parse() -- it seems the missing const is an accidental ommision.

> Assuming that changes stands alone it might be useful to split it out
> and submit it to Keir separately.

Ok.

David

[PATCH 8/8] arm: add dom0_mem command line argument

[David Vrabel]

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

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

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

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index ca8c706..26f1104 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -18,6 +18,17 @@
 static unsigned int __initdata opt_dom0_max_vcpus;
 integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
 
+#define DOM0_MEM_DEFAULT 0x8000000 /* 128 MiB */
+static u64 __initdata dom0_mem = DOM0_MEM_DEFAULT;
+
+static void __init parse_dom0_mem(const char *s)
+{
+    dom0_mem = parse_size_and_unit(s, &s);
+    if (dom0_mem == 0)
+        dom0_mem = DOM0_MEM_DEFAULT;
+}
+custom_param("dom0_mem", parse_dom0_mem);
+
 struct vcpu *__init alloc_dom0_vcpu0(void)
 {
     dom0->vcpu = xmalloc_array(struct vcpu *, opt_dom0_max_vcpus);
@@ -181,6 +192,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info *dom0)
     int new_size;
     int ret;
 
+    dom0->unassigned_mem = dom0_mem;
+
     fdt = device_tree_flattened;
 
     new_size = fdt_totalsize(fdt) + 8192;
@@ -238,8 +251,6 @@ int construct_dom0(struct domain *d)
     if ( (rc = p2m_alloc_table(d)) != 0 )
         return rc;
 
-    kinfo.unassigned_mem = 0x08000000; /* XXX */
-
     rc = prepare_dtb(d, &kinfo);
     if (rc < 0)
         return rc;
-- 
1.7.2.5

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

[Ian Campbell]

On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Add a simple dom0_mem command line argument.  It's not as flexible as
> the x86 equivalent (the 'max' and 'min' prefixes are not supported).

I presume we'd want to support them in the future. Looks good as it
stands though.

> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  xen/arch/arm/domain_build.c |   15 +++++++++++++--
>  1 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index ca8c706..26f1104 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -18,6 +18,17 @@
>  static unsigned int __initdata opt_dom0_max_vcpus;
>  integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
>  
> +#define DOM0_MEM_DEFAULT 0x8000000 /* 128 MiB */
> +static u64 __initdata dom0_mem = DOM0_MEM_DEFAULT;
> +
> +static void __init parse_dom0_mem(const char *s)
> +{
> +    dom0_mem = parse_size_and_unit(s, &s);
> +    if (dom0_mem == 0)
> +        dom0_mem = DOM0_MEM_DEFAULT;
> +}
> +custom_param("dom0_mem", parse_dom0_mem);
> +
>  struct vcpu *__init alloc_dom0_vcpu0(void)
>  {
>      dom0->vcpu = xmalloc_array(struct vcpu *, opt_dom0_max_vcpus);
> @@ -181,6 +192,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info *dom0)
>      int new_size;
>      int ret;
>  
> +    dom0->unassigned_mem = dom0_mem;
> +
>      fdt = device_tree_flattened;
>  
>      new_size = fdt_totalsize(fdt) + 8192;
> @@ -238,8 +251,6 @@ int construct_dom0(struct domain *d)
>      if ( (rc = p2m_alloc_table(d)) != 0 )
>          return rc;
>  
> -    kinfo.unassigned_mem = 0x08000000; /* XXX */
> -
>      rc = prepare_dtb(d, &kinfo);
>      if (rc < 0)
>          return rc;

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

[David Vrabel]

On 14/03/12 10:48, Ian Campbell wrote:
> On Tue, 2012-02-28 at 16:54 +0000, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Add a simple dom0_mem command line argument.  It's not as flexible as
>> the x86 equivalent (the 'max' and 'min' prefixes are not supported).
> 
> I presume we'd want to support them in the future. Looks good as it
> stands though.

Yes.  But I think this should be done at as part of a wider look at at
moving some of the command line options to common code.

David