Add a generic loader

PATCH Superseded Alistair Francis 2 participants 13 messages May 25, 2016 – Jun 13, 2016

lore

Share & tools

lore

Alistair Francis

author

2016-05-25 18:52 UTC #0 lore raw

v7 cover Add a generic loader

This work is based on the original work by Li Guang with extra features added by Peter C and myself.

The idea of this loader is to allow the user to load multiple images or values into QEMU at startup.

Memory values can be loaded like this: -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4

Images can be loaded like this: -device loader,file=./images/u-boot.elf,cpu=0

This can be useful and we use it a lot in Xilinx to load multiple images into a machine at creation (ATF, Kernel and DTB for example).

It can also be used to set registers.

This patch series also make the load_elf() function more generic by not requiring an architecture.

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 (3):
loader: Allow ELF loader to auto-detect the ELF arch
generic-loader: Add a generic loader
docs: Add a generic loader explanation document

 MAINTAINERS                      |   6 ++
 docs/generic-loader.txt          |  54 +++++++++++++
 hw/core/Makefile.objs            |   2 +
 hw/core/generic-loader.c         | 170 +++++++++++++++++++++++++++++++++++++++
 hw/core/loader.c                 |  10 +++
 include/hw/core/generic-loader.h |  45 +++++++++++
 6 files changed, 287 insertions(+)
 create mode 100644 docs/generic-loader.txt
 create mode 100644 hw/core/generic-loader.c
 create mode 100644 include/hw/core/generic-loader.h

Signature · 1 line

2.7.4

Alistair Francis

author

2016-05-25 18:52 UTC #1 lore raw

PATCH v7 1/3 loader: Allow ELF loader to auto-detect the ELF arch

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

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

---
V7:
- Fix typo

 hw/core/loader.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

hw/core/loader.c +10 -0

index 53e0e41..a8a372d 100644
@@ -419,6 +419,7 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
 {
     int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
     uint8_t e_ident[EI_NIDENT];
+    uint16_t e_machine;
 
     fd = open(filename, O_RDONLY | O_BINARY);
     if (fd < 0) {

@@ -451,6 +452,15 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
         goto fail;
     }
 
+    if (elf_machine < 1) {
+        /* The caller didn't specify an ARCH, we can figure it out */
+        lseek(fd, 0x12, SEEK_SET);
+        if (read(fd, &e_machine, sizeof(e_machine)) != sizeof(e_machine)) {
+            goto fail;
+        }
+        elf_machine = e_machine;
+    }
+
     lseek(fd, 0, SEEK_SET);
     if (e_ident[EI_CLASS] == ELFCLASS64) {
         ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab,

Signature · 1 line

2.7.4

Peter Maydell

2016-06-09 17:39 UTC #2 lore raw

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

Quoted 25 lines · Alistair Francis

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

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

---
V7:
- Fix typo

 hw/core/loader.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

hw/core/loader.c +1 -0

index 53e0e41..a8a372d 100644
@@ -419,6 +419,7 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
 {
     int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
     uint8_t e_ident[EI_NIDENT];
+    uint16_t e_machine;
 
     fd = open(filename, O_RDONLY | O_BINARY);
     if (fd < 0) {

@@ -451,6 +452,15 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
         goto fail;

hw/core/loader.c int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t), · line 462

Quoted 10 lines

hw/core/loader.c +8 -0

     }
 
+    if (elf_machine < 1) {
+        /* The caller didn't specify an ARCH, we can figure it out */
+        lseek(fd, 0x12, SEEK_SET);
+        if (read(fd, &e_machine, sizeof(e_machine)) != sizeof(e_machine)) {
+            goto fail;
+        }
+        elf_machine = e_machine;
+    }

Isn't there an endianness problem here, given that e_machine is a 16 bit field? In load_elf32()/load_elf64() we will byteswap the e_machine field we read off the disk if must_swab is true, which will mean it won't match the value we've read here and not byteswapped.

I think you're better off pushing the "allow architecture to be unspecified" support down into load_elf32()/load_elf64(), where it can just become

    if (elf_machine < 1) {
        elf_machine = ehdr.e_machine;
    }

once the load_elf code has read and byteswapped the header for you.

thanks
-- PMM

Alistair Francis

author

2016-06-13 17:08 UTC #3 lore raw

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

Quoted 46 lines · Peter Maydell

Quoted 25 lines · Alistair Francis
If the caller didn't specify an architecture for the ELF machine the load_elf() function will auto detect it based on the ELF file.
Signed-off-by Alistair Francis <[email protected]>
---
V7:
- Fix typo
 hw/core/loader.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
hw/core/loader.c +1 -0
index 53e0e41..a8a372d 100644
@@ -419,6 +419,7 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
 {
     int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
     uint8_t e_ident[EI_NIDENT];
+    uint16_t e_machine;
 
     fd = open(filename, O_RDONLY | O_BINARY);
     if (fd < 0) {
@@ -451,6 +452,15 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
         goto fail;
hw/core/loader.c int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t), · line 462
Quoted 10 lines
hw/core/loader.c +8 -0
     }
 
+    if (elf_machine < 1) {
+        /* The caller didn't specify an ARCH, we can figure it out */
+        lseek(fd, 0x12, SEEK_SET);
+        if (read(fd, &e_machine, sizeof(e_machine)) != sizeof(e_machine)) {
+            goto fail;
+        }
+        elf_machine = e_machine;
+    }
Isn't there an endianness problem here, given that e_machine is a 16 bit field? In load_elf32()/load_elf64() we will byteswap the e_machine field we read off the disk if must_swab is true, which will mean it won't match the value we've read here and not byteswapped.
I think you're better off pushing the "allow architecture to be unspecified" support down into load_elf32()/load_elf64(), where it can just become

Quoted 5 lines

    if (elf_machine < 1) {
        elf_machine = ehdr.e_machine;
    }
once the load_elf code has read and byteswapped the header for you.

Good point, I didn't realise it was the same code for both. I have moved it into load_elf64/load_elf32.

Thanks,

Alistair

Quoted 4 lines

thanks
-- PMM

Alistair Francis

author

2016-05-25 18:52 UTC #4 lore raw

PATCH v7 2/3 generic-loader: Add a generic loader

In reply to Alistair Francis

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

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

---
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         | 170 +++++++++++++++++++++++++++++++++++++++
 include/hw/core/generic-loader.h |  45 +++++++++++
 4 files changed, 223 insertions(+)
 create mode 100644 hw/core/generic-loader.c
 create mode 100644 include/hw/core/generic-loader.h

MAINTAINERS +6 -0

index 81e7fac..b4a77d8 100644
@@ -972,6 +972,12 @@ F: hw/acpi/nvdimm.c
 F: hw/mem/nvdimm.c
 F: include/hw/mem/nvdimm.h
 
+Generic Loader
+M: Alistair Francis <[email protected]>
+S: Maintained
+F: hw/core/generic-loader.c
+F: include/hw/core/generic-loader.h
+
 Subsystems
 ----------
 Audio

hw/core/Makefile.objs +2 -0

index 70951d4..eb00e7d 100644
@@ -15,3 +15,5 @@ common-obj-$(CONFIG_SOFTMMU) += null-machine.o
 common-obj-$(CONFIG_SOFTMMU) += loader.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
 common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
+
+obj-$(CONFIG_SOFTMMU) += generic-loader.o

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

170 lines not on this page · raw message

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

45 lines not on this page · raw message

Signature · 1 line

2.7.4

Peter Maydell

2016-06-09 17:50 UTC #5 lore raw

Re: [Qemu-devel] [PATCH v7 2/3] generic-loader: Add a generic loader

Quoted 113 lines · Alistair Francis

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

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

 MAINTAINERS                      |   6 ++
 hw/core/Makefile.objs            |   2 +
 hw/core/generic-loader.c         | 170 +++++++++++++++++++++++++++++++++++++++
 include/hw/core/generic-loader.h |  45 +++++++++++
 4 files changed, 223 insertions(+)
 create mode 100644 hw/core/generic-loader.c
 create mode 100644 include/hw/core/generic-loader.h

MAINTAINERS +6 -0

index 81e7fac..b4a77d8 100644
@@ -972,6 +972,12 @@ F: hw/acpi/nvdimm.c
 F: hw/mem/nvdimm.c
 F: include/hw/mem/nvdimm.h
 
+Generic Loader
+M: Alistair Francis <[email protected]>
+S: Maintained
+F: hw/core/generic-loader.c
+F: include/hw/core/generic-loader.h
+
 Subsystems
 ----------
 Audio

hw/core/Makefile.objs +2 -0

index 70951d4..eb00e7d 100644
@@ -15,3 +15,5 @@ common-obj-$(CONFIG_SOFTMMU) += null-machine.o
 common-obj-$(CONFIG_SOFTMMU) += loader.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
 common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
+
+obj-$(CONFIG_SOFTMMU) += generic-loader.o

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

new file mode 100644
index 0000000..7160d58
@@ -0,0 +1,170 @@
+/*
+ * Generic Loader
+ *
+ * Copyright (C) 2014 Li Guang
+ * Copyright (C) 2016 Xilinx Inc.
+ * Written by Li Guang <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "qom/cpu.h"
+#include "hw/sysbus.h"
+#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);
+    }

hw/core/generic-loader.c line 55

Quoted 10 lines

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

+}
+
+static void generic_loader_realize(DeviceState *dev, Error **errp)
+{
+    GenericLoaderState *s = GENERIC_LOADER(dev);
+    hwaddr entry;
+    int big_endian;
+    int size = 0;
+
+    /* Perform some eror checking on the users options */

"error". "user's".

hw/core/generic-loader.c line 60

Quoted 5 lines

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

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

I think we don't generally have trailing '.' for error_setg() messages.

Quoted 52 lines

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

+            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->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;
+        }
+    }
+
+    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(s->file, NULL, NULL, &entry, NULL, NULL,
+                            big_endian, 0, 0, 0);
+
+            if (size < 0) {
+                size = load_uimage(s->file, &entry, NULL, NULL, NULL, NULL);
+            }
+        }
+
+        if (size < 0) {
+            /* 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;
+        }
+

hw/core/generic-loader.c line 122

Quoted 10 lines

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

+        if (size < 0) {
+            error_setg(errp, "Cannot load specified image %s", s->file);
+            return;
+        }
+    }
+
+    if (s->data_len && (s->data_len > sizeof(s->data))) {
+        error_setg(errp, "data-len cannot be more then the data size");
+        return;
+    }

This seems like it should go earlier, with the other parameter validation checks. (Also, isn't this actually checking "is data-len larger than our supported maximum of 8" ?)

Quoted 77 lines

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

+
+    /* 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_UINT16("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)

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

new file mode 100644
index 0000000..ceec51f
@@ -0,0 +1,45 @@
+/*
+ * Generic Loader
+ *
+ * Copyright (C) 2014 Li Guang
+ * Written by Li Guang <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#ifndef GENERIC_LOADER_H
+#define GENERIC_LOADER_H
+
+#include "elf.h"
+
+typedef struct GenericLoaderState {

include/hw/core/generic-loader.h line 33

Quoted 10 lines

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

+    /* <private> */
+    DeviceState parent_obj;
+
+    /* <public> */
+    CPUState *cpu;
+
+    uint64_t addr;
+    uint64_t data;
+    uint8_t data_len;
+    uint16_t cpu_num;

(Why 16 bits in particular?)

include/hw/core/generic-loader.h line 45

Quoted 12 lines

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

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

thanks
-- PMM

Alistair Francis

author

2016-06-13 17:25 UTC #6 lore raw

Re: [Qemu-devel] [PATCH v7 2/3] generic-loader: Add a generic loader

Quoted 118 lines · Peter Maydell

Quoted 117 lines · Alistair Francis

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

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

 MAINTAINERS                      |   6 ++
 hw/core/Makefile.objs            |   2 +
 hw/core/generic-loader.c         | 170 +++++++++++++++++++++++++++++++++++++++
 include/hw/core/generic-loader.h |  45 +++++++++++
 4 files changed, 223 insertions(+)
 create mode 100644 hw/core/generic-loader.c
 create mode 100644 include/hw/core/generic-loader.h

MAINTAINERS +6 -0

index 81e7fac..b4a77d8 100644
@@ -972,6 +972,12 @@ F: hw/acpi/nvdimm.c
 F: hw/mem/nvdimm.c
 F: include/hw/mem/nvdimm.h
 
+Generic Loader
+M: Alistair Francis <[email protected]>
+S: Maintained
+F: hw/core/generic-loader.c
+F: include/hw/core/generic-loader.h
+
 Subsystems
 ----------
 Audio

hw/core/Makefile.objs +2 -0

index 70951d4..eb00e7d 100644
@@ -15,3 +15,5 @@ common-obj-$(CONFIG_SOFTMMU) += null-machine.o
 common-obj-$(CONFIG_SOFTMMU) += loader.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
 common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
+
+obj-$(CONFIG_SOFTMMU) += generic-loader.o

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

new file mode 100644
index 0000000..7160d58
@@ -0,0 +1,170 @@
+/*
+ * Generic Loader
+ *
+ * Copyright (C) 2014 Li Guang
+ * Copyright (C) 2016 Xilinx Inc.
+ * Written by Li Guang <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "qom/cpu.h"
+#include "hw/sysbus.h"
+#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)
+{

Quoted 8 lines

hw/core/generic-loader.c line 55

Quoted 6 lines

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

+    GenericLoaderState *s = GENERIC_LOADER(dev);
+    hwaddr entry;
+    int big_endian;
+    int size = 0;
+
+    /* Perform some eror checking on the users options */

"error". "user's".

Fixed.

Quoted 8 lines

hw/core/generic-loader.c line 60

Quoted 6 lines

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

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

I think we don't generally have trailing '.' for error_setg() messages.

Ok, I have removed all the trailing '.'s.

Quoted 59 lines

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

+            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->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;
+        }
+    }
+
+    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(s->file, NULL, NULL, &entry, NULL, NULL,
+                            big_endian, 0, 0, 0);
+
+            if (size < 0) {
+                size = load_uimage(s->file, &entry, NULL, NULL, NULL, NULL);
+            }
+        }
+
+        if (size < 0) {
+            /* 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;
+        }
+    }
+

Quoted 8 lines

hw/core/generic-loader.c line 122
Quoted 4 lines
hw/core/generic-loader.c new file +4 -0
+    if (s->data_len && (s->data_len > sizeof(s->data))) {
+        error_setg(errp, "data-len cannot be more then the data size");
+        return;
+    }
This seems like it should go earlier, with the other parameter validation checks. (Also, isn't this actually checking "is data-len larger than our supported maximum of 8" ?)

I have moved this earlier and added a check for being greater then 8.

Quoted 82 lines

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

+
+    /* 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_UINT16("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)

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

new file mode 100644
index 0000000..ceec51f
@@ -0,0 +1,45 @@
+/*
+ * Generic Loader
+ *
+ * Copyright (C) 2014 Li Guang
+ * Written by Li Guang <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#ifndef GENERIC_LOADER_H
+#define GENERIC_LOADER_H
+
+#include "elf.h"
+
+typedef struct GenericLoaderState {
+    /* <private> */
+    DeviceState parent_obj;
+
+    /* <public> */

Quoted 8 lines

include/hw/core/generic-loader.h line 33
Quoted 6 lines
include/hw/core/generic-loader.h new file +6 -0
+    CPUState *cpu;
+
+    uint64_t addr;
+    uint64_t data;
+    uint8_t data_len;
+    uint16_t cpu_num;
(Why 16 bits in particular?)

No reason in particular. Do you think it should be something else?

Thanks,

Alistair

Quoted 17 lines

include/hw/core/generic-loader.h line 45

Quoted 13 lines

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

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

thanks
-- PMM

Peter Maydell

2016-06-13 18:05 UTC #7 lore raw

Re: [Qemu-devel] [PATCH v7 2/3] generic-loader: Add a generic loader

Quoted 7 lines · Alistair Francis

Quoted 4 lines · Peter Maydell
Quoted 1 line · Alistair Francis
+ uint16_t cpu_num;
(Why 16 bits in particular?)
No reason in particular. Do you think it should be something else?

Most other places that deal with cpu indexes use 'int'. There doesn't seem any pressing reason to limit it to a 16 bit value here. (You want uint32_t for migration purposes rather than just 'int' though.)

thanks
-- PMM

Alistair Francis

author

2016-05-25 18:52 UTC #8 lore raw

PATCH v7 3/3 docs: Add a generic loader explanation document

In reply to Alistair Francis

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

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

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

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

new file mode 100644
index 0000000..effb244
@@ -0,0 +1,54 @@
+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>
+
+NOTE: It is also possible to mix the commands above, e.g. include the cpu-num
+      argument with the data argument.
+
+    <addr>      - The address to store the data or the value to set the CPUs PC
+    <data>      - The value to be written to the addr. 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.
+
+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.
+
+An example of loading an ELF file which CPU0 will boot is shown below:
+    -device loader,file=./images/boot.elf,cpu-num=0

Signature · 1 line

2.7.4

Peter Maydell

2016-06-09 17:45 UTC #9 lore raw

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

Quoted 26 lines · Alistair Francis

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

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

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

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

new file mode 100644
index 0000000..effb244
@@ -0,0 +1,54 @@
+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.
+

docs/generic-loader.txt line 19

Quoted 10 lines

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

+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>
+
+NOTE: It is also possible to mix the commands above, e.g. include the cpu-num
+      argument with the data argument.

Is that actually useful?

docs/generic-loader.txt line 21

Quoted 2 lines

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

+
+    <addr>      - The address to store the data or the value to set the CPUs PC

"to use as the CPU's PC"

docs/generic-loader.txt line 22

Quoted 1 line

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

+    <data>      - The value to be written to the addr. The maximum size of the

Either "to the address" or "to <addr>".

docs/generic-loader.txt line 23

Quoted 1 line

docs/generic-loader.txt new file +1 -0
+                  data is 8 bytes.

We should say whether addresses and data are mandatorily hex, or if decimal is also allowed.

Quoted 16 lines

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

+    <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.
+
+An example of loading value 0x8000000e to address 0xfd1a0104 is:
+    -device loader,addr=0xfd1a0104,data=0x8000000e,data-len=4
+
+Loading Files
+-------------
+The loader device also allows files to be loaded into memory. This can be done
+similarly to setting memory values. The syntax is shown below:
+

docs/generic-loader.txt line 49

Quoted 10 lines

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

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

Does it actually set the PC to where the image is stored, or does it use the ELF entry point specified in the ELF file?

Does this also cause the loader to write via this CPU's address space? (If so, is it possible to load a file via a specific CPU's address space but not set its program counter?)

Quoted 8 lines

docs/generic-loader.txt new file +5 -1

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

thanks
-- PMM

Alistair Francis

author

2016-06-13 17:46 UTC #10 lore raw

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

Quoted 31 lines · Peter Maydell

Quoted 30 lines · Alistair Francis

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

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

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

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

new file mode 100644
index 0000000..effb244
@@ -0,0 +1,54 @@
+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:

Quoted 8 lines

docs/generic-loader.txt line 19

Quoted 6 lines

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

+
+    -device loader,addr=<addr>,data=<data>,data-len=<len>
+    -device loader,addr=<addr>,cpu-num=<cpu-num>
+
+NOTE: It is also possible to mix the commands above, e.g. include the cpu-num
+      argument with the data argument.

Is that actually useful?

Sometimes it is. You can use it to set data in other CPU address spaces.

It also is not forbidden in all the checks, so I think it is still worth mentioning.

Quoted 5 lines

docs/generic-loader.txt line 21
Quoted 3 lines
docs/generic-loader.txt new file +2 -0
+
+    <addr>      - The address to store the data or the value to set the CPUs PC
"to use as the CPU's PC"

Fixed.

Quoted 4 lines

docs/generic-loader.txt line 22
Quoted 2 lines
docs/generic-loader.txt new file +1 -0
+    <data>      - The value to be written to the addr. The maximum size of the
Either "to the address" or "to <addr>".

Fixed.

Quoted 5 lines

docs/generic-loader.txt line 23
Quoted 2 lines
docs/generic-loader.txt new file +1 -0
+                  data is 8 bytes.
We should say whether addresses and data are mandatorily hex, or if decimal is also allowed.

Both are supported. I have added documentation for the values.

Quoted 22 lines

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

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

Quoted 8 lines

docs/generic-loader.txt line 49
Quoted 5 lines
docs/generic-loader.txt new file +5 -0
+                  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.
Does it actually set the PC to where the image is stored, or does it use the ELF entry point specified in the ELF file?

The value in the elf file, I will correct that.

Quoted 2 lines

Does this also cause the loader to write via this CPU's address space?

The file loading does not follow the CPUs address spaces. It only sets the CPUs program counter.

The <data> writes follow the CPUs address spaces though.

Thanks,

Alistair

Quoted 15 lines

(If so, is it possible to load a file via a specific CPU's address space but not set its program counter?)
Quoted 8 lines
docs/generic-loader.txt new file +5 -1
+    <force-raw> - Forces the file to be treated as a raw image. This can be
+                  used to specify the load address of ELF files.
+
+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
thanks
-- PMM

Peter Maydell

2016-06-13 17:49 UTC #11 lore raw

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

Quoted 41 lines · Alistair Francis

Quoted 31 lines · Peter Maydell

Quoted 30 lines · Alistair Francis

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

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

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

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

new file mode 100644
index 0000000..effb244
@@ -0,0 +1,54 @@
+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:

Quoted 8 lines

docs/generic-loader.txt line 19

Quoted 6 lines

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

+
+    -device loader,addr=<addr>,data=<data>,data-len=<len>
+    -device loader,addr=<addr>,cpu-num=<cpu-num>
+
+NOTE: It is also possible to mix the commands above, e.g. include the cpu-num
+      argument with the data argument.

Is that actually useful?

Quoted 1 line

Sometimes it is. You can use it to set data in other CPU address spaces.

Right, but do you want it to do that and also set the PC of the other CPU at the same time? That's the bit that doesn't make sense to me. I think we should keep "set PC" and "write to memory" orthogonal, rather having an option combo that does both on the same data.

Quoted 2 lines

It also is not forbidden in all the checks, so I think it is still worth mentioning.

Better to forbid combinations that don't make sense rather than document them.

Quoted 6 lines

Quoted 1 line
Does this also cause the loader to write via this CPU's address space?
The file loading does not follow the CPUs address spaces. It only sets the CPUs program counter.
The <data> writes follow the CPUs address spaces though.

That's irritatingly inconsistent. Can we have both use the CPU's address space, please?

thanks
-- PMM

Alistair Francis

author

2016-06-13 20:18 UTC #12 lore raw

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

Quoted 49 lines · Peter Maydell

Quoted 41 lines · Alistair Francis
Quoted 31 lines · Peter Maydell
Quoted 30 lines · Alistair Francis
Signed-off-by Alistair Francis <[email protected]>
---
V6:
- Fixup documentation
V4:
- Re-write to be more comprehensive
 docs/generic-loader.txt | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 docs/generic-loader.txt
docs/generic-loader.txt new file +13 -0
new file mode 100644
index 0000000..effb244
@@ -0,0 +1,54 @@
+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:
Quoted 8 lines
docs/generic-loader.txt line 19
Quoted 6 lines
docs/generic-loader.txt new file +6 -0
+
+    -device loader,addr=<addr>,data=<data>,data-len=<len>
+    -device loader,addr=<addr>,cpu-num=<cpu-num>
+
+NOTE: It is also possible to mix the commands above, e.g. include the cpu-num
+      argument with the data argument.
Is that actually useful?
Quoted 1 line
Sometimes it is. You can use it to set data in other CPU address spaces.
Right, but do you want it to do that and also set the PC of the other CPU at the same time? That's the bit that doesn't make sense to me. I think we should keep "set PC" and "write to memory" orthogonal, rather having an option combo that does both on the same data.

Quoted 4 lines

Quoted 2 lines
It also is not forbidden in all the checks, so I think it is still worth mentioning.
Better to forbid combinations that don't make sense rather than document them.

Ok, I have done that.

Quoted 10 lines

Quoted 7 lines
Quoted 2 lines
Does this also cause the loader to write via this CPU's address space?
The file loading does not follow the CPUs address spaces. It only sets the CPUs program counter.
The <data> writes follow the CPUs address spaces though.
That's irritatingly inconsistent. Can we have both use the CPU's address space, please?

I don't see an easy way to set the address space for the multiple loader functions. Any ideas?

Thanks,

Alistair

Quoted 4 lines

thanks
-- PMM