All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support
@ 2014-06-16 11:44 Ian Campbell
  2014-06-16 11:44 ` [PATCH 01/10] xen: arm: implement generic multiboot compatibility strings Ian Campbell
                   ` (10 more replies)
  0 siblings, 11 replies; 35+ messages in thread
From: Ian Campbell @ 2014-06-16 11:44 UTC (permalink / raw)
  To: xen-devel, Stefano Stabellini, Julien Grall, Tim Deegan
  Cc: Roy Franz, Naresh Bhat, Fu Wei

The following refactors the Xen early device tree stuff (i.e. which
walks the flattened tree directly) away from the regular device tree
stuff (i.e. the stuff which for the most part deals with the unflattened
tree). It also makes some changes to the Xen side multiboot support
which I think will make it easier to work with, both internally and for
e.g. bootloader integration.

Impact on UEFI/ACPI: Mostly I think the refactoring may be useful when
integrating the UEFI memory map and ACPI stuff (which wants early FDT,
but not unflatening etc) in to Xen.

Impact on multiboot: This could potentially simplify things on the grub
side by removing the need to guess default types for the modules in the
common case.

In the future I think it would be good to implement more probing on the
Xen side, e.g. to discover the XSM policy (similar to how it works on
x86 -- which walks all the modules looking for the policy magic nr).

The first two patches here have been posted before.

Ian.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH 01/10] xen: arm: implement generic multiboot compatibility strings
  2014-06-16 11:44 [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Ian Campbell
@ 2014-06-16 11:44 ` Ian Campbell
  2014-06-16 11:44 ` [PATCH 02/10] xen: arm: /chosen/module@N/bootargs bootprotcol node is not deprecated Ian Campbell
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-06-16 11:44 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Campbell, stefano.stabellini, Naresh Bhat, julien.grall, tim,
	Roy Franz, Fu Wei

This causes Xen to accept the more generic names specified in
http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot as of
2014-06-06.

These names are more generic than those proposed by Andre in
http://thread.gmane.org/gmane.linux.linaro.announce.boot/326 and those
used in earlier drafts of the /Multiboot wiki page.

This will allow bootloaders to not special case Xen (or at least to reduce
the amount which is required).

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

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index f0b17a3..48ae849 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -338,9 +338,11 @@ static void __init process_multiboot_node(const void *fdt, int node,
     struct dt_mb_module *mod;
     int len;
 
-    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 )
+    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
+         fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
         nr = MOD_KERNEL;
-    else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0)
+    else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0 ||
+              fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
         nr = MOD_INITRD;
     else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
         nr = MOD_XSM;
@@ -433,7 +435,8 @@ static int __init early_scan_node(const void *fdt,
 {
     if ( device_tree_node_matches(fdt, node, "memory") )
         process_memory_node(fdt, node, name, address_cells, size_cells);
-    else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) )
+    else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
+              device_tree_node_compatible(fdt, node, "multiboot,module" ))
         process_multiboot_node(fdt, node, name, address_cells, size_cells);
     else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
         process_chosen_node(fdt, node, name, address_cells, size_cells);
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 02/10] xen: arm: /chosen/module@N/bootargs bootprotcol node is not deprecated
  2014-06-16 11:44 [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Ian Campbell
  2014-06-16 11:44 ` [PATCH 01/10] xen: arm: implement generic multiboot compatibility strings Ian Campbell
@ 2014-06-16 11:44 ` Ian Campbell
  2014-06-16 11:44 ` [PATCH 03/10] xen: arm: prefer typesafe max()/min() over MAX()/MIN() Ian Campbell
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-06-16 11:44 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Campbell, stefano.stabellini, Naresh Bhat, julien.grall, tim,
	Roy Franz, Fu Wei

When using a multiboot capable bootloader this is exactly the field which
should be used.

Replace the deprecation wording with a reference to the information on the
priority of the bootargs fields.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
---
 docs/misc/arm/device-tree/booting.txt |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index a7fb52b..bfb8d01 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -25,8 +25,9 @@ Each node contains the following properties:
 
 - bootargs (optional)
 
-	Command line associated with this module. This is deprecated and should
-	be replaced by the bootargs variations described below.
+	Command line associated with this module. See below for the
+	priority of this field vs. other mechanisms of specifying the
+	bootargs for the kernel.
 
 
 Command lines
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 03/10] xen: arm: prefer typesafe max()/min() over MAX()/MIN()
  2014-06-16 11:44 [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Ian Campbell
  2014-06-16 11:44 ` [PATCH 01/10] xen: arm: implement generic multiboot compatibility strings Ian Campbell
  2014-06-16 11:44 ` [PATCH 02/10] xen: arm: /chosen/module@N/bootargs bootprotcol node is not deprecated Ian Campbell
@ 2014-06-16 11:44 ` Ian Campbell
  2014-06-16 12:08   ` Julien Grall
  2014-06-16 11:44 ` [PATCH 04/10] xen: arm: rename early_info structs Ian Campbell
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 35+ messages in thread
From: Ian Campbell @ 2014-06-16 11:44 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Campbell, stefano.stabellini, Naresh Bhat, julien.grall, tim,
	Roy Franz, Fu Wei

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/p2m.c       |    4 ++--
 xen/common/device_tree.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 9960e17..dd2c5a4 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -500,8 +500,8 @@ static int apply_p2m_changes(struct domain *d,
         unsigned long sgfn = paddr_to_pfn(start_gpaddr);
         unsigned long egfn = paddr_to_pfn(end_gpaddr);
 
-        p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn);
-        p2m->lowest_mapped_gfn = MIN(p2m->lowest_mapped_gfn, sgfn);
+        p2m->max_mapped_gfn = max(p2m->max_mapped_gfn, egfn);
+        p2m->lowest_mapped_gfn = min(p2m->lowest_mapped_gfn, sgfn);
     }
 
     rc = 0;
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 48ae849..03d495a 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -425,7 +425,7 @@ static void __init process_chosen_node(const void *fdt, int node,
     mod->start = start;
     mod->size = end - start;
 
-    early_info.modules.nr_mods = MAX(MOD_INITRD, early_info.modules.nr_mods);
+    early_info.modules.nr_mods = max(MOD_INITRD, early_info.modules.nr_mods);
 }
 
 static int __init early_scan_node(const void *fdt,
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 04/10] xen: arm: rename early_info structs
  2014-06-16 11:44 [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Ian Campbell
                   ` (2 preceding siblings ...)
  2014-06-16 11:44 ` [PATCH 03/10] xen: arm: prefer typesafe max()/min() over MAX()/MIN() Ian Campbell
@ 2014-06-16 11:44 ` Ian Campbell
  2014-06-16 12:10   ` Julien Grall
  2014-06-18 13:56   ` Stefano Stabellini
  2014-06-16 11:44 ` [PATCH 05/10] xen: arm: move boot time fdt parsing into separate file Ian Campbell
                   ` (6 subsequent siblings)
  10 siblings, 2 replies; 35+ messages in thread
From: Ian Campbell @ 2014-06-16 11:44 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Campbell, stefano.stabellini, Naresh Bhat, julien.grall, tim,
	Roy Franz, Fu Wei

There isn't really anything Device Tree specific about the early_info, we just
happen to get it from device tree (but in the future it might come e.g. from
UEFI or ACPI or something else).

Move the relevant structs out of device_tree.h and into asm/setup.h and rename to
be more neutral.

For now the code to parse the DT into the now arch specific structs remains in
common code.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain_build.c   |   13 ++++++-----
 xen/arch/arm/kernel.c         |    6 ++---
 xen/arch/arm/kernel.h         |    3 ++-
 xen/arch/arm/setup.c          |   51 ++++++++++++++++++++++-------------------
 xen/common/device_tree.c      |   38 +++++++++++++++---------------
 xen/include/asm-arm/setup.h   |   40 ++++++++++++++++++++++++++++++++
 xen/include/xen/device_tree.h |   39 -------------------------------
 7 files changed, 98 insertions(+), 92 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9d9cba9..5eef8a3 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -16,6 +16,7 @@
 #include <asm/setup.h>
 #include <asm/platform.h>
 #include <asm/psci.h>
+#include <asm/setup.h>
 
 #include <asm/gic.h>
 #include <xen/irq.h>
@@ -160,9 +161,9 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
     int res = 0;
     int had_dom0_bootargs = 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];
+    if ( bootinfo.modules.nr_mods >= MOD_KERNEL &&
+         bootinfo.modules.module[MOD_KERNEL].cmdline[0] )
+        bootargs = &bootinfo.modules.module[MOD_KERNEL].cmdline[0];
 
     dt_for_each_property_node (node, prop)
     {
@@ -221,7 +222,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
          * If the bootloader provides an initrd, we must create a placeholder
          * for the initrd properties. The values will be replaced later.
          */
-        if ( early_info.modules.module[MOD_INITRD].size )
+        if ( bootinfo.modules.module[MOD_INITRD].size )
         {
             u64 a = 0;
             res = fdt_property(kinfo->fdt, "linux,initrd-start", &a, sizeof(a));
@@ -976,8 +977,8 @@ static void dtb_load(struct kernel_info *kinfo)
 static void initrd_load(struct kernel_info *kinfo)
 {
     paddr_t load_addr = kinfo->initrd_paddr;
-    paddr_t paddr = early_info.modules.module[MOD_INITRD].start;
-    paddr_t len = early_info.modules.module[MOD_INITRD].size;
+    paddr_t paddr = bootinfo.modules.module[MOD_INITRD].start;
+    paddr_t len = bootinfo.modules.module[MOD_INITRD].size;
     unsigned long offs;
     int node;
     int res;
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 69182ec..ce5b95a 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -69,7 +69,7 @@ static void place_modules(struct kernel_info *info,
 {
     /* Align DTB and initrd size to 2Mb. Linux only requires 4 byte alignment */
     const paddr_t initrd_len =
-        ROUNDUP(early_info.modules.module[MOD_INITRD].size, MB(2));
+        ROUNDUP(bootinfo.modules.module[MOD_INITRD].size, MB(2));
     const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt), MB(2));
     const paddr_t modsize = initrd_len + dtb_len;
 
@@ -376,8 +376,8 @@ int kernel_probe(struct kernel_info *info)
 
     paddr_t start, size;
 
-    start = early_info.modules.module[MOD_KERNEL].start;
-    size = early_info.modules.module[MOD_KERNEL].size;
+    start = bootinfo.modules.module[MOD_KERNEL].start;
+    size = bootinfo.modules.module[MOD_KERNEL].size;
 
     if ( !size )
     {
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index fd2f61d..7c7f624 100644
--- a/xen/arch/arm/kernel.h
+++ b/xen/arch/arm/kernel.h
@@ -8,6 +8,7 @@
 
 #include <xen/libelf.h>
 #include <xen/device_tree.h>
+#include <asm/setup.h>
 
 struct kernel_info {
 #ifdef CONFIG_ARM_64
@@ -16,7 +17,7 @@ struct kernel_info {
 
     void *fdt; /* flat device tree */
     paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
-    struct dt_mem_info mem;
+    struct meminfo mem;
 
     /* kernel entry point */
     paddr_t entry;
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index b9ce7a9..63f6b8e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -43,8 +43,11 @@
 #include <asm/cpufeature.h>
 #include <asm/platform.h>
 #include <asm/procinfo.h>
+#include <asm/setup.h>
 #include <xsm/xsm.h>
 
+struct bootinfo __initdata bootinfo;
+
 struct cpuinfo_arm __read_mostly boot_cpu_data;
 
 static __used void init_done(void)
@@ -182,7 +185,7 @@ static void dt_unreserved_regions(paddr_t s, paddr_t e,
 
 void __init discard_initial_modules(void)
 {
-    struct dt_module_info *mi = &early_info.modules;
+    struct bootmodules *mi = &bootinfo.modules;
     int i;
 
     for ( i = MOD_DISCARD_FIRST; i <= mi->nr_mods; i++ )
@@ -210,7 +213,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
                                        uint32_t size, paddr_t align,
                                        int first_mod)
 {
-    const struct dt_module_info *mi = &early_info.modules;
+    const struct bootmodules *mi = &bootinfo.modules;
     int i;
     int nr_rsvd;
 
@@ -275,7 +278,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
  */
 static paddr_t __init next_module(paddr_t s, paddr_t *end)
 {
-    struct dt_module_info *mi = &early_info.modules;
+    struct bootmodules *mi = &bootinfo.modules;
     paddr_t lowest = ~(paddr_t)0;
     int i;
 
@@ -308,7 +311,7 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
  */
 static paddr_t __init get_xen_paddr(void)
 {
-    struct dt_mem_info *mi = &early_info.mem;
+    struct meminfo *mi = &bootinfo.mem;
     paddr_t min_size;
     paddr_t paddr = 0, last_end;
     int i;
@@ -357,8 +360,8 @@ static paddr_t __init get_xen_paddr(void)
     printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
            paddr, paddr + min_size);
 
-    early_info.modules.module[MOD_XEN].start = paddr;
-    early_info.modules.module[MOD_XEN].size = min_size;
+    bootinfo.modules.module[MOD_XEN].start = paddr;
+    bootinfo.modules.module[MOD_XEN].size = min_size;
 
     return paddr;
 }
@@ -376,7 +379,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
     int i;
     void *fdt;
 
-    if ( !early_info.mem.nr_banks )
+    if ( !bootinfo.mem.nr_banks )
         panic("No memory bank");
 
     /*
@@ -393,15 +396,15 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
      * We also track the number of actual RAM pages (i.e. not counting
      * the holes).
      */
-    ram_size  = early_info.mem.bank[0].size;
+    ram_size  = bootinfo.mem.bank[0].size;
 
-    contig_start = ram_start = early_info.mem.bank[0].start;
+    contig_start = ram_start = bootinfo.mem.bank[0].start;
     contig_end   = ram_end = ram_start + ram_size;
 
-    for ( i = 1; i < early_info.mem.nr_banks; i++ )
+    for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
     {
-        paddr_t bank_start = early_info.mem.bank[i].start;
-        paddr_t bank_size = early_info.mem.bank[i].size;
+        paddr_t bank_start = bootinfo.mem.bank[i].start;
+        paddr_t bank_size = bootinfo.mem.bank[i].size;
         paddr_t bank_end = bank_start + bank_size;
 
         paddr_t new_ram_size = ram_size + bank_size;
@@ -434,11 +437,11 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
         ram_end = new_ram_end;
     }
 
-    if ( i != early_info.mem.nr_banks )
+    if ( i != bootinfo.mem.nr_banks )
     {
         printk("WARNING: only using %d out of %d memory banks\n",
-               i, early_info.mem.nr_banks);
-        early_info.mem.nr_banks = i;
+               i, bootinfo.mem.nr_banks);
+        bootinfo.mem.nr_banks = i;
     }
 
     total_pages = ram_pages = ram_size >> PAGE_SHIFT;
@@ -497,10 +500,10 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
     device_tree_flattened = fdt;
 
     /* Add non-xenheap memory */
-    for ( i = 0; i < early_info.mem.nr_banks; i++ )
+    for ( i = 0; i < bootinfo.mem.nr_banks; i++ )
     {
-        paddr_t bank_start = early_info.mem.bank[i].start;
-        paddr_t bank_end = bank_start + early_info.mem.bank[i].size;
+        paddr_t bank_start = bootinfo.mem.bank[i].start;
+        paddr_t bank_end = bank_start + bootinfo.mem.bank[i].size;
 
         s = bank_start;
         while ( s < bank_end )
@@ -557,10 +560,10 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
     void *fdt;
 
     total_pages = 0;
-    for ( bank = 0 ; bank < early_info.mem.nr_banks; bank++ )
+    for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
     {
-        paddr_t bank_start = early_info.mem.bank[bank].start;
-        paddr_t bank_size = early_info.mem.bank[bank].size;
+        paddr_t bank_start = bootinfo.mem.bank[bank].start;
+        paddr_t bank_size = bootinfo.mem.bank[bank].size;
         paddr_t bank_end = bank_start + bank_size;
         paddr_t s, e;
 
@@ -609,11 +612,11 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
         }
     }
 
-    if ( bank != early_info.mem.nr_banks )
+    if ( bank != bootinfo.mem.nr_banks )
     {
         printk("WARNING: only using %d out of %d memory banks\n",
-               bank, early_info.mem.nr_banks);
-        early_info.mem.nr_banks = bank;
+               bank, bootinfo.mem.nr_banks);
+        bootinfo.mem.nr_banks = bank;
     }
 
     total_pages += ram_size >> PAGE_SHIFT;
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 03d495a..a1896d3 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -23,8 +23,8 @@
 #include <xen/cpumask.h>
 #include <xen/ctype.h>
 #include <xen/lib.h>
+#include <asm/setup.h>
 
-struct dt_early_info __initdata early_info;
 const void *device_tree_flattened;
 dt_irq_xlate_func dt_irq_xlate;
 /* Host device tree */
@@ -238,10 +238,10 @@ const char *device_tree_bootargs(const void *fdt)
     prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
     if ( prop == NULL )
     {
-        struct dt_mb_module *dom0_mod = NULL;
+        struct bootmodule *dom0_mod = NULL;
 
-        if ( early_info.modules.nr_mods >= MOD_KERNEL )
-            dom0_mod = &early_info.modules.module[MOD_KERNEL];
+        if ( bootinfo.modules.nr_mods >= MOD_KERNEL )
+            dom0_mod = &bootinfo.modules.module[MOD_KERNEL];
 
         if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL) ||
             ( dom0_mod && dom0_mod->cmdline[0] ) )
@@ -319,12 +319,12 @@ static void __init process_memory_node(const void *fdt, int node,
     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++ )
+    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
     {
         device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
-        early_info.mem.bank[early_info.mem.nr_banks].start = start;
-        early_info.mem.bank[early_info.mem.nr_banks].size = size;
-        early_info.mem.nr_banks++;
+        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
+        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
+        bootinfo.mem.nr_banks++;
     }
 }
 
@@ -335,7 +335,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
     const struct fdt_property *prop;
     const __be32 *cell;
     int nr;
-    struct dt_mb_module *mod;
+    struct bootmodule *mod;
     int len;
 
     if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
@@ -349,7 +349,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
     else
         panic("%s not a known xen multiboot type\n", name);
 
-    mod = &early_info.modules.module[nr];
+    mod = &bootinfo.modules.module[nr];
 
     prop = fdt_get_property(fdt, node, "reg", &len);
     if ( !prop )
@@ -374,8 +374,8 @@ static void __init process_multiboot_node(const void *fdt, int node,
     else
         mod->cmdline[0] = 0;
 
-    if ( nr > early_info.modules.nr_mods )
-        early_info.modules.nr_mods = nr;
+    if ( nr > bootinfo.modules.nr_mods )
+        bootinfo.modules.nr_mods = nr;
 }
 
 static void __init process_chosen_node(const void *fdt, int node,
@@ -383,7 +383,7 @@ static void __init process_chosen_node(const void *fdt, int node,
                                        u32 address_cells, u32 size_cells)
 {
     const struct fdt_property *prop;
-    struct dt_mb_module *mod = &early_info.modules.module[MOD_INITRD];
+    struct bootmodule *mod = &bootinfo.modules.module[MOD_INITRD];
     paddr_t start, end;
     int len;
 
@@ -425,7 +425,7 @@ static void __init process_chosen_node(const void *fdt, int node,
     mod->start = start;
     mod->size = end - start;
 
-    early_info.modules.nr_mods = max(MOD_INITRD, early_info.modules.nr_mods);
+    bootinfo.modules.nr_mods = max(MOD_INITRD, bootinfo.modules.nr_mods);
 }
 
 static int __init early_scan_node(const void *fdt,
@@ -446,8 +446,8 @@ static int __init early_scan_node(const void *fdt,
 
 static void __init early_print_info(void)
 {
-    struct dt_mem_info *mi = &early_info.mem;
-    struct dt_module_info *mods = &early_info.modules;
+    struct meminfo *mi = &bootinfo.mem;
+    struct bootmodules *mods = &bootinfo.modules;
     int i, nr_rsvd;
 
     for ( i = 0; i < mi->nr_banks; i++ )
@@ -483,18 +483,18 @@ static void __init early_print_info(void)
  */
 size_t __init device_tree_early_init(const void *fdt, paddr_t paddr)
 {
-    struct dt_mb_module *mod;
+    struct bootmodule *mod;
     int ret;
 
     ret = fdt_check_header(fdt);
     if ( ret < 0 )
         panic("No valid device tree\n");
 
-    mod = &early_info.modules.module[MOD_FDT];
+    mod = &bootinfo.modules.module[MOD_FDT];
     mod->start = paddr;
     mod->size = fdt_totalsize(fdt);
 
-    early_info.modules.nr_mods = max(MOD_FDT, early_info.modules.nr_mods);
+    bootinfo.modules.nr_mods = max(MOD_FDT, bootinfo.modules.nr_mods);
 
     device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
     early_print_info();
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index b09f688..ea0dc46 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -3,6 +3,46 @@
 
 #include <public/version.h>
 
+#define NR_MEM_BANKS 8
+
+#define MOD_XEN    0
+#define MOD_FDT    1
+#define MOD_KERNEL 2
+#define MOD_INITRD 3
+#define MOD_XSM    4
+#define NR_MODULES 5
+
+#define MOD_DISCARD_FIRST MOD_FDT
+
+struct membank {
+    paddr_t start;
+    paddr_t size;
+};
+
+struct meminfo {
+    int nr_banks;
+    struct membank bank[NR_MEM_BANKS];
+};
+
+struct bootmodule {
+    paddr_t start;
+    paddr_t size;
+    char cmdline[1024];
+};
+
+struct bootmodules {
+    int nr_mods;
+    /* Module 0 is Xen itself, followed by the provided modules-proper */
+    struct bootmodule module[NR_MODULES];
+};
+
+struct bootinfo {
+    struct meminfo mem;
+    struct bootmodules modules;
+};
+
+extern struct bootinfo bootinfo;
+
 void arch_init_memory(void);
 
 void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 25db076..74e98f5 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -20,44 +20,6 @@
 
 #define DEVICE_TREE_MAX_DEPTH 16
 
-#define NR_MEM_BANKS 8
-
-#define MOD_XEN    0
-#define MOD_FDT    1
-#define MOD_KERNEL 2
-#define MOD_INITRD 3
-#define MOD_XSM    4
-#define NR_MODULES 5
-
-#define MOD_DISCARD_FIRST MOD_FDT
-
-struct membank {
-    paddr_t start;
-    paddr_t size;
-};
-
-struct dt_mem_info {
-    int nr_banks;
-    struct membank bank[NR_MEM_BANKS];
-};
-
-struct dt_mb_module {
-    paddr_t start;
-    paddr_t size;
-    char cmdline[1024];
-};
-
-struct dt_module_info {
-    int nr_mods;
-    /* Module 0 is Xen itself, followed by the provided modules-proper */
-    struct dt_mb_module module[NR_MODULES];
-};
-
-struct dt_early_info {
-    struct dt_mem_info mem;
-    struct dt_module_info modules;
-};
-
 /*
  * Struct used for matching a device
  */
@@ -193,7 +155,6 @@ typedef int (*device_tree_node_func)(const void *fdt,
                                      u32 address_cells, u32 size_cells,
                                      void *data);
 
-extern struct dt_early_info early_info;
 extern const void *device_tree_flattened;
 
 size_t __init device_tree_early_init(const void *fdt, paddr_t paddr);
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 05/10] xen: arm: move boot time fdt parsing into separate file.
  2014-06-16 11:44 [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Ian Campbell
                   ` (3 preceding siblings ...)
  2014-06-16 11:44 ` [PATCH 04/10] xen: arm: rename early_info structs Ian Campbell
@ 2014-06-16 11:44 ` Ian Campbell
  2014-06-16 12:13   ` Julien Grall
  2014-06-18 14:00   ` Stefano Stabellini
  2014-06-16 11:44 ` [PATCH 06/10] xen: arm: device_tree_bootargs to bootfdt.c, renaming to boot_fdt_cmdline Ian Campbell
                   ` (5 subsequent siblings)
  10 siblings, 2 replies; 35+ messages in thread
From: Ian Campbell @ 2014-06-16 11:44 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Campbell, stefano.stabellini, Naresh Bhat, julien.grall, tim,
	Roy Franz, Fu Wei

Move the early code for walking the flattended device tree out of
device_tree.c. The intention is that eventually only only the proper (i.e.
unflattened) device tree support will live in device_tree.c.

The new home is bootfdt.c to try and better reflect the purpose of the code.
Although in theory this early code could be generic in reality it is pretty ARM
specific, so place it under xen/arch/arm until a second user wants it.

As part of the move rename device_tree_early_init to boot_fdt_info. Drop
device_tree_dump, it is unused.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/Makefile         |    1 +
 xen/arch/arm/bootfdt.c        |  343 +++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/setup.c          |    2 +-
 xen/common/device_tree.c      |  356 -----------------------------------------
 xen/include/asm-arm/setup.h   |    2 +
 xen/include/xen/device_tree.h |    3 -
 6 files changed, 347 insertions(+), 360 deletions(-)
 create mode 100644 xen/arch/arm/bootfdt.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 63e0460..58a6714 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -21,6 +21,7 @@ obj-y += guestcopy.o
 obj-y += physdev.o
 obj-y += platform.o
 obj-y += setup.o
+obj-y += bootfdt.o
 obj-y += time.o
 obj-y += smpboot.o
 obj-y += smp.o
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
new file mode 100644
index 0000000..11182cd
--- /dev/null
+++ b/xen/arch/arm/bootfdt.c
@@ -0,0 +1,343 @@
+/*
+ * Early Device Tree
+ *
+ * Copyright (C) 2012-2014 Citrix Systems, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <xen/config.h>
+#include <xen/types.h>
+#include <xen/lib.h>
+#include <xen/kernel.h>
+#include <xen/init.h>
+#include <xen/device_tree.h>
+#include <xen/libfdt/libfdt.h>
+#include <asm/setup.h>
+
+static bool_t __init device_tree_node_matches(const void *fdt, int node,
+                                              const char *match)
+{
+    const char *name;
+    size_t match_len;
+
+    name = fdt_get_name(fdt, node, NULL);
+    match_len = strlen(match);
+
+    /* Match both "match" and "match@..." patterns but not
+       "match-foo". */
+    return strncmp(name, match, match_len) == 0
+        && (name[match_len] == '@' || name[match_len] == '\0');
+}
+
+static bool_t __init device_tree_node_compatible(const void *fdt, int node,
+                                                 const char *match)
+{
+    int len, l;
+    int mlen;
+    const void *prop;
+
+    mlen = strlen(match);
+
+    prop = fdt_getprop(fdt, node, "compatible", &len);
+    if ( prop == NULL )
+        return 0;
+
+    while ( len > 0 ) {
+        if ( !dt_compat_cmp(prop, match) )
+            return 1;
+        l = strlen(prop) + 1;
+        prop += l;
+        len -= l;
+    }
+
+    return 0;
+}
+
+static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
+                                       u32 size_cells, u64 *start, u64 *size)
+{
+    *start = dt_next_cell(address_cells, cell);
+    *size = dt_next_cell(size_cells, cell);
+}
+
+static u32 __init device_tree_get_u32(const void *fdt, int node,
+                                      const char *prop_name, u32 dflt)
+{
+    const struct fdt_property *prop;
+
+    prop = fdt_get_property(fdt, node, prop_name, NULL);
+    if ( !prop || prop->len < sizeof(u32) )
+        return dflt;
+
+    return fdt32_to_cpu(*(uint32_t*)prop->data);
+}
+
+/**
+ * device_tree_for_each_node - iterate over all device tree nodes
+ * @fdt: flat device tree.
+ * @func: function to call for each node.
+ * @data: data to pass to @func.
+ *
+ * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
+ *
+ * Returns 0 if all nodes were iterated over successfully.  If @func
+ * returns a value different from 0, that value is returned immediately.
+ */
+static int __init device_tree_for_each_node(const void *fdt,
+                                            device_tree_node_func func,
+                                            void *data)
+{
+    int node;
+    int depth;
+    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
+    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
+    int ret;
+
+    for ( node = 0, depth = 0;
+          node >=0 && depth >= 0;
+          node = fdt_next_node(fdt, node, &depth) )
+    {
+        const char *name = fdt_get_name(fdt, node, NULL);
+
+        if ( depth >= DEVICE_TREE_MAX_DEPTH )
+        {
+            printk("Warning: device tree node `%s' is nested too deep\n",
+                   name);
+            continue;
+        }
+
+        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);
+
+
+        ret = func(fdt, node, name, depth,
+                   address_cells[depth-1], size_cells[depth-1], data);
+        if ( ret != 0 )
+            return ret;
+    }
+    return 0;
+}
+
+static void __init process_memory_node(const void *fdt, int node,
+                                       const char *name,
+                                       u32 address_cells, u32 size_cells)
+{
+    const struct fdt_property *prop;
+    int i;
+    int banks;
+    const __be32 *cell;
+    paddr_t start, size;
+    u32 reg_cells = address_cells + size_cells;
+
+    if ( address_cells < 1 || size_cells < 1 )
+    {
+        printk("fdt: node `%s': invalid #address-cells or #size-cells",
+               name);
+        return;
+    }
+
+    prop = fdt_get_property(fdt, node, "reg", NULL);
+    if ( !prop )
+    {
+        printk("fdt: node `%s': missing `reg' property\n", name);
+        return;
+    }
+
+    cell = (const __be32 *)prop->data;
+    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
+
+    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
+    {
+        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
+        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
+        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
+        bootinfo.mem.nr_banks++;
+    }
+}
+
+static void __init process_multiboot_node(const void *fdt, int node,
+                                          const char *name,
+                                          u32 address_cells, u32 size_cells)
+{
+    const struct fdt_property *prop;
+    const __be32 *cell;
+    int nr;
+    struct bootmodule *mod;
+    int len;
+
+    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
+         fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
+        nr = MOD_KERNEL;
+    else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0 ||
+              fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
+        nr = MOD_INITRD;
+    else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
+        nr = MOD_XSM;
+    else
+        panic("%s not a known xen multiboot type\n", name);
+
+    mod = &bootinfo.modules.module[nr];
+
+    prop = fdt_get_property(fdt, node, "reg", &len);
+    if ( !prop )
+        panic("node %s missing `reg' property\n", name);
+
+    if ( len < dt_cells_to_size(address_cells + size_cells) )
+        panic("fdt: node `%s': `reg` property length is too short\n",
+                    name);
+
+    cell = (const __be32 *)prop->data;
+    device_tree_get_reg(&cell, address_cells, size_cells,
+                        &mod->start, &mod->size);
+
+    prop = fdt_get_property(fdt, node, "bootargs", &len);
+    if ( prop )
+    {
+        if ( len > sizeof(mod->cmdline) )
+            panic("module %d command line too long\n", nr);
+
+        safe_strcpy(mod->cmdline, prop->data);
+    }
+    else
+        mod->cmdline[0] = 0;
+
+    if ( nr > bootinfo.modules.nr_mods )
+        bootinfo.modules.nr_mods = nr;
+}
+
+static void __init process_chosen_node(const void *fdt, int node,
+                                       const char *name,
+                                       u32 address_cells, u32 size_cells)
+{
+    const struct fdt_property *prop;
+    struct bootmodule *mod = &bootinfo.modules.module[MOD_INITRD];
+    paddr_t start, end;
+    int len;
+
+    printk("Checking for initrd in /chosen\n");
+
+    prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
+    if ( !prop )
+        /* No initrd present. */
+        return;
+    if ( len != sizeof(u32) && len != sizeof(u64) )
+    {
+        printk("linux,initrd-start property has invalid length %d\n", len);
+        return;
+    }
+    start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
+
+    prop = fdt_get_property(fdt, node, "linux,initrd-end", &len);
+    if ( !prop )
+    {
+        printk("linux,initrd-end not present but -start was\n");
+        return;
+    }
+    if ( len != sizeof(u32) && len != sizeof(u64) )
+    {
+        printk("linux,initrd-end property has invalid length %d\n", len);
+        return;
+    }
+    end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
+
+    if ( start >= end )
+    {
+        printk("linux,initrd limits invalid: %"PRIpaddr" >= %"PRIpaddr"\n",
+                  start, end);
+        return;
+    }
+
+    printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
+
+    mod->start = start;
+    mod->size = end - start;
+
+    bootinfo.modules.nr_mods = max(MOD_INITRD, bootinfo.modules.nr_mods);
+}
+
+static int __init early_scan_node(const void *fdt,
+                                  int node, const char *name, int depth,
+                                  u32 address_cells, u32 size_cells,
+                                  void *data)
+{
+    if ( device_tree_node_matches(fdt, node, "memory") )
+        process_memory_node(fdt, node, name, address_cells, size_cells);
+    else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
+              device_tree_node_compatible(fdt, node, "multiboot,module" ))
+        process_multiboot_node(fdt, node, name, address_cells, size_cells);
+    else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
+        process_chosen_node(fdt, node, name, address_cells, size_cells);
+
+    return 0;
+}
+
+static void __init early_print_info(void)
+{
+    struct meminfo *mi = &bootinfo.mem;
+    struct bootmodules *mods = &bootinfo.modules;
+    int i, nr_rsvd;
+
+    for ( i = 0; i < mi->nr_banks; i++ )
+        printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
+                     mi->bank[i].start,
+                     mi->bank[i].start + mi->bank[i].size - 1);
+    printk("\n");
+    for ( i = 1 ; i < mods->nr_mods + 1; i++ )
+        printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %s\n",
+                     i,
+                     mods->module[i].start,
+                     mods->module[i].start + mods->module[i].size,
+                     mods->module[i].cmdline);
+    nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
+    for ( i = 0; i < nr_rsvd; i++ )
+    {
+        paddr_t s, e;
+        if ( fdt_get_mem_rsv(device_tree_flattened, i, &s, &e) < 0 )
+            continue;
+        /* fdt_get_mem_rsv returns length */
+        e += s;
+        printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
+                     i, s, e);
+    }
+    printk("\n");
+}
+
+/**
+ * boot_fdt_info - initialize bootinfo from a DTB
+ * @fdt: flattened device tree binary
+ *
+ * Returns the size of the DTB.
+ */
+size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
+{
+    struct bootmodule *mod;
+    int ret;
+
+    ret = fdt_check_header(fdt);
+    if ( ret < 0 )
+        panic("No valid device tree\n");
+
+    mod = &bootinfo.modules.module[MOD_FDT];
+    mod->start = paddr;
+    mod->size = fdt_totalsize(fdt);
+
+    bootinfo.modules.nr_mods = max(MOD_FDT, bootinfo.modules.nr_mods);
+
+    device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
+    early_print_info();
+
+    return fdt_totalsize(fdt);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 63f6b8e..4a84a32 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -678,7 +678,7 @@ void __init start_xen(unsigned long boot_phys_offset,
     /* This is mapped by head.S */
     device_tree_flattened = (void *)BOOT_FDT_VIRT_START
         + (fdt_paddr & ((1 << SECOND_SHIFT) - 1));
-    fdt_size = device_tree_early_init(device_tree_flattened, fdt_paddr);
+    fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
 
     cmdline = device_tree_bootargs(device_tree_flattened);
     printk("Command line: %s\n", cmdline);
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index a1896d3..89de269 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -22,7 +22,6 @@
 #include <xen/string.h>
 #include <xen/cpumask.h>
 #include <xen/ctype.h>
-#include <xen/lib.h>
 #include <asm/setup.h>
 
 const void *device_tree_flattened;
@@ -89,52 +88,6 @@ struct dt_bus
     unsigned int (*get_flags)(const __be32 *addr);
 };
 
-static bool_t __init device_tree_node_matches(const void *fdt, int node,
-                                              const char *match)
-{
-    const char *name;
-    size_t match_len;
-
-    name = fdt_get_name(fdt, node, NULL);
-    match_len = strlen(match);
-
-    /* Match both "match" and "match@..." patterns but not
-       "match-foo". */
-    return strncmp(name, match, match_len) == 0
-        && (name[match_len] == '@' || name[match_len] == '\0');
-}
-
-static bool_t __init device_tree_node_compatible(const void *fdt, int node,
-                                                 const char *match)
-{
-    int len, l;
-    int mlen;
-    const void *prop;
-
-    mlen = strlen(match);
-
-    prop = fdt_getprop(fdt, node, "compatible", &len);
-    if ( prop == NULL )
-        return 0;
-
-    while ( len > 0 ) {
-        if ( !dt_compat_cmp(prop, match) )
-            return 1;
-        l = strlen(prop) + 1;
-        prop += l;
-        len -= l;
-    }
-
-    return 0;
-}
-
-static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
-                                       u32 size_cells, u64 *start, u64 *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,
                   u64 *address, u64 *size)
 {
@@ -162,66 +115,6 @@ void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
     dt_set_cell(cellp, dt_n_size_cells(np), size);
 }
 
-static u32 __init device_tree_get_u32(const void *fdt, int node,
-                                      const char *prop_name, u32 dflt)
-{
-    const struct fdt_property *prop;
-
-    prop = fdt_get_property(fdt, node, prop_name, NULL);
-    if ( !prop || prop->len < sizeof(u32) )
-        return dflt;
-
-    return fdt32_to_cpu(*(uint32_t*)prop->data);
-}
-
-/**
- * device_tree_for_each_node - iterate over all device tree nodes
- * @fdt: flat device tree.
- * @func: function to call for each node.
- * @data: data to pass to @func.
- *
- * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
- *
- * Returns 0 if all nodes were iterated over successfully.  If @func
- * returns a value different from 0, that value is returned immediately.
- */
-static int __init device_tree_for_each_node(const void *fdt,
-                                            device_tree_node_func func,
-                                            void *data)
-{
-    int node;
-    int depth;
-    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
-    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
-    int ret;
-
-    for ( node = 0, depth = 0;
-          node >=0 && depth >= 0;
-          node = fdt_next_node(fdt, node, &depth) )
-    {
-        const char *name = fdt_get_name(fdt, node, NULL);
-
-        if ( depth >= DEVICE_TREE_MAX_DEPTH )
-        {
-            printk("Warning: device tree node `%s' is nested too deep\n",
-                   name);
-            continue;
-        }
-
-        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);
-
-
-        ret = func(fdt, node, name, depth,
-                   address_cells[depth-1], size_cells[depth-1], data);
-        if ( ret != 0 )
-            return ret;
-    }
-    return 0;
-}
-
 /**
  * device_tree_bootargs - return the bootargs (the Xen command line)
  * @fdt flat device tree.
@@ -253,255 +146,6 @@ const char *device_tree_bootargs(const void *fdt)
     return prop->data;
 }
 
-static int dump_node(const void *fdt, int node, const char *name, int depth,
-                     u32 address_cells, u32 size_cells, void *data)
-{
-    char prefix[2*DEVICE_TREE_MAX_DEPTH + 1] = "";
-    int i;
-    int prop;
-
-    for ( i = 0; i < depth; i++ )
-        safe_strcat(prefix, "  ");
-
-    if ( name[0] == '\0' )
-        name = "/";
-    printk("%s%s:\n", prefix, name);
-
-    for ( prop = fdt_first_property_offset(fdt, node);
-          prop >= 0;
-          prop = fdt_next_property_offset(fdt, prop) )
-    {
-        const struct fdt_property *p;
-
-        p = fdt_get_property_by_offset(fdt, prop, NULL);
-
-        printk("%s  %s\n", prefix, fdt_string(fdt, fdt32_to_cpu(p->nameoff)));
-    }
-
-    return 0;
-}
-
-/**
- * device_tree_dump - print a text representation of a device tree
- * @fdt: flat device tree to print
- */
-void __init device_tree_dump(const void *fdt)
-{
-    device_tree_for_each_node(fdt, dump_node, NULL);
-}
-
-
-static void __init process_memory_node(const void *fdt, int node,
-                                       const char *name,
-                                       u32 address_cells, u32 size_cells)
-{
-    const struct fdt_property *prop;
-    int i;
-    int banks;
-    const __be32 *cell;
-    paddr_t start, size;
-    u32 reg_cells = address_cells + size_cells;
-
-    if ( address_cells < 1 || size_cells < 1 )
-    {
-        printk("fdt: node `%s': invalid #address-cells or #size-cells",
-               name);
-        return;
-    }
-
-    prop = fdt_get_property(fdt, node, "reg", NULL);
-    if ( !prop )
-    {
-        printk("fdt: node `%s': missing `reg' property\n", name);
-        return;
-    }
-
-    cell = (const __be32 *)prop->data;
-    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
-
-    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
-    {
-        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
-        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
-        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
-        bootinfo.mem.nr_banks++;
-    }
-}
-
-static void __init process_multiboot_node(const void *fdt, int node,
-                                          const char *name,
-                                          u32 address_cells, u32 size_cells)
-{
-    const struct fdt_property *prop;
-    const __be32 *cell;
-    int nr;
-    struct bootmodule *mod;
-    int len;
-
-    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
-         fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
-        nr = MOD_KERNEL;
-    else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0 ||
-              fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
-        nr = MOD_INITRD;
-    else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
-        nr = MOD_XSM;
-    else
-        panic("%s not a known xen multiboot type\n", name);
-
-    mod = &bootinfo.modules.module[nr];
-
-    prop = fdt_get_property(fdt, node, "reg", &len);
-    if ( !prop )
-        panic("node %s missing `reg' property\n", name);
-
-    if ( len < dt_cells_to_size(address_cells + size_cells) )
-        panic("fdt: node `%s': `reg` property length is too short\n",
-                    name);
-
-    cell = (const __be32 *)prop->data;
-    device_tree_get_reg(&cell, address_cells, size_cells,
-                        &mod->start, &mod->size);
-
-    prop = fdt_get_property(fdt, node, "bootargs", &len);
-    if ( prop )
-    {
-        if ( len > sizeof(mod->cmdline) )
-            panic("module %d command line too long\n", nr);
-
-        safe_strcpy(mod->cmdline, prop->data);
-    }
-    else
-        mod->cmdline[0] = 0;
-
-    if ( nr > bootinfo.modules.nr_mods )
-        bootinfo.modules.nr_mods = nr;
-}
-
-static void __init process_chosen_node(const void *fdt, int node,
-                                       const char *name,
-                                       u32 address_cells, u32 size_cells)
-{
-    const struct fdt_property *prop;
-    struct bootmodule *mod = &bootinfo.modules.module[MOD_INITRD];
-    paddr_t start, end;
-    int len;
-
-    printk("Checking for initrd in /chosen\n");
-
-    prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
-    if ( !prop )
-        /* No initrd present. */
-        return;
-    if ( len != sizeof(u32) && len != sizeof(u64) )
-    {
-        printk("linux,initrd-start property has invalid length %d\n", len);
-        return;
-    }
-    start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
-
-    prop = fdt_get_property(fdt, node, "linux,initrd-end", &len);
-    if ( !prop )
-    {
-        printk("linux,initrd-end not present but -start was\n");
-        return;
-    }
-    if ( len != sizeof(u32) && len != sizeof(u64) )
-    {
-        printk("linux,initrd-end property has invalid length %d\n", len);
-        return;
-    }
-    end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
-
-    if ( start >= end )
-    {
-        printk("linux,initrd limits invalid: %"PRIpaddr" >= %"PRIpaddr"\n",
-                  start, end);
-        return;
-    }
-
-    printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
-
-    mod->start = start;
-    mod->size = end - start;
-
-    bootinfo.modules.nr_mods = max(MOD_INITRD, bootinfo.modules.nr_mods);
-}
-
-static int __init early_scan_node(const void *fdt,
-                                  int node, const char *name, int depth,
-                                  u32 address_cells, u32 size_cells,
-                                  void *data)
-{
-    if ( device_tree_node_matches(fdt, node, "memory") )
-        process_memory_node(fdt, node, name, address_cells, size_cells);
-    else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
-              device_tree_node_compatible(fdt, node, "multiboot,module" ))
-        process_multiboot_node(fdt, node, name, address_cells, size_cells);
-    else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
-        process_chosen_node(fdt, node, name, address_cells, size_cells);
-
-    return 0;
-}
-
-static void __init early_print_info(void)
-{
-    struct meminfo *mi = &bootinfo.mem;
-    struct bootmodules *mods = &bootinfo.modules;
-    int i, nr_rsvd;
-
-    for ( i = 0; i < mi->nr_banks; i++ )
-        printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
-                     mi->bank[i].start,
-                     mi->bank[i].start + mi->bank[i].size - 1);
-    printk("\n");
-    for ( i = 1 ; i < mods->nr_mods + 1; i++ )
-        printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %s\n",
-                     i,
-                     mods->module[i].start,
-                     mods->module[i].start + mods->module[i].size,
-                     mods->module[i].cmdline);
-    nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
-    for ( i = 0; i < nr_rsvd; i++ )
-    {
-        paddr_t s, e;
-        if ( fdt_get_mem_rsv(device_tree_flattened, i, &s, &e) < 0 )
-            continue;
-        /* fdt_get_mem_rsv returns length */
-        e += s;
-        printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
-                     i, s, e);
-    }
-    printk("\n");
-}
-
-/**
- * device_tree_early_init - initialize early info from a DTB
- * @fdt: flattened device tree binary
- *
- * Returns the size of the DTB.
- */
-size_t __init device_tree_early_init(const void *fdt, paddr_t paddr)
-{
-    struct bootmodule *mod;
-    int ret;
-
-    ret = fdt_check_header(fdt);
-    if ( ret < 0 )
-        panic("No valid device tree\n");
-
-    mod = &bootinfo.modules.module[MOD_FDT];
-    mod->start = paddr;
-    mod->size = fdt_totalsize(fdt);
-
-    bootinfo.modules.nr_mods = max(MOD_FDT, bootinfo.modules.nr_mods);
-
-    device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
-    early_print_info();
-
-    return fdt_totalsize(fdt);
-}
-
 static void __init *unflatten_dt_alloc(unsigned long *mem, unsigned long size,
                                        unsigned long align)
 {
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index ea0dc46..21dbcd4 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -53,6 +53,8 @@ int construct_dom0(struct domain *d);
 
 void discard_initial_modules(void);
 
+size_t __init boot_fdt_info(const void *fdt, paddr_t paddr);
+
 #endif
 /*
  * Local variables:
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 74e98f5..0edec85 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -157,10 +157,7 @@ typedef int (*device_tree_node_func)(const void *fdt,
 
 extern const void *device_tree_flattened;
 
-size_t __init device_tree_early_init(const void *fdt, paddr_t paddr);
-
 const char __init *device_tree_bootargs(const void *fdt);
-void __init device_tree_dump(const void *fdt);
 
 /**
  * dt_unflatten_host_device_tree - Unflatten the host device tree
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 06/10] xen: arm: device_tree_bootargs to bootfdt.c, renaming to boot_fdt_cmdline
  2014-06-16 11:44 [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Ian Campbell
                   ` (4 preceding siblings ...)
  2014-06-16 11:44 ` [PATCH 05/10] xen: arm: move boot time fdt parsing into separate file Ian Campbell
@ 2014-06-16 11:44 ` Ian Campbell
  2014-06-16 12:32   ` Julien Grall
  2014-06-18 14:02   ` Stefano Stabellini
  2014-06-16 11:45 ` [PATCH 07/10] xen: arm: store per-boot module type instead of relying on index Ian Campbell
                   ` (4 subsequent siblings)
  10 siblings, 2 replies; 35+ messages in thread
From: Ian Campbell @ 2014-06-16 11:44 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Campbell, stefano.stabellini, Naresh Bhat, julien.grall, tim,
	Roy Franz, Fu Wei

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/bootfdt.c        |   27 +++++++++++++++++++++++++++
 xen/arch/arm/setup.c          |    2 +-
 xen/common/device_tree.c      |   31 -------------------------------
 xen/include/asm-arm/setup.h   |    1 +
 xen/include/xen/device_tree.h |    2 --
 5 files changed, 29 insertions(+), 34 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 11182cd..e48a64b 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -333,6 +333,33 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
     return fdt_totalsize(fdt);
 }
 
+const char *boot_fdt_cmdline(const void *fdt)
+{
+    int node;
+    const struct fdt_property *prop;
+
+    node = fdt_path_offset(fdt, "/chosen");
+    if ( node < 0 )
+        return NULL;
+
+    prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
+    if ( prop == NULL )
+    {
+        struct bootmodule *dom0_mod = NULL;
+
+        if ( bootinfo.modules.nr_mods >= MOD_KERNEL )
+            dom0_mod = &bootinfo.modules.module[MOD_KERNEL];
+
+        if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL) ||
+            ( dom0_mod && dom0_mod->cmdline[0] ) )
+            prop = fdt_get_property(fdt, node, "bootargs", NULL);
+    }
+    if ( prop == NULL )
+        return NULL;
+
+    return prop->data;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 4a84a32..f1ae408 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -680,7 +680,7 @@ void __init start_xen(unsigned long boot_phys_offset,
         + (fdt_paddr & ((1 << SECOND_SHIFT) - 1));
     fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
 
-    cmdline = device_tree_bootargs(device_tree_flattened);
+    cmdline = boot_fdt_cmdline(device_tree_flattened);
     printk("Command line: %s\n", cmdline);
     cmdline_parse(cmdline);
 
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 89de269..b9deb62 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -115,37 +115,6 @@ void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
     dt_set_cell(cellp, dt_n_size_cells(np), size);
 }
 
-/**
- * device_tree_bootargs - return the bootargs (the Xen command line)
- * @fdt flat device tree.
- */
-const char *device_tree_bootargs(const void *fdt)
-{
-    int node;
-    const struct fdt_property *prop;
-
-    node = fdt_path_offset(fdt, "/chosen");
-    if ( node < 0 )
-        return NULL;
-
-    prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
-    if ( prop == NULL )
-    {
-        struct bootmodule *dom0_mod = NULL;
-
-        if ( bootinfo.modules.nr_mods >= MOD_KERNEL )
-            dom0_mod = &bootinfo.modules.module[MOD_KERNEL];
-
-        if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL) ||
-            ( dom0_mod && dom0_mod->cmdline[0] ) )
-            prop = fdt_get_property(fdt, node, "bootargs", NULL);
-    }
-    if ( prop == NULL )
-        return NULL;
-
-    return prop->data;
-}
-
 static void __init *unflatten_dt_alloc(unsigned long *mem, unsigned long size,
                                        unsigned long align)
 {
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 21dbcd4..85aa866 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -54,6 +54,7 @@ int construct_dom0(struct domain *d);
 void discard_initial_modules(void);
 
 size_t __init boot_fdt_info(const void *fdt, paddr_t paddr);
+const char __init *boot_fdt_cmdline(const void *fdt);
 
 #endif
 /*
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 0edec85..b486fc6 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -157,8 +157,6 @@ typedef int (*device_tree_node_func)(const void *fdt,
 
 extern const void *device_tree_flattened;
 
-const char __init *device_tree_bootargs(const void *fdt);
-
 /**
  * dt_unflatten_host_device_tree - Unflatten the host device tree
  *
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 07/10] xen: arm: store per-boot module type instead of relying on index
  2014-06-16 11:44 [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Ian Campbell
                   ` (5 preceding siblings ...)
  2014-06-16 11:44 ` [PATCH 06/10] xen: arm: device_tree_bootargs to bootfdt.c, renaming to boot_fdt_cmdline Ian Campbell
@ 2014-06-16 11:45 ` Ian Campbell
  2014-06-16 12:48   ` Julien Grall
  2014-06-18 14:18   ` Stefano Stabellini
  2014-06-16 11:45 ` [PATCH 08/10] xen: arm: support bootmodule type detection by ordering Ian Campbell
                   ` (3 subsequent siblings)
  10 siblings, 2 replies; 35+ messages in thread
From: Ian Campbell @ 2014-06-16 11:45 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Campbell, stefano.stabellini, Naresh Bhat, julien.grall, tim,
	Roy Franz, Fu Wei

This is more natural and better matches how multiboot is actually supposed to
work.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/bootfdt.c      |   49 +++++++++++++++----------------------------
 xen/arch/arm/domain_build.c |   20 +++++++++++-------
 xen/arch/arm/kernel.c       |   15 ++++++-------
 xen/arch/arm/setup.c        |   47 ++++++++++++++++++++++++++++++++++++-----
 xen/include/asm-arm/setup.h |   29 +++++++++++++++++--------
 5 files changed, 100 insertions(+), 60 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index e48a64b..e983aa7 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -165,23 +165,22 @@ static void __init process_multiboot_node(const void *fdt, int node,
 {
     const struct fdt_property *prop;
     const __be32 *cell;
-    int nr;
-    struct bootmodule *mod;
+    bootmodulekind kind;
+    paddr_t start, size;
+    const char *cmdline;
     int len;
 
     if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
          fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
-        nr = MOD_KERNEL;
+        kind = BOOTMOD_KERNEL;
     else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0 ||
               fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
-        nr = MOD_INITRD;
+        kind = BOOTMOD_RAMDISK;
     else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
-        nr = MOD_XSM;
+        kind = BOOTMOD_XSM;
     else
         panic("%s not a known xen multiboot type\n", name);
 
-    mod = &bootinfo.modules.module[nr];
-
     prop = fdt_get_property(fdt, node, "reg", &len);
     if ( !prop )
         panic("node %s missing `reg' property\n", name);
@@ -191,22 +190,19 @@ static void __init process_multiboot_node(const void *fdt, int node,
                     name);
 
     cell = (const __be32 *)prop->data;
-    device_tree_get_reg(&cell, address_cells, size_cells,
-                        &mod->start, &mod->size);
+    device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
 
     prop = fdt_get_property(fdt, node, "bootargs", &len);
     if ( prop )
     {
-        if ( len > sizeof(mod->cmdline) )
-            panic("module %d command line too long\n", nr);
-
-        safe_strcpy(mod->cmdline, prop->data);
+        if ( len > BOOTMOD_MAX_CMDLINE )
+            panic("module %s command line too long\n", name);
+        cmdline = prop->data;
     }
     else
-        mod->cmdline[0] = 0;
+        cmdline = NULL;
 
-    if ( nr > bootinfo.modules.nr_mods )
-        bootinfo.modules.nr_mods = nr;
+    add_boot_module(kind, start, size, cmdline);
 }
 
 static void __init process_chosen_node(const void *fdt, int node,
@@ -214,7 +210,6 @@ static void __init process_chosen_node(const void *fdt, int node,
                                        u32 address_cells, u32 size_cells)
 {
     const struct fdt_property *prop;
-    struct bootmodule *mod = &bootinfo.modules.module[MOD_INITRD];
     paddr_t start, end;
     int len;
 
@@ -253,10 +248,7 @@ static void __init process_chosen_node(const void *fdt, int node,
 
     printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
 
-    mod->start = start;
-    mod->size = end - start;
-
-    bootinfo.modules.nr_mods = max(MOD_INITRD, bootinfo.modules.nr_mods);
+    add_boot_module(BOOTMOD_RAMDISK, start, end-start, NULL);
 }
 
 static int __init early_scan_node(const void *fdt,
@@ -286,7 +278,7 @@ static void __init early_print_info(void)
                      mi->bank[i].start,
                      mi->bank[i].start + mi->bank[i].size - 1);
     printk("\n");
-    for ( i = 1 ; i < mods->nr_mods + 1; i++ )
+    for ( i = 1 ; i < mods->nr_mods; i++ )
         printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %s\n",
                      i,
                      mods->module[i].start,
@@ -314,18 +306,13 @@ static void __init early_print_info(void)
  */
 size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
 {
-    struct bootmodule *mod;
     int ret;
 
     ret = fdt_check_header(fdt);
     if ( ret < 0 )
         panic("No valid device tree\n");
 
-    mod = &bootinfo.modules.module[MOD_FDT];
-    mod->start = paddr;
-    mod->size = fdt_totalsize(fdt);
-
-    bootinfo.modules.nr_mods = max(MOD_FDT, bootinfo.modules.nr_mods);
+    add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), NULL);
 
     device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
     early_print_info();
@@ -345,10 +332,8 @@ const char *boot_fdt_cmdline(const void *fdt)
     prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
     if ( prop == NULL )
     {
-        struct bootmodule *dom0_mod = NULL;
-
-        if ( bootinfo.modules.nr_mods >= MOD_KERNEL )
-            dom0_mod = &bootinfo.modules.module[MOD_KERNEL];
+        struct bootmodule *dom0_mod =
+            boot_module_find_by_kind(BOOTMOD_KERNEL);
 
         if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL) ||
             ( dom0_mod && dom0_mod->cmdline[0] ) )
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 5eef8a3..c1a54e5 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -161,9 +161,10 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
     int res = 0;
     int had_dom0_bootargs = 0;
 
-    if ( bootinfo.modules.nr_mods >= MOD_KERNEL &&
-         bootinfo.modules.module[MOD_KERNEL].cmdline[0] )
-        bootargs = &bootinfo.modules.module[MOD_KERNEL].cmdline[0];
+    struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_KERNEL);
+
+    if ( mod && mod->cmdline[0] )
+        bootargs = &mod->cmdline[0];
 
     dt_for_each_property_node (node, prop)
     {
@@ -210,6 +211,8 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
 
     if ( dt_node_path_is_equal(node, "/chosen") )
     {
+        struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_RAMDISK);
+
         if ( bootargs )
         {
             res = fdt_property(kinfo->fdt, "bootargs", bootargs,
@@ -222,7 +225,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
          * If the bootloader provides an initrd, we must create a placeholder
          * for the initrd properties. The values will be replaced later.
          */
-        if ( bootinfo.modules.module[MOD_INITRD].size )
+        if ( mod && mod->size )
         {
             u64 a = 0;
             res = fdt_property(kinfo->fdt, "linux,initrd-start", &a, sizeof(a));
@@ -976,18 +979,21 @@ static void dtb_load(struct kernel_info *kinfo)
 
 static void initrd_load(struct kernel_info *kinfo)
 {
+    struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_RAMDISK);
     paddr_t load_addr = kinfo->initrd_paddr;
-    paddr_t paddr = bootinfo.modules.module[MOD_INITRD].start;
-    paddr_t len = bootinfo.modules.module[MOD_INITRD].size;
+    paddr_t paddr, len;
     unsigned long offs;
     int node;
     int res;
     __be32 val[2];
     __be32 *cellp;
 
-    if ( !len )
+    if ( !mod || !mod->size )
         return;
 
+    paddr = mod->start;
+    len = mod->size;
+
     printk("Loading dom0 initrd from %"PRIpaddr" to 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
            paddr, load_addr, load_addr + len);
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index ce5b95a..230ff8f 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -68,8 +68,8 @@ static void place_modules(struct kernel_info *info,
                           paddr_t kernbase, paddr_t kernend)
 {
     /* Align DTB and initrd size to 2Mb. Linux only requires 4 byte alignment */
-    const paddr_t initrd_len =
-        ROUNDUP(bootinfo.modules.module[MOD_INITRD].size, MB(2));
+    const struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_RAMDISK);
+    const paddr_t initrd_len = ROUNDUP(mod ? mod->size : 0, MB(2));
     const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt), MB(2));
     const paddr_t modsize = initrd_len + dtb_len;
 
@@ -372,20 +372,21 @@ err:
 
 int kernel_probe(struct kernel_info *info)
 {
+    struct bootmodule *mod = boot_module_find_by_kind(BOOTMOD_KERNEL);
     int rc;
 
     paddr_t start, size;
 
-    start = bootinfo.modules.module[MOD_KERNEL].start;
-    size = bootinfo.modules.module[MOD_KERNEL].size;
-
-    if ( !size )
+    if ( !mod || !mod->size )
     {
         printk(XENLOG_ERR "Missing kernel boot module?\n");
         return -ENOENT;
     }
 
-    printk("Loading kernel from boot module %d\n", MOD_KERNEL);
+    start = mod->start;
+    size = mod->size;
+
+    printk("Loading kernel from boot module @ %"PRIpaddr"\n", start);
 
 #ifdef CONFIG_ARM_64
     rc = kernel_zimage64_probe(info, start, size);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index f1ae408..4c9dd3d 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -183,17 +183,56 @@ static void dt_unreserved_regions(paddr_t s, paddr_t e,
     cb(s, e);
 }
 
+void add_boot_module(bootmodulekind kind, paddr_t start, paddr_t size,
+                     const char *cmdline)
+{
+    struct bootmodules *mods = &bootinfo.modules;
+    struct bootmodule *mod;
+
+    if ( mods->nr_mods == MAX_MODULES )
+    {
+        printk("Ignoring boot module at %"PRIpaddr"-%"PRIpaddr" (too many)\n",
+               start, start + size);
+        return;
+    }
+
+    mod = &mods->module[mods->nr_mods++];
+    mod->kind = kind;
+    mod->start = start;
+    mod->size = size;
+    if ( cmdline )
+        safe_strcpy(mod->cmdline, cmdline);
+    else
+        mod->cmdline[0] = 0;
+
+}
+
+struct bootmodule * __init boot_module_find_by_kind(bootmodulekind kind)
+{
+    struct bootmodules *mods = &bootinfo.modules;
+    struct bootmodule *mod;
+    int i;
+    for (i = 0 ; i < mods->nr_mods ; i++ )
+    {
+        mod = &mods->module[i];
+        if ( mod->kind == kind )
+            return mod;
+    }
+    return NULL;
+}
+
 void __init discard_initial_modules(void)
 {
     struct bootmodules *mi = &bootinfo.modules;
     int i;
 
-    for ( i = MOD_DISCARD_FIRST; i <= mi->nr_mods; i++ )
+    for ( i = 0; i <= mi->nr_mods; i++ )
     {
         paddr_t s = mi->module[i].start;
         paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
 
-        dt_unreserved_regions(s, e, init_domheap_pages, 0);
+        if ( mi->module[i].kind > BOOTMOD_LAST_PRESERVE )
+            dt_unreserved_regions(s, e, init_domheap_pages, 0);
     }
 
     mi->nr_mods = 0;
@@ -360,9 +399,7 @@ static paddr_t __init get_xen_paddr(void)
     printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
            paddr, paddr + min_size);
 
-    bootinfo.modules.module[MOD_XEN].start = paddr;
-    bootinfo.modules.module[MOD_XEN].size = min_size;
-
+    add_boot_module(BOOTMOD_XEN, paddr, min_size, NULL);
     return paddr;
 }
 
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 85aa866..57c98cb 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -5,14 +5,19 @@
 
 #define NR_MEM_BANKS 8
 
-#define MOD_XEN    0
-#define MOD_FDT    1
-#define MOD_KERNEL 2
-#define MOD_INITRD 3
-#define MOD_XSM    4
-#define NR_MODULES 5
+#define MAX_MODULES 5 /* Current maximum useful modules */
+
+typedef enum {
+    BOOTMOD_XEN,
+    BOOTMOD_FDT,
+    /* Everything up to here is not freed after start of day */
+    BOOTMOD_LAST_PRESERVE = BOOTMOD_FDT,
+    BOOTMOD_KERNEL,
+    BOOTMOD_RAMDISK,
+    BOOTMOD_XSM,
+    BOOTMOD_UNKNOWN
+}  bootmodulekind;
 
-#define MOD_DISCARD_FIRST MOD_FDT
 
 struct membank {
     paddr_t start;
@@ -24,16 +29,18 @@ struct meminfo {
     struct membank bank[NR_MEM_BANKS];
 };
 
+#define BOOTMOD_MAX_CMDLINE 1024
 struct bootmodule {
+    bootmodulekind kind;
     paddr_t start;
     paddr_t size;
-    char cmdline[1024];
+    char cmdline[BOOTMOD_MAX_CMDLINE];
 };
 
 struct bootmodules {
     int nr_mods;
     /* Module 0 is Xen itself, followed by the provided modules-proper */
-    struct bootmodule module[NR_MODULES];
+    struct bootmodule module[MAX_MODULES];
 };
 
 struct bootinfo {
@@ -56,6 +63,10 @@ void discard_initial_modules(void);
 size_t __init boot_fdt_info(const void *fdt, paddr_t paddr);
 const char __init *boot_fdt_cmdline(const void *fdt);
 
+void add_boot_module(bootmodulekind kind, paddr_t start, paddr_t size,
+                     const char *cmdline);
+struct bootmodule *boot_module_find_by_kind(bootmodulekind kind);
+
 #endif
 /*
  * Local variables:
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 08/10] xen: arm: support bootmodule type detection by ordering
  2014-06-16 11:44 [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Ian Campbell
                   ` (6 preceding siblings ...)
  2014-06-16 11:45 ` [PATCH 07/10] xen: arm: store per-boot module type instead of relying on index Ian Campbell
@ 2014-06-16 11:45 ` Ian Campbell
  2014-06-18 14:40   ` Stefano Stabellini
  2014-06-16 11:45 ` [PATCH 09/10] xen: arm: Drop device_tree_node_compatible Ian Campbell
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 35+ messages in thread
From: Ian Campbell @ 2014-06-16 11:45 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Campbell, stefano.stabellini, Naresh Bhat, julien.grall, tim,
	Roy Franz, Fu Wei

Assign modules types based on the order in which they are defined in the FDT.
This is supported only for the dom0 kernel and ramdisk when given as the first
and second modules respectively, similar to how
http://wiki.xen.org/wiki?title=Xen_ARM_with_Virtualization_Extensions/Multiboot&oldid=11824
defined the default types from the bootloader side.

This is compatible with how Xen interprets the modules with x86 multiboot and I
think simplifies things for bootloaders which now need not contain similar
guessing code if they only care about the most basic case.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/bootfdt.c      |   10 +++++++---
 xen/arch/arm/setup.c        |   14 ++++++++++++++
 xen/include/asm-arm/setup.h |   11 ++++++++++-
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index e983aa7..a80055c 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -163,6 +163,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
                                           const char *name,
                                           u32 address_cells, u32 size_cells)
 {
+    static bootmodulekind kind_guess = BOOTMOD_LAST_PRESERVE + 1;
     const struct fdt_property *prop;
     const __be32 *cell;
     bootmodulekind kind;
@@ -178,8 +179,10 @@ static void __init process_multiboot_node(const void *fdt, int node,
         kind = BOOTMOD_RAMDISK;
     else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
         kind = BOOTMOD_XSM;
+    else if ( kind_guess < BOOTMOD_UNKNOWN )
+        kind = kind_guess++;
     else
-        panic("%s not a known xen multiboot type\n", name);
+        kind = BOOTMOD_UNKNOWN;
 
     prop = fdt_get_property(fdt, node, "reg", &len);
     if ( !prop )
@@ -278,11 +281,12 @@ static void __init early_print_info(void)
                      mi->bank[i].start,
                      mi->bank[i].start + mi->bank[i].size - 1);
     printk("\n");
-    for ( i = 1 ; i < mods->nr_mods; i++ )
-        printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %s\n",
+    for ( i = 0 ; i < mods->nr_mods; i++ )
+        printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %-8s %s\n",
                      i,
                      mods->module[i].start,
                      mods->module[i].start + mods->module[i].size,
+                     boot_module_kind_as_string(mods->module[i].kind),
                      mods->module[i].cmdline);
     nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
     for ( i = 0; i < nr_rsvd; i++ )
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 4c9dd3d..d358d04 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -221,6 +221,20 @@ struct bootmodule * __init boot_module_find_by_kind(bootmodulekind kind)
     return NULL;
 }
 
+const char * __init boot_module_kind_as_string(bootmodulekind kind)
+{
+    switch ( kind )
+    {
+    case BOOTMOD_XEN:     return "Xen";
+    case BOOTMOD_FDT:     return "FDT";
+    case BOOTMOD_KERNEL:  return "Kernel";
+    case BOOTMOD_RAMDISK: return "Ramdisk";
+    case BOOTMOD_XSM:     return "XSM";
+    case BOOTMOD_UNKNOWN: return "Unknown";
+    default: BUG();
+    }
+}
+
 void __init discard_initial_modules(void)
 {
     struct bootmodules *mi = &bootinfo.modules;
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 57c98cb..f1a27fb 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -10,12 +10,20 @@
 typedef enum {
     BOOTMOD_XEN,
     BOOTMOD_FDT,
+
     /* Everything up to here is not freed after start of day */
     BOOTMOD_LAST_PRESERVE = BOOTMOD_FDT,
+
+    /*
+     * Default module types. For modules which are not given an
+     * explict type these are automatically used, in this order.
+     */
     BOOTMOD_KERNEL,
     BOOTMOD_RAMDISK,
+    BOOTMOD_UNKNOWN,
+
+    /* The remaining module types are never automatically assigned. */
     BOOTMOD_XSM,
-    BOOTMOD_UNKNOWN
 }  bootmodulekind;
 
 
@@ -66,6 +74,7 @@ const char __init *boot_fdt_cmdline(const void *fdt);
 void add_boot_module(bootmodulekind kind, paddr_t start, paddr_t size,
                      const char *cmdline);
 struct bootmodule *boot_module_find_by_kind(bootmodulekind kind);
+const char * __init boot_module_kind_as_string(bootmodulekind kind);
 
 #endif
 /*
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 09/10] xen: arm: Drop device_tree_node_compatible
  2014-06-16 11:44 [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Ian Campbell
                   ` (7 preceding siblings ...)
  2014-06-16 11:45 ` [PATCH 08/10] xen: arm: support bootmodule type detection by ordering Ian Campbell
@ 2014-06-16 11:45 ` Ian Campbell
  2014-06-18 14:43   ` Stefano Stabellini
  2014-06-16 11:45 ` [PATCH 10/10] xen: arm: update multiboot device tree bindings Ian Campbell
  2014-07-15 18:22 ` [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Roy Franz
  10 siblings, 1 reply; 35+ messages in thread
From: Ian Campbell @ 2014-06-16 11:45 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Campbell, stefano.stabellini, Naresh Bhat, julien.grall, tim,
	Roy Franz, Fu Wei

Instead use fdt_node_check_compatible from libfdt.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/bootfdt.c |   28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index a80055c..8ab45c9 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -31,30 +31,6 @@ static bool_t __init device_tree_node_matches(const void *fdt, int node,
         && (name[match_len] == '@' || name[match_len] == '\0');
 }
 
-static bool_t __init device_tree_node_compatible(const void *fdt, int node,
-                                                 const char *match)
-{
-    int len, l;
-    int mlen;
-    const void *prop;
-
-    mlen = strlen(match);
-
-    prop = fdt_getprop(fdt, node, "compatible", &len);
-    if ( prop == NULL )
-        return 0;
-
-    while ( len > 0 ) {
-        if ( !dt_compat_cmp(prop, match) )
-            return 1;
-        l = strlen(prop) + 1;
-        prop += l;
-        len -= l;
-    }
-
-    return 0;
-}
-
 static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
                                        u32 size_cells, u64 *start, u64 *size)
 {
@@ -261,8 +237,8 @@ static int __init early_scan_node(const void *fdt,
 {
     if ( device_tree_node_matches(fdt, node, "memory") )
         process_memory_node(fdt, node, name, address_cells, size_cells);
-    else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
-              device_tree_node_compatible(fdt, node, "multiboot,module" ))
+    else if ( fdt_node_check_compatible(fdt, node, "xen,multiboot-module" ) ||
+              fdt_node_check_compatible(fdt, node, "multiboot,module" ))
         process_multiboot_node(fdt, node, name, address_cells, size_cells);
     else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
         process_chosen_node(fdt, node, name, address_cells, size_cells);
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 10/10] xen: arm: update multiboot device tree bindings.
  2014-06-16 11:44 [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Ian Campbell
                   ` (8 preceding siblings ...)
  2014-06-16 11:45 ` [PATCH 09/10] xen: arm: Drop device_tree_node_compatible Ian Campbell
@ 2014-06-16 11:45 ` Ian Campbell
  2014-06-18 14:56   ` Stefano Stabellini
  2014-07-15 18:22 ` [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Roy Franz
  10 siblings, 1 reply; 35+ messages in thread
From: Ian Campbell @ 2014-06-16 11:45 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Campbell, stefano.stabellini, Naresh Bhat, julien.grall, tim,
	Roy Franz, Fu Wei

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 docs/misc/arm/device-tree/booting.txt |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index bfb8d01..92af119 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -8,15 +8,27 @@ Each node contains the following properties:
 
 - compatible
 
-	Must be:
+	Must always include at least:
 
-		"xen,<type>", "xen,multiboot-module"
+		"multiboot,module"
 
-	where <type> must be one of:
+	Optionally a more specific compatible string may be used in
+	addition to the above. One of:
 
-	- "linux-zimage" -- the dom0 kernel
-	- "linux-initrd" -- the dom0 ramdisk
-	- "xsm-policy"	 -- XSM policy blob
+	- "multiboot,kernel"	-- the dom0 kernel
+	- "multiboot,ramdisk"	-- the dom0 ramdisk
+	- "xen,xsm-policy"	-- XSM policy blob
+
+	For compatibility with Xen 4.4 the following are also
+	supported:
+
+		"xen,multiboot-module" equivalent to "multiboot,module"
+		"xen,linux-zimage" equivalent to "multiboot,kernel"
+		"xen,linux-initrd" equivalent to "multiboot,ramdisk"
+
+	Xen 4.4 does not know the "multiboot,*" compatibility
+	strings. Also when using Xen 4.4 the more specific
+	"xen,linux-*" names are non-optional.
 
 - reg
 
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* Re: [PATCH 03/10] xen: arm: prefer typesafe max()/min() over MAX()/MIN()
  2014-06-16 11:44 ` [PATCH 03/10] xen: arm: prefer typesafe max()/min() over MAX()/MIN() Ian Campbell
@ 2014-06-16 12:08   ` Julien Grall
  0 siblings, 0 replies; 35+ messages in thread
From: Julien Grall @ 2014-06-16 12:08 UTC (permalink / raw)
  To: Ian Campbell, xen-devel
  Cc: Roy Franz, Naresh Bhat, tim, Fu Wei, stefano.stabellini

Hi Ian,

On 06/16/2014 12:44 PM, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 04/10] xen: arm: rename early_info structs
  2014-06-16 11:44 ` [PATCH 04/10] xen: arm: rename early_info structs Ian Campbell
@ 2014-06-16 12:10   ` Julien Grall
  2014-06-18 13:56   ` Stefano Stabellini
  1 sibling, 0 replies; 35+ messages in thread
From: Julien Grall @ 2014-06-16 12:10 UTC (permalink / raw)
  To: Ian Campbell, xen-devel
  Cc: Roy Franz, Naresh Bhat, tim, Fu Wei, stefano.stabellini

Hi Ian,

On 06/16/2014 12:44 PM, Ian Campbell wrote:
> There isn't really anything Device Tree specific about the early_info, we just
> happen to get it from device tree (but in the future it might come e.g. from
> UEFI or ACPI or something else).
> 
> Move the relevant structs out of device_tree.h and into asm/setup.h and rename to
> be more neutral.
> 
> For now the code to parse the DT into the now arch specific structs remains in
> common code.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 05/10] xen: arm: move boot time fdt parsing into separate file.
  2014-06-16 11:44 ` [PATCH 05/10] xen: arm: move boot time fdt parsing into separate file Ian Campbell
@ 2014-06-16 12:13   ` Julien Grall
  2014-06-18 14:00   ` Stefano Stabellini
  1 sibling, 0 replies; 35+ messages in thread
From: Julien Grall @ 2014-06-16 12:13 UTC (permalink / raw)
  To: Ian Campbell, xen-devel
  Cc: Roy Franz, Naresh Bhat, tim, Fu Wei, stefano.stabellini

Hi Ian,

On 06/16/2014 12:44 PM, Ian Campbell wrote:
> Move the early code for walking the flattended device tree out of

s/flattended/flattened/

> device_tree.c. The intention is that eventually only only the proper (i.e.

I suspect you add twice "only" by mistake.

> unflattened) device tree support will live in device_tree.c.
> 
> The new home is bootfdt.c to try and better reflect the purpose of the code.
> Although in theory this early code could be generic in reality it is pretty ARM
> specific, so place it under xen/arch/arm until a second user wants it.
> 
> As part of the move rename device_tree_early_init to boot_fdt_info. Drop
> device_tree_dump, it is unused.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

With the small changes above:

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

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 06/10] xen: arm: device_tree_bootargs to bootfdt.c, renaming to boot_fdt_cmdline
  2014-06-16 11:44 ` [PATCH 06/10] xen: arm: device_tree_bootargs to bootfdt.c, renaming to boot_fdt_cmdline Ian Campbell
@ 2014-06-16 12:32   ` Julien Grall
  2014-06-18 14:02   ` Stefano Stabellini
  1 sibling, 0 replies; 35+ messages in thread
From: Julien Grall @ 2014-06-16 12:32 UTC (permalink / raw)
  To: Ian Campbell, xen-devel
  Cc: Roy Franz, Naresh Bhat, tim, Fu Wei, stefano.stabellini

Hi Ian,

Did you forget "move" in the commit title?

On 06/16/2014 12:44 PM, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

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

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 07/10] xen: arm: store per-boot module type instead of relying on index
  2014-06-16 11:45 ` [PATCH 07/10] xen: arm: store per-boot module type instead of relying on index Ian Campbell
@ 2014-06-16 12:48   ` Julien Grall
  2014-06-16 12:52     ` Ian Campbell
  2014-06-18 14:18   ` Stefano Stabellini
  1 sibling, 1 reply; 35+ messages in thread
From: Julien Grall @ 2014-06-16 12:48 UTC (permalink / raw)
  To: Ian Campbell, xen-devel
  Cc: Roy Franz, Naresh Bhat, tim, Fu Wei, stefano.stabellini

Hi Ian,

On 06/16/2014 12:45 PM, Ian Campbell wrote:
> This is more natural and better matches how multiboot is actually supposed to
> work.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/bootfdt.c      |   49 +++++++++++++++----------------------------
>  xen/arch/arm/domain_build.c |   20 +++++++++++-------
>  xen/arch/arm/kernel.c       |   15 ++++++-------
>  xen/arch/arm/setup.c        |   47 ++++++++++++++++++++++++++++++++++++-----
>  xen/include/asm-arm/setup.h |   29 +++++++++++++++++--------
>  5 files changed, 100 insertions(+), 60 deletions(-)

It looks like you forgot to modify xsm/xsm_policy.c.

> +void add_boot_module(bootmodulekind kind, paddr_t start, paddr_t size,
> +                     const char *cmdline)
> +{
> +    struct bootmodules *mods = &bootinfo.modules;
> +    struct bootmodule *mod;
> +
> +    if ( mods->nr_mods == MAX_MODULES )
> +    {
> +        printk("Ignoring boot module at %"PRIpaddr"-%"PRIpaddr" (too many)\n",
> +               start, start + size);
> +        return;
> +    }

As we don't have any way to know if the user add multiple the kernel
and/or the initramfs, I would add a print message here to say what kind
of boot module we are currently adding. It will help the guy to find the
problem quickly.

[..]

>  void __init discard_initial_modules(void)
>  {
>      struct bootmodules *mi = &bootinfo.modules;
>      int i;
>  
> -    for ( i = MOD_DISCARD_FIRST; i <= mi->nr_mods; i++ )
> +    for ( i = 0; i <= mi->nr_mods; i++ )
>      {
>          paddr_t s = mi->module[i].start;
>          paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
>  
> -        dt_unreserved_regions(s, e, init_domheap_pages, 0);
> +        if ( mi->module[i].kind > BOOTMOD_LAST_PRESERVE )
> +            dt_unreserved_regions(s, e, init_domheap_pages, 0);
>      }

[..]


> +typedef enum {
> +    BOOTMOD_XEN,
> +    BOOTMOD_FDT,
> +    /* Everything up to here is not freed after start of day */
> +    BOOTMOD_LAST_PRESERVE = BOOTMOD_FDT,

Currently we also discard the FDT, this is because the FDT is copied
another place in the memory. With this patch the FDT module stays in memory.

I think BOOTMOD_LAST_PRESERVE should be equal to BOOTMOD_XEN.

> +    BOOTMOD_KERNEL,
> +    BOOTMOD_RAMDISK,
> +    BOOTMOD_XSM,
> +    BOOTMOD_UNKNOWN
> +}  bootmodulekind;

I would rename to bootmodule_kind. It's easier to read and you know that
the enum is used for a bootmodule.

Regards,

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 07/10] xen: arm: store per-boot module type instead of relying on index
  2014-06-16 12:48   ` Julien Grall
@ 2014-06-16 12:52     ` Ian Campbell
  0 siblings, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-06-16 12:52 UTC (permalink / raw)
  To: Julien Grall
  Cc: stefano.stabellini, Naresh Bhat, tim, xen-devel, Roy Franz, Fu Wei

On Mon, 2014-06-16 at 13:48 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 06/16/2014 12:45 PM, Ian Campbell wrote:
> > This is more natural and better matches how multiboot is actually supposed to
> > work.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/bootfdt.c      |   49 +++++++++++++++----------------------------
> >  xen/arch/arm/domain_build.c |   20 +++++++++++-------
> >  xen/arch/arm/kernel.c       |   15 ++++++-------
> >  xen/arch/arm/setup.c        |   47 ++++++++++++++++++++++++++++++++++++-----
> >  xen/include/asm-arm/setup.h |   29 +++++++++++++++++--------
> >  5 files changed, 100 insertions(+), 60 deletions(-)
> 
> It looks like you forgot to modify xsm/xsm_policy.c.

Ah, I was relying on the compiler to tell me about all uses of MOD_* but
I didn't have XSM enabled so it didn't build this file. Will fix.

> > +void add_boot_module(bootmodulekind kind, paddr_t start, paddr_t size,
> > +                     const char *cmdline)
> > +{
> > +    struct bootmodules *mods = &bootinfo.modules;
> > +    struct bootmodule *mod;
> > +
> > +    if ( mods->nr_mods == MAX_MODULES )
> > +    {
> > +        printk("Ignoring boot module at %"PRIpaddr"-%"PRIpaddr" (too many)\n",
> > +               start, start + size);
> > +        return;
> > +    }
> 
> As we don't have any way to know if the user add multiple the kernel
> and/or the initramfs, I would add a print message here to say what kind
> of boot module we are currently adding. It will help the guy to find the
> problem quickly.

OK.

> [..]
> 
> >  void __init discard_initial_modules(void)
> >  {
> >      struct bootmodules *mi = &bootinfo.modules;
> >      int i;
> >  
> > -    for ( i = MOD_DISCARD_FIRST; i <= mi->nr_mods; i++ )
> > +    for ( i = 0; i <= mi->nr_mods; i++ )
> >      {
> >          paddr_t s = mi->module[i].start;
> >          paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
> >  
> > -        dt_unreserved_regions(s, e, init_domheap_pages, 0);
> > +        if ( mi->module[i].kind > BOOTMOD_LAST_PRESERVE )
> > +            dt_unreserved_regions(s, e, init_domheap_pages, 0);
> >      }
> 
> [..]
> 
> 
> > +typedef enum {
> > +    BOOTMOD_XEN,
> > +    BOOTMOD_FDT,
> > +    /* Everything up to here is not freed after start of day */
> > +    BOOTMOD_LAST_PRESERVE = BOOTMOD_FDT,
> 
> Currently we also discard the FDT, this is because the FDT is copied
> another place in the memory. With this patch the FDT module stays in memory.
> 
> I think BOOTMOD_LAST_PRESERVE should be equal to BOOTMOD_XEN.

Agreed, this was an unintentional change.

> > +    BOOTMOD_KERNEL,
> > +    BOOTMOD_RAMDISK,
> > +    BOOTMOD_XSM,
> > +    BOOTMOD_UNKNOWN
> > +}  bootmodulekind;
> 
> I would rename to bootmodule_kind. It's easier to read and you know that
> the enum is used for a bootmodule.

OK.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 04/10] xen: arm: rename early_info structs
  2014-06-16 11:44 ` [PATCH 04/10] xen: arm: rename early_info structs Ian Campbell
  2014-06-16 12:10   ` Julien Grall
@ 2014-06-18 13:56   ` Stefano Stabellini
  1 sibling, 0 replies; 35+ messages in thread
From: Stefano Stabellini @ 2014-06-18 13:56 UTC (permalink / raw)
  To: Ian Campbell
  Cc: stefano.stabellini, Naresh Bhat, julien.grall, tim, xen-devel,
	Roy Franz, Fu Wei

On Mon, 16 Jun 2014, Ian Campbell wrote:
> There isn't really anything Device Tree specific about the early_info, we just
> happen to get it from device tree (but in the future it might come e.g. from
> UEFI or ACPI or something else).
> 
> Move the relevant structs out of device_tree.h and into asm/setup.h and rename to
> be more neutral.
> 
> For now the code to parse the DT into the now arch specific structs remains in
> common code.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/domain_build.c   |   13 ++++++-----
>  xen/arch/arm/kernel.c         |    6 ++---
>  xen/arch/arm/kernel.h         |    3 ++-
>  xen/arch/arm/setup.c          |   51 ++++++++++++++++++++++-------------------
>  xen/common/device_tree.c      |   38 +++++++++++++++---------------
>  xen/include/asm-arm/setup.h   |   40 ++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h |   39 -------------------------------
>  7 files changed, 98 insertions(+), 92 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9d9cba9..5eef8a3 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -16,6 +16,7 @@
>  #include <asm/setup.h>
>  #include <asm/platform.h>
>  #include <asm/psci.h>
> +#include <asm/setup.h>
>  
>  #include <asm/gic.h>
>  #include <xen/irq.h>
> @@ -160,9 +161,9 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>      int res = 0;
>      int had_dom0_bootargs = 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];
> +    if ( bootinfo.modules.nr_mods >= MOD_KERNEL &&
> +         bootinfo.modules.module[MOD_KERNEL].cmdline[0] )
> +        bootargs = &bootinfo.modules.module[MOD_KERNEL].cmdline[0];
>  
>      dt_for_each_property_node (node, prop)
>      {
> @@ -221,7 +222,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
>           * If the bootloader provides an initrd, we must create a placeholder
>           * for the initrd properties. The values will be replaced later.
>           */
> -        if ( early_info.modules.module[MOD_INITRD].size )
> +        if ( bootinfo.modules.module[MOD_INITRD].size )
>          {
>              u64 a = 0;
>              res = fdt_property(kinfo->fdt, "linux,initrd-start", &a, sizeof(a));
> @@ -976,8 +977,8 @@ static void dtb_load(struct kernel_info *kinfo)
>  static void initrd_load(struct kernel_info *kinfo)
>  {
>      paddr_t load_addr = kinfo->initrd_paddr;
> -    paddr_t paddr = early_info.modules.module[MOD_INITRD].start;
> -    paddr_t len = early_info.modules.module[MOD_INITRD].size;
> +    paddr_t paddr = bootinfo.modules.module[MOD_INITRD].start;
> +    paddr_t len = bootinfo.modules.module[MOD_INITRD].size;
>      unsigned long offs;
>      int node;
>      int res;
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 69182ec..ce5b95a 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -69,7 +69,7 @@ static void place_modules(struct kernel_info *info,
>  {
>      /* Align DTB and initrd size to 2Mb. Linux only requires 4 byte alignment */
>      const paddr_t initrd_len =
> -        ROUNDUP(early_info.modules.module[MOD_INITRD].size, MB(2));
> +        ROUNDUP(bootinfo.modules.module[MOD_INITRD].size, MB(2));
>      const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt), MB(2));
>      const paddr_t modsize = initrd_len + dtb_len;
>  
> @@ -376,8 +376,8 @@ int kernel_probe(struct kernel_info *info)
>  
>      paddr_t start, size;
>  
> -    start = early_info.modules.module[MOD_KERNEL].start;
> -    size = early_info.modules.module[MOD_KERNEL].size;
> +    start = bootinfo.modules.module[MOD_KERNEL].start;
> +    size = bootinfo.modules.module[MOD_KERNEL].size;
>  
>      if ( !size )
>      {
> diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
> index fd2f61d..7c7f624 100644
> --- a/xen/arch/arm/kernel.h
> +++ b/xen/arch/arm/kernel.h
> @@ -8,6 +8,7 @@
>  
>  #include <xen/libelf.h>
>  #include <xen/device_tree.h>
> +#include <asm/setup.h>
>  
>  struct kernel_info {
>  #ifdef CONFIG_ARM_64
> @@ -16,7 +17,7 @@ struct kernel_info {
>  
>      void *fdt; /* flat device tree */
>      paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
> -    struct dt_mem_info mem;
> +    struct meminfo mem;
>  
>      /* kernel entry point */
>      paddr_t entry;
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index b9ce7a9..63f6b8e 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -43,8 +43,11 @@
>  #include <asm/cpufeature.h>
>  #include <asm/platform.h>
>  #include <asm/procinfo.h>
> +#include <asm/setup.h>
>  #include <xsm/xsm.h>
>  
> +struct bootinfo __initdata bootinfo;
> +
>  struct cpuinfo_arm __read_mostly boot_cpu_data;
>  
>  static __used void init_done(void)
> @@ -182,7 +185,7 @@ static void dt_unreserved_regions(paddr_t s, paddr_t e,
>  
>  void __init discard_initial_modules(void)
>  {
> -    struct dt_module_info *mi = &early_info.modules;
> +    struct bootmodules *mi = &bootinfo.modules;
>      int i;
>  
>      for ( i = MOD_DISCARD_FIRST; i <= mi->nr_mods; i++ )
> @@ -210,7 +213,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
>                                         uint32_t size, paddr_t align,
>                                         int first_mod)
>  {
> -    const struct dt_module_info *mi = &early_info.modules;
> +    const struct bootmodules *mi = &bootinfo.modules;
>      int i;
>      int nr_rsvd;
>  
> @@ -275,7 +278,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
>   */
>  static paddr_t __init next_module(paddr_t s, paddr_t *end)
>  {
> -    struct dt_module_info *mi = &early_info.modules;
> +    struct bootmodules *mi = &bootinfo.modules;
>      paddr_t lowest = ~(paddr_t)0;
>      int i;
>  
> @@ -308,7 +311,7 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
>   */
>  static paddr_t __init get_xen_paddr(void)
>  {
> -    struct dt_mem_info *mi = &early_info.mem;
> +    struct meminfo *mi = &bootinfo.mem;
>      paddr_t min_size;
>      paddr_t paddr = 0, last_end;
>      int i;
> @@ -357,8 +360,8 @@ static paddr_t __init get_xen_paddr(void)
>      printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
>             paddr, paddr + min_size);
>  
> -    early_info.modules.module[MOD_XEN].start = paddr;
> -    early_info.modules.module[MOD_XEN].size = min_size;
> +    bootinfo.modules.module[MOD_XEN].start = paddr;
> +    bootinfo.modules.module[MOD_XEN].size = min_size;
>  
>      return paddr;
>  }
> @@ -376,7 +379,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>      int i;
>      void *fdt;
>  
> -    if ( !early_info.mem.nr_banks )
> +    if ( !bootinfo.mem.nr_banks )
>          panic("No memory bank");
>  
>      /*
> @@ -393,15 +396,15 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>       * We also track the number of actual RAM pages (i.e. not counting
>       * the holes).
>       */
> -    ram_size  = early_info.mem.bank[0].size;
> +    ram_size  = bootinfo.mem.bank[0].size;
>  
> -    contig_start = ram_start = early_info.mem.bank[0].start;
> +    contig_start = ram_start = bootinfo.mem.bank[0].start;
>      contig_end   = ram_end = ram_start + ram_size;
>  
> -    for ( i = 1; i < early_info.mem.nr_banks; i++ )
> +    for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
>      {
> -        paddr_t bank_start = early_info.mem.bank[i].start;
> -        paddr_t bank_size = early_info.mem.bank[i].size;
> +        paddr_t bank_start = bootinfo.mem.bank[i].start;
> +        paddr_t bank_size = bootinfo.mem.bank[i].size;
>          paddr_t bank_end = bank_start + bank_size;
>  
>          paddr_t new_ram_size = ram_size + bank_size;
> @@ -434,11 +437,11 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>          ram_end = new_ram_end;
>      }
>  
> -    if ( i != early_info.mem.nr_banks )
> +    if ( i != bootinfo.mem.nr_banks )
>      {
>          printk("WARNING: only using %d out of %d memory banks\n",
> -               i, early_info.mem.nr_banks);
> -        early_info.mem.nr_banks = i;
> +               i, bootinfo.mem.nr_banks);
> +        bootinfo.mem.nr_banks = i;
>      }
>  
>      total_pages = ram_pages = ram_size >> PAGE_SHIFT;
> @@ -497,10 +500,10 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>      device_tree_flattened = fdt;
>  
>      /* Add non-xenheap memory */
> -    for ( i = 0; i < early_info.mem.nr_banks; i++ )
> +    for ( i = 0; i < bootinfo.mem.nr_banks; i++ )
>      {
> -        paddr_t bank_start = early_info.mem.bank[i].start;
> -        paddr_t bank_end = bank_start + early_info.mem.bank[i].size;
> +        paddr_t bank_start = bootinfo.mem.bank[i].start;
> +        paddr_t bank_end = bank_start + bootinfo.mem.bank[i].size;
>  
>          s = bank_start;
>          while ( s < bank_end )
> @@ -557,10 +560,10 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>      void *fdt;
>  
>      total_pages = 0;
> -    for ( bank = 0 ; bank < early_info.mem.nr_banks; bank++ )
> +    for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
>      {
> -        paddr_t bank_start = early_info.mem.bank[bank].start;
> -        paddr_t bank_size = early_info.mem.bank[bank].size;
> +        paddr_t bank_start = bootinfo.mem.bank[bank].start;
> +        paddr_t bank_size = bootinfo.mem.bank[bank].size;
>          paddr_t bank_end = bank_start + bank_size;
>          paddr_t s, e;
>  
> @@ -609,11 +612,11 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>          }
>      }
>  
> -    if ( bank != early_info.mem.nr_banks )
> +    if ( bank != bootinfo.mem.nr_banks )
>      {
>          printk("WARNING: only using %d out of %d memory banks\n",
> -               bank, early_info.mem.nr_banks);
> -        early_info.mem.nr_banks = bank;
> +               bank, bootinfo.mem.nr_banks);
> +        bootinfo.mem.nr_banks = bank;
>      }
>  
>      total_pages += ram_size >> PAGE_SHIFT;
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 03d495a..a1896d3 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -23,8 +23,8 @@
>  #include <xen/cpumask.h>
>  #include <xen/ctype.h>
>  #include <xen/lib.h>
> +#include <asm/setup.h>
>  
> -struct dt_early_info __initdata early_info;
>  const void *device_tree_flattened;
>  dt_irq_xlate_func dt_irq_xlate;
>  /* Host device tree */
> @@ -238,10 +238,10 @@ const char *device_tree_bootargs(const void *fdt)
>      prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
>      if ( prop == NULL )
>      {
> -        struct dt_mb_module *dom0_mod = NULL;
> +        struct bootmodule *dom0_mod = NULL;
>  
> -        if ( early_info.modules.nr_mods >= MOD_KERNEL )
> -            dom0_mod = &early_info.modules.module[MOD_KERNEL];
> +        if ( bootinfo.modules.nr_mods >= MOD_KERNEL )
> +            dom0_mod = &bootinfo.modules.module[MOD_KERNEL];
>  
>          if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL) ||
>              ( dom0_mod && dom0_mod->cmdline[0] ) )
> @@ -319,12 +319,12 @@ static void __init process_memory_node(const void *fdt, int node,
>      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++ )
> +    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
>      {
>          device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> -        early_info.mem.bank[early_info.mem.nr_banks].start = start;
> -        early_info.mem.bank[early_info.mem.nr_banks].size = size;
> -        early_info.mem.nr_banks++;
> +        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
> +        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
> +        bootinfo.mem.nr_banks++;
>      }
>  }
>  
> @@ -335,7 +335,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
>      const struct fdt_property *prop;
>      const __be32 *cell;
>      int nr;
> -    struct dt_mb_module *mod;
> +    struct bootmodule *mod;
>      int len;
>  
>      if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
> @@ -349,7 +349,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
>      else
>          panic("%s not a known xen multiboot type\n", name);
>  
> -    mod = &early_info.modules.module[nr];
> +    mod = &bootinfo.modules.module[nr];
>  
>      prop = fdt_get_property(fdt, node, "reg", &len);
>      if ( !prop )
> @@ -374,8 +374,8 @@ static void __init process_multiboot_node(const void *fdt, int node,
>      else
>          mod->cmdline[0] = 0;
>  
> -    if ( nr > early_info.modules.nr_mods )
> -        early_info.modules.nr_mods = nr;
> +    if ( nr > bootinfo.modules.nr_mods )
> +        bootinfo.modules.nr_mods = nr;
>  }
>  
>  static void __init process_chosen_node(const void *fdt, int node,
> @@ -383,7 +383,7 @@ static void __init process_chosen_node(const void *fdt, int node,
>                                         u32 address_cells, u32 size_cells)
>  {
>      const struct fdt_property *prop;
> -    struct dt_mb_module *mod = &early_info.modules.module[MOD_INITRD];
> +    struct bootmodule *mod = &bootinfo.modules.module[MOD_INITRD];
>      paddr_t start, end;
>      int len;
>  
> @@ -425,7 +425,7 @@ static void __init process_chosen_node(const void *fdt, int node,
>      mod->start = start;
>      mod->size = end - start;
>  
> -    early_info.modules.nr_mods = max(MOD_INITRD, early_info.modules.nr_mods);
> +    bootinfo.modules.nr_mods = max(MOD_INITRD, bootinfo.modules.nr_mods);
>  }
>  
>  static int __init early_scan_node(const void *fdt,
> @@ -446,8 +446,8 @@ static int __init early_scan_node(const void *fdt,
>  
>  static void __init early_print_info(void)
>  {
> -    struct dt_mem_info *mi = &early_info.mem;
> -    struct dt_module_info *mods = &early_info.modules;
> +    struct meminfo *mi = &bootinfo.mem;
> +    struct bootmodules *mods = &bootinfo.modules;
>      int i, nr_rsvd;
>  
>      for ( i = 0; i < mi->nr_banks; i++ )
> @@ -483,18 +483,18 @@ static void __init early_print_info(void)
>   */
>  size_t __init device_tree_early_init(const void *fdt, paddr_t paddr)
>  {
> -    struct dt_mb_module *mod;
> +    struct bootmodule *mod;
>      int ret;
>  
>      ret = fdt_check_header(fdt);
>      if ( ret < 0 )
>          panic("No valid device tree\n");
>  
> -    mod = &early_info.modules.module[MOD_FDT];
> +    mod = &bootinfo.modules.module[MOD_FDT];
>      mod->start = paddr;
>      mod->size = fdt_totalsize(fdt);
>  
> -    early_info.modules.nr_mods = max(MOD_FDT, early_info.modules.nr_mods);
> +    bootinfo.modules.nr_mods = max(MOD_FDT, bootinfo.modules.nr_mods);
>  
>      device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
>      early_print_info();
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index b09f688..ea0dc46 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -3,6 +3,46 @@
>  
>  #include <public/version.h>
>  
> +#define NR_MEM_BANKS 8
> +
> +#define MOD_XEN    0
> +#define MOD_FDT    1
> +#define MOD_KERNEL 2
> +#define MOD_INITRD 3
> +#define MOD_XSM    4
> +#define NR_MODULES 5
> +
> +#define MOD_DISCARD_FIRST MOD_FDT
> +
> +struct membank {
> +    paddr_t start;
> +    paddr_t size;
> +};
> +
> +struct meminfo {
> +    int nr_banks;
> +    struct membank bank[NR_MEM_BANKS];
> +};
> +
> +struct bootmodule {
> +    paddr_t start;
> +    paddr_t size;
> +    char cmdline[1024];
> +};
> +
> +struct bootmodules {
> +    int nr_mods;
> +    /* Module 0 is Xen itself, followed by the provided modules-proper */
> +    struct bootmodule module[NR_MODULES];
> +};
> +
> +struct bootinfo {
> +    struct meminfo mem;
> +    struct bootmodules modules;
> +};
> +
> +extern struct bootinfo bootinfo;
> +
>  void arch_init_memory(void);
>  
>  void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 25db076..74e98f5 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -20,44 +20,6 @@
>  
>  #define DEVICE_TREE_MAX_DEPTH 16
>  
> -#define NR_MEM_BANKS 8
> -
> -#define MOD_XEN    0
> -#define MOD_FDT    1
> -#define MOD_KERNEL 2
> -#define MOD_INITRD 3
> -#define MOD_XSM    4
> -#define NR_MODULES 5
> -
> -#define MOD_DISCARD_FIRST MOD_FDT
> -
> -struct membank {
> -    paddr_t start;
> -    paddr_t size;
> -};
> -
> -struct dt_mem_info {
> -    int nr_banks;
> -    struct membank bank[NR_MEM_BANKS];
> -};
> -
> -struct dt_mb_module {
> -    paddr_t start;
> -    paddr_t size;
> -    char cmdline[1024];
> -};
> -
> -struct dt_module_info {
> -    int nr_mods;
> -    /* Module 0 is Xen itself, followed by the provided modules-proper */
> -    struct dt_mb_module module[NR_MODULES];
> -};
> -
> -struct dt_early_info {
> -    struct dt_mem_info mem;
> -    struct dt_module_info modules;
> -};
> -
>  /*
>   * Struct used for matching a device
>   */
> @@ -193,7 +155,6 @@ typedef int (*device_tree_node_func)(const void *fdt,
>                                       u32 address_cells, u32 size_cells,
>                                       void *data);
>  
> -extern struct dt_early_info early_info;
>  extern const void *device_tree_flattened;
>  
>  size_t __init device_tree_early_init(const void *fdt, paddr_t paddr);
> -- 
> 1.7.10.4
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 05/10] xen: arm: move boot time fdt parsing into separate file.
  2014-06-16 11:44 ` [PATCH 05/10] xen: arm: move boot time fdt parsing into separate file Ian Campbell
  2014-06-16 12:13   ` Julien Grall
@ 2014-06-18 14:00   ` Stefano Stabellini
  1 sibling, 0 replies; 35+ messages in thread
From: Stefano Stabellini @ 2014-06-18 14:00 UTC (permalink / raw)
  To: Ian Campbell
  Cc: stefano.stabellini, Naresh Bhat, julien.grall, tim, xen-devel,
	Roy Franz, Fu Wei

On Mon, 16 Jun 2014, Ian Campbell wrote:
> Move the early code for walking the flattended device tree out of
> device_tree.c. The intention is that eventually only only the proper (i.e.
                                                    ^  ^ 
> unflattened) device tree support will live in device_tree.c.
> 
> The new home is bootfdt.c to try and better reflect the purpose of the code.
> Although in theory this early code could be generic in reality it is pretty ARM
> specific, so place it under xen/arch/arm until a second user wants it.
> 
> As part of the move rename device_tree_early_init to boot_fdt_info. Drop
> device_tree_dump, it is unused.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/Makefile         |    1 +
>  xen/arch/arm/bootfdt.c        |  343 +++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/setup.c          |    2 +-
>  xen/common/device_tree.c      |  356 -----------------------------------------
>  xen/include/asm-arm/setup.h   |    2 +
>  xen/include/xen/device_tree.h |    3 -
>  6 files changed, 347 insertions(+), 360 deletions(-)
>  create mode 100644 xen/arch/arm/bootfdt.c
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 63e0460..58a6714 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -21,6 +21,7 @@ obj-y += guestcopy.o
>  obj-y += physdev.o
>  obj-y += platform.o
>  obj-y += setup.o
> +obj-y += bootfdt.o
>  obj-y += time.o
>  obj-y += smpboot.o
>  obj-y += smp.o
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> new file mode 100644
> index 0000000..11182cd
> --- /dev/null
> +++ b/xen/arch/arm/bootfdt.c
> @@ -0,0 +1,343 @@
> +/*
> + * Early Device Tree
> + *
> + * Copyright (C) 2012-2014 Citrix Systems, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <xen/config.h>
> +#include <xen/types.h>
> +#include <xen/lib.h>
> +#include <xen/kernel.h>
> +#include <xen/init.h>
> +#include <xen/device_tree.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <asm/setup.h>
> +
> +static bool_t __init device_tree_node_matches(const void *fdt, int node,
> +                                              const char *match)
> +{
> +    const char *name;
> +    size_t match_len;
> +
> +    name = fdt_get_name(fdt, node, NULL);
> +    match_len = strlen(match);
> +
> +    /* Match both "match" and "match@..." patterns but not
> +       "match-foo". */
> +    return strncmp(name, match, match_len) == 0
> +        && (name[match_len] == '@' || name[match_len] == '\0');
> +}
> +
> +static bool_t __init device_tree_node_compatible(const void *fdt, int node,
> +                                                 const char *match)
> +{
> +    int len, l;
> +    int mlen;
> +    const void *prop;
> +
> +    mlen = strlen(match);
> +
> +    prop = fdt_getprop(fdt, node, "compatible", &len);
> +    if ( prop == NULL )
> +        return 0;
> +
> +    while ( len > 0 ) {
> +        if ( !dt_compat_cmp(prop, match) )
> +            return 1;
> +        l = strlen(prop) + 1;
> +        prop += l;
> +        len -= l;
> +    }
> +
> +    return 0;
> +}
> +
> +static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
> +                                       u32 size_cells, u64 *start, u64 *size)
> +{
> +    *start = dt_next_cell(address_cells, cell);
> +    *size = dt_next_cell(size_cells, cell);
> +}
> +
> +static u32 __init device_tree_get_u32(const void *fdt, int node,
> +                                      const char *prop_name, u32 dflt)
> +{
> +    const struct fdt_property *prop;
> +
> +    prop = fdt_get_property(fdt, node, prop_name, NULL);
> +    if ( !prop || prop->len < sizeof(u32) )
> +        return dflt;
> +
> +    return fdt32_to_cpu(*(uint32_t*)prop->data);
> +}
> +
> +/**
> + * device_tree_for_each_node - iterate over all device tree nodes
> + * @fdt: flat device tree.
> + * @func: function to call for each node.
> + * @data: data to pass to @func.
> + *
> + * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
> + *
> + * Returns 0 if all nodes were iterated over successfully.  If @func
> + * returns a value different from 0, that value is returned immediately.
> + */
> +static int __init device_tree_for_each_node(const void *fdt,
> +                                            device_tree_node_func func,
> +                                            void *data)
> +{
> +    int node;
> +    int depth;
> +    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
> +    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
> +    int ret;
> +
> +    for ( node = 0, depth = 0;
> +          node >=0 && depth >= 0;
> +          node = fdt_next_node(fdt, node, &depth) )
> +    {
> +        const char *name = fdt_get_name(fdt, node, NULL);
> +
> +        if ( depth >= DEVICE_TREE_MAX_DEPTH )
> +        {
> +            printk("Warning: device tree node `%s' is nested too deep\n",
> +                   name);
> +            continue;
> +        }
> +
> +        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);
> +
> +
> +        ret = func(fdt, node, name, depth,
> +                   address_cells[depth-1], size_cells[depth-1], data);
> +        if ( ret != 0 )
> +            return ret;
> +    }
> +    return 0;
> +}
> +
> +static void __init process_memory_node(const void *fdt, int node,
> +                                       const char *name,
> +                                       u32 address_cells, u32 size_cells)
> +{
> +    const struct fdt_property *prop;
> +    int i;
> +    int banks;
> +    const __be32 *cell;
> +    paddr_t start, size;
> +    u32 reg_cells = address_cells + size_cells;
> +
> +    if ( address_cells < 1 || size_cells < 1 )
> +    {
> +        printk("fdt: node `%s': invalid #address-cells or #size-cells",
> +               name);
> +        return;
> +    }
> +
> +    prop = fdt_get_property(fdt, node, "reg", NULL);
> +    if ( !prop )
> +    {
> +        printk("fdt: node `%s': missing `reg' property\n", name);
> +        return;
> +    }
> +
> +    cell = (const __be32 *)prop->data;
> +    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> +
> +    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
> +    {
> +        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
> +        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
> +        bootinfo.mem.nr_banks++;
> +    }
> +}
> +
> +static void __init process_multiboot_node(const void *fdt, int node,
> +                                          const char *name,
> +                                          u32 address_cells, u32 size_cells)
> +{
> +    const struct fdt_property *prop;
> +    const __be32 *cell;
> +    int nr;
> +    struct bootmodule *mod;
> +    int len;
> +
> +    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
> +         fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
> +        nr = MOD_KERNEL;
> +    else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0 ||
> +              fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
> +        nr = MOD_INITRD;
> +    else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
> +        nr = MOD_XSM;
> +    else
> +        panic("%s not a known xen multiboot type\n", name);
> +
> +    mod = &bootinfo.modules.module[nr];
> +
> +    prop = fdt_get_property(fdt, node, "reg", &len);
> +    if ( !prop )
> +        panic("node %s missing `reg' property\n", name);
> +
> +    if ( len < dt_cells_to_size(address_cells + size_cells) )
> +        panic("fdt: node `%s': `reg` property length is too short\n",
> +                    name);
> +
> +    cell = (const __be32 *)prop->data;
> +    device_tree_get_reg(&cell, address_cells, size_cells,
> +                        &mod->start, &mod->size);
> +
> +    prop = fdt_get_property(fdt, node, "bootargs", &len);
> +    if ( prop )
> +    {
> +        if ( len > sizeof(mod->cmdline) )
> +            panic("module %d command line too long\n", nr);
> +
> +        safe_strcpy(mod->cmdline, prop->data);
> +    }
> +    else
> +        mod->cmdline[0] = 0;
> +
> +    if ( nr > bootinfo.modules.nr_mods )
> +        bootinfo.modules.nr_mods = nr;
> +}
> +
> +static void __init process_chosen_node(const void *fdt, int node,
> +                                       const char *name,
> +                                       u32 address_cells, u32 size_cells)
> +{
> +    const struct fdt_property *prop;
> +    struct bootmodule *mod = &bootinfo.modules.module[MOD_INITRD];
> +    paddr_t start, end;
> +    int len;
> +
> +    printk("Checking for initrd in /chosen\n");
> +
> +    prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
> +    if ( !prop )
> +        /* No initrd present. */
> +        return;
> +    if ( len != sizeof(u32) && len != sizeof(u64) )
> +    {
> +        printk("linux,initrd-start property has invalid length %d\n", len);
> +        return;
> +    }
> +    start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> +
> +    prop = fdt_get_property(fdt, node, "linux,initrd-end", &len);
> +    if ( !prop )
> +    {
> +        printk("linux,initrd-end not present but -start was\n");
> +        return;
> +    }
> +    if ( len != sizeof(u32) && len != sizeof(u64) )
> +    {
> +        printk("linux,initrd-end property has invalid length %d\n", len);
> +        return;
> +    }
> +    end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> +
> +    if ( start >= end )
> +    {
> +        printk("linux,initrd limits invalid: %"PRIpaddr" >= %"PRIpaddr"\n",
> +                  start, end);
> +        return;
> +    }
> +
> +    printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
> +
> +    mod->start = start;
> +    mod->size = end - start;
> +
> +    bootinfo.modules.nr_mods = max(MOD_INITRD, bootinfo.modules.nr_mods);
> +}
> +
> +static int __init early_scan_node(const void *fdt,
> +                                  int node, const char *name, int depth,
> +                                  u32 address_cells, u32 size_cells,
> +                                  void *data)
> +{
> +    if ( device_tree_node_matches(fdt, node, "memory") )
> +        process_memory_node(fdt, node, name, address_cells, size_cells);
> +    else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
> +              device_tree_node_compatible(fdt, node, "multiboot,module" ))
> +        process_multiboot_node(fdt, node, name, address_cells, size_cells);
> +    else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
> +        process_chosen_node(fdt, node, name, address_cells, size_cells);
> +
> +    return 0;
> +}
> +
> +static void __init early_print_info(void)
> +{
> +    struct meminfo *mi = &bootinfo.mem;
> +    struct bootmodules *mods = &bootinfo.modules;
> +    int i, nr_rsvd;
> +
> +    for ( i = 0; i < mi->nr_banks; i++ )
> +        printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
> +                     mi->bank[i].start,
> +                     mi->bank[i].start + mi->bank[i].size - 1);
> +    printk("\n");
> +    for ( i = 1 ; i < mods->nr_mods + 1; i++ )
> +        printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %s\n",
> +                     i,
> +                     mods->module[i].start,
> +                     mods->module[i].start + mods->module[i].size,
> +                     mods->module[i].cmdline);
> +    nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
> +    for ( i = 0; i < nr_rsvd; i++ )
> +    {
> +        paddr_t s, e;
> +        if ( fdt_get_mem_rsv(device_tree_flattened, i, &s, &e) < 0 )
> +            continue;
> +        /* fdt_get_mem_rsv returns length */
> +        e += s;
> +        printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
> +                     i, s, e);
> +    }
> +    printk("\n");
> +}
> +
> +/**
> + * boot_fdt_info - initialize bootinfo from a DTB
> + * @fdt: flattened device tree binary
> + *
> + * Returns the size of the DTB.
> + */
> +size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
> +{
> +    struct bootmodule *mod;
> +    int ret;
> +
> +    ret = fdt_check_header(fdt);
> +    if ( ret < 0 )
> +        panic("No valid device tree\n");
> +
> +    mod = &bootinfo.modules.module[MOD_FDT];
> +    mod->start = paddr;
> +    mod->size = fdt_totalsize(fdt);
> +
> +    bootinfo.modules.nr_mods = max(MOD_FDT, bootinfo.modules.nr_mods);
> +
> +    device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
> +    early_print_info();
> +
> +    return fdt_totalsize(fdt);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 63f6b8e..4a84a32 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -678,7 +678,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>      /* This is mapped by head.S */
>      device_tree_flattened = (void *)BOOT_FDT_VIRT_START
>          + (fdt_paddr & ((1 << SECOND_SHIFT) - 1));
> -    fdt_size = device_tree_early_init(device_tree_flattened, fdt_paddr);
> +    fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
>  
>      cmdline = device_tree_bootargs(device_tree_flattened);
>      printk("Command line: %s\n", cmdline);
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index a1896d3..89de269 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -22,7 +22,6 @@
>  #include <xen/string.h>
>  #include <xen/cpumask.h>
>  #include <xen/ctype.h>
> -#include <xen/lib.h>
>  #include <asm/setup.h>
>  
>  const void *device_tree_flattened;
> @@ -89,52 +88,6 @@ struct dt_bus
>      unsigned int (*get_flags)(const __be32 *addr);
>  };
>  
> -static bool_t __init device_tree_node_matches(const void *fdt, int node,
> -                                              const char *match)
> -{
> -    const char *name;
> -    size_t match_len;
> -
> -    name = fdt_get_name(fdt, node, NULL);
> -    match_len = strlen(match);
> -
> -    /* Match both "match" and "match@..." patterns but not
> -       "match-foo". */
> -    return strncmp(name, match, match_len) == 0
> -        && (name[match_len] == '@' || name[match_len] == '\0');
> -}
> -
> -static bool_t __init device_tree_node_compatible(const void *fdt, int node,
> -                                                 const char *match)
> -{
> -    int len, l;
> -    int mlen;
> -    const void *prop;
> -
> -    mlen = strlen(match);
> -
> -    prop = fdt_getprop(fdt, node, "compatible", &len);
> -    if ( prop == NULL )
> -        return 0;
> -
> -    while ( len > 0 ) {
> -        if ( !dt_compat_cmp(prop, match) )
> -            return 1;
> -        l = strlen(prop) + 1;
> -        prop += l;
> -        len -= l;
> -    }
> -
> -    return 0;
> -}
> -
> -static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
> -                                       u32 size_cells, u64 *start, u64 *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,
>                    u64 *address, u64 *size)
>  {
> @@ -162,66 +115,6 @@ void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
>      dt_set_cell(cellp, dt_n_size_cells(np), size);
>  }
>  
> -static u32 __init device_tree_get_u32(const void *fdt, int node,
> -                                      const char *prop_name, u32 dflt)
> -{
> -    const struct fdt_property *prop;
> -
> -    prop = fdt_get_property(fdt, node, prop_name, NULL);
> -    if ( !prop || prop->len < sizeof(u32) )
> -        return dflt;
> -
> -    return fdt32_to_cpu(*(uint32_t*)prop->data);
> -}
> -
> -/**
> - * device_tree_for_each_node - iterate over all device tree nodes
> - * @fdt: flat device tree.
> - * @func: function to call for each node.
> - * @data: data to pass to @func.
> - *
> - * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored.
> - *
> - * Returns 0 if all nodes were iterated over successfully.  If @func
> - * returns a value different from 0, that value is returned immediately.
> - */
> -static int __init device_tree_for_each_node(const void *fdt,
> -                                            device_tree_node_func func,
> -                                            void *data)
> -{
> -    int node;
> -    int depth;
> -    u32 address_cells[DEVICE_TREE_MAX_DEPTH];
> -    u32 size_cells[DEVICE_TREE_MAX_DEPTH];
> -    int ret;
> -
> -    for ( node = 0, depth = 0;
> -          node >=0 && depth >= 0;
> -          node = fdt_next_node(fdt, node, &depth) )
> -    {
> -        const char *name = fdt_get_name(fdt, node, NULL);
> -
> -        if ( depth >= DEVICE_TREE_MAX_DEPTH )
> -        {
> -            printk("Warning: device tree node `%s' is nested too deep\n",
> -                   name);
> -            continue;
> -        }
> -
> -        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);
> -
> -
> -        ret = func(fdt, node, name, depth,
> -                   address_cells[depth-1], size_cells[depth-1], data);
> -        if ( ret != 0 )
> -            return ret;
> -    }
> -    return 0;
> -}
> -
>  /**
>   * device_tree_bootargs - return the bootargs (the Xen command line)
>   * @fdt flat device tree.
> @@ -253,255 +146,6 @@ const char *device_tree_bootargs(const void *fdt)
>      return prop->data;
>  }
>  
> -static int dump_node(const void *fdt, int node, const char *name, int depth,
> -                     u32 address_cells, u32 size_cells, void *data)
> -{
> -    char prefix[2*DEVICE_TREE_MAX_DEPTH + 1] = "";
> -    int i;
> -    int prop;
> -
> -    for ( i = 0; i < depth; i++ )
> -        safe_strcat(prefix, "  ");
> -
> -    if ( name[0] == '\0' )
> -        name = "/";
> -    printk("%s%s:\n", prefix, name);
> -
> -    for ( prop = fdt_first_property_offset(fdt, node);
> -          prop >= 0;
> -          prop = fdt_next_property_offset(fdt, prop) )
> -    {
> -        const struct fdt_property *p;
> -
> -        p = fdt_get_property_by_offset(fdt, prop, NULL);
> -
> -        printk("%s  %s\n", prefix, fdt_string(fdt, fdt32_to_cpu(p->nameoff)));
> -    }
> -
> -    return 0;
> -}
> -
> -/**
> - * device_tree_dump - print a text representation of a device tree
> - * @fdt: flat device tree to print
> - */
> -void __init device_tree_dump(const void *fdt)
> -{
> -    device_tree_for_each_node(fdt, dump_node, NULL);
> -}
> -
> -
> -static void __init process_memory_node(const void *fdt, int node,
> -                                       const char *name,
> -                                       u32 address_cells, u32 size_cells)
> -{
> -    const struct fdt_property *prop;
> -    int i;
> -    int banks;
> -    const __be32 *cell;
> -    paddr_t start, size;
> -    u32 reg_cells = address_cells + size_cells;
> -
> -    if ( address_cells < 1 || size_cells < 1 )
> -    {
> -        printk("fdt: node `%s': invalid #address-cells or #size-cells",
> -               name);
> -        return;
> -    }
> -
> -    prop = fdt_get_property(fdt, node, "reg", NULL);
> -    if ( !prop )
> -    {
> -        printk("fdt: node `%s': missing `reg' property\n", name);
> -        return;
> -    }
> -
> -    cell = (const __be32 *)prop->data;
> -    banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> -
> -    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
> -    {
> -        device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
> -        bootinfo.mem.nr_banks++;
> -    }
> -}
> -
> -static void __init process_multiboot_node(const void *fdt, int node,
> -                                          const char *name,
> -                                          u32 address_cells, u32 size_cells)
> -{
> -    const struct fdt_property *prop;
> -    const __be32 *cell;
> -    int nr;
> -    struct bootmodule *mod;
> -    int len;
> -
> -    if ( fdt_node_check_compatible(fdt, node, "xen,linux-zimage") == 0 ||
> -         fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
> -        nr = MOD_KERNEL;
> -    else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0 ||
> -              fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
> -        nr = MOD_INITRD;
> -    else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
> -        nr = MOD_XSM;
> -    else
> -        panic("%s not a known xen multiboot type\n", name);
> -
> -    mod = &bootinfo.modules.module[nr];
> -
> -    prop = fdt_get_property(fdt, node, "reg", &len);
> -    if ( !prop )
> -        panic("node %s missing `reg' property\n", name);
> -
> -    if ( len < dt_cells_to_size(address_cells + size_cells) )
> -        panic("fdt: node `%s': `reg` property length is too short\n",
> -                    name);
> -
> -    cell = (const __be32 *)prop->data;
> -    device_tree_get_reg(&cell, address_cells, size_cells,
> -                        &mod->start, &mod->size);
> -
> -    prop = fdt_get_property(fdt, node, "bootargs", &len);
> -    if ( prop )
> -    {
> -        if ( len > sizeof(mod->cmdline) )
> -            panic("module %d command line too long\n", nr);
> -
> -        safe_strcpy(mod->cmdline, prop->data);
> -    }
> -    else
> -        mod->cmdline[0] = 0;
> -
> -    if ( nr > bootinfo.modules.nr_mods )
> -        bootinfo.modules.nr_mods = nr;
> -}
> -
> -static void __init process_chosen_node(const void *fdt, int node,
> -                                       const char *name,
> -                                       u32 address_cells, u32 size_cells)
> -{
> -    const struct fdt_property *prop;
> -    struct bootmodule *mod = &bootinfo.modules.module[MOD_INITRD];
> -    paddr_t start, end;
> -    int len;
> -
> -    printk("Checking for initrd in /chosen\n");
> -
> -    prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
> -    if ( !prop )
> -        /* No initrd present. */
> -        return;
> -    if ( len != sizeof(u32) && len != sizeof(u64) )
> -    {
> -        printk("linux,initrd-start property has invalid length %d\n", len);
> -        return;
> -    }
> -    start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> -
> -    prop = fdt_get_property(fdt, node, "linux,initrd-end", &len);
> -    if ( !prop )
> -    {
> -        printk("linux,initrd-end not present but -start was\n");
> -        return;
> -    }
> -    if ( len != sizeof(u32) && len != sizeof(u64) )
> -    {
> -        printk("linux,initrd-end property has invalid length %d\n", len);
> -        return;
> -    }
> -    end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> -
> -    if ( start >= end )
> -    {
> -        printk("linux,initrd limits invalid: %"PRIpaddr" >= %"PRIpaddr"\n",
> -                  start, end);
> -        return;
> -    }
> -
> -    printk("Initrd %"PRIpaddr"-%"PRIpaddr"\n", start, end);
> -
> -    mod->start = start;
> -    mod->size = end - start;
> -
> -    bootinfo.modules.nr_mods = max(MOD_INITRD, bootinfo.modules.nr_mods);
> -}
> -
> -static int __init early_scan_node(const void *fdt,
> -                                  int node, const char *name, int depth,
> -                                  u32 address_cells, u32 size_cells,
> -                                  void *data)
> -{
> -    if ( device_tree_node_matches(fdt, node, "memory") )
> -        process_memory_node(fdt, node, name, address_cells, size_cells);
> -    else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
> -              device_tree_node_compatible(fdt, node, "multiboot,module" ))
> -        process_multiboot_node(fdt, node, name, address_cells, size_cells);
> -    else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
> -        process_chosen_node(fdt, node, name, address_cells, size_cells);
> -
> -    return 0;
> -}
> -
> -static void __init early_print_info(void)
> -{
> -    struct meminfo *mi = &bootinfo.mem;
> -    struct bootmodules *mods = &bootinfo.modules;
> -    int i, nr_rsvd;
> -
> -    for ( i = 0; i < mi->nr_banks; i++ )
> -        printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
> -                     mi->bank[i].start,
> -                     mi->bank[i].start + mi->bank[i].size - 1);
> -    printk("\n");
> -    for ( i = 1 ; i < mods->nr_mods + 1; i++ )
> -        printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %s\n",
> -                     i,
> -                     mods->module[i].start,
> -                     mods->module[i].start + mods->module[i].size,
> -                     mods->module[i].cmdline);
> -    nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
> -    for ( i = 0; i < nr_rsvd; i++ )
> -    {
> -        paddr_t s, e;
> -        if ( fdt_get_mem_rsv(device_tree_flattened, i, &s, &e) < 0 )
> -            continue;
> -        /* fdt_get_mem_rsv returns length */
> -        e += s;
> -        printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
> -                     i, s, e);
> -    }
> -    printk("\n");
> -}
> -
> -/**
> - * device_tree_early_init - initialize early info from a DTB
> - * @fdt: flattened device tree binary
> - *
> - * Returns the size of the DTB.
> - */
> -size_t __init device_tree_early_init(const void *fdt, paddr_t paddr)
> -{
> -    struct bootmodule *mod;
> -    int ret;
> -
> -    ret = fdt_check_header(fdt);
> -    if ( ret < 0 )
> -        panic("No valid device tree\n");
> -
> -    mod = &bootinfo.modules.module[MOD_FDT];
> -    mod->start = paddr;
> -    mod->size = fdt_totalsize(fdt);
> -
> -    bootinfo.modules.nr_mods = max(MOD_FDT, bootinfo.modules.nr_mods);
> -
> -    device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
> -    early_print_info();
> -
> -    return fdt_totalsize(fdt);
> -}
> -
>  static void __init *unflatten_dt_alloc(unsigned long *mem, unsigned long size,
>                                         unsigned long align)
>  {
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index ea0dc46..21dbcd4 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -53,6 +53,8 @@ int construct_dom0(struct domain *d);
>  
>  void discard_initial_modules(void);
>  
> +size_t __init boot_fdt_info(const void *fdt, paddr_t paddr);
> +
>  #endif
>  /*
>   * Local variables:
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 74e98f5..0edec85 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -157,10 +157,7 @@ typedef int (*device_tree_node_func)(const void *fdt,
>  
>  extern const void *device_tree_flattened;
>  
> -size_t __init device_tree_early_init(const void *fdt, paddr_t paddr);
> -
>  const char __init *device_tree_bootargs(const void *fdt);
> -void __init device_tree_dump(const void *fdt);
>  
>  /**
>   * dt_unflatten_host_device_tree - Unflatten the host device tree
> -- 
> 1.7.10.4
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 06/10] xen: arm: device_tree_bootargs to bootfdt.c, renaming to boot_fdt_cmdline
  2014-06-16 11:44 ` [PATCH 06/10] xen: arm: device_tree_bootargs to bootfdt.c, renaming to boot_fdt_cmdline Ian Campbell
  2014-06-16 12:32   ` Julien Grall
@ 2014-06-18 14:02   ` Stefano Stabellini
  1 sibling, 0 replies; 35+ messages in thread
From: Stefano Stabellini @ 2014-06-18 14:02 UTC (permalink / raw)
  To: Ian Campbell
  Cc: stefano.stabellini, Naresh Bhat, julien.grall, tim, xen-devel,
	Roy Franz, Fu Wei

Missing "moving" in the subject line

On Mon, 16 Jun 2014, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/bootfdt.c        |   27 +++++++++++++++++++++++++++
>  xen/arch/arm/setup.c          |    2 +-
>  xen/common/device_tree.c      |   31 -------------------------------
>  xen/include/asm-arm/setup.h   |    1 +
>  xen/include/xen/device_tree.h |    2 --
>  5 files changed, 29 insertions(+), 34 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 11182cd..e48a64b 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -333,6 +333,33 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
>      return fdt_totalsize(fdt);
>  }
>  
> +const char *boot_fdt_cmdline(const void *fdt)
> +{
> +    int node;
> +    const struct fdt_property *prop;
> +
> +    node = fdt_path_offset(fdt, "/chosen");
> +    if ( node < 0 )
> +        return NULL;
> +
> +    prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
> +    if ( prop == NULL )
> +    {
> +        struct bootmodule *dom0_mod = NULL;
> +
> +        if ( bootinfo.modules.nr_mods >= MOD_KERNEL )
> +            dom0_mod = &bootinfo.modules.module[MOD_KERNEL];
> +
> +        if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL) ||
> +            ( dom0_mod && dom0_mod->cmdline[0] ) )
> +            prop = fdt_get_property(fdt, node, "bootargs", NULL);
> +    }
> +    if ( prop == NULL )
> +        return NULL;
> +
> +    return prop->data;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 4a84a32..f1ae408 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -680,7 +680,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>          + (fdt_paddr & ((1 << SECOND_SHIFT) - 1));
>      fdt_size = boot_fdt_info(device_tree_flattened, fdt_paddr);
>  
> -    cmdline = device_tree_bootargs(device_tree_flattened);
> +    cmdline = boot_fdt_cmdline(device_tree_flattened);
>      printk("Command line: %s\n", cmdline);
>      cmdline_parse(cmdline);
>  
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 89de269..b9deb62 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -115,37 +115,6 @@ void dt_set_range(__be32 **cellp, const struct dt_device_node *np,
>      dt_set_cell(cellp, dt_n_size_cells(np), size);
>  }
>  
> -/**
> - * device_tree_bootargs - return the bootargs (the Xen command line)
> - * @fdt flat device tree.
> - */
> -const char *device_tree_bootargs(const void *fdt)
> -{
> -    int node;
> -    const struct fdt_property *prop;
> -
> -    node = fdt_path_offset(fdt, "/chosen");
> -    if ( node < 0 )
> -        return NULL;
> -
> -    prop = fdt_get_property(fdt, node, "xen,xen-bootargs", NULL);
> -    if ( prop == NULL )
> -    {
> -        struct bootmodule *dom0_mod = NULL;
> -
> -        if ( bootinfo.modules.nr_mods >= MOD_KERNEL )
> -            dom0_mod = &bootinfo.modules.module[MOD_KERNEL];
> -
> -        if (fdt_get_property(fdt, node, "xen,dom0-bootargs", NULL) ||
> -            ( dom0_mod && dom0_mod->cmdline[0] ) )
> -            prop = fdt_get_property(fdt, node, "bootargs", NULL);
> -    }
> -    if ( prop == NULL )
> -        return NULL;
> -
> -    return prop->data;
> -}
> -
>  static void __init *unflatten_dt_alloc(unsigned long *mem, unsigned long size,
>                                         unsigned long align)
>  {
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index 21dbcd4..85aa866 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -54,6 +54,7 @@ int construct_dom0(struct domain *d);
>  void discard_initial_modules(void);
>  
>  size_t __init boot_fdt_info(const void *fdt, paddr_t paddr);
> +const char __init *boot_fdt_cmdline(const void *fdt);
>  
>  #endif
>  /*
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 0edec85..b486fc6 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -157,8 +157,6 @@ typedef int (*device_tree_node_func)(const void *fdt,
>  
>  extern const void *device_tree_flattened;
>  
> -const char __init *device_tree_bootargs(const void *fdt);
> -
>  /**
>   * dt_unflatten_host_device_tree - Unflatten the host device tree
>   *
> -- 
> 1.7.10.4
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 07/10] xen: arm: store per-boot module type instead of relying on index
  2014-06-16 11:45 ` [PATCH 07/10] xen: arm: store per-boot module type instead of relying on index Ian Campbell
  2014-06-16 12:48   ` Julien Grall
@ 2014-06-18 14:18   ` Stefano Stabellini
  1 sibling, 0 replies; 35+ messages in thread
From: Stefano Stabellini @ 2014-06-18 14:18 UTC (permalink / raw)
  To: Ian Campbell
  Cc: stefano.stabellini, Naresh Bhat, julien.grall, tim, xen-devel,
	Roy Franz, Fu Wei

On Mon, 16 Jun 2014, Ian Campbell wrote:
>  void __init discard_initial_modules(void)
>  {
>      struct bootmodules *mi = &bootinfo.modules;
>      int i;
>  
> -    for ( i = MOD_DISCARD_FIRST; i <= mi->nr_mods; i++ )
> +    for ( i = 0; i <= mi->nr_mods; i++ )
>      {
>          paddr_t s = mi->module[i].start;
>          paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
>  
> -        dt_unreserved_regions(s, e, init_domheap_pages, 0);
> +        if ( mi->module[i].kind > BOOTMOD_LAST_PRESERVE )
> +            dt_unreserved_regions(s, e, init_domheap_pages, 0);
>      }

Given that we have a nice enum now, it seems like a step back to me
relying on kind > BOOTMOD_LAST_PRESERVE.
I would get rid of BOOTMOD_LAST_PRESERVE and instead check specifically
for BOOTMOD_XEN and BOOTMOD_FDT, that are the only ones we want to
preserve.

I admit that it is more a matter of taste than anything else.

The rest is fine.


>      mi->nr_mods = 0;
> @@ -360,9 +399,7 @@ static paddr_t __init get_xen_paddr(void)
>      printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
>             paddr, paddr + min_size);
>  
> -    bootinfo.modules.module[MOD_XEN].start = paddr;
> -    bootinfo.modules.module[MOD_XEN].size = min_size;
> -
> +    add_boot_module(BOOTMOD_XEN, paddr, min_size, NULL);
>      return paddr;
>  }
>  
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index 85aa866..57c98cb 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -5,14 +5,19 @@
>  
>  #define NR_MEM_BANKS 8
>  
> -#define MOD_XEN    0
> -#define MOD_FDT    1
> -#define MOD_KERNEL 2
> -#define MOD_INITRD 3
> -#define MOD_XSM    4
> -#define NR_MODULES 5
> +#define MAX_MODULES 5 /* Current maximum useful modules */
> +
> +typedef enum {
> +    BOOTMOD_XEN,
> +    BOOTMOD_FDT,
> +    /* Everything up to here is not freed after start of day */
> +    BOOTMOD_LAST_PRESERVE = BOOTMOD_FDT,
> +    BOOTMOD_KERNEL,
> +    BOOTMOD_RAMDISK,
> +    BOOTMOD_XSM,
> +    BOOTMOD_UNKNOWN
> +}  bootmodulekind;
>  
> -#define MOD_DISCARD_FIRST MOD_FDT
>  
>  struct membank {
>      paddr_t start;
> @@ -24,16 +29,18 @@ struct meminfo {
>      struct membank bank[NR_MEM_BANKS];
>  };
>  
> +#define BOOTMOD_MAX_CMDLINE 1024
>  struct bootmodule {
> +    bootmodulekind kind;
>      paddr_t start;
>      paddr_t size;
> -    char cmdline[1024];
> +    char cmdline[BOOTMOD_MAX_CMDLINE];
>  };
>  
>  struct bootmodules {
>      int nr_mods;
>      /* Module 0 is Xen itself, followed by the provided modules-proper */
> -    struct bootmodule module[NR_MODULES];
> +    struct bootmodule module[MAX_MODULES];
>  };
>  
>  struct bootinfo {
> @@ -56,6 +63,10 @@ void discard_initial_modules(void);
>  size_t __init boot_fdt_info(const void *fdt, paddr_t paddr);
>  const char __init *boot_fdt_cmdline(const void *fdt);
>  
> +void add_boot_module(bootmodulekind kind, paddr_t start, paddr_t size,
> +                     const char *cmdline);
> +struct bootmodule *boot_module_find_by_kind(bootmodulekind kind);
> +
>  #endif
>  /*
>   * Local variables:
> -- 
> 1.7.10.4
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 08/10] xen: arm: support bootmodule type detection by ordering
  2014-06-16 11:45 ` [PATCH 08/10] xen: arm: support bootmodule type detection by ordering Ian Campbell
@ 2014-06-18 14:40   ` Stefano Stabellini
  2014-06-18 14:45     ` Ian Campbell
  0 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2014-06-18 14:40 UTC (permalink / raw)
  To: Ian Campbell
  Cc: stefano.stabellini, Naresh Bhat, julien.grall, tim, xen-devel,
	Roy Franz, Fu Wei

On Mon, 16 Jun 2014, Ian Campbell wrote:
> Assign modules types based on the order in which they are defined in the FDT.
> This is supported only for the dom0 kernel and ramdisk when given as the first
> and second modules respectively, similar to how
> http://wiki.xen.org/wiki?title=Xen_ARM_with_Virtualization_Extensions/Multiboot&oldid=11824
> defined the default types from the bootloader side.
> 
> This is compatible with how Xen interprets the modules with x86 multiboot and I
> think simplifies things for bootloaders which now need not contain similar
> guessing code if they only care about the most basic case.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/bootfdt.c      |   10 +++++++---
>  xen/arch/arm/setup.c        |   14 ++++++++++++++
>  xen/include/asm-arm/setup.h |   11 ++++++++++-
>  3 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index e983aa7..a80055c 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -163,6 +163,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
>                                            const char *name,
>                                            u32 address_cells, u32 size_cells)
>  {
> +    static bootmodulekind kind_guess = BOOTMOD_LAST_PRESERVE + 1;

kind_guess = BOOTMOD_KERNEL would be clearer


>      const struct fdt_property *prop;
>      const __be32 *cell;
>      bootmodulekind kind;
> @@ -178,8 +179,10 @@ static void __init process_multiboot_node(const void *fdt, int node,
>          kind = BOOTMOD_RAMDISK;
>      else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
>          kind = BOOTMOD_XSM;
> +    else if ( kind_guess < BOOTMOD_UNKNOWN )
> +        kind = kind_guess++;

This would allow for BOOTMOD_XSM to be specified this way. Do we want
that? If so, we should write to the commit message and to the wiki.
Otherwise I would prefer:

else if ( kind_guess == BOOTMOD_KERNEL || kind_guess == BOOTMOD_RAMDISK )
{
    kind = kind_guess;
    kind_guess++;
}


>      else
> -        panic("%s not a known xen multiboot type\n", name);
> +        kind = BOOTMOD_UNKNOWN;
>  
>      prop = fdt_get_property(fdt, node, "reg", &len);
>      if ( !prop )
> @@ -278,11 +281,12 @@ static void __init early_print_info(void)
>                       mi->bank[i].start,
>                       mi->bank[i].start + mi->bank[i].size - 1);
>      printk("\n");
> -    for ( i = 1 ; i < mods->nr_mods; i++ )
> -        printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %s\n",
> +    for ( i = 0 ; i < mods->nr_mods; i++ )
> +        printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %-8s %s\n",
>                       i,
>                       mods->module[i].start,
>                       mods->module[i].start + mods->module[i].size,
> +                     boot_module_kind_as_string(mods->module[i].kind),
>                       mods->module[i].cmdline);
>      nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
>      for ( i = 0; i < nr_rsvd; i++ )
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 4c9dd3d..d358d04 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -221,6 +221,20 @@ struct bootmodule * __init boot_module_find_by_kind(bootmodulekind kind)
>      return NULL;
>  }
>  
> +const char * __init boot_module_kind_as_string(bootmodulekind kind)
> +{
> +    switch ( kind )
> +    {
> +    case BOOTMOD_XEN:     return "Xen";
> +    case BOOTMOD_FDT:     return "FDT";
> +    case BOOTMOD_KERNEL:  return "Kernel";
> +    case BOOTMOD_RAMDISK: return "Ramdisk";
> +    case BOOTMOD_XSM:     return "XSM";
> +    case BOOTMOD_UNKNOWN: return "Unknown";
> +    default: BUG();
> +    }
> +}
> +
>  void __init discard_initial_modules(void)
>  {
>      struct bootmodules *mi = &bootinfo.modules;
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index 57c98cb..f1a27fb 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -10,12 +10,20 @@
>  typedef enum {
>      BOOTMOD_XEN,
>      BOOTMOD_FDT,
> +
>      /* Everything up to here is not freed after start of day */
>      BOOTMOD_LAST_PRESERVE = BOOTMOD_FDT,
> +
> +    /*
> +     * Default module types. For modules which are not given an
> +     * explict type these are automatically used, in this order.
> +     */
>      BOOTMOD_KERNEL,
>      BOOTMOD_RAMDISK,
> +    BOOTMOD_UNKNOWN,
> +
> +    /* The remaining module types are never automatically assigned. */
>      BOOTMOD_XSM,
> -    BOOTMOD_UNKNOWN
>  }  bootmodulekind;
>  
>  
> @@ -66,6 +74,7 @@ const char __init *boot_fdt_cmdline(const void *fdt);
>  void add_boot_module(bootmodulekind kind, paddr_t start, paddr_t size,
>                       const char *cmdline);
>  struct bootmodule *boot_module_find_by_kind(bootmodulekind kind);
> +const char * __init boot_module_kind_as_string(bootmodulekind kind);
>  
>  #endif
>  /*
> -- 
> 1.7.10.4
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 09/10] xen: arm: Drop device_tree_node_compatible
  2014-06-16 11:45 ` [PATCH 09/10] xen: arm: Drop device_tree_node_compatible Ian Campbell
@ 2014-06-18 14:43   ` Stefano Stabellini
  2014-06-18 14:47     ` Ian Campbell
  0 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2014-06-18 14:43 UTC (permalink / raw)
  To: Ian Campbell
  Cc: stefano.stabellini, Naresh Bhat, julien.grall, tim, xen-devel,
	Roy Franz, Fu Wei

On Mon, 16 Jun 2014, Ian Campbell wrote:
> Instead use fdt_node_check_compatible from libfdt.

Unfortunately the two functions are not equivalent:
fdt_node_check_compatible uses memcmp while device_tree_node_compatible
uses strcasecmp that ignores cases.

At the very least we should make a note of this in the commit message.



> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/bootfdt.c |   28 ++--------------------------
>  1 file changed, 2 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index a80055c..8ab45c9 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -31,30 +31,6 @@ static bool_t __init device_tree_node_matches(const void *fdt, int node,
>          && (name[match_len] == '@' || name[match_len] == '\0');
>  }
>  
> -static bool_t __init device_tree_node_compatible(const void *fdt, int node,
> -                                                 const char *match)
> -{
> -    int len, l;
> -    int mlen;
> -    const void *prop;
> -
> -    mlen = strlen(match);
> -
> -    prop = fdt_getprop(fdt, node, "compatible", &len);
> -    if ( prop == NULL )
> -        return 0;
> -
> -    while ( len > 0 ) {
> -        if ( !dt_compat_cmp(prop, match) )
> -            return 1;
> -        l = strlen(prop) + 1;
> -        prop += l;
> -        len -= l;
> -    }
> -
> -    return 0;
> -}
> -
>  static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
>                                         u32 size_cells, u64 *start, u64 *size)
>  {
> @@ -261,8 +237,8 @@ static int __init early_scan_node(const void *fdt,
>  {
>      if ( device_tree_node_matches(fdt, node, "memory") )
>          process_memory_node(fdt, node, name, address_cells, size_cells);
> -    else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
> -              device_tree_node_compatible(fdt, node, "multiboot,module" ))
> +    else if ( fdt_node_check_compatible(fdt, node, "xen,multiboot-module" ) ||
> +              fdt_node_check_compatible(fdt, node, "multiboot,module" ))
>          process_multiboot_node(fdt, node, name, address_cells, size_cells);
>      else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
>          process_chosen_node(fdt, node, name, address_cells, size_cells);
> -- 
> 1.7.10.4
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 08/10] xen: arm: support bootmodule type detection by ordering
  2014-06-18 14:40   ` Stefano Stabellini
@ 2014-06-18 14:45     ` Ian Campbell
  2014-06-18 15:19       ` Stefano Stabellini
  0 siblings, 1 reply; 35+ messages in thread
From: Ian Campbell @ 2014-06-18 14:45 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Naresh Bhat, julien.grall, tim, xen-devel, Roy Franz, Fu Wei

On Wed, 2014-06-18 at 15:40 +0100, Stefano Stabellini wrote:
> On Mon, 16 Jun 2014, Ian Campbell wrote:
> > Assign modules types based on the order in which they are defined in the FDT.
> > This is supported only for the dom0 kernel and ramdisk when given as the first
> > and second modules respectively, similar to how
> > http://wiki.xen.org/wiki?title=Xen_ARM_with_Virtualization_Extensions/Multiboot&oldid=11824
> > defined the default types from the bootloader side.
> > 
> > This is compatible with how Xen interprets the modules with x86 multiboot and I
> > think simplifies things for bootloaders which now need not contain similar
> > guessing code if they only care about the most basic case.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/bootfdt.c      |   10 +++++++---
> >  xen/arch/arm/setup.c        |   14 ++++++++++++++
> >  xen/include/asm-arm/setup.h |   11 ++++++++++-
> >  3 files changed, 31 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index e983aa7..a80055c 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -163,6 +163,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
> >                                            const char *name,
> >                                            u32 address_cells, u32 size_cells)
> >  {
> > +    static bootmodulekind kind_guess = BOOTMOD_LAST_PRESERVE + 1;
> 
> kind_guess = BOOTMOD_KERNEL would be clearer

Yeah.

> >      const struct fdt_property *prop;
> >      const __be32 *cell;
> >      bootmodulekind kind;
> > @@ -178,8 +179,10 @@ static void __init process_multiboot_node(const void *fdt, int node,
> >          kind = BOOTMOD_RAMDISK;
> >      else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
> >          kind = BOOTMOD_XSM;
> > +    else if ( kind_guess < BOOTMOD_UNKNOWN )
> > +        kind = kind_guess++;
> 
> This would allow for BOOTMOD_XSM to be specified this way. Do we want
> that?

No, explicitly not. And we don't I think because BOOTMOD_XSM >=
BOOTMOD_UNKNOWN (for exactly this reason, see comment in the enum defn)

>  If so, we should write to the commit message and to the wiki.
> Otherwise I would prefer:

Really?

> else if ( kind_guess == BOOTMOD_KERNEL || kind_guess == BOOTMOD_RAMDISK )
> {
>     kind = kind_guess;
>     kind_guess++;
> }

Ian

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 09/10] xen: arm: Drop device_tree_node_compatible
  2014-06-18 14:43   ` Stefano Stabellini
@ 2014-06-18 14:47     ` Ian Campbell
  2014-06-18 14:57       ` Julien Grall
  0 siblings, 1 reply; 35+ messages in thread
From: Ian Campbell @ 2014-06-18 14:47 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Naresh Bhat, julien.grall, tim, xen-devel, Roy Franz, Fu Wei

On Wed, 2014-06-18 at 15:43 +0100, Stefano Stabellini wrote:
> On Mon, 16 Jun 2014, Ian Campbell wrote:
> > Instead use fdt_node_check_compatible from libfdt.
> 
> Unfortunately the two functions are not equivalent:
> fdt_node_check_compatible uses memcmp while device_tree_node_compatible
> uses strcasecmp that ignores cases.

I hadn't spotted this.

They can't both be spec complaint I think. I expect
fdt_node_check_compatible (from libfdt) is more likely to be the one
which is correct.

> At the very least we should make a note of this in the commit message.

Will do.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 10/10] xen: arm: update multiboot device tree bindings.
  2014-06-16 11:45 ` [PATCH 10/10] xen: arm: update multiboot device tree bindings Ian Campbell
@ 2014-06-18 14:56   ` Stefano Stabellini
  2014-06-18 15:17     ` Ian Campbell
  0 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2014-06-18 14:56 UTC (permalink / raw)
  To: Ian Campbell
  Cc: stefano.stabellini, Naresh Bhat, julien.grall, tim, xen-devel,
	Roy Franz, Fu Wei

On Mon, 16 Jun 2014, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  docs/misc/arm/device-tree/booting.txt |   24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index bfb8d01..92af119 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -8,15 +8,27 @@ Each node contains the following properties:
>  
>  - compatible
>  
> -	Must be:
> +	Must always include at least:
>  
> -		"xen,<type>", "xen,multiboot-module"
> +		"multiboot,module"

As it stands this conflicts with the statement below that we are
compatible with Xen 4.4: the old multiboot compatible strings didn't
include "multiboot,module".
You might want to reword it.


> -	where <type> must be one of:
> +	Optionally a more specific compatible string may be used in
> +	addition to the above. One of:

We should probably recommend the usage of a more specific string in
addition to "multiboot,module".


> -	- "linux-zimage" -- the dom0 kernel
> -	- "linux-initrd" -- the dom0 ramdisk
> -	- "xsm-policy"	 -- XSM policy blob
> +	- "multiboot,kernel"	-- the dom0 kernel
> +	- "multiboot,ramdisk"	-- the dom0 ramdisk
> +	- "xen,xsm-policy"	-- XSM policy blob
> +
> +	For compatibility with Xen 4.4 the following are also
> +	supported:
> +
> +		"xen,multiboot-module" equivalent to "multiboot,module"
> +		"xen,linux-zimage" equivalent to "multiboot,kernel"
> +		"xen,linux-initrd" equivalent to "multiboot,ramdisk"
> +
> +	Xen 4.4 does not know the "multiboot,*" compatibility
> +	strings. Also when using Xen 4.4 the more specific
> +	"xen,linux-*" names are non-optional.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 09/10] xen: arm: Drop device_tree_node_compatible
  2014-06-18 14:47     ` Ian Campbell
@ 2014-06-18 14:57       ` Julien Grall
  2014-06-18 14:59         ` Stefano Stabellini
  2014-06-18 15:20         ` Ian Campbell
  0 siblings, 2 replies; 35+ messages in thread
From: Julien Grall @ 2014-06-18 14:57 UTC (permalink / raw)
  To: Ian Campbell, Stefano Stabellini
  Cc: Roy Franz, Naresh Bhat, tim, Fu Wei, xen-devel

On 06/18/2014 03:47 PM, Ian Campbell wrote:
> On Wed, 2014-06-18 at 15:43 +0100, Stefano Stabellini wrote:
>> On Mon, 16 Jun 2014, Ian Campbell wrote:
>>> Instead use fdt_node_check_compatible from libfdt.
>>
>> Unfortunately the two functions are not equivalent:
>> fdt_node_check_compatible uses memcmp while device_tree_node_compatible
>> uses strcasecmp that ignores cases.
> 
> I hadn't spotted this.
> 
> They can't both be spec complaint I think. I expect
> fdt_node_check_compatible (from libfdt) is more likely to be the one
> which is correct.

In dt_device_is_compatible we use strcasecmp as Linux does.

For Linux, it looks like the use a less restrictive way in generic code
(i.e strcasecmp) and more restrictive on SPARC (i.e strcmp)

I can't find anything in the spec to say which one is better. But I
think we want to stay compatible with Linux we have to keep
device_tree_node_compatible and use it in place of
fdt_node_check_compatible.

We could also modify the latter function for our convenience.

Regards.

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 09/10] xen: arm: Drop device_tree_node_compatible
  2014-06-18 14:57       ` Julien Grall
@ 2014-06-18 14:59         ` Stefano Stabellini
  2014-06-18 15:20         ` Ian Campbell
  1 sibling, 0 replies; 35+ messages in thread
From: Stefano Stabellini @ 2014-06-18 14:59 UTC (permalink / raw)
  To: Julien Grall
  Cc: Ian Campbell, Stefano Stabellini, Naresh Bhat, tim, xen-devel,
	Roy Franz, Fu Wei

On Wed, 18 Jun 2014, Julien Grall wrote:
> On 06/18/2014 03:47 PM, Ian Campbell wrote:
> > On Wed, 2014-06-18 at 15:43 +0100, Stefano Stabellini wrote:
> >> On Mon, 16 Jun 2014, Ian Campbell wrote:
> >>> Instead use fdt_node_check_compatible from libfdt.
> >>
> >> Unfortunately the two functions are not equivalent:
> >> fdt_node_check_compatible uses memcmp while device_tree_node_compatible
> >> uses strcasecmp that ignores cases.
> > 
> > I hadn't spotted this.
> > 
> > They can't both be spec complaint I think. I expect
> > fdt_node_check_compatible (from libfdt) is more likely to be the one
> > which is correct.
> 
> In dt_device_is_compatible we use strcasecmp as Linux does.
> 
> For Linux, it looks like the use a less restrictive way in generic code
> (i.e strcasecmp) and more restrictive on SPARC (i.e strcmp)
> 
> I can't find anything in the spec to say which one is better. But I
> think we want to stay compatible with Linux we have to keep
> device_tree_node_compatible and use it in place of
> fdt_node_check_compatible.
> 
> We could also modify the latter function for our convenience.

Who wants to open Pandora's box and send an email to the LKML?

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 10/10] xen: arm: update multiboot device tree bindings.
  2014-06-18 14:56   ` Stefano Stabellini
@ 2014-06-18 15:17     ` Ian Campbell
  2014-06-18 15:22       ` Stefano Stabellini
  0 siblings, 1 reply; 35+ messages in thread
From: Ian Campbell @ 2014-06-18 15:17 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Naresh Bhat, julien.grall, tim, xen-devel, Roy Franz, Fu Wei

On Wed, 2014-06-18 at 15:56 +0100, Stefano Stabellini wrote:
> On Mon, 16 Jun 2014, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  docs/misc/arm/device-tree/booting.txt |   24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> > index bfb8d01..92af119 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -8,15 +8,27 @@ Each node contains the following properties:
> >  
> >  - compatible
> >  
> > -	Must be:
> > +	Must always include at least:
> >  
> > -		"xen,<type>", "xen,multiboot-module"
> > +		"multiboot,module"
> 
> As it stands this conflicts with the statement below that we are
> compatible with Xen 4.4: the old multiboot compatible strings didn't
> include "multiboot,module".
> You might want to reword it.

I'm not sure what you mean or I would. 

I don't think the doc says that we are compatible with Xen 4.4, it says
that in order to be compatible with Xen 4.4 the user should... Is that
what I need to clarify perhaps?

> > -	where <type> must be one of:
> > +	Optionally a more specific compatible string may be used in
> > +	addition to the above. One of:
> 
> We should probably recommend the usage of a more specific string in
> addition to "multiboot,module".

"Normally a more specific compatible string should be used in addition
to the above" perhaps?

At some point I'd like to see us doing more probing of magic numbers
(e.g. x86 does this for the XSM policy).

Ian.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 08/10] xen: arm: support bootmodule type detection by ordering
  2014-06-18 14:45     ` Ian Campbell
@ 2014-06-18 15:19       ` Stefano Stabellini
  0 siblings, 0 replies; 35+ messages in thread
From: Stefano Stabellini @ 2014-06-18 15:19 UTC (permalink / raw)
  To: Ian Campbell
  Cc: Stefano Stabellini, Naresh Bhat, julien.grall, tim, xen-devel,
	Roy Franz, Fu Wei

On Wed, 18 Jun 2014, Ian Campbell wrote:
> On Wed, 2014-06-18 at 15:40 +0100, Stefano Stabellini wrote:
> > On Mon, 16 Jun 2014, Ian Campbell wrote:
> > > Assign modules types based on the order in which they are defined in the FDT.
> > > This is supported only for the dom0 kernel and ramdisk when given as the first
> > > and second modules respectively, similar to how
> > > http://wiki.xen.org/wiki?title=Xen_ARM_with_Virtualization_Extensions/Multiboot&oldid=11824
> > > defined the default types from the bootloader side.
> > > 
> > > This is compatible with how Xen interprets the modules with x86 multiboot and I
> > > think simplifies things for bootloaders which now need not contain similar
> > > guessing code if they only care about the most basic case.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > >  xen/arch/arm/bootfdt.c      |   10 +++++++---
> > >  xen/arch/arm/setup.c        |   14 ++++++++++++++
> > >  xen/include/asm-arm/setup.h |   11 ++++++++++-
> > >  3 files changed, 31 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > > index e983aa7..a80055c 100644
> > > --- a/xen/arch/arm/bootfdt.c
> > > +++ b/xen/arch/arm/bootfdt.c
> > > @@ -163,6 +163,7 @@ static void __init process_multiboot_node(const void *fdt, int node,
> > >                                            const char *name,
> > >                                            u32 address_cells, u32 size_cells)
> > >  {
> > > +    static bootmodulekind kind_guess = BOOTMOD_LAST_PRESERVE + 1;
> > 
> > kind_guess = BOOTMOD_KERNEL would be clearer
> 
> Yeah.
> 
> > >      const struct fdt_property *prop;
> > >      const __be32 *cell;
> > >      bootmodulekind kind;
> > > @@ -178,8 +179,10 @@ static void __init process_multiboot_node(const void *fdt, int node,
> > >          kind = BOOTMOD_RAMDISK;
> > >      else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
> > >          kind = BOOTMOD_XSM;
> > > +    else if ( kind_guess < BOOTMOD_UNKNOWN )
> > > +        kind = kind_guess++;
> > 
> > This would allow for BOOTMOD_XSM to be specified this way. Do we want
> > that?
> 
> No, explicitly not. And we don't I think because BOOTMOD_XSM >=
> BOOTMOD_UNKNOWN (for exactly this reason, see comment in the enum defn)

I didn't spot that you moved BOOTMOD_UNKNOWN before BOOTMOD_XSM in this
patch. I would rather avoid it for the principle of least astonishment.
I would expect the "unknown" type to be the last one in the enum.


> >  If so, we should write to the commit message and to the wiki.
> > Otherwise I would prefer:
> 
> Really?

The point is trying to avoid relying on the integer ordering in the
enum, otherwise we loose the benefits of using an enum instead of the
old macros.


> > else if ( kind_guess == BOOTMOD_KERNEL || kind_guess == BOOTMOD_RAMDISK )
> > {
> >     kind = kind_guess;
> >     kind_guess++;
> > }
> 
> Ian
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 09/10] xen: arm: Drop device_tree_node_compatible
  2014-06-18 14:57       ` Julien Grall
  2014-06-18 14:59         ` Stefano Stabellini
@ 2014-06-18 15:20         ` Ian Campbell
  1 sibling, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-06-18 15:20 UTC (permalink / raw)
  To: Julien Grall
  Cc: Stefano Stabellini, Naresh Bhat, tim, xen-devel, Roy Franz, Fu Wei

On Wed, 2014-06-18 at 15:57 +0100, Julien Grall wrote:
> On 06/18/2014 03:47 PM, Ian Campbell wrote:
> > On Wed, 2014-06-18 at 15:43 +0100, Stefano Stabellini wrote:
> >> On Mon, 16 Jun 2014, Ian Campbell wrote:
> >>> Instead use fdt_node_check_compatible from libfdt.
> >>
> >> Unfortunately the two functions are not equivalent:
> >> fdt_node_check_compatible uses memcmp while device_tree_node_compatible
> >> uses strcasecmp that ignores cases.
> > 
> > I hadn't spotted this.
> > 
> > They can't both be spec complaint I think. I expect
> > fdt_node_check_compatible (from libfdt) is more likely to be the one
> > which is correct.
> 
> In dt_device_is_compatible we use strcasecmp as Linux does.
> 
> For Linux, it looks like the use a less restrictive way in generic code
> (i.e strcasecmp) and more restrictive on SPARC (i.e strcmp)
> 
> I can't find anything in the spec to say which one is better. But I
> think we want to stay compatible with Linux we have to keep
> device_tree_node_compatible and use it in place of
> fdt_node_check_compatible.

Urk, what a mess.

This patch was pretty tangential to the series, I'll just drop it.

Ian.


> We could also modify the latter function for our convenience.
> 
> Regards.
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 10/10] xen: arm: update multiboot device tree bindings.
  2014-06-18 15:17     ` Ian Campbell
@ 2014-06-18 15:22       ` Stefano Stabellini
  2014-06-18 15:24         ` Ian Campbell
  0 siblings, 1 reply; 35+ messages in thread
From: Stefano Stabellini @ 2014-06-18 15:22 UTC (permalink / raw)
  To: Ian Campbell
  Cc: Stefano Stabellini, Naresh Bhat, julien.grall, tim, xen-devel,
	Roy Franz, Fu Wei

On Wed, 18 Jun 2014, Ian Campbell wrote:
> On Wed, 2014-06-18 at 15:56 +0100, Stefano Stabellini wrote:
> > On Mon, 16 Jun 2014, Ian Campbell wrote:
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > >  docs/misc/arm/device-tree/booting.txt |   24 ++++++++++++++++++------
> > >  1 file changed, 18 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> > > index bfb8d01..92af119 100644
> > > --- a/docs/misc/arm/device-tree/booting.txt
> > > +++ b/docs/misc/arm/device-tree/booting.txt
> > > @@ -8,15 +8,27 @@ Each node contains the following properties:
> > >  
> > >  - compatible
> > >  
> > > -	Must be:
> > > +	Must always include at least:
> > >  
> > > -		"xen,<type>", "xen,multiboot-module"
> > > +		"multiboot,module"
> > 
> > As it stands this conflicts with the statement below that we are
> > compatible with Xen 4.4: the old multiboot compatible strings didn't
> > include "multiboot,module".
> > You might want to reword it.
> 
> I'm not sure what you mean or I would. 
> 
> I don't think the doc says that we are compatible with Xen 4.4, it says
> that in order to be compatible with Xen 4.4 the user should... Is that
> what I need to clarify perhaps?

The doc says "For compatibility with Xen 4.4 the following are also
supported". From that I assumed that we are compatible with Xen 4.4....


> > > -	where <type> must be one of:
> > > +	Optionally a more specific compatible string may be used in
> > > +	addition to the above. One of:
> > 
> > We should probably recommend the usage of a more specific string in
> > addition to "multiboot,module".
> 
> "Normally a more specific compatible string should be used in addition
> to the above" perhaps?

That's good. Or maybe "A more specific compatible string can and should
be used in addition to the above".


> At some point I'd like to see us doing more probing of magic numbers
> (e.g. x86 does this for the XSM policy).

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 10/10] xen: arm: update multiboot device tree bindings.
  2014-06-18 15:22       ` Stefano Stabellini
@ 2014-06-18 15:24         ` Ian Campbell
  0 siblings, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-06-18 15:24 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Naresh Bhat, julien.grall, tim, xen-devel, Roy Franz, Fu Wei

On Wed, 2014-06-18 at 16:22 +0100, Stefano Stabellini wrote:
> On Wed, 18 Jun 2014, Ian Campbell wrote:
> > On Wed, 2014-06-18 at 15:56 +0100, Stefano Stabellini wrote:
> > > On Mon, 16 Jun 2014, Ian Campbell wrote:
> > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > > ---
> > > >  docs/misc/arm/device-tree/booting.txt |   24 ++++++++++++++++++------
> > > >  1 file changed, 18 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> > > > index bfb8d01..92af119 100644
> > > > --- a/docs/misc/arm/device-tree/booting.txt
> > > > +++ b/docs/misc/arm/device-tree/booting.txt
> > > > @@ -8,15 +8,27 @@ Each node contains the following properties:
> > > >  
> > > >  - compatible
> > > >  
> > > > -	Must be:
> > > > +	Must always include at least:
> > > >  
> > > > -		"xen,<type>", "xen,multiboot-module"
> > > > +		"multiboot,module"
> > > 
> > > As it stands this conflicts with the statement below that we are
> > > compatible with Xen 4.4: the old multiboot compatible strings didn't
> > > include "multiboot,module".
> > > You might want to reword it.
> > 
> > I'm not sure what you mean or I would. 
> > 
> > I don't think the doc says that we are compatible with Xen 4.4, it says
> > that in order to be compatible with Xen 4.4 the user should... Is that
> > what I need to clarify perhaps?
> 
> The doc says "For compatibility with Xen 4.4 the following are also
> supported". From that I assumed that we are compatible with Xen 4.4....

This is saying "in addition to what is described above we also support
XXX for compatibility with a previous way of doing things".

I wouldn't describe that as conflicting.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support
  2014-06-16 11:44 [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Ian Campbell
                   ` (9 preceding siblings ...)
  2014-06-16 11:45 ` [PATCH 10/10] xen: arm: update multiboot device tree bindings Ian Campbell
@ 2014-07-15 18:22 ` Roy Franz
  2014-07-16  8:31   ` Ian Campbell
  10 siblings, 1 reply; 35+ messages in thread
From: Roy Franz @ 2014-07-15 18:22 UTC (permalink / raw)
  To: Ian Campbell
  Cc: Stefano Stabellini, Naresh Bhat, Julien Grall, Tim Deegan,
	xen-devel, Fu Wei

On Mon, Jun 16, 2014 at 4:44 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> The following refactors the Xen early device tree stuff (i.e. which
> walks the flattened tree directly) away from the regular device tree
> stuff (i.e. the stuff which for the most part deals with the unflattened
> tree). It also makes some changes to the Xen side multiboot support
> which I think will make it easier to work with, both internally and for
> e.g. bootloader integration.
>
> Impact on UEFI/ACPI: Mostly I think the refactoring may be useful when
> integrating the UEFI memory map and ACPI stuff (which wants early FDT,
> but not unflatening etc) in to Xen.
>
> Impact on multiboot: This could potentially simplify things on the grub
> side by removing the need to guess default types for the modules in the
> common case.
>
> In the future I think it would be good to implement more probing on the
> Xen side, e.g. to discover the XSM policy (similar to how it works on
> x86 -- which walks all the modules looking for the policy magic nr).
>
> The first two patches here have been posted before.
>
> Ian.
>

Hi Ian,

   Is this series expected to go in? (or at least the DTB binding updates?)
I'm reviewing the device tree bindings I'm using in the stub and want to use
the updated bindings if they are going in.

Thanks,
Roy

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support
  2014-07-15 18:22 ` [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Roy Franz
@ 2014-07-16  8:31   ` Ian Campbell
  0 siblings, 0 replies; 35+ messages in thread
From: Ian Campbell @ 2014-07-16  8:31 UTC (permalink / raw)
  To: Roy Franz
  Cc: Stefano Stabellini, Naresh Bhat, Julien Grall, Tim Deegan,
	xen-devel, Fu Wei

On Tue, 2014-07-15 at 11:22 -0700, Roy Franz wrote:
> On Mon, Jun 16, 2014 at 4:44 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > The following refactors the Xen early device tree stuff (i.e. which
> > walks the flattened tree directly) away from the regular device tree
> > stuff (i.e. the stuff which for the most part deals with the unflattened
> > tree). It also makes some changes to the Xen side multiboot support
> > which I think will make it easier to work with, both internally and for
> > e.g. bootloader integration.
> >
> > Impact on UEFI/ACPI: Mostly I think the refactoring may be useful when
> > integrating the UEFI memory map and ACPI stuff (which wants early FDT,
> > but not unflatening etc) in to Xen.
> >
> > Impact on multiboot: This could potentially simplify things on the grub
> > side by removing the need to guess default types for the modules in the
> > common case.
> >
> > In the future I think it would be good to implement more probing on the
> > Xen side, e.g. to discover the XSM policy (similar to how it works on
> > x86 -- which walks all the modules looking for the policy magic nr).
> >
> > The first two patches here have been posted before.
> >
> > Ian.
> >
> 
> Hi Ian,
> 
>    Is this series expected to go in? (or at least the DTB binding updates?)

I still need to rebase and address a few comments/issues, but it should
be going in. I just keep getting preempted by other stuff. I'm hoping to
get back to it tomorrow or Friday.

> I'm reviewing the device tree bindings I'm using in the stub and want to use
> the updated bindings if they are going in.

Ian.

^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2014-07-16  8:31 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-16 11:44 [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Ian Campbell
2014-06-16 11:44 ` [PATCH 01/10] xen: arm: implement generic multiboot compatibility strings Ian Campbell
2014-06-16 11:44 ` [PATCH 02/10] xen: arm: /chosen/module@N/bootargs bootprotcol node is not deprecated Ian Campbell
2014-06-16 11:44 ` [PATCH 03/10] xen: arm: prefer typesafe max()/min() over MAX()/MIN() Ian Campbell
2014-06-16 12:08   ` Julien Grall
2014-06-16 11:44 ` [PATCH 04/10] xen: arm: rename early_info structs Ian Campbell
2014-06-16 12:10   ` Julien Grall
2014-06-18 13:56   ` Stefano Stabellini
2014-06-16 11:44 ` [PATCH 05/10] xen: arm: move boot time fdt parsing into separate file Ian Campbell
2014-06-16 12:13   ` Julien Grall
2014-06-18 14:00   ` Stefano Stabellini
2014-06-16 11:44 ` [PATCH 06/10] xen: arm: device_tree_bootargs to bootfdt.c, renaming to boot_fdt_cmdline Ian Campbell
2014-06-16 12:32   ` Julien Grall
2014-06-18 14:02   ` Stefano Stabellini
2014-06-16 11:45 ` [PATCH 07/10] xen: arm: store per-boot module type instead of relying on index Ian Campbell
2014-06-16 12:48   ` Julien Grall
2014-06-16 12:52     ` Ian Campbell
2014-06-18 14:18   ` Stefano Stabellini
2014-06-16 11:45 ` [PATCH 08/10] xen: arm: support bootmodule type detection by ordering Ian Campbell
2014-06-18 14:40   ` Stefano Stabellini
2014-06-18 14:45     ` Ian Campbell
2014-06-18 15:19       ` Stefano Stabellini
2014-06-16 11:45 ` [PATCH 09/10] xen: arm: Drop device_tree_node_compatible Ian Campbell
2014-06-18 14:43   ` Stefano Stabellini
2014-06-18 14:47     ` Ian Campbell
2014-06-18 14:57       ` Julien Grall
2014-06-18 14:59         ` Stefano Stabellini
2014-06-18 15:20         ` Ian Campbell
2014-06-16 11:45 ` [PATCH 10/10] xen: arm: update multiboot device tree bindings Ian Campbell
2014-06-18 14:56   ` Stefano Stabellini
2014-06-18 15:17     ` Ian Campbell
2014-06-18 15:22       ` Stefano Stabellini
2014-06-18 15:24         ` Ian Campbell
2014-07-15 18:22 ` [PATCH 00/10] xen: arm: Refactor/improve early DT parsing and multiboot module support Roy Franz
2014-07-16  8:31   ` Ian Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.