Add a generic loader

PATCH Superseded Alistair Francis 3 participants 23 messages Jul 2, 2016 – Jul 13, 2016

lore

Share & tools

lore

Alistair Francis

author

2016-07-02 01:07 UTC #0 lore raw

v8 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.

This patch series makes the load_elf() function more generic by not requiring an architecture. It also adds a new function load_elf_as() which allows custom AddressSpaces when loading ELF images.

At the moment I think the AddressSpace loading support is more of an RFC. If people agree with the way I'm doing it I will expand the support to image types.

V8:
- Allow custom AddressSpaces when loading images
- Move ELF architecture handling code
- Rebase
- Corrections to loading code
- Corrections to documentation
V7:
- Fix typo in comment
- Rebase
V6:
- Add error checking
V5:
- Rebase
V4:
- Re-write documentation
- 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
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 (5):
loader: Allow ELF loader to auto-detect the ELF arch
loader: All a custom SddressSpace when loading ROMs
loader: Add AddressSpace loading support to ELFs
generic-loader: Add a generic loader
docs: Add a generic loader explanation document

 MAINTAINERS                      |   6 ++
 docs/generic-loader.txt          |  60 +++++++++++++
 hw/core/Makefile.objs            |   2 +
 hw/core/generic-loader.c         | 177 +++++++++++++++++++++++++++++++++++++++
 hw/core/loader.c                 |  34 ++++++--
 include/hw/core/generic-loader.h |  45 ++++++++++
 include/hw/elf_ops.h             |  10 ++-
 include/hw/loader.h              |  24 ++++--
 8 files changed, 343 insertions(+), 15 deletions(-)
 create mode 100644 docs/generic-loader.txt
 create mode 100644 hw/core/generic-loader.c
 create mode 100644 include/hw/core/generic-loader.h

Signature · 1 line

2.7.4

Alistair Francis

author

2016-07-02 01:07 UTC #1 lore raw

PATCH v8 1/5 loader: Allow ELF loader to auto-detect the ELF arch

If the caller didn't specify an architecture for the ELF machine the load_elf() function will auto detect it based on the ELF file.

Signed-off-by Alistair Francis <[email protected]>

---
V8:
- Move into load_elf64/load_elf32
V7:
- Fix typo

 include/hw/elf_ops.h | 5 +++++
 1 file changed, 5 insertions(+)

include/hw/elf_ops.h +5 -0

index f510e7e..db70c11 100644
@@ -280,6 +280,11 @@ static int glue(load_elf, SZ)(const char *name, int fd,
         glue(bswap_ehdr, SZ)(&ehdr);
     }
 
+    if (elf_machine < 1) {
+        /* The caller didn't specify an ARCH, we can figure it out */
+        elf_machine = ehdr.e_machine;
+    }
+
     switch (elf_machine) {
         case EM_PPC64:
             if (ehdr.e_machine != EM_PPC64) {

Signature · 1 line

2.7.4

Peter Maydell

2016-07-12 16:40 UTC #2 lore raw

Re: [Qemu-devel] [PATCH v8 1/5] loader: Allow ELF loader to auto-detect the ELF arch

Quoted 19 lines · Alistair Francis

If the caller didn't specify an architecture for the ELF machine the load_elf() function will auto detect it based on the ELF file.
Signed-off-by Alistair Francis <[email protected]>
---
V8:
- Move into load_elf64/load_elf32
V7:
- Fix typo
 include/hw/elf_ops.h | 5 +++++
 1 file changed, 5 insertions(+)
include/hw/elf_ops.h +0 -0
index f510e7e..db70c11 100644
@@ -280,6 +280,11 @@ static int glue(load_elf, SZ)(const char *name, int fd,
         glue(bswap_ehdr, SZ)(&ehdr);

include/hw/elf_ops.h static int glue(load_elf, SZ)(const char *name, int fd, · line 290

Quoted 10 lines

include/hw/elf_ops.h +5 -0

     }
 
+    if (elf_machine < 1) {
+        /* The caller didn't specify an ARCH, we can figure it out */
+        elf_machine = ehdr.e_machine;
+    }
+
     switch (elf_machine) {
         case EM_PPC64:
             if (ehdr.e_machine != EM_PPC64) {

Is there also a doc comment that should be updated with this change?

thanks
-- PMM

Alistair Francis

author

2016-07-13 00:44 UTC #3 lore raw

Re: [Qemu-devel] [PATCH v8 1/5] loader: Allow ELF loader to auto-detect the ELF arch

Quoted 22 lines · Peter Maydell

Quoted 21 lines · Alistair Francis
If the caller didn't specify an architecture for the ELF machine the load_elf() function will auto detect it based on the ELF file.
Signed-off-by Alistair Francis <[email protected]>
---
V8:
- Move into load_elf64/load_elf32
V7:
- Fix typo
 include/hw/elf_ops.h | 5 +++++
 1 file changed, 5 insertions(+)
include/hw/elf_ops.h +0 -0
index f510e7e..db70c11 100644
@@ -280,6 +280,11 @@ static int glue(load_elf, SZ)(const char *name, int fd,
         glue(bswap_ehdr, SZ)(&ehdr);
     }
 

Quoted 10 lines

include/hw/elf_ops.h static int glue(load_elf, SZ)(const char *name, int fd, · line 290
Quoted 8 lines
include/hw/elf_ops.h +5 -0
+    if (elf_machine < 1) {
+        /* The caller didn't specify an ARCH, we can figure it out */
+        elf_machine = ehdr.e_machine;
+    }
+
     switch (elf_machine) {
         case EM_PPC64:
             if (ehdr.e_machine != EM_PPC64) {
Is there also a doc comment that should be updated with this change?

Yep, updated.

Thanks,

Alistair

Quoted 4 lines

thanks
-- PMM

Alistair Francis

author

2016-07-02 01:07 UTC #4 lore raw

PATCH v8 2/5 loader: All a custom SddressSpace when loading ROMs

In reply to Alistair Francis

When loading ROMs allow the caller to specify an AddressSpace to use for the load.

Signed-off-by Alistair Francis <[email protected]>

---
V8:
- Introduce an RFC version of AddressSpace loading support

 hw/core/loader.c     | 18 ++++++++++++------
 include/hw/elf_ops.h |  2 +-
 include/hw/loader.h  | 10 ++++++----
 3 files changed, 19 insertions(+), 11 deletions(-)

hw/core/loader.c +12 -6

index 53e0e41..fcbcfbf 100644
@@ -777,6 +777,7 @@ struct Rom {
 
     uint8_t *data;
     MemoryRegion *mr;
+    AddressSpace *as;
     int isrom;
     char *fw_dir;
     char *fw_file;

@@ -833,7 +834,8 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
 
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex,
-                 bool option_rom, MemoryRegion *mr)
+                 bool option_rom, MemoryRegion *mr,
+                 AddressSpace *as)
 {
     MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
     Rom *rom;

@@ -969,7 +971,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
  * memory ownership of "data", so we don't have to allocate and copy the buffer.
  */
 int rom_add_elf_program(const char *name, void *data, size_t datasize,
-                        size_t romsize, hwaddr addr)
+                        size_t romsize, hwaddr addr, AddressSpace *as)
 {
     Rom *rom;

@@ -979,18 +981,19 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize,
     rom->datasize = datasize;
     rom->romsize  = romsize;
     rom->data     = data;
+    rom->as       = as;
     rom_insert(rom);
     return 0;
 }
 
 int rom_add_vga(const char *file)
 {
-    return rom_add_file(file, "vgaroms", 0, -1, true, NULL);
+    return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL);
 }
 
 int rom_add_option(const char *file, int32_t bootindex)
 {
-    return rom_add_file(file, "genroms", 0, bootindex, true, NULL);
+    return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL);
 }
 
 static void rom_reset(void *unused)

@@ -1008,7 +1011,8 @@ static void rom_reset(void *unused)
             void *host = memory_region_get_ram_ptr(rom->mr);
             memcpy(host, rom->data, rom->datasize);
         } else {
-            cpu_physical_memory_write_rom(&address_space_memory,
+            cpu_physical_memory_write_rom(rom->as ? rom->as :
+                                                    &address_space_memory,
                                           rom->addr, rom->data, rom->datasize);
         }
         if (rom->isrom) {

@@ -1031,12 +1035,13 @@ int rom_check_and_register_reset(void)
     hwaddr addr = 0;
     MemoryRegionSection section;
     Rom *rom;
+    AddressSpace *as = NULL;
 
     QTAILQ_FOREACH(rom, &roms, next) {
         if (rom->fw_file) {
             continue;
         }
-        if (addr > rom->addr) {
+        if ((addr > rom->addr) && (as == rom->as)) {
             fprintf(stderr, "rom: requested regions overlap "
                     "(rom %s. free=0x" TARGET_FMT_plx
                     ", addr=0x" TARGET_FMT_plx ")\n",

@@ -1048,6 +1053,7 @@ int rom_check_and_register_reset(void)
         section = memory_region_find(get_system_memory(), rom->addr, 1);
         rom->isrom = int128_nz(section.size) && memory_region_is_rom(section.mr);
         memory_region_unref(section.mr);
+        as = rom->as;
     }
     qemu_register_reset(rom_reset, NULL);
     roms_loaded = 1;

include/hw/elf_ops.h +1 -1

index db70c11..1339677 100644
@@ -405,7 +405,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
             snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
 
             /* rom_add_elf_program() seize the ownership of 'data' */
-            rom_add_elf_program(label, data, file_size, mem_size, addr);
+            rom_add_elf_program(label, data, file_size, mem_size, addr, NULL);
 
             total_size += mem_size;
             if (addr < low)

include/hw/loader.h +6 -4

index 4879b63..18eb0f2 100644
@@ -118,14 +118,14 @@ extern bool rom_file_has_mr;
 
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex,
-                 bool option_rom, MemoryRegion *mr);
+                 bool option_rom, MemoryRegion *mr, AddressSpace *as);
 MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
                            size_t max_len, hwaddr addr,
                            const char *fw_file_name,
                            FWCfgReadCallback fw_callback,
                            void *callback_opaque);
 int rom_add_elf_program(const char *name, void *data, size_t datasize,
-                        size_t romsize, hwaddr addr);
+                        size_t romsize, hwaddr addr, AddressSpace *as);
 int rom_check_and_register_reset(void);
 void rom_set_fw(FWCfgState *f);
 void rom_set_order_override(int order);

@@ -135,11 +135,13 @@ void *rom_ptr(hwaddr addr);
 void hmp_info_roms(Monitor *mon, const QDict *qdict);
 
 #define rom_add_file_fixed(_f, _a, _i)          \
-    rom_add_file(_f, NULL, _a, _i, false, NULL)
+    rom_add_file(_f, NULL, _a, _i, false, NULL, NULL)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL)
 #define rom_add_file_mr(_f, _mr, _i)            \
-    rom_add_file(_f, NULL, 0, _i, false, _mr)
+    rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
+#define rom_add_file_as(_f, _as, _i)            \
+    rom_add_file(_f, NULL, 0, _i, false, NULL, _as)
 
 #define PC_ROM_MIN_VGA     0xc0000
 #define PC_ROM_MIN_OPTION  0xc8000

Signature · 1 line

2.7.4

Peter Maydell

2016-07-12 16:56 UTC #5 lore raw

Re: [Qemu-devel] [PATCH v8 2/5] loader: All a custom SddressSpace when loading ROMs

Quoted 26 lines · Alistair Francis

When loading ROMs allow the caller to specify an AddressSpace to use for the load.
Signed-off-by Alistair Francis <[email protected]>
---
V8:
- Introduce an RFC version of AddressSpace loading support
 hw/core/loader.c     | 18 ++++++++++++------
 include/hw/elf_ops.h |  2 +-
 include/hw/loader.h  | 10 ++++++----
 3 files changed, 19 insertions(+), 11 deletions(-)
hw/core/loader.c +1 -0
index 53e0e41..fcbcfbf 100644
@@ -777,6 +777,7 @@ struct Rom {
 
     uint8_t *data;
     MemoryRegion *mr;
+    AddressSpace *as;
     int isrom;
     char *fw_dir;
     char *fw_file;

hw/core/loader.c static void *rom_set_mr(Rom *rom, Object *owner, const char *name) · line 838

Quoted 6 lines

hw/core/loader.c +2 -1

@@ -833,7 +834,8 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
 
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex,
-                 bool option_rom, MemoryRegion *mr)
+                 bool option_rom, MemoryRegion *mr,
+                 AddressSpace *as)

We seem to add this argument to the function but never use it?

I think specifying both mr and as should be an error.

Quoted 60 lines

hw/core/loader.c +8 -5

 {
     MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
     Rom *rom;

@@ -969,7 +971,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
  * memory ownership of "data", so we don't have to allocate and copy the buffer.
  */
 int rom_add_elf_program(const char *name, void *data, size_t datasize,
-                        size_t romsize, hwaddr addr)
+                        size_t romsize, hwaddr addr, AddressSpace *as)
 {
     Rom *rom;

@@ -979,18 +981,19 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize,
     rom->datasize = datasize;
     rom->romsize  = romsize;
     rom->data     = data;
+    rom->as       = as;
     rom_insert(rom);
     return 0;
 }
 
 int rom_add_vga(const char *file)
 {
-    return rom_add_file(file, "vgaroms", 0, -1, true, NULL);
+    return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL);
 }
 
 int rom_add_option(const char *file, int32_t bootindex)
 {
-    return rom_add_file(file, "genroms", 0, bootindex, true, NULL);
+    return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL);
 }
 
 static void rom_reset(void *unused)

@@ -1008,7 +1011,8 @@ static void rom_reset(void *unused)
             void *host = memory_region_get_ram_ptr(rom->mr);
             memcpy(host, rom->data, rom->datasize);
         } else {
-            cpu_physical_memory_write_rom(&address_space_memory,
+            cpu_physical_memory_write_rom(rom->as ? rom->as :
+                                                    &address_space_memory,
                                           rom->addr, rom->data, rom->datasize);
         }
         if (rom->isrom) {

@@ -1031,12 +1035,13 @@ int rom_check_and_register_reset(void)
     hwaddr addr = 0;
     MemoryRegionSection section;
     Rom *rom;
+    AddressSpace *as = NULL;
 
     QTAILQ_FOREACH(rom, &roms, next) {
         if (rom->fw_file) {
             continue;
         }
-        if (addr > rom->addr) {
+        if ((addr > rom->addr) && (as == rom->as)) {
             fprintf(stderr, "rom: requested regions overlap "
                     "(rom %s. free=0x" TARGET_FMT_plx
                     ", addr=0x" TARGET_FMT_plx ")\n",

hw/core/loader.c int rom_check_and_register_reset(void) · line 1053

Quoted 1 line

hw/core/loader.c +0 -0

@@ -1048,6 +1053,7 @@ int rom_check_and_register_reset(void)
         section = memory_region_find(get_system_memory(), rom->addr, 1);

(An unrelated bug but I've just noticed that this code doesn't make sense for ROMs which go into a memory region rather than at an address in the system address space.)

hw/core/loader.c int rom_check_and_register_reset(void) · line 1057

Quoted 4 lines

hw/core/loader.c +1 -0

         rom->isrom = int128_nz(section.size) && memory_region_is_rom(section.mr);
         memory_region_unref(section.mr);
+        as = rom->as;
     }

I don't think this attempt at checking is going to actually catch all the overlap cases if you allow multiple address spaces. The rom_insert() code orders roms in this list purely by load address, so you could get cases like
[AS A, 0..0x1000], [AS B, 0..0x1000], [AS A, 0x500..0x1000]
where entries 1 and 3 overlap but won't get caught. You probably need to order the list by AS first and then by address, and then adjust this code accordingly.

Quoted 53 lines

hw/core/loader.c +0 -0

     qemu_register_reset(rom_reset, NULL);
     roms_loaded = 1;

include/hw/elf_ops.h +1 -1

index db70c11..1339677 100644
@@ -405,7 +405,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
             snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
 
             /* rom_add_elf_program() seize the ownership of 'data' */
-            rom_add_elf_program(label, data, file_size, mem_size, addr);
+            rom_add_elf_program(label, data, file_size, mem_size, addr, NULL);
 
             total_size += mem_size;
             if (addr < low)

include/hw/loader.h +6 -5

index 4879b63..18eb0f2 100644
@@ -118,14 +118,14 @@ extern bool rom_file_has_mr;
 
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex,
-                 bool option_rom, MemoryRegion *mr);
+                 bool option_rom, MemoryRegion *mr, AddressSpace *as);
 MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
                            size_t max_len, hwaddr addr,
                            const char *fw_file_name,
                            FWCfgReadCallback fw_callback,
                            void *callback_opaque);
 int rom_add_elf_program(const char *name, void *data, size_t datasize,
-                        size_t romsize, hwaddr addr);
+                        size_t romsize, hwaddr addr, AddressSpace *as);
 int rom_check_and_register_reset(void);
 void rom_set_fw(FWCfgState *f);
 void rom_set_order_override(int order);

@@ -135,11 +135,13 @@ void *rom_ptr(hwaddr addr);
 void hmp_info_roms(Monitor *mon, const QDict *qdict);
 
 #define rom_add_file_fixed(_f, _a, _i)          \
-    rom_add_file(_f, NULL, _a, _i, false, NULL)
+    rom_add_file(_f, NULL, _a, _i, false, NULL, NULL)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL)
 #define rom_add_file_mr(_f, _mr, _i)            \
-    rom_add_file(_f, NULL, 0, _i, false, _mr)
+    rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
+#define rom_add_file_as(_f, _as, _i)            \
+    rom_add_file(_f, NULL, 0, _i, false, NULL, _as)
 
 #define PC_ROM_MIN_VGA     0xc0000
 #define PC_ROM_MIN_OPTION  0xc8000
--

Quoted 2 lines

2.7.4

thanks
-- PMM

Alistair Francis

author

2016-07-13 17:15 UTC #6 lore raw

Re: [Qemu-devel] [PATCH v8 2/5] loader: All a custom SddressSpace when loading ROMs

Quoted 28 lines · Peter Maydell

Quoted 27 lines · Alistair Francis
When loading ROMs allow the caller to specify an AddressSpace to use for the load.
Signed-off-by Alistair Francis <[email protected]>
---
V8:
- Introduce an RFC version of AddressSpace loading support
 hw/core/loader.c     | 18 ++++++++++++------
 include/hw/elf_ops.h |  2 +-
 include/hw/loader.h  | 10 ++++++----
 3 files changed, 19 insertions(+), 11 deletions(-)
hw/core/loader.c +1 -0
index 53e0e41..fcbcfbf 100644
@@ -777,6 +777,7 @@ struct Rom {
 
     uint8_t *data;
     MemoryRegion *mr;
+    AddressSpace *as;
     int isrom;
     char *fw_dir;
     char *fw_file;
@@ -833,7 +834,8 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
 

Quoted 7 lines

hw/core/loader.c static void *rom_set_mr(Rom *rom, Object *owner, const char *name) · line 838

Quoted 5 lines

hw/core/loader.c +2 -1

 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex,
-                 bool option_rom, MemoryRegion *mr)
+                 bool option_rom, MemoryRegion *mr,
+                 AddressSpace *as)

We seem to add this argument to the function but never use it?

I have fixed that.

Quoted 2 lines

I think specifying both mr and as should be an error.

Agreed, it is now an error.

Quoted 51 lines

hw/core/loader.c +7 -4

 {
     MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
     Rom *rom;

@@ -969,7 +971,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
  * memory ownership of "data", so we don't have to allocate and copy the buffer.
  */
 int rom_add_elf_program(const char *name, void *data, size_t datasize,
-                        size_t romsize, hwaddr addr)
+                        size_t romsize, hwaddr addr, AddressSpace *as)
 {
     Rom *rom;

@@ -979,18 +981,19 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize,
     rom->datasize = datasize;
     rom->romsize  = romsize;
     rom->data     = data;
+    rom->as       = as;
     rom_insert(rom);
     return 0;
 }
 
 int rom_add_vga(const char *file)
 {
-    return rom_add_file(file, "vgaroms", 0, -1, true, NULL);
+    return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL);
 }
 
 int rom_add_option(const char *file, int32_t bootindex)
 {
-    return rom_add_file(file, "genroms", 0, bootindex, true, NULL);
+    return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL);
 }
 
 static void rom_reset(void *unused)

@@ -1008,7 +1011,8 @@ static void rom_reset(void *unused)
             void *host = memory_region_get_ram_ptr(rom->mr);
             memcpy(host, rom->data, rom->datasize);
         } else {
-            cpu_physical_memory_write_rom(&address_space_memory,
+            cpu_physical_memory_write_rom(rom->as ? rom->as :
+                                                    &address_space_memory,
                                           rom->addr, rom->data, rom->datasize);
         }
         if (rom->isrom) {

@@ -1031,12 +1035,13 @@ int rom_check_and_register_reset(void)
     hwaddr addr = 0;
     MemoryRegionSection section;
     Rom *rom;
+    AddressSpace *as = NULL;

Quoted 15 lines

hw/core/loader.c int rom_check_and_register_reset(void) · line 1053

Quoted 11 lines

hw/core/loader.c +1 -1

     QTAILQ_FOREACH(rom, &roms, next) {
         if (rom->fw_file) {
             continue;
         }
-        if (addr > rom->addr) {
+        if ((addr > rom->addr) && (as == rom->as)) {
             fprintf(stderr, "rom: requested regions overlap "
                     "(rom %s. free=0x" TARGET_FMT_plx
                     ", addr=0x" TARGET_FMT_plx ")\n",

@@ -1048,6 +1053,7 @@ int rom_check_and_register_reset(void)
         section = memory_region_find(get_system_memory(), rom->addr, 1);

(An unrelated bug but I've just noticed that this code doesn't make sense for ROMs which go into a memory region rather than at an address in the system address space.)

I'll add a patch to fix this as well.

Quoted 6 lines

hw/core/loader.c int rom_check_and_register_reset(void) · line 1057
Quoted 5 lines
hw/core/loader.c +1 -0
         rom->isrom = int128_nz(section.size) && memory_region_is_rom(section.mr);
         memory_region_unref(section.mr);
+        as = rom->as;
     }

Quoted 8 lines

I don't think this attempt at checking is going to actually catch all the overlap cases if you allow multiple address spaces. The rom_insert() code orders roms in this list purely by load address, so you could get cases like
[AS A, 0..0x1000], [AS B, 0..0x1000], [AS A, 0x500..0x1000]
where entries 1 and 3 overlap but won't get caught. You probably need to order the list by AS first and then by address, and then adjust this code accordingly.

I have changed the ordering now to be by AddressSpace and load address.

Thanks,

Alistair

Quoted 60 lines

Quoted 54 lines

hw/core/loader.c +0 -0

     qemu_register_reset(rom_reset, NULL);
     roms_loaded = 1;

include/hw/elf_ops.h +1 -1

index db70c11..1339677 100644
@@ -405,7 +405,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
             snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
 
             /* rom_add_elf_program() seize the ownership of 'data' */
-            rom_add_elf_program(label, data, file_size, mem_size, addr);
+            rom_add_elf_program(label, data, file_size, mem_size, addr, NULL);
 
             total_size += mem_size;
             if (addr < low)

include/hw/loader.h +6 -5

index 4879b63..18eb0f2 100644
@@ -118,14 +118,14 @@ extern bool rom_file_has_mr;
 
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex,
-                 bool option_rom, MemoryRegion *mr);
+                 bool option_rom, MemoryRegion *mr, AddressSpace *as);
 MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
                            size_t max_len, hwaddr addr,
                            const char *fw_file_name,
                            FWCfgReadCallback fw_callback,
                            void *callback_opaque);
 int rom_add_elf_program(const char *name, void *data, size_t datasize,
-                        size_t romsize, hwaddr addr);
+                        size_t romsize, hwaddr addr, AddressSpace *as);
 int rom_check_and_register_reset(void);
 void rom_set_fw(FWCfgState *f);
 void rom_set_order_override(int order);

@@ -135,11 +135,13 @@ void *rom_ptr(hwaddr addr);
 void hmp_info_roms(Monitor *mon, const QDict *qdict);
 
 #define rom_add_file_fixed(_f, _a, _i)          \
-    rom_add_file(_f, NULL, _a, _i, false, NULL)
+    rom_add_file(_f, NULL, _a, _i, false, NULL, NULL)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL)
 #define rom_add_file_mr(_f, _mr, _i)            \
-    rom_add_file(_f, NULL, 0, _i, false, _mr)
+    rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
+#define rom_add_file_as(_f, _as, _i)            \
+    rom_add_file(_f, NULL, 0, _i, false, NULL, _as)
 
 #define PC_ROM_MIN_VGA     0xc0000
 #define PC_ROM_MIN_OPTION  0xc8000
--

Quoted 2 lines

2.7.4

thanks
-- PMM

Peter Maydell

2016-07-12 16:58 UTC #7 lore raw

Re: [Qemu-devel] [PATCH v8 2/5] loader: All a custom SddressSpace when loading ROMs

In reply to Alistair Francis

Quoted 4 lines · Alistair Francis

When loading ROMs allow the caller to specify an AddressSpace to use for the load.
Signed-off-by Alistair Francis <[email protected]>

Forgot to mention -- there are some typos in the subject.

thanks
-- PMM

Alistair Francis

author

2016-07-02 01:07 UTC #8 lore raw

PATCH v8 3/5 loader: Add AddressSpace loading support to ELFs

In reply to Alistair Francis

Add a new function load_elf_as() that allows the caller to specify an AddressSpace to use when loading the ELF. The original load_elf() function doesn't have any change in functionality.

Signed-off-by Alistair Francis <[email protected]>

---
V8:
- Introduce an RFC version of AddressSpace support

 hw/core/loader.c     | 16 ++++++++++++++--
 include/hw/elf_ops.h |  5 +++--
 include/hw/loader.h  | 14 +++++++++++++-
 3 files changed, 30 insertions(+), 5 deletions(-)

hw/core/loader.c +14 -2

index fcbcfbf..9b25dfc 100644
@@ -417,6 +417,18 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
              uint64_t *highaddr, int big_endian, int elf_machine,
              int clear_lsb, int data_swab)
 {
+    return load_elf_as(filename, translate_fn, translate_opaque, pentry,
+                       lowaddr, highaddr, big_endian, elf_machine, clear_lsb,
+                       data_swab, NULL);
+}
+
+/* return < 0 if error, otherwise the number of bytes loaded in memory */
+int load_elf_as(const char *filename,
+                uint64_t (*translate_fn)(void *, uint64_t),
+                void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
+                uint64_t *highaddr, int big_endian, int elf_machine,
+                int clear_lsb, int data_swab, AddressSpace *as)
+{
     int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
     uint8_t e_ident[EI_NIDENT];

@@ -455,11 +467,11 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
     if (e_ident[EI_CLASS] == ELFCLASS64) {
         ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab,
                          pentry, lowaddr, highaddr, elf_machine, clear_lsb,
-                         data_swab);
+                         data_swab, as);
     } else {
         ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab,
                          pentry, lowaddr, highaddr, elf_machine, clear_lsb,
-                         data_swab);
+                         data_swab, as);
     }
 
  fail:

include/hw/elf_ops.h +3 -2

index 1339677..3b8c9e9 100644
@@ -263,7 +263,8 @@ static int glue(load_elf, SZ)(const char *name, int fd,
                               void *translate_opaque,
                               int must_swab, uint64_t *pentry,
                               uint64_t *lowaddr, uint64_t *highaddr,
-                              int elf_machine, int clear_lsb, int data_swab)
+                              int elf_machine, int clear_lsb, int data_swab,
+                              AddressSpace *as)
 {
     struct elfhdr ehdr;
     struct elf_phdr *phdr = NULL, *ph;

@@ -405,7 +406,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
             snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
 
             /* rom_add_elf_program() seize the ownership of 'data' */
-            rom_add_elf_program(label, data, file_size, mem_size, addr, NULL);
+            rom_add_elf_program(label, data, file_size, mem_size, addr, as);
 
             total_size += mem_size;
             if (addr < low)

include/hw/loader.h +13 -1

index 18eb0f2..d14eab1 100644
@@ -45,7 +45,7 @@ int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
 #define ELF_LOAD_WRONG_ENDIAN -4
 const char *load_elf_strerror(int error);
 
-/** load_elf:
+/** load_elf_as:
  * @filename: Path of ELF file
  * @translate_fn: optional function to translate load addresses
  * @translate_opaque: opaque data passed to @translate_fn

@@ -59,6 +59,8 @@ const char *load_elf_strerror(int error);
  * @data_swab: Set to order of byte swapping for data. 0 for no swap, 1
  *             for swapping bytes within halfwords, 2 for bytes within
  *             words and 3 for within doublewords.
+ * @as: The AddressSpace to load the ELF to. The value of address_space_memory
+ *      is used if nothing is supplied here.
  *
  * Load an ELF file's contents to the emulated system's address space.
  * Clients may optionally specify a callback to perform address

@@ -70,6 +72,16 @@ const char *load_elf_strerror(int error);
  * their particular values for @elf_machine are set.
  */
 
+int load_elf_as(const char *filename,
+                uint64_t (*translate_fn)(void *, uint64_t),
+                void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
+                uint64_t *highaddr, int big_endian, int elf_machine,
+                int clear_lsb, int data_swab, AddressSpace *as);
+
+/** load_elf:
+ * Same as above, but doesn't allow the caller to specify an AddressSpace
+ */
+
 int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
              void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
              uint64_t *highaddr, int big_endian, int elf_machine,

Signature · 1 line

2.7.4

Alistair Francis

author

2016-07-02 01:07 UTC #9 lore raw

PATCH v8 4/5 generic-loader: Add a generic loader

In reply to Alistair Francis

Add a generic loader to QEMU which can be used to load images or set memory values.

Signed-off-by Alistair Francis <[email protected]>

---
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

MAINTAINERS +6 -0

index 2ab6e3b..0077e22 100644
@@ -992,6 +992,12 @@ M: Dmitry Fleytman <[email protected]>
 S: Maintained
 F: hw/net/e1000e*
 
+Generic Loader
+M: Alistair Francis <[email protected]>
+S: Maintained
+F: hw/core/generic-loader.c
+F: include/hw/core/generic-loader.h
+
 Subsystems
 ----------
 Audio

hw/core/Makefile.objs +2 -0

index 82a9ef8..ab238fa 100644
@@ -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

hw/core/generic-loader.c new file +177 -0

177 lines not on this page · raw message

include/hw/core/generic-loader.h new file +45 -0

45 lines not on this page · raw message

Signature · 1 line

2.7.4

Peter Maydell

2016-07-12 16:40 UTC #10 lore raw

Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader

Quoted 88 lines · Alistair Francis

Add a generic loader to QEMU which can be used to load images or set memory values.

Signed-off-by Alistair Francis <[email protected]>

 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

MAINTAINERS +6 -0

index 2ab6e3b..0077e22 100644
@@ -992,6 +992,12 @@ M: Dmitry Fleytman <[email protected]>
 S: Maintained
 F: hw/net/e1000e*
 
+Generic Loader
+M: Alistair Francis <[email protected]>
+S: Maintained
+F: hw/core/generic-loader.c
+F: include/hw/core/generic-loader.h
+
 Subsystems
 ----------
 Audio

hw/core/Makefile.objs +2 -0

index 82a9ef8..ab238fa 100644
@@ -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

hw/core/generic-loader.c new file +17 -0

new file mode 100644
index 0000000..c9b0572
@@ -0,0 +1,177 @@
+/*
+ * 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.
+ */

hw/core/generic-loader.c line 27

Quoted 10 lines

hw/core/generic-loader.c new file +10 -0

+
+#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.

hw/core/generic-loader.c line 37

Quoted 10 lines

hw/core/generic-loader.c new file +10 -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);
+        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 ?

hw/core/generic-loader.c line 44

Quoted 7 lines

hw/core/generic-loader.c new file +7 -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);

We rule out specifying cpu_num below, so when is s->cpu non-NULL?

Quoted 9 lines

hw/core/generic-loader.c new file +9 -0

+    }
+}
+
+static void generic_loader_realize(DeviceState *dev, Error **errp)
+{
+    GenericLoaderState *s = GENERIC_LOADER(dev);
+    hwaddr entry;
+    int big_endian;
+    int size = 0;

hw/core/generic-loader.c line 63

Quoted 10 lines

hw/core/generic-loader.c new file +10 -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.

Quoted 9 lines

hw/core/generic-loader.c new file +9 -0

+                       "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;
+        }

hw/core/generic-loader.c line 82

Quoted 10 lines

hw/core/generic-loader.c new file +10 -0

+    } 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"

hw/core/generic-loader.c line 90

Quoted 8 lines

hw/core/generic-loader.c new file +8 -0

+            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 ?

Quoted 23 lines

hw/core/generic-loader.c new file +23 -0

+
+    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) {

hw/core/generic-loader.c line 123

Quoted 10 lines

hw/core/generic-loader.c new file +10 -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

Alistair Francis

author

2016-07-13 17:45 UTC #11 lore raw

Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader

Quoted 93 lines · Peter Maydell

Quoted 92 lines · Alistair Francis

Add a generic loader to QEMU which can be used to load images or set memory values.

Signed-off-by Alistair Francis <[email protected]>

 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

MAINTAINERS +6 -0

index 2ab6e3b..0077e22 100644
@@ -992,6 +992,12 @@ M: Dmitry Fleytman <[email protected]>
 S: Maintained
 F: hw/net/e1000e*
 
+Generic Loader
+M: Alistair Francis <[email protected]>
+S: Maintained
+F: hw/core/generic-loader.c
+F: include/hw/core/generic-loader.h
+
 Subsystems
 ----------
 Audio

hw/core/Makefile.objs +2 -0

index 82a9ef8..ab238fa 100644
@@ -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

hw/core/generic-loader.c new file +21 -0

new file mode 100644
index 0000000..c9b0572
@@ -0,0 +1,177 @@
+/*
+ * 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 "qom/cpu.h"
+#include "hw/sysbus.h"

Quoted 8 lines

hw/core/generic-loader.c line 27
Quoted 6 lines
hw/core/generic-loader.c new file +6 -0
+#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

Quoted 10 lines

hw/core/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);
+        if (cc) {

Quoted 8 lines

hw/core/generic-loader.c line 37
Quoted 1 line
hw/core/generic-loader.c new file +1 -0
+            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?

Quoted 10 lines

hw/core/generic-loader.c line 44
Quoted 8 lines
hw/core/generic-loader.c new file +7 -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);
We rule out specifying cpu_num below, so when is s->cpu non-NULL?

I have removed this.

Quoted 14 lines

hw/core/generic-loader.c new file +13 -0

+    }
+}
+
+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 */

Quoted 8 lines

hw/core/generic-loader.c line 63

Quoted 6 lines

hw/core/generic-loader.c new file +6 -0

+        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

Quoted 21 lines

Quoted 10 lines

hw/core/generic-loader.c new file +9 -0

+                       "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;
+        }

hw/core/generic-loader.c line 82

Quoted 10 lines

hw/core/generic-loader.c new file +10 -0

+    } 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");

Quoted 1 line

"greater"

Fixed

Quoted 11 lines

hw/core/generic-loader.c line 90

Quoted 9 lines

hw/core/generic-loader.c new file +8 -0

+            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.

Quoted 30 lines

hw/core/generic-loader.c new file +29 -0

+
+    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 */

Quoted 8 lines

hw/core/generic-loader.c line 123
Quoted 4 lines
hw/core/generic-loader.c new file +4 -0
+            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.

Quoted 4 lines

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

Quoted 4 lines

thanks
-- PMM

Peter Maydell

2016-07-13 19:44 UTC #12 lore raw

Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader

Quoted 19 lines · Alistair Francis

Quoted 11 lines · Peter Maydell
hw/core/generic-loader.c line 35
Quoted 9 lines · Alistair Francis
Add a generic loader to QEMU which can be used to load images or set memory values.
Signed-off-by Alistair Francis <[email protected]>
---
hw/core/generic-loader.c new file +3 -0
+    if (s->cpu) {
+        CPUClass *cc = CPU_GET_CLASS(s->cpu);
+        cpu_reset(s->cpu);
Quoted 6 lines
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 ?

Quoted 2 lines

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.)

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
+ 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?

Quoted 5 lines

Quoted 3 lines
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

Alistair Francis

author

2016-07-13 20:31 UTC #13 lore raw

Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader

Quoted 20 lines · Peter Maydell

Quoted 19 lines · Alistair Francis
Quoted 17 lines · Peter Maydell
hw/core/generic-loader.c line 35
Quoted 9 lines · Alistair Francis
Add a generic loader to QEMU which can be used to load images or set memory values.
Signed-off-by Alistair Francis <[email protected]>
---
hw/core/generic-loader.c new file +3 -0
+    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 ?

Quoted 8 lines

Quoted 2 lines
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?

Quoted 10 lines

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
+ 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.

Quoted 7 lines

Quoted 6 lines
Quoted 4 lines
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.

Quoted 9 lines

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

Quoted 4 lines

thanks
-- PMM

Peter Maydell

2016-07-13 21:09 UTC #14 lore raw

Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader

Quoted 20 lines · Alistair Francis

Quoted 5 lines · Peter Maydell
Quoted 4 lines · Alistair Francis
Quoted 3 lines · Peter Maydell
Quoted 1 line · Alistair Francis
+ qemu_register_reset(generic_loader_reset, dev);
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.

Quoted 6 lines

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

Alistair Francis

author

2016-07-13 22:18 UTC #15 lore raw

Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader

Quoted 28 lines · Peter Maydell

Quoted 20 lines · Alistair Francis
Quoted 5 lines · Peter Maydell
Quoted 4 lines · Alistair Francis
Quoted 3 lines · Peter Maydell
Quoted 1 line · Alistair Francis
+ qemu_register_reset(generic_loader_reset, dev);
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 6 lines
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.

Quoted 4 lines

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.

Quoted 5 lines

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

Quoted 4 lines

thanks
-- PMM

Alistair Francis

author

2016-07-02 01:07 UTC #16 lore raw

PATCH v8 5/5 docs: Add a generic loader explanation document

In reply to Alistair Francis

Signed-off-by Alistair Francis <[email protected]>

---
V8:
- Improve documentation
V6:
- Fixup documentation
V4:
- Re-write to be more comprehensive

 docs/generic-loader.txt | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 docs/generic-loader.txt

docs/generic-loader.txt new file +60 -0

new file mode 100644
index 0000000..34684fc
@@ -0,0 +1,60 @@
+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.
+
+
+The 'loader' device allows the user to load multiple images or values into
+QEMU at startup.
+
+Loading Memory Values
+---------------------
+The loader device allows memory values to be set from the command line. This
+can be done by following the syntax below:
+
+    -device loader,addr=<addr>,data=<data>,data-len=<len>
+    -device loader,addr=<addr>,cpu-num=<cpu-num>
+
+    <addr>      - The address to store the data or the value to use as the
+                  CPU's PC.
+    <data>      - The value to be written to the address. The maximum size of
+                  the data is 8 bytes.
+    <data-len>  - The length of the data in bytes. This argument must be
+                  included if the data argument is.
+    <data-be>   - Set to true if the data to be stored on the guest should be
+                  written as big endian data. The default is to write little
+                  endian data.
+    <cpu-num>   - This will cause the CPU to be reset and the PC to be set to
+                  the value of addr.
+
+For all values both hex and decimal values are allowed. By default the values
+will be parsed as decimal. To use hex values the user should prefix the number
+with a '0x'.
+
+An example of loading value 0x8000000e to address 0xfd1a0104 is:
+    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
+
+Loading Files
+-------------
+The loader device also allows files to be loaded into memory. This can be done
+similarly to setting memory values. The syntax is shown below:
+
+    -device loader,file=<file>,addr=<addr>,cpu-num=<cpu-num>,force-raw=<raw>
+
+    <file>      - A file to be loaded into memory
+    <addr>      - The addr in memory that the file should be loaded. This is
+                  ignored if you are using an ELF (unless force-raw is true).
+                  This is required if you aren't loading an ELF.
+    <cpu-num>   - This specifies the CPU that should be used. This is an
+                  optional argument and will cause the CPU's PC to be set to
+                  where the image is stored. This option should only be used
+                  for the boot image.
+    <force-raw> - Forces the file to be treated as a raw image. This can be
+                  used to specify the load address of ELF files.
+
+For all values both hex and decimal values are allowed. By default the values
+will be parsed as decimal. To use hex values the user should prefix the number
+with a '0x'.
+
+An example of loading an ELF file which CPU0 will boot is shown below:
+    -device loader,file=./images/boot.elf,cpu-num=0

Signature · 1 line

2.7.4

Peter Maydell

2016-07-12 16:31 UTC #17 lore raw

Re: [Qemu-devel] [PATCH v8 5/5] docs: Add a generic loader explanation document

Quoted 60 lines · Alistair Francis

Signed-off-by Alistair Francis <[email protected]>

---
V8:
- Improve documentation
V6:
- Fixup documentation
V4:
- Re-write to be more comprehensive

 docs/generic-loader.txt | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 docs/generic-loader.txt

docs/generic-loader.txt new file +41 -0

new file mode 100644
index 0000000..34684fc
@@ -0,0 +1,60 @@
+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.
+
+
+The 'loader' device allows the user to load multiple images or values into
+QEMU at startup.
+
+Loading Memory Values
+---------------------
+The loader device allows memory values to be set from the command line. This
+can be done by following the syntax below:
+
+    -device loader,addr=<addr>,data=<data>,data-len=<len>
+    -device loader,addr=<addr>,cpu-num=<cpu-num>
+
+    <addr>      - The address to store the data or the value to use as the
+                  CPU's PC.
+    <data>      - The value to be written to the address. The maximum size of
+                  the data is 8 bytes.
+    <data-len>  - The length of the data in bytes. This argument must be
+                  included if the data argument is.
+    <data-be>   - Set to true if the data to be stored on the guest should be
+                  written as big endian data. The default is to write little
+                  endian data.
+    <cpu-num>   - This will cause the CPU to be reset and the PC to be set to
+                  the value of addr.
+
+For all values both hex and decimal values are allowed. By default the values
+will be parsed as decimal. To use hex values the user should prefix the number
+with a '0x'.
+
+An example of loading value 0x8000000e to address 0xfd1a0104 is:
+    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
+
+Loading Files
+-------------
+The loader device also allows files to be loaded into memory. This can be done
+similarly to setting memory values. The syntax is shown below:
+

docs/generic-loader.txt line 51

Quoted 10 lines

docs/generic-loader.txt new file +10 -0

+    -device loader,file=<file>,addr=<addr>,cpu-num=<cpu-num>,force-raw=<raw>
+
+    <file>      - A file to be loaded into memory
+    <addr>      - The addr in memory that the file should be loaded. This is
+                  ignored if you are using an ELF (unless force-raw is true).
+                  This is required if you aren't loading an ELF.
+    <cpu-num>   - This specifies the CPU that should be used. This is an
+                  optional argument and will cause the CPU's PC to be set to
+                  where the image is stored. This option should only be used
+                  for the boot image.

For an ELF file we use the start address specified in the ELF header, right?

We also end up writing to that CPU's address space, but only for ELF files.

docs/generic-loader.txt line 53

Quoted 2 lines

docs/generic-loader.txt new file +2 -0

+    <force-raw> - Forces the file to be treated as a raw image. This can be
+                  used to specify the load address of ELF files.

Presumably it results in the ELF file being dumped into memory as a raw image without any regard to what the segment headers say about loading, offsets, etc?

docs/generic-loader.txt line 60

Quoted 7 lines

docs/generic-loader.txt new file +7 -0

+
+For all values both hex and decimal values are allowed. By default the values
+will be parsed as decimal. To use hex values the user should prefix the number
+with a '0x'.
+
+An example of loading an ELF file which CPU0 will boot is shown below:
+    -device loader,file=./images/boot.elf,cpu-num=0

thanks
-- PMM

Alistair Francis

author

2016-07-13 17:49 UTC #18 lore raw

Re: [Qemu-devel] [PATCH v8 5/5] docs: Add a generic loader explanation document

Quoted 65 lines · Peter Maydell

Quoted 64 lines · Alistair Francis

Signed-off-by Alistair Francis <[email protected]>

---
V8:
- Improve documentation
V6:
- Fixup documentation
V4:
- Re-write to be more comprehensive

 docs/generic-loader.txt | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 docs/generic-loader.txt

docs/generic-loader.txt new file +45 -0

new file mode 100644
index 0000000..34684fc
@@ -0,0 +1,60 @@
+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.
+
+
+The 'loader' device allows the user to load multiple images or values into
+QEMU at startup.
+
+Loading Memory Values
+---------------------
+The loader device allows memory values to be set from the command line. This
+can be done by following the syntax below:
+
+    -device loader,addr=<addr>,data=<data>,data-len=<len>
+    -device loader,addr=<addr>,cpu-num=<cpu-num>
+
+    <addr>      - The address to store the data or the value to use as the
+                  CPU's PC.
+    <data>      - The value to be written to the address. The maximum size of
+                  the data is 8 bytes.
+    <data-len>  - The length of the data in bytes. This argument must be
+                  included if the data argument is.
+    <data-be>   - Set to true if the data to be stored on the guest should be
+                  written as big endian data. The default is to write little
+                  endian data.
+    <cpu-num>   - This will cause the CPU to be reset and the PC to be set to
+                  the value of addr.
+
+For all values both hex and decimal values are allowed. By default the values
+will be parsed as decimal. To use hex values the user should prefix the number
+with a '0x'.
+
+An example of loading value 0x8000000e to address 0xfd1a0104 is:
+    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
+
+Loading Files
+-------------
+The loader device also allows files to be loaded into memory. This can be done
+similarly to setting memory values. The syntax is shown below:
+
+    -device loader,file=<file>,addr=<addr>,cpu-num=<cpu-num>,force-raw=<raw>
+
+    <file>      - A file to be loaded into memory
+    <addr>      - The addr in memory that the file should be loaded. This is

Quoted 8 lines

docs/generic-loader.txt line 51

Quoted 6 lines

docs/generic-loader.txt new file +6 -0

+                  ignored if you are using an ELF (unless force-raw is true).
+                  This is required if you aren't loading an ELF.
+    <cpu-num>   - This specifies the CPU that should be used. This is an
+                  optional argument and will cause the CPU's PC to be set to
+                  where the image is stored. This option should only be used
+                  for the boot image.

For an ELF file we use the start address specified in the ELF header, right?

Yes, I have specified that now in the docs.

Quoted 2 lines

We also end up writing to that CPU's address space, but only for ELF files.

Yes, this will apply to all images in the next version.

Quoted 7 lines

docs/generic-loader.txt line 53
Quoted 3 lines
docs/generic-loader.txt new file +2 -0
+    <force-raw> - Forces the file to be treated as a raw image. This can be
+                  used to specify the load address of ELF files.
Presumably it results in the ELF file being dumped into memory as a raw image without any regard to what the segment headers say about loading, offsets, etc?

Correct, I have made that more clear.

Thanks,

Alistair

Quoted 12 lines

docs/generic-loader.txt line 60

Quoted 8 lines

docs/generic-loader.txt new file +7 -0

+
+For all values both hex and decimal values are allowed. By default the values
+will be parsed as decimal. To use hex values the user should prefix the number
+with a '0x'.
+
+An example of loading an ELF file which CPU0 will boot is shown below:
+    -device loader,file=./images/boot.elf,cpu-num=0

thanks
-- PMM

Peter Maydell

2016-07-04 12:22 UTC #19 lore raw

In reply to Alistair Francis

Quoted 15 lines · Alistair Francis

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.

Quoted 7 lines

This patch series makes the load_elf() function more generic by not requiring an architecture. It also adds a new function load_elf_as() which allows custom AddressSpaces when loading ELF images.
At the moment I think the AddressSpace loading support is more of an RFC. If people agree with the way I'm doing it I will expand the support to image types.

"an RFC" at this stage of the release process is a strong hint towards "not for this release". Are you sure about that?

thanks
-- PMM

Alistair Francis

author

2016-07-05 17:45 UTC #20 lore raw

Quoted 20 lines · Peter Maydell

Quoted 19 lines · Alistair Francis
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.
This patch series makes the load_elf() function more generic by not requiring an architecture. It also adds a new function load_elf_as() which allows custom AddressSpaces when loading ELF images.

Quoted 6 lines

Quoted 3 lines
At the moment I think the AddressSpace loading support is more of an RFC. If people agree with the way I'm doing it I will expand the support to image types.
"an RFC" at this stage of the release process is a strong hint towards "not for this release". Are you sure about that?

I mostly meant it because at the moment it only supports one image type. I will need to expand it to the other images.

I wanted to see if this was considered the right approach before I did that.

Thanks,

Alistair

Quoted 4 lines

thanks
-- PMM

Christian Borntraeger

2016-07-04 12:44 UTC #21 lore raw

In reply to Alistair Francis

Quoted 15 lines · Alistair Francis

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.

Quoted 7 lines

This patch series makes the load_elf() function more generic by not requiring an architecture. It also adds a new function load_elf_as() which allows custom AddressSpaces when loading ELF images.
At the moment I think the AddressSpace loading support is more of an RFC. If people agree with the way I'm doing it I will expand the support to image types.

This looks valuable for s390 as well.
We have "*.ins" files that describe a list of files and its load address. It is used to move files from the DVD in the hardware management console to LPAR memory to bootstrap an empty lpar.

With this libvirt could parse these ins files and emulate this functionality.

Quoted 51 lines

V8:
- Allow custom AddressSpaces when loading images
- Move ELF architecture handling code
- Rebase
- Corrections to loading code
- Corrections to documentation
V7:
- Fix typo in comment
- Rebase
V6:
- Add error checking
V5:
- Rebase
V4:
- Re-write documentation
- 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
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 (5):
loader: Allow ELF loader to auto-detect the ELF arch
loader: All a custom SddressSpace when loading ROMs
loader: Add AddressSpace loading support to ELFs
generic-loader: Add a generic loader
docs: Add a generic loader explanation document
 MAINTAINERS                      |   6 ++
 docs/generic-loader.txt          |  60 +++++++++++++
 hw/core/Makefile.objs            |   2 +
 hw/core/generic-loader.c         | 177 +++++++++++++++++++++++++++++++++++++++
 hw/core/loader.c                 |  34 ++++++--
 include/hw/core/generic-loader.h |  45 ++++++++++
 include/hw/elf_ops.h             |  10 ++-
 include/hw/loader.h              |  24 ++++--
 8 files changed, 343 insertions(+), 15 deletions(-)
 create mode 100644 docs/generic-loader.txt
 create mode 100644 hw/core/generic-loader.c
 create mode 100644 include/hw/core/generic-loader.h

Alistair Francis

author

2016-07-05 17:45 UTC #22 lore raw

Quoted 25 lines · Christian Borntraeger

Quoted 15 lines · Alistair Francis
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.
Quoted 7 lines
This patch series makes the load_elf() function more generic by not requiring an architecture. It also adds a new function load_elf_as() which allows custom AddressSpaces when loading ELF images.
At the moment I think the AddressSpace loading support is more of an RFC. If people agree with the way I'm doing it I will expand the support to image types.

Quoted 6 lines

This looks valuable for s390 as well.
We have "*.ins" files that describe a list of files and its load address. It is used to move files from the DVD in the hardware management console to LPAR memory to bootstrap an empty lpar.
With this libvirt could parse these ins files and emulate this functionality.

Great! I'm glad it is useful for others.

Thanks,

Alistair

Quoted 55 lines

Quoted 53 lines
V8:
- Allow custom AddressSpaces when loading images
- Move ELF architecture handling code
- Rebase
- Corrections to loading code
- Corrections to documentation
V7:
- Fix typo in comment
- Rebase
V6:
- Add error checking
V5:
- Rebase
V4:
- Re-write documentation
- 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
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 (5):
loader: Allow ELF loader to auto-detect the ELF arch
loader: All a custom SddressSpace when loading ROMs
loader: Add AddressSpace loading support to ELFs
generic-loader: Add a generic loader
docs: Add a generic loader explanation document
 MAINTAINERS                      |   6 ++
 docs/generic-loader.txt          |  60 +++++++++++++
 hw/core/Makefile.objs            |   2 +
 hw/core/generic-loader.c         | 177 +++++++++++++++++++++++++++++++++++++++
 hw/core/loader.c                 |  34 ++++++--
 include/hw/core/generic-loader.h |  45 ++++++++++
 include/hw/elf_ops.h             |  10 ++-
 include/hw/loader.h              |  24 ++++--
 8 files changed, 343 insertions(+), 15 deletions(-)
 create mode 100644 docs/generic-loader.txt
 create mode 100644 hw/core/generic-loader.c
 create mode 100644 include/hw/core/generic-loader.h