[PATCH] xen: arm: load FDT below 0.5G

[PATCH] xen: arm: load FDT below 0.5G

[Ian Campbell]

From: Ian Campbell <ian.campbell@citrix.com>

The 32-bit Linux kernel uses its lowmem direct mapping to access the FDT. The
lowmem mapping is around 3/4GiB but varies depending on the kernel's .config.
Use a limit of 1/2G to be safe.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain_build.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 056c9df..ff9e679 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -504,13 +504,14 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
         goto err;
 
     /*
-     * 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 below around 0.5GiB and far enough from
+     * linux (Linux uses the space after it to decompress).
+     *
+     * Load the DTB as high as possible in the first bank, while
+     * ensuring it is also below 0.5G
      */
     end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size;
-    end = MIN(1ull << 32, end);
+    end = MIN(kinfo->mem.bank[0].start + (1ull << 29), 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);
-- 
1.8.3.2

Re: [PATCH] xen: arm: load FDT below 0.5G

[Julien Grall]

On 07/23/2013 06:12 PM, Ian Campbell wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> The 32-bit Linux kernel uses its lowmem direct mapping to access the FDT. The
> lowmem mapping is around 3/4GiB but varies depending on the kernel's .config.
> Use a limit of 1/2G to be safe.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

I'm curious, how did you find this value? I can't easily find a specific
range for the lowmem direct mapping.

> ---
>  xen/arch/arm/domain_build.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 056c9df..ff9e679 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -504,13 +504,14 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>          goto err;
>  
>      /*
> -     * 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 below around 0.5GiB and far enough from
> +     * linux (Linux uses the space after it to decompress).
> +     *
> +     * Load the DTB as high as possible in the first bank, while
> +     * ensuring it is also below 0.5G
>       */
>      end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size;
> -    end = MIN(1ull << 32, end);
> +    end = MIN(kinfo->mem.bank[0].start + (1ull << 29), 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);
> 

Re: [PATCH] xen: arm: load FDT below 0.5G

[Ian Campbell]

On Wed, 2013-07-24 at 14:44 +0100, Julien Grall wrote:
> On 07/23/2013 06:12 PM, Ian Campbell wrote:
> > From: Ian Campbell <ian.campbell@citrix.com>
> > 
> > The 32-bit Linux kernel uses its lowmem direct mapping to access the FDT. The
> > lowmem mapping is around 3/4GiB but varies depending on the kernel's .config.
> > Use a limit of 1/2G to be safe.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I'm curious, how did you find this value? I can't easily find a specific
> range for the lowmem direct mapping.

There isn't one...

With a standard 3:1 user:kernel split there is ~1G of kernel address
space, but some of that is taken up by vmalloc areas, fixmap areas etc.
My experience is that there is generally around 700-800M of actual 1:1
mapping of RAM, but the precise value depends on the kernel .config
(e.g. how many fixmaps are implied by the settings etc). 512MB is
therefore just a conservative guess...

BTW I asked for confirmation on linux-arm-kernel but I don't see that in
my l-a-k folder or the archives -- did you get the copy I CCd to you?
Subject is "Requirements for FDT load address on ARM".

Ian.

Re: [PATCH] xen: arm: load FDT below 0.5G

[Ian Campbell]

On Wed, 2013-07-24 at 15:37 +0100, Ian Campbell wrote:
> BTW I asked for confirmation on linux-arm-kernel but I don't see that in
> my l-a-k folder or the archives -- did you get the copy I CCd to you?
> Subject is "Requirements for FDT load address on ARM".

There was some discussion on L-A-K and I think we know what to do, see
http://thread.gmane.org/gmane.linux.ports.arm.kernel/254538 and
http://thread.gmane.org/gmane.linux.ports.arm.kernel/255716

The updated patch would look like the below (although I may try and
combine place+check_overlap+load into a single function, which tries a
bit harder to find a good address by looping until overlap == false).

However since this has grown a dependency on the 64-bit guest series I'm
going to hold off until that goes in (it looks very close) and then
rebase + resend this one. Hopefully in the meantime the Linux patch will
gain some sort of consensus.

Ian.

commit 176dafbc2f916f369450e647a33806c13270be15
Author: Ian Campbell <ian.campbell@citrix.com>
Date:   Tue Jul 23 06:05:29 2013 +0100

    xen: arm: load FDT at correct address
    
    The 64-bit Documentation/arm64/booting.txt requires that the FDT be below
    512MiB. On 32-bit things are less clear but discussion at
    http://thread.gmane.org/gmane.linux.ports.arm.kernel/254538 suggests that
    below 256MiB should be good. I've submitted a patch to
    Documentation/arm/Booting to cover this.
    
    Also note that 64-bit requires 2MiB alignment for the FDT. We already do this
    but update the comment.
    
    Locating the fdt needs to be called after prepare_kernel so we know which sort
    of kernel it is, so split out into a separate "place_fdt".
    
    Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
    ---
    v2: Load below 256MiB on 32-bit and below 512MiB on 64-bit.

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 155b436..6d92686 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -477,7 +477,6 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
     void *fdt;
     int new_size;
     int ret;
-    paddr_t end;
 
     kinfo->unassigned_mem = dom0_mem;
 
@@ -502,33 +501,50 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
     if ( ret < 0 )
         goto err;
 
+    return 0;
+
+  err:
+    printk("Device tree generation failed (%d).\n", ret);
+    xfree(kinfo->fdt);
+    return -EINVAL;
+}
+
+static int place_dtb(struct domain *d, struct kernel_info *kinfo)
+{
+    paddr_t end;
+
     /*
-     * 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 below 256MB while for 64-bit Linux the
+     * recommendation in Documentation/arm64/booting.txt is below
+     * 512MB.
      */
     end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size;
-    end = MIN(1ull << 32, end);
+    switch (kinfo->type) {
+    case DOMAIN_PV32:
+        end = MIN(kinfo->mem.bank[0].start + (256<<20), end);
+        break;
+    case DOMAIN_PV64:
+        end = MIN(kinfo->mem.bank[0].start + (512<<20), end);
+        break;
+    }
+
     kinfo->dtb_paddr = end - fdt_totalsize(kinfo->fdt);
-    /* Align the address to 2Mb. Linux only requires 4 byte alignment */
+    /*
+     * Align the address to 2Mb, as required by 64-bit ARM64
+     * Linux. 32-bit ARM Linux only requires 4 byte alignment
+     */
     kinfo->dtb_paddr &= ~((2 << 20) - 1);
 
     if ( fdt_totalsize(kinfo->fdt) > end )
     {
         printk(XENLOG_ERR "Not enough memory in the first bank for "
                "the device tree.");
-        ret = -FDT_ERR_XEN(EINVAL);
-        goto err;
+        return -ENOMEM;
     }
 
-
     return 0;
-
-  err:
-    printk("Device tree generation failed (%d).\n", ret);
-    xfree(kinfo->fdt);
-    return -EINVAL;
 }
 
 static void dtb_load(struct kernel_info *kinfo)
@@ -567,6 +583,10 @@ int construct_dom0(struct domain *d)
     if ( rc < 0 )
         return rc;
 
+    rc = place_dtb(d, &kinfo);
+    if ( rc < 0 )
+        return rc;
+
     map_devices_from_device_tree(d);
     rc = platform_specific_mapping(d);
     if ( rc < 0 )

Re: [PATCH] xen: arm: load FDT below 0.5G

[Julien Grall]

On 29 July 2013 15:46, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2013-07-24 at 15:37 +0100, Ian Campbell wrote:
>> BTW I asked for confirmation on linux-arm-kernel but I don't see that in
>> my l-a-k folder or the archives -- did you get the copy I CCd to you?
>> Subject is "Requirements for FDT load address on ARM".
>
> There was some discussion on L-A-K and I think we know what to do, see
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/254538 and
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/255716
>
> The updated patch would look like the below (although I may try and
> combine place+check_overlap+load into a single function, which tries a
> bit harder to find a good address by looping until overlap == false).

Do you plan to support position-dependent DOM0 kernel? If no, as the
kernel will be relocated to the same address, I don't think we need to
automatically find the DTB base address.

> However since this has grown a dependency on the 64-bit guest series I'm
> going to hold off until that goes in (it looks very close) and then
> rebase + resend this one. Hopefully in the meantime the Linux patch will
> gain some sort of consensus.
>
> Ian.
>
> commit 176dafbc2f916f369450e647a33806c13270be15
> Author: Ian Campbell <ian.campbell@citrix.com>
> Date:   Tue Jul 23 06:05:29 2013 +0100
>
>     xen: arm: load FDT at correct address
>
>     The 64-bit Documentation/arm64/booting.txt requires that the FDT be below
>     512MiB. On 32-bit things are less clear but discussion at
>     http://thread.gmane.org/gmane.linux.ports.arm.kernel/254538 suggests that
>     below 256MiB should be good. I've submitted a patch to
>     Documentation/arm/Booting to cover this.
>
>     Also note that 64-bit requires 2MiB alignment for the FDT. We already do this
>     but update the comment.
>
>     Locating the fdt needs to be called after prepare_kernel so we know which sort
>     of kernel it is, so split out into a separate "place_fdt".
>
>     Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
       Acked-by: Julien Grall <julien.grall@linaro.org>
>     ---
>     v2: Load below 256MiB on 32-bit and below 512MiB on 64-bit.
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 155b436..6d92686 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -477,7 +477,6 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>      void *fdt;
>      int new_size;
>      int ret;
> -    paddr_t end;
>
>      kinfo->unassigned_mem = dom0_mem;
>
> @@ -502,33 +501,50 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>      if ( ret < 0 )
>          goto err;
>
> +    return 0;
> +
> +  err:
> +    printk("Device tree generation failed (%d).\n", ret);
> +    xfree(kinfo->fdt);
> +    return -EINVAL;
> +}
> +
> +static int place_dtb(struct domain *d, struct kernel_info *kinfo)
> +{
> +    paddr_t end;
> +
>      /*
> -     * 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 below 256MB while for 64-bit Linux the
> +     * recommendation in Documentation/arm64/booting.txt is below
> +     * 512MB.
>       */
>      end = kinfo->mem.bank[0].start + kinfo->mem.bank[0].size;
> -    end = MIN(1ull << 32, end);
> +    switch (kinfo->type) {
> +    case DOMAIN_PV32:
> +        end = MIN(kinfo->mem.bank[0].start + (256<<20), end);
> +        break;
> +    case DOMAIN_PV64:
> +        end = MIN(kinfo->mem.bank[0].start + (512<<20), end);
> +        break;
> +    }
> +
>      kinfo->dtb_paddr = end - fdt_totalsize(kinfo->fdt);
> -    /* Align the address to 2Mb. Linux only requires 4 byte alignment */
> +    /*
> +     * Align the address to 2Mb, as required by 64-bit ARM64
> +     * Linux. 32-bit ARM Linux only requires 4 byte alignment
> +     */
>      kinfo->dtb_paddr &= ~((2 << 20) - 1);
>
>      if ( fdt_totalsize(kinfo->fdt) > end )
>      {
>          printk(XENLOG_ERR "Not enough memory in the first bank for "
>                 "the device tree.");
> -        ret = -FDT_ERR_XEN(EINVAL);
> -        goto err;
> +        return -ENOMEM;
>      }
>
> -
>      return 0;
> -
> -  err:
> -    printk("Device tree generation failed (%d).\n", ret);
> -    xfree(kinfo->fdt);
> -    return -EINVAL;
>  }
>
>  static void dtb_load(struct kernel_info *kinfo)
> @@ -567,6 +583,10 @@ int construct_dom0(struct domain *d)
>      if ( rc < 0 )
>          return rc;
>
> +    rc = place_dtb(d, &kinfo);
> +    if ( rc < 0 )
> +        return rc;
> +
>      map_devices_from_device_tree(d);
>      rc = platform_specific_mapping(d);
>      if ( rc < 0 )
>
>



-- 
Julien Grall

Re: [PATCH] xen: arm: load FDT below 0.5G

[Ian Campbell]

On Tue, 2013-07-30 at 10:53 +0100, Julien Grall wrote:
> On 29 July 2013 15:46, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Wed, 2013-07-24 at 15:37 +0100, Ian Campbell wrote:
> >> BTW I asked for confirmation on linux-arm-kernel but I don't see that in
> >> my l-a-k folder or the archives -- did you get the copy I CCd to you?
> >> Subject is "Requirements for FDT load address on ARM".
> >
> > There was some discussion on L-A-K and I think we know what to do, see
> > http://thread.gmane.org/gmane.linux.ports.arm.kernel/254538 and
> > http://thread.gmane.org/gmane.linux.ports.arm.kernel/255716
> >
> > The updated patch would look like the below (although I may try and
> > combine place+check_overlap+load into a single function, which tries a
> > bit harder to find a good address by looping until overlap == false).
> 
> Do you plan to support position-dependent DOM0 kernel? If no, as the
> kernel will be relocated to the same address, I don't think we need to
> automatically find the DTB base address.

What do you mean? Even if the kernel is not PIC it will still have to
decompress itself somewhere and things move around in RAM etc and
potentially clobber the DTB.

But anyway, yes we should be supporting PIC kernel, in fact I have a
suspicion we only support PIC kernels...

> > commit 176dafbc2f916f369450e647a33806c13270be15
> > Author: Ian Campbell <ian.campbell@citrix.com>
> > Date:   Tue Jul 23 06:05:29 2013 +0100
> >
> >     xen: arm: load FDT at correct address
> >
> >     The 64-bit Documentation/arm64/booting.txt requires that the FDT be below
> >     512MiB. On 32-bit things are less clear but discussion at
> >     http://thread.gmane.org/gmane.linux.ports.arm.kernel/254538 suggests that
> >     below 256MiB should be good. I've submitted a patch to
> >     Documentation/arm/Booting to cover this.
> >
> >     Also note that 64-bit requires 2MiB alignment for the FDT. We already do this
> >     but update the comment.
> >
> >     Locating the fdt needs to be called after prepare_kernel so we know which sort
> >     of kernel it is, so split out into a separate "place_fdt".
> >
> >     Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>        Acked-by: Julien Grall <julien.grall@linaro.org>

Thanks, I'm not totally sure this will be the final form but if it
doesn't change too much I will keep this.

Re: [PATCH] xen: arm: load FDT below 0.5G

[Julien Grall]

On 30 July 2013 10:57, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2013-07-30 at 10:53 +0100, Julien Grall wrote:
>> On 29 July 2013 15:46, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Wed, 2013-07-24 at 15:37 +0100, Ian Campbell wrote:
>> >> BTW I asked for confirmation on linux-arm-kernel but I don't see that in
>> >> my l-a-k folder or the archives -- did you get the copy I CCd to you?
>> >> Subject is "Requirements for FDT load address on ARM".
>> >
>> > There was some discussion on L-A-K and I think we know what to do, see
>> > http://thread.gmane.org/gmane.linux.ports.arm.kernel/254538 and
>> > http://thread.gmane.org/gmane.linux.ports.arm.kernel/255716
>> >
>> > The updated patch would look like the below (although I may try and
>> > combine place+check_overlap+load into a single function, which tries a
>> > bit harder to find a good address by looping until overlap == false).
>>
>> Do you plan to support position-dependent DOM0 kernel? If no, as the
>> kernel will be relocated to the same address, I don't think we need to
>> automatically find the DTB base address.
>
> What do you mean? Even if the kernel is not PIC it will still have to
> decompress itself somewhere and things move around in RAM etc and
> potentially clobber the DTB.

You can only guess theses different addresses. Xen is unable to know
the final memory layout for DOM 0.

If I understand the email from Nicolas Pitre
(http://thread.gmane.org/gmane.linux.ports.arm.kernel/254538/focus=254640)
we are safe in most of the case with the current computation of the
DTB address.

> But anyway, yes we should be supporting PIC kernel, in fact I have a
> suspicion we only support PIC kernels...
>
>> > commit 176dafbc2f916f369450e647a33806c13270be15
>> > Author: Ian Campbell <ian.campbell@citrix.com>
>> > Date:   Tue Jul 23 06:05:29 2013 +0100
>> >
>> >     xen: arm: load FDT at correct address
>> >
>> >     The 64-bit Documentation/arm64/booting.txt requires that the FDT be below
>> >     512MiB. On 32-bit things are less clear but discussion at
>> >     http://thread.gmane.org/gmane.linux.ports.arm.kernel/254538 suggests that
>> >     below 256MiB should be good. I've submitted a patch to
>> >     Documentation/arm/Booting to cover this.
>> >
>> >     Also note that 64-bit requires 2MiB alignment for the FDT. We already do this
>> >     but update the comment.
>> >
>> >     Locating the fdt needs to be called after prepare_kernel so we know which sort
>> >     of kernel it is, so split out into a separate "place_fdt".
>> >
>> >     Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>        Acked-by: Julien Grall <julien.grall@linaro.org>
>
> Thanks, I'm not totally sure this will be the final form but if it
> doesn't change too much I will keep this.
>
>



-- 
Julien Grall