[Qemu-devel] [PATCH v2 0/4] ARM: load_dtb() changes for -bios and ELF images

[Qemu-devel] [PATCH v2 0/4] ARM: load_dtb() changes for -bios and ELF images

[Ard Biesheuvel]

I split off these patches from the series I sent last week. Peter's NOR flash
patch has been merged in the mean time, and the CPU reset patch needs to be
discussed more widely before we can move forward with it.

So what remains are changes in the DTB handling in hw/arm/boot.c, to load a
DTB even when using -bios or using -kernel but with an ELF image.

Changes since v1:
- updated load_dtb() to take an address limit and return the dtb size, or 0
  if it didn't fit
- don't fail in the ELF case only because the DTB clashes with the ELF segments,
  in that case, just proceed without it
- reshuffled order, added R-b to #1

Ard Biesheuvel (4):
  hw/arm/boot: load DTB as a ROM image
  hw/arm/boot: pass an address limit to and return size from load_dtb()
  hw/arm/boot: load device tree to base of DRAM if no -kernel option was
    passed
  hw/arm/boot: enable DTB support when booting ELF images

 hw/arm/boot.c | 47 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 6 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH v2 1/4] hw/arm/boot: load DTB as a ROM image

[Ard Biesheuvel]

In order to make the device tree blob (DTB) available in memory not only at
first boot, but also after system reset, use rom_blob_add_fixed() to install
it into memory.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/boot.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index e32f2f415885..50eca931e1a4 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -396,7 +396,10 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
 
     qemu_fdt_dumpdtb(fdt, size);
 
-    cpu_physical_memory_write(addr, fdt, size);
+    /* Put the DTB into the memory map as a ROM image: this will ensure
+     * the DTB is copied again upon reset, even if addr points into RAM.
+     */
+    rom_add_blob_fixed("dtb", fdt, size, addr);
 
     g_free(fdt);
 
-- 
1.8.3.2

[Qemu-devel] [PATCH v2 2/4] hw/arm/boot: pass an address limit to and return size from load_dtb()

[Ard Biesheuvel]

Add an address limit input parameter to load_dtb() so that we can
tell it how much memory the dtb is allowed to consume. If the dtb
doesn't fit, return 0, otherwise return the actual size of the
loaded dtb, or -1 on error.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/boot.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 50eca931e1a4..014fab347b09 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -312,7 +312,8 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
     }
 }
 
-static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
+static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
+                    hwaddr addr_limit)
 {
     void *fdt = NULL;
     int size, rc;
@@ -341,6 +342,15 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
         }
     }
 
+    if (addr_limit > addr && size > (addr_limit - addr)) {
+        /* We have been given a non-zero address limit and we have exceeded
+         * it. Whether this is constitues a failure is up to the caller to
+         * decide, so just return 0 as size, i.e., no error.
+         */
+        g_free(fdt);
+        return 0;
+    }
+
     acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells");
     scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells");
     if (acells == 0 || scells == 0) {
@@ -403,7 +413,7 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
 
     g_free(fdt);
 
-    return 0;
+    return size;
 
 fail:
     g_free(fdt);
@@ -572,7 +582,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
              */
             hwaddr dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
                                              4096);
-            if (load_dtb(dtb_start, info)) {
+            if (load_dtb(dtb_start, info, 0) < 0) {
                 exit(1);
             }
             fixupcontext[FIXUP_ARGPTR] = dtb_start;
-- 
1.8.3.2

[Qemu-devel] [PATCH v2 3/4] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed

[Ard Biesheuvel]

If we are running the 'virt' machine, we may have a device tree blob but no
kernel to supply it to if no -kernel option was passed. In that case, copy it
to the base of RAM where it can be picked up by a bootloader.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/boot.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 014fab347b09..1f73614d8843 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -472,6 +472,16 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 
     /* Load the kernel.  */
     if (!info->kernel_filename) {
+
+        if (have_dtb(info)) {
+            /* If we have a device tree blob, but no kernel to supply it to,
+             * copy it to the base of RAM for a bootloader to pick up.
+             */
+            if (load_dtb(info->loader_start, info, 0) < 0) {
+                exit(1);
+            }
+        }
+
         /* If no kernel specified, do nothing; we will start from address 0
          * (typically a boot ROM image) in the same way as hardware.
          */
-- 
1.8.3.2

[Qemu-devel] [PATCH v2 4/4] hw/arm/boot: enable DTB support when booting ELF images

[Ard Biesheuvel]

Add support for loading DTB images when booting ELF images using
-kernel. If there are no conflicts with the placement of the ELF
segments, the DTB image is loaded at the base of RAM.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/boot.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 1f73614d8843..3878cbd97aad 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -464,7 +464,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     int kernel_size;
     int initrd_size;
     int is_linux = 0;
-    uint64_t elf_entry;
+    uint64_t elf_entry, elf_low_addr;
     int elf_machine;
     hwaddr entry, kernel_load_offset;
     int big_endian;
@@ -531,7 +531,19 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 
     /* Assume that raw images are linux kernels, and ELF images are not.  */
     kernel_size = load_elf(info->kernel_filename, NULL, NULL, &elf_entry,
-                           NULL, NULL, big_endian, elf_machine, 1);
+                           &elf_low_addr, NULL, big_endian, elf_machine, 1);
+    if (kernel_size > 0 && have_dtb(info)) {
+        /* If there is still some room left between the base of RAM and the
+         * low end of the ELF image we just loaded, try and put the DTB at the
+         * base of RAM like we do for bootloaders. Just ignore the potential 0
+         * return value of load_dtb() which indicates that the dtb didn't fit,
+         * in that case we just proceed without it.
+         */
+        if (elf_low_addr > info->loader_start &&
+            load_dtb(info->loader_start, info, elf_low_addr) < 0) {
+            exit(1);
+        }
+    }
     entry = elf_entry;
     if (kernel_size < 0) {
         kernel_size = load_uimage(info->kernel_filename, &entry, NULL,
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v2 4/4] hw/arm/boot: enable DTB support when booting ELF images

[Peter Maydell]

On 10 September 2014 11:59, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Add support for loading DTB images when booting ELF images using
> -kernel. If there are no conflicts with the placement of the ELF
> segments, the DTB image is loaded at the base of RAM.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  hw/arm/boot.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 1f73614d8843..3878cbd97aad 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -464,7 +464,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>      int kernel_size;
>      int initrd_size;
>      int is_linux = 0;
> -    uint64_t elf_entry;
> +    uint64_t elf_entry, elf_low_addr;
>      int elf_machine;
>      hwaddr entry, kernel_load_offset;
>      int big_endian;
> @@ -531,7 +531,19 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>
>      /* Assume that raw images are linux kernels, and ELF images are not.  */
>      kernel_size = load_elf(info->kernel_filename, NULL, NULL, &elf_entry,
> -                           NULL, NULL, big_endian, elf_machine, 1);
> +                           &elf_low_addr, NULL, big_endian, elf_machine, 1);
> +    if (kernel_size > 0 && have_dtb(info)) {
> +        /* If there is still some room left between the base of RAM and the
> +         * low end of the ELF image we just loaded, try and put the DTB at the
> +         * base of RAM like we do for bootloaders. Just ignore the potential 0
> +         * return value of load_dtb() which indicates that the dtb didn't fit,
> +         * in that case we just proceed without it.
> +         */
> +        if (elf_low_addr > info->loader_start &&
> +            load_dtb(info->loader_start, info, elf_low_addr) < 0) {
> +            exit(1);
> +        }
> +    }

The conditional means we won't try to load the DTB even if the
ELF file fit into the address space entirely below loader_start,
which doesn't look right.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/4] hw/arm/boot: enable DTB support when booting ELF images

[Ard Biesheuvel]

On 10 September 2014 13:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 10 September 2014 11:59, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> Add support for loading DTB images when booting ELF images using
>> -kernel. If there are no conflicts with the placement of the ELF
>> segments, the DTB image is loaded at the base of RAM.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  hw/arm/boot.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 1f73614d8843..3878cbd97aad 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -464,7 +464,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>      int kernel_size;
>>      int initrd_size;
>>      int is_linux = 0;
>> -    uint64_t elf_entry;
>> +    uint64_t elf_entry, elf_low_addr;
>>      int elf_machine;
>>      hwaddr entry, kernel_load_offset;
>>      int big_endian;
>> @@ -531,7 +531,19 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>
>>      /* Assume that raw images are linux kernels, and ELF images are not.  */
>>      kernel_size = load_elf(info->kernel_filename, NULL, NULL, &elf_entry,
>> -                           NULL, NULL, big_endian, elf_machine, 1);
>> +                           &elf_low_addr, NULL, big_endian, elf_machine, 1);
>> +    if (kernel_size > 0 && have_dtb(info)) {
>> +        /* If there is still some room left between the base of RAM and the
>> +         * low end of the ELF image we just loaded, try and put the DTB at the
>> +         * base of RAM like we do for bootloaders. Just ignore the potential 0
>> +         * return value of load_dtb() which indicates that the dtb didn't fit,
>> +         * in that case we just proceed without it.
>> +         */
>> +        if (elf_low_addr > info->loader_start &&
>> +            load_dtb(info->loader_start, info, elf_low_addr) < 0) {
>> +            exit(1);
>> +        }
>> +    }
>
> The conditional means we won't try to load the DTB even if the
> ELF file fit into the address space entirely below loader_start,
> which doesn't look right.
>

Ah right, I though loader_start was the start of usable RAM

What about assigning elf_high_addr as well, and changing the test to

if ((elf_low_addr > info->loader_start
     || elf_high_addr < info_loader_start)
    && load_dtb(info->loader_start, info,
                (elf_high_addr < info_loader_start) ? 0 : elf_low_addr) < 0) {

Re: [Qemu-devel] [PATCH v2 4/4] hw/arm/boot: enable DTB support when booting ELF images

[Peter Maydell]

On 10 September 2014 12:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 10 September 2014 13:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The conditional means we won't try to load the DTB even if the
>> ELF file fit into the address space entirely below loader_start,
>> which doesn't look right.
>>
>
> Ah right, I though loader_start was the start of usable RAM

It is, but the ELF file doesn't necessarily have to be in RAM:
it could be linked so as to load at an address in the flash...

> What about assigning elf_high_addr as well, and changing the test to
>
> if ((elf_low_addr > info->loader_start
>      || elf_high_addr < info_loader_start)
>     && load_dtb(info->loader_start, info,
>                 (elf_high_addr < info_loader_start) ? 0 : elf_low_addr) < 0) {

I think that's right, though maybe there's a less confusing
way of phrasing it.

-- PMM

Re: [Qemu-devel] [PATCH v2 2/4] hw/arm/boot: pass an address limit to and return size from load_dtb()

[Peter Maydell]

On 10 September 2014 11:59, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Add an address limit input parameter to load_dtb() so that we can
> tell it how much memory the dtb is allowed to consume. If the dtb
> doesn't fit, return 0, otherwise return the actual size of the
> loaded dtb, or -1 on error.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  hw/arm/boot.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 50eca931e1a4..014fab347b09 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -312,7 +312,8 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
>      }
>  }
>
> -static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
> +static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> +                    hwaddr addr_limit)
>  {

This patch looks good, but since you're going to respin the series
anyway we could use a doc comment before the function describing the
semantics of the return value. If you do that you can
add my Reviewed-by: tag.

>      void *fdt = NULL;
>      int size, rc;
> @@ -341,6 +342,15 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>          }
>      }
>
> +    if (addr_limit > addr && size > (addr_limit - addr)) {
> +        /* We have been given a non-zero address limit and we have exceeded
> +         * it. Whether this is constitues a failure is up to the caller to

"constitutes"

> +         * decide, so just return 0 as size, i.e., no error.
> +         */
> +        g_free(fdt);
> +        return 0;
> +    }
> +
>      acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells");
>      scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells");
>      if (acells == 0 || scells == 0) {
> @@ -403,7 +413,7 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>
>      g_free(fdt);
>
> -    return 0;
> +    return size;
>
>  fail:
>      g_free(fdt);
> @@ -572,7 +582,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>               */
>              hwaddr dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
>                                               4096);
> -            if (load_dtb(dtb_start, info)) {
> +            if (load_dtb(dtb_start, info, 0) < 0) {
>                  exit(1);
>              }
>              fixupcontext[FIXUP_ARGPTR] = dtb_start;

-- PMM