[PATCH V2 00/12] arm: support for initial modules (e.g. dom0) and DTB supplied in RAM

[PATCH V2 00/12] arm: support for initial modules (e.g. dom0) and DTB supplied in RAM

[Ian Campbell]

The following series implements support for initial images and DTB in
RAM, as opposed to in flash (dom0 kernel) or compiled into the
hypervisor (DTB). It arranges to not clobber these with either the h/v
text on relocation or with the heaps and frees them as appropriate.

Most of this is independent of the specific bootloader protocol which is
used to tell Xen where these modules actually are, but I have included a
simple PoC bootloader protocol based around device tree which is similar
to the protocol used by Linux to find its initrd
(where /chosen/linux,initrd-{start,end} indicate the physical address).

The PoC protocol is documented in docs/misc/arm/device-tree/booting.txt
which is added by this series.

I will post a patch against the boot-wrapper which implements the
"bootloader" side of this protocol shortly. With that you can boot using
the semi-hosting feature of the model (paths are host paths) like so:
        $MODEL linux-system-semi.axf -C cluster.cpu0.semihosting-cmd_line=\
                '--kernel xen-arm.bin \
                 --module zImage earlyprintk=xenboot console=ttyAMA1 root=/dev/mmcblk0 ro \
                 --dtb vexpress-v2p-aem-v7a-xen.dtb -- dom0_mem=256M'

Where xen-arm.bin, zImage and vexpress-v2p-aem-v7a-xen.dtb are all files
in your $CWD.

Ian.

[PATCH 01/12] arm: Enable build without CONFIG_DTB_FILE

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/Makefile  |    4 ----
 xen/arch/arm/xen.lds.S |    2 ++
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 634b620..bfac017 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -33,10 +33,6 @@ obj-y += hvm.o
 ifdef CONFIG_DTB_FILE
 obj-y += dtb.o
 AFLAGS += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
-else
-# XXX: When running on the model there is no bootloader to provide a
-# device tree.  It must be linked into Xen.
-$(error CONFIG_DTB_FILE must be set to the absolute filename of a DTB)
 endif
 
 ALL_OBJS := head.o $(ALL_OBJS)
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index f0f4cd3..410d7db 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -123,9 +123,11 @@ SECTIONS
   } :text
   _end = . ;
 
+#ifdef CONFIG_DTB_FILE
   /* Section for the device tree blob (if any). */
   _sdtb = .;
   .dtb : { *(.dtb) } :text
+#endif
 
   /* Sections to be discarded */
   /DISCARD/ : {
-- 
1.7.9.1

[PATCH 02/12] arm: create a raw binary target.

[Ian Campbell]

This is suitable for direct loading by a bootloader.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Don't strip the .comment section, we don't have one any way.
---
 xen/arch/arm/Makefile |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index bfac017..92a4ccf 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -37,12 +37,16 @@ endif
 
 ALL_OBJS := head.o $(ALL_OBJS)
 
-$(TARGET): $(TARGET)-syms
+$(TARGET): $(TARGET)-syms $(TARGET).bin
 	# XXX: VE model loads by VMA so instead of
 	# making a proper ELF we link with LMA == VMA and adjust crudely
 	$(OBJCOPY) --change-addresses +0x80000000 $< $@
 	$(STRIP) $@
 
+#
+$(TARGET).bin: $(TARGET)-syms
+	objcopy -O binary -S $< $@
+
 #$(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
 #	./boot/mkelf32 $(TARGET)-syms $(TARGET) 0x100000 \
 #	`$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'`
-- 
1.7.9.1

[PATCH 03/12] arm: handle xenheap which isn't at the start of RAM.

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Simplify page_to_virt by using mfn_to_virt & page_to_mfn as
    suggested by Tim.
---
 xen/include/asm-arm/mm.h |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index c0f5b1f..260af35 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -214,17 +214,15 @@ static inline struct page_info *virt_to_page(const void *v)
     ASSERT(va >= XENHEAP_VIRT_START);
     ASSERT(va < xenheap_virt_end);
 
-    return frame_table + ((va - XENHEAP_VIRT_START) >> PAGE_SHIFT);
+    return frame_table
+        + ((va - XENHEAP_VIRT_START) >> PAGE_SHIFT)
+        + xenheap_mfn_start
+        - frametable_base_mfn;
 }
 
 static inline void *page_to_virt(const struct page_info *pg)
 {
-    ASSERT((unsigned long)pg - FRAMETABLE_VIRT_START < frametable_virt_end);
-    return (void *)(XENHEAP_VIRT_START +
-                    ((unsigned long)pg - FRAMETABLE_VIRT_START) /
-                    (sizeof(*pg) / (sizeof(*pg) & -sizeof(*pg))) *
-                    (PAGE_SIZE / (sizeof(*pg) & -sizeof(*pg))));
-
+    return mfn_to_virt(page_to_mfn(pg));
 }
 
 struct domain *page_get_owner_and_reference(struct page_info *page);
-- 
1.7.9.1

[PATCH 04/12] arm: parse modules from DT during early boot.

[Ian Campbell]

The bootloader should populate /chosen/module@<N>/ for each module it
wishes to pass to the hypervisor. The content of these nodes is
described in docs/misc/arm/device-tree/booting.txt

The hypervisor allows for 2 modules (@1==kernel and @2==initrd).
Currently we don't do anything with them.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Reserve the zeroeth module for Xen itself (not used yet)
    Use a more idiomatic DT layout
    Document said layout
---
 docs/misc/arm/device-tree/booting.txt |   27 ++++++++++++
 xen/common/device_tree.c              |   75 +++++++++++++++++++++++++++++++++
 xen/include/xen/device_tree.h         |   14 ++++++
 3 files changed, 116 insertions(+), 0 deletions(-)
 create mode 100644 docs/misc/arm/device-tree/booting.txt

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
new file mode 100644
index 0000000..2609450
--- /dev/null
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -0,0 +1,27 @@
+Xen is passed the dom0 kernel and initrd via a reference in the /chosen
+node of the device tree.
+
+Each node has the form /chosen/module@<N> and contains the following
+properties:
+
+- compatible
+
+	Must be "xen,multiboot-module"
+
+- start
+
+	Physical address of the start of this module
+
+- end
+
+	Physical address of the end of this module
+
+- bootargs (optional)
+
+	Command line associated with this module
+
+The following modules are understood
+
+- 1 -- the domain 0 kernel
+- 2 -- the domain 0 ramdisk
+
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 3d1f0f4..efd1663 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -229,6 +229,79 @@ static void __init process_memory_node(const void *fdt, int node,
     }
 }
 
+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;
+    const u32 *cell;
+    paddr_t size;
+    int nr, depth, nr_modules = 0;
+    struct dt_mb_module *mod;
+    int len;
+
+    for ( depth = 0;
+          depth >= 0;
+          node = fdt_next_node(fdt, node, &depth) )
+    {
+        name = fdt_get_name(fdt, node, NULL);
+        if ( strncmp(name, "module@", strlen("module@")) == 0 ) {
+
+            if ( fdt_node_check_compatible(fdt, node,
+                                           "xen,multiboot-module" ) != 0 )
+                early_panic("%s not a compatible module node\n", name);
+
+            nr = simple_strtol(name + strlen("module@"), NULL, 10);
+            if ( nr <= 0 )
+                early_panic("Invalid module number %d\n", nr);
+
+            if ( nr > NR_MODULES )
+                early_panic("too many modules %d > %d\n", nr, NR_MODULES);
+            if ( nr > nr_modules )
+                nr_modules = nr;
+
+            mod = &early_info.modules.module[nr];
+
+            prop = fdt_get_property(fdt, node, "start", NULL);
+            if ( !prop )
+                early_panic("no start for module %d\n", nr);
+
+            cell = (const u32 *)prop->data;
+            device_tree_get_reg(&cell, address_cells, size_cells,
+                                &mod->start, &size);
+
+            prop = fdt_get_property(fdt, node, "end", NULL);
+            if ( !prop )
+                early_panic("no end for module %d\n", nr);
+
+            cell = (const u32 *)prop->data;
+            device_tree_get_reg(&cell, address_cells, size_cells,
+                                &mod->size, &size);
+            mod->size -= mod->start;
+
+            prop = fdt_get_property(fdt, node, "bootargs", &len);
+            if ( prop )
+            {
+                if ( len > sizeof(mod->cmdline) )
+                    early_panic("module %d command line too long\n", nr);
+
+                safe_strcpy(mod->cmdline, prop->data);
+            }
+            else
+                mod->cmdline[0] = 0;
+        }
+    }
+
+    for ( nr = 1 ; nr < nr_modules ; nr++ )
+    {
+        mod = &early_info.modules.module[nr];
+        if ( !mod->start || !mod->size )
+            early_panic("module %d  missing / invalid\n", nr);
+    }
+
+    early_info.modules.nr_mods = nr_modules;
+}
+
 static int __init early_scan_node(const void *fdt,
                                   int node, const char *name, int depth,
                                   u32 address_cells, u32 size_cells,
@@ -236,6 +309,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_matches(fdt, node, "chosen") )
+        process_chosen_node(fdt, node, name, address_cells, size_cells);
 
     return 0;
 }
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 4d010c0..c383677 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -15,6 +15,7 @@
 #define DEVICE_TREE_MAX_DEPTH 16
 
 #define NR_MEM_BANKS 8
+#define NR_MODULES 2
 
 struct membank {
     paddr_t start;
@@ -26,8 +27,21 @@ struct dt_mem_info {
     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 + 1];
+};
+
 struct dt_early_info {
     struct dt_mem_info mem;
+    struct dt_module_info modules;
 };
 
 typedef int (*device_tree_node_func)(const void *fdt,
-- 
1.7.9.1

Re: [PATCH 04/12] arm: parse modules from DT during early boot.

[Tim Deegan]

At 16:23 +0000 on 13 Nov (1352823796), Ian Campbell wrote:
> The bootloader should populate /chosen/module@<N>/ for each module it
> wishes to pass to the hypervisor. The content of these nodes is
> described in docs/misc/arm/device-tree/booting.txt
> 
> The hypervisor allows for 2 modules (@1==kernel and @2==initrd).
> Currently we don't do anything with them.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: Reserve the zeroeth module for Xen itself (not used yet)
>     Use a more idiomatic DT layout
>     Document said layout
> ---
>  docs/misc/arm/device-tree/booting.txt |   27 ++++++++++++
>  xen/common/device_tree.c              |   75 +++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h         |   14 ++++++
>  3 files changed, 116 insertions(+), 0 deletions(-)
>  create mode 100644 docs/misc/arm/device-tree/booting.txt
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> new file mode 100644
> index 0000000..2609450
> --- /dev/null
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -0,0 +1,27 @@
> +Xen is passed the dom0 kernel and initrd via a reference in the /chosen
> +node of the device tree.
> +
> +Each node has the form /chosen/module@<N> and contains the following
> +properties:
> +
> +- compatible
> +
> +	Must be "xen,multiboot-module"
> +
> +- start
> +
> +	Physical address of the start of this module
> +
> +- end
> +
> +	Physical address of the end of this module
> +
> +- bootargs (optional)
> +
> +	Command line associated with this module
> +
> +The following modules are understood
> +
> +- 1 -- the domain 0 kernel
> +- 2 -- the domain 0 ramdisk
> +
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 3d1f0f4..efd1663 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -229,6 +229,79 @@ static void __init process_memory_node(const void *fdt, int node,
>      }
>  }
>  
> +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;
> +    const u32 *cell;
> +    paddr_t size;
> +    int nr, depth, nr_modules = 0;
> +    struct dt_mb_module *mod;
> +    int len;
> +
> +    for ( depth = 0;
> +          depth >= 0;
> +          node = fdt_next_node(fdt, node, &depth) )
> +    {
> +        name = fdt_get_name(fdt, node, NULL);
> +        if ( strncmp(name, "module@", strlen("module@")) == 0 ) {
> +
> +            if ( fdt_node_check_compatible(fdt, node,
> +                                           "xen,multiboot-module" ) != 0 )
> +                early_panic("%s not a compatible module node\n", name);
> +
> +            nr = simple_strtol(name + strlen("module@"), NULL, 10);
> +            if ( nr <= 0 )
> +                early_panic("Invalid module number %d\n", nr);
> +
> +            if ( nr > NR_MODULES )
> +                early_panic("too many modules %d > %d\n", nr, NR_MODULES);
> +            if ( nr > nr_modules )
> +                nr_modules = nr;
> +
> +            mod = &early_info.modules.module[nr];
> +
> +            prop = fdt_get_property(fdt, node, "start", NULL);
> +            if ( !prop )
> +                early_panic("no start for module %d\n", nr);
> +
> +            cell = (const u32 *)prop->data;
> +            device_tree_get_reg(&cell, address_cells, size_cells,
> +                                &mod->start, &size);

This get_reg returns a start + size -- can/should we encode the module
as one of these rather than encdong start + end separately and
discarding the 'size' fields?

> +
> +            prop = fdt_get_property(fdt, node, "end", NULL);
> +            if ( !prop )
> +                early_panic("no end for module %d\n", nr);
> +
> +            cell = (const u32 *)prop->data;
> +            device_tree_get_reg(&cell, address_cells, size_cells,
> +                                &mod->size, &size);
> +            mod->size -= mod->start;
> +
> +            prop = fdt_get_property(fdt, node, "bootargs", &len);
> +            if ( prop )
> +            {
> +                if ( len > sizeof(mod->cmdline) )
> +                    early_panic("module %d command line too long\n", nr);
> +
> +                safe_strcpy(mod->cmdline, prop->data);
> +            }
> +            else
> +                mod->cmdline[0] = 0;
> +        }
> +    }
> +
> +    for ( nr = 1 ; nr < nr_modules ; nr++ )
> +    {
> +        mod = &early_info.modules.module[nr];
> +        if ( !mod->start || !mod->size )
> +            early_panic("module %d  missing / invalid\n", nr);
> +    }
> +
> +    early_info.modules.nr_mods = nr_modules;
> +}
> +
>  static int __init early_scan_node(const void *fdt,
>                                    int node, const char *name, int depth,
>                                    u32 address_cells, u32 size_cells,
> @@ -236,6 +309,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_matches(fdt, node, "chosen") )
> +        process_chosen_node(fdt, node, name, address_cells, size_cells);
>  
>      return 0;
>  }
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 4d010c0..c383677 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -15,6 +15,7 @@
>  #define DEVICE_TREE_MAX_DEPTH 16
>  
>  #define NR_MEM_BANKS 8
> +#define NR_MODULES 2
>  
>  struct membank {
>      paddr_t start;
> @@ -26,8 +27,21 @@ struct dt_mem_info {
>      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 + 1];
> +};
> +
>  struct dt_early_info {
>      struct dt_mem_info mem;
> +    struct dt_module_info modules;
>  };
>  
>  typedef int (*device_tree_node_func)(const void *fdt,
> -- 
> 1.7.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 04/12] arm: parse modules from DT during early boot.

[Ian Campbell]

On Thu, 2012-11-29 at 17:05 +0000, Tim Deegan wrote:
> > +            cell = (const u32 *)prop->data;
> > +            device_tree_get_reg(&cell, address_cells, size_cells,
> > +                                &mod->start, &size);
> 
> This get_reg returns a start + size -- can/should we encode the module
> as one of these rather than encdong start + end separately and
> discarding the 'size' fields?

Interesting thought, I'm not enough of a DTB guru to know what the right
way to express this is (CCing Stefano :-))

This is trying to parse
/ {
	chosen { 
		module@1 {
			start = 0x80000000;
			end = 0x2000;
		}
	}
which is roughtly equivalent to how Linux bootloaders pass in initrds
(although the name etc differ)

I suspect using device_tree_get_reg as things stands is just plain
wrong, since the above things are not actually regs.

However you might be right that this should be expressed as

/ {
	chosen { 
		module@1 {
			address = <0x80000000 0x2000>;
		}
	}

and then I think using device_tree_get_reg would be correct.

Stefano -- does that make sense? is "address = < ... >" allowed or does
the thing have to be called reg?

Ian.

Re: [PATCH 04/12] arm: parse modules from DT during early boot.

[Stefano Stabellini]

On Thu, 29 Nov 2012, Ian Campbell wrote:
> On Thu, 2012-11-29 at 17:05 +0000, Tim Deegan wrote:
> > > +            cell = (const u32 *)prop->data;
> > > +            device_tree_get_reg(&cell, address_cells, size_cells,
> > > +                                &mod->start, &size);
> > 
> > This get_reg returns a start + size -- can/should we encode the module
> > as one of these rather than encdong start + end separately and
> > discarding the 'size' fields?
> 
> Interesting thought, I'm not enough of a DTB guru to know what the right
> way to express this is (CCing Stefano :-))
> 
> This is trying to parse
> / {
> 	chosen { 
> 		module@1 {
> 			start = 0x80000000;
> 			end = 0x2000;
> 		}
> 	}
> which is roughtly equivalent to how Linux bootloaders pass in initrds
> (although the name etc differ)
> 
> I suspect using device_tree_get_reg as things stands is just plain
> wrong, since the above things are not actually regs.
> 
> However you might be right that this should be expressed as
> 
> / {
> 	chosen { 
> 		module@1 {
> 			address = <0x80000000 0x2000>;
> 		}
> 	}
> 
> and then I think using device_tree_get_reg would be correct.
> 
> Stefano -- does that make sense? is "address = < ... >" allowed or does
> the thing have to be called reg?

I don't think we should use device_tree_get_reg to parse something that
is not a reg. If we want a reg then we should just call the property
"reg" (I am in favor of that).

Re: [PATCH 04/12] arm: parse modules from DT during early boot.

[Stefano Stabellini]

On Tue, 13 Nov 2012, Ian Campbell wrote:
> The bootloader should populate /chosen/module@<N>/ for each module it
> wishes to pass to the hypervisor. The content of these nodes is
> described in docs/misc/arm/device-tree/booting.txt
> 
> The hypervisor allows for 2 modules (@1==kernel and @2==initrd).
> Currently we don't do anything with them.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: Reserve the zeroeth module for Xen itself (not used yet)
>     Use a more idiomatic DT layout
>     Document said layout
> ---
>  docs/misc/arm/device-tree/booting.txt |   27 ++++++++++++
>  xen/common/device_tree.c              |   75 +++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h         |   14 ++++++
>  3 files changed, 116 insertions(+), 0 deletions(-)
>  create mode 100644 docs/misc/arm/device-tree/booting.txt
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> new file mode 100644
> index 0000000..2609450
> --- /dev/null
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -0,0 +1,27 @@
> +Xen is passed the dom0 kernel and initrd via a reference in the /chosen
> +node of the device tree.
> +
> +Each node has the form /chosen/module@<N> and contains the following
> +properties:

Wouldn't it be better to move all the modules under /chosen/modules or
/chosen/multiboot?


> +- compatible
> +
> +	Must be "xen,multiboot-module"
> +
> +- start
> +
> +	Physical address of the start of this module
> +
> +- end
> +
> +	Physical address of the end of this module

start and end could be encoded as one reg


> +- bootargs (optional)
> +
> +	Command line associated with this module
> +
> +The following modules are understood
> +
> +- 1 -- the domain 0 kernel
> +- 2 -- the domain 0 ramdisk

It would be nice if we could express this via the compatible property
instead.
So the linux kernel could be compatible "linux,kernel" and the initrd
"linux,initrd", in addition to (or instead of) "xen,multiboot-module".
Given that they go from the most specific to the less specific, it would
become:

compatible = "linux,kernel", "xen,multiboot-module";

Re: [PATCH 04/12] arm: parse modules from DT during early boot.

[Ian Campbell]

> > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> > new file mode 100644
> > index 0000000..2609450
> > --- /dev/null
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -0,0 +1,27 @@
> > +Xen is passed the dom0 kernel and initrd via a reference in the /chosen
> > +node of the device tree.
> > +
> > +Each node has the form /chosen/module@<N> and contains the following
> > +properties:
> 
> Wouldn't it be better to move all the modules under /chosen/modules or
> /chosen/multiboot?

Why, what's the benefit?

I'm happy to do whatever is more normal in DT. Is that this:
	/foo/bar@1
	/foo/bar@2
or
	/foo/bar/bar@1
	/foo/bar/bar@2

The second (which I think is what you are suggesting) seems pretty
redundant.

> 
> 
> > +- compatible
> > +
> > +	Must be "xen,multiboot-module"
> > +
> > +- start
> > +
> > +	Physical address of the start of this module
> > +
> > +- end
> > +
> > +	Physical address of the end of this module
> 
> start and end could be encoded as one reg

Done.

> 
> 
> > +- bootargs (optional)
> > +
> > +	Command line associated with this module
> > +
> > +The following modules are understood
> > +
> > +- 1 -- the domain 0 kernel
> > +- 2 -- the domain 0 ramdisk
> 
> It would be nice if we could express this via the compatible property
> instead.
> So the linux kernel could be compatible "linux,kernel" and the initrd
> "linux,initrd", in addition to (or instead of) "xen,multiboot-module".
> Given that they go from the most specific to the less specific, it would
> become:
> 
> compatible = "linux,kernel", "xen,multiboot-module";

This bakes the word "linux" into the interface and would require a new
compatible tag and code changes in Xen for each new dom0 kernel type,
which I think we want to avoid. (maybe the code changes are unavoidable
in practice, but in principal...)

"xen,dom0-kernel", "xen,multiboot-module"

Might be an option?

I'm going to repost what I have without changing this bit yet.

Ian.

Re: [PATCH 04/12] arm: parse modules from DT during early boot.

[Stefano Stabellini]

On Mon, 3 Dec 2012, Ian Campbell wrote:
> > > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> > > new file mode 100644
> > > index 0000000..2609450
> > > --- /dev/null
> > > +++ b/docs/misc/arm/device-tree/booting.txt
> > > @@ -0,0 +1,27 @@
> > > +Xen is passed the dom0 kernel and initrd via a reference in the /chosen
> > > +node of the device tree.
> > > +
> > > +Each node has the form /chosen/module@<N> and contains the following
> > > +properties:
> > 
> > Wouldn't it be better to move all the modules under /chosen/modules or
> > /chosen/multiboot?
> 
> Why, what's the benefit?
> 
> I'm happy to do whatever is more normal in DT. Is that this:
> 	/foo/bar@1
> 	/foo/bar@2
> or
> 	/foo/bar/bar@1
> 	/foo/bar/bar@2
> 
> The second (which I think is what you are suggesting) seems pretty
> redundant.

To be precise I am suggesting:

/foo/bars/bar@0
/foo/bars/bar@1

I think it is just clearer, especially if more stuff end up inside
/chosen. Also see how the cpus node is defined, for example.


> > > +- compatible
> > > +
> > > +	Must be "xen,multiboot-module"
> > > +
> > > +- start
> > > +
> > > +	Physical address of the start of this module
> > > +
> > > +- end
> > > +
> > > +	Physical address of the end of this module
> > 
> > start and end could be encoded as one reg
> 
> Done.
> 
> > 
> > 
> > > +- bootargs (optional)
> > > +
> > > +	Command line associated with this module
> > > +
> > > +The following modules are understood
> > > +
> > > +- 1 -- the domain 0 kernel
> > > +- 2 -- the domain 0 ramdisk
> > 
> > It would be nice if we could express this via the compatible property
> > instead.
> > So the linux kernel could be compatible "linux,kernel" and the initrd
> > "linux,initrd", in addition to (or instead of) "xen,multiboot-module".
> > Given that they go from the most specific to the less specific, it would
> > become:
> > 
> > compatible = "linux,kernel", "xen,multiboot-module";
> 
> This bakes the word "linux" into the interface and would require a new
> compatible tag and code changes in Xen for each new dom0 kernel type,
> which I think we want to avoid. (maybe the code changes are unavoidable
> in practice, but in principal...)
> 
> "xen,dom0-kernel", "xen,multiboot-module"
> 
> Might be an option?
> 
> I'm going to repost what I have without changing this bit yet.

"xem,dom0-kernel" is OK.
However what about the initrd? Does Xen need to know that the second
module is the kernel's initrd or is it just another opaque module from
Xen's point of view?
If Xen needs to know that it is an initrd I think we need to introduce
another compatible string. Maybe the following:

"xen,dom0-initrd"

Re: [PATCH 04/12] arm: parse modules from DT during early boot.

[Ian Campbell]

On Tue, 2012-12-04 at 12:42 +0000, Stefano Stabellini wrote:
> On Mon, 3 Dec 2012, Ian Campbell wrote:
> > > > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> > > > new file mode 100644
> > > > index 0000000..2609450
> > > > --- /dev/null
> > > > +++ b/docs/misc/arm/device-tree/booting.txt
> > > > @@ -0,0 +1,27 @@
> > > > +Xen is passed the dom0 kernel and initrd via a reference in the /chosen
> > > > +node of the device tree.
> > > > +
> > > > +Each node has the form /chosen/module@<N> and contains the following
> > > > +properties:
> > > 
> > > Wouldn't it be better to move all the modules under /chosen/modules or
> > > /chosen/multiboot?
> > 
> > Why, what's the benefit?
> > 
> > I'm happy to do whatever is more normal in DT. Is that this:
> > 	/foo/bar@1
> > 	/foo/bar@2
> > or
> > 	/foo/bar/bar@1
> > 	/foo/bar/bar@2
> > 
> > The second (which I think is what you are suggesting) seems pretty
> > redundant.
> 
> To be precise I am suggesting:
> 
> /foo/bars/bar@0
> /foo/bars/bar@1
> 
> I think it is just clearer, especially if more stuff end up inside
> /chosen. Also see how the cpus node is defined, for example.

OK.

> > > 
> > > 
> > > > +- bootargs (optional)
> > > > +
> > > > +	Command line associated with this module
> > > > +
> > > > +The following modules are understood
> > > > +
> > > > +- 1 -- the domain 0 kernel
> > > > +- 2 -- the domain 0 ramdisk
> > > 
> > > It would be nice if we could express this via the compatible property
> > > instead.
> > > So the linux kernel could be compatible "linux,kernel" and the initrd
> > > "linux,initrd", in addition to (or instead of) "xen,multiboot-module".
> > > Given that they go from the most specific to the less specific, it would
> > > become:
> > > 
> > > compatible = "linux,kernel", "xen,multiboot-module";
> > 
> > This bakes the word "linux" into the interface and would require a new
> > compatible tag and code changes in Xen for each new dom0 kernel type,
> > which I think we want to avoid. (maybe the code changes are unavoidable
> > in practice, but in principal...)
> > 
> > "xen,dom0-kernel", "xen,multiboot-module"
> > 
> > Might be an option?
> > 
> > I'm going to repost what I have without changing this bit yet.
> 
> "xem,dom0-kernel" is OK.
> However what about the initrd? Does Xen need to know that the second
> module is the kernel's initrd or is it just another opaque module from
> Xen's point of view?
> If Xen needs to know that it is an initrd I think we need to introduce
> another compatible string. Maybe the following:
> 
> "xen,dom0-initrd"

Hr, perhaps it does need to know it is a Linux initrd, or indeed that
the kernel is Linux in order to implement the necessary boot protocol.
IOW in the absence of a more generic boot protocol for ARM maybe we
can't avoid coding OS specifics into the builder :-(

[PATCH 05/12] arm: avoid placing Xen over any modules.

[Ian Campbell]

This will still fail if the modules are such that Xen is pushed out of
the top 32M of RAM since it will then overlap with the domheap (or
possibly xenheap). This will be dealt with later.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
---
v2: Xen is module 0, modules start at 1.
---
 xen/arch/arm/setup.c |   68 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index b0b30d6..587b1e7 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -68,17 +68,55 @@ static void __init processor_id(void)
            READ_CP32(ID_ISAR3), READ_CP32(ID_ISAR4), READ_CP32(ID_ISAR5));
 }
 
+/*
+ * Returns the end address of the highest region in the range s..e
+ * with required size and alignment that does not conflict with the
+ * modules from first_mod to nr_modules.
+ *
+ * For non-recursive callers first_mod should normally be 1.
+ */
+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;
+    int i;
+
+    s = (s+align-1) & ~(align-1);
+    e = e & ~(align-1);
+
+    if ( s > e ||  e - s < size )
+        return 0;
+
+    for ( i = first_mod; i <= mi->nr_mods; i++ )
+    {
+        paddr_t mod_s = mi->module[i].start;
+        paddr_t mod_e = mod_s + mi->module[i].size;
+
+        if ( s < mod_e && mod_s < e )
+        {
+            mod_e = consider_modules(mod_e, e, size, align, i+1);
+            if ( mod_e )
+                return mod_e;
+
+            return consider_modules(s, mod_s, size, align, i+1);
+        }
+    }
+
+    return e;
+}
+
 /**
  * get_xen_paddr - get physical address to relocate Xen to
  *
- * Xen is relocated to the top of RAM and aligned to a XEN_PADDR_ALIGN
- * boundary.
+ * Xen is relocated to as near to the top of RAM as possible and
+ * aligned to a XEN_PADDR_ALIGN boundary.
  */
 static paddr_t __init get_xen_paddr(void)
 {
     struct dt_mem_info *mi = &early_info.mem;
     paddr_t min_size;
-    paddr_t paddr = 0, t;
+    paddr_t paddr = 0;
     int i;
 
     min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) & ~(XEN_PADDR_ALIGN-1);
@@ -86,17 +124,33 @@ static paddr_t __init get_xen_paddr(void)
     /* Find the highest bank with enough space. */
     for ( i = 0; i < mi->nr_banks; i++ )
     {
-        if ( mi->bank[i].size >= min_size )
+        const struct membank *bank = &mi->bank[i];
+        paddr_t s, e;
+
+        if ( bank->size >= min_size )
         {
-            t = mi->bank[i].start + mi->bank[i].size - min_size;
-            if ( t > paddr )
-                paddr = t;
+            e = consider_modules(bank->start, bank->start + bank->size,
+                                 min_size, XEN_PADDR_ALIGN, 1);
+            if ( !e )
+                continue;
+
+            s = e - min_size;
+
+            if ( s > paddr )
+                paddr = s;
         }
     }
 
     if ( !paddr )
         early_panic("Not enough memory to relocate Xen\n");
 
+    early_printk("Placing Xen at 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
+                 paddr, paddr + min_size);
+
+    /* Xen is module 0 */
+    early_info.modules.module[0].start = paddr;
+    early_info.modules.module[0].size = min_size;
+
     return paddr;
 }
 
-- 
1.7.9.1

[PATCH 06/12] arm: avoid allocating the heaps over modules or xen itself.

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/setup.c |   89 +++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 78 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 587b1e7..04d16a1 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -73,7 +73,8 @@ static void __init processor_id(void)
  * with required size and alignment that does not conflict with the
  * modules from first_mod to nr_modules.
  *
- * For non-recursive callers first_mod should normally be 1.
+ * For non-recursive callers first_mod should normally be 0 (all
+ * modules and Xen itself) or 1 (all modules but not Xen).
  */
 static paddr_t __init consider_modules(paddr_t s, paddr_t e,
                                        uint32_t size, paddr_t align,
@@ -106,6 +107,34 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
     return e;
 }
 
+/*
+ * Return the end of the non-module region starting at s. In other
+ * words return s the start of the next modules after s.
+ *
+ * Also returns the end of that module in *n.
+ */
+static paddr_t __init next_module(paddr_t s, paddr_t *n)
+{
+    struct dt_module_info *mi = &early_info.modules;
+    paddr_t lowest = ~(paddr_t)0;
+    int i;
+
+    for ( i = 0; i <= mi->nr_mods; i++ )
+    {
+        paddr_t mod_s = mi->module[i].start;
+        paddr_t mod_e = mod_s + mi->module[i].size;
+
+        if ( mod_s < s )
+            continue;
+        if ( mod_s > lowest )
+            continue;
+        lowest = mod_s;
+        *n = mod_e;
+    }
+    return lowest;
+}
+
+
 /**
  * get_xen_paddr - get physical address to relocate Xen to
  *
@@ -159,6 +188,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
     paddr_t ram_start;
     paddr_t ram_end;
     paddr_t ram_size;
+    paddr_t s, e;
     unsigned long ram_pages;
     unsigned long heap_pages, xenheap_pages, domheap_pages;
     unsigned long dtb_pages;
@@ -176,22 +206,37 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
     ram_pages = ram_size >> PAGE_SHIFT;
 
     /*
-     * Calculate the sizes for the heaps using these constraints:
+     * Locate the xenheap using these constraints:
      *
-     *  - heaps must be 32 MiB aligned
-     *  - must not include Xen itself
-     *  - xen heap must be at most 1 GiB
+     *  - must be 32 MiB aligned
+     *  - must not include Xen itself or the boot modules
+     *  - must be at most 1 GiB
+     *  - must be at least 128M
      *
-     * XXX: needs a platform with at least 1GiB of RAM or the dom
-     * heap will be empty and no domains can be created.
+     * We try to allocate the largest xenheap possible within these
+     * constraints.
      */
-    heap_pages = (ram_size >> PAGE_SHIFT) - (32 << (20 - PAGE_SHIFT));
+    heap_pages = (ram_size >> PAGE_SHIFT);
     xenheap_pages = min(1ul << (30 - PAGE_SHIFT), heap_pages);
+
+    do
+    {
+        e = consider_modules(ram_start, ram_end, xenheap_pages<<PAGE_SHIFT,
+                             32<<20, 0);
+        if ( e )
+            break;
+
+        xenheap_pages >>= 1;
+    } while ( xenheap_pages > 128<<(20-PAGE_SHIFT) );
+
+    if ( ! e )
+        panic("Not not enough space for xenheap\n");
+
     domheap_pages = heap_pages - xenheap_pages;
 
     printk("Xen heap: %lu pages  Dom heap: %lu pages\n", xenheap_pages, domheap_pages);
 
-    setup_xenheap_mappings(ram_start >> PAGE_SHIFT, xenheap_pages);
+    setup_xenheap_mappings((e >> PAGE_SHIFT) - xenheap_pages, xenheap_pages);
 
     /*
      * Need a single mapped page for populating bootmem_region_list
@@ -215,8 +260,30 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
     copy_from_paddr(device_tree_flattened, dtb_paddr, dtb_size, BUFFERABLE);
 
     /* Add non-xenheap memory */
-    init_boot_pages(pfn_to_paddr(xenheap_mfn_start + xenheap_pages),
-                    pfn_to_paddr(xenheap_mfn_start + xenheap_pages + domheap_pages));
+    s = ram_start;
+    while ( s < ram_end )
+    {
+        paddr_t n = ram_end;
+
+        e = next_module(s, &n);
+
+        if ( e == ~(paddr_t)0 )
+        {
+            e = n = ram_end;
+        }
+
+        /* Avoid the xenheap */
+        if ( s < ((xenheap_mfn_start+xenheap_pages) << PAGE_SHIFT)
+             && (xenheap_mfn_start << PAGE_SHIFT) < e )
+        {
+            e = pfn_to_paddr(xenheap_mfn_start);
+            n = pfn_to_paddr(xenheap_mfn_start+xenheap_pages);
+        }
+
+        init_boot_pages(s, e);
+
+        s = n;
+    }
 
     setup_frametable_mappings(ram_start, ram_end);
     max_page = PFN_DOWN(ram_end);
-- 
1.7.9.1

Re: [PATCH 06/12] arm: avoid allocating the heaps over modules or xen itself.

[Tim Deegan]

At 16:23 +0000 on 13 Nov (1352823798), Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/setup.c |   89 +++++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 78 insertions(+), 11 deletions(-)
> 
> @@ -215,8 +260,30 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>      copy_from_paddr(device_tree_flattened, dtb_paddr, dtb_size, BUFFERABLE);
>  
>      /* Add non-xenheap memory */
> -    init_boot_pages(pfn_to_paddr(xenheap_mfn_start + xenheap_pages),
> -                    pfn_to_paddr(xenheap_mfn_start + xenheap_pages + domheap_pages));
> +    s = ram_start;
> +    while ( s < ram_end )
> +    {
> +        paddr_t n = ram_end;
> +
> +        e = next_module(s, &n);

Does this DTRT if there's a module starting at exactly ram_start?

> +        if ( e == ~(paddr_t)0 )
> +        {
> +            e = n = ram_end;
> +        }
> +
> +        /* Avoid the xenheap */
> +        if ( s < ((xenheap_mfn_start+xenheap_pages) << PAGE_SHIFT)
> +             && (xenheap_mfn_start << PAGE_SHIFT) < e )
> +        {
> +            e = pfn_to_paddr(xenheap_mfn_start);
> +            n = pfn_to_paddr(xenheap_mfn_start+xenheap_pages);
> +        }
> +
> +        init_boot_pages(s, e);
> +
> +        s = n;
> +    }

Re: [PATCH 06/12] arm: avoid allocating the heaps over modules or xen itself.

[Ian Campbell]

On Thu, 2012-11-29 at 17:06 +0000, Tim Deegan wrote:
> At 16:23 +0000 on 13 Nov (1352823798), Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/setup.c |   89 +++++++++++++++++++++++++++++++++++++++++++------
> >  1 files changed, 78 insertions(+), 11 deletions(-)
> > 
> > @@ -215,8 +260,30 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> >      copy_from_paddr(device_tree_flattened, dtb_paddr, dtb_size, BUFFERABLE);
> >  
> >      /* Add non-xenheap memory */
> > -    init_boot_pages(pfn_to_paddr(xenheap_mfn_start + xenheap_pages),
> > -                    pfn_to_paddr(xenheap_mfn_start + xenheap_pages + domheap_pages));
> > +    s = ram_start;
> > +    while ( s < ram_end )
> > +    {
> > +        paddr_t n = ram_end;
> > +
> > +        e = next_module(s, &n);
> 
> Does this DTRT if there's a module starting at exactly ram_start?

I should probably try it...

I think in this case next_module will return the start of that module,
so e = s, and it will also set n to the end of the module.

init_boot_pages does the right thing if e <= s (i.e. ignores it) and
then we do s = n (so s = module_end) and keep going.

So I think it works?

> 
> > +        if ( e == ~(paddr_t)0 )
> > +        {
> > +            e = n = ram_end;
> > +        }
> > +
> > +        /* Avoid the xenheap */
> > +        if ( s < ((xenheap_mfn_start+xenheap_pages) << PAGE_SHIFT)
> > +             && (xenheap_mfn_start << PAGE_SHIFT) < e )
> > +        {
> > +            e = pfn_to_paddr(xenheap_mfn_start);
> > +            n = pfn_to_paddr(xenheap_mfn_start+xenheap_pages);
> > +        }
> > +
> > +        init_boot_pages(s, e);
> > +
> > +        s = n;
> > +    }

Re: [PATCH 06/12] arm: avoid allocating the heaps over modules or xen itself.

[Tim Deegan]

At 17:19 +0000 on 29 Nov (1354209553), Ian Campbell wrote:
> On Thu, 2012-11-29 at 17:06 +0000, Tim Deegan wrote:
> > At 16:23 +0000 on 13 Nov (1352823798), Ian Campbell wrote:
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > >  xen/arch/arm/setup.c |   89 +++++++++++++++++++++++++++++++++++++++++++------
> > >  1 files changed, 78 insertions(+), 11 deletions(-)
> > > 
> > > @@ -215,8 +260,30 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> > >      copy_from_paddr(device_tree_flattened, dtb_paddr, dtb_size, BUFFERABLE);
> > >  
> > >      /* Add non-xenheap memory */
> > > -    init_boot_pages(pfn_to_paddr(xenheap_mfn_start + xenheap_pages),
> > > -                    pfn_to_paddr(xenheap_mfn_start + xenheap_pages + domheap_pages));
> > > +    s = ram_start;
> > > +    while ( s < ram_end )
> > > +    {
> > > +        paddr_t n = ram_end;
> > > +
> > > +        e = next_module(s, &n);
> > 
> > Does this DTRT if there's a module starting at exactly ram_start?
> 
> I should probably try it...
> 
> I think in this case next_module will return the start of that module,
> so e = s, and it will also set n to the end of the module.

Yes, I see; you go nce around the loop with a 0-length range, which
init_boot_pages will ignore.  Fine. 

Tim.

[PATCH 07/12] arm: const-correctness in virt_to_maddr

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Acked-by: Tim Deegan <tim@xen.org>
---
v2: dropped one unnecessary const.
---
 xen/include/asm-arm/mm.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 260af35..e95ece1 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -179,7 +179,7 @@ extern void clear_fixmap(unsigned map);
 #define paddr_to_pdx(pa)    pfn_to_pdx(paddr_to_pfn(pa))
 
 
-static inline paddr_t virt_to_maddr(void *va)
+static inline paddr_t virt_to_maddr(const void *va)
 {
     uint64_t par = va_to_par((uint32_t)va);
     return (par & PADDR_MASK & PAGE_MASK) | ((unsigned long) va & ~PAGE_MASK);
-- 
1.7.9.1

[PATCH 08/12] device-tree: get_val cannot cope with cells > 2, add a BUG

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: drop unrelated white space fixup
---
 xen/common/device_tree.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index efd1663..7d3fd9f 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -45,6 +45,8 @@ static void __init get_val(const u32 **cell, u32 cells, u64 *val)
 {
     *val = 0;
 
+    BUG_ON( cells > 2 );
+
     while ( cells-- )
     {
         *val <<= 32;
-- 
1.7.9.1

Re: [PATCH 08/12] device-tree: get_val cannot cope with cells > 2, add a BUG

[Tim Deegan]

At 16:23 +0000 on 13 Nov (1352823800), Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: drop unrelated white space fixup
> ---
>  xen/common/device_tree.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index efd1663..7d3fd9f 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -45,6 +45,8 @@ static void __init get_val(const u32 **cell, u32 cells, u64 *val)
>  {
>      *val = 0;
>  
> +    BUG_ON( cells > 2 );
> +

If this is caused by a malformed DTB (rather than a Xen bug), I think
this should be an early_panic() or similar.

>      while ( cells-- )
>      {
>          *val <<= 32;
> -- 
> 1.7.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 08/12] device-tree: get_val cannot cope with cells > 2, add a BUG

[Ian Campbell]

On Thu, 2012-11-29 at 17:09 +0000, Tim Deegan wrote:
> At 16:23 +0000 on 13 Nov (1352823800), Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > v2: drop unrelated white space fixup
> > ---
> >  xen/common/device_tree.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index efd1663..7d3fd9f 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -45,6 +45,8 @@ static void __init get_val(const u32 **cell, u32 cells, u64 *val)
> >  {
> >      *val = 0;
> >  
> > +    BUG_ON( cells > 2 );
> > +
> 
> If this is caused by a malformed DTB (rather than a Xen bug), I think
> this should be an early_panic() or similar.

I agree, will change.

> 
> >      while ( cells-- )
> >      {
> >          *val <<= 32;
> > -- 
> > 1.7.9.1
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

[PATCH 09/12] arm: load dom0 kernel from first boot module

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/kernel.c |   74 +++++++++++++++++++++++++++++++++++++-----------
 xen/arch/arm/kernel.h |   10 ++++++
 2 files changed, 67 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 2d56130..7fb6268 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -65,13 +65,13 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len, int attrindx)
 static void kernel_zimage_load(struct kernel_info *info)
 {
     paddr_t load_addr = info->zimage.load_addr;
+    paddr_t paddr = info->zimage.kernel_addr;
     paddr_t len = info->zimage.len;
-    paddr_t flash = KERNEL_FLASH_ADDRESS;
     void *src = (void *)FIXMAP_ADDR(FIXMAP_MISC);
     unsigned long offs;
 
-    printk("Loading %"PRIpaddr" byte zImage from flash %"PRIpaddr" to %"PRIpaddr"-%"PRIpaddr": [",
-           len, flash, load_addr, load_addr + len);
+    printk("Loading zImage from %"PRIpaddr" to %"PRIpaddr"-%"PRIpaddr": [",
+           paddr, load_addr, load_addr + len);
     for ( offs = 0; offs < len; offs += PAGE_SIZE )
     {
         paddr_t ma = gvirt_to_maddr(load_addr + offs);
@@ -80,7 +80,7 @@ static void kernel_zimage_load(struct kernel_info *info)
         if ( ( offs % (1<<20) ) == 0 )
             printk(".");
 
-        set_fixmap(FIXMAP_MISC, (flash+offs) >> PAGE_SHIFT, DEV_SHARED);
+        set_fixmap(FIXMAP_MISC, (paddr+offs) >> PAGE_SHIFT, DEV_SHARED);
         memcpy(dst, src, PAGE_SIZE);
         clear_fixmap(FIXMAP_MISC);
 
@@ -92,30 +92,48 @@ static void kernel_zimage_load(struct kernel_info *info)
 /**
  * Check the image is a zImage and return the load address and length
  */
-static int kernel_try_zimage_prepare(struct kernel_info *info)
+static int kernel_try_zimage_prepare(struct kernel_info *info,
+                                     paddr_t addr, paddr_t size)
 {
     uint32_t *zimage = (void *)FIXMAP_ADDR(FIXMAP_MISC);
     uint32_t start, end;
     struct minimal_dtb_header dtb_hdr;
 
-    set_fixmap(FIXMAP_MISC, KERNEL_FLASH_ADDRESS >> PAGE_SHIFT, DEV_SHARED);
+    set_fixmap(FIXMAP_MISC, addr >> PAGE_SHIFT, DEV_SHARED);
+
+    zimage += addr & ~PAGE_MASK;
 
     if (zimage[ZIMAGE_MAGIC_OFFSET/4] != ZIMAGE_MAGIC)
+    {
+        clear_fixmap(FIXMAP_MISC);
         return -EINVAL;
+    }
 
     start = zimage[ZIMAGE_START_OFFSET/4];
     end = zimage[ZIMAGE_END_OFFSET/4];
 
     clear_fixmap(FIXMAP_MISC);
 
+    if ( end > addr + size )
+        return -EINVAL;
+
     /*
      * Check for an appended DTB.
      */
-    copy_from_paddr(&dtb_hdr, KERNEL_FLASH_ADDRESS + end - start, sizeof(dtb_hdr), DEV_SHARED);
-    if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) {
-        end += be32_to_cpu(dtb_hdr.total_size);
+    if ( addr + end - start + sizeof(dtb_hdr) <= size )
+    {
+        copy_from_paddr(&dtb_hdr, addr + end - start,
+                        sizeof(dtb_hdr), DEV_SHARED);
+        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) {
+            end += be32_to_cpu(dtb_hdr.total_size);
+
+            if ( end > addr + size )
+                return -EINVAL;
+        }
     }
 
+    info->zimage.kernel_addr = addr;
+
     /*
      * If start is zero, the zImage is position independent -- load it
      * at 32k from start of RAM.
@@ -142,25 +160,26 @@ static void kernel_elf_load(struct kernel_info *info)
     free_xenheap_pages(info->kernel_img, info->kernel_order);
 }
 
-static int kernel_try_elf_prepare(struct kernel_info *info)
+static int kernel_try_elf_prepare(struct kernel_info *info,
+                                  paddr_t addr, paddr_t size)
 {
     int rc;
 
-    info->kernel_order = get_order_from_bytes(KERNEL_FLASH_SIZE);
+    info->kernel_order = get_order_from_bytes(size);
     info->kernel_img = alloc_xenheap_pages(info->kernel_order, 0);
     if ( info->kernel_img == NULL )
         panic("Cannot allocate temporary buffer for kernel.\n");
 
-    copy_from_paddr(info->kernel_img, KERNEL_FLASH_ADDRESS, KERNEL_FLASH_SIZE, DEV_SHARED);
+    copy_from_paddr(info->kernel_img, addr, size, DEV_SHARED);
 
-    if ( (rc = elf_init(&info->elf.elf, info->kernel_img, KERNEL_FLASH_SIZE )) != 0 )
-        return rc;
+    if ( (rc = elf_init(&info->elf.elf, info->kernel_img, size )) != 0 )
+        goto err;
 #ifdef VERBOSE
     elf_set_verbose(&info->elf.elf);
 #endif
     elf_parse_binary(&info->elf.elf);
     if ( (rc = elf_xen_parse(&info->elf.elf, &info->elf.parms)) != 0 )
-        return rc;
+        goto err;
 
     /*
      * TODO: can the ELF header be used to find the physical address
@@ -170,15 +189,36 @@ static int kernel_try_elf_prepare(struct kernel_info *info)
     info->load = kernel_elf_load;
 
     return 0;
+err:
+    free_xenheap_pages(info->kernel_img, info->kernel_order);
+    return rc;
 }
 
 int kernel_prepare(struct kernel_info *info)
 {
     int rc;
 
-    rc = kernel_try_zimage_prepare(info);
+    paddr_t start, size;
+
+    if ( early_info.modules.nr_mods > 1 )
+        panic("Cannot handle dom0 initrd yet\n");
+
+    if ( early_info.modules.nr_mods < 1 )
+    {
+        printk("No boot modules found, trying flash\n");
+        start = KERNEL_FLASH_ADDRESS;
+        size = KERNEL_FLASH_SIZE;
+    }
+    else
+    {
+        printk("Loading kernel from boot module 1\n");
+        start = early_info.modules.module[1].start;
+        size = early_info.modules.module[1].size;
+    }
+
+    rc = kernel_try_zimage_prepare(info, start, size);
     if (rc < 0)
-        rc = kernel_try_elf_prepare(info);
+        rc = kernel_try_elf_prepare(info, start, size);
 
     return rc;
 }
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index 4533568..2353e13 100644
--- a/xen/arch/arm/kernel.h
+++ b/xen/arch/arm/kernel.h
@@ -22,6 +22,7 @@ struct kernel_info {
 
     union {
         struct {
+            paddr_t kernel_addr;
             paddr_t load_addr;
             paddr_t len;
         } zimage;
@@ -39,3 +40,12 @@ int kernel_prepare(struct kernel_info *info);
 void kernel_load(struct kernel_info *info);
 
 #endif /* #ifdef __ARCH_ARM_KERNEL_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-set-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.7.9.1

Re: [PATCH 09/12] arm: load dom0 kernel from first boot module

[Tim Deegan]

At 16:23 +0000 on 13 Nov (1352823801), Ian Campbell wrote:
> -static int kernel_try_zimage_prepare(struct kernel_info *info)
> +static int kernel_try_zimage_prepare(struct kernel_info *info,
> +                                     paddr_t addr, paddr_t size)
>  {
>      uint32_t *zimage = (void *)FIXMAP_ADDR(FIXMAP_MISC);
>      uint32_t start, end;
>      struct minimal_dtb_header dtb_hdr;
>  
> -    set_fixmap(FIXMAP_MISC, KERNEL_FLASH_ADDRESS >> PAGE_SHIFT, DEV_SHARED);
> +    set_fixmap(FIXMAP_MISC, addr >> PAGE_SHIFT, DEV_SHARED);
> +
> +    zimage += addr & ~PAGE_MASK;
>  
>      if (zimage[ZIMAGE_MAGIC_OFFSET/4] != ZIMAGE_MAGIC)
> +    {
> +        clear_fixmap(FIXMAP_MISC);
>          return -EINVAL;
> +    }
>  
>      start = zimage[ZIMAGE_START_OFFSET/4];
>      end = zimage[ZIMAGE_END_OFFSET/4];
>  
>      clear_fixmap(FIXMAP_MISC);
>  
> +    if ( end > addr + size )
> +        return -EINVAL;

Should this also check for start == 0 && end > size?

>  int kernel_prepare(struct kernel_info *info)
>  {
>      int rc;
>  
> -    rc = kernel_try_zimage_prepare(info);
> +    paddr_t start, size;
> +
> +    if ( early_info.modules.nr_mods > 1 )
> +        panic("Cannot handle dom0 initrd yet\n");
> +
> +    if ( early_info.modules.nr_mods < 1 )
> +    {
> +        printk("No boot modules found, trying flash\n");
> +        start = KERNEL_FLASH_ADDRESS;
> +        size = KERNEL_FLASH_SIZE;
> +    }
> +    else
> +    {
> +        printk("Loading kernel from boot module 1\n");
> +        start = early_info.modules.module[1].start;
> +        size = early_info.modules.module[1].size;

Do we want (here or elsewhere) to check that start is page-aligned?

Tim.

Re: [PATCH 09/12] arm: load dom0 kernel from first boot module

[Ian Campbell]

On Thu, 2012-11-29 at 17:15 +0000, Tim Deegan wrote:
> At 16:23 +0000 on 13 Nov (1352823801), Ian Campbell wrote:
> > -static int kernel_try_zimage_prepare(struct kernel_info *info)
> > +static int kernel_try_zimage_prepare(struct kernel_info *info,
> > +                                     paddr_t addr, paddr_t size)
> >  {
> >      uint32_t *zimage = (void *)FIXMAP_ADDR(FIXMAP_MISC);
> >      uint32_t start, end;
> >      struct minimal_dtb_header dtb_hdr;
> >  
> > -    set_fixmap(FIXMAP_MISC, KERNEL_FLASH_ADDRESS >> PAGE_SHIFT, DEV_SHARED);
> > +    set_fixmap(FIXMAP_MISC, addr >> PAGE_SHIFT, DEV_SHARED);
> > +
> > +    zimage += addr & ~PAGE_MASK;
> >  
> >      if (zimage[ZIMAGE_MAGIC_OFFSET/4] != ZIMAGE_MAGIC)
> > +    {
> > +        clear_fixmap(FIXMAP_MISC);
> >          return -EINVAL;
> > +    }
> >  
> >      start = zimage[ZIMAGE_START_OFFSET/4];
> >      end = zimage[ZIMAGE_END_OFFSET/4];
> >  
> >      clear_fixmap(FIXMAP_MISC);
> >  
> > +    if ( end > addr + size )
> > +        return -EINVAL;
> 
> Should this also check for start == 0 && end > size?

Possibly ought to be checking for (end - start) > size which covers
both?

Looking at it now comparing addr + size with end seems a bit nonsensical
since addr is where it is now and end is the end of where it would like
to be loaded (or the size if start == 0, which is what has saved us so
far).

> 
> >  int kernel_prepare(struct kernel_info *info)
> >  {
> >      int rc;
> >  
> > -    rc = kernel_try_zimage_prepare(info);
> > +    paddr_t start, size;
> > +
> > +    if ( early_info.modules.nr_mods > 1 )
> > +        panic("Cannot handle dom0 initrd yet\n");
> > +
> > +    if ( early_info.modules.nr_mods < 1 )
> > +    {
> > +        printk("No boot modules found, trying flash\n");
> > +        start = KERNEL_FLASH_ADDRESS;
> > +        size = KERNEL_FLASH_SIZE;
> > +    }
> > +    else
> > +    {
> > +        printk("Loading kernel from boot module 1\n");
> > +        start = early_info.modules.module[1].start;
> > +        size = early_info.modules.module[1].size;
> 
> Do we want (here or elsewhere) to check that start is page-aligned?

I think kernel_try_zimage_prepare tries to do the right thing

Although you've made me look and I suspect it is buggy if start is <
sizeof(zimage header) from the end of a page. It should probably just
use copy_from_paddr into a local buffer.

Ian.

Re: [PATCH 09/12] arm: load dom0 kernel from first boot module

[Tim Deegan]

At 17:24 +0000 on 29 Nov (1354209882), Ian Campbell wrote:
> On Thu, 2012-11-29 at 17:15 +0000, Tim Deegan wrote:
> > At 16:23 +0000 on 13 Nov (1352823801), Ian Campbell wrote:
> > > -static int kernel_try_zimage_prepare(struct kernel_info *info)
> > > +static int kernel_try_zimage_prepare(struct kernel_info *info,
> > > +                                     paddr_t addr, paddr_t size)
> > >  {
> > >      uint32_t *zimage = (void *)FIXMAP_ADDR(FIXMAP_MISC);
> > >      uint32_t start, end;
> > >      struct minimal_dtb_header dtb_hdr;
> > >  
> > > -    set_fixmap(FIXMAP_MISC, KERNEL_FLASH_ADDRESS >> PAGE_SHIFT, DEV_SHARED);
> > > +    set_fixmap(FIXMAP_MISC, addr >> PAGE_SHIFT, DEV_SHARED);
> > > +
> > > +    zimage += addr & ~PAGE_MASK;
> > >  
> > >      if (zimage[ZIMAGE_MAGIC_OFFSET/4] != ZIMAGE_MAGIC)
> > > +    {
> > > +        clear_fixmap(FIXMAP_MISC);
> > >          return -EINVAL;
> > > +    }
> > >  
> > >      start = zimage[ZIMAGE_START_OFFSET/4];
> > >      end = zimage[ZIMAGE_END_OFFSET/4];
> > >  
> > >      clear_fixmap(FIXMAP_MISC);
> > >  
> > > +    if ( end > addr + size )
> > > +        return -EINVAL;
> > 
> > Should this also check for start == 0 && end > size?
> 
> Possibly ought to be checking for (end - start) > size which covers
> both?

Er, yes. :)

> > Do we want (here or elsewhere) to check that start is page-aligned?
> 
> I think kernel_try_zimage_prepare tries to do the right thing

Oh, so it does.  But kernel_zimage_load doesn't seem to -- it will copy
the data just below the start (which may be OK) but might not copy the
last part-page.   Also it won't handle the case where the 
(load_addr & 0xfff) != (source_addr & 0xfff).

Tim.

> Although you've made me look and I suspect it is buggy if start is <
> sizeof(zimage header) from the end of a page. It should probably just
> use copy_from_paddr into a local buffer.
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

[PATCH 10/12] arm: discard boot modules after building domain 0.

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/domain_build.c |    3 +++
 xen/arch/arm/setup.c        |   16 ++++++++++++++++
 xen/include/asm-arm/setup.h |    2 ++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index a9e7f43..e96ed10 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -10,6 +10,7 @@
 #include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.h>
 #include <xen/guest_access.h>
+#include <asm/setup.h>
 
 #include "gic.h"
 #include "kernel.h"
@@ -308,6 +309,8 @@ int construct_dom0(struct domain *d)
     dtb_load(&kinfo);
     kernel_load(&kinfo);
 
+    discard_initial_modules();
+
     clear_bit(_VPF_down, &v->pause_flags);
 
     memset(regs, 0, sizeof(*regs));
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 04d16a1..0b668fa 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -68,6 +68,22 @@ static void __init processor_id(void)
            READ_CP32(ID_ISAR3), READ_CP32(ID_ISAR4), READ_CP32(ID_ISAR5));
 }
 
+void __init discard_initial_modules(void)
+{
+    struct dt_module_info *mi = &early_info.modules;
+    int i;
+
+    for ( i = 1; i <= mi->nr_mods; i++ )
+    {
+        paddr_t s = mi->module[i].start;
+        paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
+
+        init_domheap_pages(s, e);
+    }
+
+    mi->nr_mods = 0;
+}
+
 /*
  * Returns the end address of the highest region in the range s..e
  * with required size and alignment that does not conflict with the
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 8769f66..3267db0 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -9,6 +9,8 @@ void arch_get_xen_caps(xen_capabilities_info_t *info);
 
 int construct_dom0(struct domain *d);
 
+void discard_initial_modules(void);
+
 #endif
 /*
  * Local variables:
-- 
1.7.9.1

[PATCH 11/12] arm: use /chosen/module@1/bootargs for domain 0 command line

[Ian Campbell]

Fallback to xen,dom0-bootargs if this isn't present.

Ideally this would use module1-args iff the kernel came from
module@1/{start,end} and the existing xen,dom0-bootargs if the kernel
came from flash, but this approach is simpler and has the same effect
in practice.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: update for new DT layout
---
 xen/arch/arm/domain_build.c |   28 ++++++++++++++++++++++++----
 1 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e96ed10..7a964f7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -86,8 +86,13 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
                             int node, const char *name, int depth,
                             u32 address_cells, u32 size_cells)
 {
+    const char *bootargs = NULL;
     int prop;
 
+    if ( early_info.modules.nr_mods >= 1 &&
+         early_info.modules.module[1].cmdline[0] )
+        bootargs = &early_info.modules.module[1].cmdline[0];
+
     for ( prop = fdt_first_property_offset(fdt, node);
           prop >= 0;
           prop = fdt_next_property_offset(fdt, prop) )
@@ -104,15 +109,22 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
         prop_len  = fdt32_to_cpu(p->len);
 
         /*
-         * In chosen node: replace bootargs with value from
-         * xen,dom0-bootargs.
+         * In chosen node:
+         *
+         * * remember xen,dom0-bootargs if we don't already have
+         *   bootargs (from module #1, above).
+         * * remove bootargs and xen,dom0-bootargs.
          */
         if ( device_tree_node_matches(fdt, node, "chosen") )
         {
             if ( strcmp(prop_name, "bootargs") == 0 )
                 continue;
-            if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
-                prop_name = "bootargs";
+            else if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
+            {
+                if ( !bootargs )
+                    bootargs = prop_data;
+                continue;
+            }
         }
         /*
          * In a memory node: adjust reg property.
@@ -147,6 +159,14 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
         xfree(new_data);
     }
 
+    if ( device_tree_node_matches(fdt, node, "chosen") && bootargs )
+        fdt_property(kinfo->fdt, "bootargs", bootargs, strlen(bootargs));
+
+    /*
+     * XXX should populate /chosen/linux,initrd-{start,end} here if we
+     * have module[2]
+     */
+
     if ( prop == -FDT_ERR_NOTFOUND )
         return 0;
     return prop;
-- 
1.7.9.1

[PATCH 12/12] xen: strip /chosen/module@<N>/* from dom0 device tree

[Ian Campbell]

These nodes are used by Xen to find the initial modules.

Only drop the "xen,multiboot-module" compatible nodes in case someone
else has a similar idea.

Signed-off-by: Ian Campbell <ian.campbel@citrix.com>
---
 xen/arch/arm/domain_build.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 7a964f7..d2158d8 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -179,6 +179,7 @@ static int write_nodes(struct domain *d, struct kernel_info *kinfo,
     int depth = 0, last_depth = -1;
     u32 address_cells[DEVICE_TREE_MAX_DEPTH];
     u32 size_cells[DEVICE_TREE_MAX_DEPTH];
+    int parents[DEVICE_TREE_MAX_DEPTH];
     int ret;
 
     for ( node = 0, depth = 0;
@@ -196,10 +197,25 @@ static int write_nodes(struct domain *d, struct kernel_info *kinfo,
         }
 
         while ( last_depth-- >= depth )
+        {
+            parents[last_depth+1] = -1;
             fdt_end_node(kinfo->fdt);
+        }
 
         address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells");
         size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells");
+        parents[depth] = node;
+
+        /* Skip /chosen/module@<N>/ nodes */
+        if ( depth == 2 &&
+             device_tree_node_matches(fdt, parents[1], "chosen") &&
+             device_tree_node_matches(fdt, node, "module") &&
+             fdt_node_check_compatible(fdt, node,
+                                       "xen,multiboot-module" ) == 0 )
+        {
+            last_depth = depth - 1;
+            continue;
+        }
 
         fdt_begin_node(kinfo->fdt, name);
 
-- 
1.7.9.1

Re: [PATCH V2 00/12] arm: support for initial modules (e.g. dom0) and DTB supplied in RAM

[Ian Campbell]

On Tue, 2012-11-13 at 16:22 +0000, Ian Campbell wrote:
> I will post a patch against the boot-wrapper which implements the
> "bootloader" side of this protocol shortly.

See
http://xenbits.xen.org/gitweb/?p=people/ianc/boot-wrapper.git;a=summary

Ian.

Re: [PATCH V2 00/12] arm: support for initial modules (e.g. dom0) and DTB supplied in RAM

[Tim Deegan]

Right.  For 1, 2, 3, 6 and 11:

Acked-by: Tim Deegan <tim@xen.org>

5, 7 and 10 already have my Ack on them.  4, 8, 9 and 12 need some more
attention.

Cheers,

Tim.

Re: [PATCH V2 00/12] arm: support for initial modules (e.g. dom0) and DTB supplied in RAM

[Ian Campbell]

On Thu, 2012-11-29 at 17:59 +0000, Tim Deegan wrote:
> Right.  For 1, 2, 3, 6 and 11:
> 
> Acked-by: Tim Deegan <tim@xen.org>
> 
> 5, 7 and 10 already have my Ack on them.  4, 8, 9 and 12 need some more
> attention.

Thanks!

I'll see what I can sweep through into the tree (perhaps tomorrow) given
this set of acks and then look at addressing your comments on the rest.

Cheers,
Ian.

Re: [PATCH V2 00/12] arm: support for initial modules (e.g. dom0) and DTB supplied in RAM

[Ian Campbell]

On Thu, 2012-11-29 at 18:05 +0000, Ian Campbell wrote:
> On Thu, 2012-11-29 at 17:59 +0000, Tim Deegan wrote:
> > Right.  For 1, 2, 3, 6 and 11:
> > 
> > Acked-by: Tim Deegan <tim@xen.org>
> > 
> > 5, 7 and 10 already have my Ack on them.  4, 8, 9 and 12 need some more
> > attention.
> 
> Thanks!
> 
> I'll see what I can sweep through into the tree (perhaps tomorrow) given
> this set of acks and then look at addressing your comments on the rest.

Thanks, I have applied 1-3 & 7:
arm: Enable build without CONFIG_DTB_FILE
arm: create a raw binary target.
arm: handle xenheap which isn't at the start of RAM.
arm: const-correctness in virt_to_maddr

The rest of the acked ones didn't make sense without some of the unacked
ones.

> 
> Cheers,
> Ian.
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel