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

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

On 05/28/2013 10:03 AM, Ian Campbell wrote:

> On Mon, 2013-05-27 at 14:50 +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 decrompression stage, load
> 
> "decompression"
> 
>> the DTB near the end of the first memory bank and below 4Gib (if memory range is
>> greater).
> 
> I've been considering something like this too. I have a feeling we might
> end up just pushing things around in memory continually breaking one
> platform in favour of another. However this new choice location seems
> likely to be more compatible than what we have now...
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/domain_build.c |   19 +++++++++++++------
>>  1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index f857e40..ca086a3 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -519,6 +519,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;
>>  
>> @@ -543,11 +544,6 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>>      if ( ret < 0 )
>>          goto err;
>>  
>> -    /*
>> -     * Put the device tree at the beginning of the first bank.  It
>> -     * must be below 4 GiB.
>> -     */
>> -    kinfo->dtb_paddr = kinfo->mem.bank[0].start + 0x100;
>>      if ( kinfo->dtb_paddr + fdt_totalsize(kinfo->fdt) > (1ull << 32) )
> 
> Isn't this check now misplaced?


Right. I think we can remove kinfo->dtb_paddr and just check the DTB
size is less than 4GiB.

> 
>>      {
>>          printk("Not enough memory below 4 GiB for the device tree.");
>> @@ -555,6 +551,16 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>>          goto err;
>>      }
>>  
>> +    /*
>> +     * 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
>> +     */
>> +    end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size;
>> +    kinfo->dtb_paddr = (MIN(1ull << 32, end) - fdt_totalsize(kinfo->fdt));
>> +    /* Linux requires the address to be a multiple of 4 */
>> +    kinfo->dtb_paddr &= ~3;
> 
> What do you think of aligning to e.g. a 2MB boundary?

Sounds good for, in fact, I wanted to avoid a big wasted space after the
DTB. I will send a new version of this patch with all the modifications.

> "multiple of 4" would be more usually expressed in this context as
> "aligned to 4 bytes".
> 
>> +
>>      return 0;
>>  
>>    err:
>> @@ -566,6 +572,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>>  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);
>> @@ -604,7 +612,6 @@ int construct_dom0(struct domain *d)
>>      /* 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);
>>  
> 
>