[Qemu-devel] [PATCH v10 0/8] Add a generic loader

[Qemu-devel] [PATCH v10 0/8] Add a generic loader

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

Tested with the latest Xilinx ZynqMP machine, if I enable EL3 and EL2 I can
boot ATF through to u-boot using the loader to load the images.

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 new functions load_elf_as(),
load_uimage_as and load_image_targphys_as which allows custom
AddressSpaces when loading images.

V9:
 - Logic and documentation corrections
 - Add address space loading support for uImages and targphys
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 (8):
  loader: Allow ELF loader to auto-detect the ELF arch
  loader: Use the specified MemoryRegion
  loader: Allow a custom AddressSpace when loading ROMs
  loader: Add AddressSpace loading support to ELFs
  loader: Add AddressSpace loading support to uImages
  loader: Add AddressSpace loading support to targphys
  generic-loader: Add a generic loader
  docs: Add a generic loader explanation document

 MAINTAINERS                      |   6 ++
 docs/generic-loader.txt          |  81 ++++++++++++++++
 hw/core/Makefile.objs            |   2 +
 hw/core/generic-loader.c         | 195 +++++++++++++++++++++++++++++++++++++++
 hw/core/loader.c                 |  98 +++++++++++++++-----
 include/hw/core/generic-loader.h |  46 +++++++++
 include/hw/elf_ops.h             |  10 +-
 include/hw/loader.h              |  73 ++++++++++++++-
 8 files changed, 483 insertions(+), 28 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

-- 
2.7.4

[Qemu-devel] [PATCH v10 1/8] loader: Allow ELF loader to auto-detect the ELF arch

[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 <alistair.francis@xilinx.com>
---
V10:
 - Base checks on the EM_NONE macro
V9:
 - Update documentation
V8:
 - Move into load_elf64/load_elf32
V7:
 - Fix typo

 include/hw/elf_ops.h | 5 +++++
 include/hw/loader.h  | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index f510e7e..5038c7f 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -280,6 +280,11 @@ static int glue(load_elf, SZ)(const char *name, int fd,
         glue(bswap_ehdr, SZ)(&ehdr);
     }
 
+    if (elf_machine <= EM_NONE) {
+        /* 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) {
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 4879b63..c59673d 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -68,6 +68,8 @@ const char *load_elf_strerror(int error);
  * load will fail if the target ELF does not match. Some architectures
  * have some architecture-specific behaviours that come into effect when
  * their particular values for @elf_machine are set.
+ * If @elf_machine is EM_NONE then the machine type will be read from the
+ * ELF header and no checks will be carried out against the machine type.
  */
 
 int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
-- 
2.7.4

[Qemu-devel] [PATCH v10 2/8] loader: Use the specified MemoryRegion

[Alistair Francis]

Prevously the specified MemoryRegion was ignored during the rom register
reset. This patch uses the rom MemoryRegion is avaliable.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

 hw/core/loader.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 53e0e41..6b61f29 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1045,7 +1045,8 @@ int rom_check_and_register_reset(void)
         }
         addr  = rom->addr;
         addr += rom->romsize;
-        section = memory_region_find(get_system_memory(), rom->addr, 1);
+        section = memory_region_find(rom->mr ? rom->mr : get_system_memory(),
+                                     rom->addr, 1);
         rom->isrom = int128_nz(section.size) && memory_region_is_rom(section.mr);
         memory_region_unref(section.mr);
     }
-- 
2.7.4

[Qemu-devel] [PATCH v10 3/8] loader: Allow a custom AddressSpace when loading ROMs

[Alistair Francis]

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

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
V10:
 - Set the rom address space instead of leaving it NULL
 - Cleanup ordering logic
V9:
 - Fixup the ROM ordering
 - Don't allow address space and memory region to be specified
V8:
 - Introduce an RFC version of AddressSpace loading support

 hw/core/loader.c     | 52 ++++++++++++++++++++++++++++++++++++++++------------
 include/hw/elf_ops.h |  2 +-
 include/hw/loader.h  | 10 ++++++----
 3 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 6b61f29..aef7bc9 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -777,6 +777,7 @@ struct Rom {
 
     uint8_t *data;
     MemoryRegion *mr;
+    AddressSpace *as;
     int isrom;
     char *fw_dir;
     char *fw_file;
@@ -788,6 +789,11 @@ struct Rom {
 static FWCfgState *fw_cfg;
 static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
 
+static inline bool rom_order_compare(Rom *rom, Rom *item)
+{
+    return rom->addr >= item->addr;
+}
+
 static void rom_insert(Rom *rom)
 {
     Rom *item;
@@ -796,12 +802,22 @@ static void rom_insert(Rom *rom)
         hw_error ("ROM images must be loaded at startup\n");
     }
 
-    /* list is ordered by load address */
+    /* The user didn't specify an address space, this is the default */
+    if (!rom->as) {
+        rom->as = &address_space_memory;
+    }
+
+    /* List is ordered by load address in the same address space */
     QTAILQ_FOREACH(item, &roms, next) {
-        if (rom->addr >= item->addr)
-            continue;
-        QTAILQ_INSERT_BEFORE(item, rom, next);
-        return;
+        if (rom->as == item->as) {
+            if (rom_order_compare(rom, item)) {
+                QTAILQ_INSERT_AFTER(&roms, item, rom, next);
+                return;
+            } else {
+                QTAILQ_INSERT_BEFORE(item, rom, next);
+                return;
+            }
+        }
     }
     QTAILQ_INSERT_TAIL(&roms, rom, next);
 }
@@ -833,16 +849,25 @@ 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;
     int rc, fd = -1;
     char devpath[100];
 
+    if (as && mr) {
+        fprintf(stderr, "Specifying an Address Space and Memory Region is " \
+                "not valid when loading a rom\n");
+        /* We haven't allocated anything so we don't need any cleanup */
+        return -1;
+    }
+
     rom = g_malloc0(sizeof(*rom));
     rom->name = g_strdup(file);
     rom->path = qemu_find_file(QEMU_FILE_TYPE_BIOS, rom->name);
+    rom->as = as;
     if (rom->path == NULL) {
         rom->path = g_strdup(file);
     }
@@ -969,7 +994,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 +1004,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,8 +1034,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,
-                                          rom->addr, rom->data, rom->datasize);
+            cpu_physical_memory_write_rom(rom->as, rom->addr, rom->data,
+                                          rom->datasize);
         }
         if (rom->isrom) {
             /* rom needs to be written only once */
@@ -1031,12 +1057,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",
@@ -1049,6 +1076,7 @@ int rom_check_and_register_reset(void)
                                      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;
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 5038c7f..4744d11 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -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)
diff --git a/include/hw/loader.h b/include/hw/loader.h
index c59673d..815a8d6 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -120,14 +120,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);
@@ -137,11 +137,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
-- 
2.7.4

[Qemu-devel] [PATCH v10 4/8] loader: Add AddressSpace loading support to ELFs

[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 <alistair.francis@xilinx.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
V8:
 - Introduce an RFC version of AddressSpace support

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

diff --git a/hw/core/loader.c b/hw/core/loader.c
index aef7bc9..0a17faf 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -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:
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 4744d11..25659b9 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -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)
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 815a8d6..fdf0a51 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -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
@@ -71,7 +73,16 @@ const char *load_elf_strerror(int error);
  * If @elf_machine is EM_NONE then the machine type will be read from the
  * ELF header and no checks will be carried out against the machine type.
  */
+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 load_elf_as(), 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,
-- 
2.7.4

[Qemu-devel] [PATCH v10 5/8] loader: Add AddressSpace loading support to uImages

[Alistair Francis]

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

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
V10:
 - Add comment about the function in the header file.

 hw/core/loader.c    | 17 +++++++++++++----
 include/hw/loader.h | 26 ++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 0a17faf..19b73cf 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -581,7 +581,7 @@ static ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src,
 static int load_uboot_image(const char *filename, hwaddr *ep, hwaddr *loadaddr,
                             int *is_linux, uint8_t image_type,
                             uint64_t (*translate_fn)(void *, uint64_t),
-                            void *translate_opaque)
+                            void *translate_opaque, AddressSpace *as)
 {
     int fd;
     int size;
@@ -682,7 +682,7 @@ static int load_uboot_image(const char *filename, hwaddr *ep, hwaddr *loadaddr,
         hdr->ih_size = bytes;
     }
 
-    rom_add_blob_fixed(filename, data, hdr->ih_size, address);
+    rom_add_blob_fixed_as(filename, data, hdr->ih_size, address, as);
 
     ret = hdr->ih_size;
 
@@ -698,14 +698,23 @@ int load_uimage(const char *filename, hwaddr *ep, hwaddr *loadaddr,
                 void *translate_opaque)
 {
     return load_uboot_image(filename, ep, loadaddr, is_linux, IH_TYPE_KERNEL,
-                            translate_fn, translate_opaque);
+                            translate_fn, translate_opaque, NULL);
+}
+
+int load_uimage_as(const char *filename, hwaddr *ep, hwaddr *loadaddr,
+                   int *is_linux,
+                   uint64_t (*translate_fn)(void *, uint64_t),
+                   void *translate_opaque, AddressSpace *as)
+{
+    return load_uboot_image(filename, ep, loadaddr, is_linux, IH_TYPE_KERNEL,
+                            translate_fn, translate_opaque, as);
 }
 
 /* Load a ramdisk.  */
 int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
 {
     return load_uboot_image(filename, NULL, &addr, NULL, IH_TYPE_RAMDISK,
-                            NULL, NULL);
+                            NULL, NULL, NULL);
 }
 
 /* Load a gzip-compressed kernel to a dynamically allocated buffer. */
diff --git a/include/hw/loader.h b/include/hw/loader.h
index fdf0a51..bce8f43 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -102,6 +102,30 @@ void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
 
 int load_aout(const char *filename, hwaddr addr, int max_sz,
               int bswap_needed, hwaddr target_page_size);
+
+/** load_uimage_as:
+ * @filename: Path of uimage file
+ * @ep: Populated with program entry point. Ignored if NULL.
+ * @loadaddr: Populated with the load address. Ignored if NULL.
+ * @is_linux: Is set to true if the image loaded is Linux. Ignored if NULL.
+ * @translate_fn: optional function to translate load addresses
+ * @translate_opaque: opaque data passed to @translate_fn
+ * @as: The AddressSpace to load the ELF to. The value of address_space_memory
+ *      is used if nothing is supplied here.
+ *
+ * Loads a u-boot image into memory.
+ *
+ * Returns the size of the loaded image on success, -1 otherwise.
+ */
+int load_uimage_as(const char *filename, hwaddr *ep,
+                   hwaddr *loadaddr, int *is_linux,
+                   uint64_t (*translate_fn)(void *, uint64_t),
+                   void *translate_opaque, AddressSpace *as);
+
+/** load_uimage:
+ * Same as load_uimage_as(), but doesn't allow the caller to specify an
+ * AddressSpace.
+ */
 int load_uimage(const char *filename, hwaddr *ep,
                 hwaddr *loadaddr, int *is_linux,
                 uint64_t (*translate_fn)(void *, uint64_t),
@@ -155,6 +179,8 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
     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 rom_add_blob_fixed_as(_f, _b, _l, _a, _as)      \
+    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, _as)
 
 #define PC_ROM_MIN_VGA     0xc0000
 #define PC_ROM_MIN_OPTION  0xc8000
-- 
2.7.4

[Qemu-devel] [PATCH v10 6/8] loader: Add AddressSpace loading support to targphys

[Alistair Francis]

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

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
V10:
 - Add comment about the function in the header file.

 hw/core/loader.c    | 10 ++++++++--
 include/hw/loader.h | 22 ++++++++++++++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 19b73cf..e83353a 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -133,10 +133,16 @@ ssize_t read_targphys(const char *name,
     return did;
 }
 
-/* return the size or -1 if error */
 int load_image_targphys(const char *filename,
                         hwaddr addr, uint64_t max_sz)
 {
+    return load_image_targphys_as(filename, addr, max_sz, NULL);
+}
+
+/* return the size or -1 if error */
+int load_image_targphys_as(const char *filename,
+                           hwaddr addr, uint64_t max_sz, AddressSpace *as)
+{
     int size;
 
     size = get_image_size(filename);
@@ -144,7 +150,7 @@ int load_image_targphys(const char *filename,
         return -1;
     }
     if (size > 0) {
-        rom_add_file_fixed(filename, addr, -1);
+        rom_add_file_fixed_as(filename, addr, -1, as);
     }
     return size;
 }
diff --git a/include/hw/loader.h b/include/hw/loader.h
index bce8f43..0381706 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -14,8 +14,28 @@
 int get_image_size(const char *filename);
 int load_image(const char *filename, uint8_t *addr); /* deprecated */
 ssize_t load_image_size(const char *filename, void *addr, size_t size);
+
+/**load_image_targphys_as:
+ * @filename: Path to the image file
+ * @addr: Address to load the image to
+ * @max_sz: The maximum size of the image to load
+ * @as: The AddressSpace to load the ELF to. The value of address_space_memory
+ *      is used if nothing is supplied here.
+ *
+ * Load a fixed image into memory.
+ *
+ * Returns the size of the loaded image on success, -1 otherwise.
+ */
+int load_image_targphys_as(const char *filename,
+                           hwaddr addr, uint64_t max_sz, AddressSpace *as);
+
+/** load_image_targphys:
+ * Same as load_image_targphys_as(), but doesn't allow the caller to specify
+ * an AddressSpace.
+ */
 int load_image_targphys(const char *filename, hwaddr,
                         uint64_t max_sz);
+
 /**
  * load_image_mr: load an image into a memory region
  * @filename: Path to the image file
@@ -179,6 +199,8 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
     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 rom_add_file_fixed_as(_f, _a, _i, _as)          \
+    rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
 #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as)      \
     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, _as)
 
-- 
2.7.4

[Qemu-devel] [PATCH v10 7/8] generic-loader: Add a generic loader

[Alistair Francis]

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

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
V10:
 - Split out the PC setting and data loading
V9:
 - Fix error messages
 - Updated some incorrect logic
 - Add address space loading support for all image types
 - Explain why the reset is manually registered
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         | 195 +++++++++++++++++++++++++++++++++++++++
 include/hw/core/generic-loader.h |  46 +++++++++
 4 files changed, 249 insertions(+)
 create mode 100644 hw/core/generic-loader.c
 create mode 100644 include/hw/core/generic-loader.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 9c88c44..cc34ae0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -984,6 +984,12 @@ M: Dmitry Fleytman <dmitry@daynix.com>
 S: Maintained
 F: hw/net/e1000e*
 
+Generic Loader
+M: Alistair Francis <alistair.francis@xilinx.com>
+S: Maintained
+F: hw/core/generic-loader.c
+F: include/hw/core/generic-loader.h
+
 Subsystems
 ----------
 Audio
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index cfd4840..939c94e 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -17,3 +17,5 @@ common-obj-$(CONFIG_SOFTMMU) += loader.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
 common-obj-$(CONFIG_SOFTMMU) += register.o
 common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
+
+obj-$(CONFIG_SOFTMMU) += generic-loader.o
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
new file mode 100644
index 0000000..4ee71cb
--- /dev/null
+++ b/hw/core/generic-loader.c
@@ -0,0 +1,195 @@
+/*
+ * Generic Loader
+ *
+ * Copyright (C) 2014 Li Guang
+ * Copyright (C) 2016 Xilinx Inc.
+ * Written by Li Guang <lig.fnst@cn.fujitsu.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "qom/cpu.h"
+#include "hw/sysbus.h"
+#include "sysemu/dma.h"
+#include "hw/loader.h"
+#include "qapi/error.h"
+#include "hw/core/generic-loader.h"
+
+#define CPU_NONE 0xFFFFFFFF
+
+static void generic_loader_reset(void *opaque)
+{
+    GenericLoaderState *s = GENERIC_LOADER(opaque);
+
+    if (s->set_pc && s->cpu) {
+        CPUClass *cc = CPU_GET_CLASS(s->cpu);
+        cpu_reset(s->cpu);
+        if (cc) {
+            cc->set_pc(s->cpu, s->addr);
+        }
+    }
+
+    if (s->data_len) {
+        assert(s->data_len < sizeof(s->data));
+        dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr, &s->data,
+                         s->data_len);
+    }
+}
+
+static void generic_loader_realize(DeviceState *dev, Error **errp)
+{
+    GenericLoaderState *s = GENERIC_LOADER(dev);
+    hwaddr entry;
+    int big_endian;
+    int size = 0;
+
+    s->set_pc = false;
+
+    /* Perform some error checking on the user's options */
+    if (s->data || s->data_len  || s->data_be) {
+        /* User is loading memory values */
+        if (s->file) {
+            error_setg(errp, "Specifying a file is not supported when loading "
+                       "memory values");
+            return;
+        } else if (s->force_raw) {
+            error_setg(errp, "Specifying force-raw is not supported when "
+                       "loading memory values");
+            return;
+        } else if (!s->data || !s->data_len) {
+            error_setg(errp, "Both data and data-len must be specified");
+            return;
+        } else if (s->data_len > 8) {
+            error_setg(errp, "data-len cannot be greater then 8 bytes");
+            return;
+        } else if (s->data_len > sizeof(s->data)) {
+            error_setg(errp, "data-len cannot be more then the data size");
+            return;
+        }
+    } 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;
+        }
+        s->set_pc = true;
+    } else if (s->addr) {
+        /* User is setting the PC */
+        if (s->data || s->data_len || s->data_be) {
+            error_setg(errp, "data can not be specified when setting a "
+                       "program counter");
+            return;
+        }
+        s->set_pc = true;
+    } else {
+        /* Did the user specify anything? */
+        error_setg(errp, "please include valid arguments");
+        return;
+    }
+
+    qemu_register_reset(generic_loader_reset, dev);
+
+    if (s->cpu_num != CPU_NONE) {
+        s->cpu = qemu_get_cpu(s->cpu_num);
+        if (!s->cpu) {
+            error_setg(errp, "Specified boot CPU#%d is nonexistent",
+                       s->cpu_num);
+            return;
+        }
+    }
+
+#ifdef TARGET_WORDS_BIGENDIAN
+    big_endian = 1;
+#else
+    big_endian = 0;
+#endif
+
+    if (s->file) {
+        if (!s->force_raw) {
+            size = load_elf_as(s->file, NULL, NULL, &entry, NULL, NULL,
+                               big_endian, 0, 0, 0,
+                               (s->cpu ? s->cpu : first_cpu)->as);
+
+            if (size < 0) {
+                size = load_uimage_as(s->file, &entry, NULL, NULL, NULL, NULL,
+                                      (s->cpu ? s->cpu : first_cpu)->as);
+            }
+        }
+
+        if (size < 0 || s->force_raw) {
+            /* Default to the maximum size being the machine's ram size */
+            size = load_image_targphys_as(s->file, s->addr, ram_size,
+                                          (s->cpu ? s->cpu : first_cpu)->as);
+        } else {
+            s->addr = entry;
+        }
+
+        if (size < 0) {
+            error_setg(errp, "Cannot load specified image %s", s->file);
+            return;
+        }
+    }
+
+    /* Convert the data endiannes */
+    if (s->data_be) {
+        s->data = cpu_to_be64(s->data);
+    } else {
+        s->data = cpu_to_le64(s->data);
+    }
+}
+
+static void generic_loader_unrealize(DeviceState *dev, Error **errp)
+{
+    qemu_unregister_reset(generic_loader_reset, dev);
+}
+
+static Property generic_loader_props[] = {
+    DEFINE_PROP_UINT64("addr", GenericLoaderState, addr, 0),
+    DEFINE_PROP_UINT64("data", GenericLoaderState, data, 0),
+    DEFINE_PROP_UINT8("data-len", GenericLoaderState, data_len, 0),
+    DEFINE_PROP_BOOL("data-be", GenericLoaderState, data_be, false),
+    DEFINE_PROP_UINT32("cpu-num", GenericLoaderState, cpu_num, CPU_NONE),
+    DEFINE_PROP_BOOL("force-raw", GenericLoaderState, force_raw, false),
+    DEFINE_PROP_STRING("file", GenericLoaderState, file),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void generic_loader_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    /* The reset function is not registered here and is instead registered in
+     * the realize function to allow this device to be added via the device_add
+     * command in the QEMU monitor.
+     * TODO: Improve the device_add functionality to allow resets to be
+     * connected
+     */
+    dc->realize = generic_loader_realize;
+    dc->unrealize = generic_loader_unrealize;
+    dc->props = generic_loader_props;
+    dc->desc = "Generic Loader";
+}
+
+static TypeInfo generic_loader_info = {
+    .name = TYPE_GENERIC_LOADER,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(GenericLoaderState),
+    .class_init = generic_loader_class_init,
+};
+
+static void generic_loader_register_type(void)
+{
+    type_register_static(&generic_loader_info);
+}
+
+type_init(generic_loader_register_type)
diff --git a/include/hw/core/generic-loader.h b/include/hw/core/generic-loader.h
new file mode 100644
index 0000000..dd27c42
--- /dev/null
+++ b/include/hw/core/generic-loader.h
@@ -0,0 +1,46 @@
+/*
+ * Generic Loader
+ *
+ * Copyright (C) 2014 Li Guang
+ * Written by Li Guang <lig.fnst@cn.fujitsu.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#ifndef GENERIC_LOADER_H
+#define GENERIC_LOADER_H
+
+#include "elf.h"
+
+typedef struct GenericLoaderState {
+    /* <private> */
+    DeviceState parent_obj;
+
+    /* <public> */
+    CPUState *cpu;
+
+    uint64_t addr;
+    uint64_t data;
+    uint8_t data_len;
+    uint32_t cpu_num;
+
+    char *file;
+
+    bool force_raw;
+    bool data_be;
+    bool set_pc;
+} GenericLoaderState;
+
+#define TYPE_GENERIC_LOADER "loader"
+#define GENERIC_LOADER(obj) OBJECT_CHECK(GenericLoaderState, (obj), \
+                                         TYPE_GENERIC_LOADER)
+
+#endif
-- 
2.7.4

[Qemu-devel] [PATCH v10 8/8] docs: Add a generic loader explanation document

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
V10:
 - Split the data loading and PC setting
V9:
 - Clarify the image loading options
V8:
 - Improve documentation
V6:
 - Fixup documentation
V4:
 - Re-write to be more comprehensive

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

diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
new file mode 100644
index 0000000..cbb57f7
--- /dev/null
+++ b/docs/generic-loader.txt
@@ -0,0 +1,81 @@
+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 Data into 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=<data-len>
+                   [,data-be=<data-be>][,cpu-num=<cpu-num>]
+
+    <addr>      - The address to store the data in.
+    <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>   - The number of the CPU's address space where the data should
+                  be loaded. If not sepcified the default address space is
+                  used.
+
+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
+
+Setting a CPU's Program Counter
+---------------------
+The loader device allows the CPU's PC to be set from the command line. This
+can be done by following the syntax below:
+
+     -device loader,addr=<addr>,cpu-num=<cpu-num>
+
+    <addr>      - The value to use as the CPU's PC.
+    <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=0x8000,cpu-num=0
+
+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 or in the case of an ELF file to
+                  the value in the header. This option should only be used
+                  for the boot image.
+                  This will also cause the image to be written to the specified
+                  CPUs address space.
+    <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
-- 
2.7.4

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

[Peter Maydell]

On 3 August 2016 at 21:06, Alistair Francis <alistair.francis@xilinx.com> wrote:
> 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 <alistair.francis@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v10 5/8] loader: Add AddressSpace loading support to uImages

[Peter Maydell]

On 3 August 2016 at 21:06, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Add a new function load_uimage_as() that allows the caller to
> specify an AddressSpace to use when loading the uImage. The
> original load_uimage() function doesn't have any change in
> functionality.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> V10:
>  - Add comment about the function in the header file.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v10 6/8] loader: Add AddressSpace loading support to targphys

[Peter Maydell]

On 3 August 2016 at 21:06, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Add a new function load_image_targphys_as() that allows the caller
> to specify an AddressSpace to use when loading a targphys. The
> original load_image_targphys() function doesn't have any change in
> functionality.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v10 7/8] generic-loader: Add a generic loader

[Peter Maydell]

On 3 August 2016 at 21:06, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Add a generic loader to QEMU which can be used to load images or set
> memory values.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> V10:
>  - Split out the PC setting and data loading
> V9:
>  - Fix error messages
>  - Updated some incorrect logic
>  - Add address space loading support for all image types
>  - Explain why the reset is manually registered
> 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         | 195 +++++++++++++++++++++++++++++++++++++++
>  include/hw/core/generic-loader.h |  46 +++++++++
>  4 files changed, 249 insertions(+)
>  create mode 100644 hw/core/generic-loader.c
>  create mode 100644 include/hw/core/generic-loader.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9c88c44..cc34ae0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -984,6 +984,12 @@ M: Dmitry Fleytman <dmitry@daynix.com>
>  S: Maintained
>  F: hw/net/e1000e*
>
> +Generic Loader
> +M: Alistair Francis <alistair.francis@xilinx.com>
> +S: Maintained
> +F: hw/core/generic-loader.c
> +F: include/hw/core/generic-loader.h
> +
>  Subsystems
>  ----------
>  Audio
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index cfd4840..939c94e 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -17,3 +17,5 @@ common-obj-$(CONFIG_SOFTMMU) += loader.o
>  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
>  common-obj-$(CONFIG_SOFTMMU) += register.o
>  common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
> +
> +obj-$(CONFIG_SOFTMMU) += generic-loader.o
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> new file mode 100644
> index 0000000..4ee71cb
> --- /dev/null
> +++ b/hw/core/generic-loader.c
> @@ -0,0 +1,195 @@
> +/*
> + * Generic Loader
> + *
> + * Copyright (C) 2014 Li Guang
> + * Copyright (C) 2016 Xilinx Inc.
> + * Written by Li Guang <lig.fnst@cn.fujitsu.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qom/cpu.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/dma.h"
> +#include "hw/loader.h"
> +#include "qapi/error.h"
> +#include "hw/core/generic-loader.h"
> +
> +#define CPU_NONE 0xFFFFFFFF
> +
> +static void generic_loader_reset(void *opaque)
> +{
> +    GenericLoaderState *s = GENERIC_LOADER(opaque);
> +
> +    if (s->set_pc && s->cpu) {

This check on s->cpu means that -device loader,file=<...> won't set the
PC to the entry point unless you specifically pass cpu-num=0.
The easiest fix for this is just to remove it (see below).

> +        CPUClass *cc = CPU_GET_CLASS(s->cpu);
> +        cpu_reset(s->cpu);
> +        if (cc) {
> +            cc->set_pc(s->cpu, s->addr);
> +        }
> +    }
> +
> +    if (s->data_len) {
> +        assert(s->data_len < sizeof(s->data));
> +        dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr, &s->data,
> +                         s->data_len);
> +    }
> +}
> +
> +static void generic_loader_realize(DeviceState *dev, Error **errp)
> +{
> +    GenericLoaderState *s = GENERIC_LOADER(dev);
> +    hwaddr entry;
> +    int big_endian;
> +    int size = 0;
> +
> +    s->set_pc = false;
> +
> +    /* Perform some error checking on the user's options */
> +    if (s->data || s->data_len  || s->data_be) {
> +        /* User is loading memory values */
> +        if (s->file) {
> +            error_setg(errp, "Specifying a file is not supported when loading "
> +                       "memory values");
> +            return;
> +        } else if (s->force_raw) {
> +            error_setg(errp, "Specifying force-raw is not supported when "
> +                       "loading memory values");
> +            return;
> +        } else if (!s->data || !s->data_len) {
> +            error_setg(errp, "Both data and data-len must be specified");
> +            return;
> +        } else if (s->data_len > 8) {
> +            error_setg(errp, "data-len cannot be greater then 8 bytes");
> +            return;
> +        } else if (s->data_len > sizeof(s->data)) {
> +            error_setg(errp, "data-len cannot be more then the data size");

This can't ever happen, because sizeof(s->data) is always 8.

> +            return;
> +        }
> +    } else if (s->file || s->force_raw)  {
> +        /* User is loading an image */
> +        if (s->data || s->data_len || s->data_be) {
> +            error_setg(errp, "data can not be specified when loading an "
> +                       "image");
> +            return;
> +        }
> +        s->set_pc = true;
> +    } else if (s->addr) {
> +        /* User is setting the PC */
> +        if (s->data || s->data_len || s->data_be) {
> +            error_setg(errp, "data can not be specified when setting a "
> +                       "program counter");
> +            return;
> +        }

Failing to specify cpu_num in this case needs to be an error.

> +        s->set_pc = true;
> +    } else {
> +        /* Did the user specify anything? */
> +        error_setg(errp, "please include valid arguments");
> +        return;
> +    }


> +
> +    qemu_register_reset(generic_loader_reset, dev);
> +
> +    if (s->cpu_num != CPU_NONE) {
> +        s->cpu = qemu_get_cpu(s->cpu_num);
> +        if (!s->cpu) {
> +            error_setg(errp, "Specified boot CPU#%d is nonexistent",
> +                       s->cpu_num);
> +            return;
> +        }
> +    }

If you always set s->cpu to be first_cpu in the "cpu_num is CPU_NONE"
case you can avoid all the ?: everywhere you use it, and you don't need
to check for 's->cpu' being non-NULL in the function where you set the PC.

> +
> +#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_as(s->file, &entry, NULL, NULL, NULL, NULL,
> +                                      (s->cpu ? s->cpu : first_cpu)->as);
> +            }
> +        }
> +
> +        if (size < 0 || s->force_raw) {
> +            /* Default to the maximum size being the machine's ram size */
> +            size = load_image_targphys_as(s->file, s->addr, ram_size,
> +                                          (s->cpu ? s->cpu : first_cpu)->as);
> +        } else {
> +            s->addr = entry;
> +        }
> +
> +        if (size < 0) {
> +            error_setg(errp, "Cannot load specified image %s", s->file);
> +            return;
> +        }
> +    }

Otherwise:

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Peter Maydell]

On 3 August 2016 at 21:06, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> V10:
>  - Split the data loading and PC setting
> V9:
>  - Clarify the image loading options
> V8:
>  - Improve documentation
> V6:
>  - Fixup documentation
> V4:
>  - Re-write to be more comprehensive
>
>  docs/generic-loader.txt | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 docs/generic-loader.txt
>
> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
> new file mode 100644
> index 0000000..cbb57f7
> --- /dev/null
> +++ b/docs/generic-loader.txt
> @@ -0,0 +1,81 @@
> +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 Data into 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=<data-len>
> +                   [,data-be=<data-be>][,cpu-num=<cpu-num>]
> +
> +    <addr>      - The address to store the data in.
> +    <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>   - The number of the CPU's address space where the data should
> +                  be loaded. If not sepcified the default address space is
> +                  used.

"specified"

"the address space of the first CPU is used", or at least that's what
the code does.

> +
> +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
> +
> +Setting a CPU's Program Counter
> +---------------------
> +The loader device allows the CPU's PC to be set from the command line. This
> +can be done by following the syntax below:
> +
> +     -device loader,addr=<addr>,cpu-num=<cpu-num>
> +
> +    <addr>      - The value to use as the CPU's PC.
> +    <cpu-num>   - This will cause the CPU to be reset and the PC to be set to
> +                  the value of addr.

"The number of the CPU whose PC should be set to the specified value."

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

"An example of setting CPU 0's PC to 0x8000 is:"

> +    -device loader,addr=0x8000,cpu-num=0
> +
> +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 or in the case of an ELF file to
> +                  the value in the header. This option should only be used
> +                  for the boot image.
> +                  This will also cause the image to be written to the specified
> +                  CPUs address space.

"CPU's".

"If not specified, the default is CPU 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.
> +
> +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
> --
> 2.7.4

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v10 3/8] loader: Allow a custom AddressSpace when loading ROMs

[Peter Maydell]

On 3 August 2016 at 21:06, Alistair Francis <alistair.francis@xilinx.com> wrote:
> When loading ROMs allow the caller to specify an AddressSpace to use for
> the load.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> V10:
>  - Set the rom address space instead of leaving it NULL
>  - Cleanup ordering logic
> V9:
>  - Fixup the ROM ordering
>  - Don't allow address space and memory region to be specified
> V8:
>  - Introduce an RFC version of AddressSpace loading support
>
>  hw/core/loader.c     | 52 ++++++++++++++++++++++++++++++++++++++++------------
>  include/hw/elf_ops.h |  2 +-
>  include/hw/loader.h  | 10 ++++++----
>  3 files changed, 47 insertions(+), 17 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 6b61f29..aef7bc9 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -777,6 +777,7 @@ struct Rom {
>
>      uint8_t *data;
>      MemoryRegion *mr;
> +    AddressSpace *as;
>      int isrom;
>      char *fw_dir;
>      char *fw_file;
> @@ -788,6 +789,11 @@ struct Rom {
>  static FWCfgState *fw_cfg;
>  static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
>
> +static inline bool rom_order_compare(Rom *rom, Rom *item)
> +{
> +    return rom->addr >= item->addr;
> +}
> +
>  static void rom_insert(Rom *rom)
>  {
>      Rom *item;
> @@ -796,12 +802,22 @@ static void rom_insert(Rom *rom)
>          hw_error ("ROM images must be loaded at startup\n");
>      }
>
> -    /* list is ordered by load address */
> +    /* The user didn't specify an address space, this is the default */
> +    if (!rom->as) {
> +        rom->as = &address_space_memory;
> +    }
> +
> +    /* List is ordered by load address in the same address space */
>      QTAILQ_FOREACH(item, &roms, next) {
> -        if (rom->addr >= item->addr)
> -            continue;
> -        QTAILQ_INSERT_BEFORE(item, rom, next);
> -        return;
> +        if (rom->as == item->as) {
> +            if (rom_order_compare(rom, item)) {
> +                QTAILQ_INSERT_AFTER(&roms, item, rom, next);
> +                return;
> +            } else {
> +                QTAILQ_INSERT_BEFORE(item, rom, next);
> +                return;
> +            }
> +        }

This isn't quite what I meant, and if we're doing it like this
we might as well have "rom->addr >= item->addr" opencoded here.

What I had in mind when I suggested a separate rom_order_compare()
was that it should be doing the full ordering operation:
    return (rom->as > item->as) ||
        (rom->as == item->as && rom->addr >= item->addr);

(ie it defines a total order on ROMs which for any two ROMs
tells you which way round they go in the list).

and then the change in this function would just be to change the old
   if (rom->addr >= item->addr)
to
   if (rom_order_compare(rom, item))

This has the advantage that it's really obvious that we're
maintaining a sorted array and all we've done is change the
function defining our total order on ROMs.

The problem which I've realised in the course of thinking about
this this evening is that the compare function I suggest above:
 (a) requires a total ordering on pointers, which is not
  guaranteed by the C standard
 (b) even if you cast the rom->as pointers all to uintptr_t
  (getting you defined behaviour) still gives you an ordering
  which is variable from run to run of QEMU, which isn't great.
Unfortunately there's no other obvious ordering on AddressSpaces
(I guess you could look at their position in the address_spaces
list, but that's getting pretty heavyweight.)

That said, the code as you have it in this patch doesn't
sort things properly I think. Consider three ROMs with
addresses 0, 100, 200, all in the same AS, inserted in that order:
 * ROM-0 is added as the first and only item in the list
 * ROM-100 is added in the list after ROM-0 (same AS, and
   rom_order_compare is true, so we do INSERT_AFTER)
 * when we insert ROM-200 the first item in the list is ROM-0.
   This has the same AS, and rom_order_compare(ROM-200, ROM-0)
   is true, so we do an INSERT_AFTER.
Now the list is (ROM-0, ROM-200, ROM-100), which isn't sorted.


So our options are:
 (1) fix up the open-coded code to sort properly
 (2) figure out a rom_order_compare() that does give a total
     ordering on ROMs which isn't variable from run to run of QEMU
 (3) decide that it's OK for the ROM list ordering not to be
     the same from run to run (or on two sides of a migration)
     so we can use the cast-to-uintptr_t version

I don't currently have an opinion (well, I would like 2 except
I can't think of a workable definition for the comparison function).
Anybody?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v10 7/8] generic-loader: Add a generic loader

[Alistair Francis]

On Tue, Aug 9, 2016 at 11:17 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 August 2016 at 21:06, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Add a generic loader to QEMU which can be used to load images or set
>> memory values.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> V10:
>>  - Split out the PC setting and data loading
>> V9:
>>  - Fix error messages
>>  - Updated some incorrect logic
>>  - Add address space loading support for all image types
>>  - Explain why the reset is manually registered
>> 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         | 195 +++++++++++++++++++++++++++++++++++++++
>>  include/hw/core/generic-loader.h |  46 +++++++++
>>  4 files changed, 249 insertions(+)
>>  create mode 100644 hw/core/generic-loader.c
>>  create mode 100644 include/hw/core/generic-loader.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9c88c44..cc34ae0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -984,6 +984,12 @@ M: Dmitry Fleytman <dmitry@daynix.com>
>>  S: Maintained
>>  F: hw/net/e1000e*
>>
>> +Generic Loader
>> +M: Alistair Francis <alistair.francis@xilinx.com>
>> +S: Maintained
>> +F: hw/core/generic-loader.c
>> +F: include/hw/core/generic-loader.h
>> +
>>  Subsystems
>>  ----------
>>  Audio
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index cfd4840..939c94e 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -17,3 +17,5 @@ common-obj-$(CONFIG_SOFTMMU) += loader.o
>>  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
>>  common-obj-$(CONFIG_SOFTMMU) += register.o
>>  common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
>> +
>> +obj-$(CONFIG_SOFTMMU) += generic-loader.o
>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
>> new file mode 100644
>> index 0000000..4ee71cb
>> --- /dev/null
>> +++ b/hw/core/generic-loader.c
>> @@ -0,0 +1,195 @@
>> +/*
>> + * Generic Loader
>> + *
>> + * Copyright (C) 2014 Li Guang
>> + * Copyright (C) 2016 Xilinx Inc.
>> + * Written by Li Guang <lig.fnst@cn.fujitsu.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
>> + * for more details.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qom/cpu.h"
>> +#include "hw/sysbus.h"
>> +#include "sysemu/dma.h"
>> +#include "hw/loader.h"
>> +#include "qapi/error.h"
>> +#include "hw/core/generic-loader.h"
>> +
>> +#define CPU_NONE 0xFFFFFFFF
>> +
>> +static void generic_loader_reset(void *opaque)
>> +{
>> +    GenericLoaderState *s = GENERIC_LOADER(opaque);
>> +
>> +    if (s->set_pc && s->cpu) {
>
> This check on s->cpu means that -device loader,file=<...> won't set the
> PC to the entry point unless you specifically pass cpu-num=0.
> The easiest fix for this is just to remove it (see below).

Fixed!

>
>> +        CPUClass *cc = CPU_GET_CLASS(s->cpu);
>> +        cpu_reset(s->cpu);
>> +        if (cc) {
>> +            cc->set_pc(s->cpu, s->addr);
>> +        }
>> +    }
>> +
>> +    if (s->data_len) {
>> +        assert(s->data_len < sizeof(s->data));
>> +        dma_memory_write((s->cpu ? s->cpu : first_cpu)->as, s->addr, &s->data,
>> +                         s->data_len);
>> +    }
>> +}
>> +
>> +static void generic_loader_realize(DeviceState *dev, Error **errp)
>> +{
>> +    GenericLoaderState *s = GENERIC_LOADER(dev);
>> +    hwaddr entry;
>> +    int big_endian;
>> +    int size = 0;
>> +
>> +    s->set_pc = false;
>> +
>> +    /* Perform some error checking on the user's options */
>> +    if (s->data || s->data_len  || s->data_be) {
>> +        /* User is loading memory values */
>> +        if (s->file) {
>> +            error_setg(errp, "Specifying a file is not supported when loading "
>> +                       "memory values");
>> +            return;
>> +        } else if (s->force_raw) {
>> +            error_setg(errp, "Specifying force-raw is not supported when "
>> +                       "loading memory values");
>> +            return;
>> +        } else if (!s->data || !s->data_len) {
>> +            error_setg(errp, "Both data and data-len must be specified");
>> +            return;
>> +        } else if (s->data_len > 8) {
>> +            error_setg(errp, "data-len cannot be greater then 8 bytes");
>> +            return;
>> +        } else if (s->data_len > sizeof(s->data)) {
>> +            error_setg(errp, "data-len cannot be more then the data size");
>
> This can't ever happen, because sizeof(s->data) is always 8.

Ok, removed.

>
>> +            return;
>> +        }
>> +    } else if (s->file || s->force_raw)  {
>> +        /* User is loading an image */
>> +        if (s->data || s->data_len || s->data_be) {
>> +            error_setg(errp, "data can not be specified when loading an "
>> +                       "image");
>> +            return;
>> +        }
>> +        s->set_pc = true;
>> +    } else if (s->addr) {
>> +        /* User is setting the PC */
>> +        if (s->data || s->data_len || s->data_be) {
>> +            error_setg(errp, "data can not be specified when setting a "
>> +                       "program counter");
>> +            return;
>> +        }
>
> Failing to specify cpu_num in this case needs to be an error.

Fixed.

>
>> +        s->set_pc = true;
>> +    } else {
>> +        /* Did the user specify anything? */
>> +        error_setg(errp, "please include valid arguments");
>> +        return;
>> +    }
>
>
>> +
>> +    qemu_register_reset(generic_loader_reset, dev);
>> +
>> +    if (s->cpu_num != CPU_NONE) {
>> +        s->cpu = qemu_get_cpu(s->cpu_num);
>> +        if (!s->cpu) {
>> +            error_setg(errp, "Specified boot CPU#%d is nonexistent",
>> +                       s->cpu_num);
>> +            return;
>> +        }
>> +    }
>
> If you always set s->cpu to be first_cpu in the "cpu_num is CPU_NONE"
> case you can avoid all the ?: everywhere you use it, and you don't need
> to check for 's->cpu' being non-NULL in the function where you set the PC.

Fixed.

>
>> +
>> +#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_as(s->file, &entry, NULL, NULL, NULL, NULL,
>> +                                      (s->cpu ? s->cpu : first_cpu)->as);
>> +            }
>> +        }
>> +
>> +        if (size < 0 || s->force_raw) {
>> +            /* Default to the maximum size being the machine's ram size */
>> +            size = load_image_targphys_as(s->file, s->addr, ram_size,
>> +                                          (s->cpu ? s->cpu : first_cpu)->as);
>> +        } else {
>> +            s->addr = entry;
>> +        }
>> +
>> +        if (size < 0) {
>> +            error_setg(errp, "Cannot load specified image %s", s->file);
>> +            return;
>> +        }
>> +    }
>
> Otherwise:
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks!

Alistair

>
> thanks
> -- PMM
>

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

[Alistair Francis]

On Tue, Aug 9, 2016 at 11:19 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 August 2016 at 21:06, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> V10:
>>  - Split the data loading and PC setting
>> V9:
>>  - Clarify the image loading options
>> V8:
>>  - Improve documentation
>> V6:
>>  - Fixup documentation
>> V4:
>>  - Re-write to be more comprehensive
>>
>>  docs/generic-loader.txt | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 81 insertions(+)
>>  create mode 100644 docs/generic-loader.txt
>>
>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>> new file mode 100644
>> index 0000000..cbb57f7
>> --- /dev/null
>> +++ b/docs/generic-loader.txt
>> @@ -0,0 +1,81 @@
>> +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 Data into 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=<data-len>
>> +                   [,data-be=<data-be>][,cpu-num=<cpu-num>]
>> +
>> +    <addr>      - The address to store the data in.
>> +    <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>   - The number of the CPU's address space where the data should
>> +                  be loaded. If not sepcified the default address space is
>> +                  used.
>
> "specified"
>
> "the address space of the first CPU is used", or at least that's what
> the code does.
>
>> +
>> +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
>> +
>> +Setting a CPU's Program Counter
>> +---------------------
>> +The loader device allows the CPU's PC to be set from the command line. This
>> +can be done by following the syntax below:
>> +
>> +     -device loader,addr=<addr>,cpu-num=<cpu-num>
>> +
>> +    <addr>      - The value to use as the CPU's PC.
>> +    <cpu-num>   - This will cause the CPU to be reset and the PC to be set to
>> +                  the value of addr.
>
> "The number of the CPU whose PC should be set to the specified value."
>
>> +
>> +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:
>
> "An example of setting CPU 0's PC to 0x8000 is:"
>
>> +    -device loader,addr=0x8000,cpu-num=0
>> +
>> +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 or in the case of an ELF file to
>> +                  the value in the header. This option should only be used
>> +                  for the boot image.
>> +                  This will also cause the image to be written to the specified
>> +                  CPUs address space.
>
> "CPU's".
>
> "If not specified, the default is CPU 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.
>> +
>> +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
>> --
>> 2.7.4
>
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Fixed them all, thanks for the review.

Thanks,

Alistair

>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v10 3/8] loader: Allow a custom AddressSpace when loading ROMs

[Alistair Francis]

On Tue, Aug 9, 2016 at 11:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 August 2016 at 21:06, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> When loading ROMs allow the caller to specify an AddressSpace to use for
>> the load.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> V10:
>>  - Set the rom address space instead of leaving it NULL
>>  - Cleanup ordering logic
>> V9:
>>  - Fixup the ROM ordering
>>  - Don't allow address space and memory region to be specified
>> V8:
>>  - Introduce an RFC version of AddressSpace loading support
>>
>>  hw/core/loader.c     | 52 ++++++++++++++++++++++++++++++++++++++++------------
>>  include/hw/elf_ops.h |  2 +-
>>  include/hw/loader.h  | 10 ++++++----
>>  3 files changed, 47 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 6b61f29..aef7bc9 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -777,6 +777,7 @@ struct Rom {
>>
>>      uint8_t *data;
>>      MemoryRegion *mr;
>> +    AddressSpace *as;
>>      int isrom;
>>      char *fw_dir;
>>      char *fw_file;
>> @@ -788,6 +789,11 @@ struct Rom {
>>  static FWCfgState *fw_cfg;
>>  static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
>>
>> +static inline bool rom_order_compare(Rom *rom, Rom *item)
>> +{
>> +    return rom->addr >= item->addr;
>> +}
>> +
>>  static void rom_insert(Rom *rom)
>>  {
>>      Rom *item;
>> @@ -796,12 +802,22 @@ static void rom_insert(Rom *rom)
>>          hw_error ("ROM images must be loaded at startup\n");
>>      }
>>
>> -    /* list is ordered by load address */
>> +    /* The user didn't specify an address space, this is the default */
>> +    if (!rom->as) {
>> +        rom->as = &address_space_memory;
>> +    }
>> +
>> +    /* List is ordered by load address in the same address space */
>>      QTAILQ_FOREACH(item, &roms, next) {
>> -        if (rom->addr >= item->addr)
>> -            continue;
>> -        QTAILQ_INSERT_BEFORE(item, rom, next);
>> -        return;
>> +        if (rom->as == item->as) {
>> +            if (rom_order_compare(rom, item)) {
>> +                QTAILQ_INSERT_AFTER(&roms, item, rom, next);
>> +                return;
>> +            } else {
>> +                QTAILQ_INSERT_BEFORE(item, rom, next);
>> +                return;
>> +            }
>> +        }
>
> This isn't quite what I meant, and if we're doing it like this
> we might as well have "rom->addr >= item->addr" opencoded here.
>
> What I had in mind when I suggested a separate rom_order_compare()
> was that it should be doing the full ordering operation:
>     return (rom->as > item->as) ||
>         (rom->as == item->as && rom->addr >= item->addr);
>
> (ie it defines a total order on ROMs which for any two ROMs
> tells you which way round they go in the list).
>
> and then the change in this function would just be to change the old
>    if (rom->addr >= item->addr)
> to
>    if (rom_order_compare(rom, item))

Aw, now I understand. I was a little confused why you wanted the extra function.

>
> This has the advantage that it's really obvious that we're
> maintaining a sorted array and all we've done is change the
> function defining our total order on ROMs.
>
> The problem which I've realised in the course of thinking about
> this this evening is that the compare function I suggest above:
>  (a) requires a total ordering on pointers, which is not
>   guaranteed by the C standard
>  (b) even if you cast the rom->as pointers all to uintptr_t
>   (getting you defined behaviour) still gives you an ordering
>   which is variable from run to run of QEMU, which isn't great.
> Unfortunately there's no other obvious ordering on AddressSpaces
> (I guess you could look at their position in the address_spaces
> list, but that's getting pretty heavyweight.)
>
> That said, the code as you have it in this patch doesn't
> sort things properly I think. Consider three ROMs with
> addresses 0, 100, 200, all in the same AS, inserted in that order:
>  * ROM-0 is added as the first and only item in the list
>  * ROM-100 is added in the list after ROM-0 (same AS, and
>    rom_order_compare is true, so we do INSERT_AFTER)
>  * when we insert ROM-200 the first item in the list is ROM-0.
>    This has the same AS, and rom_order_compare(ROM-200, ROM-0)
>    is true, so we do an INSERT_AFTER.
> Now the list is (ROM-0, ROM-200, ROM-100), which isn't sorted.

I missed that case. Good catch.

>
>
> So our options are:
>  (1) fix up the open-coded code to sort properly
>  (2) figure out a rom_order_compare() that does give a total
>      ordering on ROMs which isn't variable from run to run of QEMU
>  (3) decide that it's OK for the ROM list ordering not to be
>      the same from run to run (or on two sides of a migration)
>      so we can use the cast-to-uintptr_t version

Remember that the original reason to order these ROMs was to ensure
that there were no overlaps. So I don't think that it matters too much
if the address space ordering changes.

One solution though to sort them is to sort the address spaces by
name. Are they guaranteed to have a unique names?

Thanks,

Alistair

>
> I don't currently have an opinion (well, I would like 2 except
> I can't think of a workable definition for the comparison function).
> Anybody?
>
> thanks
> -- PMM
>