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

["Alex Bennée" <alex.bennee@linaro.org>]

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