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

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

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?

>      /* 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?

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

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