[Qemu-devel] [PATCH v7 0/3] Add a generic loader

[Qemu-devel] [PATCH v7 0/3] 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 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

-- 
2.7.4

[Qemu-devel] [PATCH v7 1/3] 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>
---
V7:
 - Fix typo

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

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 53e0e41..a8a372d 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -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,
-- 
2.7.4

[Qemu-devel] [PATCH v7 2/3] 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>
---
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

diff --git a/MAINTAINERS b/MAINTAINERS
index 81e7fac..b4a77d8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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 <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 70951d4..eb00e7d 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -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
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
new file mode 100644
index 0000000..7160d58
--- /dev/null
+++ b/hw/core/generic-loader.c
@@ -0,0 +1,170 @@
+/*
+ * 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 eror checking on the users 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->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;
+        }
+    }
+
+    if (s->data_len && (s->data_len > sizeof(s->data))) {
+        error_setg(errp, "data-len cannot be more then the data size");
+        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_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)
diff --git a/include/hw/core/generic-loader.h b/include/hw/core/generic-loader.h
new file mode 100644
index 0000000..ceec51f
--- /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;
+    uint16_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 v7 3/3] docs: Add a generic loader explanation document

[Alistair Francis]

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

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

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

[Peter Maydell]

On 25 May 2016 at 19:49, 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>
> ---
> V7:
>  - Fix typo
>
>  hw/core/loader.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 53e0e41..a8a372d 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -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;
> +    }

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

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

[Peter Maydell]

On 25 May 2016 at 19:49, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 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
>
> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
> new file mode 100644
> index 0000000..effb244
> --- /dev/null
> +++ b/docs/generic-loader.txt
> @@ -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.

Is that actually useful?

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

"to use as the CPU's PC"

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

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

> +                  data is 8 bytes.

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

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

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

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

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

[Peter Maydell]

On 25 May 2016 at 19:49, 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>
> ---
> 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
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 81e7fac..b4a77d8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -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 <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 70951d4..eb00e7d 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -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
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> new file mode 100644
> index 0000000..7160d58
> --- /dev/null
> +++ b/hw/core/generic-loader.c
> @@ -0,0 +1,170 @@
> +/*
> + * 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 eror checking on the users options */

"error". "user's".

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

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

> +
> +    /* 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)
> diff --git a/include/hw/core/generic-loader.h b/include/hw/core/generic-loader.h
> new file mode 100644
> index 0000000..ceec51f
> --- /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;
> +    uint16_t cpu_num;

(Why 16 bits in particular?)

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

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

[Alistair Francis]

On Thu, Jun 9, 2016 at 10:38 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 May 2016 at 19:49, 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>
>> ---
>> V7:
>>  - Fix typo
>>
>>  hw/core/loader.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 53e0e41..a8a372d 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -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;
>> +    }
>
> 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.

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

Thanks,

Alistair

>
> thanks
> -- PMM
>

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

[Alistair Francis]

On Thu, Jun 9, 2016 at 10:50 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 May 2016 at 19:49, 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>
>> ---
>> 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
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 81e7fac..b4a77d8 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -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 <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 70951d4..eb00e7d 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -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
>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
>> new file mode 100644
>> index 0000000..7160d58
>> --- /dev/null
>> +++ b/hw/core/generic-loader.c
>> @@ -0,0 +1,170 @@
>> +/*
>> + * 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 eror checking on the users options */
>
> "error". "user's".

Fixed.

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

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

>
>> +
>> +    /* 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)
>> diff --git a/include/hw/core/generic-loader.h b/include/hw/core/generic-loader.h
>> new file mode 100644
>> index 0000000..ceec51f
>> --- /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;
>> +    uint16_t cpu_num;
>
> (Why 16 bits in particular?)

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

Thanks,

Alistair

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

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

[Alistair Francis]

On Thu, Jun 9, 2016 at 10:45 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 May 2016 at 19:49, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> 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
>>
>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>> new file mode 100644
>> index 0000000..effb244
>> --- /dev/null
>> +++ b/docs/generic-loader.txt
>> @@ -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.
>
> 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.

>
>> +
>> +    <addr>      - The address to store the data or the value to set the CPUs PC
>
> "to use as the CPU's PC"

Fixed.

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

Fixed.

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

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

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

> (If so, is it possible to load a file via a specific CPU's address space
> but not set its program counter?)
>
>> +    <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
>

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

[Peter Maydell]

On 13 June 2016 at 18:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Thu, Jun 9, 2016 at 10:45 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 25 May 2016 at 19:49, Alistair Francis <alistair.francis@xilinx.com> wrote:
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>> 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
>>>
>>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>>> new file mode 100644
>>> index 0000000..effb244
>>> --- /dev/null
>>> +++ b/docs/generic-loader.txt
>>> @@ -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.
>>
>> Is that actually useful?
>
> 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.

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

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

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

[Peter Maydell]

On 13 June 2016 at 18:25, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Thu, Jun 9, 2016 at 10:50 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 25 May 2016 at 19:49, Alistair Francis <alistair.francis@xilinx.com> wrote:
>>> +    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

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

[Alistair Francis]

On Mon, Jun 13, 2016 at 10:49 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 13 June 2016 at 18:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> On Thu, Jun 9, 2016 at 10:45 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 25 May 2016 at 19:49, Alistair Francis <alistair.francis@xilinx.com> wrote:
>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>> ---
>>>> 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
>>>>
>>>> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
>>>> new file mode 100644
>>>> index 0000000..effb244
>>>> --- /dev/null
>>>> +++ b/docs/generic-loader.txt
>>>> @@ -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.
>>>
>>> Is that actually useful?
>>
>> 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.
>
>> 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.

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

>
> thanks
> -- PMM
>