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

[Peter Crosthwaite]

On Tue, Feb 21, 2012 at 5:47 AM, Peter Maydell <address@hidden> wrote:
> Ping re patch 3 and 4 here -- I know there was some discussion under the
> thread on Peter Crosthwaite's 'qomify arm_boot' patch series, but it's a
> bit hard to disentangle from the comments on that patch series.

Conclusion reached over there was the yet to be implemented --property
command line argument should work for this. But blocking dtb support
while waiting for this seems excessive to me. Can we push this
regardless of the issues identifed in that thread considering the the
groundwork for the ideal solution is yet to be implemented?

> Specifically, does anybody (a) dislike the user-facing command line
> interface here or (b) otherwise think it's misguided? I'm not a huge
> fan of the -machine options (they seem to be a bit of a grab-bag of
> stuff) but I can live with them as a mechanism for passing the data
> to the right place...
>
> thanks
> -- PMM
>
> On 8 February 2012 05:41, Peter Maydell <address@hidden> wrote:
>> From: Grant Likely <address@hidden>
>>
>> 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.
>>
>> Signed-off-by: Jeremy Kerr <address@hidden>
>> Signed-off-by: Grant Likely <address@hidden>
>> [Peter Maydell: Use machine opt rather than global to pass dtb filename]
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  Makefile.target |    1 +
>>  configure       |    1 +
>>  hw/arm-misc.h   |    1 +
>>  hw/arm_boot.c   |   96 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++---
>>  qemu-config.c   |    4 ++
>>  qemu-options.hx |    9 +++++
>>  vl.c            |    9 +++++
>>  7 files changed, 115 insertions(+), 6 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 763db24..1d8b3ac 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3458,6 +3458,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-misc.h b/hw/arm-misc.h
>> index 5e5204b..4b55fb8 100644
>> --- a/hw/arm-misc.h
>> +++ b/hw/arm-misc.h
>> @@ -29,6 +29,7 @@ struct arm_boot_info {
>>     const char *kernel_filename;
>>     const char *kernel_cmdline;
>>     const char *initrd_filename;
>> +    const char *dtb_filename;
>>     target_phys_addr_t loader_start;
>>     /* multicore boards that use the default secondary core boot functions
>>      * need to put the address of the secondary boot code, the boot reg,
>> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
>> index 5f163fd..769fe2e 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,67 @@ 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;
>> +
>> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
>> +    if (!filename) {
>> +        fprintf(stderr, "Couldn't open dtb file %s\n", binfo->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, "Device tree requested, "
>> +                "but qemu was compiled without fdt support\n");
>> +    return -1;
>> +#endif
>> +}
>> +
>>  static void do_cpu_reset(void *opaque)
>>  {
>>     CPUState *env = opaque;
>> @@ -221,12 +285,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 (!info->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);
>> @@ -251,6 +317,9 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info 
>> *info)
>>         exit(1);
>>     }
>>
>> +    info->dtb_filename = 
>> qemu_opt_get(qemu_opts_find(qemu_find_opts("machine"),
>> +                                                     0), "dtb");
>> +
>>     if (!info->secondary_cpu_reset_hook) {
>>         info->secondary_cpu_reset_hook = default_reset_secondary;
>>     }
>> @@ -302,7 +371,22 @@ void arm_load_kernel(CPUState *env, struct 
>> arm_boot_info *info)
>>             initrd_size = 0;
>>         }
>>         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 (info->dtb_filename) {
>> +            /* Place the DTB after the initrd in memory */
>> +            target_phys_addr_t dtb_start = 
>> TARGET_PAGE_ALIGN(info->loader_start
>> +                                                             + 
>> INITRD_LOAD_ADDR
>> +                                                             + initrd_size);
>> +            if (load_dtb(dtb_start, info)) {
>> +                exit(1);
>> +            }
>> +            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]);
>> diff --git a/qemu-config.c b/qemu-config.c
>> index 07480a4..151a61a 100644
>> --- a/qemu-config.c
>> +++ b/qemu-config.c
>> @@ -548,6 +548,10 @@ static QemuOptsList qemu_machine_opts = {
>>             .name = "append",
>>             .type = QEMU_OPT_STRING,
>>             .help = "Linux kernel command line",
>> +        }, {
>> +            .name = "dtb",
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "Linux kernel device tree file",
>>         },
>>         { /* End of list */ }
>>     },
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 19906e5..80316ed 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -2011,6 +2011,15 @@ Use @var{file1} and @var{file2} as modules and pass 
>> arg=foo as parameter to the
>>  first module.
>>  ETEXI
>>
>> +DEF("dtb", HAS_ARG, QEMU_OPTION_dtb, \
>> +    "-dtb    file    use 'file' as device tree image\n", QEMU_ARCH_ARM)
>> +STEXI
>> address@hidden -dtb @var{file}
>> address@hidden -dtb
>> +Use @var{file} as a device tree binary (dtb) image and pass it to the kernel
>> +on boot.
>> +ETEXI
>> +
>>  STEXI
>> address@hidden table
>>  ETEXI
>> diff --git a/vl.c b/vl.c
>> index b8bb955..03c17d6 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2453,6 +2453,9 @@ int main(int argc, char **argv, char **envp)
>>             case QEMU_OPTION_append:
>>                 qemu_opts_set(qemu_find_opts("machine"), 0, "append", 
>> optarg);
>>                 break;
>> +            case QEMU_OPTION_dtb:
>> +                qemu_opts_set(qemu_find_opts("machine"), 0, "dtb", optarg);
>> +                break;
>>             case QEMU_OPTION_cdrom:
>>                 drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
>>                 break;
>> @@ -3261,6 +3264,12 @@ int main(int argc, char **argv, char **envp)
>>         exit(1);
>>     }
>>
>> +    if (!linux_boot
>> +        && qemu_opt_get(qemu_opts_find(qemu_find_opts("machine"), 0), 
>> "dtb")) {
>> +        fprintf(stderr, "-dtb only allowed with -kernel option\n");
>> +        exit(1);
>> +    }
>> +
>>     os_set_line_buffering();
>>
>>     if (init_timer_alarm() < 0) {
>> --
>> 1.7.5.4
>>
>>
>