Re: [PATCH 1/2] xen/arm: Add support to load initrd in dom0

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

On 09/25/2013 04:30 PM, Ian Campbell wrote:
> On Wed, 2013-09-25 at 16:23 +0100, Julien Grall wrote:
>> On 09/25/2013 04:15 PM, Ian Campbell wrote:
>>> On Mon, 2013-09-16 at 16:20 +0100, Julien Grall wrote:
>>>> @@ -806,7 +821,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>>>>          goto err;
>>>>  
>>>>      /* Actual new size */
>>>> -    new_size = fdt_totalsize(kinfo->fdt);
>>>> +    initrd_len = early_info.modules.module[MOD_INITRD].size;
>>>
>>> I think you need to check nr_modules here and in write_properties (which
>>> I already trimmed by mistake.
>>>
>>
>> Even if we check nr_modules, we can't assume MOD_INITRD is the last
>> modules in the array, so it's possible to have nr_modules greater than
>> MOD_INITRD but the module is not set. That's why I only choose to rely
>> on size.
> 
> Hrm that's true.
> 
>>> .size may not be initialised otherwise. (in reality it's probably
>>> in .bss not sure I want to rely on that though)
>>
>> It's in .bss, on common/device_tree.c we already rely that this
>> structure is zeroed (nr_modules is never initialized to 0).
> 
> OK, I guess its fine then.
> 
>>
>>
>>>
>>>> +    new_size = fdt_totalsize(kinfo->fdt) + initrd_len;
>>>>  
>>>>      /*
>>>>       * DTB must be loaded such that it does not conflict with the
>>>> @@ -815,15 +831,20 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>>>>       * the recommendation in Documentation/arm64/booting.txt is below
>>>>       * 512MB. Place at 128MB, (or, if we have less RAM, as high as
>>>>       * possible) in order to satisfy both.
>>>> +     * If the bootloader provides an initrd, it will be loaded just
>>>> +     * after the DTB.
>>>>       */
>>>>      end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size;
>>>>      end = MIN(kinfo->mem.bank[0].start + (128<<20) + new_size, end);
>>>>  
>>>> -    kinfo->dtb_paddr = end - fdt_totalsize(kinfo->fdt);
>>>> +    kinfo->initrd_paddr = end - initrd_len;
>>>> +    kinfo->initrd_paddr &= ~((1 << 20) - 1);
>>>
>>> 1MB aligned, why not 2 like most things?
>>
>> A mistake, I will fix it in the next patch series.
> 
> Actually, this extra alignment makes me think that maybe new_size needs
> to account for the slop too? Does it?


> Otherwise start + 128M + new_size doesn't make it such that
> {dtb,initrd}_paddr are actually above 128M?
> 

Right, what about?

#define ALIGN_2MB(size) ((len) + ((1 << 20 - 1)) & (~((1 << 20) - 1))
new_size = ALIGN_2MB(dtb_size) + ALIGN_2MB(initrd_size)

-- 
Julien Grall