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

[Ard Biesheuvel]

On 1 September 2014 19:36, Peter Maydell <address@hidden> wrote:
> On 26 August 2014 16:31, Ard Biesheuvel <address@hidden> wrote:
>> 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 DRAM where it can be picked up by a bootloader executing from
>> NOR flash.
>>
>> Signed-off-by: Ard Biesheuvel <address@hidden>
>
> In general I like this approach for providing a BIOS/bootloader
> with information about the platform it's running on.
> (For the benefit of readers who may be missing context, this
> bootloader is likely to be UEFI.) Since we already have DTB for
> telling the guest about the shape of the platform this makes
> more sense to me than having a separate fw_cfg style
> interface to repeat the same information.
>

I agree. But perhaps we should address the reset issue we discussed
over IRC last Friday?
Currently, reset does not work at all when using -bios (and your
patch), but we should fix that in a sane way, i.e., the DTB should be
reloaded into DRAM, and this patch currently does not cover that.

> I should dig out the NOR-flash-in-virt patches and get them
> through review/commit so this code has a more immediately
> obvious purpose.
>
> A couple of style nits below.
>
>> ---
>>  hw/arm/boot.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 3d1f4a255b48..ff6a727613aa 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -455,6 +455,14 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info 
>> *info)
>>
>>      /* Load the kernel.  */
>>      if (!info->kernel_filename) {
>> +        /*
>> +         * If we have a device tree blob, but no kernel to supply it to, 
>> copy it
>> +         * to the base of DRAM for a bootloader executing from NOR flash to
>> +         * pick up.
>> +         */
>> +        if (have_dtb(info))
>> +            load_dtb(info->loader_start, info);
>
> Our coding style demands braces even on single-statement
> if()s. Also, we should check the return value from load_dtb()
> and exit(1) on failure (compare the existing callsite).
>

OK

>> +
>>          /* If no kernel specified, do nothing; we will start from address 0
>>           * (typically a boot ROM image) in the same way as hardware.
>>           */
>
> A style nit so minuscule I wouldn't point it out if you weren't
> going to reroll this patch anyway: notice how this comment
> differs from yours slightly in style...
>

OK

>> --
>> 1.8.3.2
>
> thanks
> -- PMM