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

[Peter Maydell]

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 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).

> +
>          /* 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...

> --
> 1.8.3.2

thanks
-- PMM