[Qemu-devel] [PATCH for-4.1? 0/2] arm: further improve initrd positioning

[Qemu-devel] [PATCH for-4.1? 0/2] arm: further improve initrd positioning

[Peter Maydell]

In commit e6b2b20d9735d4ef we made the boot loader code try to avoid
putting the initrd on top of the kernel.  However the expression used
to calculate the start of the initrd:

    info->initrd_start = info->loader_start +
        MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);

incorrectly uses 'kernel_size' as the offset within RAM of the
highest address to avoid.  This is incorrect because the kernel
doesn't start at address 0, but slightly higher than that.  This
means that we can still incorrectly end up overlaying the initrd on
the kernel in some cases, for example:

* The kernel's image_size is 0x0a7a8000
* The kernel was loaded at   0x40080000
* The end of the kernel is   0x4A828000
* The DTB was loaded at      0x4a800000

To get this right we need to track the actual highest address used
by the kernel and use that rather than kernel_size. We already
trace the low_addr and high_addr for ELF images; set them
also for the various other image types we support, and then use
high_addr as the lowest allowed address for the initrd.

Patch 1 just does a preliminary variable rename; patch 2 is the meat.

Only very lightly tested...

Marked as 'maybe for 4.1' because it is a bug fix and to code which
is new in 4.1. OTOH cases that fail now would have failed with 4.0
so it is not a regression strictly speaking. And we're getting
steadily closer to release and I haven't very heavily tested this
patch. I incline towards including it, overall.

thanks
-- PMM

Peter Maydell (2):
  hw/arm/boot: Rename elf_{low,high}_addr to image_{low,high}_addr
  hw/arm/boot: Further improve initrd positioning code

 hw/arm/boot.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH for-4.1? 1/2] hw/arm/boot: Rename elf_{low, high}_addr to image_{low, high}_addr

[Peter Maydell]

Rename the elf_low_addr and elf_high_addr variables to image_low_addr
and image_high_addr -- in the next commit we will extend them to
be set for other kinds of image file and not just ELF files.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 1fb24fbef27..b7b31753aca 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -986,7 +986,9 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
     int kernel_size;
     int initrd_size;
     int is_linux = 0;
-    uint64_t elf_entry, elf_low_addr, elf_high_addr;
+    uint64_t elf_entry;
+    /* Addresses of first byte used and first byte not used by the image */
+    uint64_t image_low_addr, image_high_addr;
     int elf_machine;
     hwaddr entry;
     static const ARMInsnFixup *primary_loader;
@@ -1014,24 +1016,24 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
         info->nb_cpus = 1;
 
     /* Assume that raw images are linux kernels, and ELF images are not.  */
-    kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr,
-                               &elf_high_addr, elf_machine, as);
+    kernel_size = arm_load_elf(info, &elf_entry, &image_low_addr,
+                               &image_high_addr, elf_machine, as);
     if (kernel_size > 0 && have_dtb(info)) {
         /*
          * If there is still some room left at the base of RAM, try and put
          * the DTB there like we do for images loaded with -bios or -pflash.
          */
-        if (elf_low_addr > info->loader_start
-            || elf_high_addr < info->loader_start) {
+        if (image_low_addr > info->loader_start
+            || image_high_addr < info->loader_start) {
             /*
-             * Set elf_low_addr as address limit for arm_load_dtb if it may be
+             * Set image_low_addr as address limit for arm_load_dtb if it may be
              * pointing into RAM, otherwise pass '0' (no limit)
              */
-            if (elf_low_addr < info->loader_start) {
-                elf_low_addr = 0;
+            if (image_low_addr < info->loader_start) {
+                image_low_addr = 0;
             }
             info->dtb_start = info->loader_start;
-            info->dtb_limit = elf_low_addr;
+            info->dtb_limit = image_low_addr;
         }
     }
     entry = elf_entry;
-- 
2.20.1

Re: [Qemu-devel] [Qemu-arm] [PATCH for-4.1? 1/2] hw/arm/boot: Rename elf_{low, high}_addr to image_{low, high}_addr

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Rename the elf_low_addr and elf_high_addr variables to image_low_addr
> and image_high_addr -- in the next commit we will extend them to
> be set for other kinds of image file and not just ELF files.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  hw/arm/boot.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 1fb24fbef27..b7b31753aca 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -986,7 +986,9 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>      int kernel_size;
>      int initrd_size;
>      int is_linux = 0;
> -    uint64_t elf_entry, elf_low_addr, elf_high_addr;
> +    uint64_t elf_entry;
> +    /* Addresses of first byte used and first byte not used by the image */
> +    uint64_t image_low_addr, image_high_addr;
>      int elf_machine;
>      hwaddr entry;
>      static const ARMInsnFixup *primary_loader;
> @@ -1014,24 +1016,24 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>          info->nb_cpus = 1;
>
>      /* Assume that raw images are linux kernels, and ELF images are not.  */
> -    kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr,
> -                               &elf_high_addr, elf_machine, as);
> +    kernel_size = arm_load_elf(info, &elf_entry, &image_low_addr,
> +                               &image_high_addr, elf_machine, as);
>      if (kernel_size > 0 && have_dtb(info)) {
>          /*
>           * If there is still some room left at the base of RAM, try and put
>           * the DTB there like we do for images loaded with -bios or -pflash.
>           */
> -        if (elf_low_addr > info->loader_start
> -            || elf_high_addr < info->loader_start) {
> +        if (image_low_addr > info->loader_start
> +            || image_high_addr < info->loader_start) {
>              /*
> -             * Set elf_low_addr as address limit for arm_load_dtb if it may be
> +             * Set image_low_addr as address limit for arm_load_dtb if it may be
>               * pointing into RAM, otherwise pass '0' (no limit)
>               */
> -            if (elf_low_addr < info->loader_start) {
> -                elf_low_addr = 0;
> +            if (image_low_addr < info->loader_start) {
> +                image_low_addr = 0;
>              }
>              info->dtb_start = info->loader_start;
> -            info->dtb_limit = elf_low_addr;
> +            info->dtb_limit = image_low_addr;
>          }
>      }
>      entry = elf_entry;


--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH for-4.1? 1/2] hw/arm/boot: Rename elf_{low, high}_addr to image_{low, high}_addr

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Rename the elf_low_addr and elf_high_addr variables to image_low_addr
> and image_high_addr -- in the next commit we will extend them to
> be set for other kinds of image file and not just ELF files.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  hw/arm/boot.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 1fb24fbef27..b7b31753aca 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -986,7 +986,9 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>      int kernel_size;
>      int initrd_size;
>      int is_linux = 0;
> -    uint64_t elf_entry, elf_low_addr, elf_high_addr;
> +    uint64_t elf_entry;
> +    /* Addresses of first byte used and first byte not used by the image */
> +    uint64_t image_low_addr, image_high_addr;
>      int elf_machine;
>      hwaddr entry;
>      static const ARMInsnFixup *primary_loader;
> @@ -1014,24 +1016,24 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>          info->nb_cpus = 1;
>
>      /* Assume that raw images are linux kernels, and ELF images are not.  */
> -    kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr,
> -                               &elf_high_addr, elf_machine, as);
> +    kernel_size = arm_load_elf(info, &elf_entry, &image_low_addr,
> +                               &image_high_addr, elf_machine, as);
>      if (kernel_size > 0 && have_dtb(info)) {
>          /*
>           * If there is still some room left at the base of RAM, try and put
>           * the DTB there like we do for images loaded with -bios or -pflash.
>           */
> -        if (elf_low_addr > info->loader_start
> -            || elf_high_addr < info->loader_start) {
> +        if (image_low_addr > info->loader_start
> +            || image_high_addr < info->loader_start) {
>              /*
> -             * Set elf_low_addr as address limit for arm_load_dtb if it may be
> +             * Set image_low_addr as address limit for arm_load_dtb if it may be
>               * pointing into RAM, otherwise pass '0' (no limit)
>               */
> -            if (elf_low_addr < info->loader_start) {
> -                elf_low_addr = 0;
> +            if (image_low_addr < info->loader_start) {
> +                image_low_addr = 0;
>              }
>              info->dtb_start = info->loader_start;
> -            info->dtb_limit = elf_low_addr;
> +            info->dtb_limit = image_low_addr;
>          }
>      }
>      entry = elf_entry;


--
Alex Bennée

Re: [Qemu-devel] [PATCH for-4.1? 1/2] hw/arm/boot: Rename elf_{low, high}_addr to image_{low, high}_addr

[Philippe Mathieu-Daudé]

On 7/22/19 5:18 PM, Peter Maydell wrote:
> Rename the elf_low_addr and elf_high_addr variables to image_low_addr
> and image_high_addr -- in the next commit we will extend them to
> be set for other kinds of image file and not just ELF files.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/boot.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 1fb24fbef27..b7b31753aca 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -986,7 +986,9 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>      int kernel_size;
>      int initrd_size;
>      int is_linux = 0;
> -    uint64_t elf_entry, elf_low_addr, elf_high_addr;
> +    uint64_t elf_entry;
> +    /* Addresses of first byte used and first byte not used by the image */
> +    uint64_t image_low_addr, image_high_addr;
>      int elf_machine;
>      hwaddr entry;
>      static const ARMInsnFixup *primary_loader;
> @@ -1014,24 +1016,24 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>          info->nb_cpus = 1;
>  
>      /* Assume that raw images are linux kernels, and ELF images are not.  */
> -    kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr,
> -                               &elf_high_addr, elf_machine, as);
> +    kernel_size = arm_load_elf(info, &elf_entry, &image_low_addr,
> +                               &image_high_addr, elf_machine, as);
>      if (kernel_size > 0 && have_dtb(info)) {
>          /*
>           * If there is still some room left at the base of RAM, try and put
>           * the DTB there like we do for images loaded with -bios or -pflash.
>           */
> -        if (elf_low_addr > info->loader_start
> -            || elf_high_addr < info->loader_start) {
> +        if (image_low_addr > info->loader_start
> +            || image_high_addr < info->loader_start) {
>              /*
> -             * Set elf_low_addr as address limit for arm_load_dtb if it may be
> +             * Set image_low_addr as address limit for arm_load_dtb if it may be
>               * pointing into RAM, otherwise pass '0' (no limit)
>               */
> -            if (elf_low_addr < info->loader_start) {
> -                elf_low_addr = 0;
> +            if (image_low_addr < info->loader_start) {
> +                image_low_addr = 0;
>              }
>              info->dtb_start = info->loader_start;
> -            info->dtb_limit = elf_low_addr;
> +            info->dtb_limit = image_low_addr;
>          }
>      }
>      entry = elf_entry;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[Qemu-devel] [PATCH for-4.1? 2/2] hw/arm/boot: Further improve initrd positioning code

[Peter Maydell]

In commit e6b2b20d9735d4ef we made the boot loader code try to avoid
putting the initrd on top of the kernel.  However the expression used
to calculate the start of the initrd:

    info->initrd_start = info->loader_start +
        MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);

incorrectly uses 'kernel_size' as the offset within RAM of the
highest address to avoid.  This is incorrect because the kernel
doesn't start at address 0, but slightly higher than that.  This
means that we can still incorrectly end up overlaying the initrd on
the kernel in some cases, for example:

* The kernel's image_size is 0x0a7a8000
* The kernel was loaded at   0x40080000
* The end of the kernel is   0x4A828000
* The DTB was loaded at      0x4a800000

To get this right we need to track the actual highest address used
by the kernel and use that rather than kernel_size. We already
set image_low_addr and image_high_addr for ELF images; set them
also for the various other image types we support, and then use
image_high_addr as the lowest allowed address for the initrd.
(We don't use image_low_addr, but we set it for consistency
with the existing code path for ELF files.)

Fixes: e6b2b20d9735d4ef
Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index b7b31753aca..c2b89b3bb9b 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -988,7 +988,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
     int is_linux = 0;
     uint64_t elf_entry;
     /* Addresses of first byte used and first byte not used by the image */
-    uint64_t image_low_addr, image_high_addr;
+    uint64_t image_low_addr = 0, image_high_addr = 0;
     int elf_machine;
     hwaddr entry;
     static const ARMInsnFixup *primary_loader;
@@ -1041,17 +1041,29 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
         uint64_t loadaddr = info->loader_start + KERNEL_NOLOAD_ADDR;
         kernel_size = load_uimage_as(info->kernel_filename, &entry, &loadaddr,
                                      &is_linux, NULL, NULL, as);
+        if (kernel_size >= 0) {
+            image_low_addr = loadaddr;
+            image_high_addr = image_low_addr + kernel_size;
+        }
     }
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
         kernel_size = load_aarch64_image(info->kernel_filename,
                                          info->loader_start, &entry, as);
         is_linux = 1;
+        if (kernel_size >= 0) {
+            image_low_addr = entry;
+            image_high_addr = image_low_addr + kernel_size;
+        }
     } else if (kernel_size < 0) {
         /* 32-bit ARM */
         entry = info->loader_start + KERNEL_LOAD_ADDR;
         kernel_size = load_image_targphys_as(info->kernel_filename, entry,
                                              ram_end - KERNEL_LOAD_ADDR, as);
         is_linux = 1;
+        if (kernel_size >= 0) {
+            image_low_addr = entry;
+            image_high_addr = image_low_addr + kernel_size;
+        }
     }
     if (kernel_size < 0) {
         error_report("could not load kernel '%s'", info->kernel_filename);
@@ -1083,7 +1095,10 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
      * we might still make a bad choice here.
      */
     info->initrd_start = info->loader_start +
-        MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);
+        MIN(info->ram_size / 2, 128 * 1024 * 1024);
+    if (image_high_addr) {
+        info->initrd_start = MAX(info->initrd_start, image_high_addr);
+    }
     info->initrd_start = TARGET_PAGE_ALIGN(info->initrd_start);
 
     if (is_linux) {
-- 
2.20.1

Re: [Qemu-devel] [PATCH for-4.1? 2/2] hw/arm/boot: Further improve initrd positioning code

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> In commit e6b2b20d9735d4ef we made the boot loader code try to avoid
> putting the initrd on top of the kernel.  However the expression used
> to calculate the start of the initrd:
>
>     info->initrd_start = info->loader_start +
>         MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);
>
> incorrectly uses 'kernel_size' as the offset within RAM of the
> highest address to avoid.  This is incorrect because the kernel
> doesn't start at address 0, but slightly higher than that.  This
> means that we can still incorrectly end up overlaying the initrd on
> the kernel in some cases, for example:
>
> * The kernel's image_size is 0x0a7a8000
> * The kernel was loaded at   0x40080000
> * The end of the kernel is   0x4A828000
> * The DTB was loaded at      0x4a800000
>
> To get this right we need to track the actual highest address used
> by the kernel and use that rather than kernel_size. We already
> set image_low_addr and image_high_addr for ELF images; set them
> also for the various other image types we support, and then use
> image_high_addr as the lowest allowed address for the initrd.
> (We don't use image_low_addr, but we set it for consistency
> with the existing code path for ELF files.)
>
> Fixes: e6b2b20d9735d4ef
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  hw/arm/boot.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index b7b31753aca..c2b89b3bb9b 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -988,7 +988,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>      int is_linux = 0;
>      uint64_t elf_entry;
>      /* Addresses of first byte used and first byte not used by the image */
> -    uint64_t image_low_addr, image_high_addr;
> +    uint64_t image_low_addr = 0, image_high_addr = 0;
>      int elf_machine;
>      hwaddr entry;
>      static const ARMInsnFixup *primary_loader;
> @@ -1041,17 +1041,29 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>          uint64_t loadaddr = info->loader_start + KERNEL_NOLOAD_ADDR;
>          kernel_size = load_uimage_as(info->kernel_filename, &entry, &loadaddr,
>                                       &is_linux, NULL, NULL, as);
> +        if (kernel_size >= 0) {
> +            image_low_addr = loadaddr;
> +            image_high_addr = image_low_addr + kernel_size;
> +        }
>      }
>      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
>          kernel_size = load_aarch64_image(info->kernel_filename,
>                                           info->loader_start, &entry, as);
>          is_linux = 1;
> +        if (kernel_size >= 0) {
> +            image_low_addr = entry;
> +            image_high_addr = image_low_addr + kernel_size;
> +        }
>      } else if (kernel_size < 0) {
>          /* 32-bit ARM */
>          entry = info->loader_start + KERNEL_LOAD_ADDR;
>          kernel_size = load_image_targphys_as(info->kernel_filename, entry,
>                                               ram_end - KERNEL_LOAD_ADDR, as);
>          is_linux = 1;
> +        if (kernel_size >= 0) {
> +            image_low_addr = entry;
> +            image_high_addr = image_low_addr + kernel_size;
> +        }
>      }
>      if (kernel_size < 0) {
>          error_report("could not load kernel '%s'", info->kernel_filename);
> @@ -1083,7 +1095,10 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>       * we might still make a bad choice here.
>       */
>      info->initrd_start = info->loader_start +
> -        MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);
> +        MIN(info->ram_size / 2, 128 * 1024 * 1024);
> +    if (image_high_addr) {
> +        info->initrd_start = MAX(info->initrd_start, image_high_addr);
> +    }
>      info->initrd_start = TARGET_PAGE_ALIGN(info->initrd_start);
>
>      if (is_linux) {


--
Alex Bennée

Re: [Qemu-devel] [PATCH for-4.1? 0/2] arm: further improve initrd positioning

[Mark Rutland]

Hi Peter,

On Mon, Jul 22, 2019 at 04:18:02PM +0100, Peter Maydell wrote:
> In commit e6b2b20d9735d4ef we made the boot loader code try to avoid
> putting the initrd on top of the kernel.  However the expression used
> to calculate the start of the initrd:
> 
>     info->initrd_start = info->loader_start +
>         MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size);
> 
> incorrectly uses 'kernel_size' as the offset within RAM of the
> highest address to avoid.  This is incorrect because the kernel
> doesn't start at address 0, but slightly higher than that.  This
> means that we can still incorrectly end up overlaying the initrd on
> the kernel in some cases, for example:
> 
> * The kernel's image_size is 0x0a7a8000
> * The kernel was loaded at   0x40080000
> * The end of the kernel is   0x4A828000
> * The DTB was loaded at      0x4a800000
> 
> To get this right we need to track the actual highest address used
> by the kernel and use that rather than kernel_size. We already
> trace the low_addr and high_addr for ELF images; set them
> also for the various other image types we support, and then use
> high_addr as the lowest allowed address for the initrd.
> 
> Patch 1 just does a preliminary variable rename; patch 2 is the meat.
> 
> Only very lightly tested...

FWIW, I've given this a spin, and it manages to boot my growing
collection of problematic images, along with all the previously-working
images I had lying around.

For both patches, feel free to take that as:

Tested-by: Mark Rutland <mark.rutland@arm.com>

> Marked as 'maybe for 4.1' because it is a bug fix and to code which
> is new in 4.1. OTOH cases that fail now would have failed with 4.0
> so it is not a regression strictly speaking. And we're getting
> steadily closer to release and I haven't very heavily tested this
> patch. I incline towards including it, overall.

Having these in 4.1 would be nice. AIUI the logic is correct, and this
seems low risk to me, but it is your call.

If you decide to hold off for a release, I can apply these patches
locally without too much pain.

Thanks for dealing with this, anyhow!

Mark.

> 
> thanks
> -- PMM
> 
> Peter Maydell (2):
>   hw/arm/boot: Rename elf_{low,high}_addr to image_{low,high}_addr
>   hw/arm/boot: Further improve initrd positioning code
> 
>  hw/arm/boot.c | 37 +++++++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> -- 
> 2.20.1
>