Xen devel - Sep 2012 - [PATCH 0/16] arm: support for initial modules (e.g. dom0) and DTB supplied in RAM

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).

In the PoC the modules are listed in the chosen node starting
with /chosen/nr-modules which contains the count and then /chosen/module
%d-{start,end} which gives the physical address of the module
and /chosen/module%d-args which give its command line.

I will post a patch against the boot-wrapper[0] 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''

Until we know what bootloaders are going to become common in the ARM
servers world it hard to know who we should be working with to define a
proper protocol going forward and which bootloaders to supply patches
for. (see the mail with the boot-wrapper patch for more on this).

I suspect that we will inevitably need a tool which takes Xen and the
modules and builds them into a single self exploding image since
bootloader support for any protocol we end up defining is likely to be
spotty at least for the time being.

One thing I''m wondering is whether or not we should duplicate the Linux
zImage header (magic numbers, length etc) at the start of our image too.
That is this:
        
        start:
                        .type   start,#function
                        .rept   7
                        mov     r0, r0
                        .endr
           ARM(         mov     r0, r0          )
           ARM(         b       1f              )
         THUMB(         adr     r12, BSYM(1f)   )
         THUMB(         bx      r12             )
        
                        .word   0x016f2818              @ Magic numbers to help
the loader
                        .word   start                   @ absolute load/run
zImage address
                        .word   _edata                  @ zImage end address
         THUMB(         .thumb                  )
        1:              mov     r7, r1                  @ save architecture ID
                        mov     r8, r2                  @ save atags pointer

It''s pretty trivial but I''m not sure of how much use it is.

[0] git://git.linaro.org/arm/models/boot-wrapper.git

Avoids surprises e.g. when loading via the boot-wrapper.

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

diff --git a/xen/arch/arm/head.S b/xen/arch/arm/head.S
index cdbe011..131cdf9 100644
--- a/xen/arch/arm/head.S
+++ b/xen/arch/arm/head.S
@@ -127,8 +127,26 @@ boot_cpu:
 	add   pc, r0, r10            /* Call PA of function */
 
 hyp:
-	PRINT("- Setting up control registers -\r\n")
 
+	/* Zero BSS On the boot CPU to avoid nasty surprises */
+	teq   r12, #0
+	bne   skip_bss
+
+	PRINT("- Zero BSS -\r\n")
+	ldr   r0, =__bss_start       /* Load start & end of bss */
+	ldr   r1, =__bss_end
+	add   r0, r0, r10            /* Apply physical offset */  
+	add   r1, r1, r10
+	
+	mov   r2, #0
+1:	str   r2, [r0], #4
+	cmp   r0, r1
+	blo   1b
+
+skip_bss:	
+
+	PRINT("- Setting up control registers -\r\n")
+	
 	/* Set up memory attribute type tables */
 	ldr   r0, =MAIR0VAL
 	ldr   r1, =MAIR1VAL
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 4a9d086..410d7db 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -119,6 +119,7 @@ SECTIONS
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
+       __bss_end = .;
   } :text
   _end = . ;
 
-- 
1.7.9.1

This is suitable for direct loading by a bootloader.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 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..f296c2f 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 -R .comment -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

avoids confusion due to overflow etc.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/include/asm-arm/config.h |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 7d02cc7..2a05539 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -69,14 +69,14 @@
  *   - in setup_pagetables() when relocating Xen.
  */
 
-#define XEN_VIRT_START         0x00200000
-#define FIXMAP_ADDR(n)        (0x00400000 + (n) * PAGE_SIZE)
-#define BOOT_MISC_VIRT_START   0x00600000
-#define FRAMETABLE_VIRT_START  0x02000000
-#define XENHEAP_VIRT_START     0x40000000
-#define DOMHEAP_VIRT_START     0x80000000
-
-#define HYPERVISOR_VIRT_START mk_unsigned_long(XEN_VIRT_START)
+#define XEN_VIRT_START         mk_unsigned_long(0x00200000)
+#define FIXMAP_ADDR(n)        (mk_unsigned_long(0x00400000) + (n) * PAGE_SIZE)
+#define BOOT_MISC_VIRT_START   mk_unsigned_long(0x00600000)
+#define FRAMETABLE_VIRT_START  mk_unsigned_long(0x02000000)
+#define XENHEAP_VIRT_START     mk_unsigned_long(0x40000000)
+#define DOMHEAP_VIRT_START     mk_unsigned_long(0x80000000)
+
+#define HYPERVISOR_VIRT_START  XEN_VIRT_START
 
 #define DOMHEAP_ENTRIES        1024  /* 1024 2MB mapping slots */
 
-- 
1.7.9.1

Also refactor page_to_virt somewhat in an attempt to make it clearer
what is happening.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/include/asm-arm/mm.h |   24 +++++++++++++++++++-----
 1 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index b37bd35..6498322 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -214,17 +214,31 @@ 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)
 {
+    unsigned long va;
+    const unsigned long offset +       
(xenheap_mfn_start-frametable_base_mfn)*sizeof(*pg);
+
+    /*
+     * Dividing by this on both top and bottom factors out the largest
+     * common factor of 2 which helps the compiler to use smaller shifts.
+     */
+    const unsigned long lcd = (sizeof(*pg) & -sizeof(*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))));
 
+    va = (unsigned long)pg;
+    va = XENHEAP_VIRT_START +
+        ((va - FRAMETABLE_VIRT_START - offset) / (sizeof(*pg) / lcd)) *
+        (PAGE_SIZE / lcd);
+    return (void *)va;
 }
 
 struct domain *page_get_owner_and_reference(struct page_info *page);
-- 
1.7.9.1

It''s not realy got any DT functionality in it and its only caller is
setup_pagetables.

Put it here because future patches want to incorporate of the module
layout in memory and I''d like to confine that to setup.c

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/mm.c             |    5 +----
 xen/arch/arm/setup.c          |   35 ++++++++++++++++++++++++++++++++++-
 xen/common/device_tree.c      |   32 --------------------------------
 xen/include/asm-arm/mm.h      |    2 +-
 xen/include/xen/device_tree.h |    1 -
 5 files changed, 36 insertions(+), 39 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 08bc55b..52bb5c7 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -206,15 +206,12 @@ void unmap_domain_page(const void *va)
 
 /* Boot-time pagetable setup.
  * Changes here may need matching changes in head.S */
-void __init setup_pagetables(unsigned long boot_phys_offset)
+void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
 {
-    paddr_t xen_paddr;
     unsigned long dest_va;
     lpae_t pte, *p;
     int i;
 
-    xen_paddr = device_tree_get_xen_paddr();
-
     /* Map the destination in the boot misc area. */
     dest_va = BOOT_MISC_VIRT_START;
     pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index c4ca270..b466875 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -37,6 +37,7 @@
 #include <asm/current.h>
 #include <asm/setup.h>
 #include <asm/vfp.h>
+#include <asm/early_printk.h>
 #include "gic.h"
 
 static __attribute_used__ void init_done(void)
@@ -66,6 +67,38 @@ static void __init processor_id(void)
            READ_CP32(ID_ISAR3), READ_CP32(ID_ISAR4), READ_CP32(ID_ISAR5));
 }
 
+/**
+ * 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.
+ */
+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;
+    int i;
+
+    min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) &
~(XEN_PADDR_ALIGN-1);
+
+    /* Find the highest bank with enough space. */
+    for ( i = 0; i < mi->nr_banks; i++ )
+    {
+        if ( mi->bank[i].size >= min_size )
+        {
+            t = mi->bank[i].start + mi->bank[i].size - min_size;
+            if ( t > paddr )
+                paddr = t;
+        }
+    }
+
+    if ( !paddr )
+        early_panic("Not enough memory to relocate Xen\n");
+
+    return paddr;
+}
+
 static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
 {
     paddr_t ram_start;
@@ -155,7 +188,7 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     cmdline_parse(device_tree_bootargs(fdt));
 
-    setup_pagetables(boot_phys_offset);
+    setup_pagetables(boot_phys_offset, get_xen_paddr());
 
 #ifdef EARLY_UART_ADDRESS
     /* Map the UART */
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 04619f4..3d1f0f4 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -273,38 +273,6 @@ size_t __init device_tree_early_init(const void *fdt)
     return fdt_totalsize(fdt);
 }
 
-/**
- * device_tree_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.
- */
-paddr_t __init device_tree_get_xen_paddr(void)
-{
-    struct dt_mem_info *mi = &early_info.mem;
-    paddr_t min_size;
-    paddr_t paddr = 0, t;
-    int i;
-
-    min_size = (_end - _start + (XEN_PADDR_ALIGN-1)) &
~(XEN_PADDR_ALIGN-1);
-
-    /* Find the highest bank with enough space. */
-    for ( i = 0; i < mi->nr_banks; i++ )
-    {
-        if ( mi->bank[i].size >= min_size )
-        {
-            t = mi->bank[i].start + mi->bank[i].size - min_size;
-            if ( t > paddr )
-                paddr = t;
-        }
-    }
-
-    if ( !paddr )
-        early_panic("Not enough memory to relocate Xen\n");
-
-    return paddr;
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 6498322..b0e5a67 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -138,7 +138,7 @@ extern unsigned long max_page;
 extern unsigned long total_pages;
 
 /* Boot-time pagetable setup */
-extern void setup_pagetables(unsigned long boot_phys_offset);
+extern void setup_pagetables(unsigned long boot_phys_offset, paddr_t
xen_paddr);
 /* MMU setup for seccondary CPUS (which already have paging enabled) */
 extern void __cpuinit mmu_init_secondary_cpu(void);
 /* Set up the xenheap: up to 1GB of contiguous, always-mapped memory.
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 8e1626c..4d010c0 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -39,7 +39,6 @@ extern struct dt_early_info early_info;
 extern void *device_tree_flattened;
 
 size_t device_tree_early_init(const void *fdt);
-paddr_t device_tree_get_xen_paddr(void);
 
 void device_tree_get_reg(const u32 **cell, u32 address_cells, u32 size_cells,
                          u64 *start, u64 *size);
-- 
1.7.9.1

The bootloader should populate /chosen/nr-modules with the number of
modules and then for each module 0..nr-modules-1 it should populate
/chosen/moduleN-{start,end} with the physical address of the module.

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

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/common/device_tree.c      |   57 +++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/device_tree.h |    7 +++++
 2 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 3d1f0f4..226e3f3 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -229,6 +229,61 @@ 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 i, nr_modules;
+
+    prop = fdt_get_property(fdt, node, "nr-modules", NULL);
+    if ( !prop )
+    {
+        early_info.modules.nr_mods = 0;
+        return;
+    }
+
+    cell = (const u32 *)prop->data;
+    get_val(&cell, 1, (u64*)&nr_modules);
+
+    if ( nr_modules > NR_MODULES )
+    {
+        early_panic("too many modules %d > %d\n", nr_modules,
NR_MODULES);
+    }
+
+    for ( i = 0; i < nr_modules; i++ )
+    {
+        struct membank *mod = &early_info.modules.module[i];
+        /* longest prop name is "start", single digit numbers of
modules */
+        char propname[strlen("moduleX-start") + 1];
+
+        BUILD_BUG_ON(NR_MODULES > 9);
+
+        snprintf(propname, sizeof(propname), "module%d-start", i+1);
+        prop = fdt_get_property(fdt, node, propname, NULL);
+        if ( !prop )
+            early_panic("no start for module %d\n", i);
+
+        cell = (const u32 *)prop->data;
+        device_tree_get_reg(&cell, address_cells, size_cells,
+                            &mod->start, &size);
+
+        snprintf(propname, sizeof(propname), "module%d-end", i+1);
+        prop = fdt_get_property(fdt, node, propname, NULL);
+        if ( !prop )
+            early_panic("no end for module %d\n", i);
+
+        cell = (const u32 *)prop->data;
+        device_tree_get_reg(&cell, address_cells, size_cells,
+                            &mod->size, &size);
+        mod->size -= mod->start;
+    }
+
+    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 +291,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..585fcc9 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,14 @@ struct dt_mem_info {
     struct membank bank[NR_MEM_BANKS];
 };
 
+struct dt_module_info {
+    int nr_mods;
+    struct membank module[NR_MODULES];
+};
+
 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

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>
---
 xen/arch/arm/setup.c |   66 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 59 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index b466875..5f8a3d7 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -67,17 +67,58 @@ 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 0.
+ */
+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;
+        paddr_t mod_e;
+
+        mod_s = mi->module[i].start;
+        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);
@@ -85,17 +126,28 @@ 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, 0);
+            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);
+
     return paddr;
 }
 
-- 
1.7.9.1

This argument to alloc_boot_pages is "pfn_align" and not an order.
We''ve been lucky until now that the area given to the boot allocator
happened to be properly aligned and this allocation was early enough
to benefit.

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

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 52bb5c7..aafc48c 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -353,7 +353,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t
pe)
 
     /* Round up to 32M boundary */
     frametable_size = (frametable_size + 0x1ffffff) & ~0x1ffffff;
-    base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 5);
+    base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT,
32<<(20-12));
     create_mappings(FRAMETABLE_VIRT_START, base_mfn, frametable_size >>
PAGE_SHIFT);
 
     memset(&frame_table[0], 0, nr_pages * sizeof(struct page_info));
-- 
1.7.9.1

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

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 5f8a3d7..369e164 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -40,6 +40,8 @@
 #include <asm/early_printk.h>
 #include "gic.h"
 
+static __initdata paddr_t xen_paddr;
+
 static __attribute_used__ void init_done(void)
 {
     free_init_memory();
@@ -72,7 +74,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 0.
+ * For non-recursive callers first_mod should normally be 0 (all
+ * modules) or -1 (all modules and Xen itself).
  */
 static paddr_t __init consider_modules(paddr_t s, paddr_t e,
                                        uint32_t size, paddr_t align,
@@ -92,8 +95,17 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
         paddr_t mod_s;
         paddr_t mod_e;
 
-        mod_s = mi->module[i].start;
-        mod_e = mod_s + mi->module[i].size;
+        /* module "-1" is Xen itself. */
+        if ( i == -1 )
+        {
+            mod_s = xen_paddr;
+            mod_e = mod_s + ((_end - _start + (XEN_PADDR_ALIGN-1)) &
~(XEN_PADDR_ALIGN-1));
+        }
+        else
+        {
+            mod_s = mi->module[i].start;
+            mod_e = mod_s + mi->module[i].size;
+        }
 
         if ( s < mod_e && mod_s < e )
         {
@@ -108,6 +120,46 @@ 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 = -1; i < mi->nr_mods; i++ )
+    {
+        paddr_t mod_s;
+        paddr_t mod_e;
+
+        /* module "-1" is Xen itself. */
+        if ( i == -1 )
+        {
+            mod_s = xen_paddr;
+            mod_e = mod_s + ((_end - _start + (XEN_PADDR_ALIGN-1)) &
~(XEN_PADDR_ALIGN-1));
+        }
+        else
+        {
+            mod_s = mi->module[i].start;
+            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
  *
@@ -156,6 +208,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;
@@ -171,22 +224,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, -1);
+        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
@@ -210,8 +278,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);
@@ -240,7 +330,8 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     cmdline_parse(device_tree_bootargs(fdt));
 
-    setup_pagetables(boot_phys_offset, get_xen_paddr());
+    xen_paddr = get_xen_paddr();
+    setup_pagetables(boot_phys_offset, xen_paddr);
 
 #ifdef EARLY_UART_ADDRESS
     /* Map the UART */
-- 
1.7.9.1

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

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 369e164..85487fe 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -218,6 +218,8 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t
dtb_size)
      * TODO: only using the first RAM bank for now.  The heaps and the
      * frame table assume RAM is physically contiguous.
      */
+    if ( early_info.mem.nr_banks > 1 )
+        early_printk("WARNING: Only using first bank of memory\n");
     ram_start = early_info.mem.bank[0].start;
     ram_size  = early_info.mem.bank[0].size;
     ram_end = ram_start + ram_size;
-- 
1.7.9.1

These are used in virt_to_page and page_to_virt so I imagine there''s
some small benefit to this (but I''ve not measured)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/mm.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index aafc48c..368911b 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -47,11 +47,12 @@ uint64_t boot_httbr;
 static paddr_t phys_offset;
 
 /* Limits of the Xen heap */
-unsigned long xenheap_mfn_start, xenheap_mfn_end;
-unsigned long xenheap_virt_end;
+unsigned long xenheap_mfn_start __read_mostly;
+unsigned long xenheap_mfn_end __read_mostly;
+unsigned long xenheap_virt_end __read_mostly;
 
-unsigned long frametable_base_mfn;
-unsigned long frametable_virt_end;
+unsigned long frametable_base_mfn __read_mostly;
+unsigned long frametable_virt_end __read_mostly;
 
 unsigned long max_page;
 
-- 
1.7.9.1

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/include/asm-arm/mm.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index b0e5a67..7440fe5 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -179,9 +179,9 @@ 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);
+    uint64_t par = va_to_par((const uint32_t)va);
     return (par & PADDR_MASK & PAGE_MASK) | ((unsigned long) va &
~PAGE_MASK);
 }
 
-- 
1.7.9.1

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

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 226e3f3..60f3a03 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;
@@ -139,7 +141,7 @@ int device_tree_for_each_node(const void *fdt,
  */
 const char *device_tree_bootargs(const void *fdt)
 {
-    int node; 
+    int node;
     const struct fdt_property *prop;
 
     node = fdt_path_offset(fdt, "/chosen");
-- 
1.7.9.1

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

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 2d56130..d82c3e1 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,13 +92,16 @@ 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)
         return -EINVAL;
@@ -106,16 +109,24 @@ static int kernel_try_zimage_prepare(struct kernel_info
*info)
     start = zimage[ZIMAGE_START_OFFSET/4];
     end = zimage[ZIMAGE_END_OFFSET/4];
 
+    if ( end > addr + size )
+        return -EINVAL;
+
     clear_fixmap(FIXMAP_MISC);
 
     /*
      * Check for an appended DTB.
      */
-    copy_from_paddr(&dtb_hdr, KERNEL_FLASH_ADDRESS + end - start,
sizeof(dtb_hdr), DEV_SHARED);
+    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 +153,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 +182,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[0].start;
+        size = early_info.modules.module[0].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

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 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 85487fe..b23bdc1 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -69,6 +69,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 = 0; 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

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

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

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e96ed10..2b65637 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -88,6 +88,8 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
 {
     int prop;
 
+    int had_mod1_args = 0;
+
     for ( prop = fdt_first_property_offset(fdt, node);
           prop >= 0;
           prop = fdt_next_property_offset(fdt, prop) )
@@ -104,15 +106,30 @@ 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:
+         *
+         * * replace bootargs with value from module1-args, falling
+         *   back to xen,dom0-bootargs if not present.
+         * * remove all other module*.
          */
         if ( device_tree_node_matches(fdt, node, "chosen") )
         {
             if ( strcmp(prop_name, "bootargs") == 0 )
                 continue;
-            if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
+            if ( strcmp(prop_name, "module1-args") == 0 )
+            {
                 prop_name = "bootargs";
+                had_mod1_args = 1;
+            }
+            if ( strncmp(prop_name, "module",
strlen("module")) == 0 )
+                 continue;
+            if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
+            {
+                if ( had_mod1_args )
+                    continue;
+                else
+                    prop_name = "bootargs";
+            }
         }
         /*
          * In a memory node: adjust reg property.
-- 
1.7.9.1

At 13:30 +0000 on 03 Sep (1346679041), Ian Campbell
wrote:
> Avoids surprises e.g. when loading via the boot-wrapper.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>

At 13:30 +0000 on 03 Sep (1346679042), Ian Campbell
wrote:
> +# 
> +$(TARGET).bin: $(TARGET)-syms
> +	objcopy -O binary -R .comment -S $< $@
Why remove .comment in particular and not the other non-load sections?

Tim.

At 13:30 +0000 on 03 Sep (1346679043), Ian Campbell
wrote:
> avoids confusion due to overflow etc.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>

On Thu, 2012-09-06 at 11:01 +0100, Tim Deegan wrote:
> At 13:30 +0000 on 03 Sep (1346679042), Ian Campbell wrote:
> > +# 
> > +$(TARGET).bin: $(TARGET)-syms
> > +	objcopy -O binary -R .comment -S $< $@
> 
> Why remove .comment in particular and not the other non-load sections?
Because wherever I copied the rune from (probably Linux) did ;-)

Ian.

At 13:30 +0000 on 03 Sep (1346679044), Ian Campbell
wrote:
> Also refactor page_to_virt somewhat in an attempt to make it clearer
> what is happening.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/include/asm-arm/mm.h |   24 +++++++++++++++++++-----
>  1 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index b37bd35..6498322 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -214,17 +214,31 @@ 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)
>  {
> +    unsigned long va;
> +    const unsigned long offset > +       
(xenheap_mfn_start-frametable_base_mfn)*sizeof(*pg);
> +
> +    /*
> +     * Dividing by this on both top and bottom factors out the largest
> +     * common factor of 2 which helps the compiler to use smaller shifts.
> +     */
> +    const unsigned long lcd = (sizeof(*pg) & -sizeof(*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))));
>  
> +    va = (unsigned long)pg;
> +    va = XENHEAP_VIRT_START +
> +        ((va - FRAMETABLE_VIRT_START - offset) / (sizeof(*pg) / lcd)) *
> +        (PAGE_SIZE / lcd);
> +    return (void *)va;
This function is getting a bit too confusing.  I think we should just
use page_to_mfn and mfn_to_virt instead: the magic lcd trick eliminates
a shift but having to construct "offset" to match it adds one.

Tim.

At 13:30 +0000 on 03 Sep (1346679045), Ian Campbell
wrote:
> It''s not realy got any DT functionality in it and its only caller
is
> setup_pagetables.
> 
> Put it here because future patches want to incorporate of the module
> layout in memory and I''d like to confine that to setup.c
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>

At 13:30 +0000 on 03 Sep (1346679046), Ian Campbell
wrote:
> The bootloader should populate /chosen/nr-modules with the number of
> modules and then for each module 0..nr-modules-1 it should populate
> /chosen/moduleN-{start,end} with the physical address of the module.
The code below looks like it''s expecting the modules to be numbered
from
1, not from 0.

Tim.
> The hypervisor allows for 2 modules (kernel and initrd). Currently we
> don''t do anything with them.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/common/device_tree.c      |   57
+++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h |    7 +++++
>  2 files changed, 64 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 3d1f0f4..226e3f3 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -229,6 +229,61 @@ 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 i, nr_modules;
> +
> +    prop = fdt_get_property(fdt, node, "nr-modules", NULL);
> +    if ( !prop )
> +    {
> +        early_info.modules.nr_mods = 0;
> +        return;
> +    }
> +
> +    cell = (const u32 *)prop->data;
> +    get_val(&cell, 1, (u64*)&nr_modules);
> +
> +    if ( nr_modules > NR_MODULES )
> +    {
> +        early_panic("too many modules %d > %d\n", nr_modules,
NR_MODULES);
> +    }
> +
> +    for ( i = 0; i < nr_modules; i++ )
> +    {
> +        struct membank *mod = &early_info.modules.module[i];
> +        /* longest prop name is "start", single digit numbers of
modules */
> +        char propname[strlen("moduleX-start") + 1];
> +
> +        BUILD_BUG_ON(NR_MODULES > 9);
> +
> +        snprintf(propname, sizeof(propname), "module%d-start",
i+1);
> +        prop = fdt_get_property(fdt, node, propname, NULL);
> +        if ( !prop )
> +            early_panic("no start for module %d\n", i);
> +
> +        cell = (const u32 *)prop->data;
> +        device_tree_get_reg(&cell, address_cells, size_cells,
> +                            &mod->start, &size);
> +
> +        snprintf(propname, sizeof(propname), "module%d-end",
i+1);
> +        prop = fdt_get_property(fdt, node, propname, NULL);
> +        if ( !prop )
> +            early_panic("no end for module %d\n", i);
> +
> +        cell = (const u32 *)prop->data;
> +        device_tree_get_reg(&cell, address_cells, size_cells,
> +                            &mod->size, &size);
> +        mod->size -= mod->start;
> +    }
> +
> +    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 +291,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..585fcc9 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,14 @@ struct dt_mem_info {
>      struct membank bank[NR_MEM_BANKS];
>  };
>  
> +struct dt_module_info {
> +    int nr_mods;
> +    struct membank module[NR_MODULES];
> +};
> +
>  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

On Thu, 2012-09-06 at 12:47 +0100, Tim Deegan wrote:
> At 13:30 +0000 on 03 Sep (1346679046), Ian Campbell wrote:
> > The bootloader should populate /chosen/nr-modules with the number of
> > modules and then for each module 0..nr-modules-1 it should populate
> > /chosen/moduleN-{start,end} with the physical address of the module.
> 
> The code below looks like it''s expecting the modules to be
numbered from
> 1, not from 0.
Yes, looks like I''d forgotten that by the time I wrote the commit
message. Thanks.

Ian.
> 
> Tim.
> 
> > The hypervisor allows for 2 modules (kernel and initrd). Currently we
> > don''t do anything with them.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/common/device_tree.c      |   57
+++++++++++++++++++++++++++++++++++++++++
> >  xen/include/xen/device_tree.h |    7 +++++
> >  2 files changed, 64 insertions(+), 0 deletions(-)
> > 
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 3d1f0f4..226e3f3 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -229,6 +229,61 @@ 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 i, nr_modules;
> > +
> > +    prop = fdt_get_property(fdt, node, "nr-modules", NULL);
> > +    if ( !prop )
> > +    {
> > +        early_info.modules.nr_mods = 0;
> > +        return;
> > +    }
> > +
> > +    cell = (const u32 *)prop->data;
> > +    get_val(&cell, 1, (u64*)&nr_modules);
> > +
> > +    if ( nr_modules > NR_MODULES )
> > +    {
> > +        early_panic("too many modules %d > %d\n",
nr_modules, NR_MODULES);
> > +    }
> > +
> > +    for ( i = 0; i < nr_modules; i++ )
> > +    {
> > +        struct membank *mod = &early_info.modules.module[i];
> > +        /* longest prop name is "start", single digit
numbers of modules */
> > +        char propname[strlen("moduleX-start") + 1];
> > +
> > +        BUILD_BUG_ON(NR_MODULES > 9);
> > +
> > +        snprintf(propname, sizeof(propname),
"module%d-start", i+1);
> > +        prop = fdt_get_property(fdt, node, propname, NULL);
> > +        if ( !prop )
> > +            early_panic("no start for module %d\n", i);
> > +
> > +        cell = (const u32 *)prop->data;
> > +        device_tree_get_reg(&cell, address_cells, size_cells,
> > +                            &mod->start, &size);
> > +
> > +        snprintf(propname, sizeof(propname),
"module%d-end", i+1);
> > +        prop = fdt_get_property(fdt, node, propname, NULL);
> > +        if ( !prop )
> > +            early_panic("no end for module %d\n", i);
> > +
> > +        cell = (const u32 *)prop->data;
> > +        device_tree_get_reg(&cell, address_cells, size_cells,
> > +                            &mod->size, &size);
> > +        mod->size -= mod->start;
> > +    }
> > +
> > +    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 +291,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..585fcc9 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,14 @@ struct dt_mem_info {
> >      struct membank bank[NR_MEM_BANKS];
> >  };
> >  
> > +struct dt_module_info {
> > +    int nr_mods;
> > +    struct membank module[NR_MODULES];
> > +};
> > +
> >  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

At 13:30 +0000 on 03 Sep (1346679047), Ian Campbell
wrote:
> @@ -85,17 +126,28 @@ 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, 0);
> +            if ( !e )continue;
Missing newline.  With that fixed,
Acked-by: Tim Deegan <tim@xen.org>

At 13:30 +0000 on 03 Sep (1346679048), Ian Campbell
wrote:
> This argument to alloc_boot_pages is "pfn_align" and not an
order.
> We''ve been lucky until now that the area given to the boot
allocator
> happened to be properly aligned and this allocation was early enough
> to benefit.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>

At 13:30 +0000 on 03 Sep (1346679049), Ian Campbell
wrote:
> @@ -72,7 +74,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 0.
> + * For non-recursive callers first_mod should normally be 0 (all
> + * modules) or -1 (all modules and Xen itself).
Oh dear.  I think that having Xen be module 0 would make this and the
module-parsing code much cleaner. 

Tim.

At 13:30 +0000 on 03 Sep (1346679050), Ian Campbell
wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>

At 13:30 +0000 on 03 Sep (1346679051), Ian Campbell
wrote:
> These are used in virt_to_page and page_to_virt so I imagine
there''s
> some small benefit to this (but I''ve not measured)
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>

At 13:30 +0000 on 03 Sep (1346679052), Ian Campbell
wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/include/asm-arm/mm.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index b0e5a67..7440fe5 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -179,9 +179,9 @@ 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);
> +    uint64_t par = va_to_par((const uint32_t)va);
This second const is unnecessary (or else you also need one in the cast
on the next line).  For the first one, though:

Acked-by: Tim Deegan <tim@xen.org> 
>      return (par & PADDR_MASK & PAGE_MASK) | ((unsigned long) va
& ~PAGE_MASK);
>  }
>  
> -- 
> 1.7.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

At 13:30 +0000 on 03 Sep (1346679053), Ian Campbell
wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/common/device_tree.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 226e3f3..60f3a03 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;
> @@ -139,7 +141,7 @@ int device_tree_for_each_node(const void *fdt,
>   */
>  const char *device_tree_bootargs(const void *fdt)
>  {
> -    int node; 
> +    int node;
Unrelated change - maybe in a cset of its own, if that''s not too
ridiculous?

Tim.

At 13:30 +0000 on 03 Sep (1346679054), 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)
>          return -EINVAL;
> @@ -106,16 +109,24 @@ static int kernel_try_zimage_prepare(struct
kernel_info *info)
>      start = zimage[ZIMAGE_START_OFFSET/4];
>      end = zimage[ZIMAGE_END_OFFSET/4];
>  
> +    if ( end > addr + size )
> +        return -EINVAL;
> +
>      clear_fixmap(FIXMAP_MISC);
No clear_fixmap() on the error path?  I see there isn''t one on the
existing error path above, but I suspect that''s not deliberate.
>  
>      /*
>       * Check for an appended DTB.
>       */
> -    copy_from_paddr(&dtb_hdr, KERNEL_FLASH_ADDRESS + end - start,
sizeof(dtb_hdr), DEV_SHARED);
> +    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;
There ought to be a bounds check before the copy_from_paddr as well
(though I suppose there''s not much to do except fail more gracefully).

Tim.

At 13:30 +0000 on 03 Sep (1346679056), Ian Campbell
wrote:
> Ideally this would use module1-args iff the kernel came from
> module1-{start,end} and the existing xen,dom0-bootargs if the kernel
> came from flash, but this approach is simpler and has the sme effect
> in practice.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/domain_build.c |   23 ++++++++++++++++++++---
>  1 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index e96ed10..2b65637 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -88,6 +88,8 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
>  {
>      int prop;
>  
> +    int had_mod1_args = 0;
> +
>      for ( prop = fdt_first_property_offset(fdt, node);
>            prop >= 0;
>            prop = fdt_next_property_offset(fdt, prop) )
> @@ -104,15 +106,30 @@ 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:
> +         *
> +         * * replace bootargs with value from module1-args, falling
> +         *   back to xen,dom0-bootargs if not present.
> +         * * remove all other module*.
>           */
>          if ( device_tree_node_matches(fdt, node, "chosen") )
>          {
>              if ( strcmp(prop_name, "bootargs") == 0 )
>                  continue;
> -            if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
> +            if ( strcmp(prop_name, "module1-args") == 0 )
> +            {
>                  prop_name = "bootargs";
> +                had_mod1_args = 1;
> +            }
> +            if ( strncmp(prop_name, "module",
strlen("module")) == 0 )
> +                 continue;
ITYM "else if" here, or the module1-args property gets dropped too,
doesn''t it?  

Tim.
> +            if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
> +            {
> +                if ( had_mod1_args )
> +                    continue;
> +                else
> +                    prop_name = "bootargs";
> +            }
>          }
>          /*
>           * In a memory node: adjust reg property.
> -- 
> 1.7.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

At 13:30 +0000 on 03 Sep (1346679055), Ian Campbell
wrote:
> +void __init discard_initial_modules(void)
> +{
> +    struct dt_module_info *mi = &early_info.modules;
> +    int 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);
> +
> +        init_domheap_pages(s, e);
Is there a risk of weirdness from adding non-superpage-aligned memory to
the domheap here, given that map_domain_page always uses 2MB mappings?
> +    }
> +
> +    mi->nr_mods = 0;
> +}

On Thu, 2012-09-06 at 14:50 +0100, Tim Deegan wrote:
> At 13:30 +0000 on 03 Sep (1346679056), Ian Campbell wrote:
> > Ideally this would use module1-args iff the kernel came from
> > module1-{start,end} and the existing xen,dom0-bootargs if the kernel
> > came from flash, but this approach is simpler and has the sme effect
> > in practice.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/domain_build.c |   23 ++++++++++++++++++++---
> >  1 files changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index e96ed10..2b65637 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -88,6 +88,8 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
> >  {
> >      int prop;
> >  
> > +    int had_mod1_args = 0;
> > +
> >      for ( prop = fdt_first_property_offset(fdt, node);
> >            prop >= 0;
> >            prop = fdt_next_property_offset(fdt, prop) )
> > @@ -104,15 +106,30 @@ 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:
> > +         *
> > +         * * replace bootargs with value from module1-args, falling
> > +         *   back to xen,dom0-bootargs if not present.
> > +         * * remove all other module*.
> >           */
> >          if ( device_tree_node_matches(fdt, node, "chosen")
)
> >          {
> >              if ( strcmp(prop_name, "bootargs") == 0 )
> >                  continue;
> > -            if ( strcmp(prop_name, "xen,dom0-bootargs") ==
0 )
> > +            if ( strcmp(prop_name, "module1-args") == 0 )
> > +            {
> >                  prop_name = "bootargs";
> > +                had_mod1_args = 1;
> > +            }
> > +            if ( strncmp(prop_name, "module",
strlen("module")) == 0 )
> > +                 continue;
> 
> ITYM "else if" here, or the module1-args property gets dropped
too,
> doesn''t it?  
The intention was to filter "module*" from the DTB altogether, since
they were intended for Xen not dom0. All that should remain is
module1-args which has been renamed to bootargs which is what dom0
expects.

That isn''t really mentioned in the commit log but I did at least
comment
it above ;-)
> 
> Tim.
> 
> > +            if ( strcmp(prop_name, "xen,dom0-bootargs") ==
0 )
> > +            {
> > +                if ( had_mod1_args )
> > +                    continue;
> > +                else
> > +                    prop_name = "bootargs";
> > +            }
> >          }
> >          /*
> >           * In a memory node: adjust reg property.
> > -- 
> > 1.7.9.1
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

On Thu, 2012-09-06 at 14:53 +0100, Tim Deegan wrote:
> At 13:30 +0000 on 03 Sep (1346679055), Ian Campbell wrote:
> > +void __init discard_initial_modules(void)
> > +{
> > +    struct dt_module_info *mi = &early_info.modules;
> > +    int 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);
> > +
> > +        init_domheap_pages(s, e);
> 
> Is there a risk of weirdness from adding non-superpage-aligned memory to
> the domheap here, given that map_domain_page always uses 2MB mappings?
I hadn''t thought about this.

These regions will mesh precisely with what setup_mm has added and
therefore the result is that the entire 2MB is on the heap, I think.

These non-aligned looking regions are actually within larger regions of
RAM so I don''t think there is any danger of actually mapping some
non-RAM as part of such a 2MB mapping?

Ian.

At 14:55 +0100 on 06 Sep (1346943300), Ian Campbell
wrote:
> > >          if ( device_tree_node_matches(fdt, node,
"chosen") )
> > >          {
> > >              if ( strcmp(prop_name, "bootargs") == 0 )
> > >                  continue;
> > > -            if ( strcmp(prop_name,
"xen,dom0-bootargs") == 0 )
> > > +            if ( strcmp(prop_name, "module1-args") ==
0 )
> > > +            {
> > >                  prop_name = "bootargs";
> > > +                had_mod1_args = 1;
> > > +            }
> > > +            if ( strncmp(prop_name, "module",
strlen("module")) == 0 )
> > > +                 continue;
> > 
> > ITYM "else if" here, or the module1-args property gets
dropped too,
> > doesn''t it?  
> 
> The intention was to filter "module*" from the DTB altogether,
since
> they were intended for Xen not dom0. All that should remain is
> module1-args which has been renamed to bootargs which is what dom0
> expects.
Argh, of course now prop_name''s been clobbered it won''t be
filtered. :)
Sorry for the noise.

Acked-by: Tim Deegan <tim@xen.org>, though I imagine you''ll want
David''s ack rather than mine.

Tim.

On Thu, 2012-09-06 at 14:58 +0100, Tim Deegan wrote:
> At 14:55 +0100 on 06 Sep (1346943300), Ian Campbell wrote:
> > > >          if ( device_tree_node_matches(fdt, node,
"chosen") )
> > > >          {
> > > >              if ( strcmp(prop_name, "bootargs") ==
0 )
> > > >                  continue;
> > > > -            if ( strcmp(prop_name,
"xen,dom0-bootargs") == 0 )
> > > > +            if ( strcmp(prop_name,
"module1-args") == 0 )
> > > > +            {
> > > >                  prop_name = "bootargs";
> > > > +                had_mod1_args = 1;
> > > > +            }
> > > > +            if ( strncmp(prop_name, "module",
strlen("module")) == 0 )
> > > > +                 continue;
> > > 
> > > ITYM "else if" here, or the module1-args property gets
dropped too,
> > > doesn''t it?  
> > 
> > The intention was to filter "module*" from the DTB
altogether, since
> > they were intended for Xen not dom0. All that should remain is
> > module1-args which has been renamed to bootargs which is what dom0
> > expects.
> 
> Argh, of course now prop_name''s been clobbered it won''t
be filtered. :)
> Sorry for the noise.
Actually I hadn''t got quite what you were suggesting, and it is a bit
subtle now you put it that way. An else if would probably help the code
be self documenting.
> 
> Acked-by: Tim Deegan <tim@xen.org>, though I imagine you''ll
want
> David''s ack rather than mine.
> 
> Tim.

At 14:57 +0100 on 06 Sep (1346943456), Ian Campbell
wrote:
> On Thu, 2012-09-06 at 14:53 +0100, Tim Deegan wrote:
> > At 13:30 +0000 on 03 Sep (1346679055), Ian Campbell wrote:
> > > +void __init discard_initial_modules(void)
> > > +{
> > > +    struct dt_module_info *mi = &early_info.modules;
> > > +    int 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);
> > > +
> > > +        init_domheap_pages(s, e);
> > 
> > Is there a risk of weirdness from adding non-superpage-aligned memory
to
> > the domheap here, given that map_domain_page always uses 2MB mappings?
> 
> I hadn''t thought about this.
> 
> These regions will mesh precisely with what setup_mm has added and
> therefore the result is that the entire 2MB is on the heap, I think.
> 
> These non-aligned looking regions are actually within larger regions of
> RAM so I don''t think there is any danger of actually mapping some
> non-RAM as part of such a 2MB mapping?
Righto.  We also might care about accidental r/w mappings of r/o things,
but since Xen itself is 32MB-aligned that''s OK.  I think for now this
is
all fine, so:

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

though it will need adjustment if Xen becomes module 0, of course. :)

Cheers,

Tim.

On 03/09/12 14:30, Ian Campbell wrote:
> Ideally this would use module1-args iff the kernel came from
> module1-{start,end} and the existing xen,dom0-bootargs if the kernel
> came from flash, but this approach is simpler and has the sme effect
> in practice.
Is module1-args defined in a spec somewhere?

If the DT has xen,dom0-bootargs followed by module1-args you will end up
with two bootargs nodes.  Suggest preferring the first one found and
discard the other.
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/domain_build.c |   23 ++++++++++++++++++++---
>  1 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index e96ed10..2b65637 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -88,6 +88,8 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
>  {
>      int prop;
>  
> +    int had_mod1_args = 0;
> +
>      for ( prop = fdt_first_property_offset(fdt, node);
>            prop >= 0;
>            prop = fdt_next_property_offset(fdt, prop) )
> @@ -104,15 +106,30 @@ 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:
> +         *
> +         * * replace bootargs with value from module1-args, falling
> +         *   back to xen,dom0-bootargs if not present.
> +         * * remove all other module*.
>           */
>          if ( device_tree_node_matches(fdt, node, "chosen") )
>          {
>              if ( strcmp(prop_name, "bootargs") == 0 )
>                  continue;
> -            if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
> +            if ( strncmp(prop_name, "module",
strlen("module")) == 0 )
> +                 continue;
Clearer to write this as:

    if ( strncmp(prop_name, "module", strlen("module")) == 0
)
        if ( strcmp(prop_name, "module1-args") == 0 )
        {
            prop_name = "bootargs";
            had_mod1_args = 1;
        }
        else
            continue;
    }
> +            if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 )
> +            {
> +                if ( had_mod1_args )
> +                    continue;
> +                else
> +                    prop_name = "bootargs";
> +            }
>          }
>          /*
>           * In a memory node: adjust reg property.

On Thu, 2012-09-06 at 15:19 +0100, David Vrabel wrote:
> On 03/09/12 14:30, Ian Campbell wrote:
> > Ideally this would use module1-args iff the kernel came from
> > module1-{start,end} and the existing xen,dom0-bootargs if the kernel
> > came from flash, but this approach is simpler and has the sme effect
> > in practice.
> 
> Is module1-args defined in a spec somewhere?
Nope, I made it up as a PoC. I described it a little bit in the 0/16
mail.
> If the DT has xen,dom0-bootargs followed by module1-args you will end up
> with two bootargs nodes.  Suggest preferring the first one found and
> discard the other.
Yes.
> 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/domain_build.c |   23 ++++++++++++++++++++---
> >  1 files changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index e96ed10..2b65637 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -88,6 +88,8 @@ static int write_properties(struct domain *d, struct
kernel_info *kinfo,
> >  {
> >      int prop;
> >  
> > +    int had_mod1_args = 0;
> > +
> >      for ( prop = fdt_first_property_offset(fdt, node);
> >            prop >= 0;
> >            prop = fdt_next_property_offset(fdt, prop) )
> > @@ -104,15 +106,30 @@ 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:
> > +         *
> > +         * * replace bootargs with value from module1-args, falling
> > +         *   back to xen,dom0-bootargs if not present.
> > +         * * remove all other module*.
> >           */
> >          if ( device_tree_node_matches(fdt, node, "chosen")
)
> >          {
> >              if ( strcmp(prop_name, "bootargs") == 0 )
> >                  continue;
> > -            if ( strcmp(prop_name, "xen,dom0-bootargs") ==
0 )
> > +            if ( strncmp(prop_name, "module",
strlen("module")) == 0 )
> > +                 continue;
> 
> Clearer to write this as:
> 
>     if ( strncmp(prop_name, "module", strlen("module"))
== 0 )
>         if ( strcmp(prop_name, "module1-args") == 0 )
>         {
>             prop_name = "bootargs";
>             had_mod1_args = 1;
>         }
>         else
>             continue;
>     }
> 
> > +            if ( strcmp(prop_name, "xen,dom0-bootargs") ==
0 )
> > +            {
> > +                if ( had_mod1_args )
> > +                    continue;
> > +                else
> > +                    prop_name = "bootargs";
> > +            }
> >          }
> >          /*
> >           * In a memory node: adjust reg property.
>

On 03/09/12 14:28, Ian Campbell wrote:
> 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).
> 
> In the PoC the modules are listed in the chosen node starting
> with /chosen/nr-modules which contains the count and then /chosen/module
> %d-{start,end} which gives the physical address of the module
> and /chosen/module%d-args which give its command line.
Until there is an agreement on this protocol I would prepend a "xen,"
prefix to the node names (xen,nr-modules etc.).

bootargs instead of args would be more consistent perhaps. So,
module1-args becomes xen,module1-bootargs.

The proposed protocol is functional and useful using nodes for each
module seems to be more device-tree-ish. I think in the longer term,
perhaps something like the following would be better?

chosen {
    module@1 {
        compatible = "multiboot-module";
        regs = <0x12345678 0x01000>;
        bootargs = "frob";
    };
    module@2 {
        compatible = "multiboot-module";
        regs = <0x12345678 0x01000>;
    }
}

David

On Thu, 2012-09-06 at 15:46 +0100, David Vrabel wrote:
> On 03/09/12 14:28, Ian Campbell wrote:
> > 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).
> > 
> > In the PoC the modules are listed in the chosen node starting
> > with /chosen/nr-modules which contains the count and then
/chosen/module
> > %d-{start,end} which gives the physical address of the module
> > and /chosen/module%d-args which give its command line.
> 
> Until there is an agreement on this protocol I would prepend a
"xen,"
> prefix to the node names (xen,nr-modules etc.).
OK.
> bootargs instead of args would be more consistent perhaps. So,
> module1-args becomes xen,module1-bootargs.
> 
> The proposed protocol is functional and useful using nodes for each
> module seems to be more device-tree-ish. I think in the longer term,
> perhaps something like the following would be better?
I rather suspect I''m going to end up porting multiboot to ARM. But you
are right that this looks like a better DTish way than mine.
> 
> chosen {
>     module@1 {
Are these normally 1 based or 0 based in DT?
>         compatible = "multiboot-module";
>         regs = <0x12345678 0x01000>;
Is this start and length? This relates somehow to #address-cells or
something doesn''t it?
>         bootargs = "frob";
>     };
>     module@2 {
>         compatible = "multiboot-module";
>         regs = <0x12345678 0x01000>;
>     }
> }
> 
> David

On Mon, 10 Sep 2012, Ian Campbell wrote:
> On Thu, 2012-09-06 at 15:46 +0100, David Vrabel wrote:
> > On 03/09/12 14:28, Ian Campbell wrote:
> > > 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).
> > > 
> > > In the PoC the modules are listed in the chosen node starting
> > > with /chosen/nr-modules which contains the count and then
/chosen/module
> > > %d-{start,end} which gives the physical address of the module
> > > and /chosen/module%d-args which give its command line.
> > 
> > Until there is an agreement on this protocol I would prepend a
"xen,"
> > prefix to the node names (xen,nr-modules etc.).
> 
> OK.
> 
> > bootargs instead of args would be more consistent perhaps. So,
> > module1-args becomes xen,module1-bootargs.
> > 
> > The proposed protocol is functional and useful using nodes for each
> > module seems to be more device-tree-ish. I think in the longer term,
> > perhaps something like the following would be better?
> 
> I rather suspect I''m going to end up porting multiboot to ARM. But
you
> are right that this looks like a better DTish way than mine.
I think that the grub folks have already extended the original spec to
make it easier to port to other archs.
Give a look at include/multiboot2.h, it seems to support mips as well as
x86.

On Mon, 2012-09-03 at 14:28 +0100, Ian Campbell wrote:
> The following series implements support for initial images and DTB in
> RAM
A subset of these patches have been acked so I have applied them. They
are generally cleanups rather than the main boot-wrapper stuff.
        arm: Zero the BSS at start of day.
        arm: make virtual address defines unsigned
        arm: move get_paddr_function to arch setup.c from device_tree.c
        arm: really allocate boot frametable pages with 32M alignment
        arm: print a message if multiple banks of memory are present.
        arm: mark heap and frametable limits as read mostly

Ian.

On Mon, 3 Sep 2012, Ian Campbell wrote:
> The bootloader should populate /chosen/nr-modules with the number of
> modules and then for each module 0..nr-modules-1 it should populate
> /chosen/moduleN-{start,end} with the physical address of the module.
Considering that we can traverse the DT and enumerate all the children
of a particular node, we don''t need a nr-modules property.

Also I would rather move all the modules under /chosen/modules than have
/chosen/module-N.

We could even go as far as having one single modules node, with every
module expressed as a reg range in memory.

Then you can document that for Xen the first module should be the linux
kernel and the second module should be the initrd.

chosen {
    modules {
        reg = < first_address first_size >, <second_address
second_size>;
    };
};

Besides being more concise and requiring simpler parsing, I think that
it is much more... aesthetic :)

> The hypervisor allows for 2 modules (kernel and initrd). Currently we
> don''t do anything with them.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/common/device_tree.c      |   57
+++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h |    7 +++++
>  2 files changed, 64 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 3d1f0f4..226e3f3 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -229,6 +229,61 @@ 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 i, nr_modules;
> +
> +    prop = fdt_get_property(fdt, node, "nr-modules", NULL);
> +    if ( !prop )
> +    {
> +        early_info.modules.nr_mods = 0;
> +        return;
> +    }
> +
> +    cell = (const u32 *)prop->data;
> +    get_val(&cell, 1, (u64*)&nr_modules);
> +
> +    if ( nr_modules > NR_MODULES )
> +    {
> +        early_panic("too many modules %d > %d\n", nr_modules,
NR_MODULES);
> +    }
> +
> +    for ( i = 0; i < nr_modules; i++ )
> +    {
> +        struct membank *mod = &early_info.modules.module[i];
> +        /* longest prop name is "start", single digit numbers of
modules */
> +        char propname[strlen("moduleX-start") + 1];
> +
> +        BUILD_BUG_ON(NR_MODULES > 9);
> +
> +        snprintf(propname, sizeof(propname), "module%d-start",
i+1);
> +        prop = fdt_get_property(fdt, node, propname, NULL);
> +        if ( !prop )
> +            early_panic("no start for module %d\n", i);
> +
> +        cell = (const u32 *)prop->data;
> +        device_tree_get_reg(&cell, address_cells, size_cells,
> +                            &mod->start, &size);
I don''t think you should be using device_tree_get_reg to parse a non
reg
property

> +        snprintf(propname, sizeof(propname), "module%d-end",
i+1);
> +        prop = fdt_get_property(fdt, node, propname, NULL);
> +        if ( !prop )
> +            early_panic("no end for module %d\n", i);
> +
> +        cell = (const u32 *)prop->data;
> +        device_tree_get_reg(&cell, address_cells, size_cells,
> +                            &mod->size, &size);
> +        mod->size -= mod->start;
> +    }
> +
> +    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 +291,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..585fcc9 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,14 @@ struct dt_mem_info {
>      struct membank bank[NR_MEM_BANKS];
>  };
>  
> +struct dt_module_info {
> +    int nr_mods;
> +    struct membank module[NR_MODULES];
> +};
> +
>  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
>