| Message ID | d7eb2116e80ff604f87a9c0120ab4621fcd8d564.1467417982.git.alistair.francis@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@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.francis@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 <dmitry@daynix.com> > S: Maintained > F: hw/net/e1000e* > > +Generic Loader > +M: Alistair Francis <alistair.francis@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.fnst@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
On Tue, Jul 12, 2016 at 9:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@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.francis@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 <dmitry@daynix.com> >> S: Maintained >> F: hw/net/e1000e* >> >> +Generic Loader >> +M: Alistair Francis <alistair.francis@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.fnst@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. Fixed > >> + >> +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 ? This is up to the user to specify the commands in the order they want them applied. What other method should we be using? > >> + } >> + } >> + >> + 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? I have removed this. > >> + } >> +} >> + >> +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. Fixed > >> + "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" Fixed > >> + 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 ? This allows setting values from the HMP command line interface once the machine is running. The dc->reset isn't applied in that case. > >> + >> + 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. Agreed, I just wanted to make sure that this approach was agreed upon before I converted every other image loading. > > 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. That's fair, but upsetting. Thanks, Alistair > > thanks > -- PMM >
On 13 July 2016 at 18:45, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Tue, Jul 12, 2016 at 9:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@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.francis@xilinx.com> >>> --- >>> + if (s->cpu) { >>> + CPUClass *cc = CPU_GET_CLASS(s->cpu); >>> + cpu_reset(s->cpu); >> >> 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 ? > > This is up to the user to specify the commands in the order they want > them applied. What other method should we be using? Well, if it works it works, I guess, but one day we will need to figure out an actual model for reset order rather than having it work by chance (in this case, because command line devices get reset after ones in the board model proper, I think.) >>> + qemu_register_reset(generic_loader_reset, dev); >> >> What's wrong with a device reset function set via dc->reset ? > > This allows setting values from the HMP command line interface once > the machine is running. The dc->reset isn't applied in that case. I don't understand this -- could you explain in a bit more detail, please? >> 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. > > That's fair, but upsetting. I'm not really happy about pushing this back yet another release either, and there are two weeks left til hard freeze. So it's not impossible that we could fit it in, but we're still discussing the semantics of the device at this point, so if it did get into 2.7 I think it would be ending up going in really fairly late, and I tend by preference to be conservative about freezes. Wider review (ie somebody else as well as me) would help in providing reassurance that the interface is correct. thanks -- PMM
On Wed, Jul 13, 2016 at 12:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 13 July 2016 at 18:45, Alistair Francis <alistair.francis@xilinx.com> wrote: >> On Tue, Jul 12, 2016 at 9:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@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.francis@xilinx.com> >>>> --- > >>>> + if (s->cpu) { >>>> + CPUClass *cc = CPU_GET_CLASS(s->cpu); >>>> + cpu_reset(s->cpu); >>> >>> 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 ? >> >> This is up to the user to specify the commands in the order they want >> them applied. What other method should we be using? > > Well, if it works it works, I guess, but one day we > will need to figure out an actual model for reset order > rather than having it work by chance (in this case, > because command line devices get reset after ones in > the board model proper, I think.) Agreed, but I think that is out of scope of this series. That would require a overhaul of the reset infrastructure to allow a more configurable system or at least some stricter ordering rules to follow. I thought I saw some patches on the list to start to improve the reset functionality? > >>>> + qemu_register_reset(generic_loader_reset, dev); >>> >>> What's wrong with a device reset function set via dc->reset ? >> >> This allows setting values from the HMP command line interface once >> the machine is running. The dc->reset isn't applied in that case. > > I don't understand this -- could you explain in a bit > more detail, please? So this loading system is just a device, which means that it can be loaded whenever a normal device would be loaded. The more interesting option is using this via the command line but it is also possible to use the QEMU Monitor once QEMU is running to call this. A user can use the device_add function to add set values once QEMU is running. The problem with that though is that the qdev_device_add() function doesn't connect the reset function. I initially had a patch to do this "qdev-monitor.c: Register reset function if the device has one" but it was decided to just register the reset function in the realise instead. At the moment rom loading is only supported at startup, but in the future maybe that could be supported as well, adding even more value to this. > >>> 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. >> >> That's fair, but upsetting. > > I'm not really happy about pushing this back yet another > release either, and there are two weeks left til hard > freeze. So it's not impossible that we could fit it in, > but we're still discussing the semantics of the device > at this point, so if it did get into 2.7 I think it > would be ending up going in really fairly late, and I > tend by preference to be conservative about freezes. > Wider review (ie somebody else as well as me) would help > in providing reassurance that the interface is correct. That is fair, I can see that it is risky. I have the next version ready to send, I'll send it by the end of today (PST time) if no other issues come up. Maybe it can still make it in. Thanks, Alistair > > thanks > -- PMM >
On 13 July 2016 at 21:30, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Wed, Jul 13, 2016 at 12:44 PM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> On 13 July 2016 at 18:45, Alistair Francis <alistair.francis@xilinx.com> wrote: >>> On Tue, Jul 12, 2016 at 9:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>> On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@xilinx.com> wrote: >>>>> + qemu_register_reset(generic_loader_reset, dev); >>>> >>>> What's wrong with a device reset function set via dc->reset ? >>> >>> This allows setting values from the HMP command line interface once >>> the machine is running. The dc->reset isn't applied in that case. >> >> I don't understand this -- could you explain in a bit >> more detail, please? > > So this loading system is just a device, which means that it can be > loaded whenever a normal device would be loaded. The more interesting > option is using this via the command line but it is also possible to > use the QEMU Monitor once QEMU is running to call this. > > A user can use the device_add function to add set values once QEMU is > running. The problem with that though is that the qdev_device_add() > function doesn't connect the reset function. I initially had a patch > to do this "qdev-monitor.c: Register reset function if the device has > one" but it was decided to just register the reset function in the > realise instead. I'm really surprised that device_add doesn't wire up the device's reset method. This sounds to me like it's a bug -- adding a device dynamically via the monitor shouldn't behave any differently to adding it via the command line. If we are doing this to avoid a bug (and it's too risky to fix that bug at this point) then we should have a comment explaining why we're not using dc->reset, so we can fix this when we do fix device_add. thanks -- PMM
On Wed, Jul 13, 2016 at 2:09 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 13 July 2016 at 21:30, Alistair Francis <alistair.francis@xilinx.com> wrote: >> On Wed, Jul 13, 2016 at 12:44 PM, Peter Maydell >> <peter.maydell@linaro.org> wrote: >>> On 13 July 2016 at 18:45, Alistair Francis <alistair.francis@xilinx.com> wrote: >>>> On Tue, Jul 12, 2016 at 9:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>>> On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@xilinx.com> wrote: >>>>>> + qemu_register_reset(generic_loader_reset, dev); >>>>> >>>>> What's wrong with a device reset function set via dc->reset ? >>>> >>>> This allows setting values from the HMP command line interface once >>>> the machine is running. The dc->reset isn't applied in that case. >>> >>> I don't understand this -- could you explain in a bit >>> more detail, please? >> >> So this loading system is just a device, which means that it can be >> loaded whenever a normal device would be loaded. The more interesting >> option is using this via the command line but it is also possible to >> use the QEMU Monitor once QEMU is running to call this. >> >> A user can use the device_add function to add set values once QEMU is >> running. The problem with that though is that the qdev_device_add() >> function doesn't connect the reset function. I initially had a patch >> to do this "qdev-monitor.c: Register reset function if the device has >> one" but it was decided to just register the reset function in the >> realise instead. > > I'm really surprised that device_add doesn't wire up the device's > reset method. This sounds to me like it's a bug -- adding a device > dynamically via the monitor shouldn't behave any differently > to adding it via the command line. I thought the same as you, but there was a general consensus that registering the reset there was not correct as there are issues with bus-less devices. I think this comes back to the infrastructure not being ready to connect all devices via the device_add, hopefully in the future this can be improved on. > > If we are doing this to avoid a bug (and it's too risky > to fix that bug at this point) then we should have a comment > explaining why we're not using dc->reset, so we can fix this > when we do fix device_add. I wouldn't call it a bug, I would call it more of a limitation. I'll add a comment. Thanks, Alistair > > thanks > -- PMM >
diff --git a/MAINTAINERS b/MAINTAINERS index 2ab6e3b..0077e22 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -992,6 +992,12 @@ M: Dmitry Fleytman <dmitry@daynix.com> S: Maintained F: hw/net/e1000e* +Generic Loader +M: Alistair Francis <alistair.francis@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.fnst@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 + +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 (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); + } +} + +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 " + "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"); + 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); + + 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; + } + + if (size < 0) { + error_setg(errp, "Cannot load specified image %s", s->file); + return; + } + } + + /* Convert the data endiannes */ + if (s->data_be) { + s->data = cpu_to_be64(s->data); + } else { + s->data = cpu_to_le64(s->data); + } +} + +static void generic_loader_unrealize(DeviceState *dev, Error **errp) +{ + qemu_unregister_reset(generic_loader_reset, dev); +} + +static Property generic_loader_props[] = { + DEFINE_PROP_UINT64("addr", GenericLoaderState, addr, 0), + DEFINE_PROP_UINT64("data", GenericLoaderState, data, 0), + DEFINE_PROP_UINT8("data-len", GenericLoaderState, data_len, 0), + DEFINE_PROP_BOOL("data-be", GenericLoaderState, data_be, false), + DEFINE_PROP_UINT32("cpu-num", GenericLoaderState, cpu_num, CPU_NONE), + DEFINE_PROP_BOOL("force-raw", GenericLoaderState, force_raw, false), + DEFINE_PROP_STRING("file", GenericLoaderState, file), + DEFINE_PROP_END_OF_LIST(), +}; + +static void generic_loader_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->realize = generic_loader_realize; + dc->unrealize = generic_loader_unrealize; + dc->props = generic_loader_props; + dc->desc = "Generic Loader"; +} + +static TypeInfo generic_loader_info = { + .name = TYPE_GENERIC_LOADER, + .parent = TYPE_DEVICE, + .instance_size = sizeof(GenericLoaderState), + .class_init = generic_loader_class_init, +}; + +static void generic_loader_register_type(void) +{ + type_register_static(&generic_loader_info); +} + +type_init(generic_loader_register_type) diff --git a/include/hw/core/generic-loader.h b/include/hw/core/generic-loader.h new file mode 100644 index 0000000..4dd2361 --- /dev/null +++ b/include/hw/core/generic-loader.h @@ -0,0 +1,45 @@ +/* + * Generic Loader + * + * Copyright (C) 2014 Li Guang + * Written by Li Guang <lig.fnst@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. + */ + +#ifndef GENERIC_LOADER_H +#define GENERIC_LOADER_H + +#include "elf.h" + +typedef struct GenericLoaderState { + /* <private> */ + DeviceState parent_obj; + + /* <public> */ + CPUState *cpu; + + uint64_t addr; + uint64_t data; + uint8_t data_len; + uint32_t cpu_num; + + char *file; + + bool force_raw; + bool data_be; +} GenericLoaderState; + +#define TYPE_GENERIC_LOADER "loader" +#define GENERIC_LOADER(obj) OBJECT_CHECK(GenericLoaderState, (obj), \ + TYPE_GENERIC_LOADER) + +#endif
Add a generic loader to QEMU which can be used to load images or set memory values. Signed-off-by: Alistair Francis <alistair.francis@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