Re: [PATCH v2] xen: arm: rework placement of fdt in initial dom0 memory map

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

On 09/13/2013 04:43 PM, Ian Campbell wrote:
> On Fri, 2013-09-13 at 16:18 +0100, Julien Grall wrote:
>> On 09/13/2013 03:26 PM, Ian Campbell wrote:
>>> The 32-bit Linux kernel uses its lowmem direct mapping to access the FDT. The
>>> lowmem mapping is around 0.75GiB but varies depending on the kernel's .config.
>>> Our current scheme of loading the FDT as high as 4GB therefore fails with
>>> larger amounts of dom0 RAM.
>>>
>>> The upstream documentation has recently been update to provide more guidance
>>> <http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=7824/1>. In
>>> accordance with this load the kernel just below 128MiB (aligned to 2MB) and
>>> the FDT just above, or if there is less RAM available then as high as
>>> possible.
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>> v2: Use the kernels own reported size rather than the size of the module. This
>>>     is consistent with the amount we will actually copy (zimage.len)
>>> v1: This is technically v2 of "xen: arm: load FDT below 0.5G"
>>> ---
>>>  xen/arch/arm/domain_build.c |   16 +++++++++++-----
>>>  xen/arch/arm/kernel.c       |   27 ++++++++++++++++++++++++---
>>>  2 files changed, 35 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 77e2a61..172c985 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -503,14 +503,20 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>>>      if ( ret < 0 )
>>>          goto err;
>>>  
>>> +    /* Actual new size */
>>> +    new_size = fdt_totalsize(kinfo->fdt);
>>> +
>>>      /*
>>> -     * DTB must be load below 4GiB and far enough from linux (Linux uses
>>> -     * the space after it to decompress)
>>> -     * Load the DTB at the end of the first bank, while ensuring it is
>>> -     * also below 4G
>>> +     * DTB must be loaded such that it does not conflict with the
>>> +     * kernel decompressor. For 32-bit Linux Documentation/arm/Booting
>>> +     * recommends just after the 128MB boundary while for 64-bit Linux
>>> +     * 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.
>>>       */
>>>      end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size;
>>> -    end = MIN(1ull << 32, end);
>>> +    end = MIN(kinfo->mem.bank[0].start + (128<<20) + new_size, end);
>>> +
>>>      kinfo->dtb_paddr = end - fdt_totalsize(kinfo->fdt);
>>
>> Can you use new_size here?
> 
> Yes.
> 
>>
>>>      /* Align the address to 2Mb. Linux only requires 4 byte alignment */
>>>      kinfo->dtb_paddr &= ~((2 << 20) - 1);
>>
>> I have noticed that the check below is wrong
>> if ( fdt_totalsize(...) > end )
>>
>> Can you fix the check in this patch?
> 
> What's wrong with it?
> 
> if ( fdt_totalsize(...) > end ) then the dtb_paddr will have underflowed
> and we panic. Or is that not what you are referring to?

end is an absolute address and fdt_totalsize(...) is relative.

I think the check should be
(fdt_totalsize(...) + mem.bank[0].start) > end

> 
>>
>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>>> index f12f895..e4c0981 100644
>>> --- a/xen/arch/arm/kernel.c
>>> +++ b/xen/arch/arm/kernel.c
>>> @@ -211,11 +211,32 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info,
>>>      info->zimage.kernel_addr = addr;
>>>  
>>>      /*
>>> -     * If start is zero, the zImage is position independent -- load it
>>> -     * at 32k from start of RAM.
>>> +     * If start is zero, the zImage is position independent, in this
>>> +     * case Documentation/arm/Booting recommends loading below 128MiB
>>> +     * and above 32MiB. Load it as high as possible within these
>>> +     * constraints, while also avoiding the DTB.
>>>       */
>>>      if (start == 0)
>>> -        info->zimage.load_addr = info->mem.bank[0].start + 0x8000;
>>> +    {
>>> +        paddr_t load_end;
>>> +
>>> +        load_end = info->mem.bank[0].start + info->mem.bank[0].size;
>>> +        load_end = MIN(info->mem.bank[0].start + (128<<20), load_end);
>>> +
>>> +        /*
>>> +         * FDT is loaded above 128M or as high as possible, so the
>>> +         * only way we can clash is if we have <=128MB, in which case
>>> +         * FDT will be right at the end and so dtb_paddr will be below
>>> +         * the proposed kernel load address. Move the kernel down if
>>> +         * necessary.
>>> +         */
>>> +        if ( load_end >= info->dtb_paddr )
>>> +            load_end = info->dtb_paddr;
>>> +
>>> +        info->zimage.load_addr = load_end - end + start;
>>
>> Actually start is always equals to 0, so you don't need to add it.
> 
> Oh yes.
> 
>>
>> In the future, we will need some check here to verify the kernel belongs
>> to bank 0 and start won't.
>>
>>> +        /* Align to 2MB */
>>> +        info->zimage.load_addr &= ~((2 << 20) - 1);
>>> +    }
>>>      else
>>>          info->zimage.load_addr = start;
>>>      info->zimage.len = end - start;
>>>
>>
>>
> 
> 


-- 
Julien Grall