| Message ID | 174e5d40f6c59bf57ea997c0575f21b8b67ce5d6.1455739133.git.alistair.francis@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/17/2016 02:04 PM, Alistair Francis wrote: > Add a generic loader to QEMU which can be used to load images or set > memory values. > > This only supports ARM architectures at the moment. > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > Changes since RFC: > - Add BE support > > hw/misc/generic-loader.c | 127 +++++++++++++++++++++++++++++++++++++++ > include/hw/misc/generic-loader.h | 50 +++++++++++++++ > 4 files changed, 180 insertions(+) > create mode 100644 hw/misc/generic-loader.c > create mode 100644 include/hw/misc/generic-loader.h We really ought to improve checkpatch.pl to flag patches that add new files not covered by MAINTAINERS. > +++ b/hw/misc/generic-loader.c > @@ -0,0 +1,127 @@ > +/* > + * Generic Loader > + * > + * Copyright (C) 2014 Li Guang > + * Written by Li Guang <lig.fnst@cn.fujitsu.com> Want to claim 2016? > > + > +#include "hw/sysbus.h" > +#include "sysemu/dma.h" > +#include "hw/loader.h" > +#include "hw/misc/generic-loader.h" New .c files should include "qemu/osdep.h" first, before anything else. > +static void generic_loader_realize(DeviceState *dev, Error **errp) > +{ > + GenericLoaderState *s = GENERIC_LOADER(dev); > + hwaddr entry; > + int big_endian; > + int size = 0; > + > + if (s->cpu_nr != CPU_NONE) { > + CPUState *cs = first_cpu; > + int cpu_num = 0; > + > + CPU_FOREACH(cs) { > + if (cpu_num == s->cpu_nr) { > + s->cpu = cs; > + break; > + } else if (!CPU_NEXT(cs)) { > + error_setg(errp, "Specified boot CPU#%d is non existant", > + s->cpu_nr); s/non existant/nonexistent/ > +++ b/include/hw/misc/generic-loader.h > @@ -0,0 +1,50 @@ > +/* > + * Generic Loader > + * > + * Copyright (C) 2014 Li Guang > + * Written by Li Guang <lig.fnst@cn.fujitsu.com> 2016
On Wed, Feb 17, 2016 at 1:41 PM, Eric Blake <eblake@redhat.com> wrote: > On 02/17/2016 02:04 PM, Alistair Francis wrote: >> Add a generic loader to QEMU which can be used to load images or set >> memory values. >> >> This only supports ARM architectures at the moment. >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> Changes since RFC: >> - Add BE support >> > >> hw/misc/generic-loader.c | 127 +++++++++++++++++++++++++++++++++++++++ >> include/hw/misc/generic-loader.h | 50 +++++++++++++++ >> 4 files changed, 180 insertions(+) >> create mode 100644 hw/misc/generic-loader.c >> create mode 100644 include/hw/misc/generic-loader.h > > We really ought to improve checkpatch.pl to flag patches that add new > files not covered by MAINTAINERS. Adding an entry for this. > >> +++ b/hw/misc/generic-loader.c >> @@ -0,0 +1,127 @@ >> +/* >> + * Generic Loader >> + * >> + * Copyright (C) 2014 Li Guang >> + * Written by Li Guang <lig.fnst@cn.fujitsu.com> > > Want to claim 2016? Yep, I can do that. I'm never too sure when this can be changed or not. Should I add a written by as well? > >> >> + >> +#include "hw/sysbus.h" >> +#include "sysemu/dma.h" >> +#include "hw/loader.h" >> +#include "hw/misc/generic-loader.h" > > New .c files should include "qemu/osdep.h" first, before anything else. Adding it > >> +static void generic_loader_realize(DeviceState *dev, Error **errp) >> +{ >> + GenericLoaderState *s = GENERIC_LOADER(dev); >> + hwaddr entry; >> + int big_endian; >> + int size = 0; >> + >> + if (s->cpu_nr != CPU_NONE) { >> + CPUState *cs = first_cpu; >> + int cpu_num = 0; >> + >> + CPU_FOREACH(cs) { >> + if (cpu_num == s->cpu_nr) { >> + s->cpu = cs; >> + break; >> + } else if (!CPU_NEXT(cs)) { >> + error_setg(errp, "Specified boot CPU#%d is non existant", >> + s->cpu_nr); > > s/non existant/nonexistent/ Thanks, fixed Thanks, Alistair > > >> +++ b/include/hw/misc/generic-loader.h >> @@ -0,0 +1,50 @@ >> +/* >> + * Generic Loader >> + * >> + * Copyright (C) 2014 Li Guang >> + * Written by Li Guang <lig.fnst@cn.fujitsu.com> > > 2016 > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On 02/17/2016 05:03 PM, Alistair Francis wrote: >>> +++ b/hw/misc/generic-loader.c >>> @@ -0,0 +1,127 @@ >>> +/* >>> + * Generic Loader >>> + * >>> + * Copyright (C) 2014 Li Guang >>> + * Written by Li Guang <lig.fnst@cn.fujitsu.com> >> >> Want to claim 2016? > > Yep, I can do that. I'm never too sure when this can be changed or > not. Should I add a written by as well? I'm not a lawyer, so my response may not be authoritative; in particular, your employer may have specific rules that you must follow for any code you submit that was written on your employer's time, and that trumps anything I say here (that is, trust your lawyers more than you trust me). But in general, I tend to go by the simple rule of listing the first year that any of the code was first developed (if you are copying significant portions from some other file, then use the starting year from that file, even if your file didn't exist back then), through the current year, if my change is non-trivial (more than 10 lines, or altering an interface), while ignoring the issue for trivial things (such as fixing a typo, or doing a bulk search-and-replace across the tree). As for an authorship line, I tend to omit those (they quickly go stale, and git history is sufficient for a much more accurate picture); the copyright line is more important legally than any author line.
On Wed, Feb 17, 2016 at 4:11 PM, Eric Blake <eblake@redhat.com> wrote: > On 02/17/2016 05:03 PM, Alistair Francis wrote: > >>>> +++ b/hw/misc/generic-loader.c >>>> @@ -0,0 +1,127 @@ >>>> +/* >>>> + * Generic Loader >>>> + * >>>> + * Copyright (C) 2014 Li Guang >>>> + * Written by Li Guang <lig.fnst@cn.fujitsu.com> >>> >>> Want to claim 2016? >> >> Yep, I can do that. I'm never too sure when this can be changed or >> not. Should I add a written by as well? > > I'm not a lawyer, so my response may not be authoritative; in > particular, your employer may have specific rules that you must follow > for any code you submit that was written on your employer's time, and > that trumps anything I say here (that is, trust your lawyers more than > you trust me). > > But in general, I tend to go by the simple rule of listing the first > year that any of the code was first developed (if you are copying > significant portions from some other file, then use the starting year > from that file, even if your file didn't exist back then), through the > current year, if my change is non-trivial (more than 10 lines, or > altering an interface), while ignoring the issue for trivial things > (such as fixing a typo, or doing a bulk search-and-replace across the > tree). As for an authorship line, I tend to omit those (they quickly go > stale, and git history is sufficient for a much more accurate picture); > the copyright line is more important legally than any author line. Ok, I have added a Xilinx copyright line and not bothered with a written by line. Thanks for your help. Thanks, Alistair > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On 02/17/2016 01:04 PM, Alistair Francis wrote: > +static void generic_loader_reset(DeviceState *dev) > +{ > + GenericLoaderState *s = GENERIC_LOADER(dev); > + > + if (s->cpu) { > + CPUClass *cc = CPU_GET_CLASS(s->cpu); > + cpu_reset(s->cpu); > + cc->set_pc(s->cpu, s->addr); > + } > + > + if (s->data_len) { > + dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr, &s->data, > + s->data_len); > + } > +} What happens if I accidentally make "data-len" bigger than sizeof(s->data)? I think some bounds checking is needed? Hollis Blanchard Mentor Graphics Emulation Division
On Thu, Feb 18, 2016 at 10:23 AM, Hollis Blanchard <hollis_blanchard@mentor.com> wrote: > On 02/17/2016 01:04 PM, Alistair Francis wrote: >> >> +static void generic_loader_reset(DeviceState *dev) >> +{ >> + GenericLoaderState *s = GENERIC_LOADER(dev); >> + >> + if (s->cpu) { >> + CPUClass *cc = CPU_GET_CLASS(s->cpu); >> + cpu_reset(s->cpu); >> + cc->set_pc(s->cpu, s->addr); >> + } >> + >> + if (s->data_len) { >> + dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr, >> &s->data, >> + s->data_len); >> + } >> +} > > > What happens if I accidentally make "data-len" bigger than sizeof(s->data)? > I think some bounds checking is needed? Good point! I'll add an assert as it isn't a recoverable error. Thanks, Alistair > > Hollis Blanchard > Mentor Graphics Emulation Division >
On 02/18/2016 10:49 AM, Alistair Francis wrote: > On Thu, Feb 18, 2016 at 10:23 AM, Hollis Blanchard > <hollis_blanchard@mentor.com> wrote: >> On 02/17/2016 01:04 PM, Alistair Francis wrote: >>> +static void generic_loader_reset(DeviceState *dev) >>> +{ >>> + GenericLoaderState *s = GENERIC_LOADER(dev); >>> + >>> + if (s->cpu) { >>> + CPUClass *cc = CPU_GET_CLASS(s->cpu); >>> + cpu_reset(s->cpu); >>> + cc->set_pc(s->cpu, s->addr); >>> + } >>> + >>> + if (s->data_len) { >>> + dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr, >>> &s->data, >>> + s->data_len); >>> + } >>> +} >> >> What happens if I accidentally make "data-len" bigger than sizeof(s->data)? >> I think some bounds checking is needed? > Good point! I'll add an assert as it isn't a recoverable error. Perhaps a more user-friendly error message would be, well, more user-friendly. :-) That could be done when reading the "data-len" property, in addition to an assert when using s->data_len. Hollis Blanchard Mentor Graphics Emulation Division
On Thu, Feb 18, 2016 at 10:58 AM, Hollis Blanchard <hollis_blanchard@mentor.com> wrote: > On 02/18/2016 10:49 AM, Alistair Francis wrote: >> >> On Thu, Feb 18, 2016 at 10:23 AM, Hollis Blanchard >> <hollis_blanchard@mentor.com> wrote: >>> >>> On 02/17/2016 01:04 PM, Alistair Francis wrote: >>>> >>>> +static void generic_loader_reset(DeviceState *dev) >>>> +{ >>>> + GenericLoaderState *s = GENERIC_LOADER(dev); >>>> + >>>> + if (s->cpu) { >>>> + CPUClass *cc = CPU_GET_CLASS(s->cpu); >>>> + cpu_reset(s->cpu); >>>> + cc->set_pc(s->cpu, s->addr); >>>> + } >>>> + >>>> + if (s->data_len) { >>>> + dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr, >>>> &s->data, >>>> + s->data_len); >>>> + } >>>> +} >>> >>> >>> What happens if I accidentally make "data-len" bigger than >>> sizeof(s->data)? >>> I think some bounds checking is needed? >> >> Good point! I'll add an assert as it isn't a recoverable error. > > > Perhaps a more user-friendly error message would be, well, more > user-friendly. :-) That could be done when reading the "data-len" property, > in addition to an assert when using s->data_len. Fair enough, there is now a more appropriate check in the realise function. Thanks, Alistair > > > Hollis Blanchard > Mentor Graphics Emulation Division >
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak index a9f82a1..b246b75 100644 --- a/default-configs/arm-softmmu.mak +++ b/default-configs/arm-softmmu.mak @@ -110,3 +110,4 @@ CONFIG_IOH3420=y CONFIG_I82801B11=y CONFIG_ACPI=y CONFIG_SMBIOS=y +CONFIG_LOADER=y diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs index ea6cd3c..9f05dcf 100644 --- a/hw/misc/Makefile.objs +++ b/hw/misc/Makefile.objs @@ -46,3 +46,5 @@ obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o obj-$(CONFIG_PVPANIC) += pvpanic.o obj-$(CONFIG_EDU) += edu.o obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o + +obj-$(CONFIG_LOADER) += generic-loader.o diff --git a/hw/misc/generic-loader.c b/hw/misc/generic-loader.c new file mode 100644 index 0000000..0c52c6a --- /dev/null +++ b/hw/misc/generic-loader.c @@ -0,0 +1,127 @@ +/* + * 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. + */ + +#include "hw/sysbus.h" +#include "sysemu/dma.h" +#include "hw/loader.h" +#include "hw/misc/generic-loader.h" + +#define CPU_NONE 0xFF + +static void generic_loader_reset(DeviceState *dev) +{ + GenericLoaderState *s = GENERIC_LOADER(dev); + + if (s->cpu) { + CPUClass *cc = CPU_GET_CLASS(s->cpu); + cpu_reset(s->cpu); + cc->set_pc(s->cpu, s->addr); + } + + if (s->data_len) { + 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; + + if (s->cpu_nr != CPU_NONE) { + CPUState *cs = first_cpu; + int cpu_num = 0; + + CPU_FOREACH(cs) { + if (cpu_num == s->cpu_nr) { + s->cpu = cs; + break; + } else if (!CPU_NEXT(cs)) { + error_setg(errp, "Specified boot CPU#%d is non existant", + s->cpu_nr); + return; + } else { + cpu_num++; + } + } + } + +#ifdef TARGET_WORDS_BIGENDIAN + big_endian = 1; +#else + big_endian = 0; +#endif + + if (s->file) { + if (!s->force_raw) { + size = load_elf(s->file, NULL, NULL, &entry, NULL, NULL, + big_endian, ELF_ARCH, 0); + + if (size < 0) { + size = load_uimage(s->file, &entry, NULL, NULL, NULL, NULL); + } + } + + if (size < 0) { + size = load_image_targphys(s->file, s->addr, 0); + } else { + s->addr = entry; + } + + if (size < 0) { + error_setg(errp, "Cannot load specified image %s", s->file); + return; + } + } +} + +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_UINT8("cpu", GenericLoaderState, cpu_nr, 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->reset = generic_loader_reset; + dc->realize = generic_loader_realize; + 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/misc/generic-loader.h b/include/hw/misc/generic-loader.h new file mode 100644 index 0000000..79b5536 --- /dev/null +++ b/include/hw/misc/generic-loader.h @@ -0,0 +1,50 @@ +/* + * 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" + +#if defined(TARGET_AARCH64) + #define ELF_ARCH EM_AARCH64 +#elif defined(TARGET_ARM) + #define ELF_ARCH EM_ARM +#endif + +typedef struct GenericLoaderState { + /* <private> */ + DeviceState parent_obj; + + /* <public> */ + CPUState *cpu; + + uint64_t addr; + uint64_t data; + uint8_t data_len; + uint8_t cpu_nr; + + char *file; + + bool force_raw; +} 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. This only supports ARM architectures at the moment. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- Changes since RFC: - Add BE support default-configs/arm-softmmu.mak | 1 + hw/misc/Makefile.objs | 2 + hw/misc/generic-loader.c | 127 +++++++++++++++++++++++++++++++++++++++ include/hw/misc/generic-loader.h | 50 +++++++++++++++ 4 files changed, 180 insertions(+) create mode 100644 hw/misc/generic-loader.c create mode 100644 include/hw/misc/generic-loader.h