[PATCH V1 00/29] Allow Xen to boot with a raw Device Tree

[PATCH V1 00/29] Allow Xen to boot with a raw Device Tree

[Julien Grall]

Hi,

This is the second version of this patch series. It allow Xen to boot with
a raw device tree, ie without any specific modification for Xen.

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

Major changes since v1:
    - Add multiple commandlines patch from Andre and rebase it (mostly
      use the new helpers): http://patches.linaro.org/19356/
    - Remove and add fake node for cpus, gic and timer

All the changes can be found in each patch.

TODO list:
    - Add support for memreserve. This is needed for HDLCD framebuffer on the
    versatile express.

Cheers,

Andre Przywara (1):
  ARM: parse separate DT properties for different commandlines

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

 docs/misc/arm/device-tree/booting.txt |   28 +-
 xen/arch/arm/device.c                 |    3 +
 xen/arch/arm/domain_build.c           |  582 ++++++++++++++++++++++++---------
 xen/arch/arm/platform.c               |   10 +
 xen/arch/arm/platforms/exynos5.c      |   11 +
 xen/arch/arm/platforms/vexpress.c     |   17 +
 xen/arch/arm/setup.c                  |    6 +-
 xen/arch/arm/time.c                   |   10 +-
 xen/arch/arm/traps.c                  |    5 -
 xen/common/device_tree.c              |  281 +++++++++-------
 xen/common/string.c                   |   15 +
 xen/drivers/char/dt-uart.c            |   16 +-
 xen/drivers/video/arm_hdlcd.c         |   69 ++--
 xen/include/asm-arm/platform.h        |    7 +
 xen/include/asm-arm/psci.h            |    5 +
 xen/include/xen/device_tree.h         |  172 ++++++++--
 xen/include/xen/init.h                |    1 +
 xen/include/xen/string.h              |    3 +
 18 files changed, 894 insertions(+), 347 deletions(-)

-- 
1.7.10.4

[PATCH V1 01/29] xen/char: dt-uart: Allow the user to give a path to the node

[Julien Grall]

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

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

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

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

Re: [PATCH V1 01/29] xen/char: dt-uart: Allow the user to give a path to the node

[Ian Campbell]

~/On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> On some board, there is no alias to the UART. To avoid modification in
> the device tree, dt-uart should also search device by path.
> 
> To distinguish an alias from a path, dt-uart will check the first character.
> If it's a / then it's path, otherwise it's an alias.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Acked-by: Ian Cambell <ian.campbell@citrix.com>

Fixed the spelling of my name and applied.

(nb, it was wrong in at least one subsequent patch I looked at too)

Re: [PATCH V1 01/29] xen/char: dt-uart: Allow the user to give a path to the node

[Julien Grall]

On 09/06/2013 02:08 PM, Ian Campbell wrote:
> ~/On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> On some board, there is no alias to the UART. To avoid modification in
>> the device tree, dt-uart should also search device by path.
>>
>> To distinguish an alias from a path, dt-uart will check the first character.
>> If it's a / then it's path, otherwise it's an alias.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Acked-by: Ian Cambell <ian.campbell@citrix.com>
>
> Fixed the spelling of my name and applied.
>
> (nb, it was wrong in at least one subsequent patch I looked at too)
>

Sorry, I will check the other patches.

-- 
Julien Grall

[PATCH V1 02/29] xen: Introduce __initconst to store initial const data

[Julien Grall]

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

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

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

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Cambell <ian.campbell@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Keir Fraser <keir@xen.org>
---
 xen/include/xen/init.h |    1 +
 1 file changed, 1 insertion(+)

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

Re: [PATCH V1 02/29] xen: Introduce __initconst to store initial const data

[Ian Campbell]

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

Jan said he "didn't mind" this change, so although I've not taken that
as an Ack I have applied this patch.

> CC: Keir Fraser <keir@xen.org>
> ---
>  xen/include/xen/init.h |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
> index b602577..9d481b3 100644
> --- a/xen/include/xen/init.h
> +++ b/xen/include/xen/init.h
> @@ -10,6 +10,7 @@
>  #define __init            __text_section(".init.text")
>  #define __exit            __text_section(".exit.text")
>  #define __initdata        __section(".init.data")
> +#define __initconst       __section(".init.rodata")
>  #define __exitdata        __used_section(".exit.data")
>  #define __initsetup       __used_section(".init.setup")
>  #define __init_call(lvl)  __used_section(".initcall" lvl ".init")

[PATCH V1 03/29] xen/dts: Don't check the number of address and size cells in process_cpu_node

[Julien Grall]

CPU nodes are not required to have #address-cells == 1 and #size-cells == 0, so
don't check for that (see Linux Documentation/devicetree/booting-without-of.txt
Section III.5.a).

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

Signed-off-by: Julien Grall <julien.grall@linaro.org>
CC: andrii.anisov@globallogic.com
CC: baozich@gmail.com

---
    Changes in v2:
        - Update commit message
---
 xen/common/device_tree.c |    6 ------
 1 file changed, 6 deletions(-)

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

Re: [PATCH V1 03/29] xen/dts: Don't check the number of address and size cells in process_cpu_node

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> CPU nodes are not required to have #address-cells == 1 and #size-cells == 0, so
> don't check for that (see Linux Documentation/devicetree/booting-without-of.txt
> Section III.5.a).
> 
> In some OMAP5 device, tree, these 2 properties are not correctly set. Therefore,
> Xen will only able to handle 1 CPU.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

> CC: andrii.anisov@globallogic.com
> CC: baozich@gmail.com
> 
> ---
>     Changes in v2:
>         - Update commit message
> ---
>  xen/common/device_tree.c |    6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 3a3c99c..c4f0f2c 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -414,12 +414,6 @@ static void __init process_cpu_node(const void *fdt, int node,
>      const u32 *cell;
>      paddr_t start, size;
>  
> -    if ( address_cells != 1 || size_cells != 0 )
> -    {
> -        early_printk("fdt: node `%s': invalid #address-cells or #size-cells",
> -                     name);
> -        return;
> -    }
>  
>      prop = fdt_get_property(fdt, node, "reg", NULL);
>      if ( !prop )

Re: [PATCH V1 03/29] xen/dts: Don't check the number of address and size cells in process_cpu_node

[Ian Campbell]

On Fri, 2013-09-06 at 17:24 +0100, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> > CPU nodes are not required to have #address-cells == 1 and #size-cells == 0, so
> > don't check for that (see Linux Documentation/devicetree/booting-without-of.txt
> > Section III.5.a).
> > 
> > In some OMAP5 device, tree, these 2 properties are not correctly set. Therefore,
> > Xen will only able to handle 1 CPU.
> > 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> > CC: andrii.anisov@globallogic.com
> > CC: baozich@gmail.com

I missed Chen's ack from
<CCE916EB-FC49-41D4-B672-9BD0D021222C@gmail.com> when I applied this,
please remember to collect them into reposts.

Re: [PATCH V1 03/29] xen/dts: Don't check the number of address and size cells in process_cpu_node

[Julien Grall]

On 09/10/2013 11:52 AM, Ian Campbell wrote:
> On Fri, 2013-09-06 at 17:24 +0100, Ian Campbell wrote:
>> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>>> CPU nodes are not required to have #address-cells == 1 and #size-cells == 0, so
>>> don't check for that (see Linux Documentation/devicetree/booting-without-of.txt
>>> Section III.5.a).
>>>
>>> In some OMAP5 device, tree, these 2 properties are not correctly set. Therefore,
>>> Xen will only able to handle 1 CPU.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>
>>> CC: andrii.anisov@globallogic.com
>>> CC: baozich@gmail.com
> 
> I missed Chen's ack from
> <CCE916EB-FC49-41D4-B672-9BD0D021222C@gmail.com> when I applied this,
> please remember to collect them into reposts.
> 
> 

Sorry, as I modified the patch (here the commit message), I though Chen
have to re-ack it.

-- 
Julien Grall

Re: [PATCH V1 03/29] xen/dts: Don't check the number of address and size cells in process_cpu_node

[Ian Campbell]

On Tue, 2013-09-10 at 11:54 +0100, Julien Grall wrote:
> On 09/10/2013 11:52 AM, Ian Campbell wrote:
> > On Fri, 2013-09-06 at 17:24 +0100, Ian Campbell wrote:
> >> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> >>> CPU nodes are not required to have #address-cells == 1 and #size-cells == 0, so
> >>> don't check for that (see Linux Documentation/devicetree/booting-without-of.txt
> >>> Section III.5.a).
> >>>
> >>> In some OMAP5 device, tree, these 2 properties are not correctly set. Therefore,
> >>> Xen will only able to handle 1 CPU.
> >>>
> >>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>
> >> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >>
> >>> CC: andrii.anisov@globallogic.com
> >>> CC: baozich@gmail.com
> > 
> > I missed Chen's ack from
> > <CCE916EB-FC49-41D4-B672-9BD0D021222C@gmail.com> when I applied this,
> > please remember to collect them into reposts.
> > 
> > 
> 
> Sorry, as I modified the patch (here the commit message), I though Chen
> have to re-ack it.

It depends, minor stuff like that doesn't invalidate an Ack IMHO. But
you are right, it's a judgement call.

Ian.

[PATCH V1 04/29] xen/dts: Constify device_tree_flattened

[Julien Grall]

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

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

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

Re: [PATCH V1 04/29] xen/dts: Constify device_tree_flattened

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> The Flat Device Tree is given by the bootloader. Xen doesn't need to modify it.

On arm64 this gives:

        setup.c: In function 'setup_mm':
        setup.c:475:5: error: passing argument 1 of 'copy_from_paddr' discards 'const' qualifier from pointer target type [-Werror]
        In file included from setup.c:40:0:
        /local/scratch/ianc/devel/committer.git/xen/include/asm/setup.h:8:6: note: expected 'void *' but argument is of type 'const void *'
        cc1: all warnings being treated as errors
        make[4]: *** [setup.o] Error 1
        make[4]: *** Waiting for unfinished jobs....

[PATCH V1 05/29] xen/arm: Move __PSCI* from traps.c to the header

[Julien Grall]

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

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

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

[PATCH V1 06/29] xen: Add new string function

[Julien Grall]

Add strcasecmp. The code is copied from Linux.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Remove kbasename
---
 xen/common/string.c      |   15 +++++++++++++++
 xen/include/xen/string.h |    3 +++
 2 files changed, 18 insertions(+)

diff --git a/xen/common/string.c b/xen/common/string.c
index db9d9d5..9a5a4ba 100644
--- a/xen/common/string.c
+++ b/xen/common/string.c
@@ -41,6 +41,21 @@ int strnicmp(const char *s1, const char *s2, size_t len)
 }
 #endif
 
+#ifndef __HAVE_ARCH_STRCASECMP
+int strcasecmp(const char *s1, const char *s2)
+{
+    int c1, c2;
+
+    do
+    {
+        c1 = tolower(*s1++);
+        c2 = tolower(*s2++);
+    } while ( c1 == c2 && c1 != 0 );
+
+    return c1 - c2;
+}
+#endif
+
 #ifndef __HAVE_ARCH_STRLCPY
 /**
  * strlcpy - Copy a %NUL terminated string into a sized buffer
diff --git a/xen/include/xen/string.h b/xen/include/xen/string.h
index f26b994..f35934e 100644
--- a/xen/include/xen/string.h
+++ b/xen/include/xen/string.h
@@ -43,6 +43,9 @@ extern int strncmp(const char *,const char *,__kernel_size_t);
 #ifndef __HAVE_ARCH_STRNICMP
 extern int strnicmp(const char *, const char *, __kernel_size_t);
 #endif
+#ifndef __HAVE_ARCH_STRCASECMP
+extern int strcasecmp(const char *, const char *);
+#endif
 #ifndef __HAVE_ARCH_STRCHR
 extern char * strchr(const char *,int);
 #endif
-- 
1.7.10.4

Re: [PATCH V1 06/29] xen: Add new string function

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> Add strcasecmp. The code is copied from Linux.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

Does Linux have interesting optimised versions for arm or x86? Not that
this is going to be used on any hotpaths though I guess.

Re: [PATCH V1 06/29] xen: Add new string function

[Julien Grall]

On 09/06/2013 05:26 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> Add strcasecmp. The code is copied from Linux.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Does Linux have interesting optimised versions for arm or x86? Not that
> this is going to be used on any hotpaths though I guess.

I didn't find specific implementations for arm/x86. So I guess, we can 
stay to a generic implementation.

-- 
Julien Grall

[PATCH V1 07/29] xen: Use the right string comparison function in device tree

[Julien Grall]

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

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

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

Re: [PATCH V1 07/29] xen: Use the right string comparison function in device tree

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> @@ -549,7 +549,7 @@ dt_find_property(const struct dt_device_node *np,
>  
>      for ( pp = np->properties; pp; pp = pp->next )
>      {
> -        if ( strcmp(pp->name, name) == 0 )
> +        if ( dt_prop_cmp(pp->name, name) == 0 )

This guy isn't added until patch #9.

Re: [PATCH V1 07/29] xen: Use the right string comparison function in device tree

[Julien Grall]

On 09/10/2013 11:35 AM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> @@ -549,7 +549,7 @@ dt_find_property(const struct dt_device_node *np,
>>  
>>      for ( pp = np->properties; pp; pp = pp->next )
>>      {
>> -        if ( strcmp(pp->name, name) == 0 )
>> +        if ( dt_prop_cmp(pp->name, name) == 0 )
> 
> This guy isn't added until patch #9.

Good catch. I will move the patch later, thanks.

-- 
Julien Grall

[PATCH V1 08/29] xen/dts: Don't add a fake property "name" in the device tree

[Julien Grall]

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

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

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

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

Re: [PATCH V1 08/29] xen/dts: Don't add a fake property "name" in the device tree

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> On new Flat Device Tree version, the property "name" may not exist.
> The property is never used in Xen code except to set the field "name" of
> dt_device_node.
> 
> For convenience, remove the fake property. It will save space during the
> creation of the dom0 FDT.

Is it worth diverging from the Linux code this is based on over this
though?

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/common/device_tree.c |   21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index be592d2..07a19ac 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -1543,6 +1543,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
>      if ( !has_name )
>      {
>          char *p1 = pathp, *ps = pathp, *pa = NULL;
> +        char *tmp = NULL;
>          int sz;
>  
>          while ( *p1 )
> @@ -1556,25 +1557,21 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
>          if ( pa < ps )
>              pa = p1;
>          sz = (pa - ps) + 1;
> -        pp = unflatten_dt_alloc(&mem, sizeof(struct dt_property) + sz,
> -                                __alignof__(struct dt_property));
> +
> +        tmp = unflatten_dt_alloc(&mem, sz, 1);
>          if ( allnextpp )
>          {
> -            pp->name = "name";
> -            pp->length = sz;
> -            pp->value = pp + 1;
> -            *prev_pp = pp;
> -            prev_pp = &pp->next;
> -            memcpy(pp->value, ps, sz - 1);
> -            ((char *)pp->value)[sz - 1] = 0;
> -            dt_dprintk("fixed up name for %s -> %s\n", pathp,
> -                       (char *)pp->value);
> +            memcpy(tmp, ps, sz - 1);
> +            np->name = tmp;
> +            tmp[sz - 1] = 0;
> +            dt_dprintk("fixed up name for %s -> %s\n", pathp, np->name);
>          }
>      }
> +
>      if ( allnextpp )
>      {
>          *prev_pp = NULL;
> -        np->name = dt_get_property(np, "name", NULL);
> +        np->name = (np->name) ? : dt_get_property(np, "name", NULL);
>          np->type = dt_get_property(np, "device_type", NULL);
>  
>          if ( !np->name )

Re: [PATCH V1 08/29] xen/dts: Don't add a fake property "name" in the device tree

[Julien Grall]

On 09/06/2013 05:28 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> On new Flat Device Tree version, the property "name" may not exist.
>> The property is never used in Xen code except to set the field "name" of
>> dt_device_node.
>>
>> For convenience, remove the fake property. It will save space during the
>> creation of the dom0 FDT.
>
> Is it worth diverging from the Linux code this is based on over this
> though?

If we don't diverge, I need to add some logic in device tree creation to 
know if the "name" property was generated by Xen.

The others solutions was:
    1) Add the logic during device tree creation
    2) Add a boolean to know if the property is a fake

Currently, the "name" property value is replicated in the name field of 
dt_device_node. Except staying close to Linux code, I don't see a good 
reason to create this fake property.

>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/common/device_tree.c |   21 +++++++++------------
>>   1 file changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index be592d2..07a19ac 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -1543,6 +1543,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
>>       if ( !has_name )
>>       {
>>           char *p1 = pathp, *ps = pathp, *pa = NULL;
>> +        char *tmp = NULL;
>>           int sz;
>>
>>           while ( *p1 )
>> @@ -1556,25 +1557,21 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
>>           if ( pa < ps )
>>               pa = p1;
>>           sz = (pa - ps) + 1;
>> -        pp = unflatten_dt_alloc(&mem, sizeof(struct dt_property) + sz,
>> -                                __alignof__(struct dt_property));
>> +
>> +        tmp = unflatten_dt_alloc(&mem, sz, 1);
>>           if ( allnextpp )
>>           {
>> -            pp->name = "name";
>> -            pp->length = sz;
>> -            pp->value = pp + 1;
>> -            *prev_pp = pp;
>> -            prev_pp = &pp->next;
>> -            memcpy(pp->value, ps, sz - 1);
>> -            ((char *)pp->value)[sz - 1] = 0;
>> -            dt_dprintk("fixed up name for %s -> %s\n", pathp,
>> -                       (char *)pp->value);
>> +            memcpy(tmp, ps, sz - 1);
>> +            np->name = tmp;
>> +            tmp[sz - 1] = 0;
>> +            dt_dprintk("fixed up name for %s -> %s\n", pathp, np->name);
>>           }
>>       }
>> +
>>       if ( allnextpp )
>>       {
>>           *prev_pp = NULL;
>> -        np->name = dt_get_property(np, "name", NULL);
>> +        np->name = (np->name) ? : dt_get_property(np, "name", NULL);
>>           np->type = dt_get_property(np, "device_type", NULL);
>>
>>           if ( !np->name )
>
>


-- 
Julien Grall

Re: [PATCH V1 08/29] xen/dts: Don't add a fake property "name" in the device tree

[Ian Campbell]

On Mon, 2013-09-09 at 10:30 +0100, Julien Grall wrote:
> On 09/06/2013 05:28 PM, Ian Campbell wrote:
> > On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> >> On new Flat Device Tree version, the property "name" may not exist.
> >> The property is never used in Xen code except to set the field "name" of
> >> dt_device_node.
> >>
> >> For convenience, remove the fake property. It will save space during the
> >> creation of the dom0 FDT.
> >
> > Is it worth diverging from the Linux code this is based on over this
> > though?
> 
> If we don't diverge, I need to add some logic in device tree creation to 
> know if the "name" property was generated by Xen.

Because otherwise it gets included in the DTB given to dom0? Is that
harmful though?

> 
> The others solutions was:
>     1) Add the logic during device tree creation
>     2) Add a boolean to know if the property is a fake

Would it be possible to reduce the divergence, at the cost of a little
bit of dead code by not actually removing the code which makes up the
name, but rather short circuiting it with a "continue" or an "if (0
&&  ...)" (plus an appropriate comment).

> Currently, the "name" property value is replicated in the name field of 
> dt_device_node. Except staying close to Linux code, I don't see a good 
> reason to create this fake property.
> 
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> ---
> >>   xen/common/device_tree.c |   21 +++++++++------------
> >>   1 file changed, 9 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> >> index be592d2..07a19ac 100644
> >> --- a/xen/common/device_tree.c
> >> +++ b/xen/common/device_tree.c
> >> @@ -1543,6 +1543,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
> >>       if ( !has_name )
> >>       {
> >>           char *p1 = pathp, *ps = pathp, *pa = NULL;
> >> +        char *tmp = NULL;
> >>           int sz;
> >>
> >>           while ( *p1 )
> >> @@ -1556,25 +1557,21 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
> >>           if ( pa < ps )
> >>               pa = p1;
> >>           sz = (pa - ps) + 1;
> >> -        pp = unflatten_dt_alloc(&mem, sizeof(struct dt_property) + sz,
> >> -                                __alignof__(struct dt_property));
> >> +
> >> +        tmp = unflatten_dt_alloc(&mem, sz, 1);
> >>           if ( allnextpp )
> >>           {
> >> -            pp->name = "name";
> >> -            pp->length = sz;
> >> -            pp->value = pp + 1;
> >> -            *prev_pp = pp;
> >> -            prev_pp = &pp->next;
> >> -            memcpy(pp->value, ps, sz - 1);
> >> -            ((char *)pp->value)[sz - 1] = 0;
> >> -            dt_dprintk("fixed up name for %s -> %s\n", pathp,
> >> -                       (char *)pp->value);
> >> +            memcpy(tmp, ps, sz - 1);
> >> +            np->name = tmp;
> >> +            tmp[sz - 1] = 0;
> >> +            dt_dprintk("fixed up name for %s -> %s\n", pathp, np->name);
> >>           }
> >>       }
> >> +
> >>       if ( allnextpp )
> >>       {
> >>           *prev_pp = NULL;
> >> -        np->name = dt_get_property(np, "name", NULL);
> >> +        np->name = (np->name) ? : dt_get_property(np, "name", NULL);
> >>           np->type = dt_get_property(np, "device_type", NULL);
> >>
> >>           if ( !np->name )
> >
> >
> 
> 

Re: [PATCH V1 08/29] xen/dts: Don't add a fake property "name" in the device tree

[Julien Grall]

On 09/09/2013 10:40 AM, Ian Campbell wrote:
> On Mon, 2013-09-09 at 10:30 +0100, Julien Grall wrote:
>> On 09/06/2013 05:28 PM, Ian Campbell wrote:
>>> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>>>> On new Flat Device Tree version, the property "name" may not exist.
>>>> The property is never used in Xen code except to set the field "name" of
>>>> dt_device_node.
>>>>
>>>> For convenience, remove the fake property. It will save space during the
>>>> creation of the dom0 FDT.
>>>
>>> Is it worth diverging from the Linux code this is based on over this
>>> though?
>>
>> If we don't diverge, I need to add some logic in device tree creation to
>> know if the "name" property was generated by Xen.
>
> Because otherwise it gets included in the DTB given to dom0? Is that
> harmful though?

The current device tree creation code is not able to reallocate memory 
if the device tree is bigger than the real one plus a small constant. I 
don't think it's easy to handle it because we don't have realloc 
function in Xen.

>
>>
>> The others solutions was:
>>      1) Add the logic during device tree creation
>>      2) Add a boolean to know if the property is a fake
>
> Would it be possible to reduce the divergence, at the cost of a little
> bit of dead code by not actually removing the code which makes up the
> name, but rather short circuiting it with a "continue" or an "if (0
> &&  ...)" (plus an appropriate comment).

I found this solution a bit ugly, but if we can avoid a divergence and 
some logic in device creation... I will rework the patch with this solution.

>> Currently, the "name" property value is replicated in the name field of
>> dt_device_node. Except staying close to Linux code, I don't see a good
>> reason to create this fake property.
>>
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>> ---
>>>>    xen/common/device_tree.c |   21 +++++++++------------
>>>>    1 file changed, 9 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>>> index be592d2..07a19ac 100644
>>>> --- a/xen/common/device_tree.c
>>>> +++ b/xen/common/device_tree.c
>>>> @@ -1543,6 +1543,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
>>>>        if ( !has_name )
>>>>        {
>>>>            char *p1 = pathp, *ps = pathp, *pa = NULL;
>>>> +        char *tmp = NULL;
>>>>            int sz;
>>>>
>>>>            while ( *p1 )
>>>> @@ -1556,25 +1557,21 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
>>>>            if ( pa < ps )
>>>>                pa = p1;
>>>>            sz = (pa - ps) + 1;
>>>> -        pp = unflatten_dt_alloc(&mem, sizeof(struct dt_property) + sz,
>>>> -                                __alignof__(struct dt_property));
>>>> +
>>>> +        tmp = unflatten_dt_alloc(&mem, sz, 1);
>>>>            if ( allnextpp )
>>>>            {
>>>> -            pp->name = "name";
>>>> -            pp->length = sz;
>>>> -            pp->value = pp + 1;
>>>> -            *prev_pp = pp;
>>>> -            prev_pp = &pp->next;
>>>> -            memcpy(pp->value, ps, sz - 1);
>>>> -            ((char *)pp->value)[sz - 1] = 0;
>>>> -            dt_dprintk("fixed up name for %s -> %s\n", pathp,
>>>> -                       (char *)pp->value);
>>>> +            memcpy(tmp, ps, sz - 1);
>>>> +            np->name = tmp;
>>>> +            tmp[sz - 1] = 0;
>>>> +            dt_dprintk("fixed up name for %s -> %s\n", pathp, np->name);
>>>>            }
>>>>        }
>>>> +
>>>>        if ( allnextpp )
>>>>        {
>>>>            *prev_pp = NULL;
>>>> -        np->name = dt_get_property(np, "name", NULL);
>>>> +        np->name = (np->name) ? : dt_get_property(np, "name", NULL);
>>>>            np->type = dt_get_property(np, "device_type", NULL);
>>>>
>>>>            if ( !np->name )
>>>
>>>
>>
>>
>
>


-- 
Julien Grall

Re: [PATCH V1 08/29] xen/dts: Don't add a fake property "name" in the device tree

[Ian Campbell]

On Mon, 2013-09-09 at 10:59 +0100, Julien Grall wrote:
> On 09/09/2013 10:40 AM, Ian Campbell wrote:
> > On Mon, 2013-09-09 at 10:30 +0100, Julien Grall wrote:
> >> On 09/06/2013 05:28 PM, Ian Campbell wrote:
> >>> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> >>>> On new Flat Device Tree version, the property "name" may not exist.
> >>>> The property is never used in Xen code except to set the field "name" of
> >>>> dt_device_node.
> >>>>
> >>>> For convenience, remove the fake property. It will save space during the
> >>>> creation of the dom0 FDT.
> >>>
> >>> Is it worth diverging from the Linux code this is based on over this
> >>> though?
> >>
> >> If we don't diverge, I need to add some logic in device tree creation to
> >> know if the "name" property was generated by Xen.
> >
> > Because otherwise it gets included in the DTB given to dom0? Is that
> > harmful though?
> 
> The current device tree creation code is not able to reallocate memory 
> if the device tree is bigger than the real one plus a small constant. I 
> don't think it's easy to handle it because we don't have realloc 
> function in Xen.

Understood.

I think we have plenty of RAM at this point, so the small constant could
actually be quite large.

> >> The others solutions was:
> >>      1) Add the logic during device tree creation
> >>      2) Add a boolean to know if the property is a fake
> >
> > Would it be possible to reduce the divergence, at the cost of a little
> > bit of dead code by not actually removing the code which makes up the
> > name, but rather short circuiting it with a "continue" or an "if (0
> > &&  ...)" (plus an appropriate comment).
> 
> I found this solution a bit ugly, but if we can avoid a divergence and 
> some logic in device creation... I will rework the patch with this solution.

I think a bit of ugliness is worth it in this case, since the code we
have inhereited is pretty complex and it would be nice to be able to
resync fixed versions in the future.

> 
> >> Currently, the "name" property value is replicated in the name field of
> >> dt_device_node. Except staying close to Linux code, I don't see a good
> >> reason to create this fake property.
> >>
> >>>>
> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>>> ---
> >>>>    xen/common/device_tree.c |   21 +++++++++------------
> >>>>    1 file changed, 9 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> >>>> index be592d2..07a19ac 100644
> >>>> --- a/xen/common/device_tree.c
> >>>> +++ b/xen/common/device_tree.c
> >>>> @@ -1543,6 +1543,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
> >>>>        if ( !has_name )
> >>>>        {
> >>>>            char *p1 = pathp, *ps = pathp, *pa = NULL;
> >>>> +        char *tmp = NULL;
> >>>>            int sz;
> >>>>
> >>>>            while ( *p1 )
> >>>> @@ -1556,25 +1557,21 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
> >>>>            if ( pa < ps )
> >>>>                pa = p1;
> >>>>            sz = (pa - ps) + 1;
> >>>> -        pp = unflatten_dt_alloc(&mem, sizeof(struct dt_property) + sz,
> >>>> -                                __alignof__(struct dt_property));
> >>>> +
> >>>> +        tmp = unflatten_dt_alloc(&mem, sz, 1);
> >>>>            if ( allnextpp )
> >>>>            {
> >>>> -            pp->name = "name";
> >>>> -            pp->length = sz;
> >>>> -            pp->value = pp + 1;
> >>>> -            *prev_pp = pp;
> >>>> -            prev_pp = &pp->next;
> >>>> -            memcpy(pp->value, ps, sz - 1);
> >>>> -            ((char *)pp->value)[sz - 1] = 0;
> >>>> -            dt_dprintk("fixed up name for %s -> %s\n", pathp,
> >>>> -                       (char *)pp->value);
> >>>> +            memcpy(tmp, ps, sz - 1);
> >>>> +            np->name = tmp;
> >>>> +            tmp[sz - 1] = 0;
> >>>> +            dt_dprintk("fixed up name for %s -> %s\n", pathp, np->name);
> >>>>            }
> >>>>        }
> >>>> +
> >>>>        if ( allnextpp )
> >>>>        {
> >>>>            *prev_pp = NULL;
> >>>> -        np->name = dt_get_property(np, "name", NULL);
> >>>> +        np->name = (np->name) ? : dt_get_property(np, "name", NULL);
> >>>>            np->type = dt_get_property(np, "device_type", NULL);
> >>>>
> >>>>            if ( !np->name )
> >>>
> >>>
> >>
> >>
> >
> >
> 
> 

[PATCH V1 09/29] xen/dts: Add new helpers to use the device tree

[Julien Grall]

List of new helpers taken from linux (commit 74b9272):
    - dt_property_read_string
    - dt_match_node
    - dt_find_maching_node
    - dt_device_is_available
    - dt_prop_cmp

Other new helpers:
    - dt_set_cell
    - for_each_child
    - dt_set_range
    - dt_cells_to_size
    - dt_next_cell
    - dt_get_range
    - dt_node_name_is_equal
    - dt_node_path_is_equal
    - dt_property_name_is_equal

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Update commit message
        - Add for_each_child
        - Add dt_get_range
        - Update documentation
        - Typoes
---
 xen/common/device_tree.c      |  109 +++++++++++++++++++++++++++--
 xen/include/xen/device_tree.h |  151 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 252 insertions(+), 8 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 07a19ac..833d67d 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -182,23 +182,38 @@ void __init device_tree_get_reg(const u32 **cell, u32 address_cells,
     get_val(cell, size_cells, size);
 }
 
-static void __init set_val(u32 **cell, u32 cells, u64 val)
+void dt_get_range(const __be32 **cell, const struct dt_device_node *np,
+                  u64 *address, u64 *size)
 {
-    u32 c = cells;
+    *address = dt_next_cell(dt_n_addr_cells(np), cell);
+    *size = dt_next_cell(dt_n_size_cells(np), cell);
+}
+
+void dt_set_cell(__be32 **cellp, int size, u64 val)
+{
+    int cells = size;
 
-    while ( c-- )
+    while ( size-- )
     {
-        (*cell)[c] = cpu_to_fdt32(val);
+        (*cellp)[size] = cpu_to_fdt32(val);
         val >>= 32;
     }
-    (*cell) += cells;
+
+    (*cellp) += cells;
 }
 
 void __init device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
                                 u64 start, u64 size)
 {
-    set_val(cell, address_cells, start);
-    set_val(cell, size_cells, size);
+    dt_set_cell(cell, address_cells, start);
+    dt_set_cell(cell, size_cells, size);
+}
+
+void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
+                  u64 address, u64 size)
+{
+    dt_set_cell(cellp, dt_n_addr_cells(np), address);
+    dt_set_cell(cellp, dt_n_size_cells(np), size);
 }
 
 u32 __init device_tree_get_u32(const void *fdt, int node, const char *prop_name,
@@ -583,6 +598,23 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
     return 1;
 }
 
+int dt_property_read_string(const struct dt_device_node *np,
+                            const char *propname, const char **out_string)
+{
+    const struct dt_property *pp = dt_find_property(np, propname, NULL);
+
+    if ( !pp )
+        return -EINVAL;
+    if ( !pp->value )
+        return -ENODATA;
+    if ( strnlen(pp->value, pp->length) >= pp->length )
+        return -EILSEQ;
+
+    *out_string = pp->value;
+
+    return 0;
+}
+
 bool_t dt_device_is_compatible(const struct dt_device_node *device,
                                const char *compat)
 {
@@ -655,6 +687,34 @@ struct dt_device_node *dt_find_node_by_alias(const char *alias)
     return NULL;
 }
 
+bool_t dt_match_node(const struct dt_device_match *matches,
+                     const struct dt_device_node *node)
+{
+    if ( !matches )
+        return 0;
+
+    while ( matches->path || matches->type || matches->compatible )
+    {
+        bool_t match = 1;
+
+        if ( matches->path )
+            match &= dt_node_path_is_equal(node, matches->path);
+
+        if ( matches->type )
+            match &= dt_device_type_is_equal(node, matches->type);
+
+        if ( matches->compatible )
+            match &= dt_device_is_compatible(node, matches->compatible);
+
+        if ( match )
+            return match;
+
+        matches++;
+    }
+
+    return 0;
+}
+
 const struct dt_device_node *dt_get_parent(const struct dt_device_node *node)
 {
     if ( !node )
@@ -684,6 +744,23 @@ dt_find_compatible_node(struct dt_device_node *from,
     return np;
 }
 
+struct dt_device_node *
+dt_find_matching_node(struct dt_device_node *from,
+                      const struct dt_device_match *matches)
+{
+    struct dt_device_node *np;
+    struct dt_device_node *dt;
+
+    dt = from ? from->allnext : dt_host;
+    for_each_device_node(dt, np)
+    {
+        if ( dt_match_node(matches, np) )
+            return np;
+    }
+
+    return NULL;
+}
+
 int dt_n_addr_cells(const struct dt_device_node *np)
 {
     const __be32 *ip;
@@ -1372,6 +1449,24 @@ int dt_device_get_irq(const struct dt_device_node *device, int index,
     return dt_irq_translate(&raw, out_irq);
 }
 
+bool_t dt_device_is_available(const struct dt_device_node *device)
+{
+    const char *status;
+    u32 statlen;
+
+    status = dt_get_property(device, "status", &statlen);
+    if ( status == NULL )
+        return 1;
+
+    if ( statlen > 0 )
+    {
+        if ( !strcmp(status, "okay") || !strcmp(status, "ok") )
+            return 1;
+    }
+
+    return 0;
+}
+
 /**
  * unflatten_dt_node - Alloc and populate a device_node from the flat tree
  * @fdt: The parent device tree blob
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 7cbf736..fa4f314 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -54,6 +54,19 @@ struct dt_early_info {
     struct dt_module_info modules;
 };
 
+/*
+ * Struct used for matching a device
+ */
+struct dt_device_match {
+    const char *path;
+    const char *type;
+    const char *compatible;
+};
+
+#define DT_MATCH_PATH(p)                { .path = p }
+#define DT_MATCH_TYPE(typ)              { .type = typ }
+#define DT_MATCH_COMPATIBLE(compat)     { .compatible = compat }
+
 typedef u32 dt_phandle;
 
 /**
@@ -229,6 +242,7 @@ extern const struct dt_device_node *dt_interrupt_controller;
  */
 struct dt_device_node * __init dt_find_interrupt_controller(const char *compat);
 
+#define dt_prop_cmp(s1, s2) strcmp((s1), (s2))
 #define dt_node_cmp(s1, s2) strcasecmp((s1), (s2))
 #define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2))
 
@@ -242,6 +256,9 @@ struct dt_device_node * __init dt_find_interrupt_controller(const char *compat);
 #define for_each_device_node(dt, dn)                         \
     for ( dn = dt; dn != NULL; dn = dn->allnext )
 
+#define for_each_child_node(dt, dn)                         \
+    for ( dn = dt->child; dn != NULL; dn = dn->sibling )
+
 /* Helper to read a big number; size is in cells (not bytes) */
 static inline u64 dt_read_number(const __be32 *cell, int size)
 {
@@ -252,6 +269,20 @@ static inline u64 dt_read_number(const __be32 *cell, int size)
     return r;
 }
 
+/* Helper to convert a number of cells in bytes */
+static inline int dt_cells_to_size(int size)
+{
+    return (size * sizeof (u32));
+}
+
+static inline u64 dt_next_cell(int s, const __be32 **cellp)
+{
+    const __be32 *p = *cellp;
+
+    *cellp = p + s;
+    return dt_read_number(p, s);
+}
+
 static inline const char *dt_node_full_name(const struct dt_device_node *np)
 {
     return (np && np->full_name) ? np->full_name : "<no-node>";
@@ -262,6 +293,18 @@ static inline const char *dt_node_name(const struct dt_device_node *np)
     return (np && np->name) ? np->name : "<no-node>";
 }
 
+static inline bool_t dt_node_name_is_equal(const struct dt_device_node *np,
+                                           const char *name)
+{
+    return !dt_node_cmp(np->name, name);
+}
+
+static inline bool_t dt_node_path_is_equal(const struct dt_device_node *np,
+                                           const char *path)
+{
+    return !dt_node_cmp(np->full_name, path);
+}
+
 static inline bool_t
 dt_device_type_is_equal(const struct dt_device_node *device,
                         const char *type)
@@ -281,6 +324,12 @@ static inline domid_t dt_device_used_by(const struct dt_device_node *device)
     return device->used_by;
 }
 
+static inline bool_t dt_property_name_is_equal(const struct dt_property *pp,
+                                               const char *name)
+{
+    return !dt_prop_cmp(pp->name, name);
+}
+
 /**
  * dt_find_compatible_node - Find a node based on type and one of the
  *                           tokens in its "compatible" property
@@ -315,6 +364,23 @@ const void *dt_get_property(const struct dt_device_node *np,
  */
 bool_t dt_property_read_u32(const struct dt_device_node *np,
                             const char *name, u32 *out_value);
+/**
+ * dt_property_read_string - Find and read a string from a property
+ * @np:         Device node from which the property value is to be read
+ * @propname:   Name of the property to be searched
+ * @out_string: Pointer to null terminated return string, modified only
+ *              if return value if 0.
+ *
+ * Search for a property in a device tree node and retrieve a null
+ * terminated string value (pointer to data, not a copy). Returns 0 on
+ * success, -EINVAL if the property does not exist, -ENODATA if property
+ * doest not have value, and -EILSEQ if the string is not
+ * null-terminated with the length of the property data.
+ *
+ * The out_string pointer is modified only if a valid string can be decoded.
+ */
+int dt_property_read_string(const struct dt_device_node *np,
+                            const char *propname, const char **out_string);
 
 /**
  * Checks if the given "compat" string matches one of the strings in
@@ -450,4 +516,87 @@ int dt_n_size_cells(const struct dt_device_node *np);
  */
 int dt_n_addr_cells(const struct dt_device_node *np);
 
-#endif
+/**
+ * dt_device_is_available - Check if a device is available for use
+ *
+ * @device: Node to check for availability
+ *
+ * Returns true if the status property is absent or set to "okay" or "ok",
+ * false otherwise.
+ */
+bool_t dt_device_is_available(const struct dt_device_node *device);
+
+/**
+ * dt_match_node - Tell if a device_node has a matching of dt_device_match
+ * @matches: array of dt_device_match structures to search in
+ * @node: the dt_device_node structure to match against
+ *
+ * Returns true if the device node match one of dt_device_match.
+ */
+bool_t dt_match_node(const struct dt_device_match *matches,
+                     const struct dt_device_node *node);
+
+/**
+ * dt_find_matching_node - Find a node based on an dt_device_match match table
+ * @from: The node to start searching from or NULL, the node you pass
+ *        will not be searched, only the next one will; typically, you pass
+ *        what the returned call returned
+ * @matches: array of dt_device_match structures to search in
+ *
+ * Returns a node pointer.
+ */
+struct dt_device_node *
+dt_find_matching_node(struct dt_device_node *from,
+                      const struct dt_device_match *matches);
+
+/**
+ * dt_set_cell - Write a value into a series of cells
+ *
+ * @cellp: Pointer to cells
+ * @size: number of cells to write the value
+ * @value: number to write
+ *
+ * Write a value into a series of cells and update cellp to point to the
+ * cell just after.
+ */
+void dt_set_cell(__be32 **cellp, int size, u64 val);
+
+/**
+ * dt_set_range - Write range into a series of cells
+ *
+ * @cellp: Pointer to cells
+ * @np: Node which contains the encoding for the address and the size
+ * @address: Start of range
+ * @size: Size of the range
+ *
+ * Write a range into a series of cells and update cellp to point to the
+ * cell just after.
+ */
+void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
+                  u64 address, u64 size);
+
+/**
+ * dt_get_range - Read a range (address/size) from a series of cells
+ *
+ * @cellp: Pointer to cells
+ * @np Node which  contains the encoding for the addresss and the size
+ * @address: Address filled by this function
+ * @size: Size filled by this function
+ *
+ * WARNING: This function should not be used to decode an address
+ * This function reads a range (address/size) from a series of cells and
+ * update cellp to point to the cell just after.
+ */
+void dt_get_range(const __be32 **cellp, const struct dt_device_node *np,
+                  u64 *address, u64 *size);
+
+#endif /* __XEN_DEVICE_TREE_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.7.10.4

Re: [PATCH V1 09/29] xen/dts: Add new helpers to use the device tree

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> List of new helpers taken from linux (commit 74b9272):
>     - dt_property_read_string
>     - dt_match_node
>     - dt_find_maching_node
>     - dt_device_is_available
>     - dt_prop_cmp
> 
> Other new helpers:
>     - dt_set_cell
>     - for_each_child

This seems too generic. Perhaps dt_for_each_child or for_each_dt_child?

>     - dt_set_range
>     - dt_cells_to_size
>     - dt_next_cell
>     - dt_get_range
>     - dt_node_name_is_equal
>     - dt_node_path_is_equal
>     - dt_property_name_is_equal
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Update commit message
>         - Add for_each_child
>         - Add dt_get_range
>         - Update documentation
>         - Typoes
> ---
>  xen/common/device_tree.c      |  109 +++++++++++++++++++++++++++--
>  xen/include/xen/device_tree.h |  151 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 252 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 07a19ac..833d67d 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -182,23 +182,38 @@ void __init device_tree_get_reg(const u32 **cell, u32 address_cells,
>      get_val(cell, size_cells, size);
>  }
>  
> -static void __init set_val(u32 **cell, u32 cells, u64 val)
> +void dt_get_range(const __be32 **cell, const struct dt_device_node *np,
> +                  u64 *address, u64 *size)
>  {
> -    u32 c = cells;
> +    *address = dt_next_cell(dt_n_addr_cells(np), cell);
> +    *size = dt_next_cell(dt_n_size_cells(np), cell);
> +}
> +
> +void dt_set_cell(__be32 **cellp, int size, u64 val)
> +{
> +    int cells = size;
>  
> -    while ( c-- )
> +    while ( size-- )
>      {
> -        (*cell)[c] = cpu_to_fdt32(val);
> +        (*cellp)[size] = cpu_to_fdt32(val);
>          val >>= 32;
>      }
> -    (*cell) += cells;
> +
> +    (*cellp) += cells;
>  }
>  
>  void __init device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
>                                  u64 start, u64 size)
>  {
> -    set_val(cell, address_cells, start);
> -    set_val(cell, size_cells, size);
> +    dt_set_cell(cell, address_cells, start);
> +    dt_set_cell(cell, size_cells, size);
> +}
> +
> +void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
> +                  u64 address, u64 size)
> +{
> +    dt_set_cell(cellp, dt_n_addr_cells(np), address);
> +    dt_set_cell(cellp, dt_n_size_cells(np), size);
>  }
>  
>  u32 __init device_tree_get_u32(const void *fdt, int node, const char *prop_name,
> @@ -583,6 +598,23 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
>      return 1;
>  }
>  
> +int dt_property_read_string(const struct dt_device_node *np,
> +                            const char *propname, const char **out_string)
> +{
> +    const struct dt_property *pp = dt_find_property(np, propname, NULL);
> +
> +    if ( !pp )
> +        return -EINVAL;
> +    if ( !pp->value )
> +        return -ENODATA;
> +    if ( strnlen(pp->value, pp->length) >= pp->length )
> +        return -EILSEQ;
> +
> +    *out_string = pp->value;
> +
> +    return 0;
> +}
> +
>  bool_t dt_device_is_compatible(const struct dt_device_node *device,
>                                 const char *compat)
>  {
> @@ -655,6 +687,34 @@ struct dt_device_node *dt_find_node_by_alias(const char *alias)
>      return NULL;
>  }
>  
> +bool_t dt_match_node(const struct dt_device_match *matches,
> +                     const struct dt_device_node *node)
> +{
> +    if ( !matches )
> +        return 0;
> +
> +    while ( matches->path || matches->type || matches->compatible )
> +    {
> +        bool_t match = 1;
> +
> +        if ( matches->path )
> +            match &= dt_node_path_is_equal(node, matches->path);
> +
> +        if ( matches->type )
> +            match &= dt_device_type_is_equal(node, matches->type);
> +
> +        if ( matches->compatible )
> +            match &= dt_device_is_compatible(node, matches->compatible);
> +
> +        if ( match )
> +            return match;
> +
> +        matches++;
> +    }
> +
> +    return 0;
> +}
> +
>  const struct dt_device_node *dt_get_parent(const struct dt_device_node *node)
>  {
>      if ( !node )
> @@ -684,6 +744,23 @@ dt_find_compatible_node(struct dt_device_node *from,
>      return np;
>  }
>  
> +struct dt_device_node *
> +dt_find_matching_node(struct dt_device_node *from,
> +                      const struct dt_device_match *matches)
> +{
> +    struct dt_device_node *np;
> +    struct dt_device_node *dt;
> +
> +    dt = from ? from->allnext : dt_host;
> +    for_each_device_node(dt, np)
> +    {
> +        if ( dt_match_node(matches, np) )
> +            return np;
> +    }
> +
> +    return NULL;
> +}
> +
>  int dt_n_addr_cells(const struct dt_device_node *np)
>  {
>      const __be32 *ip;
> @@ -1372,6 +1449,24 @@ int dt_device_get_irq(const struct dt_device_node *device, int index,
>      return dt_irq_translate(&raw, out_irq);
>  }
>  
> +bool_t dt_device_is_available(const struct dt_device_node *device)
> +{
> +    const char *status;
> +    u32 statlen;
> +
> +    status = dt_get_property(device, "status", &statlen);
> +    if ( status == NULL )
> +        return 1;
> +
> +    if ( statlen > 0 )
> +    {
> +        if ( !strcmp(status, "okay") || !strcmp(status, "ok") )
> +            return 1;
> +    }
> +
> +    return 0;
> +}
> +
>  /**
>   * unflatten_dt_node - Alloc and populate a device_node from the flat tree
>   * @fdt: The parent device tree blob
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 7cbf736..fa4f314 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -54,6 +54,19 @@ struct dt_early_info {
>      struct dt_module_info modules;
>  };
>  
> +/*
> + * Struct used for matching a device
> + */
> +struct dt_device_match {
> +    const char *path;
> +    const char *type;
> +    const char *compatible;
> +};
> +
> +#define DT_MATCH_PATH(p)                { .path = p }
> +#define DT_MATCH_TYPE(typ)              { .type = typ }
> +#define DT_MATCH_COMPATIBLE(compat)     { .compatible = compat }
> +
>  typedef u32 dt_phandle;
>  
>  /**
> @@ -229,6 +242,7 @@ extern const struct dt_device_node *dt_interrupt_controller;
>   */
>  struct dt_device_node * __init dt_find_interrupt_controller(const char *compat);
>  
> +#define dt_prop_cmp(s1, s2) strcmp((s1), (s2))
>  #define dt_node_cmp(s1, s2) strcasecmp((s1), (s2))
>  #define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2))
>  
> @@ -242,6 +256,9 @@ struct dt_device_node * __init dt_find_interrupt_controller(const char *compat);
>  #define for_each_device_node(dt, dn)                         \
>      for ( dn = dt; dn != NULL; dn = dn->allnext )
>  
> +#define for_each_child_node(dt, dn)                         \
> +    for ( dn = dt->child; dn != NULL; dn = dn->sibling )
> +
>  /* Helper to read a big number; size is in cells (not bytes) */
>  static inline u64 dt_read_number(const __be32 *cell, int size)
>  {
> @@ -252,6 +269,20 @@ static inline u64 dt_read_number(const __be32 *cell, int size)
>      return r;
>  }
>  
> +/* Helper to convert a number of cells in bytes */
> +static inline int dt_cells_to_size(int size)
> +{
> +    return (size * sizeof (u32));
> +}
> +
> +static inline u64 dt_next_cell(int s, const __be32 **cellp)
> +{
> +    const __be32 *p = *cellp;
> +
> +    *cellp = p + s;
> +    return dt_read_number(p, s);
> +}
> +
>  static inline const char *dt_node_full_name(const struct dt_device_node *np)
>  {
>      return (np && np->full_name) ? np->full_name : "<no-node>";
> @@ -262,6 +293,18 @@ static inline const char *dt_node_name(const struct dt_device_node *np)
>      return (np && np->name) ? np->name : "<no-node>";
>  }
>  
> +static inline bool_t dt_node_name_is_equal(const struct dt_device_node *np,
> +                                           const char *name)
> +{
> +    return !dt_node_cmp(np->name, name);
> +}
> +
> +static inline bool_t dt_node_path_is_equal(const struct dt_device_node *np,
> +                                           const char *path)
> +{
> +    return !dt_node_cmp(np->full_name, path);
> +}
> +
>  static inline bool_t
>  dt_device_type_is_equal(const struct dt_device_node *device,
>                          const char *type)
> @@ -281,6 +324,12 @@ static inline domid_t dt_device_used_by(const struct dt_device_node *device)
>      return device->used_by;
>  }
>  
> +static inline bool_t dt_property_name_is_equal(const struct dt_property *pp,
> +                                               const char *name)
> +{
> +    return !dt_prop_cmp(pp->name, name);
> +}
> +
>  /**
>   * dt_find_compatible_node - Find a node based on type and one of the
>   *                           tokens in its "compatible" property
> @@ -315,6 +364,23 @@ const void *dt_get_property(const struct dt_device_node *np,
>   */
>  bool_t dt_property_read_u32(const struct dt_device_node *np,
>                              const char *name, u32 *out_value);
> +/**
> + * dt_property_read_string - Find and read a string from a property
> + * @np:         Device node from which the property value is to be read
> + * @propname:   Name of the property to be searched
> + * @out_string: Pointer to null terminated return string, modified only
> + *              if return value if 0.
> + *
> + * Search for a property in a device tree node and retrieve a null
> + * terminated string value (pointer to data, not a copy). Returns 0 on
> + * success, -EINVAL if the property does not exist, -ENODATA if property
> + * doest not have value, and -EILSEQ if the string is not
> + * null-terminated with the length of the property data.
> + *
> + * The out_string pointer is modified only if a valid string can be decoded.
> + */
> +int dt_property_read_string(const struct dt_device_node *np,
> +                            const char *propname, const char **out_string);
>  
>  /**
>   * Checks if the given "compat" string matches one of the strings in
> @@ -450,4 +516,87 @@ int dt_n_size_cells(const struct dt_device_node *np);
>   */
>  int dt_n_addr_cells(const struct dt_device_node *np);
>  
> -#endif
> +/**
> + * dt_device_is_available - Check if a device is available for use
> + *
> + * @device: Node to check for availability
> + *
> + * Returns true if the status property is absent or set to "okay" or "ok",
> + * false otherwise.
> + */
> +bool_t dt_device_is_available(const struct dt_device_node *device);
> +
> +/**
> + * dt_match_node - Tell if a device_node has a matching of dt_device_match
> + * @matches: array of dt_device_match structures to search in
> + * @node: the dt_device_node structure to match against
> + *
> + * Returns true if the device node match one of dt_device_match.
> + */
> +bool_t dt_match_node(const struct dt_device_match *matches,
> +                     const struct dt_device_node *node);
> +
> +/**
> + * dt_find_matching_node - Find a node based on an dt_device_match match table
> + * @from: The node to start searching from or NULL, the node you pass
> + *        will not be searched, only the next one will; typically, you pass
> + *        what the returned call returned
> + * @matches: array of dt_device_match structures to search in
> + *
> + * Returns a node pointer.
> + */
> +struct dt_device_node *
> +dt_find_matching_node(struct dt_device_node *from,
> +                      const struct dt_device_match *matches);
> +
> +/**
> + * dt_set_cell - Write a value into a series of cells
> + *
> + * @cellp: Pointer to cells
> + * @size: number of cells to write the value
> + * @value: number to write
> + *
> + * Write a value into a series of cells and update cellp to point to the
> + * cell just after.
> + */
> +void dt_set_cell(__be32 **cellp, int size, u64 val);
> +
> +/**
> + * dt_set_range - Write range into a series of cells
> + *
> + * @cellp: Pointer to cells
> + * @np: Node which contains the encoding for the address and the size
> + * @address: Start of range
> + * @size: Size of the range
> + *
> + * Write a range into a series of cells and update cellp to point to the
> + * cell just after.
> + */
> +void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
> +                  u64 address, u64 size);
> +
> +/**
> + * dt_get_range - Read a range (address/size) from a series of cells
> + *
> + * @cellp: Pointer to cells
> + * @np Node which  contains the encoding for the addresss and the size
> + * @address: Address filled by this function
> + * @size: Size filled by this function
> + *
> + * WARNING: This function should not be used to decode an address
> + * This function reads a range (address/size) from a series of cells and
> + * update cellp to point to the cell just after.
> + */
> +void dt_get_range(const __be32 **cellp, const struct dt_device_node *np,
> +                  u64 *address, u64 *size);
> +
> +#endif /* __XEN_DEVICE_TREE_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Re: [PATCH V1 09/29] xen/dts: Add new helpers to use the device tree

[Julien Grall]

On 09/06/2013 05:31 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> List of new helpers taken from linux (commit 74b9272):
>>      - dt_property_read_string
>>      - dt_match_node
>>      - dt_find_maching_node
>>      - dt_device_is_available
>>      - dt_prop_cmp
>>
>> Other new helpers:
>>      - dt_set_cell
>>      - for_each_child
>
> This seems too generic. Perhaps dt_for_each_child or for_each_dt_child?

Right, I will rename the function to dt_for_each_child.

>>      - dt_set_range
>>      - dt_cells_to_size
>>      - dt_next_cell
>>      - dt_get_range
>>      - dt_node_name_is_equal
>>      - dt_node_path_is_equal
>>      - dt_property_name_is_equal
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>      Changes in v2:
>>          - Update commit message
>>          - Add for_each_child
>>          - Add dt_get_range
>>          - Update documentation
>>          - Typoes
>> ---
>>   xen/common/device_tree.c      |  109 +++++++++++++++++++++++++++--
>>   xen/include/xen/device_tree.h |  151 ++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 252 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 07a19ac..833d67d 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -182,23 +182,38 @@ void __init device_tree_get_reg(const u32 **cell, u32 address_cells,
>>       get_val(cell, size_cells, size);
>>   }
>>
>> -static void __init set_val(u32 **cell, u32 cells, u64 val)
>> +void dt_get_range(const __be32 **cell, const struct dt_device_node *np,
>> +                  u64 *address, u64 *size)
>>   {
>> -    u32 c = cells;
>> +    *address = dt_next_cell(dt_n_addr_cells(np), cell);
>> +    *size = dt_next_cell(dt_n_size_cells(np), cell);
>> +}
>> +
>> +void dt_set_cell(__be32 **cellp, int size, u64 val)
>> +{
>> +    int cells = size;
>>
>> -    while ( c-- )
>> +    while ( size-- )
>>       {
>> -        (*cell)[c] = cpu_to_fdt32(val);
>> +        (*cellp)[size] = cpu_to_fdt32(val);
>>           val >>= 32;
>>       }
>> -    (*cell) += cells;
>> +
>> +    (*cellp) += cells;
>>   }
>>
>>   void __init device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
>>                                   u64 start, u64 size)
>>   {
>> -    set_val(cell, address_cells, start);
>> -    set_val(cell, size_cells, size);
>> +    dt_set_cell(cell, address_cells, start);
>> +    dt_set_cell(cell, size_cells, size);
>> +}
>> +
>> +void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
>> +                  u64 address, u64 size)
>> +{
>> +    dt_set_cell(cellp, dt_n_addr_cells(np), address);
>> +    dt_set_cell(cellp, dt_n_size_cells(np), size);
>>   }
>>
>>   u32 __init device_tree_get_u32(const void *fdt, int node, const char *prop_name,
>> @@ -583,6 +598,23 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
>>       return 1;
>>   }
>>
>> +int dt_property_read_string(const struct dt_device_node *np,
>> +                            const char *propname, const char **out_string)
>> +{
>> +    const struct dt_property *pp = dt_find_property(np, propname, NULL);
>> +
>> +    if ( !pp )
>> +        return -EINVAL;
>> +    if ( !pp->value )
>> +        return -ENODATA;
>> +    if ( strnlen(pp->value, pp->length) >= pp->length )
>> +        return -EILSEQ;
>> +
>> +    *out_string = pp->value;
>> +
>> +    return 0;
>> +}
>> +
>>   bool_t dt_device_is_compatible(const struct dt_device_node *device,
>>                                  const char *compat)
>>   {
>> @@ -655,6 +687,34 @@ struct dt_device_node *dt_find_node_by_alias(const char *alias)
>>       return NULL;
>>   }
>>
>> +bool_t dt_match_node(const struct dt_device_match *matches,
>> +                     const struct dt_device_node *node)
>> +{
>> +    if ( !matches )
>> +        return 0;
>> +
>> +    while ( matches->path || matches->type || matches->compatible )
>> +    {
>> +        bool_t match = 1;
>> +
>> +        if ( matches->path )
>> +            match &= dt_node_path_is_equal(node, matches->path);
>> +
>> +        if ( matches->type )
>> +            match &= dt_device_type_is_equal(node, matches->type);
>> +
>> +        if ( matches->compatible )
>> +            match &= dt_device_is_compatible(node, matches->compatible);
>> +
>> +        if ( match )
>> +            return match;
>> +
>> +        matches++;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   const struct dt_device_node *dt_get_parent(const struct dt_device_node *node)
>>   {
>>       if ( !node )
>> @@ -684,6 +744,23 @@ dt_find_compatible_node(struct dt_device_node *from,
>>       return np;
>>   }
>>
>> +struct dt_device_node *
>> +dt_find_matching_node(struct dt_device_node *from,
>> +                      const struct dt_device_match *matches)
>> +{
>> +    struct dt_device_node *np;
>> +    struct dt_device_node *dt;
>> +
>> +    dt = from ? from->allnext : dt_host;
>> +    for_each_device_node(dt, np)
>> +    {
>> +        if ( dt_match_node(matches, np) )
>> +            return np;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>>   int dt_n_addr_cells(const struct dt_device_node *np)
>>   {
>>       const __be32 *ip;
>> @@ -1372,6 +1449,24 @@ int dt_device_get_irq(const struct dt_device_node *device, int index,
>>       return dt_irq_translate(&raw, out_irq);
>>   }
>>
>> +bool_t dt_device_is_available(const struct dt_device_node *device)
>> +{
>> +    const char *status;
>> +    u32 statlen;
>> +
>> +    status = dt_get_property(device, "status", &statlen);
>> +    if ( status == NULL )
>> +        return 1;
>> +
>> +    if ( statlen > 0 )
>> +    {
>> +        if ( !strcmp(status, "okay") || !strcmp(status, "ok") )
>> +            return 1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   /**
>>    * unflatten_dt_node - Alloc and populate a device_node from the flat tree
>>    * @fdt: The parent device tree blob
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index 7cbf736..fa4f314 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -54,6 +54,19 @@ struct dt_early_info {
>>       struct dt_module_info modules;
>>   };
>>
>> +/*
>> + * Struct used for matching a device
>> + */
>> +struct dt_device_match {
>> +    const char *path;
>> +    const char *type;
>> +    const char *compatible;
>> +};
>> +
>> +#define DT_MATCH_PATH(p)                { .path = p }
>> +#define DT_MATCH_TYPE(typ)              { .type = typ }
>> +#define DT_MATCH_COMPATIBLE(compat)     { .compatible = compat }
>> +
>>   typedef u32 dt_phandle;
>>
>>   /**
>> @@ -229,6 +242,7 @@ extern const struct dt_device_node *dt_interrupt_controller;
>>    */
>>   struct dt_device_node * __init dt_find_interrupt_controller(const char *compat);
>>
>> +#define dt_prop_cmp(s1, s2) strcmp((s1), (s2))
>>   #define dt_node_cmp(s1, s2) strcasecmp((s1), (s2))
>>   #define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2))
>>
>> @@ -242,6 +256,9 @@ struct dt_device_node * __init dt_find_interrupt_controller(const char *compat);
>>   #define for_each_device_node(dt, dn)                         \
>>       for ( dn = dt; dn != NULL; dn = dn->allnext )
>>
>> +#define for_each_child_node(dt, dn)                         \
>> +    for ( dn = dt->child; dn != NULL; dn = dn->sibling )
>> +
>>   /* Helper to read a big number; size is in cells (not bytes) */
>>   static inline u64 dt_read_number(const __be32 *cell, int size)
>>   {
>> @@ -252,6 +269,20 @@ static inline u64 dt_read_number(const __be32 *cell, int size)
>>       return r;
>>   }
>>
>> +/* Helper to convert a number of cells in bytes */
>> +static inline int dt_cells_to_size(int size)
>> +{
>> +    return (size * sizeof (u32));
>> +}
>> +
>> +static inline u64 dt_next_cell(int s, const __be32 **cellp)
>> +{
>> +    const __be32 *p = *cellp;
>> +
>> +    *cellp = p + s;
>> +    return dt_read_number(p, s);
>> +}
>> +
>>   static inline const char *dt_node_full_name(const struct dt_device_node *np)
>>   {
>>       return (np && np->full_name) ? np->full_name : "<no-node>";
>> @@ -262,6 +293,18 @@ static inline const char *dt_node_name(const struct dt_device_node *np)
>>       return (np && np->name) ? np->name : "<no-node>";
>>   }
>>
>> +static inline bool_t dt_node_name_is_equal(const struct dt_device_node *np,
>> +                                           const char *name)
>> +{
>> +    return !dt_node_cmp(np->name, name);
>> +}
>> +
>> +static inline bool_t dt_node_path_is_equal(const struct dt_device_node *np,
>> +                                           const char *path)
>> +{
>> +    return !dt_node_cmp(np->full_name, path);
>> +}
>> +
>>   static inline bool_t
>>   dt_device_type_is_equal(const struct dt_device_node *device,
>>                           const char *type)
>> @@ -281,6 +324,12 @@ static inline domid_t dt_device_used_by(const struct dt_device_node *device)
>>       return device->used_by;
>>   }
>>
>> +static inline bool_t dt_property_name_is_equal(const struct dt_property *pp,
>> +                                               const char *name)
>> +{
>> +    return !dt_prop_cmp(pp->name, name);
>> +}
>> +
>>   /**
>>    * dt_find_compatible_node - Find a node based on type and one of the
>>    *                           tokens in its "compatible" property
>> @@ -315,6 +364,23 @@ const void *dt_get_property(const struct dt_device_node *np,
>>    */
>>   bool_t dt_property_read_u32(const struct dt_device_node *np,
>>                               const char *name, u32 *out_value);
>> +/**
>> + * dt_property_read_string - Find and read a string from a property
>> + * @np:         Device node from which the property value is to be read
>> + * @propname:   Name of the property to be searched
>> + * @out_string: Pointer to null terminated return string, modified only
>> + *              if return value if 0.
>> + *
>> + * Search for a property in a device tree node and retrieve a null
>> + * terminated string value (pointer to data, not a copy). Returns 0 on
>> + * success, -EINVAL if the property does not exist, -ENODATA if property
>> + * doest not have value, and -EILSEQ if the string is not
>> + * null-terminated with the length of the property data.
>> + *
>> + * The out_string pointer is modified only if a valid string can be decoded.
>> + */
>> +int dt_property_read_string(const struct dt_device_node *np,
>> +                            const char *propname, const char **out_string);
>>
>>   /**
>>    * Checks if the given "compat" string matches one of the strings in
>> @@ -450,4 +516,87 @@ int dt_n_size_cells(const struct dt_device_node *np);
>>    */
>>   int dt_n_addr_cells(const struct dt_device_node *np);
>>
>> -#endif
>> +/**
>> + * dt_device_is_available - Check if a device is available for use
>> + *
>> + * @device: Node to check for availability
>> + *
>> + * Returns true if the status property is absent or set to "okay" or "ok",
>> + * false otherwise.
>> + */
>> +bool_t dt_device_is_available(const struct dt_device_node *device);
>> +
>> +/**
>> + * dt_match_node - Tell if a device_node has a matching of dt_device_match
>> + * @matches: array of dt_device_match structures to search in
>> + * @node: the dt_device_node structure to match against
>> + *
>> + * Returns true if the device node match one of dt_device_match.
>> + */
>> +bool_t dt_match_node(const struct dt_device_match *matches,
>> +                     const struct dt_device_node *node);
>> +
>> +/**
>> + * dt_find_matching_node - Find a node based on an dt_device_match match table
>> + * @from: The node to start searching from or NULL, the node you pass
>> + *        will not be searched, only the next one will; typically, you pass
>> + *        what the returned call returned
>> + * @matches: array of dt_device_match structures to search in
>> + *
>> + * Returns a node pointer.
>> + */
>> +struct dt_device_node *
>> +dt_find_matching_node(struct dt_device_node *from,
>> +                      const struct dt_device_match *matches);
>> +
>> +/**
>> + * dt_set_cell - Write a value into a series of cells
>> + *
>> + * @cellp: Pointer to cells
>> + * @size: number of cells to write the value
>> + * @value: number to write
>> + *
>> + * Write a value into a series of cells and update cellp to point to the
>> + * cell just after.
>> + */
>> +void dt_set_cell(__be32 **cellp, int size, u64 val);
>> +
>> +/**
>> + * dt_set_range - Write range into a series of cells
>> + *
>> + * @cellp: Pointer to cells
>> + * @np: Node which contains the encoding for the address and the size
>> + * @address: Start of range
>> + * @size: Size of the range
>> + *
>> + * Write a range into a series of cells and update cellp to point to the
>> + * cell just after.
>> + */
>> +void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
>> +                  u64 address, u64 size);
>> +
>> +/**
>> + * dt_get_range - Read a range (address/size) from a series of cells
>> + *
>> + * @cellp: Pointer to cells
>> + * @np Node which  contains the encoding for the addresss and the size
>> + * @address: Address filled by this function
>> + * @size: Size filled by this function
>> + *
>> + * WARNING: This function should not be used to decode an address
>> + * This function reads a range (address/size) from a series of cells and
>> + * update cellp to point to the cell just after.
>> + */
>> +void dt_get_range(const __be32 **cellp, const struct dt_device_node *np,
>> +                  u64 *address, u64 *size);
>> +
>> +#endif /* __XEN_DEVICE_TREE_H */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>
>


-- 
Julien Grall

[PATCH V1 10/29] xen/dts: Remove device_get_reg call in process_cpu_node

[Julien Grall]

The "reg" property is only composed of one uint32. device_get_reg can be
replaced by dt_read_number.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Rework the commit message
---
 xen/common/device_tree.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

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

Re: [PATCH V1 10/29] xen/dts: Remove device_get_reg call in process_cpu_node

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> The "reg" property is only composed of one uint32. device_get_reg can be
> replaced by dt_read_number.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

Although I notice Linux has helpers like dt_property_read_u32 etc. Would
be useful for us too I suppose?

> 
> ---
>     Changes in v2:
>         - Rework the commit message
> ---
>  xen/common/device_tree.c |   19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 833d67d..9568250 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -426,21 +426,26 @@ static void __init process_cpu_node(const void *fdt, int node,
>                                      u32 address_cells, u32 size_cells)
>  {
>      const struct fdt_property *prop;
> -    const u32 *cell;
> -    paddr_t start, size;
> -
> +    u32 cpuid;
> +    int len;
>  
> -    prop = fdt_get_property(fdt, node, "reg", NULL);
> +    prop = fdt_get_property(fdt, node, "reg", &len);
>      if ( !prop )
>      {
>          early_printk("fdt: node `%s': missing `reg' property\n", name);
>          return;
>      }
>  
> -    cell = (const u32 *)prop->data;
> -    device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +    if ( len < sizeof (cpuid) )
> +    {
> +        dt_printk("fdt: node `%s': `reg` property length is too short\n",
> +                  name);
> +        return;
> +    }
> +
> +    cpuid = dt_read_number((const __be32 *)prop->data, 1);
>  
> -    cpumask_set_cpu(start, &cpu_possible_map);
> +    cpumask_set_cpu(cpuid, &cpu_possible_map);
>  }
>  
>  static void __init process_multiboot_node(const void *fdt, int node,

Re: [PATCH V1 10/29] xen/dts: Remove device_get_reg call in process_cpu_node

[Julien Grall]

On 09/06/2013 05:36 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> The "reg" property is only composed of one uint32. device_get_reg can be
>> replaced by dt_read_number.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Although I notice Linux has helpers like dt_property_read_u32 etc. Would
> be useful for us too I suppose?

The device tree API already have dt_property_read_u32. It was added by 
Chen a couple of weeks ago. But this function only works with the device 
tree structure.
Here, we are using the flat device tree, because the memory layout has 
not yet been setup (so no possibility to allocate memory for the structure).

>>
>> ---
>>      Changes in v2:
>>          - Rework the commit message
>> ---
>>   xen/common/device_tree.c |   19 ++++++++++++-------
>>   1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 833d67d..9568250 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -426,21 +426,26 @@ static void __init process_cpu_node(const void *fdt, int node,
>>                                       u32 address_cells, u32 size_cells)
>>   {
>>       const struct fdt_property *prop;
>> -    const u32 *cell;
>> -    paddr_t start, size;
>> -
>> +    u32 cpuid;
>> +    int len;
>>
>> -    prop = fdt_get_property(fdt, node, "reg", NULL);
>> +    prop = fdt_get_property(fdt, node, "reg", &len);
>>       if ( !prop )
>>       {
>>           early_printk("fdt: node `%s': missing `reg' property\n", name);
>>           return;
>>       }
>>
>> -    cell = (const u32 *)prop->data;
>> -    device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>> +    if ( len < sizeof (cpuid) )
>> +    {
>> +        dt_printk("fdt: node `%s': `reg` property length is too short\n",
>> +                  name);
>> +        return;
>> +    }
>> +
>> +    cpuid = dt_read_number((const __be32 *)prop->data, 1);
>>
>> -    cpumask_set_cpu(start, &cpu_possible_map);
>> +    cpumask_set_cpu(cpuid, &cpu_possible_map);
>>   }
>>
>>   static void __init process_multiboot_node(const void *fdt, int node,
>
>


-- 
Julien Grall

[PATCH V1 11/29] xen/dts: Check "reg" property length in process_multiboot_node

[Julien Grall]

When the device tree compiler (dtc) can't find right #address-cells
and #size-cells, it will assume the encoding is 1 for each.
As multiboot node are inside the /chosen, dtc will asume the previous values
if the both property are not correct set.

During boot, Xen will browse the fdt and store the nearest #address-cells
and #size-cells value. If the size of "reg" is smaller, Xen can retrieve
wrong range.

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

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 9568250..7295f34 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -467,10 +467,14 @@ static void __init process_multiboot_node(const void *fdt, int node,
 
     mod = &early_info.modules.module[nr];
 
-    prop = fdt_get_property(fdt, node, "reg", NULL);
+    prop = fdt_get_property(fdt, node, "reg", &len);
     if ( !prop )
         early_panic("node %s missing `reg' property\n", name);
 
+    if ( len < dt_cells_to_size(address_cells + size_cells) )
+        early_panic("fdt: node `%s': `reg` property length is too short\n",
+                    name);
+
     cell = (const u32 *)prop->data;
     device_tree_get_reg(&cell, address_cells, size_cells,
                         &mod->start, &mod->size);
-- 
1.7.10.4

Re: [PATCH V1 11/29] xen/dts: Check "reg" property length in process_multiboot_node

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> When the device tree compiler (dtc) can't find right #address-cells
> and #size-cells, it will assume the encoding is 1 for each.

Without inserting them into the DTB I suppose?

> As multiboot node are inside the /chosen, dtc will asume the previous values

"assume"

> if the both property are not correct set.

"the both property" => "both properties" and "correctly"

By "previous values" do you mean the values set in the (grand)parent
node?

> During boot, Xen will browse the fdt and store the nearest #address-cells
> and #size-cells value. If the size of "reg" is smaller, Xen can retrieve
> wrong range.

How come Xen can find a suitable #address-cells but dtc cannot? Are we
not handling them correctly perhaps?

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/common/device_tree.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 9568250..7295f34 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -467,10 +467,14 @@ static void __init process_multiboot_node(const void *fdt, int node,
>  
>      mod = &early_info.modules.module[nr];
>  
> -    prop = fdt_get_property(fdt, node, "reg", NULL);
> +    prop = fdt_get_property(fdt, node, "reg", &len);
>      if ( !prop )
>          early_panic("node %s missing `reg' property\n", name);
>  
> +    if ( len < dt_cells_to_size(address_cells + size_cells) )
> +        early_panic("fdt: node `%s': `reg` property length is too short\n",
> +                    name);
> +
>      cell = (const u32 *)prop->data;
>      device_tree_get_reg(&cell, address_cells, size_cells,
>                          &mod->start, &mod->size);

Re: [PATCH V1 11/29] xen/dts: Check "reg" property length in process_multiboot_node

[Julien Grall]

On 09/06/2013 05:40 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> When the device tree compiler (dtc) can't find right #address-cells
>> and #size-cells, it will assume the encoding is 1 for each.
>
> Without inserting them into the DTB I suppose?

Yes. I have noticed that the encoding is not 1 for each but
"If missing, a client program should assume a default value of 2 for 
#address-cells, and a value of 1 for #size-cells." (ePAR 2.3.5)

dtc will warns with the following messages:
Warning (reg_format): "reg" property in /chosen/modules/module@0 has 
invalid length (8 bytes) (#address-cells == 2, #size-cells == 1)
Warning (avoid_default_addr_size): Relying on default #address-cells 
value for /chosen/modules/module@0
Warning (avoid_default_addr_size): Relying on default #size-cells value 
for /chosen/modules/module@0

>> As multiboot node are inside the /chosen, dtc will asume the previous values
>
> "assume"
>
>> if the both property are not correct set.
>
> "the both property" => "both properties" and "correctly"
>
> By "previous values" do you mean the values set in the (grand)parent
> node?

Yes. I will update the comment.

>> During boot, Xen will browse the fdt and store the nearest #address-cells
>> and #size-cells value. If the size of "reg" is smaller, Xen can retrieve
>> wrong range.
>
> How come Xen can find a suitable #address-cells but dtc cannot? Are we
> not handling them correctly perhaps?

So we have two issues:
   1) Xen doesn't check the length of the reg property (this patch)
   2) Default value for #address-cells and #size-cells are wrong

>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/common/device_tree.c |    6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 9568250..7295f34 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -467,10 +467,14 @@ static void __init process_multiboot_node(const void *fdt, int node,
>>
>>       mod = &early_info.modules.module[nr];
>>
>> -    prop = fdt_get_property(fdt, node, "reg", NULL);
>> +    prop = fdt_get_property(fdt, node, "reg", &len);
>>       if ( !prop )
>>           early_panic("node %s missing `reg' property\n", name);
>>
>> +    if ( len < dt_cells_to_size(address_cells + size_cells) )
>> +        early_panic("fdt: node `%s': `reg` property length is too short\n",
>> +                    name);
>> +
>>       cell = (const u32 *)prop->data;
>>       device_tree_get_reg(&cell, address_cells, size_cells,
>>                           &mod->start, &mod->size);
>
>


-- 
Julien Grall

[PATCH V1 12/29] xen/dts: Check the CPU ID is not greater than NR_CPUS

[Julien Grall]

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

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

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

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

[PATCH V1 13/29] xen/video: hdlcd: Convert the driver to the new device tree API

[Julien Grall]

Avoid to use FDT API which will be removed soon

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Move s/printk/early_printk changes to a new patch
        - hdlcd_start and hdlcd_size are physical address
        - Update early printk to use HDLCD instead of hdlcd
---
 xen/drivers/video/arm_hdlcd.c |   46 +++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 20 deletions(-)

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

Re: [PATCH V1 13/29] xen/video: hdlcd: Convert the driver to the new device tree API

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> Avoid to use FDT API which will be removed soon
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

[PATCH V1 14/29] xen/video: hdlcd: Use early_printk instead of printk

[Julien Grall]

The video driver is initialized before the console is correctly set up.
Therefore, printk will never output if there is no serial configured.

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

diff --git a/xen/drivers/video/arm_hdlcd.c b/xen/drivers/video/arm_hdlcd.c
index ab464c6..dffda9a 100644
--- a/xen/drivers/video/arm_hdlcd.c
+++ b/xen/drivers/video/arm_hdlcd.c
@@ -146,13 +146,13 @@ void __init video_init(void)
 
     if ( !hdlcd_start )
     {
-        printk(KERN_ERR "HDLCD address missing from device tree, disabling driver\n");
+        early_printk(KERN_ERR "HDLCD: address missing from device tree, disabling driver\n");
         return;
     }
 
     if ( !hdlcd_start || !framebuffer_start )
     {
-        printk(KERN_ERR "HDLCD: framebuffer address missing from device tree, disabling driver\n");
+        early_printk(KERN_ERR "HDLCD: framebuffer address missing from device tree, disabling driver\n");
         return;
     }
 
@@ -166,27 +166,27 @@ void __init video_init(void)
     else if ( strlen(mode_string) < strlen("800x600@60") ||
             strlen(mode_string) > sizeof(_mode_string) - 1 )
     {
-        printk(KERN_ERR "HDLCD: invalid modeline=%s\n", mode_string);
+        early_printk(KERN_ERR "HDLCD: invalid modeline=%s\n", mode_string);
         return;
     } else {
         char *s = strchr(mode_string, '-');
         if ( !s )
         {
-            printk(KERN_INFO "HDLCD: bpp not found in modeline %s, assume 32 bpp\n",
-                    mode_string);
+            early_printk(KERN_INFO "HDLCD: bpp not found in modeline %s, assume 32 bpp\n",
+                         mode_string);
             get_color_masks("32", &c);
             memcpy(_mode_string, mode_string, strlen(mode_string) + 1);
             bytes_per_pixel = 4;
         } else {
             if ( strlen(s) < 6 )
             {
-                printk(KERN_ERR "HDLCD: invalid mode %s\n", mode_string);
+                early_printk(KERN_ERR "HDLCD: invalid mode %s\n", mode_string);
                 return;
             }
             s++;
             if ( get_color_masks(s, &c) < 0 )
             {
-                printk(KERN_WARNING "HDLCD: unsupported bpp %s\n", s);
+                early_printk(KERN_WARNING "HDLCD: unsupported bpp %s\n", s);
                 return;
             }
             bytes_per_pixel = simple_strtoll(s, NULL, 10) / 8;
@@ -205,22 +205,23 @@ void __init video_init(void)
     }
     if ( !videomode )
     {
-        printk(KERN_WARNING "HDLCD: unsupported videomode %s\n", _mode_string);
+        early_printk(KERN_WARNING "HDLCD: unsupported videomode %s\n",
+                     _mode_string);
         return;
     }
 
     if ( framebuffer_size < bytes_per_pixel * videomode->xres * videomode->yres )
     {
-        printk(KERN_ERR "HDLCD: the framebuffer is too small, disabling the HDLCD driver\n");
+        early_printk(KERN_ERR "HDLCD: the framebuffer is too small, disabling the HDLCD driver\n");
         return;
     }
 
-    printk(KERN_INFO "Initializing HDLCD driver\n");
+    early_printk(KERN_INFO "Initializing HDLCD driver\n");
 
     lfb = ioremap_wc(framebuffer_start, framebuffer_size);
     if ( !lfb )
     {
-        printk(KERN_ERR "Couldn't map the framebuffer\n");
+        early_printk(KERN_ERR "Couldn't map the framebuffer\n");
         return;
     }
     memset(lfb, 0x00, bytes_per_pixel * videomode->xres * videomode->yres);
-- 
1.7.10.4

Re: [PATCH V1 14/29] xen/video: hdlcd: Use early_printk instead of printk

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> The video driver is initialized before the console is correctly set up.
> Therefore, printk will never output if there is no serial configured.

On the flip side if you have a serial console but not early printk you
won't see these messages instead.

I suppose in that case we don't even try to initialise the hdlcd, since
it will belong to dom0, right? Except I don't see where that happens.
Shouldn't this code only be activated if you have console=hdlcd or
something?

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/drivers/video/arm_hdlcd.c |   23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/drivers/video/arm_hdlcd.c b/xen/drivers/video/arm_hdlcd.c
> index ab464c6..dffda9a 100644
> --- a/xen/drivers/video/arm_hdlcd.c
> +++ b/xen/drivers/video/arm_hdlcd.c
> @@ -146,13 +146,13 @@ void __init video_init(void)
>  
>      if ( !hdlcd_start )
>      {
> -        printk(KERN_ERR "HDLCD address missing from device tree, disabling driver\n");
> +        early_printk(KERN_ERR "HDLCD: address missing from device tree, disabling driver\n");
>          return;
>      }
>  
>      if ( !hdlcd_start || !framebuffer_start )
>      {
> -        printk(KERN_ERR "HDLCD: framebuffer address missing from device tree, disabling driver\n");
> +        early_printk(KERN_ERR "HDLCD: framebuffer address missing from device tree, disabling driver\n");
>          return;
>      }
>  
> @@ -166,27 +166,27 @@ void __init video_init(void)
>      else if ( strlen(mode_string) < strlen("800x600@60") ||
>              strlen(mode_string) > sizeof(_mode_string) - 1 )
>      {
> -        printk(KERN_ERR "HDLCD: invalid modeline=%s\n", mode_string);
> +        early_printk(KERN_ERR "HDLCD: invalid modeline=%s\n", mode_string);
>          return;
>      } else {
>          char *s = strchr(mode_string, '-');
>          if ( !s )
>          {
> -            printk(KERN_INFO "HDLCD: bpp not found in modeline %s, assume 32 bpp\n",
> -                    mode_string);
> +            early_printk(KERN_INFO "HDLCD: bpp not found in modeline %s, assume 32 bpp\n",
> +                         mode_string);
>              get_color_masks("32", &c);
>              memcpy(_mode_string, mode_string, strlen(mode_string) + 1);
>              bytes_per_pixel = 4;
>          } else {
>              if ( strlen(s) < 6 )
>              {
> -                printk(KERN_ERR "HDLCD: invalid mode %s\n", mode_string);
> +                early_printk(KERN_ERR "HDLCD: invalid mode %s\n", mode_string);
>                  return;
>              }
>              s++;
>              if ( get_color_masks(s, &c) < 0 )
>              {
> -                printk(KERN_WARNING "HDLCD: unsupported bpp %s\n", s);
> +                early_printk(KERN_WARNING "HDLCD: unsupported bpp %s\n", s);
>                  return;
>              }
>              bytes_per_pixel = simple_strtoll(s, NULL, 10) / 8;
> @@ -205,22 +205,23 @@ void __init video_init(void)
>      }
>      if ( !videomode )
>      {
> -        printk(KERN_WARNING "HDLCD: unsupported videomode %s\n", _mode_string);
> +        early_printk(KERN_WARNING "HDLCD: unsupported videomode %s\n",
> +                     _mode_string);
>          return;
>      }
>  
>      if ( framebuffer_size < bytes_per_pixel * videomode->xres * videomode->yres )
>      {
> -        printk(KERN_ERR "HDLCD: the framebuffer is too small, disabling the HDLCD driver\n");
> +        early_printk(KERN_ERR "HDLCD: the framebuffer is too small, disabling the HDLCD driver\n");
>          return;
>      }
>  
> -    printk(KERN_INFO "Initializing HDLCD driver\n");
> +    early_printk(KERN_INFO "Initializing HDLCD driver\n");
>  
>      lfb = ioremap_wc(framebuffer_start, framebuffer_size);
>      if ( !lfb )
>      {
> -        printk(KERN_ERR "Couldn't map the framebuffer\n");
> +        early_printk(KERN_ERR "Couldn't map the framebuffer\n");
>          return;
>      }
>      memset(lfb, 0x00, bytes_per_pixel * videomode->xres * videomode->yres);

Re: [PATCH V1 14/29] xen/video: hdlcd: Use early_printk instead of printk

[Julien Grall]

On 09/06/2013 05:48 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> The video driver is initialized before the console is correctly set up.
>> Therefore, printk will never output if there is no serial configured.
>
> On the flip side if you have a serial console but not early printk you
> won't see these messages instead.

It depends, the console code will parse the console string and 
initialize the different console driver (serial, vga,...) one by one.
   - console=vga,serial => the VGA needs to use early printk
   - console=vga        => the VGA needs to use early printk
   - console=serial,vga => the VGA needs to use printk and can use early 
printk if it's enabled.

>
> I suppose in that case we don't even try to initialise the hdlcd, since
> it will belong to dom0, right? Except I don't see where that happens.
> Shouldn't this code only be activated if you have console=hdlcd or
> something?

For the moment, the hdlcd is the default device for vga. So if 
console=vga, Xen will initialize the hdcld by calling video_init().
We will need to implement something similar to dt-uart.

>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/drivers/video/arm_hdlcd.c |   23 ++++++++++++-----------
>>   1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/drivers/video/arm_hdlcd.c b/xen/drivers/video/arm_hdlcd.c
>> index ab464c6..dffda9a 100644
>> --- a/xen/drivers/video/arm_hdlcd.c
>> +++ b/xen/drivers/video/arm_hdlcd.c
>> @@ -146,13 +146,13 @@ void __init video_init(void)
>>
>>       if ( !hdlcd_start )
>>       {
>> -        printk(KERN_ERR "HDLCD address missing from device tree, disabling driver\n");
>> +        early_printk(KERN_ERR "HDLCD: address missing from device tree, disabling driver\n");
>>           return;
>>       }
>>
>>       if ( !hdlcd_start || !framebuffer_start )
>>       {
>> -        printk(KERN_ERR "HDLCD: framebuffer address missing from device tree, disabling driver\n");
>> +        early_printk(KERN_ERR "HDLCD: framebuffer address missing from device tree, disabling driver\n");
>>           return;
>>       }
>>
>> @@ -166,27 +166,27 @@ void __init video_init(void)
>>       else if ( strlen(mode_string) < strlen("800x600@60") ||
>>               strlen(mode_string) > sizeof(_mode_string) - 1 )
>>       {
>> -        printk(KERN_ERR "HDLCD: invalid modeline=%s\n", mode_string);
>> +        early_printk(KERN_ERR "HDLCD: invalid modeline=%s\n", mode_string);
>>           return;
>>       } else {
>>           char *s = strchr(mode_string, '-');
>>           if ( !s )
>>           {
>> -            printk(KERN_INFO "HDLCD: bpp not found in modeline %s, assume 32 bpp\n",
>> -                    mode_string);
>> +            early_printk(KERN_INFO "HDLCD: bpp not found in modeline %s, assume 32 bpp\n",
>> +                         mode_string);
>>               get_color_masks("32", &c);
>>               memcpy(_mode_string, mode_string, strlen(mode_string) + 1);
>>               bytes_per_pixel = 4;
>>           } else {
>>               if ( strlen(s) < 6 )
>>               {
>> -                printk(KERN_ERR "HDLCD: invalid mode %s\n", mode_string);
>> +                early_printk(KERN_ERR "HDLCD: invalid mode %s\n", mode_string);
>>                   return;
>>               }
>>               s++;
>>               if ( get_color_masks(s, &c) < 0 )
>>               {
>> -                printk(KERN_WARNING "HDLCD: unsupported bpp %s\n", s);
>> +                early_printk(KERN_WARNING "HDLCD: unsupported bpp %s\n", s);
>>                   return;
>>               }
>>               bytes_per_pixel = simple_strtoll(s, NULL, 10) / 8;
>> @@ -205,22 +205,23 @@ void __init video_init(void)
>>       }
>>       if ( !videomode )
>>       {
>> -        printk(KERN_WARNING "HDLCD: unsupported videomode %s\n", _mode_string);
>> +        early_printk(KERN_WARNING "HDLCD: unsupported videomode %s\n",
>> +                     _mode_string);
>>           return;
>>       }
>>
>>       if ( framebuffer_size < bytes_per_pixel * videomode->xres * videomode->yres )
>>       {
>> -        printk(KERN_ERR "HDLCD: the framebuffer is too small, disabling the HDLCD driver\n");
>> +        early_printk(KERN_ERR "HDLCD: the framebuffer is too small, disabling the HDLCD driver\n");
>>           return;
>>       }
>>
>> -    printk(KERN_INFO "Initializing HDLCD driver\n");
>> +    early_printk(KERN_INFO "Initializing HDLCD driver\n");
>>
>>       lfb = ioremap_wc(framebuffer_start, framebuffer_size);
>>       if ( !lfb )
>>       {
>> -        printk(KERN_ERR "Couldn't map the framebuffer\n");
>> +        early_printk(KERN_ERR "Couldn't map the framebuffer\n");
>>           return;
>>       }
>>       memset(lfb, 0x00, bytes_per_pixel * videomode->xres * videomode->yres);
>
>


-- 
Julien Grall

[PATCH V1 15/29] xen/arm: Use dt_device_match to avoid multiple if conditions

[Julien Grall]

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

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Move skip_match and timer_ids within the appropriate functions
---
 xen/arch/arm/domain_build.c |   13 ++++++++++---
 xen/arch/arm/time.c         |   10 +++++++---
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 345f0f7..eb8b662 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -437,15 +437,22 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
 
 static int handle_node(struct domain *d, const struct dt_device_node *np)
 {
+    static const struct dt_device_match skip_matches[] __initconst =
+    {
+        DT_MATCH_COMPATIBLE("xen,xen"),
+        DT_MATCH_TYPE("memory"),
+        DT_MATCH_PATH("/chosen"),
+        { /* sentinel */ },
+    };
     const struct dt_device_node *child;
     int res;
 
+
+
     DPRINT("handle %s\n", dt_node_full_name(np));
 
     /* Skip theses nodes and the sub-nodes */
-    if ( dt_device_is_compatible(np, "xen,xen") ||
-         dt_device_type_is_equal(np, "memory") ||
-         !strcmp("/chosen", dt_node_full_name(np)) )
+    if ( dt_match_node(skip_matches, np ) )
         return 0;
 
     if ( dt_device_used_by(np) != DOMID_XEN )
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 9c176cd..8125b92 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -101,14 +101,18 @@ static uint32_t calibrate_timer(void)
 /* Set up the timer on the boot CPU */
 int __init init_xen_time(void)
 {
+    static const struct dt_device_match timer_ids[] __initconst =
+    {
+        DT_MATCH_COMPATIBLE("arm,armv7-timer"),
+        DT_MATCH_COMPATIBLE("arm,armv8-timer"),
+        { /* sentinel */ },
+    };
     struct dt_device_node *dev;
     int res;
     unsigned int i;
     u32 rate;
 
-    dev = dt_find_compatible_node(NULL, NULL, "arm,armv7-timer");
-    if ( !dev )
-        dev = dt_find_compatible_node(NULL, NULL, "arm,armv8-timer");
+    dev = dt_find_matching_node(NULL, timer_ids);
     if ( !dev )
         panic("Unable to find a compatible timer in the device tree\n");
 
-- 
1.7.10.4

Re: [PATCH V1 15/29] xen/arm: Use dt_device_match to avoid multiple if conditions

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> There is some place in Xen ARM code where multiple if conditions is used
> check the presence of a node or find a node.
> These pieces of code can be replace by an array and using proper device tree
> helpers.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

> 
> ---
>     Changes in v2:
>         - Move skip_match and timer_ids within the appropriate functions
> ---
>  xen/arch/arm/domain_build.c |   13 ++++++++++---
>  xen/arch/arm/time.c         |   10 +++++++---
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 345f0f7..eb8b662 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -437,15 +437,22 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
>  
>  static int handle_node(struct domain *d, const struct dt_device_node *np)
>  {
> +    static const struct dt_device_match skip_matches[] __initconst =
> +    {
> +        DT_MATCH_COMPATIBLE("xen,xen"),
> +        DT_MATCH_TYPE("memory"),
> +        DT_MATCH_PATH("/chosen"),
> +        { /* sentinel */ },
> +    };
>      const struct dt_device_node *child;
>      int res;
>  
> +
> +
>      DPRINT("handle %s\n", dt_node_full_name(np));
>  
>      /* Skip theses nodes and the sub-nodes */
> -    if ( dt_device_is_compatible(np, "xen,xen") ||
> -         dt_device_type_is_equal(np, "memory") ||
> -         !strcmp("/chosen", dt_node_full_name(np)) )
> +    if ( dt_match_node(skip_matches, np ) )
>          return 0;
>  
>      if ( dt_device_used_by(np) != DOMID_XEN )
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 9c176cd..8125b92 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -101,14 +101,18 @@ static uint32_t calibrate_timer(void)
>  /* Set up the timer on the boot CPU */
>  int __init init_xen_time(void)
>  {
> +    static const struct dt_device_match timer_ids[] __initconst =
> +    {
> +        DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> +        DT_MATCH_COMPATIBLE("arm,armv8-timer"),
> +        { /* sentinel */ },
> +    };
>      struct dt_device_node *dev;
>      int res;
>      unsigned int i;
>      u32 rate;
>  
> -    dev = dt_find_compatible_node(NULL, NULL, "arm,armv7-timer");
> -    if ( !dev )
> -        dev = dt_find_compatible_node(NULL, NULL, "arm,armv8-timer");
> +    dev = dt_find_matching_node(NULL, timer_ids);
>      if ( !dev )
>          panic("Unable to find a compatible timer in the device tree\n");
>  

[PATCH V1 16/29] xen/arm: Build DOM0 FDT by browsing the device tree structure

[Julien Grall]

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

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - use dt_set_range
        - add set_interrupts
        - add more debug print
        - add comment to explain why some device are not mapped
        - mix kbasename directly in handle_node
        - fix malloc check when memory cells are allocated
---
 xen/arch/arm/domain_build.c |  310 ++++++++++++++++++++-----------------------
 1 file changed, 142 insertions(+), 168 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index eb8b662..76decf4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -63,10 +63,10 @@ struct vcpu *__init alloc_dom0_vcpu0(void)
 }
 
 static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo,
-                             const void *fdt, const u32 *cell, int len,
-                             int address_cells, int size_cells, u32 *new_cell)
+                             const struct dt_property *pp,
+                             const struct dt_device_node *np, __be32 *new_cell)
 {
-    int reg_size = (address_cells + size_cells) * sizeof(*cell);
+    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np));
     paddr_t start;
     paddr_t size;
     struct page_info *pg;
@@ -90,7 +90,7 @@ static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo,
     if ( res )
         panic("Unable to add pages in DOM0: %d\n", res);
 
-    device_tree_set_reg(&new_cell, address_cells, size_cells, start, size);
+    dt_set_range(&new_cell, np, start, size);
 
     kinfo->mem.bank[0].start = start;
     kinfo->mem.bank[0].size = size;
@@ -100,25 +100,30 @@ static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo,
 }
 
 static int set_memory_reg(struct domain *d, struct kernel_info *kinfo,
-                          const void *fdt, const u32 *cell, int len,
-                          int address_cells, int size_cells, u32 *new_cell)
+                          const struct dt_property *pp,
+                          const struct dt_device_node *np, __be32 *new_cell)
 {
-    int reg_size = (address_cells + size_cells) * sizeof(*cell);
+    int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np));
     int l = 0;
+    unsigned int bank = 0;
     u64 start;
     u64 size;
+    int ret;
 
     if ( platform_has_quirk(PLATFORM_QUIRK_DOM0_MAPPING_11) )
-        return set_memory_reg_11(d, kinfo, fdt, cell, len, address_cells,
-                                 size_cells, new_cell);
+        return set_memory_reg_11(d, kinfo, pp, np, new_cell);
 
-    while ( kinfo->unassigned_mem > 0 && l + reg_size <= len
+    while ( kinfo->unassigned_mem > 0 && l + reg_size <= pp->length
             && kinfo->mem.nr_banks < NR_MEM_BANKS )
     {
-        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
+        ret = dt_device_get_address(np, bank, &start, &size);
+        if ( ret )
+            panic("Unable to retrieve the bank %u for %s\n",
+                  bank, dt_node_full_name(np));
+
         if ( size > kinfo->unassigned_mem )
             size = kinfo->unassigned_mem;
-        device_tree_set_reg(&new_cell, address_cells, size_cells, start, size);
+        dt_set_range(&new_cell, np, start, size);
 
         printk("Populate P2M %#"PRIx64"->%#"PRIx64"\n", start, start + size);
         if ( p2m_populate_ram(d, start, start + size) < 0 )
@@ -135,31 +140,21 @@ static int set_memory_reg(struct domain *d, struct kernel_info *kinfo,
 }
 
 static int write_properties(struct domain *d, struct kernel_info *kinfo,
-                            const void *fdt,
-                            int node, const char *name, int depth,
-                            u32 address_cells, u32 size_cells)
+                            const struct dt_device_node *np)
 {
     const char *bootargs = NULL;
-    int prop;
+    const struct dt_property *pp;
+    int res = 0;
 
     if ( early_info.modules.nr_mods >= MOD_KERNEL &&
          early_info.modules.module[MOD_KERNEL].cmdline[0] )
         bootargs = &early_info.modules.module[MOD_KERNEL].cmdline[0];
 
-    for ( prop = fdt_first_property_offset(fdt, node);
-          prop >= 0;
-          prop = fdt_next_property_offset(fdt, prop) )
+    for_each_property_of_node (np, pp)
     {
-        const struct fdt_property *p;
-        const char *prop_name;
-        const char *prop_data;
-        int prop_len;
-        char *new_data = NULL;
-
-        p = fdt_get_property_by_offset(fdt, prop, NULL);
-        prop_name = fdt_string(fdt, fdt32_to_cpu(p->nameoff));
-        prop_data = p->data;
-        prop_len  = fdt32_to_cpu(p->len);
+        const void *prop_data = pp->value;
+        void *new_data = NULL;
+        u32 prop_len = pp->length;
 
         /*
          * In chosen node:
@@ -168,105 +163,93 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
          *   bootargs (from module #1, above).
          * * remove bootargs and xen,dom0-bootargs.
          */
-        if ( device_tree_node_matches(fdt, node, "chosen") )
+        if ( dt_node_path_is_equal(np, "/chosen") )
         {
-            if ( strcmp(prop_name, "bootargs") == 0 )
+            if ( dt_property_name_is_equal(pp, "bootargs") )
                 continue;
-            else if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
+            else if ( dt_property_name_is_equal(pp, "xen,dom0-bootargs") )
             {
                 if ( !bootargs )
-                    bootargs = prop_data;
+                    bootargs = pp->value;
                 continue;
             }
         }
         /*
          * In a memory node: adjust reg property.
+         * TODO: handle properly memory node (ie: device_type = "memory")
          */
-        else if ( device_tree_node_matches(fdt, node, "memory") )
+        else if ( dt_node_name_is_equal(np, "memory") )
         {
-            if ( strcmp(prop_name, "reg") == 0 )
+            if ( dt_property_name_is_equal(pp, "reg") )
             {
-                new_data = xzalloc_bytes(prop_len);
+                new_data = xzalloc_bytes(pp->length);
                 if ( new_data  == NULL )
                     return -FDT_ERR_XEN(ENOMEM);
 
-                prop_len = set_memory_reg(d, kinfo, fdt,
-                                          (u32 *)prop_data, prop_len,
-                                          address_cells, size_cells,
-                                          (u32 *)new_data);
+                prop_len = set_memory_reg(d, kinfo, pp, np,
+                                          (__be32 *)new_data);
                 prop_data = new_data;
             }
         }
 
-        /*
-         * TODO: Should call map_mmio_regions() for all devices in the
-         * tree that have a "reg" parameter (except cpus).  This
-         * requires looking into the parent node's "ranges" property
-         * to translate the bus address in the "reg" value into
-         * physical addresses.  Regions also need to be rounded up to
-         * whole pages.
-         */
-
-        fdt_property(kinfo->fdt, prop_name, prop_data, prop_len);
+        res = fdt_property(kinfo->fdt, pp->name, prop_data, prop_len);
 
         xfree(new_data);
+
+        if ( res )
+            return res;
     }
 
-    if ( device_tree_node_matches(fdt, node, "chosen") && bootargs )
-        fdt_property(kinfo->fdt, "bootargs", bootargs, strlen(bootargs) + 1);
+    if ( dt_node_path_is_equal(np, "/chosen") && bootargs )
+    {
+        res = fdt_property(kinfo->fdt, "bootargs", bootargs,
+                           strlen(bootargs) + 1);
+        if ( res )
+            return res;
+    }
 
     /*
      * XXX should populate /chosen/linux,initrd-{start,end} here if we
      * have module[2]
      */
 
-    if ( prop == -FDT_ERR_NOTFOUND )
-        return 0;
-    return prop;
+    return 0;
 }
 
-/* Returns the next node in fdt (starting from offset) which should be
- * passed through to dom0.
+/*
+ * Helper to write an interrupts with the GIC format
+ * This code is assuming the irq is an PPI.
+ * TODO: Handle SPI
  */
-static int fdt_next_dom0_node(const void *fdt, int node,
-                              int *depth_out)
-{
-    int depth = *depth_out;
-
-    while ( (node = fdt_next_node(fdt, node, &depth)) &&
-            node >= 0 && depth >= 0 )
-    {
-        if ( depth >= DEVICE_TREE_MAX_DEPTH )
-            break;
 
-        /* Skip /hypervisor/ node. We will inject our own. */
-        if ( fdt_node_check_compatible(fdt, node, "xen,xen" ) == 0 )
-        {
-            printk("Device-tree contains \"xen,xen\" node. Ignoring.\n");
-            continue;
-        }
+typedef __be32 interrupt_t[3];
 
-        /* Skip multiboot subnodes */
-        if ( fdt_node_check_compatible(fdt, node,
-                                       "xen,multiboot-module" ) == 0 )
-            continue;
+static void set_interrupts(interrupt_t interrupt, unsigned int irq,
+                           unsigned int cpumask, unsigned int level)
+{
+    __be32 *cells = interrupt;
 
-        /* We've arrived at a node which dom0 is interested in. */
-        break;
-    }
+    BUG_ON(irq < 16 && irq >= 32);
 
-    *depth_out = depth;
-    return node;
+    /* See linux Documentation/devictree/bindings/arm/gic.txt */
+    dt_set_cell(&cells, 1, 1); /* is a PPI */
+    dt_set_cell(&cells, 1, irq - 16); /* PPIs start at 16 */
+    dt_set_cell(&cells, 1, (cpumask << 8) | level);
 }
 
-static void make_hypervisor_node(void *fdt, int addrcells, int sizecells)
+static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent)
 {
     const char compat[] =
         "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
         "xen,xen";
-    u32 reg[4];
-    u32 intr[3];
-    u32 *cell;
+    __be32 reg[4];
+    interrupt_t intr[1];
+    __be32 *cells;
+    int res;
+    int addrcells = dt_n_addr_cells(parent);
+    int sizecells = dt_n_size_cells(parent);
+
+    DPRINT("Create hypervisor node\n");
 
     /*
      * Sanity-check address sizes, since addresses and sizes which do
@@ -277,82 +260,42 @@ static void make_hypervisor_node(void *fdt, int addrcells, int sizecells)
         panic("Cannot cope with this size");
 
     /* See linux Documentation/devicetree/bindings/arm/xen.txt */
-    fdt_begin_node(fdt, "hypervisor");
+    res = fdt_begin_node(fdt, "hypervisor");
+    if ( res )
+        return res;
 
     /* Cannot use fdt_property_string due to embedded nulls */
-    fdt_property(fdt, "compatible", compat, sizeof(compat));
+    res = fdt_property(fdt, "compatible", compat, sizeof(compat));
+    if ( res )
+        return res;
 
+    DPRINT("  Grant table range: 0xb0000000-0x20000\n");
     /* reg 0 is grant table space */
-    cell = &reg[0];
-    device_tree_set_reg(&cell, addrcells, sizecells, 0xb0000000, 0x20000);
-    fdt_property(fdt, "reg", reg,
-                 sizeof(reg[0]) * (addrcells + sizecells));
+    cells = &reg[0];
+    dt_set_range(&cells, parent, 0xb0000000, 0x20000);
+    res = fdt_property(fdt, "reg", reg,
+                       dt_cells_to_size(addrcells + sizecells));
+    if ( res )
+        return res;
 
     /*
-     * interrupts is evtchn upcall  <1 15 0xf08>
-     * See linux Documentation/devicetree/bindings/arm/gic.txt
+     * interrupts is evtchn upcall:
+     *  - Active-low level-sensitive
+     *  - All cpus
+     *
+     * TODO: Handle correctly the cpumask
      */
-    intr[0] = cpu_to_fdt32(1); /* is a PPI */
-    intr[1] = cpu_to_fdt32(VGIC_IRQ_EVTCHN_CALLBACK - 16); /* PPIs start at 16 */
-    intr[2] = cpu_to_fdt32(0xf08); /* Active-low level-sensitive */
-
-    fdt_property(fdt, "interrupts", intr, sizeof(intr[0]) * 3);
-
-    fdt_end_node(fdt);
-}
-
-static int write_nodes(struct domain *d, struct kernel_info *kinfo,
-                       const void *fdt)
-{
-    int node;
-    int depth = 0, last_depth = -1;
-    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
-    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
-    int ret;
-
-    for ( node = 0, depth = 0;
-          node >= 0 && depth >= 0;
-          node = fdt_next_dom0_node(fdt, node, &depth) )
-    {
-        const char *name;
-
-        name = fdt_get_name(fdt, node, NULL);
-
-        if ( depth >= DEVICE_TREE_MAX_DEPTH )
-        {
-            printk("warning: node `%s' is nested too deep (%d)\n",
-                   name, depth);
-            continue;
-        }
-
-        /* We cannot handle descending more than one level at a time */
-        ASSERT( depth <= last_depth + 1 );
-
-        while ( last_depth-- >= depth )
-            fdt_end_node(kinfo->fdt);
-
-        address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells",
-                                    depth > 0 ? address_cells[depth-1] : 0);
-        size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells",
-                                    depth > 0 ? size_cells[depth-1] : 0);
-
-        fdt_begin_node(kinfo->fdt, name);
+    DPRINT("  Event channel interrupt to %u\n", VGIC_IRQ_EVTCHN_CALLBACK);
+    set_interrupts(intr[0], VGIC_IRQ_EVTCHN_CALLBACK, 0xf,
+                   DT_IRQ_TYPE_LEVEL_LOW);
 
-        ret = write_properties(d, kinfo, fdt, node, name, depth,
-                               address_cells[depth-1], size_cells[depth-1]);
-        if ( ret < 0 )
-            return ret;
-
-        last_depth = depth;
-    }
-
-    while ( last_depth-- >= 1 )
-        fdt_end_node(kinfo->fdt);
+    res = fdt_property(fdt, "interrupts", intr, sizeof(intr[0]));
+    if ( res )
+        return res;
 
-    make_hypervisor_node(kinfo->fdt, address_cells[0], size_cells[0]);
+    res = fdt_end_node(fdt);
 
-    fdt_end_node(kinfo->fdt);
-    return 0;
+    return res;
 }
 
 /* Map the device in the domain */
@@ -435,27 +378,39 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
     return 0;
 }
 
-static int handle_node(struct domain *d, const struct dt_device_node *np)
+static int handle_node(struct domain *d, struct kernel_info *kinfo,
+                       const struct dt_device_node *np)
 {
     static const struct dt_device_match skip_matches[] __initconst =
     {
         DT_MATCH_COMPATIBLE("xen,xen"),
-        DT_MATCH_TYPE("memory"),
-        DT_MATCH_PATH("/chosen"),
+        DT_MATCH_COMPATIBLE("xen,multiboot-module"),
         { /* sentinel */ },
     };
     const struct dt_device_node *child;
     int res;
+    const char *name;
+    const char *path;
 
+    path = dt_node_full_name(np);
 
-
-    DPRINT("handle %s\n", dt_node_full_name(np));
+    DPRINT("handle %s\n", path);
 
     /* Skip theses nodes and the sub-nodes */
     if ( dt_match_node(skip_matches, np ) )
+    {
+        DPRINT("  Skip it!\n");
         return 0;
+    }
 
-    if ( dt_device_used_by(np) != DOMID_XEN )
+    /*
+     * Some device doesn't need to be mapped in Xen:
+     *  - Device used by Xen: Obviously dom0 can't use them
+     *  - Memory: the guest will see a different view of memory. It will
+     *  be allocated later.
+     */
+    if ( dt_device_used_by(np) != DOMID_XEN &&
+         !dt_device_type_is_equal(np, "memory") )
     {
         res = map_device(d, np);
 
@@ -463,21 +418,39 @@ static int handle_node(struct domain *d, const struct dt_device_node *np)
             return res;
     }
 
+    /*
+     * The property "name" is used to have a different name on older FDT
+     * version. We want to keep the name retrieved during the tree
+     * structure creation, that is store in the node path.
+     */
+    name = strrchr(path, '/');
+    name = name ? name + 1 : path;
+
+    res = fdt_begin_node(kinfo->fdt, name);
+    if ( res )
+        return res;
+
+    res = write_properties(d, kinfo, np);
+    if ( res )
+        return res;
+
     for ( child = np->child; child != NULL; child = child->sibling )
     {
-        res = handle_node(d, child);
+        res = handle_node(d, kinfo, child);
         if ( res )
             return res;
     }
 
-    return 0;
-}
+    if ( np == dt_host )
+    {
+        res = make_hypervisor_node(kinfo->fdt, np);
+        if ( res )
+            return res;
+    }
 
-static int map_devices_from_device_tree(struct domain *d)
-{
-    ASSERT(dt_host && (dt_host->sibling == NULL));
+    res = fdt_end_node(kinfo->fdt);
 
-    return handle_node(d, dt_host);
+    return res;
 }
 
 static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
@@ -487,6 +460,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
     int ret;
     paddr_t end;
 
+    ASSERT(dt_host && (dt_host->sibling == NULL));
+
     kinfo->unassigned_mem = dom0_mem;
 
     fdt = device_tree_flattened;
@@ -502,8 +477,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
 
     fdt_finish_reservemap(kinfo->fdt);
 
-    ret = write_nodes(d, kinfo, fdt);
-    if ( ret < 0 )
+    ret = handle_node(d, kinfo, dt_host);
+    if ( ret )
         goto err;
 
     ret = fdt_finish(kinfo->fdt);
@@ -575,7 +550,6 @@ int construct_dom0(struct domain *d)
     if ( rc < 0 )
         return rc;
 
-    map_devices_from_device_tree(d);
     rc = platform_specific_mapping(d);
     if ( rc < 0 )
         return rc;
-- 
1.7.10.4

Re: [PATCH V1 16/29] xen/arm: Build DOM0 FDT by browsing the device tree structure

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>      if ( early_info.modules.nr_mods >= MOD_KERNEL &&
>           early_info.modules.module[MOD_KERNEL].cmdline[0] )
>          bootargs = &early_info.modules.module[MOD_KERNEL].cmdline[0];
>  
> -    for ( prop = fdt_first_property_offset(fdt, node);
> -          prop >= 0;
> -          prop = fdt_next_property_offset(fdt, prop) )
> +    for_each_property_of_node (np, pp)

Is "of" here as in "the property of the node" or is it a stray Open
Firmware from the Linux naming of these functions?

Perhaps a dt_ prefix to match all the others?

> -/* Returns the next node in fdt (starting from offset) which should be
> - * passed through to dom0.
> +/*
> + * Helper to write an interrupts with the GIC format
> + * This code is assuming the irq is an PPI.
> + * TODO: Handle SPI
>   */
> -static int fdt_next_dom0_node(const void *fdt, int node,
> -                              int *depth_out)
> -{
> -    int depth = *depth_out;
> -
> -    while ( (node = fdt_next_node(fdt, node, &depth)) &&
> -            node >= 0 && depth >= 0 )
> -    {
> -        if ( depth >= DEVICE_TREE_MAX_DEPTH )
> -            break;
>  
> -        /* Skip /hypervisor/ node. We will inject our own. */
> -        if ( fdt_node_check_compatible(fdt, node, "xen,xen" ) == 0 )
> -        {
> -            printk("Device-tree contains \"xen,xen\" node. Ignoring.\n");
> -            continue;
> -        }
> +typedef __be32 interrupt_t[3];
>  
> -        /* Skip multiboot subnodes */
> -        if ( fdt_node_check_compatible(fdt, node,
> -                                       "xen,multiboot-module" ) == 0 )
> -            continue;
> +static void set_interrupts(interrupt_t interrupt, unsigned int irq,
> +                           unsigned int cpumask, unsigned int level)
> +{
> +    __be32 *cells = interrupt;

This function and the interrupt_t type is only used for the
event-channel ppi? I think gic_interrupt_t would be a better typedef
name

The function name is plural but I think it only handles one interrupt at
a time, and it only handles PPIs?

Unless there are other users of this code I think it could continue to
be inside make_hypervisor_node, given that it is pretty particular
already.

>  
> -        /* We've arrived at a node which dom0 is interested in. */
> -        break;
> -    }
> +    BUG_ON(irq < 16 && irq >= 32);
>  
> -    *depth_out = depth;
> -    return node;
> +    /* See linux Documentation/devictree/bindings/arm/gic.txt */

devicetree.

> +    dt_set_cell(&cells, 1, 1); /* is a PPI */
> +    dt_set_cell(&cells, 1, irq - 16); /* PPIs start at 16 */
> +    dt_set_cell(&cells, 1, (cpumask << 8) | level);
>  }
>  
> -static void make_hypervisor_node(void *fdt, int addrcells, int sizecells)
> +static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent)
>  {
>      const char compat[] =
>          "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
>          "xen,xen";
> -    u32 reg[4];
> -    u32 intr[3];
> -    u32 *cell;
> +    __be32 reg[4];
> +    interrupt_t intr[1];

A single element array seems a bit pointless in this context.

> @@ -463,21 +418,39 @@ static int handle_node(struct domain *d, const struct dt_device_node *np)
>              return res;
>      }
>  
> +    /*
> +     * The property "name" is used to have a different name on older FDT

"is used" => "used"

> +     * version. We want to keep the name retrieved during the tree
> +     * structure creation, that is store in the node path.

"stored"

This comment is saying that the name of the name property used to be
something else? What was it? Which version of FDT was that -- do we need
to care?

Ian.

Re: [PATCH V1 16/29] xen/arm: Build DOM0 FDT by browsing the device tree structure

[Julien Grall]

On 09/09/2013 12:33 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>>       if ( early_info.modules.nr_mods >= MOD_KERNEL &&
>>            early_info.modules.module[MOD_KERNEL].cmdline[0] )
>>           bootargs = &early_info.modules.module[MOD_KERNEL].cmdline[0];
>>
>> -    for ( prop = fdt_first_property_offset(fdt, node);
>> -          prop >= 0;
>> -          prop = fdt_next_property_offset(fdt, prop) )
>> +    for_each_property_of_node (np, pp)
>
> Is "of" here as in "the property of the node" or is it a stray Open
> Firmware from the Linux naming of these functions?
>
> Perhaps a dt_ prefix to match all the others?

Right. I will send a patch to rename for_each_property_of_node to 
dt_for_each_property_of_node.

>
>> -/* Returns the next node in fdt (starting from offset) which should be
>> - * passed through to dom0.
>> +/*
>> + * Helper to write an interrupts with the GIC format
>> + * This code is assuming the irq is an PPI.
>> + * TODO: Handle SPI
>>    */
>> -static int fdt_next_dom0_node(const void *fdt, int node,
>> -                              int *depth_out)
>> -{
>> -    int depth = *depth_out;
>> -
>> -    while ( (node = fdt_next_node(fdt, node, &depth)) &&
>> -            node >= 0 && depth >= 0 )
>> -    {
>> -        if ( depth >= DEVICE_TREE_MAX_DEPTH )
>> -            break;
>>
>> -        /* Skip /hypervisor/ node. We will inject our own. */
>> -        if ( fdt_node_check_compatible(fdt, node, "xen,xen" ) == 0 )
>> -        {
>> -            printk("Device-tree contains \"xen,xen\" node. Ignoring.\n");
>> -            continue;
>> -        }
>> +typedef __be32 interrupt_t[3];
>>
>> -        /* Skip multiboot subnodes */
>> -        if ( fdt_node_check_compatible(fdt, node,
>> -                                       "xen,multiboot-module" ) == 0 )
>> -            continue;
>> +static void set_interrupts(interrupt_t interrupt, unsigned int irq,
>> +                           unsigned int cpumask, unsigned int level)
>> +{
>> +    __be32 *cells = interrupt;
>
> This function and the interrupt_t type is only used for the
> event-channel ppi? I think gic_interrupt_t would be a better typedef
> name
> The function name is plural but I think it only handles one interrupt at
> a time, and it only handles PPIs?
>
> Unless there are other users of this code I think it could continue to
> be inside make_hypervisor_node, given that it is pretty particular
> already.

This function is used in another patch later. I will rename the 
different name in the next patch series.

>>
>> -        /* We've arrived at a node which dom0 is interested in. */
>> -        break;
>> -    }
>> +    BUG_ON(irq < 16 && irq >= 32);
>>
>> -    *depth_out = depth;
>> -    return node;
>> +    /* See linux Documentation/devictree/bindings/arm/gic.txt */
>
> devicetree.
>
>> +    dt_set_cell(&cells, 1, 1); /* is a PPI */
>> +    dt_set_cell(&cells, 1, irq - 16); /* PPIs start at 16 */
>> +    dt_set_cell(&cells, 1, (cpumask << 8) | level);
>>   }
>>
>> -static void make_hypervisor_node(void *fdt, int addrcells, int sizecells)
>> +static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent)
>>   {
>>       const char compat[] =
>>           "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
>>           "xen,xen";
>> -    u32 reg[4];
>> -    u32 intr[3];
>> -    u32 *cell;
>> +    __be32 reg[4];
>> +    interrupt_t intr[1];
>
> A single element array seems a bit pointless in this context.
>
>> @@ -463,21 +418,39 @@ static int handle_node(struct domain *d, const struct dt_device_node *np)
>>               return res;
>>       }
>>
>> +    /*
>> +     * The property "name" is used to have a different name on older FDT
>
> "is used" => "used"
>
>> +     * version. We want to keep the name retrieved during the tree
>> +     * structure creation, that is store in the node path.
>
> "stored"
>
> This comment is saying that the name of the name property used to be
> something else? What was it? Which version of FDT was that -- do we need
> to care?

Right, on older FDT version (< 0x10) each node has 2 different name:
   - the name just after FDT_BEGIN_NODE in the fdt which correspond to 
the "filename".
   - the name in property "name" which is a convenient name.

So we can't use the name field in device tree to retrieve the name to 
create the node.

For the FDT version, I don't know if we need to care. Linux pays 
attention to it in the device tree code.

-- 
Julien Grall

Re: [PATCH V1 16/29] xen/arm: Build DOM0 FDT by browsing the device tree structure

[Ian Campbell]

On Mon, 2013-09-09 at 13:26 +0100, Julien Grall wrote:
> On 09/09/2013 12:33 PM, Ian Campbell wrote:
> > On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> >>       if ( early_info.modules.nr_mods >= MOD_KERNEL &&
> >>            early_info.modules.module[MOD_KERNEL].cmdline[0] )
> >>           bootargs = &early_info.modules.module[MOD_KERNEL].cmdline[0];
> >>
> >> -    for ( prop = fdt_first_property_offset(fdt, node);
> >> -          prop >= 0;
> >> -          prop = fdt_next_property_offset(fdt, prop) )
> >> +    for_each_property_of_node (np, pp)
> >
> > Is "of" here as in "the property of the node" or is it a stray Open
> > Firmware from the Linux naming of these functions?
> >
> > Perhaps a dt_ prefix to match all the others?
> 
> Right. I will send a patch to rename for_each_property_of_node to 
> dt_for_each_property_of_node.

Might dt_for_each_node_property fit better with the other functions?

> >
> > This comment is saying that the name of the name property used to be
> > something else? What was it? Which version of FDT was that -- do we need
> > to care?
> 
> Right, on older FDT version (< 0x10) each node has 2 different name:
>    - the name just after FDT_BEGIN_NODE in the fdt which correspond to 
> the "filename".
>    - the name in property "name" which is a convenient name.
> 
> So we can't use the name field in device tree to retrieve the name to 
> create the node.
> 
> For the FDT version, I don't know if we need to care. Linux pays 
> attention to it in the device tree code.

I'm not sure we need to care either, I expect we will never see <0x10 in
our uses (they are probbaly burnt into the ROMs of PPC machines) but of
it is easy enough to so we might as well I guess?

The alternative would be an explicit check for versions we know we
understand.

Ian.

Re: [PATCH V1 16/29] xen/arm: Build DOM0 FDT by browsing the device tree structure

[Julien Grall]

On 09/09/2013 01:39 PM, Ian Campbell wrote:
> On Mon, 2013-09-09 at 13:26 +0100, Julien Grall wrote:
>> On 09/09/2013 12:33 PM, Ian Campbell wrote:
>>> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>>>>        if ( early_info.modules.nr_mods >= MOD_KERNEL &&
>>>>             early_info.modules.module[MOD_KERNEL].cmdline[0] )
>>>>            bootargs = &early_info.modules.module[MOD_KERNEL].cmdline[0];
>>>>
>>>> -    for ( prop = fdt_first_property_offset(fdt, node);
>>>> -          prop >= 0;
>>>> -          prop = fdt_next_property_offset(fdt, prop) )
>>>> +    for_each_property_of_node (np, pp)
>>>
>>> Is "of" here as in "the property of the node" or is it a stray Open
>>> Firmware from the Linux naming of these functions?
>>>
>>> Perhaps a dt_ prefix to match all the others?
>>
>> Right. I will send a patch to rename for_each_property_of_node to
>> dt_for_each_property_of_node.
>
> Might dt_for_each_node_property fit better with the other functions?

Sounds good.

>>>
>>> This comment is saying that the name of the name property used to be
>>> something else? What was it? Which version of FDT was that -- do we need
>>> to care?
>>
>> Right, on older FDT version (< 0x10) each node has 2 different name:
>>     - the name just after FDT_BEGIN_NODE in the fdt which correspond to
>> the "filename".
>>     - the name in property "name" which is a convenient name.
>>
>> So we can't use the name field in device tree to retrieve the name to
>> create the node.
>>
>> For the FDT version, I don't know if we need to care. Linux pays
>> attention to it in the device tree code.
>
> I'm not sure we need to care either, I expect we will never see <0x10 in
> our uses (they are probbaly burnt into the ROMs of PPC machines) but of
> it is easy enough to so we might as well I guess?

I forgot that there is another issue, the ePAR describes the name has 
node-name@unit-address. The name field will contains node-name and not 
the full node name.

Lets say Xen only uses the field name (ie node-name) to create the FDT 
node name. We Linux will create the procfs for the device tree 
(/proc/devicetree), it's possible to have numerous warning because there 
is 2 nodes with the same name.

> The alternative would be an explicit check for versions we know we
> understand.

-- 
Julien Grall

Re: [PATCH V1 16/29] xen/arm: Build DOM0 FDT by browsing the device tree structure

[Ian Campbell]

On Mon, 2013-09-09 at 22:53 +0100, Julien Grall wrote:

> >>>
> >>> This comment is saying that the name of the name property used to be
> >>> something else? What was it? Which version of FDT was that -- do we need
> >>> to care?
> >>
> >> Right, on older FDT version (< 0x10) each node has 2 different name:
> >>     - the name just after FDT_BEGIN_NODE in the fdt which correspond to
> >> the "filename".
> >>     - the name in property "name" which is a convenient name.
> >>
> >> So we can't use the name field in device tree to retrieve the name to
> >> create the node.
> >>
> >> For the FDT version, I don't know if we need to care. Linux pays
> >> attention to it in the device tree code.
> >
> > I'm not sure we need to care either, I expect we will never see <0x10 in
> > our uses (they are probbaly burnt into the ROMs of PPC machines) but of
> > it is easy enough to so we might as well I guess?
> 
> I forgot that there is another issue, the ePAR describes the name has 
> node-name@unit-address. The name field will contains node-name and not 
> the full node name.
> 
> Lets say Xen only uses the field name (ie node-name) to create the FDT 
> node name. We Linux will create the procfs for the device tree 
> (/proc/devicetree), it's possible to have numerous warning because there 
> is 2 nodes with the same name.

Yes, we need to avoid that. Isn't there a full_name field or something?

(I've tripped over this in debugging, it's a bit annoying that name is
just node-name and not unit-address too)

Ian.

Re: [PATCH V1 16/29] xen/arm: Build DOM0 FDT by browsing the device tree structure

[Julien Grall]

On 09/10/2013 09:58 AM, Ian Campbell wrote:
> On Mon, 2013-09-09 at 22:53 +0100, Julien Grall wrote:
> 
>>>>>
>>>>> This comment is saying that the name of the name property used to be
>>>>> something else? What was it? Which version of FDT was that -- do we need
>>>>> to care?
>>>>
>>>> Right, on older FDT version (< 0x10) each node has 2 different name:
>>>>     - the name just after FDT_BEGIN_NODE in the fdt which correspond to
>>>> the "filename".
>>>>     - the name in property "name" which is a convenient name.
>>>>
>>>> So we can't use the name field in device tree to retrieve the name to
>>>> create the node.
>>>>
>>>> For the FDT version, I don't know if we need to care. Linux pays
>>>> attention to it in the device tree code.
>>>
>>> I'm not sure we need to care either, I expect we will never see <0x10 in
>>> our uses (they are probbaly burnt into the ROMs of PPC machines) but of
>>> it is easy enough to so we might as well I guess?
>>
>> I forgot that there is another issue, the ePAR describes the name has 
>> node-name@unit-address. The name field will contains node-name and not 
>> the full node name.
>>
>> Lets say Xen only uses the field name (ie node-name) to create the FDT 
>> node name. We Linux will create the procfs for the device tree 
>> (/proc/devicetree), it's possible to have numerous warning because there 
>> is 2 nodes with the same name.
> 
> Yes, we need to avoid that. Isn't there a full_name field or something?

The full_name field contains the full path to this node, for instance
/cpus/cpu@0. So we can retrieve the node-name@unit-address with a
basename-like function.

> (I've tripped over this in debugging, it's a bit annoying that name is
> just node-name and not unit-address too)

-- 
Julien Grall

Re: [PATCH V1 16/29] xen/arm: Build DOM0 FDT by browsing the device tree structure

[Ian Campbell]

On Tue, 2013-09-10 at 11:39 +0100, Julien Grall wrote:
> On 09/10/2013 09:58 AM, Ian Campbell wrote:
> > On Mon, 2013-09-09 at 22:53 +0100, Julien Grall wrote:
> > 
> >>>>>
> >>>>> This comment is saying that the name of the name property used to be
> >>>>> something else? What was it? Which version of FDT was that -- do we need
> >>>>> to care?
> >>>>
> >>>> Right, on older FDT version (< 0x10) each node has 2 different name:
> >>>>     - the name just after FDT_BEGIN_NODE in the fdt which correspond to
> >>>> the "filename".
> >>>>     - the name in property "name" which is a convenient name.
> >>>>
> >>>> So we can't use the name field in device tree to retrieve the name to
> >>>> create the node.
> >>>>
> >>>> For the FDT version, I don't know if we need to care. Linux pays
> >>>> attention to it in the device tree code.
> >>>
> >>> I'm not sure we need to care either, I expect we will never see <0x10 in
> >>> our uses (they are probbaly burnt into the ROMs of PPC machines) but of
> >>> it is easy enough to so we might as well I guess?
> >>
> >> I forgot that there is another issue, the ePAR describes the name has 
> >> node-name@unit-address. The name field will contains node-name and not 
> >> the full node name.
> >>
> >> Lets say Xen only uses the field name (ie node-name) to create the FDT 
> >> node name. We Linux will create the procfs for the device tree 
> >> (/proc/devicetree), it's possible to have numerous warning because there 
> >> is 2 nodes with the same name.
> > 
> > Yes, we need to avoid that. Isn't there a full_name field or something?
> 
> The full_name field contains the full path to this node, for instance
> /cpus/cpu@0. So we can retrieve the node-name@unit-address with a
> basename-like function.

Bit of a shame to have to jump through such hoop, but OK.

Do we keep the unit-address in the datastructure?

Re: [PATCH V1 16/29] xen/arm: Build DOM0 FDT by browsing the device tree structure

[Julien Grall]

On 09/10/2013 11:47 AM, Ian Campbell wrote:
> On Tue, 2013-09-10 at 11:39 +0100, Julien Grall wrote:
>> On 09/10/2013 09:58 AM, Ian Campbell wrote:
>>> On Mon, 2013-09-09 at 22:53 +0100, Julien Grall wrote:
>>>
>>>>>>>
>>>>>>> This comment is saying that the name of the name property used to be
>>>>>>> something else? What was it? Which version of FDT was that -- do we need
>>>>>>> to care?
>>>>>>
>>>>>> Right, on older FDT version (< 0x10) each node has 2 different name:
>>>>>>     - the name just after FDT_BEGIN_NODE in the fdt which correspond to
>>>>>> the "filename".
>>>>>>     - the name in property "name" which is a convenient name.
>>>>>>
>>>>>> So we can't use the name field in device tree to retrieve the name to
>>>>>> create the node.
>>>>>>
>>>>>> For the FDT version, I don't know if we need to care. Linux pays
>>>>>> attention to it in the device tree code.
>>>>>
>>>>> I'm not sure we need to care either, I expect we will never see <0x10 in
>>>>> our uses (they are probbaly burnt into the ROMs of PPC machines) but of
>>>>> it is easy enough to so we might as well I guess?
>>>>
>>>> I forgot that there is another issue, the ePAR describes the name has 
>>>> node-name@unit-address. The name field will contains node-name and not 
>>>> the full node name.
>>>>
>>>> Lets say Xen only uses the field name (ie node-name) to create the FDT 
>>>> node name. We Linux will create the procfs for the device tree 
>>>> (/proc/devicetree), it's possible to have numerous warning because there 
>>>> is 2 nodes with the same name.
>>>
>>> Yes, we need to avoid that. Isn't there a full_name field or something?
>>
>> The full_name field contains the full path to this node, for instance
>> /cpus/cpu@0. So we can retrieve the node-name@unit-address with a
>> basename-like function.
> 
> Bit of a shame to have to jump through such hoop, but OK.
> 
> Do we keep the unit-address in the datastructure?

No because the unit-address is optional and is equal to the first
address of the property "reg".

-- 
Julien Grall

[PATCH V1 17/29] xen/arm: Mark each device used by Xen as disabled in DOM0 FDT

[Julien Grall]

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

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

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

Re: [PATCH V1 17/29] xen/arm: Mark each device used by Xen as disabled in DOM0 FDT

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> When a device has a property status with disabled inside, Linux will not use
> the device.

Do we not filter such devices out already?

Or did the previous patch remove that functionality? In which case is
there a bisection hazard here?

Why is it preferred to leave the device with disabled in it instead of
removing it? (Can this go in the changelog please)

I think I've asked before, but is the handling of status = disabled
generic and therefore reliable?

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain_build.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 76decf4..d56bc70a 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -208,6 +208,14 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>              return res;
>      }
>  
> +    /* Disable all devices used by Xen */
> +    if ( dt_device_used_by(np) == DOMID_XEN )
> +    {
> +        res = fdt_property(kinfo->fdt, "status", "disabled", 8 + 1);
> +        if ( res )
> +            return res;
> +    }
> +
>      /*
>       * XXX should populate /chosen/linux,initrd-{start,end} here if we
>       * have module[2]

Re: [PATCH V1 17/29] xen/arm: Mark each device used by Xen as disabled in DOM0 FDT

[Julien Grall]

On 09/09/2013 12:37 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> When a device has a property status with disabled inside, Linux will not use
>> the device.
>
> Do we not filter such devices out already?

We only filter gic/timer node. All the others nodes used by Xen, for 
instance the serial device, will be marked by status = "disabled".

> Or did the previous patch remove that functionality? In which case is
> there a bisection hazard here?

AFAIK, there is no bisection hazard with the current order of this patch 
series.

>
> Why is it preferred to leave the device with disabled in it instead of
> removing it? (Can this go in the changelog please)
> I think I've asked before, but is the handling of status = disabled
> generic and therefore reliable?

I let this patch because I didn't implement all the fake nodes in the 
previous patch series. In another hand, I'm not sure how Linux will 
react if there is a missing node and it should cope with status = 
"disabled" with most of nodes.

I will give a try and see if I can remove completely nodes used by Xen. 
Except, of course, gic and timer nodes which are recreated later.

>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>   xen/arch/arm/domain_build.c |    8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 76decf4..d56bc70a 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -208,6 +208,14 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>>               return res;
>>       }
>>
>> +    /* Disable all devices used by Xen */
>> +    if ( dt_device_used_by(np) == DOMID_XEN )
>> +    {
>> +        res = fdt_property(kinfo->fdt, "status", "disabled", 8 + 1);
>> +        if ( res )
>> +            return res;
>> +    }
>> +
>>       /*
>>        * XXX should populate /chosen/linux,initrd-{start,end} here if we
>>        * have module[2]
>
>


-- 
Julien Grall

Re: [PATCH V1 17/29] xen/arm: Mark each device used by Xen as disabled in DOM0 FDT

[Ian Campbell]

On Mon, 2013-09-09 at 22:53 +0100, Julien Grall wrote:
> On 09/09/2013 12:37 PM, Ian Campbell wrote:
> > On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> >> When a device has a property status with disabled inside, Linux will not use
> >> the device.
> >
> > Do we not filter such devices out already?
> 
> We only filter gic/timer node. All the others nodes used by Xen, for 
> instance the serial device, will be marked by status = "disabled".

Ah, the current code omits devices from the mapping but not the fdt, I
was confusing the two. In the current way of doing things
fdt_next_dom0_node would need a check for the owner, but that's not
relevant with this series.

> > Or did the previous patch remove that functionality? In which case is
> > there a bisection hazard here?
> 
> AFAIK, there is no bisection hazard with the current order of this patch 
> series.

Ack, thanks.

> 
> >
> > Why is it preferred to leave the device with disabled in it instead of
> > removing it? (Can this go in the changelog please)
> > I think I've asked before, but is the handling of status = disabled
> > generic and therefore reliable?
> 
> I let this patch because I didn't implement all the fake nodes in the 
> previous patch series. In another hand, I'm not sure how Linux will 
> react if there is a missing node and it should cope with status = 
> "disabled" with most of nodes.
> 
> I will give a try and see if I can remove completely nodes used by Xen. 
> Except, of course, gic and timer nodes which are recreated later.

If you want you can add that to the end of the series/future work rather
than reworking this patch in the middle of a large series.

[PATCH V1 18/29] xen/arm: Don't map disabled device in DOM0

[Julien Grall]

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

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Add a comment
---
 xen/arch/arm/domain_build.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d56bc70a..11f247b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -416,9 +416,13 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
      *  - Device used by Xen: Obviously dom0 can't use them
      *  - Memory: the guest will see a different view of memory. It will
      *  be allocated later.
+     *  - Disabled device: Linux is able to cope with status="disabled"
+     *  property. Therefore these device doesn't need to be mapped. This
+     *  solution can be use later for pass through.
      */
     if ( dt_device_used_by(np) != DOMID_XEN &&
-         !dt_device_type_is_equal(np, "memory") )
+         !dt_device_type_is_equal(np, "memory") &&
+         dt_device_is_available(np) )
     {
         res = map_device(d, np);
 
-- 
1.7.10.4

Re: [PATCH V1 18/29] xen/arm: Don't map disabled device in DOM0

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> Linux should cope with 'status = "disabled"' in the Device Tree.

This is devices which were disabled in the DT passed to Xen, rather than
ones which we have disabled ourselves (e.g. the previous patch)?

> This solution can be used later to pass-through device to a specific guest.

How? I thought disabled meant "not present/wired-up on this variant of
the platform"? A user cannot be expected to edit the host DTB to enable
passthrough. We might want a dtb_hide cmdline option, or to do it
dynamically like pciback is capable of.

Or maybe I'm wrong above and this is indeed devices we have disabled
ourselves?

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Add a comment
> ---
>  xen/arch/arm/domain_build.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d56bc70a..11f247b 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -416,9 +416,13 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>       *  - Device used by Xen: Obviously dom0 can't use them
>       *  - Memory: the guest will see a different view of memory. It will
>       *  be allocated later.
> +     *  - Disabled device: Linux is able to cope with status="disabled"
> +     *  property. Therefore these device doesn't need to be mapped. This
> +     *  solution can be use later for pass through.
>       */
>      if ( dt_device_used_by(np) != DOMID_XEN &&
> -         !dt_device_type_is_equal(np, "memory") )
> +         !dt_device_type_is_equal(np, "memory") &&
> +         dt_device_is_available(np) )
>      {
>          res = map_device(d, np);
>  

Re: [PATCH V1 18/29] xen/arm: Don't map disabled device in DOM0

[Julien Grall]

On 09/09/2013 12:40 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> Linux should cope with 'status = "disabled"' in the Device Tree.
>
> This is devices which were disabled in the DT passed to Xen, rather than
> ones which we have disabled ourselves (e.g. the previous patch)?

Yes.


>> This solution can be used later to pass-through device to a specific guest.
>
> How? I thought disabled meant "not present/wired-up on this variant of
> the platform"? A user cannot be expected to edit the host DTB to enable
> passthrough. We might want a dtb_hide cmdline option, or to do it
> dynamically like pciback is capable of.

Disabled also means "the device is not presently operational, but it 
might become operational in the future".

> Or maybe I'm wrong above and this is indeed devices we have disabled
> ourselves?

I'm not sure it's usefull. But I don't see any reason to map disabled 
device in dom0.


>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>      Changes in v2:
>>          - Add a comment
>> ---
>>   xen/arch/arm/domain_build.c |    6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index d56bc70a..11f247b 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -416,9 +416,13 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>>        *  - Device used by Xen: Obviously dom0 can't use them
>>        *  - Memory: the guest will see a different view of memory. It will
>>        *  be allocated later.
>> +     *  - Disabled device: Linux is able to cope with status="disabled"
>> +     *  property. Therefore these device doesn't need to be mapped. This
>> +     *  solution can be use later for pass through.
>>        */
>>       if ( dt_device_used_by(np) != DOMID_XEN &&
>> -         !dt_device_type_is_equal(np, "memory") )
>> +         !dt_device_type_is_equal(np, "memory") &&
>> +         dt_device_is_available(np) )
>>       {
>>           res = map_device(d, np);
>>
>
>


-- 
Julien Grall

Re: [PATCH V1 18/29] xen/arm: Don't map disabled device in DOM0

[Ian Campbell]

On Mon, 2013-09-09 at 22:59 +0100, Julien Grall wrote:
> On 09/09/2013 12:40 PM, Ian Campbell wrote:
> > On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> >> Linux should cope with 'status = "disabled"' in the Device Tree.
> >
> > This is devices which were disabled in the DT passed to Xen, rather than
> > ones which we have disabled ourselves (e.g. the previous patch)?
> 
> Yes.
> 
> 
> >> This solution can be used later to pass-through device to a specific guest.
> >
> > How? I thought disabled meant "not present/wired-up on this variant of
> > the platform"? A user cannot be expected to edit the host DTB to enable
> > passthrough. We might want a dtb_hide cmdline option, or to do it
> > dynamically like pciback is capable of.
> 
> Disabled also means "the device is not presently operational, but it 
> might become operational in the future".
> 
> > Or maybe I'm wrong above and this is indeed devices we have disabled
> > ourselves?
> 
> I'm not sure it's usefull. But I don't see any reason to map disabled 
> device in dom0.

Agreed, I just think the reference to passthrough in the changelog is
not relevant to this and might not be accurate.

Ian.

[PATCH V1 19/29] xen/arm: Create a fake PSCI node in dom0 device tree

[Julien Grall]

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

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Use fdt_property_cell
---
 xen/arch/arm/domain_build.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 11f247b..4c936c1 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -13,6 +13,7 @@
 #include <xen/guest_access.h>
 #include <asm/setup.h>
 #include <asm/platform.h>
+#include <asm/psci.h>
 
 #include <asm/gic.h>
 #include <xen/irq.h>
@@ -306,6 +307,38 @@ static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent)
     return res;
 }
 
+static int make_psci_node(void *fdt, const struct dt_device_node *parent)
+{
+    int res;
+
+    DPRINT("Create PSCI node\n");
+
+    /* See linux Documentation/devicetree/bindings/arm/psci.txt */
+    res = fdt_begin_node(fdt, "psci");
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "compatible", "arm,psci");
+    if ( res )
+        return res;
+
+    res = fdt_property_string(fdt, "method", "hvc");
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "cpu_off", __PSCI_cpu_off);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "cpu_on", __PSCI_cpu_on);
+    if ( res )
+        return res;
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
 /* Map the device in the domain */
 static int map_device(struct domain *d, const struct dt_device_node *dev)
 {
@@ -393,6 +426,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
     {
         DT_MATCH_COMPATIBLE("xen,xen"),
         DT_MATCH_COMPATIBLE("xen,multiboot-module"),
+        DT_MATCH_COMPATIBLE("arm,psci"),
         { /* sentinel */ },
     };
     const struct dt_device_node *child;
@@ -458,6 +492,10 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         res = make_hypervisor_node(kinfo->fdt, np);
         if ( res )
             return res;
+
+        res = make_psci_node(kinfo->fdt, np);
+        if ( res )
+            return res;
     }
 
     res = fdt_end_node(kinfo->fdt);
-- 
1.7.10.4

Re: [PATCH V1 19/29] xen/arm: Create a fake PSCI node in dom0 device tree

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> Xen uses PSCI to bring up secondary cpus for the guest.

Does this account for/incorporate the changes in "dt: update PSCI
binding documentation for v0.2" from Rob Herring? Msg-di
<1377564633-31638-1-git-send-email-robherring2@gmail.com>

That might require actual code changes in Xen too I suppose?

Ian.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Use fdt_property_cell
> ---
>  xen/arch/arm/domain_build.c |   38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 11f247b..4c936c1 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -13,6 +13,7 @@
>  #include <xen/guest_access.h>
>  #include <asm/setup.h>
>  #include <asm/platform.h>
> +#include <asm/psci.h>
>  
>  #include <asm/gic.h>
>  #include <xen/irq.h>
> @@ -306,6 +307,38 @@ static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent)
>      return res;
>  }
>  
> +static int make_psci_node(void *fdt, const struct dt_device_node *parent)
> +{
> +    int res;
> +
> +    DPRINT("Create PSCI node\n");
> +
> +    /* See linux Documentation/devicetree/bindings/arm/psci.txt */
> +    res = fdt_begin_node(fdt, "psci");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_string(fdt, "compatible", "arm,psci");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_string(fdt, "method", "hvc");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_cell(fdt, "cpu_off", __PSCI_cpu_off);
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_cell(fdt, "cpu_on", __PSCI_cpu_on);
> +    if ( res )
> +        return res;
> +
> +    res = fdt_end_node(fdt);
> +
> +    return res;
> +}
> +
>  /* Map the device in the domain */
>  static int map_device(struct domain *d, const struct dt_device_node *dev)
>  {
> @@ -393,6 +426,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>      {
>          DT_MATCH_COMPATIBLE("xen,xen"),
>          DT_MATCH_COMPATIBLE("xen,multiboot-module"),
> +        DT_MATCH_COMPATIBLE("arm,psci"),
>          { /* sentinel */ },
>      };
>      const struct dt_device_node *child;
> @@ -458,6 +492,10 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>          res = make_hypervisor_node(kinfo->fdt, np);
>          if ( res )
>              return res;
> +
> +        res = make_psci_node(kinfo->fdt, np);
> +        if ( res )
> +            return res;
>      }
>  
>      res = fdt_end_node(kinfo->fdt);

Re: [PATCH V1 19/29] xen/arm: Create a fake PSCI node in dom0 device tree

[Julien Grall]

On 09/09/2013 12:41 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> Xen uses PSCI to bring up secondary cpus for the guest.
>
> Does this account for/incorporate the changes in "dt: update PSCI
> binding documentation for v0.2" from Rob Herring? Msg-di
> <1377564633-31638-1-git-send-email-robherring2@gmail.com>
>
> That might require actual code changes in Xen too I suppose?

I don't think, the PSCI code in Xen is generic. We will only have to 
update the current patch.

Do you know if Linux supports PSCI 0.2?

>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>      Changes in v2:
>>          - Use fdt_property_cell
>> ---
>>   xen/arch/arm/domain_build.c |   38 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 11f247b..4c936c1 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -13,6 +13,7 @@
>>   #include <xen/guest_access.h>
>>   #include <asm/setup.h>
>>   #include <asm/platform.h>
>> +#include <asm/psci.h>
>>
>>   #include <asm/gic.h>
>>   #include <xen/irq.h>
>> @@ -306,6 +307,38 @@ static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent)
>>       return res;
>>   }
>>
>> +static int make_psci_node(void *fdt, const struct dt_device_node *parent)
>> +{
>> +    int res;
>> +
>> +    DPRINT("Create PSCI node\n");
>> +
>> +    /* See linux Documentation/devicetree/bindings/arm/psci.txt */
>> +    res = fdt_begin_node(fdt, "psci");
>> +    if ( res )
>> +        return res;
>> +
>> +    res = fdt_property_string(fdt, "compatible", "arm,psci");
>> +    if ( res )
>> +        return res;
>> +
>> +    res = fdt_property_string(fdt, "method", "hvc");
>> +    if ( res )
>> +        return res;
>> +
>> +    res = fdt_property_cell(fdt, "cpu_off", __PSCI_cpu_off);
>> +    if ( res )
>> +        return res;
>> +
>> +    res = fdt_property_cell(fdt, "cpu_on", __PSCI_cpu_on);
>> +    if ( res )
>> +        return res;
>> +
>> +    res = fdt_end_node(fdt);
>> +
>> +    return res;
>> +}
>> +
>>   /* Map the device in the domain */
>>   static int map_device(struct domain *d, const struct dt_device_node *dev)
>>   {
>> @@ -393,6 +426,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>>       {
>>           DT_MATCH_COMPATIBLE("xen,xen"),
>>           DT_MATCH_COMPATIBLE("xen,multiboot-module"),
>> +        DT_MATCH_COMPATIBLE("arm,psci"),
>>           { /* sentinel */ },
>>       };
>>       const struct dt_device_node *child;
>> @@ -458,6 +492,10 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>>           res = make_hypervisor_node(kinfo->fdt, np);
>>           if ( res )
>>               return res;
>> +
>> +        res = make_psci_node(kinfo->fdt, np);
>> +        if ( res )
>> +            return res;
>>       }
>>
>>       res = fdt_end_node(kinfo->fdt);
>
>

-- 
Julien Grall

Re: [PATCH V1 19/29] xen/arm: Create a fake PSCI node in dom0 device tree

[Ian Campbell]

On Mon, 2013-09-09 at 23:04 +0100, Julien Grall wrote:
> On 09/09/2013 12:41 PM, Ian Campbell wrote:
> > On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> >> Xen uses PSCI to bring up secondary cpus for the guest.
> >
> > Does this account for/incorporate the changes in "dt: update PSCI
> > binding documentation for v0.2" from Rob Herring? Msg-di
> > <1377564633-31638-1-git-send-email-robherring2@gmail.com>
> >
> > That might require actual code changes in Xen too I suppose?
> 
> I don't think, the PSCI code in Xen is generic. We will only have to 
> update the current patch.

Stefano can probably remember the details but there were some meaningful
changes in PSCI 0.2, and in particular the calling mechanism and how it
is described in DTB has been improved.

> Do you know if Linux supports PSCI 0.2?

If it doesn't then it will very soon I expect.

> 
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>
> >> ---
> >>      Changes in v2:
> >>          - Use fdt_property_cell
> >> ---
> >>   xen/arch/arm/domain_build.c |   38 ++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 38 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >> index 11f247b..4c936c1 100644
> >> --- a/xen/arch/arm/domain_build.c
> >> +++ b/xen/arch/arm/domain_build.c
> >> @@ -13,6 +13,7 @@
> >>   #include <xen/guest_access.h>
> >>   #include <asm/setup.h>
> >>   #include <asm/platform.h>
> >> +#include <asm/psci.h>
> >>
> >>   #include <asm/gic.h>
> >>   #include <xen/irq.h>
> >> @@ -306,6 +307,38 @@ static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent)
> >>       return res;
> >>   }
> >>
> >> +static int make_psci_node(void *fdt, const struct dt_device_node *parent)
> >> +{
> >> +    int res;
> >> +
> >> +    DPRINT("Create PSCI node\n");
> >> +
> >> +    /* See linux Documentation/devicetree/bindings/arm/psci.txt */
> >> +    res = fdt_begin_node(fdt, "psci");
> >> +    if ( res )
> >> +        return res;
> >> +
> >> +    res = fdt_property_string(fdt, "compatible", "arm,psci");
> >> +    if ( res )
> >> +        return res;
> >> +
> >> +    res = fdt_property_string(fdt, "method", "hvc");
> >> +    if ( res )
> >> +        return res;
> >> +
> >> +    res = fdt_property_cell(fdt, "cpu_off", __PSCI_cpu_off);
> >> +    if ( res )
> >> +        return res;
> >> +
> >> +    res = fdt_property_cell(fdt, "cpu_on", __PSCI_cpu_on);
> >> +    if ( res )
> >> +        return res;
> >> +
> >> +    res = fdt_end_node(fdt);
> >> +
> >> +    return res;
> >> +}
> >> +
> >>   /* Map the device in the domain */
> >>   static int map_device(struct domain *d, const struct dt_device_node *dev)
> >>   {
> >> @@ -393,6 +426,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
> >>       {
> >>           DT_MATCH_COMPATIBLE("xen,xen"),
> >>           DT_MATCH_COMPATIBLE("xen,multiboot-module"),
> >> +        DT_MATCH_COMPATIBLE("arm,psci"),
> >>           { /* sentinel */ },
> >>       };
> >>       const struct dt_device_node *child;
> >> @@ -458,6 +492,10 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
> >>           res = make_hypervisor_node(kinfo->fdt, np);
> >>           if ( res )
> >>               return res;
> >> +
> >> +        res = make_psci_node(kinfo->fdt, np);
> >> +        if ( res )
> >> +            return res;
> >>       }
> >>
> >>       res = fdt_end_node(kinfo->fdt);
> >
> >
> 

[PATCH V1 20/29] xen/arm: Create a fake cpus node in dom0 device tree

[Julien Grall]

The number of cpus in dom0 can be different compare to the real number of
physical cpus.

For the moment, Xen assumes that the cpus are identical.

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

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4c936c1..21cd4e0 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -339,6 +339,92 @@ static int make_psci_node(void *fdt, const struct dt_device_node *parent)
     return res;
 }
 
+static int make_cpus_node(const struct domain *d, void *fdt,
+                          const struct dt_device_node *parent)
+{
+    int res;
+    const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
+    const struct dt_device_node *npcpu;
+    unsigned int cpu;
+    const void *compatible = NULL;
+    u32 len;
+    /* Placeholder for cpu@ + a 32-bit number + \0 */
+    char buf[15];
+
+    DPRINT("Create cpus node\n");
+
+    if ( !cpus )
+    {
+        dprintk(XENLOG_ERR, "Missing /cpus node in the device tree?\n");
+        return -ENOENT;
+    }
+
+    /*
+     * Get the compatible property of CPUs from the device tree.
+     * We are assuming that all CPUs are the same
+     * TODO: Handle compatible per VCPU
+     */
+    for_each_child_node(cpus, npcpu)
+    {
+        if ( dt_device_type_is_equal(npcpu, "cpu") )
+        {
+            compatible = dt_get_property(npcpu, "compatible", &len);
+            break;
+        }
+    }
+
+    if ( !compatible )
+    {
+        dprintk(XENLOG_ERR, "Can't find cpu in the device tree?\n");
+        return -ENOENT;
+    }
+
+    /* See Linux Documentation/devicetree/booting-without-of.txt
+     * section III.5.b
+     */
+    res = fdt_begin_node(fdt, "cpus");
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "#address-cells", 1);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "#size-cells", 0);
+    if ( res )
+        return res;
+
+    for ( cpu = 0; cpu < d->max_vcpus; cpu++ )
+    {
+        DPRINT("Create cpu@%u node\n", cpu);
+
+        snprintf(buf, sizeof(buf), "cpu@%u", cpu);
+        res = fdt_begin_node(fdt, buf);
+        if ( res )
+            return res;
+
+        res = fdt_property(fdt, "compatible", compatible, len);
+        if ( res )
+            return res;
+
+        res = fdt_property_string(fdt, "device_type", "cpu");
+        if ( res )
+            return res;
+
+        res = fdt_property_cell(fdt, "reg", cpu);
+        if ( res )
+            return res;
+
+        res = fdt_end_node(fdt);
+        if ( res )
+            return res;
+    }
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
 /* Map the device in the domain */
 static int map_device(struct domain *d, const struct dt_device_node *dev)
 {
@@ -427,6 +513,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         DT_MATCH_COMPATIBLE("xen,xen"),
         DT_MATCH_COMPATIBLE("xen,multiboot-module"),
         DT_MATCH_COMPATIBLE("arm,psci"),
+        DT_MATCH_PATH("/cpus"),
         { /* sentinel */ },
     };
     const struct dt_device_node *child;
@@ -496,6 +583,10 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         res = make_psci_node(kinfo->fdt, np);
         if ( res )
             return res;
+
+        res = make_cpus_node(d, kinfo->fdt, np);
+        if ( res )
+            return res;
     }
 
     res = fdt_end_node(kinfo->fdt);
-- 
1.7.10.4

Re: [PATCH V1 20/29] xen/arm: Create a fake cpus node in dom0 device tree

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> The number of cpus in dom0 can be different compare to the real number of
> physical cpus.
> 
> For the moment, Xen assumes that the cpus are identical.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/domain_build.c |   91 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4c936c1..21cd4e0 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -339,6 +339,92 @@ static int make_psci_node(void *fdt, const struct dt_device_node *parent)
>      return res;
>  }
>  
> +static int make_cpus_node(const struct domain *d, void *fdt,
> +                          const struct dt_device_node *parent)
> +{
> +    int res;
> +    const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
> +    const struct dt_device_node *npcpu;
> +    unsigned int cpu;
> +    const void *compatible = NULL;
> +    u32 len;
> +    /* Placeholder for cpu@ + a 32-bit number + \0 */
> +    char buf[15];
> +
> +    DPRINT("Create cpus node\n");
> +
> +    if ( !cpus )
> +    {
> +        dprintk(XENLOG_ERR, "Missing /cpus node in the device tree?\n");
> +        return -ENOENT;
> +    }
> +
> +    /*
> +     * Get the compatible property of CPUs from the device tree.
> +     * We are assuming that all CPUs are the same

"... so we just look for the first one" would clarify what is going on
here a bit.

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

> +     * TODO: Handle compatible per VCPU
> +     */
> +    for_each_child_node(cpus, npcpu)
> +    {
> +        if ( dt_device_type_is_equal(npcpu, "cpu") )
> +        {
> +            compatible = dt_get_property(npcpu, "compatible", &len);
> +            break;
> +        }
> +    }
> +
> +    if ( !compatible )
> +    {
> +        dprintk(XENLOG_ERR, "Can't find cpu in the device tree?\n");
> +        return -ENOENT;
> +    }
> +
> +    /* See Linux Documentation/devicetree/booting-without-of.txt
> +     * section III.5.b
> +     */
> +    res = fdt_begin_node(fdt, "cpus");
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_cell(fdt, "#address-cells", 1);
> +    if ( res )
> +        return res;
> +
> +    res = fdt_property_cell(fdt, "#size-cells", 0);
> +    if ( res )
> +        return res;
> +
> +    for ( cpu = 0; cpu < d->max_vcpus; cpu++ )
> +    {
> +        DPRINT("Create cpu@%u node\n", cpu);
> +
> +        snprintf(buf, sizeof(buf), "cpu@%u", cpu);
> +        res = fdt_begin_node(fdt, buf);
> +        if ( res )
> +            return res;
> +
> +        res = fdt_property(fdt, "compatible", compatible, len);
> +        if ( res )
> +            return res;
> +
> +        res = fdt_property_string(fdt, "device_type", "cpu");
> +        if ( res )
> +            return res;
> +
> +        res = fdt_property_cell(fdt, "reg", cpu);
> +        if ( res )
> +            return res;
> +
> +        res = fdt_end_node(fdt);
> +        if ( res )
> +            return res;
> +    }
> +
> +    res = fdt_end_node(fdt);
> +
> +    return res;
> +}
> +
>  /* Map the device in the domain */
>  static int map_device(struct domain *d, const struct dt_device_node *dev)
>  {
> @@ -427,6 +513,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>          DT_MATCH_COMPATIBLE("xen,xen"),
>          DT_MATCH_COMPATIBLE("xen,multiboot-module"),
>          DT_MATCH_COMPATIBLE("arm,psci"),
> +        DT_MATCH_PATH("/cpus"),
>          { /* sentinel */ },
>      };
>      const struct dt_device_node *child;
> @@ -496,6 +583,10 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>          res = make_psci_node(kinfo->fdt, np);
>          if ( res )
>              return res;
> +
> +        res = make_cpus_node(d, kinfo->fdt, np);
> +        if ( res )
> +            return res;
>      }
>  
>      res = fdt_end_node(kinfo->fdt);

[PATCH V1 21/29] xen/arm: Create a fake GIC node in dom0 device tree

[Julien Grall]

Recreate the GIC node and remove hypervisor specific ranges (vgic and hypervisor
controls).

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

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 21cd4e0..c57a79d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -425,6 +425,79 @@ static int make_cpus_node(const struct domain *d, void *fdt,
     return res;
 }
 
+static int make_gic_node(const struct domain *d, void *fdt,
+                         const struct dt_device_node *parent)
+{
+    const struct dt_device_node *gic = dt_interrupt_controller;
+    const void *compatible = NULL;
+    u32 len;
+    __be32 *new_cells, *tmp;
+    int res = 0;
+
+    DPRINT("Create gic node\n");
+
+    compatible = dt_get_property(gic, "compatible", &len);
+    if ( !compatible )
+    {
+        dprintk(XENLOG_ERR, "Can't find compatible property for the gic node\n");
+        return -FDT_ERR_XEN(ENOENT);
+    }
+
+    res = fdt_begin_node(fdt, "interrupt-controller");
+    if ( res )
+        return res;
+
+    res = fdt_property(fdt, "compatible", compatible, len);
+    if ( res )
+        return res;
+
+    res = fdt_property_cell(fdt, "#interrupt-cells", 3);
+    if ( res )
+        return res;
+
+    res = fdt_property(fdt, "interrupt-controller", NULL, 0);
+
+    if ( res )
+        return res;
+
+    len = dt_cells_to_size(dt_n_addr_cells(parent) + dt_n_size_cells(parent));
+    len *= 2;
+    new_cells = xzalloc_bytes(dt_cells_to_size(len));
+    if ( new_cells == NULL )
+        return -FDT_ERR_XEN(ENOMEM);
+
+    tmp = new_cells;
+    DPRINT("  Set Distributor Base 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
+           d->arch.vgic.dbase, d->arch.vgic.dbase + PAGE_SIZE - 1);
+    dt_set_range(&tmp, parent, d->arch.vgic.dbase, PAGE_SIZE);
+
+    DPRINT("  Set Cpu Base 0x%"PRIpaddr" size = 0x%"PRIpaddr"\n",
+           d->arch.vgic.cbase, d->arch.vgic.cbase + (PAGE_SIZE * 2) - 1);
+    dt_set_range(&tmp, parent, d->arch.vgic.cbase, PAGE_SIZE * 2);
+
+    res = fdt_property(fdt, "reg", new_cells, len);
+    xfree(new_cells);
+
+    if ( res )
+        return res;
+
+    /*
+     * The value of the property "phandle" in the property "interrupts"
+     * to know on which interrupt controller the interrupt is wired.
+     */
+    if ( gic->phandle )
+    {
+        DPRINT("  Set phandle = 0x%x\n", gic->phandle);
+        res = fdt_property_cell(fdt, "phandle", gic->phandle);
+        if ( res )
+            return res;
+    }
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
 /* Map the device in the domain */
 static int map_device(struct domain *d, const struct dt_device_node *dev)
 {
@@ -514,6 +587,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         DT_MATCH_COMPATIBLE("xen,multiboot-module"),
         DT_MATCH_COMPATIBLE("arm,psci"),
         DT_MATCH_PATH("/cpus"),
+        DT_MATCH_COMPATIBLE("arm,cortex-a15-gic"),
         { /* sentinel */ },
     };
     const struct dt_device_node *child;
@@ -587,6 +661,10 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         res = make_cpus_node(d, kinfo->fdt, np);
         if ( res )
             return res;
+
+        res = make_gic_node(d, kinfo->fdt, np);
+        if ( res )
+            return res;
     }
 
     res = fdt_end_node(kinfo->fdt);
-- 
1.7.10.4

Re: [PATCH V1 21/29] xen/arm: Create a fake GIC node in dom0 device tree

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> @@ -514,6 +587,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>          DT_MATCH_COMPATIBLE("xen,multiboot-module"),
>          DT_MATCH_COMPATIBLE("arm,psci"),
>          DT_MATCH_PATH("/cpus"),
> +        DT_MATCH_COMPATIBLE("arm,cortex-a15-gic"),

This will need changing as we change the list in gic.c.

Is there some way we can make this generic by using
dt_interrupt_controller->compatible?

Otherwise I think we either need a comment at both locations or way to
share the lists.

Ian.

>          { /* sentinel */ },
>      };
>      const struct dt_device_node *child;

Re: [PATCH V1 21/29] xen/arm: Create a fake GIC node in dom0 device tree

[Julien Grall]

On 09/09/2013 12:49 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> @@ -514,6 +587,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>>          DT_MATCH_COMPATIBLE("xen,multiboot-module"),
>>          DT_MATCH_COMPATIBLE("arm,psci"),
>>          DT_MATCH_PATH("/cpus"),
>> +        DT_MATCH_COMPATIBLE("arm,cortex-a15-gic"),
> 
> This will need changing as we change the list in gic.c.
> 
> Is there some way we can make this generic by using
> dt_interrupt_controller->compatible?

compatible is a list of zeroed string. How about a define in gic.h?

#define GIC_COMPATIBLE                   \
  DT_MATCH_PATH("arm,cortex-a15-gic)

So we can use in the different place.

> Otherwise I think we either need a comment at both locations or way to
> share the lists.
> 
> Ian.
> 
>>          { /* sentinel */ },
>>      };
>>      const struct dt_device_node *child;
> 
> 


-- 
Julien Grall

Re: [PATCH V1 21/29] xen/arm: Create a fake GIC node in dom0 device tree

[Ian Campbell]

On Tue, 2013-09-10 at 11:49 +0100, Julien Grall wrote:
> On 09/09/2013 12:49 PM, Ian Campbell wrote:
> > On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> >> @@ -514,6 +587,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
> >>          DT_MATCH_COMPATIBLE("xen,multiboot-module"),
> >>          DT_MATCH_COMPATIBLE("arm,psci"),
> >>          DT_MATCH_PATH("/cpus"),
> >> +        DT_MATCH_COMPATIBLE("arm,cortex-a15-gic"),
> > 
> > This will need changing as we change the list in gic.c.
> > 
> > Is there some way we can make this generic by using
> > dt_interrupt_controller->compatible?
> 
> compatible is a list of zeroed string. How about a define in gic.h?
> 
> #define GIC_COMPATIBLE                   \
>   DT_MATCH_PATH("arm,cortex-a15-gic)
> 
> So we can use in the different place.

Sure. DT_GIC_COMPATIBLE perhaps? or DT_MATCH_GIC?

> 
> > Otherwise I think we either need a comment at both locations or way to
> > share the lists.
> > 
> > Ian.
> > 
> >>          { /* sentinel */ },
> >>      };
> >>      const struct dt_device_node *child;
> > 
> > 
> 
> 

[PATCH V1 22/29] xen/arm: Create a fake timer node in dom0 device tree

[Julien Grall]

Recreate the timer node and remove hypervisor specific interrupt.

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

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c57a79d..3afe48f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -498,6 +498,67 @@ static int make_gic_node(const struct domain *d, void *fdt,
     return res;
 }
 
+static int make_timer_node(const struct domain *d, void *fdt,
+                           const struct dt_device_node *parent)
+{
+    static const struct dt_device_match timer_ids[] __initconst =
+    {
+        DT_MATCH_COMPATIBLE("arm,armv7-timer"),
+        DT_MATCH_COMPATIBLE("arm,armv8-timer"),
+        { /* sentinel */ },
+    };
+    struct dt_device_node *dev;
+    u32 len;
+    const void *compatible;
+    int res;
+    const struct dt_irq *irq;
+    interrupt_t intrs[3];
+
+    DPRINT("Create timer node\n");
+
+    dev = dt_find_matching_node(NULL, timer_ids);
+    if ( !dev )
+    {
+        dprintk(XENLOG_ERR, "Missing timer node in the device tree?\n");
+        return -FDT_ERR_XEN(ENOENT);
+    }
+
+    compatible = dt_get_property(dev, "compatible", &len);
+    if ( !compatible )
+    {
+        dprintk(XENLOG_ERR, "Can't find compatible property for timer node\n");
+        return -FDT_ERR_XEN(ENOENT);
+    }
+
+    res = fdt_begin_node(fdt, "timer");
+    if ( res )
+        return res;
+
+    res = fdt_property(fdt, "compatible", compatible, len);
+    if ( res )
+        return res;
+
+    irq = timer_dt_irq(TIMER_PHYS_SECURE_PPI);
+    DPRINT("  Secure interrupt %u\n", irq->irq);
+    set_interrupts(intrs[0], irq->irq, 0xf, irq->type);
+
+    irq = timer_dt_irq(TIMER_PHYS_NONSECURE_PPI);
+    DPRINT("  Non secure interrupt %u\n", irq->irq);
+    set_interrupts(intrs[1], irq->irq, 0xf, irq->type);
+
+    irq = timer_dt_irq(TIMER_VIRT_PPI);
+    DPRINT("  Virt interrupt %u\n", irq->irq);
+    set_interrupts(intrs[2], irq->irq, 0xf, irq->type);
+
+    res = fdt_property(fdt, "interrupts", intrs, sizeof (intrs[0]) * 3);
+    if ( res )
+        return res;
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
 /* Map the device in the domain */
 static int map_device(struct domain *d, const struct dt_device_node *dev)
 {
@@ -588,6 +649,8 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         DT_MATCH_COMPATIBLE("arm,psci"),
         DT_MATCH_PATH("/cpus"),
         DT_MATCH_COMPATIBLE("arm,cortex-a15-gic"),
+        DT_MATCH_COMPATIBLE("arm,armv7-timer"),
+        DT_MATCH_COMPATIBLE("arm,armv8-timer"),
         { /* sentinel */ },
     };
     const struct dt_device_node *child;
@@ -665,6 +728,10 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         res = make_gic_node(d, kinfo->fdt, np);
         if ( res )
             return res;
+
+        res = make_timer_node(d, kinfo->fdt, np);
+        if ( res )
+            return res;
     }
 
     res = fdt_end_node(kinfo->fdt);
-- 
1.7.10.4

Re: [PATCH V1 22/29] xen/arm: Create a fake timer node in dom0 device tree

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
[...]
> +    static const struct dt_device_match timer_ids[] __initconst =
> +    {
> +        DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> +        DT_MATCH_COMPATIBLE("arm,armv8-timer"),
> +        { /* sentinel */ },
> +    };
[...]
> @@ -588,6 +649,8 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>          DT_MATCH_COMPATIBLE("arm,psci"),
>          DT_MATCH_PATH("/cpus"),
>          DT_MATCH_COMPATIBLE("arm,cortex-a15-gic"),
> +        DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> +        DT_MATCH_COMPATIBLE("arm,armv8-timer"),

Both of these now need to be kept in sync with time.c. Can we find a
better way?

Ian

Re: [PATCH V1 22/29] xen/arm: Create a fake timer node in dom0 device tree

[Julien Grall]

On 09/09/2013 12:51 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> [...]
>> +    static const struct dt_device_match timer_ids[] __initconst =
>> +    {
>> +        DT_MATCH_COMPATIBLE("arm,armv7-timer"),
>> +        DT_MATCH_COMPATIBLE("arm,armv8-timer"),
>> +        { /* sentinel */ },
>> +    };
> [...]
>> @@ -588,6 +649,8 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>>          DT_MATCH_COMPATIBLE("arm,psci"),
>>          DT_MATCH_PATH("/cpus"),
>>          DT_MATCH_COMPATIBLE("arm,cortex-a15-gic"),
>> +        DT_MATCH_COMPATIBLE("arm,armv7-timer"),
>> +        DT_MATCH_COMPATIBLE("arm,armv8-timer"),
> 
> Both of these now need to be kept in sync with time.c. Can we find a
> better way?

What about the same solution as for the GIC?

#define TIMER_COMPATIBLE                   \
  DT_MATCH_COMPATIBLE("arm,armv7-timer"),  \
  DT_MATCH_COMPATIBLE("arm,armv8-timer")

-- 
Julien Grall

Re: [PATCH V1 22/29] xen/arm: Create a fake timer node in dom0 device tree

[Ian Campbell]

On Tue, 2013-09-10 at 11:56 +0100, Julien Grall wrote:
> On 09/09/2013 12:51 PM, Ian Campbell wrote:
> > On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> > [...]
> >> +    static const struct dt_device_match timer_ids[] __initconst =
> >> +    {
> >> +        DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> >> +        DT_MATCH_COMPATIBLE("arm,armv8-timer"),
> >> +        { /* sentinel */ },
> >> +    };
> > [...]
> >> @@ -588,6 +649,8 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
> >>          DT_MATCH_COMPATIBLE("arm,psci"),
> >>          DT_MATCH_PATH("/cpus"),
> >>          DT_MATCH_COMPATIBLE("arm,cortex-a15-gic"),
> >> +        DT_MATCH_COMPATIBLE("arm,armv7-timer"),
> >> +        DT_MATCH_COMPATIBLE("arm,armv8-timer"),
> > 
> > Both of these now need to be kept in sync with time.c. Can we find a
> > better way?
> 
> What about the same solution as for the GIC?

Sure.

[PATCH V1 23/29] xen/arm: Add new platform specific callback device_is_blacklist

[Julien Grall]

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

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

This callback is enough until we will start to care about power performance.
For this purpose, we may need to extend this interface to implement per-device
MMIO filtering to allow dom0 to continue to control devices which it owns which
happen to share e.g. a clock controller with Xen.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Update commit message
---
 xen/arch/arm/domain_build.c    |    2 +-
 xen/arch/arm/platform.c        |   10 ++++++++++
 xen/include/asm-arm/platform.h |    7 +++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

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

Re: [PATCH V1 23/29] xen/arm: Add new platform specific callback device_is_blacklist

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> Each platform code will list the device that must not pass-through to a guest.
> Theses devices are used for: power management, timer,...
> 
> When theses devices are given to DOM0, it can controls the hardware and then
> break the whole platform.
> 
> This callback is enough until we will start to care about power performance.
> For this purpose, we may need to extend this interface to implement per-device
> MMIO filtering to allow dom0 to continue to control devices which it owns which
> happen to share e.g. a clock controller with Xen.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Update commit message
> ---
>  xen/arch/arm/domain_build.c    |    2 +-
>  xen/arch/arm/platform.c        |   10 ++++++++++
>  xen/include/asm-arm/platform.h |    7 +++++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 3afe48f..1ac261e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -663,7 +663,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>      DPRINT("handle %s\n", path);
>  
>      /* Skip theses nodes and the sub-nodes */
> -    if ( dt_match_node(skip_matches, np ) )
> +    if ( dt_match_node(skip_matches, np ) || platform_device_is_blacklist(np) )

Should be "blacklisted". Other than that:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH V1 24/29] xen/arm: vexpress: Blacklist a list of board specific devices

[Julien Grall]

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

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

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Fix typoes in commit message
---
 xen/arch/arm/platforms/vexpress.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

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

Re: [PATCH V1 24/29] xen/arm: vexpress: Blacklist a list of board specific devices

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> On Versatile there are a bunch of devices which must not be pass-through to any
> guest (power management and cache coherency devices).
> 
> This commit also blacklist the HDLCD device because then is unable to correctly
                           ^s

"then ____ is unable" ? Missing word? "Linux" or "dom0" maybe? Or maybe
s/then/dom0/?

Is this a consequence of our HDLCD driver or just of lack of memreserve?

> map the framebuffer. Therefore, when Linux will try to access to the framebuffer,
> Xen will receive a non-handled data access.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Fix typoes in commit message
> ---
>  xen/arch/arm/platforms/vexpress.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c
> index 6f7dc2c..298c141 100644
> --- a/xen/arch/arm/platforms/vexpress.c
> +++ b/xen/arch/arm/platforms/vexpress.c
> @@ -125,9 +125,26 @@ static const char * const vexpress_dt_compat[] __initdata =
>      NULL
>  };
>  
> +static const struct dt_device_match vexpress_blacklist_dev[] __initconst =
> +{
> +    /* Cache Coherent Interconnect */
> +    DT_MATCH_COMPATIBLE("arm,cci-400"),
> +    DT_MATCH_COMPATIBLE("arm,cci-400-pmu"),
> +    /* Video device
> +     * TODO: remove it once memreserve is handled properly by Xen
> +     */
> +    DT_MATCH_COMPATIBLE("arm,hdlcd"),
> +    /* Hardware power management */
> +    DT_MATCH_COMPATIBLE("arm,vexpress-reset"),
> +    DT_MATCH_COMPATIBLE("arm,vexpress-reboot"),
> +    DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"),
> +    { /* sentinel */ },
> +};
> +
>  PLATFORM_START(vexpress, "VERSATILE EXPRESS")
>      .compatible = vexpress_dt_compat,
>      .reset = vexpress_reset,
> +    .blacklist_dev = vexpress_blacklist_dev,
>  PLATFORM_END
>  
>  /*

Re: [PATCH V1 24/29] xen/arm: vexpress: Blacklist a list of board specific devices

[Julien Grall]

On 09/09/2013 12:54 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> On Versatile there are a bunch of devices which must not be pass-through to any
>> guest (power management and cache coherency devices).
>>
>> This commit also blacklist the HDLCD device because then is unable to correctly
>                            ^s
> 
> "then ____ is unable" ? Missing word? "Linux" or "dom0" maybe? Or maybe
> s/then/dom0/?

Xen. I updated the commit message.

> Is this a consequence of our HDLCD driver or just of lack of memreserve?

It's a lack of memreserve.

> 
>> map the framebuffer. Therefore, when Linux will try to access to the framebuffer,
>> Xen will receive a non-handled data access.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v2:
>>         - Fix typoes in commit message
>> ---
>>  xen/arch/arm/platforms/vexpress.c |   17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c
>> index 6f7dc2c..298c141 100644
>> --- a/xen/arch/arm/platforms/vexpress.c
>> +++ b/xen/arch/arm/platforms/vexpress.c
>> @@ -125,9 +125,26 @@ static const char * const vexpress_dt_compat[] __initdata =
>>      NULL
>>  };
>>  
>> +static const struct dt_device_match vexpress_blacklist_dev[] __initconst =
>> +{
>> +    /* Cache Coherent Interconnect */
>> +    DT_MATCH_COMPATIBLE("arm,cci-400"),
>> +    DT_MATCH_COMPATIBLE("arm,cci-400-pmu"),
>> +    /* Video device
>> +     * TODO: remove it once memreserve is handled properly by Xen
>> +     */
>> +    DT_MATCH_COMPATIBLE("arm,hdlcd"),
>> +    /* Hardware power management */
>> +    DT_MATCH_COMPATIBLE("arm,vexpress-reset"),
>> +    DT_MATCH_COMPATIBLE("arm,vexpress-reboot"),
>> +    DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"),
>> +    { /* sentinel */ },
>> +};
>> +
>>  PLATFORM_START(vexpress, "VERSATILE EXPRESS")
>>      .compatible = vexpress_dt_compat,
>>      .reset = vexpress_reset,
>> +    .blacklist_dev = vexpress_blacklist_dev,
>>  PLATFORM_END
>>  
>>  /*
> 
> 


-- 
Julien Grall

[PATCH V1 25/29] xen/arm: exynos5: Blacklist MCT device

[Julien Grall]

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

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

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

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

Re: [PATCH V1 25/29] xen/arm: exynos5: Blacklist MCT device

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> The Multi Core Timer (MCT) is a Samsung specific device.
> This device tries to route IRQ in non-boot CPU which is not yet handled by Xen.
> 
> The user will see randomly dom0 hang, but I'm not sure that is the real reason.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

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

[PATCH V1 26/29] xen/dts: Clean up the exported API for device tree

[Julien Grall]

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

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

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    Changes in v2:
        - Don't remove device_tree_get_reg and don't export it
---
 xen/common/device_tree.c      |   92 +++++++----------------------------------
 xen/include/xen/device_tree.h |   15 -------
 2 files changed, 14 insertions(+), 93 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 5b4dde3..b120585 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -103,8 +103,8 @@ struct dt_bus
     unsigned int (*get_flags)(const __be32 *addr);
 };
 
-bool_t __init device_tree_node_matches(const void *fdt, int node,
-                                       const char *match)
+static bool_t __init device_tree_node_matches(const void *fdt, int node,
+                                              const char *match)
 {
     const char *name;
     size_t match_len;
@@ -118,7 +118,7 @@ bool_t __init device_tree_node_matches(const void *fdt, int node,
         && (name[match_len] == '@' || name[match_len] == '\0');
 }
 
-bool_t __init device_tree_type_matches(const void *fdt, int node,
+static bool_t __init device_tree_type_matches(const void *fdt, int node,
                                        const char *match)
 {
     const void *prop;
@@ -130,8 +130,8 @@ bool_t __init device_tree_type_matches(const void *fdt, int node,
     return !dt_node_cmp(prop, match);
 }
 
-bool_t __init device_tree_node_compatible(const void *fdt, int node,
-                                          const char *match)
+static bool_t __init device_tree_node_compatible(const void *fdt, int node,
+                                                 const char *match)
 {
     int len, l;
     int mlen;
@@ -154,13 +154,6 @@ bool_t __init device_tree_node_compatible(const void *fdt, int node,
     return 0;
 }
 
-static __init int device_tree_nr_reg_ranges(const struct fdt_property *prop,
-        u32 address_cells, u32 size_cells)
-{
-    u32 reg_cells = address_cells + size_cells;
-    return fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32));
-}
-
 static void __init get_val(const u32 **cell, u32 cells, u64 *val)
 {
     *val = 0;
@@ -175,8 +168,8 @@ static void __init get_val(const u32 **cell, u32 cells, u64 *val)
     }
 }
 
-void __init device_tree_get_reg(const u32 **cell, u32 address_cells,
-                                u32 size_cells, u64 *start, u64 *size)
+static void __init device_tree_get_reg(const u32 **cell, u32 address_cells,
+                                       u32 size_cells, u64 *start, u64 *size)
 {
     get_val(cell, address_cells, start);
     get_val(cell, size_cells, size);
@@ -202,13 +195,6 @@ void dt_set_cell(__be32 **cellp, int size, u64 val)
     (*cellp) += cells;
 }
 
-void __init device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
-                                u64 start, u64 size)
-{
-    dt_set_cell(cell, address_cells, start);
-    dt_set_cell(cell, size_cells, size);
-}
-
 void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
                   u64 address, u64 size)
 {
@@ -216,8 +202,8 @@ void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
     dt_set_cell(cellp, dt_n_size_cells(np), size);
 }
 
-u32 __init device_tree_get_u32(const void *fdt, int node, const char *prop_name,
-                               u32 dflt)
+static u32 __init device_tree_get_u32(const void *fdt, int node,
+                                      const char *prop_name, u32 dflt)
 {
     const struct fdt_property *prop;
 
@@ -239,8 +225,9 @@ u32 __init device_tree_get_u32(const void *fdt, int node, const char *prop_name,
  * Returns 0 if all nodes were iterated over successfully.  If @func
  * returns a value different from 0, that value is returned immediately.
  */
-int __init device_tree_for_each_node(const void *fdt,
-                                     device_tree_node_func func, void *data)
+static int __init device_tree_for_each_node(const void *fdt,
+                                            device_tree_node_func func,
+                                            void *data)
 {
     int node;
     int depth;
@@ -275,58 +262,6 @@ int __init device_tree_for_each_node(const void *fdt,
     return 0;
 }
 
-struct find_compat {
-    const char *compatible;
-    int found;
-    int node;
-    int depth;
-    u32 address_cells;
-    u32 size_cells;
-};
-
-static int _find_compatible_node(const void *fdt,
-                             int node, const char *name, int depth,
-                             u32 address_cells, u32 size_cells,
-                             void *data)
-{
-    struct find_compat *c = (struct find_compat *) data;
-
-    if (  c->found  )
-        return 1;
-
-    if ( device_tree_node_compatible(fdt, node, c->compatible) )
-    {
-        c->found = 1;
-        c->node = node;
-        c->depth = depth;
-        c->address_cells = address_cells;
-        c->size_cells = size_cells;
-        return 1;
-    }
-    return 0;
-}
-
-int __init find_compatible_node(const char *compatible, int *node, int *depth,
-                                u32 *address_cells, u32 *size_cells)
-{
-    int ret;
-    struct find_compat c;
-    c.compatible = compatible;
-    c.found = 0;
-
-    ret = device_tree_for_each_node(device_tree_flattened, _find_compatible_node, &c);
-    if ( !c.found )
-        return ret;
-    else
-    {
-        *node = c.node;
-        *depth = c.depth;
-        *address_cells = c.address_cells;
-        *size_cells = c.size_cells;
-        return 1;
-    }
-}
-
 /**
  * device_tree_bootargs - return the bootargs (the Xen command line)
  * @fdt flat device tree.
@@ -394,6 +329,7 @@ static void __init process_memory_node(const void *fdt, int node,
     int banks;
     const u32 *cell;
     paddr_t start, size;
+    u32 reg_cells = address_cells + size_cells;
 
     if ( address_cells < 1 || size_cells < 1 )
     {
@@ -410,7 +346,7 @@ static void __init process_memory_node(const void *fdt, int node,
     }
 
     cell = (const u32 *)prop->data;
-    banks = device_tree_nr_reg_ranges(prop, address_cells, size_cells);
+    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
 
     for ( i = 0; i < banks && early_info.mem.nr_banks < NR_MEM_BANKS; i++ )
     {
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index fa4f314..160c4b4 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -181,21 +181,6 @@ extern const void *device_tree_flattened;
 
 size_t __init device_tree_early_init(const void *fdt);
 
-void __init device_tree_get_reg(const u32 **cell, u32 address_cells,
-                                u32 size_cells,
-                                u64 *start, u64 *size);
-void __init device_tree_set_reg(u32 **cell, u32 address_cells, u32 size_cells,
-                                u64 start, u64 size);
-u32 __init device_tree_get_u32(const void *fdt, int node,
-                               const char *prop_name, u32 dflt);
-bool_t __init device_tree_node_matches(const void *fdt, int node,
-                                       const char *match);
-bool_t __init device_tree_node_compatible(const void *fdt, int node,
-                                          const char *match);
-int __init find_compatible_node(const char *compatible, int *node, int *depth,
-                                u32 *address_cells, u32 *size_cells);
-int __init device_tree_for_each_node(const void *fdt,
-                                     device_tree_node_func func, void *data);
 const char __init *device_tree_bootargs(const void *fdt);
 void __init device_tree_dump(const void *fdt);
 
-- 
1.7.10.4

[PATCH V1 27/29] xen/dts: device_get_reg: cells are 32 bits big endian value

[Julien Grall]

Device tree cells are 32-bit big endian value. Use __be32 to avoid confusion
later. Also replace get_val call by dt_next_cell.

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

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index b120585..4bc1ce4 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -154,25 +154,11 @@ static bool_t __init device_tree_node_compatible(const void *fdt, int node,
     return 0;
 }
 
-static void __init get_val(const u32 **cell, u32 cells, u64 *val)
-{
-    *val = 0;
-
-    if ( cells > 2 )
-        early_panic("dtb value contains > 2 cells\n");
-
-    while ( cells-- )
-    {
-        *val <<= 32;
-        *val |= fdt32_to_cpu(*(*cell)++);
-    }
-}
-
-static void __init device_tree_get_reg(const u32 **cell, u32 address_cells,
+static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
                                        u32 size_cells, u64 *start, u64 *size)
 {
-    get_val(cell, address_cells, start);
-    get_val(cell, size_cells, size);
+    *start = dt_next_cell(address_cells, cell);
+    *size = dt_next_cell(size_cells, cell);
 }
 
 void dt_get_range(const __be32 **cell, const struct dt_device_node *np,
@@ -327,7 +313,7 @@ static void __init process_memory_node(const void *fdt, int node,
     const struct fdt_property *prop;
     int i;
     int banks;
-    const u32 *cell;
+    const __be32 *cell;
     paddr_t start, size;
     u32 reg_cells = address_cells + size_cells;
 
@@ -345,7 +331,7 @@ static void __init process_memory_node(const void *fdt, int node,
         return;
     }
 
-    cell = (const u32 *)prop->data;
+    cell = (const __be32 *)prop->data;
     banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
 
     for ( i = 0; i < banks && early_info.mem.nr_banks < NR_MEM_BANKS; i++ )
@@ -396,7 +382,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
                                           u32 address_cells, u32 size_cells)
 {
     const struct fdt_property *prop;
-    const u32 *cell;
+    const __be32 *cell;
     int nr;
     struct dt_mb_module *mod;
     int len;
@@ -418,7 +404,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
         early_panic("fdt: node `%s': `reg` property length is too short\n",
                     name);
 
-    cell = (const u32 *)prop->data;
+    cell = (const __be32 *)prop->data;
     device_tree_get_reg(&cell, address_cells, size_cells,
                         &mod->start, &mod->size);
 
-- 
1.7.10.4

Re: [PATCH V1 27/29] xen/dts: device_get_reg: cells are 32 bits big endian value

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> Device tree cells are 32-bit big endian value. Use __be32 to avoid confusion
> later.

These are only used by sparse, right? So we wouldn't expect this to
actually save us from any mistakes unless we make Xen support the use of
sparse? (IOW I shouldn't rely on this and not think about this too hard
during patch review because the compiler will get it right for me)

> Also replace get_val call by dt_next_cell.

Would prefer this separately.

> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>
(on both patches if you split)

> ---
>  xen/common/device_tree.c |   28 +++++++---------------------
>  1 file changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index b120585..4bc1ce4 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -154,25 +154,11 @@ static bool_t __init device_tree_node_compatible(const void *fdt, int node,
>      return 0;
>  }
>  
> -static void __init get_val(const u32 **cell, u32 cells, u64 *val)
> -{
> -    *val = 0;
> -
> -    if ( cells > 2 )
> -        early_panic("dtb value contains > 2 cells\n");
> -
> -    while ( cells-- )
> -    {
> -        *val <<= 32;
> -        *val |= fdt32_to_cpu(*(*cell)++);
> -    }
> -}
> -
> -static void __init device_tree_get_reg(const u32 **cell, u32 address_cells,
> +static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
>                                         u32 size_cells, u64 *start, u64 *size)
>  {
> -    get_val(cell, address_cells, start);
> -    get_val(cell, size_cells, size);
> +    *start = dt_next_cell(address_cells, cell);
> +    *size = dt_next_cell(size_cells, cell);
>  }
>  
>  void dt_get_range(const __be32 **cell, const struct dt_device_node *np,
> @@ -327,7 +313,7 @@ static void __init process_memory_node(const void *fdt, int node,
>      const struct fdt_property *prop;
>      int i;
>      int banks;
> -    const u32 *cell;
> +    const __be32 *cell;
>      paddr_t start, size;
>      u32 reg_cells = address_cells + size_cells;
>  
> @@ -345,7 +331,7 @@ static void __init process_memory_node(const void *fdt, int node,
>          return;
>      }
>  
> -    cell = (const u32 *)prop->data;
> +    cell = (const __be32 *)prop->data;
>      banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
>  
>      for ( i = 0; i < banks && early_info.mem.nr_banks < NR_MEM_BANKS; i++ )
> @@ -396,7 +382,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
>                                            u32 address_cells, u32 size_cells)
>  {
>      const struct fdt_property *prop;
> -    const u32 *cell;
> +    const __be32 *cell;
>      int nr;
>      struct dt_mb_module *mod;
>      int len;
> @@ -418,7 +404,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
>          early_panic("fdt: node `%s': `reg` property length is too short\n",
>                      name);
>  
> -    cell = (const u32 *)prop->data;
> +    cell = (const __be32 *)prop->data;
>      device_tree_get_reg(&cell, address_cells, size_cells,
>                          &mod->start, &mod->size);
>  

Re: [PATCH V1 27/29] xen/dts: device_get_reg: cells are 32 bits big endian value

[Julien Grall]

On 09/09/2013 12:57 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> Device tree cells are 32-bit big endian value. Use __be32 to avoid confusion
>> later.
> 
> These are only used by sparse, right? So we wouldn't expect this to
> actually save us from any mistakes unless we make Xen support the use of
> sparse? (IOW I shouldn't rely on this and not think about this too hard
> during patch review because the compiler will get it right for me)

Rigth. I think it also helps the developer to know that value may not
have the same endianess as the processor.

>> Also replace get_val call by dt_next_cell.
> 
> Would prefer this separately.
> 
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> (on both patches if you split)

I will split the patch in 2 parts.

> 
>> ---
>>  xen/common/device_tree.c |   28 +++++++---------------------
>>  1 file changed, 7 insertions(+), 21 deletions(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index b120585..4bc1ce4 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -154,25 +154,11 @@ static bool_t __init device_tree_node_compatible(const void *fdt, int node,
>>      return 0;
>>  }
>>  
>> -static void __init get_val(const u32 **cell, u32 cells, u64 *val)
>> -{
>> -    *val = 0;
>> -
>> -    if ( cells > 2 )
>> -        early_panic("dtb value contains > 2 cells\n");
>> -
>> -    while ( cells-- )
>> -    {
>> -        *val <<= 32;
>> -        *val |= fdt32_to_cpu(*(*cell)++);
>> -    }
>> -}
>> -
>> -static void __init device_tree_get_reg(const u32 **cell, u32 address_cells,
>> +static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
>>                                         u32 size_cells, u64 *start, u64 *size)
>>  {
>> -    get_val(cell, address_cells, start);
>> -    get_val(cell, size_cells, size);
>> +    *start = dt_next_cell(address_cells, cell);
>> +    *size = dt_next_cell(size_cells, cell);
>>  }
>>  
>>  void dt_get_range(const __be32 **cell, const struct dt_device_node *np,
>> @@ -327,7 +313,7 @@ static void __init process_memory_node(const void *fdt, int node,
>>      const struct fdt_property *prop;
>>      int i;
>>      int banks;
>> -    const u32 *cell;
>> +    const __be32 *cell;
>>      paddr_t start, size;
>>      u32 reg_cells = address_cells + size_cells;
>>  
>> @@ -345,7 +331,7 @@ static void __init process_memory_node(const void *fdt, int node,
>>          return;
>>      }
>>  
>> -    cell = (const u32 *)prop->data;
>> +    cell = (const __be32 *)prop->data;
>>      banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
>>  
>>      for ( i = 0; i < banks && early_info.mem.nr_banks < NR_MEM_BANKS; i++ )
>> @@ -396,7 +382,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
>>                                            u32 address_cells, u32 size_cells)
>>  {
>>      const struct fdt_property *prop;
>> -    const u32 *cell;
>> +    const __be32 *cell;
>>      int nr;
>>      struct dt_mb_module *mod;
>>      int len;
>> @@ -418,7 +404,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
>>          early_panic("fdt: node `%s': `reg` property length is too short\n",
>>                      name);
>>  
>> -    cell = (const u32 *)prop->data;
>> +    cell = (const __be32 *)prop->data;
>>      device_tree_get_reg(&cell, address_cells, size_cells,
>>                          &mod->start, &mod->size);
>>  
> 
> 


-- 
Julien Grall

[PATCH V1 28/29] xen/arm: Check if the device is available before using it

[Julien Grall]

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

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

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

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

[PATCH V1 29/29] ARM: parse separate DT properties for different commandlines

[Julien Grall]

From: Andre Przywara <andre.przywara@linaro.org>

Currently we use the chosen/bootargs property as the Xen commandline
and rely on xen,dom0-bootargs for Dom0. However this brings issues
with bootloaders, which usually build bootargs by bootscripts for a
Linux kernel - and not for the entirely different Xen hypervisor.

Introduce a new possible device tree property "xen,xen-bootargs"
explicitly for the Xen hypervisor and make the selection of which to
use more fine grained:
- If xen,xen-bootargs is present, it will be used for Xen.
- If xen,dom0-bootargs is present, it will be used for Dom0.
- If xen,xen-bootargs is _not_ present, but xen,dom0-bootargs is,
  bootargs will be used for Xen. Like the current situation.
- If no Xen specific properties are present, bootargs is for Dom0.
- If xen,xen-bootargs is present, but xen,dom0-bootargs is missing,
  bootargs will be used for Dom0.

The aim is to allow common bootscripts to boot both Xen and native
Linux with the same device tree blob. If needed, one could hard-code
the Xen commandline into the DTB, leaving bootargs for Dom0 to be set
by the (non Xen-aware) bootloader.

I will send out a appropriate u-boot patch, which writes the content
of the "xen_bootargs" environment variable into the xen,xen-bootargs
dtb property.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
    Changes in v2:
        - Add and rebase this patch to this patch series

    Changes before the patch was added to this patch series:
    v1 .. v2:
     - fix whitespace issues
    v2 .. v3:
     - add documentation
---
 docs/misc/arm/device-tree/booting.txt |   28 +++++++++++++++++++++++++++-
 xen/arch/arm/domain_build.c           |   15 +++++++++++----
 xen/common/device_tree.c              |    7 ++++++-
 3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 94cd3f1..08ed775 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -1,3 +1,6 @@
+Dom0 kernel and ramdisk modules
+================================
+
 Xen is passed the dom0 kernel and initrd via a reference in the /chosen
 node of the device tree.
 
@@ -22,4 +25,27 @@ properties:
 
 - bootargs (optional)
 
-	Command line associated with this module
+	Command line associated with this module. This is deprecated and should
+	be replaced by the bootargs variations described below.
+
+
+Command lines
+=============
+
+Xen also checks for properties directly under /chosen to find suitable command
+lines for Xen and Dom0. The logic is the following:
+
+ - If xen,xen-bootargs is present, it will be used for Xen.
+ - If xen,dom0-bootargs is present, it will be used for Dom0.
+ - If xen,xen-bootargs is _not_ present, but xen,dom0-bootargs is,
+   bootargs will be used for Xen.
+ - If no Xen specific properties are present, bootargs is for Dom0.
+ - If xen,xen-bootargs is present, but xen,dom0-bootargs is missing,
+   bootargs will be used for Dom0.
+
+Most of these cases is to make booting with Xen-unaware bootloaders easier.
+For those you would hardcode the Xen commandline in the DTB under
+/chosen/xen,xen-bootargs and would let the bootloader set the Dom0 command
+line by writing bootargs (as for native Linux).
+A Xen-aware bootloader would set xen,xen-bootargs for Xen, xen,dom0-bootargs
+for Dom0 and bootargs for native Linux.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 1ac261e..e45e0e7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -146,6 +146,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
     const char *bootargs = NULL;
     const struct dt_property *pp;
     int res = 0;
+    int had_dom0_bootargs = 0;
 
     if ( early_info.modules.nr_mods >= MOD_KERNEL &&
          early_info.modules.module[MOD_KERNEL].cmdline[0] )
@@ -162,15 +163,21 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
          *
          * * remember xen,dom0-bootargs if we don't already have
          *   bootargs (from module #1, above).
-         * * remove bootargs and xen,dom0-bootargs.
+         * * remove bootargs,  xen,dom0-bootargs and xen,xen-bootargs.
          */
         if ( dt_node_path_is_equal(np, "/chosen") )
         {
-            if ( dt_property_name_is_equal(pp, "bootargs") )
+            if ( dt_property_name_is_equal(pp, "xen,xen-bootargs") )
+                continue;
+            if ( dt_property_name_is_equal(pp, "xen,dom0-bootargs") )
+            {
+                had_dom0_bootargs = 1;
+                bootargs = pp->value;
                 continue;
-            else if ( dt_property_name_is_equal(pp, "xen,dom0-bootargs") )
+            }
+            if ( dt_property_name_is_equal(pp, "bootargs") )
             {
-                if ( !bootargs )
+                if ( !bootargs  && !had_dom0_bootargs )
                     bootargs = pp->value;
                 continue;
             }
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 4bc1ce4..5620b23 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -261,7 +261,12 @@ const char *device_tree_bootargs(const void *fdt)
     if ( node < 0 )
         return NULL;
 
-    prop = fdt_get_property(fdt, node, "bootargs", NULL);
+    prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
+    if ( prop == NULL )
+    {
+        if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL))
+            prop = fdt_get_property(fdt, node, "bootargs", NULL);
+    }
     if ( prop == NULL )
         return NULL;
 
-- 
1.7.10.4

Re: [PATCH V1 29/29] ARM: parse separate DT properties for different commandlines

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
> From: Andre Przywara <andre.przywara@linaro.org>
> 
> Currently we use the chosen/bootargs property as the Xen commandline
> and rely on xen,dom0-bootargs for Dom0. However this brings issues
> with bootloaders, which usually build bootargs by bootscripts for a
> Linux kernel - and not for the entirely different Xen hypervisor.
> 
> Introduce a new possible device tree property "xen,xen-bootargs"
> explicitly for the Xen hypervisor and make the selection of which to
> use more fine grained:
> - If xen,xen-bootargs is present, it will be used for Xen.
> - If xen,dom0-bootargs is present, it will be used for Dom0.
> - If xen,xen-bootargs is _not_ present, but xen,dom0-bootargs is,
>   bootargs will be used for Xen. Like the current situation.
> - If no Xen specific properties are present, bootargs is for Dom0.
> - If xen,xen-bootargs is present, but xen,dom0-bootargs is missing,
>   bootargs will be used for Dom0.
> 
> The aim is to allow common bootscripts to boot both Xen and native
> Linux with the same device tree blob. If needed, one could hard-code
> the Xen commandline into the DTB, leaving bootargs for Dom0 to be set
> by the (non Xen-aware) bootloader.
> 
> I will send out a appropriate u-boot patch, which writes the content
> of the "xen_bootargs" environment variable into the xen,xen-bootargs
> dtb property.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

Do we want/need to wait for the result of the bindings proposal Andre
made?

This is unrelated to Andre's "use more flexible node naming", isn't it?

> ---
>     Changes in v2:
>         - Add and rebase this patch to this patch series
> 
>     Changes before the patch was added to this patch series:
>     v1 .. v2:
>      - fix whitespace issues
>     v2 .. v3:
>      - add documentation
> ---
>  docs/misc/arm/device-tree/booting.txt |   28 +++++++++++++++++++++++++++-
>  xen/arch/arm/domain_build.c           |   15 +++++++++++----
>  xen/common/device_tree.c              |    7 ++++++-
>  3 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 94cd3f1..08ed775 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -1,3 +1,6 @@
> +Dom0 kernel and ramdisk modules
> +================================
> +
>  Xen is passed the dom0 kernel and initrd via a reference in the /chosen
>  node of the device tree.
>  
> @@ -22,4 +25,27 @@ properties:
>  
>  - bootargs (optional)
>  
> -	Command line associated with this module
> +	Command line associated with this module. This is deprecated and should
> +	be replaced by the bootargs variations described below.
> +
> +
> +Command lines
> +=============
> +
> +Xen also checks for properties directly under /chosen to find suitable command
> +lines for Xen and Dom0. The logic is the following:
> +
> + - If xen,xen-bootargs is present, it will be used for Xen.
> + - If xen,dom0-bootargs is present, it will be used for Dom0.
> + - If xen,xen-bootargs is _not_ present, but xen,dom0-bootargs is,
> +   bootargs will be used for Xen.
> + - If no Xen specific properties are present, bootargs is for Dom0.
> + - If xen,xen-bootargs is present, but xen,dom0-bootargs is missing,
> +   bootargs will be used for Dom0.
> +
> +Most of these cases is to make booting with Xen-unaware bootloaders easier.
> +For those you would hardcode the Xen commandline in the DTB under
> +/chosen/xen,xen-bootargs and would let the bootloader set the Dom0 command
> +line by writing bootargs (as for native Linux).
> +A Xen-aware bootloader would set xen,xen-bootargs for Xen, xen,dom0-bootargs
> +for Dom0 and bootargs for native Linux.
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 1ac261e..e45e0e7 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -146,6 +146,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>      const char *bootargs = NULL;
>      const struct dt_property *pp;
>      int res = 0;
> +    int had_dom0_bootargs = 0;
>  
>      if ( early_info.modules.nr_mods >= MOD_KERNEL &&
>           early_info.modules.module[MOD_KERNEL].cmdline[0] )
> @@ -162,15 +163,21 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>           *
>           * * remember xen,dom0-bootargs if we don't already have
>           *   bootargs (from module #1, above).
> -         * * remove bootargs and xen,dom0-bootargs.
> +         * * remove bootargs,  xen,dom0-bootargs and xen,xen-bootargs.
>           */
>          if ( dt_node_path_is_equal(np, "/chosen") )
>          {
> -            if ( dt_property_name_is_equal(pp, "bootargs") )
> +            if ( dt_property_name_is_equal(pp, "xen,xen-bootargs") )
> +                continue;
> +            if ( dt_property_name_is_equal(pp, "xen,dom0-bootargs") )
> +            {
> +                had_dom0_bootargs = 1;
> +                bootargs = pp->value;
>                  continue;
> -            else if ( dt_property_name_is_equal(pp, "xen,dom0-bootargs") )
> +            }
> +            if ( dt_property_name_is_equal(pp, "bootargs") )
>              {
> -                if ( !bootargs )
> +                if ( !bootargs  && !had_dom0_bootargs )
>                      bootargs = pp->value;
>                  continue;
>              }
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 4bc1ce4..5620b23 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -261,7 +261,12 @@ const char *device_tree_bootargs(const void *fdt)
>      if ( node < 0 )
>          return NULL;
>  
> -    prop = fdt_get_property(fdt, node, "bootargs", NULL);
> +    prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
> +    if ( prop == NULL )
> +    {
> +        if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL))
> +            prop = fdt_get_property(fdt, node, "bootargs", NULL);
> +    }
>      if ( prop == NULL )
>          return NULL;
>  

Re: [PATCH V1 29/29] ARM: parse separate DT properties for different commandlines

[Andre Przywara]

On 09/09/2013 01:59 PM, Ian Campbell wrote:
> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>> From: Andre Przywara <andre.przywara@linaro.org>
>>
>> Currently we use the chosen/bootargs property as the Xen commandline
>> and rely on xen,dom0-bootargs for Dom0. However this brings issues
>> with bootloaders, which usually build bootargs by bootscripts for a
>> Linux kernel - and not for the entirely different Xen hypervisor.
>>
>> Introduce a new possible device tree property "xen,xen-bootargs"
>> explicitly for the Xen hypervisor and make the selection of which to
>> use more fine grained:
>> - If xen,xen-bootargs is present, it will be used for Xen.
>> - If xen,dom0-bootargs is present, it will be used for Dom0.
>> - If xen,xen-bootargs is _not_ present, but xen,dom0-bootargs is,
>>    bootargs will be used for Xen. Like the current situation.
>> - If no Xen specific properties are present, bootargs is for Dom0.
>> - If xen,xen-bootargs is present, but xen,dom0-bootargs is missing,
>>    bootargs will be used for Dom0.
>>
>> The aim is to allow common bootscripts to boot both Xen and native
>> Linux with the same device tree blob. If needed, one could hard-code
>> the Xen commandline into the DTB, leaving bootargs for Dom0 to be set
>> by the (non Xen-aware) bootloader.
>>
>> I will send out a appropriate u-boot patch, which writes the content
>> of the "xen_bootargs" environment variable into the xen,xen-bootargs
>> dtb property.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Do we want/need to wait for the result of the bindings proposal Andre
> made?
>
> This is unrelated to Andre's "use more flexible node naming", isn't it?

Yes and no.

Unrelated: the code does not conflict and it adds flexibility in 
booting, allowing less capable boot loaders to start Xen.
Related: The flexible node naming solves this particular issue in a more 
generic manner.
But as I think one goal was to make Xen boot on every toaster, we may 
keep this patch (or a rebased version of it) for flexibility.

Regards,
Andre.

>> ---
>>      Changes in v2:
>>          - Add and rebase this patch to this patch series
>>
>>      Changes before the patch was added to this patch series:
>>      v1 .. v2:
>>       - fix whitespace issues
>>      v2 .. v3:
>>       - add documentation
>> ---
>>   docs/misc/arm/device-tree/booting.txt |   28 +++++++++++++++++++++++++++-
>>   xen/arch/arm/domain_build.c           |   15 +++++++++++----
>>   xen/common/device_tree.c              |    7 ++++++-
>>   3 files changed, 44 insertions(+), 6 deletions(-)
>>
>> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
>> index 94cd3f1..08ed775 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -1,3 +1,6 @@
>> +Dom0 kernel and ramdisk modules
>> +================================
>> +
>>   Xen is passed the dom0 kernel and initrd via a reference in the /chosen
>>   node of the device tree.
>>
>> @@ -22,4 +25,27 @@ properties:
>>
>>   - bootargs (optional)
>>
>> -	Command line associated with this module
>> +	Command line associated with this module. This is deprecated and should
>> +	be replaced by the bootargs variations described below.
>> +
>> +
>> +Command lines
>> +=============
>> +
>> +Xen also checks for properties directly under /chosen to find suitable command
>> +lines for Xen and Dom0. The logic is the following:
>> +
>> + - If xen,xen-bootargs is present, it will be used for Xen.
>> + - If xen,dom0-bootargs is present, it will be used for Dom0.
>> + - If xen,xen-bootargs is _not_ present, but xen,dom0-bootargs is,
>> +   bootargs will be used for Xen.
>> + - If no Xen specific properties are present, bootargs is for Dom0.
>> + - If xen,xen-bootargs is present, but xen,dom0-bootargs is missing,
>> +   bootargs will be used for Dom0.
>> +
>> +Most of these cases is to make booting with Xen-unaware bootloaders easier.
>> +For those you would hardcode the Xen commandline in the DTB under
>> +/chosen/xen,xen-bootargs and would let the bootloader set the Dom0 command
>> +line by writing bootargs (as for native Linux).
>> +A Xen-aware bootloader would set xen,xen-bootargs for Xen, xen,dom0-bootargs
>> +for Dom0 and bootargs for native Linux.
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 1ac261e..e45e0e7 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -146,6 +146,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>>       const char *bootargs = NULL;
>>       const struct dt_property *pp;
>>       int res = 0;
>> +    int had_dom0_bootargs = 0;
>>
>>       if ( early_info.modules.nr_mods >= MOD_KERNEL &&
>>            early_info.modules.module[MOD_KERNEL].cmdline[0] )
>> @@ -162,15 +163,21 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>>            *
>>            * * remember xen,dom0-bootargs if we don't already have
>>            *   bootargs (from module #1, above).
>> -         * * remove bootargs and xen,dom0-bootargs.
>> +         * * remove bootargs,  xen,dom0-bootargs and xen,xen-bootargs.
>>            */
>>           if ( dt_node_path_is_equal(np, "/chosen") )
>>           {
>> -            if ( dt_property_name_is_equal(pp, "bootargs") )
>> +            if ( dt_property_name_is_equal(pp, "xen,xen-bootargs") )
>> +                continue;
>> +            if ( dt_property_name_is_equal(pp, "xen,dom0-bootargs") )
>> +            {
>> +                had_dom0_bootargs = 1;
>> +                bootargs = pp->value;
>>                   continue;
>> -            else if ( dt_property_name_is_equal(pp, "xen,dom0-bootargs") )
>> +            }
>> +            if ( dt_property_name_is_equal(pp, "bootargs") )
>>               {
>> -                if ( !bootargs )
>> +                if ( !bootargs  && !had_dom0_bootargs )
>>                       bootargs = pp->value;
>>                   continue;
>>               }
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 4bc1ce4..5620b23 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -261,7 +261,12 @@ const char *device_tree_bootargs(const void *fdt)
>>       if ( node < 0 )
>>           return NULL;
>>
>> -    prop = fdt_get_property(fdt, node, "bootargs", NULL);
>> +    prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
>> +    if ( prop == NULL )
>> +    {
>> +        if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL))
>> +            prop = fdt_get_property(fdt, node, "bootargs", NULL);
>> +    }
>>       if ( prop == NULL )
>>           return NULL;
>>
>
>

Re: [PATCH V1 00/29] Allow Xen to boot with a raw Device Tree

[Ian Campbell]

On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote:
>   xen: Introduce __initconst to store initial const data
>   xen/dts: Don't check the number of address and size cells in
>     process_cpu_node
>   xen/arm: Move __PSCI* from traps.c to the header
>   xen: Add new string function

I applied these few.

Skipping this one from the middle:
>  xen/dts: Constify device_tree_flattened

Ian.