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

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

On 09/17/2013 04:28 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>
Acked-by: Julien Grall <julien.grall@linaro.org>
> ---
> v3: Improve checking on final location of dtb, to make sure it is in guest RAM
>     Use new_size for dtbsize where possible
>     Remove pointless subtractions of start when it is always zero
> 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 | 23 +++++++++++++++--------
>  xen/arch/arm/kernel.c       | 27 ++++++++++++++++++++++++---
>  2 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 77e2a61..34d3320 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -503,19 +503,27 @@ 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);
> -    kinfo->dtb_paddr = end - fdt_totalsize(kinfo->fdt);
> +    end = MIN(kinfo->mem.bank[0].start + (128<<20) + new_size, end);
> +
> +    kinfo->dtb_paddr = end - new_size;
> +
>      /* Align the address to 2Mb. Linux only requires 4 byte alignment */
>      kinfo->dtb_paddr &= ~((2 << 20) - 1);
>  
> -    if ( fdt_totalsize(kinfo->fdt) > end )
> +    if ( kinfo->dtb_paddr < kinfo->mem.bank[0].start ||
> +         kinfo->mem.bank[0].start + new_size > end )
>      {
>          printk(XENLOG_ERR "Not enough memory in the first bank for "
>                 "the device tree.");
> @@ -523,7 +531,6 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>          goto err;
>      }
>  
> -
>      return 0;
>  
>    err:
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index f12f895..7a91d60 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;
> +        /* Align to 2MB */
> +        info->zimage.load_addr &= ~((2 << 20) - 1);
> +    }
>      else
>          info->zimage.load_addr = start;
>      info->zimage.len = end - start;
> 


-- 
Julien Grall