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

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

[Ian Campbell]

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
+         * 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;
+    }
     else
         info->zimage.load_addr = start;
     info->zimage.len = end - start;
-- 
1.7.2.5

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

[Julien Grall]

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

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

[Ian Campbell]

On Wed, 2013-08-28 at 11:43 +0100, Julien Grall wrote:
> > 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?

Yes, but...

> If yes, we need the same code in kernel_try_zimage64_prepare.

On arm64 the load address is mandated in the zImage head (text_offset
field) so we don't get to choose.

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

load_end is absolute (i.e. it incorporates the ram base address) while
size is relative. I think the appropriate check is that load_addr is >=
bank[0].start. I can incorporate this into the check that the kernel is
within the dom0 memory which you suggest below.

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

Right, I don't know whether end-start or load_addr is better (since the
former will cut of an appended dtb -- but that might be a feature) but
since the len is already end-start I will make this consistent and if we
want to change we can do so later.

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

I think we will end up doing this when it tries to load the kernel into
non-existent ram...

Ian.