Add a generic loader
v3 cover Add a generic loader
This work is based on the original work by Li Guang with extra features added by Peter C and myself.
The idea of this loader is to allow the user to load multiple images or values into QEMU at startup.
Memory values can be loaded like this: -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
Images can be loaded like this: -device loader,file=./images/u-boot.elf,cpu=0
This can be useful and we use it a lot in Xilinx to load multiple images into a machine at creation (ATF, Kernel and DTB for example).
It can also be used to set registers.
The limiation for arch is based off settting the ELF_ARCH macro.
The reset patch is required otherwise the reset will never be registered and the loader can't change the PC in the case of images.
V2:
- Add an entry to the maintainers file
- Add some documentation
- Perform bounds checking on the data_len
- Register and unregister the reset in the realise/unrealise
Changes since RFC:
- Add support for BE
Alistair Francis (2):
generic-loader: Add a generic loader
docs: Add a generic loader explanation document
MAINTAINERS | 6 ++ default-configs/arm-softmmu.mak | 1 + docs/generic-loader.txt | 21 ++++++ hw/misc/Makefile.objs | 2 + hw/misc/generic-loader.c | 143 +++++++++++++++++++++++++++++++++++++++ include/hw/misc/generic-loader.h | 50 ++++++++++++++ 6 files changed, 223 insertions(+) create mode 100644 docs/generic-loader.txt create mode 100644 hw/misc/generic-loader.c create mode 100644 include/hw/misc/generic-loader.h
Signature · 1 line
2.5.0
PATCH v3 1/2 generic-loader: Add a generic loader
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.
---
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
MAINTAINERS +6 -0
@@ -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 <[email protected]> +S: Maintained +F: hw/misc/generic-loader.c +F: include/hw/misc/generic-loader.h + Subsystems ---------- Audio
default-configs/arm-softmmu.mak +1 -0
@@ -110,3 +110,4 @@ CONFIG_IOH3420=y CONFIG_I82801B11=y CONFIG_ACPI=y CONFIG_SMBIOS=y +CONFIG_LOADER=y
hw/misc/Makefile.objs +2 -0
@@ -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
hw/misc/generic-loader.c new file +143 -0
143 lines not on this page · raw message
include/hw/misc/generic-loader.h new file +50 -0
50 lines not on this page · raw message
Signature · 1 line
2.5.0
Re: [Qemu-devel] [PATCH v3 1/2] generic-loader: Add a generic loader
Quoted 2 lines · Alistair Francis
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.
Quoted 1 line
This only supports ARM architectures at the moment.
This doesn't sound very generic :-)
Quoted 77 lines
Signed-off-by Alistair Francis <[email protected]>---
V3:
- Pass theram_sizetoload_image_targphys()
V2:
- Add maintainers entry
- Perform bounds checking
- Register and unregister the reset in the realise/unrealise
Changes since RFC:
- Add BE supportMAINTAINERS | 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.hMAINTAINERS +6
-0@@ -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 <[email protected]> +S: Maintained +F: hw/misc/generic-loader.c +F: include/hw/misc/generic-loader.h + Subsystems ---------- Audiodefault-configs/arm-softmmu.mak +1
-0@@ -110,3 +110,4 @@ CONFIG_IOH3420=y CONFIG_I82801B11=y CONFIG_ACPI=y CONFIG_SMBIOS=y +CONFIG_LOADER=yhw/misc/Makefile.objs +2
-0@@ -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.ohw/misc/generic-loader.c new file +15
-0@@ -0,0 +1,143 @@ +/* +* Generic Loader +* +* Copyright (C) 2014 Li Guang +* Copyright (C) 2016 Xilinx Inc. +* Written by Li Guang <[email protected]> +* +* 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
hw/misc/generic-loader.c line 25
Quoted 10 lines
hw/misc/generic-loader.c new file +10
-0+* 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.
hw/misc/generic-loader.c line 34
Quoted 9 lines
hw/misc/generic-loader.c new file +9
-0+ +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) ?
hw/misc/generic-loader.c line 40
Quoted 6 lines
hw/misc/generic-loader.c new file +6
-0+ } + +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?
Quoted 18 lines
hw/misc/generic-loader.c new file +18
-0+ } +} + +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) {
hw/misc/generic-loader.c line 68
Quoted 10 lines
hw/misc/generic-loader.c new file +10
-0+ 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.)
hw/misc/generic-loader.c line 80
Quoted 12 lines
hw/misc/generic-loader.c new file +12
-0+ } + +#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'.
hw/misc/generic-loader.c line 88
Quoted 8 lines
hw/misc/generic-loader.c new file +8
-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 */
"machine's".
hw/misc/generic-loader.c line 101
Quoted 13 lines
hw/misc/generic-loader.c new file +13
-0+ 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.
hw/misc/generic-loader.c line 113
Quoted 12 lines
hw/misc/generic-loader.c new file +12
-0+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 ?
hw/misc/generic-loader.c line 117
Quoted 4 lines
hw/misc/generic-loader.c new file +4
-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),
At least one of these options didn't make it to the documentation.
Quoted 49 lines
hw/misc/generic-loader.c new file +26
-0+ 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)include/hw/misc/generic-loader.h new file +17
-0@@ -0,0 +1,50 @@ +/* +* Generic Loader +* +* Copyright (C) 2014 Li Guang +* Written by Li Guang <[email protected]> +* +* 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/misc/generic-loader.h line 27
Quoted 10 lines
include/hw/misc/generic-loader.h new file +10
-0+#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.
Quoted 13 lines
include/hw/misc/generic-loader.h new file +13
-0+ +typedef struct GenericLoaderState { + /* <private> */ + DeviceState parent_obj; + + /* <public> */ + CPUState *cpu; + + uint64_t addr; + uint64_t data; + uint8_t data_len; + uint8_t cpu_nr; +
include/hw/misc/generic-loader.h line 50
Quoted 10 lines
include/hw/misc/generic-loader.h new file +10
-0+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
Re: [Qemu-devel] [PATCH v3 1/2] generic-loader: Add a generic loader
Quoted 5 lines · Peter Maydell
Quoted 2 lines · Alistair Francis
Add a generic loader to QEMU which can be used to load images or set memory values.
Quoted 7 lines
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.
Quoted 1 line
This only supports ARM architectures at the moment.
This doesn't sound very generic :-)
I have a fix for this :)
Quoted 83 lines
Quoted 83 lines
Signed-off-by Alistair Francis <[email protected]>---
V3:
- Pass theram_sizetoload_image_targphys()
V2:
- Add maintainers entry
- Perform bounds checking
- Register and unregister the reset in the realise/unrealise
Changes since RFC:
- Add BE supportMAINTAINERS | 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.hMAINTAINERS +6
-0@@ -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 <[email protected]> +S: Maintained +F: hw/misc/generic-loader.c +F: include/hw/misc/generic-loader.h + Subsystems ---------- Audiodefault-configs/arm-softmmu.mak +1
-0@@ -110,3 +110,4 @@ CONFIG_IOH3420=y CONFIG_I82801B11=y CONFIG_ACPI=y CONFIG_SMBIOS=y +CONFIG_LOADER=yhw/misc/Makefile.objs +2
-0@@ -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.ohw/misc/generic-loader.c new file +20
-0@@ -0,0 +1,143 @@ +/* +* Generic Loader +* +* Copyright (C) 2014 Li Guang +* Copyright (C) 2016 Xilinx Inc. +* Written by Li Guang <[email protected]> +* +* 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"
Quoted 8 lines
hw/misc/generic-loader.c line 25
Quoted 5 lines
hw/misc/generic-loader.c new file +5
-0+#include "sysemu/dma.h" +#include "hw/loader.h" +#include "hw/misc/generic-loader.h" + +#define CPU_NONE 0xFFThis is a mine waiting to be triggered if we ever raise
MAX_CPUMASK_BITSand allow 256 CPUs.
True. I have changed it to 0xFFFF (and a 16bit value). Is that enough?
Quoted 13 lines
hw/misc/generic-loader.c line 34
Quoted 10 lines
hw/misc/generic-loader.c new file +9
-0+ +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.
Quoted 11 lines
hw/misc/generic-loader.c line 40
Quoted 7 lines
hw/misc/generic-loader.c new file +6
-0+ } + +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->asis 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?
Quoted 23 lines
Quoted 23 lines
hw/misc/generic-loader.c new file +22
-0+ } +} + +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",
Quoted 8 lines
hw/misc/generic-loader.c line 68
Quoted 6 lines
hw/misc/generic-loader.c new file +6
-0+ s->cpu_nr); +return ; + }else { + cpu_num++; + } + }This appears to be reimplementing
qemu_get_cpu(s->cpu_nr).
Good point, I'll fix this.
Quoted 4 lines
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.
Quoted 20 lines
(We should be consistent with however the user specifies CPUs for things like CPU hotplug, anyway.)
hw/misc/generic-loader.c line 80
Quoted 12 lines
hw/misc/generic-loader.c new file +12
-0+ } + +#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? :-)
Quoted 4 lines
It looks like
load_elf()mostly takes theelf_machineargument 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!
Quoted 11 lines
hw/misc/generic-loader.c line 88
Quoted 9 lines
hw/misc/generic-loader.c new file +8
-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 */ "machine's".
Fixed
Quoted 17 lines
Quoted 4 lines
hw/misc/generic-loader.c new file +3
-0+ size =load_image_targphys (s->file, s->addr, ram_size); + }else { + s->addr = entry;hw/misc/generic-loader.c line 101
Quoted 10 lines
hw/misc/generic-loader.c new file +10
-0+ } + +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.
Quoted 17 lines
hw/misc/generic-loader.c line 113
Quoted 13 lines
hw/misc/generic-loader.c new file +12
-0+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?
Quoted 7 lines
hw/misc/generic-loader.c line 117
Quoted 5 lines
hw/misc/generic-loader.c new file +4
-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),At least one of these options didn't make it to the documentation.
Fixed
Quoted 56 lines
Quoted 56 lines
hw/misc/generic-loader.c new file +26
-0+ 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)include/hw/misc/generic-loader.h new file +23
-0@@ -0,0 +1,50 @@ +/* +* Generic Loader +* +* Copyright (C) 2014 Li Guang +* Written by Li Guang <[email protected]> +* +* 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 )
Quoted 8 lines
include/hw/misc/generic-loader.h line 27
Quoted 4 lines
include/hw/misc/generic-loader.h new file +4
-0+#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_THINGladders, 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
Quoted 28 lines
Quoted 14 lines
include/hw/misc/generic-loader.h new file +13
-0+ +typedef struct GenericLoaderState { + /* <private> */ + DeviceState parent_obj; + + /* <public> */ + CPUState *cpu; + + uint64_t addr; + uint64_t data; + uint8_t data_len; + uint8_t cpu_nr; +include/hw/misc/generic-loader.h line 50
Quoted 10 lines
include/hw/misc/generic-loader.h new file +10
-0+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
Re: [Qemu-devel] [PATCH v3 1/2] generic-loader: Add a generic loader
Quoted 7 lines · Alistair Francis
Quoted 3 lines · Peter Maydell
Writing directly to
cpu->asis 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.
Quoted 10 lines
Quoted 3 lines
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...
Quoted 2 lines
Quoted 2 lines
(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.
Quoted 9 lines
Quoted 3 lines
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
Re: [Qemu-devel] [PATCH v3 1/2] generic-loader: Add a generic loader
Quoted 7 lines · Peter Maydell
Quoted 6 lines · Alistair Francis
Quoted 3 lines · Peter Maydell
Writing directly to
cpu->asis not very generic. In particular, how should this interact with TrustZone, where you might want to write the image to the Secure address space?
Quoted 7 lines
Quoted 1 line
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.
Quoted 16 lines
Quoted 11 lines
Quoted 4 lines
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...
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
(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.
Quoted 19 lines
Quoted 10 lines
Quoted 4 lines
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".)
Quoted 5 lines
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
Quoted 4 lines
thanks
-- PMM
PATCH v3 2/2 docs: Add a generic loader explanation document
In reply to Alistair Francis---
docs/generic-loader.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 docs/generic-loader.txt
docs/generic-loader.txt new file +21 -0
@@ -0,0 +1,21 @@ +Copyright (c) 2016 Xilinx Inc. + +This work is licensed under the terms of the GNU GPL, version 2 or later. See +the COPYING file in the top-level directory. + + +This loader allows the user to load multiple images or values into QEMU at startup. + +Loading Memory Values +--------------------------- +Memory values can be loaded like this: + -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4 + +Loading Images +--------------------------- +Images can be loaded like this: + -device loader,file=./images/boot.elf,cpu=0 + +The limiation for arch is based off settting the ELF_ARCH macro. + +At the moment only the ARM arhitectures are supported
Signature · 1 line
2.5.0
Re: [Qemu-devel] [PATCH v3 2/2] docs: Add a generic loader explanation document
Quoted 13 lines · Alistair Francis
Signed-off-by Alistair Francis <[email protected]>---
docs/generic-loader.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 docs/generic-loader.txtdocs/generic-loader.txt new file +0
-0
docs/generic-loader.txt line 7
Quoted 7 lines
docs/generic-loader.txt new file +7
-0@@ -0,0 +1,21 @@ +Copyright (c) 2016 Xilinx Inc. + +This work is licensed under the terms of the GNU GPL, version 2 or later. See +the COPYING file in the top-level directory. + + +This loader allows the user to load multiple images or values into QEMU at startup.
This is a bit confusing, because it's not entirely clear what "this" is referring to. Something with a heading and then a paragraph starting "The 'loader' device allows ..." might be better.
docs/generic-loader.txt line 12
Quoted 5 lines
docs/generic-loader.txt new file +5
-0+ +Loading Memory Values +--------------------------- +Memory values can be loaded like this: + -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
What values are permitted for data-len ? What endianness is the data stored as? What address space is the data written to? Is 'cpu=N' valid here as it is for the image loader syntax below?
docs/generic-loader.txt line 19
Quoted 7 lines
docs/generic-loader.txt new file +7
-0+ +Loading Images +--------------------------- +Images can be loaded like this: + -device loader,file=./images/boot.elf,cpu=0 + +The limiation for arch is based off settting the ELF_ARCH macro.
I don't understand this sentence (also it has two typos in it :-))
docs/generic-loader.txt line 21
Quoted 2 lines
docs/generic-loader.txt new file +2
-0+ +At the moment only the ARM arhitectures are supported
"architectures". Why is there an architecture limitation? Missing full stop.
Straightforward documentation of all possible options, followed by examples, is probably a better way to structure this than trying to describe it only by example.
thanks
-- PMM
Re: [Qemu-devel] [PATCH v3 2/2] docs: Add a generic loader explanation document
Quoted 55 lines · Peter Maydell
Quoted 13 lines · Alistair Francis
Signed-off-by Alistair Francis <[email protected]>---
docs/generic-loader.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 docs/generic-loader.txtdocs/generic-loader.txt new file +0
-0docs/generic-loader.txt line 7
Quoted 7 lines
docs/generic-loader.txt new file +7
-0@@ -0,0 +1,21 @@ +Copyright (c) 2016 Xilinx Inc. + +This work is licensed under the terms of the GNU GPL, version 2 or later. See +the COPYING file in the top-level directory. + + +This loader allows the user to load multiple images or values into QEMU at startup.This is a bit confusing, because it's not entirely clear what "this" is referring to. Something with a heading and then a paragraph starting "The 'loader' device allows ..." might be better.
docs/generic-loader.txt line 12
Quoted 5 lines
docs/generic-loader.txt new file +5
-0+ +Loading Memory Values +--------------------------- +Memory values can be loaded like this: + -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4What values are permitted for data-len ? What endianness is the data stored as? What address space is the data written to? Is 'cpu=N' valid here as it is for the image loader syntax below?
docs/generic-loader.txt line 19
Quoted 7 lines
docs/generic-loader.txt new file +7
-0+ +Loading Images +--------------------------- +Images can be loaded like this: + -device loader,file=./images/boot.elf,cpu=0 + +The limiation for arch is based off settting the ELF_ARCH macro.I don't understand this sentence (also it has two typos in it :-))
docs/generic-loader.txt line 21
Quoted 2 lines
docs/generic-loader.txt new file +2
-0+ +At the moment only the ARM arhitectures are supported"architectures". Why is there an architecture limitation? Missing full stop.
Quoted 3 lines
Straightforward documentation of all possible options, followed by examples, is probably a better way to structure this than trying to describe it only by example.
Agreed! I am re-writing it, it is now much more comprehensive.
Thanks,
Alistair
Quoted 4 lines
thanks
-- PMM