[Qemu-devel] [PULL 0/7] fw_cfg: add dma interface, add strings via cmdline.

[Qemu-devel] [PULL 0/7] fw_cfg: add dma interface, add strings via cmdline.

[Gerd Hoffmann]

  Hi,

As promised last week I've picked up the fw_cfg patches which are ready
to go, so here comes the pull request for them.  Headline feature is
the fw_cfg dam support fo course, enabled for arm and x86.  Also small
fixes and an option to add string fw_cfg entries via command line.

please pull,
  Gerd

The following changes since commit 40fe17bea478793fc9106a630fa3610dad51f939:

  hw/ide/ahci.c: Fix shift left into sign bit (2015-10-18 11:00:40 +0100)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/pull-fw_cfg-20151019-1

for you to fetch changes up to 7b0eec285d447057405df73beae78c8e8aeb9793:

  fw_cfg: Define a static signature to be returned on DMA port reads (2015-10-19 11:00:50 +0200)

----------------------------------------------------------------
fw_cfg: add dma interface, add strings via cmdline.

----------------------------------------------------------------
Gabriel L. Somlo (2):
      fw_cfg: insert string blobs via qemu cmdline
      fw_cfg: document fw_cfg_modify_iXX() update functions

Kevin O'Connor (1):
      fw_cfg: Define a static signature to be returned on DMA port reads

Marc Marí (4):
      fw_cfg DMA interface documentation
      Implement fw_cfg DMA interface
      Enable fw_cfg DMA interface for ARM
      Enable fw_cfg DMA interface for x86

 docs/specs/fw_cfg.txt     |  94 ++++++++++++++++-
 hw/arm/virt.c             |   8 +-
 hw/i386/pc.c              |   8 +-
 hw/nvram/fw_cfg.c         | 250 ++++++++++++++++++++++++++++++++++++++++++++--
 include/hw/nvram/fw_cfg.h |  16 ++-
 qemu-options.hx           |   7 +-
 vl.c                      |  33 ++++--
 7 files changed, 385 insertions(+), 31 deletions(-)

[Qemu-devel] [PULL 1/7] fw_cfg: insert string blobs via qemu cmdline

[Gerd Hoffmann]

From: "Gabriel L. Somlo" <somlo@cmu.edu>

Allow users to provide custom fw_cfg blobs with ascii string
payloads specified directly on the qemu command line.

Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Message-id: 1443544141-26568-1-git-send-email-somlo@cmu.edu
Reviewd-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 docs/specs/fw_cfg.txt | 15 +++++++++++++++
 qemu-options.hx       |  7 ++++++-
 vl.c                  | 33 +++++++++++++++++++++++++++------
 3 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 74351dd..c0e76aa 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -216,6 +216,21 @@ the following syntax:
 where <item_name> is the fw_cfg item name, and <path> is the location
 on the host file system of a file containing the data to be inserted.
 
+Small enough items may be provided directly as strings on the command
+line, using the syntax:
+
+    -fw_cfg [name=]<item_name>,string=<string>
+
+The terminating NUL character of the content <string> will NOT be
+included as part of the fw_cfg item data, which is consistent with
+the absence of a NUL terminator for items inserted via the file option.
+
+Both <item_name> and, if applicable, the content <string> are passed
+through by QEMU without any interpretation, expansion, or further
+processing. Any such processing (potentially performed e.g., by the shell)
+is outside of QEMU's responsibility; as such, using plain ASCII characters
+is recommended.
+
 NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/"
 when using the "-fw_cfg" command line option, to avoid conflicting with
 item names used internally by QEMU. For instance:
diff --git a/qemu-options.hx b/qemu-options.hx
index 2485b94..edee5f4 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2724,13 +2724,18 @@ ETEXI
 
 DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
     "-fw_cfg [name=]<name>,file=<file>\n"
-    "                add named fw_cfg entry from file\n",
+    "                add named fw_cfg entry from file\n"
+    "-fw_cfg [name=]<name>,string=<str>\n"
+    "                add named fw_cfg entry from string\n",
     QEMU_ARCH_ALL)
 STEXI
 @item -fw_cfg [name=]@var{name},file=@var{file}
 @findex -fw_cfg
 Add named fw_cfg entry from file. @var{name} determines the name of
 the entry in the fw_cfg file directory exposed to the guest.
+
+@item -fw_cfg [name=]@var{name},string=@var{str}
+Add named fw_cfg entry from string.
 ETEXI
 
 DEF("serial", HAS_ARG, QEMU_OPTION_serial, \
diff --git a/vl.c b/vl.c
index 7c806a2..332d828 100644
--- a/vl.c
+++ b/vl.c
@@ -512,6 +512,10 @@ static QemuOptsList qemu_fw_cfg_opts = {
             .type = QEMU_OPT_STRING,
             .help = "Sets the name of the file from which\n"
                     "the fw_cfg blob will be loaded",
+        }, {
+            .name = "string",
+            .type = QEMU_OPT_STRING,
+            .help = "Sets content of the blob to be inserted from a string",
         },
         { /* end of list */ }
     },
@@ -2239,11 +2243,16 @@ char *qemu_find_file(int type, const char *name)
     return NULL;
 }
 
+static inline bool nonempty_str(const char *str)
+{
+    return str && *str;
+}
+
 static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
 {
     gchar *buf;
     size_t size;
-    const char *name, *file;
+    const char *name, *file, *str;
 
     if (opaque == NULL) {
         error_report("fw_cfg device not available");
@@ -2251,8 +2260,15 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
     }
     name = qemu_opt_get(opts, "name");
     file = qemu_opt_get(opts, "file");
-    if (name == NULL || *name == '\0' || file == NULL || *file == '\0') {
-        error_report("invalid argument value");
+    str = qemu_opt_get(opts, "string");
+
+    /* we need name and either a file or the content string */
+    if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) {
+        error_report("invalid argument(s)");
+        return -1;
+    }
+    if (nonempty_str(file) && nonempty_str(str)) {
+        error_report("file and string are mutually exclusive");
         return -1;
     }
     if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
@@ -2263,9 +2279,14 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
         error_report("WARNING: externally provided fw_cfg item names "
                      "should be prefixed with \"opt/\"!");
     }
-    if (!g_file_get_contents(file, &buf, &size, NULL)) {
-        error_report("can't load %s", file);
-        return -1;
+    if (nonempty_str(str)) {
+        size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
+        buf = g_memdup(str, size);
+    } else {
+        if (!g_file_get_contents(file, &buf, &size, NULL)) {
+            error_report("can't load %s", file);
+            return -1;
+        }
     }
     fw_cfg_add_file((FWCfgState *)opaque, name, buf, size);
     return 0;
-- 
1.8.3.1

[Qemu-devel] [PULL 2/7] fw_cfg: document fw_cfg_modify_iXX() update functions

[Gerd Hoffmann]

From: "Gabriel L. Somlo" <somlo@cmu.edu>

Document the behavior of fw_cfg_modify_iXX() for leak-less updating
of integer-type blobs.

Currently only fw_cfg_modify_i16() is coded, but 32- and 64-bit versions
may be added later if necessary..

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 docs/specs/fw_cfg.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index c0e76aa..d5dee4b 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -159,6 +159,17 @@ will convert a 16-, 32-, or 64-bit integer to little-endian, then add
 a dynamically allocated copy of the appropriately sized item to fw_cfg
 under the given selector key value.
 
+== fw_cfg_modify_iXX() ==
+
+Modify the value of an XX-bit item (where XX may be 16, 32, or 64).
+Similarly to the corresponding fw_cfg_add_iXX() function set, convert
+a 16-, 32-, or 64-bit integer to little endian, create a dynamically
+allocated copy of the required size, and replace the existing item at
+the given selector key value with the newly allocated one. The previous
+item, assumed to have been allocated during an earlier call to
+fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed
+before the function returns.
+
 == fw_cfg_add_file() ==
 
 Given a filename (i.e., fw_cfg item name), starting pointer, and size,
-- 
1.8.3.1

[Qemu-devel] [PULL 3/7] fw_cfg DMA interface documentation

[Gerd Hoffmann]

From: Marc Marí <markmb@redhat.com>

Add fw_cfg DMA interface specification in the documentation.

Based on Gerd Hoffman's initial implementation.

Signed-off-by: Marc Marí <markmb@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 docs/specs/fw_cfg.txt | 65 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 61 insertions(+), 4 deletions(-)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index d5dee4b..536909a 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -76,6 +76,13 @@ increasing address order, similar to memcpy().
 
 Selector Register IOport: 0x510
 Data Register IOport:     0x511
+DMA Address IOport:       0x514
+
+=== ARM Register Locations ===
+
+Selector Register address: Base + 8 (2 bytes)
+Data Register address:     Base + 0 (8 bytes)
+DMA Address address:       Base + 16 (8 bytes)
 
 == Firmware Configuration Items ==
 
@@ -86,11 +93,12 @@ by selecting the "signature" item using key 0x0000 (FW_CFG_SIGNATURE),
 and reading four bytes from the data register. If the fw_cfg device is
 present, the four bytes read will contain the characters "QEMU".
 
-=== Revision (Key 0x0001, FW_CFG_ID) ===
+=== Revision / feature bitmap (Key 0x0001, FW_CFG_ID) ===
 
-A 32-bit little-endian unsigned int, this item is used as an interface
-revision number, and is currently set to 1 by QEMU when fw_cfg is
-initialized.
+A 32-bit little-endian unsigned int, this item is used to check for enabled
+features.
+ - Bit 0: traditional interface. Always set.
+ - Bit 1: DMA interface.
 
 === File Directory (Key 0x0019, FW_CFG_FILE_DIR) ===
 
@@ -132,6 +140,55 @@ Selector Reg.    Range Usage
 In practice, the number of allowed firmware configuration items is given
 by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
 
+= Guest-side DMA Interface =
+
+If bit 1 of the feature bitmap is set, the DMA interface is present. This does
+not replace the existing fw_cfg interface, it is an add-on. This interface
+can be used through the 64-bit wide address register.
+
+The address register is in big-endian format. The value for the register is 0
+at startup and after an operation. A write to the least significant half (at
+offset 4) triggers an operation. This means that operations with 32-bit
+addresses can be triggered with just one write, whereas operations with
+64-bit addresses can be triggered with one 64-bit write or two 32-bit writes,
+starting with the most significant half (at offset 0).
+
+In this register, the physical address of a FWCfgDmaAccess structure in RAM
+should be written. This is the format of the FWCfgDmaAccess structure:
+
+typedef struct FWCfgDmaAccess {
+    uint32_t control;
+    uint32_t length;
+    uint64_t address;
+} FWCfgDmaAccess;
+
+The fields of the structure are in big endian mode, and the field at the lowest
+address is the "control" field.
+
+The "control" field has the following bits:
+ - Bit 0: Error
+ - Bit 1: Read
+ - Bit 2: Skip
+ - Bit 3: Select. The upper 16 bits are the selected index.
+
+When an operation is triggered, if the "control" field has bit 3 set, the
+upper 16 bits are interpreted as an index of a firmware configuration item.
+This has the same effect as writing the selector register.
+
+If the "control" field has bit 1 set, a read operation will be performed.
+"length" bytes for the current selector and offset will be copied into the
+physical RAM address specified by the "address" field.
+
+If the "control" field has bit 2 set (and not bit 1), a skip operation will be
+performed. The offset for the current selector will be advanced "length" bytes.
+
+To check the result, read the "control" field:
+   error bit set        ->  something went wrong.
+   all bits cleared     ->  transfer finished successfully.
+   otherwise            ->  transfer still in progress (doesn't happen
+                            today due to implementation not being async,
+                            but may in the future).
+
 = Host-side API =
 
 The following functions are available to the QEMU programmer for adding
-- 
1.8.3.1

[Qemu-devel] [PULL 4/7] Implement fw_cfg DMA interface

[Gerd Hoffmann]

From: Marc Marí <markmb@redhat.com>

Based on the specifications on docs/specs/fw_cfg.txt

This interface is an addon. The old interface can still be used as usual.

Based on Gerd Hoffman's initial implementation.

Signed-off-by: Marc Marí <markmb@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/arm/virt.c             |   2 +-
 hw/nvram/fw_cfg.c         | 240 +++++++++++++++++++++++++++++++++++++++++++---
 include/hw/nvram/fw_cfg.h |  16 +++-
 3 files changed, 244 insertions(+), 14 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4e7160c..b670bc6 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -683,7 +683,7 @@ static void create_fw_cfg(const VirtBoardInfo *vbi)
     hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
     char *nodename;
 
-    fw_cfg_init_mem_wide(base + 8, base, 8);
+    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
 
     nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 658f8c4..91829d4 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -23,6 +23,7 @@
  */
 #include "hw/hw.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/dma.h"
 #include "hw/isa/isa.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/sysbus.h"
@@ -30,7 +31,7 @@
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 
-#define FW_CFG_SIZE 2
+#define FW_CFG_CTL_SIZE 2
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
@@ -42,6 +43,16 @@
 #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
 #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
 
+/* FW_CFG_VERSION bits */
+#define FW_CFG_VERSION      0x01
+#define FW_CFG_VERSION_DMA  0x02
+
+/* FW_CFG_DMA_CONTROL bits */
+#define FW_CFG_DMA_CTL_ERROR   0x01
+#define FW_CFG_DMA_CTL_READ    0x02
+#define FW_CFG_DMA_CTL_SKIP    0x04
+#define FW_CFG_DMA_CTL_SELECT  0x08
+
 typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
@@ -59,6 +70,11 @@ struct FWCfgState {
     uint16_t cur_entry;
     uint32_t cur_offset;
     Notifier machine_ready;
+
+    bool dma_enabled;
+    dma_addr_t dma_addr;
+    AddressSpace *dma_as;
+    MemoryRegion dma_iomem;
 };
 
 struct FWCfgIoState {
@@ -67,7 +83,7 @@ struct FWCfgIoState {
     /*< public >*/
 
     MemoryRegion comb_iomem;
-    uint32_t iobase;
+    uint32_t iobase, dma_iobase;
 };
 
 struct FWCfgMemState {
@@ -292,6 +308,122 @@ static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
     } while (i);
 }
 
+static void fw_cfg_dma_transfer(FWCfgState *s)
+{
+    dma_addr_t len;
+    FWCfgDmaAccess dma;
+    int arch;
+    FWCfgEntry *e;
+    int read;
+    dma_addr_t dma_addr;
+
+    /* Reset the address before the next access */
+    dma_addr = s->dma_addr;
+    s->dma_addr = 0;
+
+    if (dma_memory_read(s->dma_as, dma_addr, &dma, sizeof(dma))) {
+        stl_be_dma(s->dma_as, dma_addr + offsetof(FWCfgDmaAccess, control),
+                   FW_CFG_DMA_CTL_ERROR);
+        return;
+    }
+
+    dma.address = be64_to_cpu(dma.address);
+    dma.length = be32_to_cpu(dma.length);
+    dma.control = be32_to_cpu(dma.control);
+
+    if (dma.control & FW_CFG_DMA_CTL_SELECT) {
+        fw_cfg_select(s, dma.control >> 16);
+    }
+
+    arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
+    e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
+
+    if (dma.control & FW_CFG_DMA_CTL_READ) {
+        read = 1;
+    } else if (dma.control & FW_CFG_DMA_CTL_SKIP) {
+        read = 0;
+    } else {
+        dma.length = 0;
+    }
+
+    dma.control = 0;
+
+    while (dma.length > 0 && !(dma.control & FW_CFG_DMA_CTL_ERROR)) {
+        if (s->cur_entry == FW_CFG_INVALID || !e->data ||
+                                s->cur_offset >= e->len) {
+            len = dma.length;
+
+            /* If the access is not a read access, it will be a skip access,
+             * tested before.
+             */
+            if (read) {
+                if (dma_memory_set(s->dma_as, dma.address, 0, len)) {
+                    dma.control |= FW_CFG_DMA_CTL_ERROR;
+                }
+            }
+
+        } else {
+            if (dma.length <= (e->len - s->cur_offset)) {
+                len = dma.length;
+            } else {
+                len = (e->len - s->cur_offset);
+            }
+
+            if (e->read_callback) {
+                e->read_callback(e->callback_opaque, s->cur_offset);
+            }
+
+            /* If the access is not a read access, it will be a skip access,
+             * tested before.
+             */
+            if (read) {
+                if (dma_memory_write(s->dma_as, dma.address,
+                                    &e->data[s->cur_offset], len)) {
+                    dma.control |= FW_CFG_DMA_CTL_ERROR;
+                }
+            }
+
+            s->cur_offset += len;
+        }
+
+        dma.address += len;
+        dma.length  -= len;
+
+    }
+
+    stl_be_dma(s->dma_as, dma_addr + offsetof(FWCfgDmaAccess, control),
+                dma.control);
+
+    trace_fw_cfg_read(s, 0);
+}
+
+static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
+                                 uint64_t value, unsigned size)
+{
+    FWCfgState *s = opaque;
+
+    if (size == 4) {
+        if (addr == 0) {
+            /* FWCfgDmaAccess high address */
+            s->dma_addr = value << 32;
+        } else if (addr == 4) {
+            /* FWCfgDmaAccess low address */
+            s->dma_addr |= value;
+            fw_cfg_dma_transfer(s);
+        }
+    } else if (size == 8 && addr == 0) {
+        s->dma_addr = value;
+        fw_cfg_dma_transfer(s);
+    }
+}
+
+static bool fw_cfg_dma_mem_valid(void *opaque, hwaddr addr,
+                                  unsigned size, bool is_write)
+{
+    return is_write && ((size == 4 && (addr == 0 || addr == 4)) ||
+                        (size == 8 && addr == 0));
+}
+
 static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
@@ -359,6 +491,14 @@ static const MemoryRegionOps fw_cfg_comb_mem_ops = {
     .valid.accepts = fw_cfg_comb_valid,
 };
 
+static const MemoryRegionOps fw_cfg_dma_mem_ops = {
+    .write = fw_cfg_dma_mem_write,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .valid.accepts = fw_cfg_dma_mem_valid,
+    .valid.max_access_size = 8,
+    .impl.max_access_size = 8,
+};
+
 static void fw_cfg_reset(DeviceState *d)
 {
     FWCfgState *s = FW_CFG(d);
@@ -399,6 +539,22 @@ static bool is_version_1(void *opaque, int version_id)
     return version_id == 1;
 }
 
+static bool fw_cfg_dma_enabled(void *opaque)
+{
+    FWCfgState *s = opaque;
+
+    return s->dma_enabled;
+}
+
+static const VMStateDescription vmstate_fw_cfg_dma = {
+    .name = "fw_cfg/dma",
+    .needed = fw_cfg_dma_enabled,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(dma_addr, FWCfgState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static const VMStateDescription vmstate_fw_cfg = {
     .name = "fw_cfg",
     .version_id = 2,
@@ -408,6 +564,10 @@ static const VMStateDescription vmstate_fw_cfg = {
         VMSTATE_UINT16_HACK(cur_offset, FWCfgState, is_version_1),
         VMSTATE_UINT32_V(cur_offset, FWCfgState, 2),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription*[]) {
+        &vmstate_fw_cfg_dma,
+        NULL,
     }
 };
 
@@ -593,7 +753,6 @@ static void fw_cfg_init1(DeviceState *dev)
     qdev_init_nofail(dev);
 
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
-    fw_cfg_add_i32(s, FW_CFG_ID, 1);
     fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));
     fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
@@ -605,25 +764,53 @@ static void fw_cfg_init1(DeviceState *dev)
     qemu_add_machine_init_done_notifier(&s->machine_ready);
 }
 
-FWCfgState *fw_cfg_init_io(uint32_t iobase)
+FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
+                                AddressSpace *dma_as)
 {
     DeviceState *dev;
+    FWCfgState *s;
+    uint32_t version = FW_CFG_VERSION;
+    bool dma_enabled = dma_iobase && dma_as;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_IO);
     qdev_prop_set_uint32(dev, "iobase", iobase);
+    qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
+    qdev_prop_set_bit(dev, "dma_enabled", dma_enabled);
+
     fw_cfg_init1(dev);
+    s = FW_CFG(dev);
 
-    return FW_CFG(dev);
+    if (dma_enabled) {
+        /* 64 bits for the address field */
+        s->dma_as = dma_as;
+        s->dma_addr = 0;
+
+        version |= FW_CFG_VERSION_DMA;
+    }
+
+    fw_cfg_add_i32(s, FW_CFG_ID, version);
+
+    return s;
+}
+
+FWCfgState *fw_cfg_init_io(uint32_t iobase)
+{
+    return fw_cfg_init_io_dma(iobase, 0, NULL);
 }
 
-FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
-                                 uint32_t data_width)
+FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
+                                 hwaddr data_addr, uint32_t data_width,
+                                 hwaddr dma_addr, AddressSpace *dma_as)
 {
     DeviceState *dev;
     SysBusDevice *sbd;
+    FWCfgState *s;
+    uint32_t version = FW_CFG_VERSION;
+    bool dma_enabled = dma_addr && dma_as;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
     qdev_prop_set_uint32(dev, "data_width", data_width);
+    qdev_prop_set_bit(dev, "dma_enabled", dma_enabled);
 
     fw_cfg_init1(dev);
 
@@ -631,13 +818,25 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
     sysbus_mmio_map(sbd, 0, ctl_addr);
     sysbus_mmio_map(sbd, 1, data_addr);
 
-    return FW_CFG(dev);
+    s = FW_CFG(dev);
+
+    if (dma_enabled) {
+        s->dma_as = dma_as;
+        s->dma_addr = 0;
+        sysbus_mmio_map(sbd, 2, dma_addr);
+        version |= FW_CFG_VERSION_DMA;
+    }
+
+    fw_cfg_add_i32(s, FW_CFG_ID, version);
+
+    return s;
 }
 
 FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
 {
     return fw_cfg_init_mem_wide(ctl_addr, data_addr,
-                                fw_cfg_data_mem_ops.valid.max_access_size);
+                                fw_cfg_data_mem_ops.valid.max_access_size,
+                                0, NULL);
 }
 
 
@@ -664,6 +863,9 @@ static const TypeInfo fw_cfg_info = {
 
 static Property fw_cfg_io_properties[] = {
     DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
+    DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
+    DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
+                     false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -673,8 +875,15 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
     memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
-                          FW_CFG(s), "fwcfg", FW_CFG_SIZE);
+                          FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
     sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
+
+    if (FW_CFG(s)->dma_enabled) {
+        memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
+                              &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
+                              sizeof(dma_addr_t));
+        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
+    }
 }
 
 static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
@@ -695,6 +904,8 @@ static const TypeInfo fw_cfg_io_info = {
 
 static Property fw_cfg_mem_properties[] = {
     DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
+    DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
+                     false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -705,7 +916,7 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
     const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
 
     memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
-                          FW_CFG(s), "fwcfg.ctl", FW_CFG_SIZE);
+                          FW_CFG(s), "fwcfg.ctl", FW_CFG_CTL_SIZE);
     sysbus_init_mmio(sbd, &s->ctl_iomem);
 
     if (s->data_width > data_ops->valid.max_access_size) {
@@ -723,6 +934,13 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
     memory_region_init_io(&s->data_iomem, OBJECT(s), data_ops, FW_CFG(s),
                           "fwcfg.data", data_ops->valid.max_access_size);
     sysbus_init_mmio(sbd, &s->data_iomem);
+
+    if (FW_CFG(s)->dma_enabled) {
+        memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
+                              &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
+                              sizeof(dma_addr_t));
+        sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
+    }
 }
 
 static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index e60d3ca..ee0cd8a 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -61,6 +61,15 @@ typedef struct FWCfgFiles {
     FWCfgFile f[];
 } FWCfgFiles;
 
+/* Control as first field allows for different structures selected by this
+ * field, which might be useful in the future
+ */
+typedef struct FWCfgDmaAccess {
+    uint32_t control;
+    uint32_t length;
+    uint64_t address;
+} QEMU_PACKED FWCfgDmaAccess;
+
 typedef void (*FWCfgCallback)(void *opaque, uint8_t *data);
 typedef void (*FWCfgReadCallback)(void *opaque, uint32_t offset);
 
@@ -77,10 +86,13 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
                               void *data, size_t len);
 void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
                          size_t len);
+FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
+                                AddressSpace *dma_as);
 FWCfgState *fw_cfg_init_io(uint32_t iobase);
 FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr);
-FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
-                                 uint32_t data_width);
+FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
+                                 hwaddr data_addr, uint32_t data_width,
+                                 hwaddr dma_addr, AddressSpace *dma_as);
 
 FWCfgState *fw_cfg_find(void);
 
-- 
1.8.3.1

[Qemu-devel] [PULL 5/7] Enable fw_cfg DMA interface for ARM

[Gerd Hoffmann]

From: Marc Marí <markmb@redhat.com>

Enable the fw_cfg DMA interface for the ARM virt machine.

Based on Gerd Hoffman's initial implementation.

Signed-off-by: Marc Marí <markmb@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/arm/virt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b670bc6..5d38c47 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -119,7 +119,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_GIC_REDIST] =         { 0x080A0000, 0x00F60000 },
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
-    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
+    [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -677,13 +677,13 @@ static void create_flash(const VirtBoardInfo *vbi)
     g_free(nodename);
 }
 
-static void create_fw_cfg(const VirtBoardInfo *vbi)
+static void create_fw_cfg(const VirtBoardInfo *vbi, AddressSpace *as)
 {
     hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
     hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
     char *nodename;
 
-    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
+    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
 
     nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
@@ -1031,7 +1031,7 @@ static void machvirt_init(MachineState *machine)
      */
     create_virtio_devices(vbi, pic);
 
-    create_fw_cfg(vbi);
+    create_fw_cfg(vbi, &address_space_memory);
     rom_set_fw(fw_cfg_find());
 
     guest_info->smp_cpus = smp_cpus;
-- 
1.8.3.1

[Qemu-devel] [PULL 6/7] Enable fw_cfg DMA interface for x86

[Gerd Hoffmann]

From: Marc Marí <markmb@redhat.com>

Enable the fw_cfg DMA interface for all the x86 platforms.

Based on Gerd Hoffman's initial implementation.

Signed-off-by: Marc Marí <markmb@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/i386/pc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 682867a..b25a872 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -752,14 +752,15 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
     }
 }
 
-static FWCfgState *bochs_bios_init(void)
+static FWCfgState *bochs_bios_init(AddressSpace *as)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;
     int i, j;
     unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
 
-    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
+    fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 4, as);
+
     /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
      *
      * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU hotplug
@@ -1393,7 +1394,8 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = bochs_bios_init();
+    fw_cfg = bochs_bios_init(&address_space_memory);
+
     rom_set_fw(fw_cfg);
 
     if (guest_info->has_reserved_memory && pcms->hotplug_memory.base) {
-- 
1.8.3.1

[Qemu-devel] [PULL 7/7] fw_cfg: Define a static signature to be returned on DMA port reads

[Gerd Hoffmann]

From: Kevin O'Connor <kevin@koconnor.net>

Return a static signature ("QEMU CFG") if the guest does a read to the
DMA address io register.

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 docs/specs/fw_cfg.txt |  3 +++
 hw/nvram/fw_cfg.c     | 14 ++++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 536909a..b8c794f 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -93,6 +93,9 @@ by selecting the "signature" item using key 0x0000 (FW_CFG_SIGNATURE),
 and reading four bytes from the data register. If the fw_cfg device is
 present, the four bytes read will contain the characters "QEMU".
 
+If the DMA interface is available, then reading the DMA Address
+Register returns 0x51454d5520434647 ("QEMU CFG" in big-endian format).
+
 === Revision / feature bitmap (Key 0x0001, FW_CFG_ID) ===
 
 A 32-bit little-endian unsigned int, this item is used to check for enabled
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 91829d4..69d674f 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -53,6 +53,8 @@
 #define FW_CFG_DMA_CTL_SKIP    0x04
 #define FW_CFG_DMA_CTL_SELECT  0x08
 
+#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647 /* "QEMU CFG" */
+
 typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
@@ -397,6 +399,13 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
     trace_fw_cfg_read(s, 0);
 }
 
+static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr,
+                                    unsigned size)
+{
+    /* Return a signature value (and handle various read sizes) */
+    return extract64(FW_CFG_DMA_SIGNATURE, (8 - addr - size) * 8, size * 8);
+}
+
 static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
                                  uint64_t value, unsigned size)
 {
@@ -420,8 +429,8 @@ static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
 static bool fw_cfg_dma_mem_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
-    return is_write && ((size == 4 && (addr == 0 || addr == 4)) ||
-                        (size == 8 && addr == 0));
+    return !is_write || ((size == 4 && (addr == 0 || addr == 4)) ||
+                         (size == 8 && addr == 0));
 }
 
 static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
@@ -492,6 +501,7 @@ static const MemoryRegionOps fw_cfg_comb_mem_ops = {
 };
 
 static const MemoryRegionOps fw_cfg_dma_mem_ops = {
+    .read = fw_cfg_dma_mem_read,
     .write = fw_cfg_dma_mem_write,
     .endianness = DEVICE_BIG_ENDIAN,
     .valid.accepts = fw_cfg_dma_mem_valid,
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 0/7] fw_cfg: add dma interface, add strings via cmdline.

[Peter Maydell]

On 19 October 2015 at 10:17, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> As promised last week I've picked up the fw_cfg patches which are ready
> to go, so here comes the pull request for them.  Headline feature is
> the fw_cfg dam support fo course, enabled for arm and x86.  Also small
> fixes and an option to add string fw_cfg entries via command line.
>
> please pull,
>   Gerd
>
> The following changes since commit 40fe17bea478793fc9106a630fa3610dad51f939:
>
>   hw/ide/ahci.c: Fix shift left into sign bit (2015-10-18 11:00:40 +0100)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/pull-fw_cfg-20151019-1
>
> for you to fetch changes up to 7b0eec285d447057405df73beae78c8e8aeb9793:
>
>   fw_cfg: Define a static signature to be returned on DMA port reads (2015-10-19 11:00:50 +0200)
>
> ----------------------------------------------------------------
> fw_cfg: add dma interface, add strings via cmdline.
>
> ----------------------------------------------------------------

Hi. I'm afraid this fails 'make check':

TEST: tests/fw_cfg-test... (pid=17533)
  /i386/fw_cfg/signature:                                              OK
  /i386/fw_cfg/id:                                                     **
ERROR:/home/petmay01/qemu/tests/fw_cfg-test.c:40:test_fw_cfg_id:
assertion failed (qfw_cfg_get_u32(fw_cfg, FW_CFG_ID) == 1): (3 == 1)
FAIL

(same failure on 64-bit ARM, ppc64be, OSX, 32-bit ARM).

Windows fails to compile:
/home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c: In function
‘fw_cfg_dma_mem_read’:
/home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c:406: warning:
integer constant is too large for ‘long’ type

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/7] fw_cfg: add dma interface, add strings via cmdline.

[Kevin O'Connor]

On Mon, Oct 19, 2015 at 12:12:34PM +0100, Peter Maydell wrote:
> On 19 October 2015 at 10:17, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > ----------------------------------------------------------------
> > fw_cfg: add dma interface, add strings via cmdline.
> >
> > ----------------------------------------------------------------
> 
> Hi. I'm afraid this fails 'make check':
> 
> TEST: tests/fw_cfg-test... (pid=17533)
>   /i386/fw_cfg/signature:                                              OK
>   /i386/fw_cfg/id:                                                     **
> ERROR:/home/petmay01/qemu/tests/fw_cfg-test.c:40:test_fw_cfg_id:
> assertion failed (qfw_cfg_get_u32(fw_cfg, FW_CFG_ID) == 1): (3 == 1)
> FAIL
> 
> (same failure on 64-bit ARM, ppc64be, OSX, 32-bit ARM).
> 
> Windows fails to compile:
> /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c: In function
> ‘fw_cfg_dma_mem_read’:
> /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c:406: warning:
> integer constant is too large for ‘long’ type

I don't have a Windows test environment, but I suspect the following:

#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647 /* "QEMU CFG" */

should be changed to:

#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */

If that sounds right, I'll respin the last patch.

-Kevin

Re: [Qemu-devel] [PULL 0/7] fw_cfg: add dma interface, add strings via cmdline.

[Peter Maydell]

On 19 October 2015 at 12:52, Kevin O'Connor <kevin@koconnor.net> wrote:
> On Mon, Oct 19, 2015 at 12:12:34PM +0100, Peter Maydell wrote:
>> Windows fails to compile:
>> /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c: In function
>> ‘fw_cfg_dma_mem_read’:
>> /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c:406: warning:
>> integer constant is too large for ‘long’ type
>
> I don't have a Windows test environment, but I suspect the following:
>
> #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647 /* "QEMU CFG" */
>
> should be changed to:
>
> #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */

Yes. All 64 bit constants should have an LL or ULL suffix.

(The test failure is presumably something different.)

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/7] fw_cfg: add dma interface, add strings via cmdline.

[Kevin O'Connor]

On Mon, Oct 19, 2015 at 01:02:41PM +0100, Peter Maydell wrote:
> On 19 October 2015 at 12:52, Kevin O'Connor <kevin@koconnor.net> wrote:
> > On Mon, Oct 19, 2015 at 12:12:34PM +0100, Peter Maydell wrote:
> >> Windows fails to compile:
> >> /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c: In function
> >> ‘fw_cfg_dma_mem_read’:
> >> /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c:406: warning:
> >> integer constant is too large for ‘long’ type
> >
> > I don't have a Windows test environment, but I suspect the following:
> >
> > #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647 /* "QEMU CFG" */
> >
> > should be changed to:
> >
> > #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
> 
> Yes. All 64 bit constants should have an LL or ULL suffix.

Okay - I'll respin patch 7.

> (The test failure is presumably something different.)

I think the fix for the test failure is to make the test less picky
(it should accept a feature bitmap of either 1 or 3).  But I'll leave
that to Marc or Gerd.

-Kevin

Re: [Qemu-devel] [PULL 0/7] fw_cfg: add dma interface, add strings via cmdline.

[Marc Marí]

On Mon, 19 Oct 2015 08:12:22 -0400
"Kevin O'Connor" <kevin@koconnor.net> wrote:

> On Mon, Oct 19, 2015 at 01:02:41PM +0100, Peter Maydell wrote:
> > On 19 October 2015 at 12:52, Kevin O'Connor <kevin@koconnor.net>
> > wrote:
> > > On Mon, Oct 19, 2015 at 12:12:34PM +0100, Peter Maydell wrote:
> > >> Windows fails to compile:
> > >> /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c: In
> > >> function ‘fw_cfg_dma_mem_read’:
> > >> /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c:406:
> > >> warning: integer constant is too large for ‘long’ type
> > >
> > > I don't have a Windows test environment, but I suspect the
> > > following:
> > >
> > > #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647 /* "QEMU CFG" */
> > >
> > > should be changed to:
> > >
> > > #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG"
> > > */
> > 
> > Yes. All 64 bit constants should have an LL or ULL suffix.
> 
> Okay - I'll respin patch 7.

Thanks
 
> > (The test failure is presumably something different.)
> 
> I think the fix for the test failure is to make the test less picky
> (it should accept a feature bitmap of either 1 or 3).  But I'll leave
> that to Marc or Gerd.

Yes, that's the solution. We updated the version number, and the test
expects exclusively version 1. And it would also be nice to update the
test to test also fw_cfg DMA

I can send a patch for the test (at least the version) tomorrow.

Thanks
Marc

Re: [Qemu-devel] [PULL 0/7] fw_cfg: add dma interface, add strings via cmdline.

[Gerd Hoffmann]

  Hi,

> Yes, that's the solution. We updated the version number, and the test
> expects exclusively version 1.

Fixed that.  Patch updated, tests running, updated pull soon.

> And it would also be nice to update the
> test to test also fw_cfg DMA

Patch for that is welcome.

thanks,
  Gerd

Re: [Qemu-devel] [PULL 0/7] fw_cfg: add dma interface, add strings via cmdline.

[Gerd Hoffmann]

  Hi

> > Windows fails to compile:
> > /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c: In function
> > ‘fw_cfg_dma_mem_read’:
> > /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c:406: warning:
> > integer constant is too large for ‘long’ type
> 
> I don't have a Windows test environment, but I suspect the following:

Builds fine here for some strange reason ...

> #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647 /* "QEMU CFG" */
> 
> should be changed to:
> 
> #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */

But that is correct.  Fixed.

cheers,
  Gerd

Re: [Qemu-devel] [PULL 0/7] fw_cfg: add dma interface, add strings via cmdline.

[Peter Maydell]

On 19 October 2015 at 14:29, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi
>
>> > Windows fails to compile:
>> > /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c: In function
>> > ‘fw_cfg_dma_mem_read’:
>> > /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c:406: warning:
>> > integer constant is too large for ‘long’ type
>>
>> I don't have a Windows test environment, but I suspect the following:
>
> Builds fine here for some strange reason ...

It's really an "older version of gcc" kind of build warning, it's just
that my w32 setup happens to be the oldest gcc I have in the test set.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/7] fw_cfg: add dma interface, add strings via cmdline.

[Laszlo Ersek]

On 10/19/15 13:12, Peter Maydell wrote:
> On 19 October 2015 at 10:17, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>   Hi,
>>
>> As promised last week I've picked up the fw_cfg patches which are ready
>> to go, so here comes the pull request for them.  Headline feature is
>> the fw_cfg dam support fo course, enabled for arm and x86.  Also small
>> fixes and an option to add string fw_cfg entries via command line.
>>
>> please pull,
>>   Gerd
>>
>> The following changes since commit 40fe17bea478793fc9106a630fa3610dad51f939:
>>
>>   hw/ide/ahci.c: Fix shift left into sign bit (2015-10-18 11:00:40 +0100)
>>
>> are available in the git repository at:
>>
>>   git://git.kraxel.org/qemu tags/pull-fw_cfg-20151019-1
>>
>> for you to fetch changes up to 7b0eec285d447057405df73beae78c8e8aeb9793:
>>
>>   fw_cfg: Define a static signature to be returned on DMA port reads (2015-10-19 11:00:50 +0200)
>>
>> ----------------------------------------------------------------
>> fw_cfg: add dma interface, add strings via cmdline.
>>
>> ----------------------------------------------------------------
> 
> Hi. I'm afraid this fails 'make check':
> 
> TEST: tests/fw_cfg-test... (pid=17533)
>   /i386/fw_cfg/signature:                                              OK
>   /i386/fw_cfg/id:                                                     **
> ERROR:/home/petmay01/qemu/tests/fw_cfg-test.c:40:test_fw_cfg_id:
> assertion failed (qfw_cfg_get_u32(fw_cfg, FW_CFG_ID) == 1): (3 == 1)
> FAIL
> 
> (same failure on 64-bit ARM, ppc64be, OSX, 32-bit ARM).

Heh, tests. We have tests to keep fixing them. :)

Marc, can you add a hunk to patch "Implement fw_cfg DMA interface" that
changes the above test to mask out everything but bit 0? I don't think
we need to test DMA in the test suite right now (can be done in another
series I guess), but DMA's presence shouldn't break the current test. I
guess something like this would suffice:

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 9be78e9..27f115d 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -37,7 +37,7 @@ static void test_fw_cfg_signature(void)

 static void test_fw_cfg_id(void)
 {
-    g_assert_cmpint(qfw_cfg_get_u32(fw_cfg, FW_CFG_ID), ==, 1);
+    g_assert_cmpint(qfw_cfg_get_u32(fw_cfg, FW_CFG_ID) & 1, ==, 1);
 }

 static void test_fw_cfg_uuid(void)

(Sorry if this has been suggested already, trying to catch up with my
mailbox...)

Thanks
Laszlo

> 
> Windows fails to compile:
> /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c: In function
> ‘fw_cfg_dma_mem_read’:
> /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c:406: warning:
> integer constant is too large for ‘long’ type
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PULL 0/7] fw_cfg: add dma interface, add strings via cmdline.

[Marc Marí]

On Mon, 19 Oct 2015 17:17:12 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 10/19/15 13:12, Peter Maydell wrote:
> > On 19 October 2015 at 10:17, Gerd Hoffmann <kraxel@redhat.com>
> > wrote:
> >>   Hi,
> >>
> >> As promised last week I've picked up the fw_cfg patches which are
> >> ready to go, so here comes the pull request for them.  Headline
> >> feature is the fw_cfg dam support fo course, enabled for arm and
> >> x86.  Also small fixes and an option to add string fw_cfg entries
> >> via command line.
> >>
> >> please pull,
> >>   Gerd
> >>
> >> The following changes since commit
> >> 40fe17bea478793fc9106a630fa3610dad51f939:
> >>
> >>   hw/ide/ahci.c: Fix shift left into sign bit (2015-10-18 11:00:40
> >> +0100)
> >>
> >> are available in the git repository at:
> >>
> >>   git://git.kraxel.org/qemu tags/pull-fw_cfg-20151019-1
> >>
> >> for you to fetch changes up to
> >> 7b0eec285d447057405df73beae78c8e8aeb9793:
> >>
> >>   fw_cfg: Define a static signature to be returned on DMA port
> >> reads (2015-10-19 11:00:50 +0200)
> >>
> >> ----------------------------------------------------------------
> >> fw_cfg: add dma interface, add strings via cmdline.
> >>
> >> ----------------------------------------------------------------
> > 
> > Hi. I'm afraid this fails 'make check':
> > 
> > TEST: tests/fw_cfg-test... (pid=17533)
> >   /i386/fw_cfg/signature:
> > OK /i386/fw_cfg/id:
> > ** ERROR:/home/petmay01/qemu/tests/fw_cfg-test.c:40:test_fw_cfg_id:
> > assertion failed (qfw_cfg_get_u32(fw_cfg, FW_CFG_ID) == 1): (3 == 1)
> > FAIL
> > 
> > (same failure on 64-bit ARM, ppc64be, OSX, 32-bit ARM).
> 
> Heh, tests. We have tests to keep fixing them. :)
> 
> Marc, can you add a hunk to patch "Implement fw_cfg DMA interface"
> that changes the above test to mask out everything but bit 0? I don't
> think we need to test DMA in the test suite right now (can be done in
> another series I guess), but DMA's presence shouldn't break the
> current test. I guess something like this would suffice:
> 
> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> index 9be78e9..27f115d 100644
> --- a/tests/fw_cfg-test.c
> +++ b/tests/fw_cfg-test.c
> @@ -37,7 +37,7 @@ static void test_fw_cfg_signature(void)
> 
>  static void test_fw_cfg_id(void)
>  {
> -    g_assert_cmpint(qfw_cfg_get_u32(fw_cfg, FW_CFG_ID), ==, 1);
> +    g_assert_cmpint(qfw_cfg_get_u32(fw_cfg, FW_CFG_ID) & 1, ==, 1);
>  }
> 
>  static void test_fw_cfg_uuid(void)
> 
> (Sorry if this has been suggested already, trying to catch up with my
> mailbox...)

"Fixed that.  Patch updated, tests running, updated pull soon." - Gerd
Hoffman

> Thanks
> Laszlo
> 
> > 
> > Windows fails to compile:
> > /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c: In function
> > ‘fw_cfg_dma_mem_read’:
> > /home/petmay01/linaro/qemu-for-merges/hw/nvram/fw_cfg.c:406:
> > warning: integer constant is too large for ‘long’ type
> > 
> > thanks
> > -- PMM
> > 
>