Re: [Qemu-devel] [PATCH v2] arm: add device tree support

[Alexander Graf]

On 01.02.2012, at 01:11, Grant Likely wrote:

> If compiled with CONFIG_FDT, allow user to specify a device tree file using
> the -dtb argument.  If the machine supports it then the dtb will be loaded
> into memory and passed to the kernel on boot.
> 
> v2: - Enable for all arm platforms by making arm_boot use the filename 
> directly
>    - Fix style issues
> 
> Signed-off-by: Jeremy Kerr <address@hidden>
> Signed-off-by: Grant Likely <address@hidden>
> Cc: Paul Brook <address@hidden>
> Cc: Peter Maydell <address@hidden>
> Cc: Rob Herring <address@hidden>
> Cc: Edgar E. Iglesias <address@hidden>
> ---
> Makefile.target |    1 +
> configure       |    1 +
> hw/arm_boot.c   |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++----
> hw/boards.h     |    1 +
> qemu-options.hx |    9 +++++
> vl.c            |    9 +++++
> 6 files changed, 113 insertions(+), 8 deletions(-)
> 
> diff --git a/Makefile.target b/Makefile.target
> index 68481a3..5e465ec 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -363,6 +363,7 @@ obj-arm-y += vexpress.o
> obj-arm-y += strongarm.o
> obj-arm-y += collie.o
> obj-arm-y += pl041.o lm4549.o
> +obj-arm-$(CONFIG_FDT) += device_tree.o
> 
> obj-sh4-y = shix.o r2d.o sh7750.o sh7750_regnames.o tc58128.o
> obj-sh4-y += sh_timer.o sh_serial.o sh_intc.o sh_pci.o sm501.o
> diff --git a/configure b/configure
> index f69e08f..2856897 100755
> --- a/configure
> +++ b/configure
> @@ -3411,6 +3411,7 @@ case "$target_arch2" in
>     gdb_xml_files="arm-core.xml arm-vfp.xml arm-vfp3.xml arm-neon.xml"
>     target_phys_bits=32
>     target_llong_alignment=4
> +    target_libs_softmmu="$fdt_libs"
>   ;;
>   cris)
>     target_nptl="yes"
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index 5f163fd..377d202 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -7,11 +7,14 @@
>  * This code is licensed under the GPL.
>  */
> 
> +#include "config.h"
> #include "hw.h"
> #include "arm-misc.h"
> #include "sysemu.h"
> +#include "boards.h"
> #include "loader.h"
> #include "elf.h"
> +#include "device_tree.h"
> 
> #define KERNEL_ARGS_ADDR 0x100
> #define KERNEL_LOAD_ADDR 0x00010000
> @@ -207,6 +210,71 @@ static void set_kernel_args_old(const struct 
> arm_boot_info *info,
>     }
> }
> 
> +static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info 
> *binfo)
> +{
> +#ifdef CONFIG_FDT
> +    uint32_t mem_reg_property[] = { cpu_to_be32(binfo->loader_start),
> +                                    cpu_to_be32(binfo->ram_size) };
> +    void *fdt = NULL;
> +    char *filename;
> +    int size, rc;
> +
> +    if (!current_dtb_filename) {
> +        return 0;
> +    }
> +
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, current_dtb_filename);
> +    if (!filename) {
> +        fprintf(stderr, "Couldn't open dtb file %s\n", current_dtb_filename);
> +        return -1;
> +    }
> +
> +    fdt = load_device_tree(filename, &size);
> +    if (!fdt) {
> +        fprintf(stderr, "Couldn't open dtb file %s\n", filename);
> +        g_free(filename);
> +        return -1;
> +    }
> +    g_free(filename);
> +
> +    rc = qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_property,
> +                               sizeof(mem_reg_property));
> +    if (rc < 0) {
> +        fprintf(stderr, "couldn't set /memory/reg\n");
> +    }
> +
> +    rc = qemu_devtree_setprop_string(fdt, "/chosen", "bootargs",
> +                                      binfo->kernel_cmdline);
> +    if (rc < 0) {
> +        fprintf(stderr, "couldn't set /chosen/bootargs\n");
> +    }
> +
> +    if (binfo->initrd_size) {
> +        rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-start",
> +                binfo->loader_start + INITRD_LOAD_ADDR);
> +        if (rc < 0) {
> +            fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
> +        }
> +
> +        rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-end",
> +                    binfo->loader_start + INITRD_LOAD_ADDR +
> +                    binfo->initrd_size);
> +        if (rc < 0) {
> +            fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
> +        }
> +    }
> +
> +    cpu_physical_memory_write(addr, fdt, size);
> +
> +    return 0;
> +
> +#else
> +    fprintf(stderr, "Platform requested a device tree, "
> +                "but qemu was compiled without fdt support\n");
> +    return -1;
> +#endif
> +}
> +
> static void do_cpu_reset(void *opaque)
> {
>     CPUState *env = opaque;
> @@ -221,12 +289,14 @@ static void do_cpu_reset(void *opaque)
>         } else {
>             if (env == first_cpu) {
>                 env->regs[15] = info->loader_start;
> -                if (old_param) {
> -                    set_kernel_args_old(info, info->initrd_size,
> +                if (!current_dtb_filename) {
> +                    if (old_param) {
> +                        set_kernel_args_old(info, info->initrd_size,
> +                                            info->loader_start);
> +                    } else {
> +                        set_kernel_args(info, info->initrd_size,
>                                         info->loader_start);
> -                } else {
> -                    set_kernel_args(info, info->initrd_size,
> -                                    info->loader_start);
> +                    }
>                 }
>             } else {
>                 info->secondary_cpu_reset_hook(env, info);
> @@ -242,7 +312,7 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info 
> *info)
>     int n;
>     int is_linux = 0;
>     uint64_t elf_entry;
> -    target_phys_addr_t entry;
> +    target_phys_addr_t entry, dtb_start;
>     int big_endian;
> 
>     /* Load the kernel.  */
> @@ -301,8 +371,23 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info 
> *info)
>         } else {
>             initrd_size = 0;
>         }
> +        info->initrd_size = initrd_size;
> +
> +        /* Place the DTB after the initrd in memory */
> +        dtb_start = TARGET_PAGE_ALIGN(info->loader_start + INITRD_LOAD_ADDR +
> +                                      initrd_size);
> +        if (load_dtb(dtb_start, info)) {
> +            exit(1);
> +        }
> +
>         bootloader[4] = info->board_id;
> -        bootloader[5] = info->loader_start + KERNEL_ARGS_ADDR;
> +        /* for device tree boot, we pass the DTB directly in r2. Otherwise
> +         * we point to the kernel args */
> +        if (current_dtb_filename) {
> +            bootloader[5] = dtb_start;
> +        } else {
> +            bootloader[5] = info->loader_start + KERNEL_ARGS_ADDR;
> +        }
>         bootloader[6] = entry;
>         for (n = 0; n < sizeof(bootloader) / 4; n++) {
>             bootloader[n] = tswap32(bootloader[n]);
> @@ -312,7 +397,6 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info 
> *info)
>         if (info->nb_cpus > 1) {
>             info->write_secondary_boot(env, info);
>         }
> -        info->initrd_size = initrd_size;

This seems to be an unrelated change. Any reason for moving that line up?

>     }
>     info->is_linux = is_linux;
> 
> diff --git a/hw/boards.h b/hw/boards.h
> index f6d3784..d06776c 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -34,5 +34,6 @@ typedef struct QEMUMachine {
> int qemu_register_machine(QEMUMachine *m);
> 
> extern QEMUMachine *current_machine;
> +extern const char *current_dtb_filename;
> 

I'm not sure I like that naming. It really should only contain the cmdline 
option. If I have a default dtb file for my machine, I'd like to keep this 
variable empty.

Also, are you sure that boards.h is the right place for the extern declaration? 
In fact, is the global a good idea in the first place?

We could also just change machine->init() and pass the dtb in there. In a QOM 
world these would become machine device properties anyways.

    machine->init(ram_size, boot_devices,
                  kernel_filename, kernel_cmdline, initrd_filename, cpu_model);

Essentially we shouldn't treat -dtb any different than -kernel or -initrd. It's 
also useful for more than ARM, namely embedded ppc systems. But I can easily 
post a follow-up patch for those.


Alex