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 <address@hidden> wrote:
> On 10 September 2014 11:59, Ard Biesheuvel <address@hidden> 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 <address@hidden>
>> ---
>>  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) {