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

[Julien Grall <julien.grall@linaro.org>]

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 {
> 
>