Xen devel - Jun 2013 - [PATCH v4] xen/arm: Rework the way to compute dom0 DTB base address

If the DTB is loading right after the kernel, on some setup, Linux will
overwrite the DTB during the decompression step.

To be sure the DTB won''t be overwritten by the decompression stage,
load
the DTB near the end of the first memory bank and below 4Gib (if memory range is
greater).

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

Changes in v4:
    - Rename type to ktype to avoid clash name on arm64

Changes in v3:
    - Rework comments
    - Missing blank line
    - Transform dtb_check_overlap to a void function
    - Directly panic in dtb_check_overlap
    - Use the right formula to check the overlap

Changes in v2:
    - Align the DTB base address to 2Mib
    - Add dtb_check_overlap to check if the kernel will overlap the DTB
    - Fix typo
---
 xen/arch/arm/domain_build.c |   49 +++++++++++++++++++++++++++++++++++++------
 xen/arch/arm/kernel.c       |    2 ++
 xen/arch/arm/kernel.h       |    7 +++++++
 3 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b92c64b..7abe537 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -477,6 +477,7 @@ static int prepare_dtb(struct domain *d, struct kernel_info
*kinfo)
     void *fdt;
     int new_size;
     int ret;
+    paddr_t end;
 
     kinfo->unassigned_mem = dom0_mem;
 
@@ -502,17 +503,26 @@ static int prepare_dtb(struct domain *d, struct
kernel_info *kinfo)
         goto err;
 
     /*
-     * Put the device tree at the beginning of the first bank.  It
-     * must be below 4 GiB.
+     * DTB must be load below 4GiB and far enough to linux (Linux use
+     * the space after it to decompress)
+     * Load the DTB at the end of the first bank, while ensure it is
+     * also below 4G
      */
-    kinfo->dtb_paddr = kinfo->mem.bank[0].start + 0x100;
-    if ( kinfo->dtb_paddr + fdt_totalsize(kinfo->fdt) > (1ull <<
32) )
+    end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size;
+    end = MIN(1ull << 32, end);
+    kinfo->dtb_paddr = end - fdt_totalsize(kinfo->fdt);
+    /* Aligned the address to 2Mb. Linux only requires to be aligned to 4 bytes
*/
+    kinfo->dtb_paddr &= ~((2 << 20) - 1);
+
+    if ( fdt_totalsize(kinfo->fdt) > end )
     {
-        printk("Not enough memory below 4 GiB for the device tree.");
+        printk(XENLOG_ERR "Not enough memory in the first bank for "
+               "the device tree.");
         ret = -FDT_ERR_XEN(EINVAL);
         goto err;
     }
 
+
     return 0;
 
   err:
@@ -521,10 +531,36 @@ static int prepare_dtb(struct domain *d, struct
kernel_info *kinfo)
     return -EINVAL;
 }
 
+static void dtb_check_overlap(struct kernel_info *kinfo)
+{
+    paddr_t zimage_start = kinfo->zimage.load_addr;
+    paddr_t zimage_end = kinfo->zimage.load_addr + kinfo->zimage.len;
+    paddr_t dtb_start = kinfo->dtb_paddr;
+    paddr_t dtb_end = kinfo->dtb_paddr + fdt_totalsize(kinfo->fdt);
+
+    /*
+     * Check the kernel won''t overlap the kernel
+     * Only when it''s a ZIMAGE
+     */
+    if ( kinfo->ktype != KERNEL_ZIMAGE )
+        return;
+
+    if ( (dtb_start > zimage_end) || (dtb_end < zimage_start) )
+        return;
+
+    panic(XENLOG_ERR "The kernel(0x%"PRIpaddr"-0x%"PRIpaddr
+          ") is overlapping the
DTB(0x%"PRIpaddr"-0x%"PRIpaddr")\n",
+          zimage_start, zimage_end, dtb_start, dtb_end);
+
+}
+
 static void dtb_load(struct kernel_info *kinfo)
 {
     void * __user dtb_virt = (void * __user)(register_t)kinfo->dtb_paddr;
 
+    printk("Loading dom0 DTB to
0x%"PRIpaddr"-0x%"PRIpaddr"\n",
+           kinfo->dtb_paddr, kinfo->dtb_paddr +
fdt_totalsize(kinfo->fdt));
+
     raw_copy_to_guest(dtb_virt, kinfo->fdt, fdt_totalsize(kinfo->fdt));
     xfree(kinfo->fdt);
 }
@@ -559,10 +595,11 @@ int construct_dom0(struct domain *d)
     if ( rc < 0 )
         return rc;
 
+    dtb_check_overlap(&kinfo);
+
     /* The following loads use the domain''s p2m */
     p2m_load_VTTBR(d);
 
-    kinfo.dtb_paddr = kinfo.zimage.load_addr + kinfo.zimage.len;
     kernel_load(&kinfo);
     dtb_load(&kinfo);
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 8f4a60d..cdaca9c 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -152,6 +152,7 @@ static int kernel_try_zimage_prepare(struct kernel_info
*info,
 
     info->entry = info->zimage.load_addr;
     info->load = kernel_zimage_load;
+    info->ktype = KERNEL_ZIMAGE;
 
     return 0;
 }
@@ -193,6 +194,7 @@ static int kernel_try_elf_prepare(struct kernel_info *info,
      */
     info->entry = info->elf.parms.virt_entry;
     info->load = kernel_elf_load;
+    info->ktype = KERNEL_ELF;
 
     return 0;
 err:
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index 1776a4d..69a572c 100644
--- a/xen/arch/arm/kernel.h
+++ b/xen/arch/arm/kernel.h
@@ -9,6 +9,12 @@
 #include <xen/libelf.h>
 #include <xen/device_tree.h>
 
+enum kernel_type
+{
+    KERNEL_ELF,
+    KERNEL_ZIMAGE,
+};
+
 struct kernel_info {
 #ifdef CONFIG_ARM_64
     enum domain_type type;
@@ -23,6 +29,7 @@ struct kernel_info {
 
     void *kernel_img;
     unsigned kernel_order;
+    enum kernel_type ktype;
 
     union {
         struct {
-- 
1.7.10.4

On Mon, 2013-06-03 at 14:57 +0100, Julien Grall wrote:
> +    /*
> +     * Check the kernel won''t overlap the kernel
> +     * Only when it''s a ZIMAGE
> +     */
> +    if ( kinfo->ktype != KERNEL_ZIMAGE )
> +        return;
It occurs to me that we could avoid needing this and the associated
gubbins to remember the type of kernel by having a ->check function hook
alongside the ->load hook and only setting it for zImage.

What do you think?

On 06/13/2013 04:58 PM, Ian Campbell wrote:
> On Mon, 2013-06-03 at 14:57 +0100, Julien Grall wrote:
>> +    /*
>> +     * Check the kernel won''t overlap the kernel
>> +     * Only when it''s a ZIMAGE
>> +     */
>> +    if ( kinfo->ktype != KERNEL_ZIMAGE )
>> +        return;
> 
> It occurs to me that we could avoid needing this and the associated
> gubbins to remember the type of kernel by having a ->check function hook
> alongside the ->load hook and only setting it for zImage.
> 
> What do you think?
Sounds good to me. I will update the patch with this solution.

-- 
Julien