On 2 July 2016 at 02:07, Alistair Francis <alistair.fran...@xilinx.com> wrote: > Add a generic loader to QEMU which can be used to load images or set > memory values. > > Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com> > --- > V8: > - Code corrections > - Rebase > V7: > - Rebase > V6: > - Add error checking > V5: > - Rebase > V4: > - Allow the loader to work with every architecture > - Move the file to hw/core > - Increase the maximum number of CPUs > - Make the CPU operations conditional > - Convert the cpu option to cpu-num > - Require the user to specify endianess > V3: > - Pass the ram_size to load_image_targphys() > V2: > - Add maintainers entry > - Perform bounds checking > - Register and unregister the reset in the realise/unrealise > Changes since RFC: > - Add BE support > > MAINTAINERS | 6 ++ > hw/core/Makefile.objs | 2 + > hw/core/generic-loader.c | 177 > +++++++++++++++++++++++++++++++++++++++ > include/hw/core/generic-loader.h | 45 ++++++++++ > 4 files changed, 230 insertions(+) > create mode 100644 hw/core/generic-loader.c > create mode 100644 include/hw/core/generic-loader.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 2ab6e3b..0077e22 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -992,6 +992,12 @@ M: Dmitry Fleytman <dmi...@daynix.com> > S: Maintained > F: hw/net/e1000e* > > +Generic Loader > +M: Alistair Francis <alistair.fran...@xilinx.com> > +S: Maintained > +F: hw/core/generic-loader.c > +F: include/hw/core/generic-loader.h > + > Subsystems > ---------- > Audio > diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs > index 82a9ef8..ab238fa 100644 > --- a/hw/core/Makefile.objs > +++ b/hw/core/Makefile.objs > @@ -16,3 +16,5 @@ common-obj-$(CONFIG_SOFTMMU) += null-machine.o > common-obj-$(CONFIG_SOFTMMU) += loader.o > common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o > common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o > + > +obj-$(CONFIG_SOFTMMU) += generic-loader.o > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c > new file mode 100644 > index 0000000..c9b0572 > --- /dev/null > +++ b/hw/core/generic-loader.c > @@ -0,0 +1,177 @@ > +/* > + * Generic Loader > + * > + * Copyright (C) 2014 Li Guang > + * Copyright (C) 2016 Xilinx Inc. > + * Written by Li Guang <lig.f...@cn.fujitsu.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but > WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > + * for more details. > + */ > + > +#include "qemu/osdep.h" > +#include "qom/cpu.h" > +#include "hw/sysbus.h" > +#include "sysemu/dma.h" > +#include "hw/loader.h" > +#include "qapi/error.h" > +#include "hw/core/generic-loader.h" > + > +#define CPU_NONE 0xFFFF
This should be updated now we've widened the type from 16 bits. > + > +static void generic_loader_reset(void *opaque) > +{ > + GenericLoaderState *s = GENERIC_LOADER(opaque); > + > + if (s->cpu) { > + CPUClass *cc = CPU_GET_CLASS(s->cpu); > + cpu_reset(s->cpu); > + if (cc) { > + cc->set_pc(s->cpu, s->addr); If something else sets the PC later on this gets lost, which is ugly and fragile. It's kinda-sorta OK that we bodge this for things like the hw/arm/boot.c code because all the pieces are inside QEMU, but when you start adding in command line devices I'm a bit nervous. Maybe we should actually figure out our reset order woes properly ? > + } > + } > + > + if (s->data_len) { > + assert(s->data_len < sizeof(s->data)); > + dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr, > &s->data, > + s->data_len); We rule out specifying cpu_num below, so when is s->cpu non-NULL? > + } > +} > + > +static void generic_loader_realize(DeviceState *dev, Error **errp) > +{ > + GenericLoaderState *s = GENERIC_LOADER(dev); > + hwaddr entry; > + int big_endian; > + int size = 0; > + > + /* Perform some error checking on the user's options */ > + if (s->data || s->data_len || s->data_be) { > + /* User is loading memory values */ > + if (s->file) { > + error_setg(errp, "Specifying a file is not supported when > loading " > + "memory values"); > + return; > + } else if (s->force_raw) { > + error_setg(errp, "Specifying force raw is not supported when " "force-raw", ie state the option name. Similarly in these other errors. > + "loading memory values"); > + return; > + } else if (!s->data || !s->data_len) { > + error_setg(errp, "Both data and data length must be specified"); > + return; > + } else if (s->cpu_num) { > + error_setg(errp, "Setting data and a cpu number is not > supported"); > + return; > + } > + } else if (s->file || s->force_raw) { > + /* User is loading an image */ > + if (s->data || s->data_len || s->data_be) { > + error_setg(errp, "Data can not be specified when loading an " > + "image"); > + return; > + } > + } else if (s->data_len) { > + if (s->data_len > 8) { > + error_setg(errp, "data-len cannot be greate then 8 bytes"); "greater" > + return; > + } else if (s->data_len > sizeof(s->data)) { > + error_setg(errp, "data-len cannot be more then the data size"); > + return; > + } > + } > + > + qemu_register_reset(generic_loader_reset, dev); What's wrong with a device reset function set via dc->reset ? > + > + if (s->cpu_num != CPU_NONE) { > + s->cpu = qemu_get_cpu(s->cpu_num); > + if (!s->cpu) { > + error_setg(errp, "Specified boot CPU#%d is nonexistent", > + s->cpu_num); > + return; > + } > + } > + > +#ifdef TARGET_WORDS_BIGENDIAN > + big_endian = 1; > +#else > + big_endian = 0; > +#endif > + > + if (s->file) { > + if (!s->force_raw) { > + size = load_elf_as(s->file, NULL, NULL, &entry, NULL, NULL, > + big_endian, 0, 0, 0, > + (s->cpu ? s->cpu : first_cpu)->as); > + > + if (size < 0) { > + size = load_uimage(s->file, &entry, NULL, NULL, NULL, NULL); > + } > + } > + > + if (size < 0 || s->force_raw) { > + /* Default to the maximum size being the machine's ram size */ > + size = load_image_targphys(s->file, s->addr, ram_size); > + } else { > + s->addr = entry; > + } We only use the CPU's address space for ELF loads, not for uimage or raw. That's an annoying inconsistency. I think we need to use the CPU's address space for everything or nothing, preferably for everything. Changing that after this goes into the tree would be a command line compatibility break, so this inclines me to saying that this isn't baked enough for 2.7 and we should aim to put it into 2.8. thanks -- PMM