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

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

On 08/28/2013 09:16 AM, 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 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>
> ---
> This is technically v2 of "xen: arm: load FDT below 0.5G"
> ---
>  xen/arch/arm/domain_build.c |   16 +++++++++++-----
>  xen/arch/arm/kernel.c       |   25 ++++++++++++++++++++++---
>  2 files changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9ca663a..8ce8e60 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);
>      /* Align the address to 2Mb. Linux only requires 4 byte alignment */
>      kinfo->dtb_paddr &= ~((2 << 20) - 1);
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index f12f895..e8f3f3b 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -211,11 +211,30 @@ 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

Is it possible to start an ARM64 dom0 with only 128MB of RAM? If yes, we
need the same code in kernel_try_zimage64_prepare.

> +         * 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 - size;

With this solution, it's possible to have size > load_end. load_addr
will have a wrong address.

Furthermore, I think you need to use (end - start) instead of size. On
the current implementation size can be greater than the real image size.
If you want to stick on your solution, then you must update zimage.len.

> +    }
>      else
>          info->zimage.load_addr = start;

This is not a part of the patch. The line above is wrong, Xen needs to
check if start belongs to the dom0 RAM.

>      info->zimage.len = end - start;


-- 
Julien Grall