[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] hw/arm/boot: load device tree to base of DRAM i
From: |
Ard Biesheuvel |
Subject: |
Re: [Qemu-devel] [PATCH] hw/arm/boot: load device tree to base of DRAM if no -kernel option was passed |
Date: |
Mon, 1 Sep 2014 19:46:12 +0200 |
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