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

[Grant Likely]

On Sun, Jan 29, 2012 at 07:13:55PM +0000, Peter Maydell wrote:
> On 29 January 2012 16:01, Grant Likely <address@hidden> wrote:
> > diff --git a/configure b/configure
> > index f69e08f..0c2deab 100755
> > --- a/configure
> > +++ b/configure
> > @@ -3411,6 +3411,9 @@ 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
> > +    if test "$fdt" = "yes" ; then
> > +      target_libs_softmmu="$fdt_libs"
> > +    fi
> 
> This doesn't need to be conditional -- compare the similar stanzas
> for other fdt-using architectures.

Okay.

> >   ;;
> >   cris)
> >     target_nptl="yes"
> > diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> > index 5f163fd..35bfa62 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,66 @@ 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;
> 
> scripts/checkpatch.pl complains about this and other style issues...
> 

Fixed.

> > +
> > +    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");
> 
> This seems kind of weird -- if you're not trying to use 'rc' as
> a running "something failed" flag to return to the caller then
> why not just have "if (function() < 0) { fprintf(...); }" ?

Mostly because it looked ugly when done that way.  There is already a
line break to deal with the long arguments, it looked worse to me when
also put inside an if() block.  If you prefer, then I will change it.

> 
> > +
> > +    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 +284,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);
> > @@ -239,10 +304,10 @@ void arm_load_kernel(CPUState *env, struct 
> > arm_boot_info *info)
> >  {
> >     int kernel_size;
> >     int initrd_size;
> > -    int n;
> > +    int n, rc;
> >     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 +366,22 @@ 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);
> > +        rc = load_dtb(dtb_start, info);
> > +        if (rc)
> > +            exit(1);
> 
> The rc variable is pretty obviously unnecessary here.

Fixed

> 
> > +
> >         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 +391,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;
> >     }
> >     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;
> 
> The suggestion on IRC for the long-term Right Thing for passing
> dtb filename/kernel/etc to arm_boot is that the arm_boot code ought
> to turn into a QOM object, and then the top level code can just
> search the QOM tree the machine model instantiates for a QOM
> object of the right type and pass filenames to it directly by
> setting properties on it. So this new global is ok as it's
> likely to go away again eventually.

Okay, good.  I had assumed that this would be a temporary solution until
the needed infrastructure is in place.

> 
> >  #endif
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 3a07ae8..0a01baa 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1964,6 +1964,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 a device tree image\n", QEMU_ARCH_ARM)
> 
> Needs more spaces to make it line up right in -help output:
> Linux/Multiboot boot specific:
> -kernel bzImage use 'bzImage' as kernel image
> -append cmdline use 'cmdline' as kernel command line
> -initrd file    use 'file' as initial ram disk
> -dtb file use 'file' as a device tree image

Fixed.

New patch below...

---
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;
     }
     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;
 
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index 3a07ae8..309a403 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1964,6 +1964,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
 @end table
 ETEXI
diff --git a/vl.c b/vl.c
index d88a18c..b22eae3 100644
--- a/vl.c
+++ b/vl.c
@@ -1154,6 +1154,7 @@ void pcmcia_info(Monitor *mon)
 
 static QEMUMachine *first_machine = NULL;
 QEMUMachine *current_machine = NULL;
+const char *current_dtb_filename = NULL;
 
 int qemu_register_machine(QEMUMachine *m)
 {
@@ -2304,6 +2305,9 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_initrd:
                 initrd_filename = optarg;
                 break;
+            case QEMU_OPTION_dtb:
+                current_dtb_filename = optarg;
+                break;
             case QEMU_OPTION_hda:
                 {
                     char buf[256];
@@ -3241,6 +3245,11 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    if (!linux_boot && current_dtb_filename != NULL) {
+        fprintf(stderr, "-dtb only allowed with -kernel option\n");
+        exit(1);
+    }
+
     os_set_line_buffering();
 
     if (init_timer_alarm() < 0) {