[PATCH v2 0/2] Rework the way to compute dom0 DTB base address

[PATCH v2 0/2] Rework the way to compute dom0 DTB base address

[Julien Grall]

Hello

The first version of this series contained a single patch.

I have added a patch to use the address right after the kernel, if a DTB
is already appended.

It could happen if the same kernel is used for both bare metal and XEN without
a bootloader. I use this configuration on the versatile express, so I appended
the DTB to my kernel and enable CONFIG_APPENDED_DTB. Linux will prefer to use
the appended one rather than the one pointed by r2.

For the other changes see in each patch.

Cheers,

Julien Grall (2):
  xen/arm: Rework the way to compute dom0 DTB base address
  xen/arm: If the DOM0 zImage as a DTB appended replace it

 xen/arch/arm/domain_build.c |   51 ++++++++++++++++++++++++++++++++++++++-----
 xen/arch/arm/kernel.c       |   28 ++++++++++++------------
 xen/arch/arm/kernel.h       |    7 ++++++
 3 files changed, 66 insertions(+), 20 deletions(-)

-- 
1.7.10.4

[PATCH v2 1/2] xen/arm: Rework the way to compute dom0 DTB base address

[Julien Grall]

If the DTB is loading right the 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 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 |   51 ++++++++++++++++++++++++++++++++++++++-----
 xen/arch/arm/kernel.c       |    2 ++
 xen/arch/arm/kernel.h       |    7 ++++++
 3 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b92c64b..515fabe 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,25 @@ 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 or below 4Gib
      */
-    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);
+    /* Linux requires the address to be aligned to 2Mb */
+    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,9 +530,36 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
     return -EINVAL;
 }
 
+static int 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->type != KERNEL_ZIMAGE )
+        return 0;
+
+    if ( !(MAX(zimage_start, dtb_end) < MIN(zimage_end, dtb_start)) )
+        return 0;
+
+    printk(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);
+    xfree(kinfo->fdt);
+
+    return -EINVAL;
+}
+
 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,13 @@ int construct_dom0(struct domain *d)
     if ( rc < 0 )
         return rc;
 
+    rc = dtb_check_overlap(&kinfo);
+    if ( rc < 0 )
+        return rc;
+
     /* 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..f8c8850 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->type = 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->type = KERNEL_ELF;
 
     return 0;
 err:
diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
index 1776a4d..b4ecd30 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 type;
 
     union {
         struct {
-- 
1.7.10.4

Re: [PATCH v2 1/2] xen/arm: Rework the way to compute dom0 DTB base address

[Ian Campbell]

On Thu, 2013-05-30 at 15:05 +0100, Julien Grall wrote:
> If the DTB is loading right the 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 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 |   51 ++++++++++++++++++++++++++++++++++++++-----
>  xen/arch/arm/kernel.c       |    2 ++
>  xen/arch/arm/kernel.h       |    7 ++++++
>  3 files changed, 54 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b92c64b..515fabe 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,25 @@ 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
                                                     ^from          ^uses

> +     * the space after it to decompress)
> +     * Load the DTB at the end of the first bank or below 4Gib
                                                   and below?

Perhaps clearer to write "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);
> +    /* Linux requires the address to be aligned to 2Mb */

Does it? I don't disagree with aligning it to 2MB but I'm sure that e.g.
CONFIG_APPENDED_DTB handles the unaligned case (perhaps that is
special)?

> +    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,9 +530,36 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>      return -EINVAL;
>  }
>  
> +static int 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->type != KERNEL_ZIMAGE )
> +        return 0;
> +
> +    if ( !(MAX(zimage_start, dtb_end) < MIN(zimage_end, dtb_start)) )
> +        return 0;

I had to draw quite a few pictures to convince myself this was right,
and I'm still not entirely sure ;-)

> +
> +    printk(XENLOG_ERR "The kernel(0x%"PRIpaddr"-0x%"PRIpaddr
> +           ") is overlapping the DTB(0x%"PRIpaddr"-0x%"PRIpaddr":\n",

Missing a closing ) on the DTB range. Also why the trailing ":"?

> +           zimage_start, zimage_end, dtb_start, dtb_end);
> +    xfree(kinfo->fdt);

This seems like an odd place to free this.

Returning an error here is only going to cause us to give up and panic
anyway. I'd suggest to just panic here directly and not bother
propagating an error code.

Not really sure why construct_dom0 has a return code at all, instead of
it (or the functions it calls) just panicing. But lets not worry about
that now.

> +
> +    return -EINVAL;
> +}
> +
>  static void dtb_load(struct kernel_info *kinfo)
>  {
>      void * __user dtb_virt = (void * __user)(register_t)kinfo->dtb_paddr;

Blank line here please.

> +    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,13 @@ int construct_dom0(struct domain *d)
>      if ( rc < 0 )
>          return rc;
>  
> +    rc = dtb_check_overlap(&kinfo);
> +    if ( rc < 0 )
> +        return rc;
> +
>      /* 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..f8c8850 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->type = 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->type = KERNEL_ELF;
>  
>      return 0;
>  err:
> diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
> index 1776a4d..b4ecd30 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 type;
>  
>      union {
>          struct {

Re: [PATCH v2 1/2] xen/arm: Rework the way to compute dom0 DTB base address

[Julien Grall]

On 05/31/2013 12:45 PM, Ian Campbell wrote:

> On Thu, 2013-05-30 at 15:05 +0100, Julien Grall wrote:
>> If the DTB is loading right the 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 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 |   51 ++++++++++++++++++++++++++++++++++++++-----
>>  xen/arch/arm/kernel.c       |    2 ++
>>  xen/arch/arm/kernel.h       |    7 ++++++
>>  3 files changed, 54 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index b92c64b..515fabe 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,25 @@ 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
>                                                      ^from          ^uses
> 
>> +     * the space after it to decompress)
>> +     * Load the DTB at the end of the first bank or below 4Gib
>                                                    and below?
> 
> Perhaps clearer to write "Load the DTB at the end of the first bank,
> while ensure it is also below 4G"
>


Sound goods. I will send a new patch with this change.

>>       */
>> -    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);
>> +    /* Linux requires the address to be aligned to 2Mb */
> 
> Does it? I don't disagree with aligning it to 2MB but I'm sure that e.g.
> CONFIG_APPENDED_DTB handles the unaligned case (perhaps that is
> special)?


I didn't pay attention when I have replaced 4 bytes by 2Mb. Linux only
requires to have an address 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,9 +530,36 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>>      return -EINVAL;
>>  }
>>  
>> +static int 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->type != KERNEL_ZIMAGE )
>> +        return 0;
>> +
>> +    if ( !(MAX(zimage_start, dtb_end) < MIN(zimage_end, dtb_start)) )
>> +        return 0;
> 
> I had to draw quite a few pictures to convince myself this was right,
> and I'm still not entirely sure ;-)


In fact it's totally wrong :/. What about this check?

if ( (dtb_start < zimage_end) || (dtb_end < zimage_start) )

> 
>> +
>> +    printk(XENLOG_ERR "The kernel(0x%"PRIpaddr"-0x%"PRIpaddr
>> +           ") is overlapping the DTB(0x%"PRIpaddr"-0x%"PRIpaddr":\n",
> 
> Missing a closing ) on the DTB range. Also why the trailing ":"?


I planned to have a different sentence. I will replace the ":" by ")".

> 
>> +           zimage_start, zimage_end, dtb_start, dtb_end);
>> +    xfree(kinfo->fdt);
> 
> This seems like an odd place to free this.
> 
> Returning an error here is only going to cause us to give up and panic
> anyway. I'd suggest to just panic here directly and not bother
> propagating an error code.
> 
> Not really sure why construct_dom0 has a return code at all, instead of
> it (or the functions it calls) just panicing. But lets not worry about
> that now.
> 
>> +
>> +    return -EINVAL;
>> +}
>> +
>>  static void dtb_load(struct kernel_info *kinfo)
>>  {
>>      void * __user dtb_virt = (void * __user)(register_t)kinfo->dtb_paddr;
> 
> Blank line here please.
> 
>> +    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,13 @@ int construct_dom0(struct domain *d)
>>      if ( rc < 0 )
>>          return rc;
>>  
>> +    rc = dtb_check_overlap(&kinfo);
>> +    if ( rc < 0 )
>> +        return rc;
>> +
>>      /* 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..f8c8850 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->type = 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->type = KERNEL_ELF;
>>  
>>      return 0;
>>  err:
>> diff --git a/xen/arch/arm/kernel.h b/xen/arch/arm/kernel.h
>> index 1776a4d..b4ecd30 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 type;
>>  
>>      union {
>>          struct {
> 
> 

Re: [PATCH v2 1/2] xen/arm: Rework the way to compute dom0 DTB base address

[Julien Grall]

On 05/31/2013 02:28 PM, Julien Grall wrote:

>>> +    if ( !(MAX(zimage_start, dtb_end) < MIN(zimage_end, dtb_start)) )
>>> +        return 0;
>>
>> I had to draw quite a few pictures to convince myself this was right,
>> and I'm still not entirely sure ;-)
> 
> 
> In fact it's totally wrong :/. What about this check?

>

Another mistake on the first check:

if ( (dtb_start > zimage_end) || (dtb_end < zimage_start) )


-- 
Julien

Re: [PATCH v2 1/2] xen/arm: Rework the way to compute dom0 DTB base address

[Ian Campbell]

On Fri, 2013-05-31 at 14:45 +0100, Julien Grall wrote:
> On 05/31/2013 02:28 PM, Julien Grall wrote:
> 
> >>> +    if ( !(MAX(zimage_start, dtb_end) < MIN(zimage_end, dtb_start)) )
> >>> +        return 0;
> >>
> >> I had to draw quite a few pictures to convince myself this was right,
> >> and I'm still not entirely sure ;-)
> > 
> > 
> > In fact it's totally wrong :/. What about this check?
> 
> >
> 
> Another mistake on the first check:
> 
> if ( (dtb_start > zimage_end) || (dtb_end < zimage_start) )

This one looks right (third time's the charm...)

Ian.

[PATCH v2 2/2] xen/arm: If the DOM0 zImage as a DTB appended replace it

[Julien Grall]

If a DTB is appended to the DOM0 zImage, it's probably means that the linux
decompression stage will replace the DBT register value (r2 on arm32)
by the address of the appended DTB.
In this case, to avoid issue with Linux, Xen needs to load the new device tree
just after the kernel.

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

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index f8c8850..1d6c927 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -123,20 +123,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info,
     if ( (end - start) > size )
         return -EINVAL;
 
-    /*
-     * Check for an appended DTB.
-     */
-    if ( addr + end - start + sizeof(dtb_hdr) <= size )
-    {
-        copy_from_paddr(&dtb_hdr, addr + end - start,
-                        sizeof(dtb_hdr), DEV_SHARED);
-        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) {
-            end += be32_to_cpu(dtb_hdr.total_size);
-
-            if ( end > addr + size )
-                return -EINVAL;
-        }
-    }
 
     info->zimage.kernel_addr = addr;
 
@@ -154,6 +140,18 @@ static int kernel_try_zimage_prepare(struct kernel_info *info,
     info->load = kernel_zimage_load;
     info->type = KERNEL_ZIMAGE;
 
+    /*
+     * If there is an appended DTB, ask XEN to replace the DTB
+     * by the generate one.
+     */
+    if ( info->zimage.len + sizeof(dtb_hdr) <= size )
+    {
+        copy_from_paddr(&dtb_hdr, addr + end - start,
+                        sizeof(dtb_hdr), DEV_SHARED);
+        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC)
+            info->dtb_paddr = info->zimage.load_addr + info->zimage.len;
+    }
+
     return 0;
 }
 
-- 
1.7.10.4

Re: [PATCH v2 2/2] xen/arm: If the DOM0 zImage as a DTB appended replace it

[Ian Campbell]

On Thu, 2013-05-30 at 15:05 +0100, Julien Grall wrote:
> If a DTB is appended to the DOM0 zImage, it's probably means that the linux
> decompression stage will replace the DBT register value (r2 on arm32)
> by the address of the appended DTB.

I don't think we should do this, it certainly violates the principal of
least surprise, since no other "bootloader" does anything like this
AFAIK. Yes that means you occasionally trip over your DTB changes
appearing not to take affect, but that's true on native and is one of
the known and understood bad things about appended DTB.

So I think this is a case of "don't do that then". At most we could
issue a warning, I suppose. But really we shouldn't be making any
assumptions (good or bad) about what happens to live in the memory just
past the end of the kernel, it may or may not be an appended DTB.

> In this case, to avoid issue with Linux, Xen needs to load the new device tree
> just after the kernel.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/kernel.c |   26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index f8c8850..1d6c927 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -123,20 +123,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info,
>      if ( (end - start) > size )
>          return -EINVAL;
>  
> -    /*
> -     * Check for an appended DTB.
> -     */
> -    if ( addr + end - start + sizeof(dtb_hdr) <= size )
> -    {
> -        copy_from_paddr(&dtb_hdr, addr + end - start,
> -                        sizeof(dtb_hdr), DEV_SHARED);
> -        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) {
> -            end += be32_to_cpu(dtb_hdr.total_size);
> -
> -            if ( end > addr + size )
> -                return -EINVAL;
> -        }
> -    }
>  
>      info->zimage.kernel_addr = addr;
>  
> @@ -154,6 +140,18 @@ static int kernel_try_zimage_prepare(struct kernel_info *info,
>      info->load = kernel_zimage_load;
>      info->type = KERNEL_ZIMAGE;
>  
> +    /*
> +     * If there is an appended DTB, ask XEN to replace the DTB
> +     * by the generate one.
> +     */
> +    if ( info->zimage.len + sizeof(dtb_hdr) <= size )
> +    {
> +        copy_from_paddr(&dtb_hdr, addr + end - start,
> +                        sizeof(dtb_hdr), DEV_SHARED);
> +        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC)
> +            info->dtb_paddr = info->zimage.load_addr + info->zimage.len;
> +    }
> +
>      return 0;
>  }
>  

Re: [PATCH v2 2/2] xen/arm: If the DOM0 zImage as a DTB appended replace it

[Julien Grall]

On 05/30/2013 03:24 PM, Ian Campbell wrote:

> On Thu, 2013-05-30 at 15:05 +0100, Julien Grall wrote:
>> If a DTB is appended to the DOM0 zImage, it's probably means that the linux
>> decompression stage will replace the DBT register value (r2 on arm32)
>> by the address of the appended DTB.
> 
> I don't think we should do this, it certainly violates the principal of
> least surprise, since no other "bootloader" does anything like this
> AFAIK. Yes that means you occasionally trip over your DTB changes
> appearing not to take affect, but that's true on native and is one of
> the known and understood bad things about appended DTB.


Shall we load the DTB if there is one appended? If yes, I think the user
will be confused because Linux won't use the right device tree.
It's hard for the user to find the issue because there is no log.

> So I think this is a case of "don't do that then". At most we could
> issue a warning, I suppose. But really we shouldn't be making any
> assumptions (good or bad) about what happens to live in the memory just
> past the end of the kernel, it may or may not be an appended DTB.


I can add a warning and also fix the check the line:
if ( addr + end - start + sizeof(dtb_hdr) <= size )
must be replace by:
if ( end - start + sizeof(dtb_hdr) <= size )

>> In this case, to avoid issue with Linux, Xen needs to load the new device tree
>> just after the kernel.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/kernel.c |   26 ++++++++++++--------------
>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>> index f8c8850..1d6c927 100644
>> --- a/xen/arch/arm/kernel.c
>> +++ b/xen/arch/arm/kernel.c
>> @@ -123,20 +123,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info,
>>      if ( (end - start) > size )
>>          return -EINVAL;
>>  
>> -    /*
>> -     * Check for an appended DTB.
>> -     */
>> -    if ( addr + end - start + sizeof(dtb_hdr) <= size )
>> -    {
>> -        copy_from_paddr(&dtb_hdr, addr + end - start,
>> -                        sizeof(dtb_hdr), DEV_SHARED);
>> -        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) {
>> -            end += be32_to_cpu(dtb_hdr.total_size);
>> -
>> -            if ( end > addr + size )
>> -                return -EINVAL;
>> -        }
>> -    }
>>  
>>      info->zimage.kernel_addr = addr;
>>  
>> @@ -154,6 +140,18 @@ static int kernel_try_zimage_prepare(struct kernel_info *info,
>>      info->load = kernel_zimage_load;
>>      info->type = KERNEL_ZIMAGE;
>>  
>> +    /*
>> +     * If there is an appended DTB, ask XEN to replace the DTB
>> +     * by the generate one.
>> +     */
>> +    if ( info->zimage.len + sizeof(dtb_hdr) <= size )
>> +    {
>> +        copy_from_paddr(&dtb_hdr, addr + end - start,
>> +                        sizeof(dtb_hdr), DEV_SHARED);
>> +        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC)
>> +            info->dtb_paddr = info->zimage.load_addr + info->zimage.len;
>> +    }
>> +
>>      return 0;
>>  }
>>  
> 
> 

Re: [PATCH v2 2/2] xen/arm: If the DOM0 zImage as a DTB appended replace it

[Ian Campbell]

On Thu, 2013-05-30 at 15:47 +0100, Julien Grall wrote:
> On 05/30/2013 03:24 PM, Ian Campbell wrote:
> 
> > On Thu, 2013-05-30 at 15:05 +0100, Julien Grall wrote:
> >> If a DTB is appended to the DOM0 zImage, it's probably means that the linux
> >> decompression stage will replace the DBT register value (r2 on arm32)
> >> by the address of the appended DTB.
> > 
> > I don't think we should do this, it certainly violates the principal of
> > least surprise, since no other "bootloader" does anything like this
> > AFAIK. Yes that means you occasionally trip over your DTB changes
> > appearing not to take affect, but that's true on native and is one of
> > the known and understood bad things about appended DTB.
> 
> 
> Shall we load the DTB if there is one appended?

We shouldn't make any assumptions based on whether we suspect there
might be an appended DTB. We should just carry on.

The presence or absence of an appended DTB is purely the guests internal
concern, it is none of our business, even if it breaks things

>  If yes, I think the user
> will be confused because Linux won't use the right device tree.

Yes. Take a look at the Kconfig help text for the kernel option. This is
a known shortcoming of APPEND_DTB, it's a hack and shouldn't be used.

> It's hard for the user to find the issue because there is no log.
> 
> > So I think this is a case of "don't do that then". At most we could
> > issue a warning, I suppose. But really we shouldn't be making any
> > assumptions (good or bad) about what happens to live in the memory just
> > past the end of the kernel, it may or may not be an appended DTB.
> 
> 
> I can add a warning and also fix the check the line:
> if ( addr + end - start + sizeof(dtb_hdr) <= size )
> must be replace by:
> if ( end - start + sizeof(dtb_hdr) <= size )
> 
> >> In this case, to avoid issue with Linux, Xen needs to load the new device tree
> >> just after the kernel.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> ---
> >>  xen/arch/arm/kernel.c |   26 ++++++++++++--------------
> >>  1 file changed, 12 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> >> index f8c8850..1d6c927 100644
> >> --- a/xen/arch/arm/kernel.c
> >> +++ b/xen/arch/arm/kernel.c
> >> @@ -123,20 +123,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info,
> >>      if ( (end - start) > size )
> >>          return -EINVAL;
> >>  
> >> -    /*
> >> -     * Check for an appended DTB.
> >> -     */
> >> -    if ( addr + end - start + sizeof(dtb_hdr) <= size )
> >> -    {
> >> -        copy_from_paddr(&dtb_hdr, addr + end - start,
> >> -                        sizeof(dtb_hdr), DEV_SHARED);
> >> -        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) {
> >> -            end += be32_to_cpu(dtb_hdr.total_size);
> >> -
> >> -            if ( end > addr + size )
> >> -                return -EINVAL;
> >> -        }
> >> -    }
> >>  
> >>      info->zimage.kernel_addr = addr;
> >>  
> >> @@ -154,6 +140,18 @@ static int kernel_try_zimage_prepare(struct kernel_info *info,
> >>      info->load = kernel_zimage_load;
> >>      info->type = KERNEL_ZIMAGE;
> >>  
> >> +    /*
> >> +     * If there is an appended DTB, ask XEN to replace the DTB
> >> +     * by the generate one.
> >> +     */
> >> +    if ( info->zimage.len + sizeof(dtb_hdr) <= size )
> >> +    {
> >> +        copy_from_paddr(&dtb_hdr, addr + end - start,
> >> +                        sizeof(dtb_hdr), DEV_SHARED);
> >> +        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC)
> >> +            info->dtb_paddr = info->zimage.load_addr + info->zimage.len;
> >> +    }
> >> +
> >>      return 0;
> >>  }
> >>  
> > 
> > 
> 
> 

Re: [PATCH v2 2/2] xen/arm: If the DOM0 zImage as a DTB appended replace it

[Julien Grall]

On 05/30/2013 03:52 PM, Ian Campbell wrote:

> On Thu, 2013-05-30 at 15:47 +0100, Julien Grall wrote:
>> On 05/30/2013 03:24 PM, Ian Campbell wrote:
>>
>>> On Thu, 2013-05-30 at 15:05 +0100, Julien Grall wrote:
>>>> If a DTB is appended to the DOM0 zImage, it's probably means that the linux
>>>> decompression stage will replace the DBT register value (r2 on arm32)
>>>> by the address of the appended DTB.
>>>
>>> I don't think we should do this, it certainly violates the principal of
>>> least surprise, since no other "bootloader" does anything like this
>>> AFAIK. Yes that means you occasionally trip over your DTB changes
>>> appearing not to take affect, but that's true on native and is one of
>>> the known and understood bad things about appended DTB.
>>
>>
>> Shall we load the DTB if there is one appended?
> 
> We shouldn't make any assumptions based on whether we suspect there
> might be an appended DTB. We should just carry on.
> 
> The presence or absence of an appended DTB is purely the guests internal
> concern, it is none of our business, even if it breaks things

Ok. I will rework the patch and send it back alone.

>>  If yes, I think the user
>> will be confused because Linux won't use the right device tree.
> 
> Yes. Take a look at the Kconfig help text for the kernel option. This is
> a known shortcoming of APPEND_DTB, it's a hack and shouldn't be used.
> 
>> It's hard for the user to find the issue because there is no log.
>>
>>> So I think this is a case of "don't do that then". At most we could
>>> issue a warning, I suppose. But really we shouldn't be making any
>>> assumptions (good or bad) about what happens to live in the memory just
>>> past the end of the kernel, it may or may not be an appended DTB.
>>
>>
>> I can add a warning and also fix the check the line:
>> if ( addr + end - start + sizeof(dtb_hdr) <= size )
>> must be replace by:
>> if ( end - start + sizeof(dtb_hdr) <= size )
>>
>>>> In this case, to avoid issue with Linux, Xen needs to load the new device tree
>>>> just after the kernel.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>> ---
>>>>  xen/arch/arm/kernel.c |   26 ++++++++++++--------------
>>>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>>> index f8c8850..1d6c927 100644
>>>> --- a/xen/arch/arm/kernel.c
>>>> +++ b/xen/arch/arm/kernel.c
>>>> @@ -123,20 +123,6 @@ static int kernel_try_zimage_prepare(struct kernel_info *info,
>>>>      if ( (end - start) > size )
>>>>          return -EINVAL;
>>>>  
>>>> -    /*
>>>> -     * Check for an appended DTB.
>>>> -     */
>>>> -    if ( addr + end - start + sizeof(dtb_hdr) <= size )
>>>> -    {
>>>> -        copy_from_paddr(&dtb_hdr, addr + end - start,
>>>> -                        sizeof(dtb_hdr), DEV_SHARED);
>>>> -        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) {
>>>> -            end += be32_to_cpu(dtb_hdr.total_size);
>>>> -
>>>> -            if ( end > addr + size )
>>>> -                return -EINVAL;
>>>> -        }
>>>> -    }
>>>>  
>>>>      info->zimage.kernel_addr = addr;
>>>>  
>>>> @@ -154,6 +140,18 @@ static int kernel_try_zimage_prepare(struct kernel_info *info,
>>>>      info->load = kernel_zimage_load;
>>>>      info->type = KERNEL_ZIMAGE;
>>>>  
>>>> +    /*
>>>> +     * If there is an appended DTB, ask XEN to replace the DTB
>>>> +     * by the generate one.
>>>> +     */
>>>> +    if ( info->zimage.len + sizeof(dtb_hdr) <= size )
>>>> +    {
>>>> +        copy_from_paddr(&dtb_hdr, addr + end - start,
>>>> +                        sizeof(dtb_hdr), DEV_SHARED);
>>>> +        if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC)
>>>> +            info->dtb_paddr = info->zimage.load_addr + info->zimage.len;
>>>> +    }
>>>> +
>>>>      return 0;
>>>>  }
>>>>  
>>>
>>>
>>
>>
> 
>