| Message ID | 8e82fa93734f045394c067fc238a553340cb916f.1455913505.git.alistair.francis@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19 February 2016 at 20:40, Alistair Francis <alistair.francis@xilinx.com> wrote: > Add a generic loader to QEMU which can be used to load images or set > memory values. I'm not inherently opposed to this (it seems like a nice way to deal with the desire to load arbitrary images), but it feels a bit half-baked at the moment. More detailed comments below. > This only supports ARM architectures at the moment. This doesn't sound very generic :-) > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > 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 ++ > default-configs/arm-softmmu.mak | 1 + > hw/misc/Makefile.objs | 2 + > hw/misc/generic-loader.c | 143 +++++++++++++++++++++++++++++++++++++++ > include/hw/misc/generic-loader.h | 50 ++++++++++++++ > 5 files changed, 202 insertions(+) > create mode 100644 hw/misc/generic-loader.c > create mode 100644 include/hw/misc/generic-loader.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9adeda3..7dae3dd 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -963,6 +963,12 @@ F: hw/acpi/nvdimm.c > F: hw/mem/nvdimm.c > F: include/hw/mem/nvdimm.h > > +Generic Loader > +M: Alistair Francis <alistair.francis@xilinx.com> > +S: Maintained > +F: hw/misc/generic-loader.c > +F: include/hw/misc/generic-loader.h > + > Subsystems > ---------- > Audio > 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..20f07a8 > --- /dev/null > +++ b/hw/misc/generic-loader.c > @@ -0,0 +1,143 @@ > +/* > + * 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 "hw/sysbus.h" > +#include "sysemu/dma.h" > +#include "hw/loader.h" > +#include "hw/misc/generic-loader.h" > + > +#define CPU_NONE 0xFF This is a mine waiting to be triggered if we ever raise MAX_CPUMASK_BITS and allow 256 CPUs. > + > +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); > + cc->set_pc(s->cpu, s->addr); Why is a loader device messing with the CPU state (especially unconditionally) ? > + } > + > + 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); Writing directly to cpu->as is not very generic. In particular, how should this interact with TrustZone, where you might want to write the image to the Secure address space? > + } > +} > + > +static void generic_loader_realize(DeviceState *dev, Error **errp) > +{ > + GenericLoaderState *s = GENERIC_LOADER(dev); > + hwaddr entry; > + int big_endian; > + int size = 0; > + > + qemu_register_reset(generic_loader_reset, dev); > + > + 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 nonexistent", > + s->cpu_nr); > + return; > + } else { > + cpu_num++; > + } > + } This appears to be reimplementing qemu_get_cpu(s->cpu_nr). Is the internal QEMU CPU index really the right thing to expose to the user? Should we be considering letting the user specify by MPIDR or some other thing? By path-to-CPU-in-QOM-tree? (We should be consistent with however the user specifies CPUs for things like CPU hotplug, anyway.) > + } > + > +#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); It would be nice not to add another obstacle to supporting multiple CPU architectures in one QEMU binary, right? :-) It looks like load_elf() mostly takes the elf_machine argument in order to fall over if you pass it an ELF file of the wrong format (and partly for relocation stuff?) so perhaps we should enhance it to accept 'any architecture'. > + > + if (size < 0) { > + size = load_uimage(s->file, &entry, NULL, NULL, NULL, NULL); > + } > + } > + > + if (size < 0) { > + /* Default to the maximum size being the machines ram size */ "machine's". > + 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; > + } > + } > + > + if (s->data_len && (s->data_len > sizeof(s->data))) { > + error_setg(errp, "data-len cannot be more then the data size"); ...and the data size can't be more than 8 bytes, but you don't say that anywhere. > + return; > + } > +} > + > +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), Doesn't defining this as a UINT64 property mean that the same qemu command line will write different data on a big-endian and little-endian host ? > + 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), At least one of these options didn't make it to the documentation. > + 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/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 I definitely don't want to add any new #ifdef TARGET_THING ladders, please. Anything that must be defined per-target should be done by having a file or define or whatever in target-*/ somewhere. > + > +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 thanks -- PMM
On Fri, Feb 26, 2016 at 8:22 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 February 2016 at 20:40, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> Add a generic loader to QEMU which can be used to load images or set >> memory values. > > I'm not inherently opposed to this (it seems like a nice way > to deal with the desire to load arbitrary images), but it feels > a bit half-baked at the moment. More detailed comments below. > >> This only supports ARM architectures at the moment. > > This doesn't sound very generic :-) I have a fix for this :) > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> 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 ++ >> default-configs/arm-softmmu.mak | 1 + >> hw/misc/Makefile.objs | 2 + >> hw/misc/generic-loader.c | 143 +++++++++++++++++++++++++++++++++++++++ >> include/hw/misc/generic-loader.h | 50 ++++++++++++++ >> 5 files changed, 202 insertions(+) >> create mode 100644 hw/misc/generic-loader.c >> create mode 100644 include/hw/misc/generic-loader.h >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 9adeda3..7dae3dd 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -963,6 +963,12 @@ F: hw/acpi/nvdimm.c >> F: hw/mem/nvdimm.c >> F: include/hw/mem/nvdimm.h >> >> +Generic Loader >> +M: Alistair Francis <alistair.francis@xilinx.com> >> +S: Maintained >> +F: hw/misc/generic-loader.c >> +F: include/hw/misc/generic-loader.h >> + >> Subsystems >> ---------- >> Audio >> 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..20f07a8 >> --- /dev/null >> +++ b/hw/misc/generic-loader.c >> @@ -0,0 +1,143 @@ >> +/* >> + * 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 "hw/sysbus.h" >> +#include "sysemu/dma.h" >> +#include "hw/loader.h" >> +#include "hw/misc/generic-loader.h" >> + >> +#define CPU_NONE 0xFF > > This is a mine waiting to be triggered if we ever raise > MAX_CPUMASK_BITS and allow 256 CPUs. True. I have changed it to 0xFFFF (and a 16bit value). Is that enough? > >> + >> +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); >> + cc->set_pc(s->cpu, s->addr); > > Why is a loader device messing with the CPU state (especially > unconditionally) ? This is here for when the user loads an image that they would like to boot. QEMU needs to set the PC to the start of the image. I'll add some conditions around it. > >> + } >> + >> + 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); > > Writing directly to cpu->as is not very generic. In particular, > how should this interact with TrustZone, where you might want to > write the image to the Secure address space? Hmmm.... Good point. Do you have any ideas for a solution? > >> + } >> +} >> + >> +static void generic_loader_realize(DeviceState *dev, Error **errp) >> +{ >> + GenericLoaderState *s = GENERIC_LOADER(dev); >> + hwaddr entry; >> + int big_endian; >> + int size = 0; >> + >> + qemu_register_reset(generic_loader_reset, dev); >> + >> + 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 nonexistent", >> + s->cpu_nr); >> + return; >> + } else { >> + cpu_num++; >> + } >> + } > > This appears to be reimplementing qemu_get_cpu(s->cpu_nr). Good point, I'll fix this. > > Is the internal QEMU CPU index really the right thing to expose to > the user? Should we be considering letting the user specify by > MPIDR or some other thing? By path-to-CPU-in-QOM-tree? I don't have a problem with numbers. I don't see it as too confusing as it is generally pretty obvious which CPU you are targeting. It also matches what the 'info cpus' command prints. The kernel also just uses numbers when printing out CPU info. Plus I would consider this a more advanced feature. People who don't know about the CPUs in the system can still use -kernel. > > (We should be consistent with however the user specifies CPUs > for things like CPU hotplug, anyway.) > >> + } >> + >> +#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); > > It would be nice not to add another obstacle to supporting multiple > CPU architectures in one QEMU binary, right? :-) > > It looks like load_elf() mostly takes the elf_machine argument in > order to fall over if you pass it an ELF file of the wrong format > (and partly for relocation stuff?) so perhaps we should enhance > it to accept 'any architecture'. This is actually pretty easy to do, I have a fix! > >> + >> + if (size < 0) { >> + size = load_uimage(s->file, &entry, NULL, NULL, NULL, NULL); >> + } >> + } >> + >> + if (size < 0) { >> + /* Default to the maximum size being the machines ram size */ > > "machine's". Fixed > >> + 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; >> + } >> + } >> + >> + if (s->data_len && (s->data_len > sizeof(s->data))) { >> + error_setg(errp, "data-len cannot be more then the data size"); > > ...and the data size can't be more than 8 bytes, but you don't say > that anywhere. I'll add that in the documentation. > >> + return; >> + } >> +} >> + >> +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), > > Doesn't defining this as a UINT64 property mean that the same qemu > command line will write different data on a big-endian and little-endian > host ? Yeah, I'm not really sure what to do here. All my use cases are both LE, so I don't have any test cases. If I am understanding this right I should swap the bytes if the host and guest endianess are different? > >> + 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), > > At least one of these options didn't make it to the documentation. Fixed > >> + 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/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 > > I definitely don't want to add any new #ifdef TARGET_THING ladders, > please. Anything that must be defined per-target should be done by > having a file or define or whatever in target-*/ somewhere. Removed. Thanks, Alistair > >> + >> +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 > > thanks > -- PMM >
On 1 March 2016 at 23:56, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Fri, Feb 26, 2016 at 8:22 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> Writing directly to cpu->as is not very generic. In particular, >> how should this interact with TrustZone, where you might want to >> write the image to the Secure address space? > > Hmmm.... Good point. Do you have any ideas for a solution? Not really. For loading images into the virt board's secure-only flash I took the approach of having the loader let you load into a particular MemoryRegion, and then wiring up -bios etc to be "load into this device" rather than "load at this address". But that could be awkward to specify for the user. >> Is the internal QEMU CPU index really the right thing to expose to >> the user? Should we be considering letting the user specify by >> MPIDR or some other thing? By path-to-CPU-in-QOM-tree? > > I don't have a problem with numbers. I don't see it as too confusing > as it is generally pretty obvious which CPU you are targeting. It also > matches what the 'info cpus' command prints. The kernel also just uses > numbers when printing out CPU info. Plus I would consider this a more > advanced feature. People who don't know about the CPUs in the system > can still use -kernel. The difficulty is that command line UI is ABI for us -- once we pick it we're stuck with it. So we need to be sure we get it right... >> (We should be consistent with however the user specifies CPUs >> for things like CPU hotplug, anyway.) ...hence this hint to try to find out how we're already specifying (or planning to specify) CPUs elsewhere. >> Doesn't defining this as a UINT64 property mean that the same qemu >> command line will write different data on a big-endian and little-endian >> host ? > > Yeah, I'm not really sure what to do here. All my use cases are both > LE, so I don't have any test cases. > > If I am understanding this right I should swap the bytes if the host > and guest endianess are different? You need to first define what you want as a user interface, ie what you mean when you say data=0x123456. Then you can figure out what the right way to convert that to the values to write into the guest memory. (You might decide that the user-facing UI really needs to be something more flexible or less confusing than "here's a number which I'm going to convert into a byte string somehow".) Your constraints are that the same command line must do the same thing on any host, and that you want it to "make sense" to the user whether the guest CPU is big or little endian. (This is why I'm not sure about data=0x12345678, because the natural interpretation of that depends on CPU endianness.) thanks -- PMM
On Tue, Mar 1, 2016 at 4:07 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 1 March 2016 at 23:56, Alistair Francis <alistair.francis@xilinx.com> wrote: >> On Fri, Feb 26, 2016 at 8:22 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > >>> Writing directly to cpu->as is not very generic. In particular, >>> how should this interact with TrustZone, where you might want to >>> write the image to the Secure address space? >> >> Hmmm.... Good point. Do you have any ideas for a solution? > > Not really. For loading images into the virt board's secure-only > flash I took the approach of having the loader let you load > into a particular MemoryRegion, and then wiring up -bios etc > to be "load into this device" rather than "load at this address". > But that could be awkward to specify for the user. Yeah, I'll think about this and see if I can think of something. > >>> Is the internal QEMU CPU index really the right thing to expose to >>> the user? Should we be considering letting the user specify by >>> MPIDR or some other thing? By path-to-CPU-in-QOM-tree? >> >> I don't have a problem with numbers. I don't see it as too confusing >> as it is generally pretty obvious which CPU you are targeting. It also >> matches what the 'info cpus' command prints. The kernel also just uses >> numbers when printing out CPU info. Plus I would consider this a more >> advanced feature. People who don't know about the CPUs in the system >> can still use -kernel. > > The difficulty is that command line UI is ABI for us -- once > we pick it we're stuck with it. So we need to be sure we > get it right... > >>> (We should be consistent with however the user specifies CPUs >>> for things like CPU hotplug, anyway.) > > ...hence this hint to try to find out how we're already > specifying (or planning to specify) CPUs elsewhere. I didn't think of that, that's a good point. I'll have a look on the mailing list to see what I can find. Right now (I haven't looked yet) I'm thinking that maybe it would be a good idea to rename the 'cpu' option to 'cpu-num' and allow it to specify a number. Then when we have a standard CPU plan the 'cpu' property can be extended to follow that and users can use either option. > >>> Doesn't defining this as a UINT64 property mean that the same qemu >>> command line will write different data on a big-endian and little-endian >>> host ? >> >> Yeah, I'm not really sure what to do here. All my use cases are both >> LE, so I don't have any test cases. >> >> If I am understanding this right I should swap the bytes if the host >> and guest endianess are different? > > You need to first define what you want as a user interface, > ie what you mean when you say data=0x123456. Then you can > figure out what the right way to convert that to the values > to write into the guest memory. (You might decide that the > user-facing UI really needs to be something more flexible > or less confusing than "here's a number which I'm going to > convert into a byte string somehow".) > > Your constraints are that the same command line must do the > same thing on any host, and that you want it to "make sense" > to the user whether the guest CPU is big or little endian. > (This is why I'm not sure about data=0x12345678, because the > natural interpretation of that depends on CPU endianness.) Would it be too much on the user to ask them to specify the data endianess they would like the data written in? Then QEMU can convert the data from the cpu endianess to the specified endianess. Thanks, Alistair > > thanks > -- PMM >
diff --git a/MAINTAINERS b/MAINTAINERS index 9adeda3..7dae3dd 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -963,6 +963,12 @@ F: hw/acpi/nvdimm.c F: hw/mem/nvdimm.c F: include/hw/mem/nvdimm.h +Generic Loader +M: Alistair Francis <alistair.francis@xilinx.com> +S: Maintained +F: hw/misc/generic-loader.c +F: include/hw/misc/generic-loader.h + Subsystems ---------- Audio 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..20f07a8 --- /dev/null +++ b/hw/misc/generic-loader.c @@ -0,0 +1,143 @@ +/* + * 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 "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(void *opaque) +{ + GenericLoaderState *s = GENERIC_LOADER(opaque); + + 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) { + 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; + + qemu_register_reset(generic_loader_reset, dev); + + 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 nonexistent", + 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) { + /* Default to the maximum size being the machines 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; + } + } + + if (s->data_len && (s->data_len > sizeof(s->data))) { + error_setg(errp, "data-len cannot be more then the data size"); + return; + } +} + +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_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->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/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> --- 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 ++ default-configs/arm-softmmu.mak | 1 + hw/misc/Makefile.objs | 2 + hw/misc/generic-loader.c | 143 +++++++++++++++++++++++++++++++++++++++ include/hw/misc/generic-loader.h | 50 ++++++++++++++ 5 files changed, 202 insertions(+) create mode 100644 hw/misc/generic-loader.c create mode 100644 include/hw/misc/generic-loader.h