[Qemu-devel] [PATCH v8 0/5] Add a generic loader

[Qemu-devel] [PATCH v8 0/5] 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).

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

-- 
2.7.4

[Qemu-devel] [PATCH v8 1/5] 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>
---
V8:
 - Move into load_elf64/load_elf32
V7:
 - Fix typo

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

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index f510e7e..db70c11 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 < 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) {
-- 
2.7.4

[Qemu-devel] [PATCH v8 2/5] loader: All a custom SddressSpace 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>
---
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(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 53e0e41..fcbcfbf 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;
@@ -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;
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index db70c11..1339677 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 4879b63..18eb0f2 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -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
-- 
2.7.4

[Qemu-devel] [PATCH v8 3/5] 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>
---
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(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index fcbcfbf..9b25dfc 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 1339677..3b8c9e9 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 18eb0f2..d14eab1 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
@@ -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,
-- 
2.7.4

[Qemu-devel] [PATCH v8 4/5] 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>
---
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

diff --git a/MAINTAINERS b/MAINTAINERS
index 2ab6e3b..0077e22 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -992,6 +992,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 82a9ef8..ab238fa 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -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
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
new file mode 100644
index 0000000..c9b0572
--- /dev/null
+++ b/hw/core/generic-loader.c
@@ -0,0 +1,177 @@
+/*
+ * 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 0xFFFF
+
+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 (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;
+
+    /* 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 length must be specified");
+            return;
+        } else if (s->cpu_num) {
+            error_setg(errp, "Setting data and a cpu number is not supported");
+            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;
+        }
+    } else if (s->data_len) {
+        if (s->data_len > 8) {
+            error_setg(errp, "data-len cannot be greate 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;
+        }
+    }
+
+    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(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;
+        }
+
+        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);
+
+    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..4dd2361
--- /dev/null
+++ b/include/hw/core/generic-loader.h
@@ -0,0 +1,45 @@
+/*
+ * 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;
+} GenericLoaderState;
+
+#define TYPE_GENERIC_LOADER "loader"
+#define GENERIC_LOADER(obj) OBJECT_CHECK(GenericLoaderState, (obj), \
+                                         TYPE_GENERIC_LOADER)
+
+#endif
-- 
2.7.4

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

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
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

diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
new file mode 100644
index 0000000..34684fc
--- /dev/null
+++ b/docs/generic-loader.txt
@@ -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
-- 
2.7.4

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

[Peter Maydell]

On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@xilinx.com> wrote:
> 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.

"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

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

[Christian Borntraeger]

On 07/02/2016 03:07 AM, Alistair Francis wrote:
> 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.


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.


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

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

[Alistair Francis]

On Mon, Jul 4, 2016 at 5:22 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> 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.
>
> "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

>
> thanks
> -- PMM
>

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

[Alistair Francis]

On Mon, Jul 4, 2016 at 5:43 AM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
> On 07/02/2016 03:07 AM, Alistair Francis wrote:
>> 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.
>
>
> 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

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

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

[Peter Maydell]

On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 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

> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
> new file mode 100644
> index 0000000..34684fc
> --- /dev/null
> +++ b/docs/generic-loader.txt
> @@ -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.

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.

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

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

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

[Peter Maydell]

On 2 July 2016 at 02:07, 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>
> ---
> 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
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2ab6e3b..0077e22 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -992,6 +992,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 82a9ef8..ab238fa 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -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
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> new file mode 100644
> index 0000000..c9b0572
> --- /dev/null
> +++ b/hw/core/generic-loader.c
> @@ -0,0 +1,177 @@
> +/*
> + * 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 0xFFFF

This should be updated now we've widened the type from 16 bits.

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

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

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

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

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

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

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

[Peter Maydell]

On 2 July 2016 at 02:07, 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>
> ---
> V8:
>  - Move into load_elf64/load_elf32
> V7:
>  - Fix typo
>
>  include/hw/elf_ops.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index f510e7e..db70c11 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 < 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

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

[Peter Maydell]

On 2 July 2016 at 02:07, 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>
> ---
> 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(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 53e0e41..fcbcfbf 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;
> @@ -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.

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

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

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

>      qemu_register_reset(rom_reset, NULL);
>      roms_loaded = 1;
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index db70c11..1339677 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 4879b63..18eb0f2 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -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
> --
> 2.7.4
>

thanks
-- PMM

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

[Peter Maydell]

On 2 July 2016 at 02:07, 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>

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

thanks
-- PMM

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

[Alistair Francis]

On Tue, Jul 12, 2016 at 9:40 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 July 2016 at 02:07, 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>
>> ---
>> V8:
>>  - Move into load_elf64/load_elf32
>> V7:
>>  - Fix typo
>>
>>  include/hw/elf_ops.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
>> index f510e7e..db70c11 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 < 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

>
> thanks
> -- PMM
>

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

[Alistair Francis]

On Tue, Jul 12, 2016 at 9:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 July 2016 at 02:07, 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>
>> ---
>> 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(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 53e0e41..fcbcfbf 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;
>> @@ -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 have fixed that.

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

Agreed, it is now an error.

>
>>  {
>>      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);
>
> (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.

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

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

Thanks,

Alistair

>
>>      qemu_register_reset(rom_reset, NULL);
>>      roms_loaded = 1;
>> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
>> index db70c11..1339677 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 4879b63..18eb0f2 100644
>> --- a/include/hw/loader.h
>> +++ b/include/hw/loader.h
>> @@ -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
>> --
>> 2.7.4
>>
>
> thanks
> -- PMM
>

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

[Alistair Francis]

On Tue, Jul 12, 2016 at 9:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 July 2016 at 02:07, 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>
>> ---
>> 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
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2ab6e3b..0077e22 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -992,6 +992,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 82a9ef8..ab238fa 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -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
>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
>> new file mode 100644
>> index 0000000..c9b0572
>> --- /dev/null
>> +++ b/hw/core/generic-loader.c
>> @@ -0,0 +1,177 @@
>> +/*
>> + * 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 0xFFFF
>
> This should be updated now we've widened the type from 16 bits.

Fixed

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

This is up to the user to specify the commands in the order they want
them applied. What other method should we be using?

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

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

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

Fixed

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

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

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

>
> thanks
> -- PMM
>

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

[Alistair Francis]

On Tue, Jul 12, 2016 at 9:31 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> 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
>
>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>> new file mode 100644
>> index 0000000..34684fc
>> --- /dev/null
>> +++ b/docs/generic-loader.txt
>> @@ -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.
>
> For an ELF file we use the start address specified in the ELF header, right?

Yes, I have specified that now in the docs.

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

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

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

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

[Peter Maydell]

On 13 July 2016 at 18:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Tue, Jul 12, 2016 at 9:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 2 July 2016 at 02:07, 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>
>>> ---

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

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

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

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

[Alistair Francis]

On Wed, Jul 13, 2016 at 12:44 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 13 July 2016 at 18:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> On Tue, Jul 12, 2016 at 9:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 2 July 2016 at 02:07, 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>
>>>> ---
>
>>>> +    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 ?
>>
>> 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?

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

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

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

>
> thanks
> -- PMM
>

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

[Peter Maydell]

On 13 July 2016 at 21:30, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Wed, Jul 13, 2016 at 12:44 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> On 13 July 2016 at 18:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
>>> On Tue, Jul 12, 2016 at 9:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@xilinx.com> wrote:
>>>>> +    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.

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

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

[Alistair Francis]

On Wed, Jul 13, 2016 at 2:09 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 13 July 2016 at 21:30, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> On Wed, Jul 13, 2016 at 12:44 PM, Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>> On 13 July 2016 at 18:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
>>>> On Tue, Jul 12, 2016 at 9:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>> On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@xilinx.com> wrote:
>>>>>> +    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.
>
> 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.

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

>
> thanks
> -- PMM
>