| Message ID | 20120129160105.GA10629@ponder.secretlab.ca |
|---|---|
| State | New |
| Headers | show |
Am 29.01.2012 17:01, schrieb Grant Likely: > Okay, well at least let's start with this. Here is an updated version of the > patch that doesn't touch the board code. It makes the -dtb option available > to all ARM platforms. > > Even if dtb is not the primary structure, we absolutely need this option. Well, the option itself is what has been the most controversial here. Yours is at least the third attempt, so far we've seen discussed -dtb, -kern-dtb, -kernel-dtb, -kernel dtb=, etc. Also how to pass around the data obtained that way - whether as global variable or via struct passed to machine init or in a few weeks via QOM. Andreas
On 29 January 2012 16:01, Grant Likely <grant.likely@secretlab.ca> wrote: > On Sun, Jan 29, 2012 at 11:15:42AM +0000, Paul Brook wrote: >> Clearly we need to have some sort of FDT support. However I'm unconvinced >> that it's the correct format for the primary data structure. For one thing >> it's a hierarchical tree structure, and in my experience describing links >> outside that structure gets hairy. One of the things we're doing with QOM is >> move from a hierarchical tree/bus to a more general connected web of links >> between devices. > > Okay, well at least let's start with this. Here is an updated version of the > patch that doesn't touch the board code. It makes the -dtb option available > to all ARM platforms. > > Even if dtb is not the primary structure, we absolutely need this option. > Developers using qemu to simulate real hardware are going to still want > to pass in the exact same .dtb file to qemu as they use for the real target, > and those .dtb files are maintained completely separately from qemu. I agree with Paul that trying to use the dtb as the primary structure for describing a machine model to qemu is decidedly problematic. Let's start with the basic "we're a bootloader, provide minimal support for passing in a device tree blob"... Incidentally, it would probably be useful to sort out the problems with libfdt not being very widely provided (cf previous discussion, http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg02252.html). I see Aurelien has suggested a patch to the debian package to provide it (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=477565) which ought to help there. Some more detailed-level code review follows on the assumption that this patch is a reasonable direction to go in (which it seems like to me). > 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..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. > ;; > 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... > + > + 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(...); }" ? > + > + 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. > + > 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. > #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 > +STEXI > +@item -dtb @var{file} > +@findex -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) { -- PMM
On 29 January 2012 18:48, Andreas Färber <afaerber@suse.de> wrote: > Am 29.01.2012 17:01, schrieb Grant Likely: >> Okay, well at least let's start with this. Here is an updated version of the >> patch that doesn't touch the board code. It makes the -dtb option available >> to all ARM platforms. >> >> Even if dtb is not the primary structure, we absolutely need this option. > > Well, the option itself is what has been the most controversial here. > Yours is at least the third attempt, so far we've seen discussed -dtb, > -kern-dtb, -kernel-dtb, -kernel dtb=, etc. FWIW, my vote is for '-dtb' because it parallels the existing -initrd and -append. (Or -devicetree if we think -dtb is too cryptic; but anyway not something with 'kernel' in it.) -- PMM
On Mon, Jan 30, 2012 at 5:13 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 29 January 2012 16:01, Grant Likely <grant.likely@secretlab.ca> wrote: >> On Sun, Jan 29, 2012 at 11:15:42AM +0000, Paul Brook wrote: >>> Clearly we need to have some sort of FDT support. However I'm unconvinced >>> that it's the correct format for the primary data structure. For one thing >>> it's a hierarchical tree structure, and in my experience describing links >>> outside that structure gets hairy. One of the things we're doing with QOM is >>> move from a hierarchical tree/bus to a more general connected web of links >>> between devices. >> >> Okay, well at least let's start with this. Here is an updated version of the >> patch that doesn't touch the board code. It makes the -dtb option available >> to all ARM platforms. >> >> Even if dtb is not the primary structure, we absolutely need this option. >> Developers using qemu to simulate real hardware are going to still want >> to pass in the exact same .dtb file to qemu as they use for the real target, >> and those .dtb files are maintained completely separately from qemu. > > I agree with Paul that trying to use the dtb as the primary structure > for describing a machine model to qemu is decidedly problematic. Let's > start with the basic "we're a bootloader, provide minimal support for > passing in a device tree blob"... We've never advocated DTB as primary structure, but rather just one very useful, simple and generic approach. Among the the sea of fixed machine models that is QEMU today, having a DTB-driven machine in there would be an excellent proving ground for QOM conversion and steps towards fully data driven machines which should be the ultimate objective anyway. As Peter C says elsewhere in this thread, through its link syntax foo=<&ref> you can describe any sort of topology you like in a device tree. Regards, John
On Sun, Jan 29, 2012 at 09:29:58PM +0000, Peter Maydell wrote: > On 29 January 2012 18:48, Andreas Färber <afaerber@suse.de> wrote: > > Am 29.01.2012 17:01, schrieb Grant Likely: > >> Okay, well at least let's start with this. Here is an updated version of the > >> patch that doesn't touch the board code. It makes the -dtb option available > >> to all ARM platforms. > >> > >> Even if dtb is not the primary structure, we absolutely need this option. > > > > Well, the option itself is what has been the most controversial here. > > Yours is at least the third attempt, so far we've seen discussed -dtb, > > -kern-dtb, -kernel-dtb, -kernel dtb=, etc. > > FWIW, my vote is for '-dtb' because it parallels the existing -initrd > and -append. (Or -devicetree if we think -dtb is too cryptic; but anyway > not something with 'kernel' in it.) That was my reasoning too. I'm not stuck on any particular flag though as long as the feature gets merged. :-) g.
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..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 ;; 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; + + 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 +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); + 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; #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) +STEXI +@item -dtb @var{file} +@findex -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) {