QEMU fw_cfg DMA interface

QEMU fw_cfg DMA interface

[Marc Marí]

Implementation of the FW CFG DMA interface.

When running a Linux guest on top of QEMU, using the -kernel options, this
is the timing improvement for x86:

QEMU commit 090d0bf and SeaBIOS commit 2fc20dc
QEMU startup time: .078
BIOS startup time: .060
Kernel setup time: .578
Total time: .716

QEMU with this patch series and SeaBIOS with this patch series
QEMU startup time: .080
BIOS startup time: .039
Kernel setup time: .002
Total time: .121

QEMU startup time is the time between the start and the first kvm_entry.
BIOS startup time is the time between the first kvm_entry and the start of
function do_boot, in SeaBIOS.
Kernel setup time is the time between the start of the function do_boot in
SeaBIOS and the jump to the Linux kernel.

As you can see, both the BIOS (because of ACPI tables and other configurations)
and the Linux kernel boot (because of the copy to memory) are greatly
improved with this new interface.

Also, this new interface is an addon to the old interface. Both interfaces
are compatible and interchangeable.

Changes from v1:
 - Take into account order of fields in the FWCfgDmaAccess structure
 - Check and change endianness of FWCfgDmaAccess fields
 - Change order of fields in the FWCfgDmaAccess structure
 - Add FW_CFG_DMA_CTL_SKIP feature for control field
 - Split FW_CFG_SIZE in QEMU
 - Make FW_CFG_ID a bitmap of features
 - Add 64 bit address support for the transfer. Trigger when writing the low
   address, and address is 0 by default and at the end of each transfer.
 - Align ports and addresses.
 - Preserve old fw_cfg_comb_valid behaviour in QEMU
 - Update documentation to reflect all these changes

QEMU fw_cfg DMA interface

[Marc Marí]

Implementation of the FW CFG DMA interface.

When running a Linux guest on top of QEMU, using the -kernel options, this
is the timing improvement for x86:

QEMU commit 090d0bf and SeaBIOS commit 2fc20dc
QEMU startup time: .078
BIOS startup time: .060
Kernel setup time: .578
Total time: .716

QEMU with this patch series and SeaBIOS with this patch series
QEMU startup time: .080
BIOS startup time: .039
Kernel setup time: .002
Total time: .121

QEMU startup time is the time between the start and the first kvm_entry.
BIOS startup time is the time between the first kvm_entry and the start of
function do_boot, in SeaBIOS.
Kernel setup time is the time between the start of the function do_boot in
SeaBIOS and the jump to the Linux kernel.

As you can see, both the BIOS (because of ACPI tables and other configurations)
and the Linux kernel boot (because of the copy to memory) are greatly
improved with this new interface.

Also, this new interface is an addon to the old interface. Both interfaces
are compatible and interchangeable.

Changes from v1:
 - Take into account order of fields in the FWCfgDmaAccess structure
 - Check and change endianness of FWCfgDmaAccess fields
 - Change order of fields in the FWCfgDmaAccess structure
 - Add FW_CFG_DMA_CTL_SKIP feature for control field
 - Split FW_CFG_SIZE in QEMU
 - Make FW_CFG_ID a bitmap of features
 - Add 64 bit address support for the transfer. Trigger when writing the low
   address, and address is 0 by default and at the end of each transfer.
 - Align ports and addresses.
 - Preserve old fw_cfg_comb_valid behaviour in QEMU
 - Update documentation to reflect all these changes
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Qemu-devel] QEMU fw_cfg DMA interface

[Marc Marí]

Implementation of the FW CFG DMA interface.

When running a Linux guest on top of QEMU, using the -kernel options, this
is the timing improvement for x86:

QEMU commit 090d0bf and SeaBIOS commit 2fc20dc
QEMU startup time: .078
BIOS startup time: .060
Kernel setup time: .578
Total time: .716

QEMU with this patch series and SeaBIOS with this patch series
QEMU startup time: .080
BIOS startup time: .039
Kernel setup time: .002
Total time: .121

QEMU startup time is the time between the start and the first kvm_entry.
BIOS startup time is the time between the first kvm_entry and the start of
function do_boot, in SeaBIOS.
Kernel setup time is the time between the start of the function do_boot in
SeaBIOS and the jump to the Linux kernel.

As you can see, both the BIOS (because of ACPI tables and other configurations)
and the Linux kernel boot (because of the copy to memory) are greatly
improved with this new interface.

Also, this new interface is an addon to the old interface. Both interfaces
are compatible and interchangeable.

Changes from v1:
 - Take into account order of fields in the FWCfgDmaAccess structure
 - Check and change endianness of FWCfgDmaAccess fields
 - Change order of fields in the FWCfgDmaAccess structure
 - Add FW_CFG_DMA_CTL_SKIP feature for control field
 - Split FW_CFG_SIZE in QEMU
 - Make FW_CFG_ID a bitmap of features
 - Add 64 bit address support for the transfer. Trigger when writing the low
   address, and address is 0 by default and at the end of each transfer.
 - Align ports and addresses.
 - Preserve old fw_cfg_comb_valid behaviour in QEMU
 - Update documentation to reflect all these changes

[Qemu-devel] [PATCH v2 0/5] fw_cfg DMA interface

[Marc Marí]

Implement host-side of the FW CFG DMA interface both for x86 and ARM.

Based on Gerd Hoffman's initial implementation.

Gabriel L. Somlo (1):
  fw_cfg: document fw_cfg_modify_iXX() update functions

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     |  79 +++++++++++++-
 hw/arm/virt.c             |  13 ++-
 hw/i386/pc.c              |  11 +-
 hw/nvram/fw_cfg.c         | 261 +++++++++++++++++++++++++++++++++++++++++++---
 include/hw/nvram/fw_cfg.h |  15 ++-
 5 files changed, 351 insertions(+), 28 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Marc Marí]

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>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@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 74351dd..5bc7b96 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,
-- 
2.4.3

[Qemu-devel] [PATCH v2 2/5] fw_cfg DMA interface documentation

[Marc Marí]

Add fw_cfg DMA interface specification in the documentation.

Based on Gerd Hoffman's initial implementation.

Signed-off-by: Marc Marí <markmb@redhat.com>
---
 docs/specs/fw_cfg.txt | 68 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 4 deletions(-)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 5bc7b96..06302f6 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: 0x09020000
+Data Register address:     0x09020008
+DMA Address address:       0x0902000c
 
 == 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,58 @@ 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, as the selector register, is in little-endian format
+when using IOports, and in big-endian format when using MMIO. The value for
+the register is 0 at startup and after an operation. A write to the lower
+half 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
+higher part.
+
+In this register, a physical RAM address to a FWCfgDmaAccess structure 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
+
+When an operation is triggered, 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 address specified by the "address" field.
+
+If the control field has only bit 2 set, a skip operation will be perfomed.
+The offset for the current selector will be advanced "length" bytes.
+
+To check 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).
+
+Target address goes up and transfer length goes down as the transfer happens,
+so after a successful transfer the length field is zero and the address field
+points right after the memory block written.
+
+If a partial transfer happened before an error occured the address and
+length registers indicate how much data has been transfered successfully.
+
 = Host-side API =
 
 The following functions are available to the QEMU programmer for adding
-- 
2.4.3

[Qemu-devel] [PATCH v2 3/5] Implement fw_cfg DMA interface

[Marc Marí]

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>
---
 hw/arm/virt.c             |   2 +-
 hw/nvram/fw_cfg.c         | 261 +++++++++++++++++++++++++++++++++++++++++++---
 include/hw/nvram/fw_cfg.h |  15 ++-
 3 files changed, 260 insertions(+), 18 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d5a8417..b88c104 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -620,7 +620,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 88481b7..7e5ba96 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,8 @@
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 
-#define FW_CFG_SIZE 2
+#define FW_CFG_IO_SIZE 12 /* Address aligned to 4 bytes */
+#define FW_CFG_CTL_SIZE 2
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
@@ -42,6 +44,15 @@
 #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
+
 typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
@@ -59,6 +70,10 @@ struct FWCfgState {
     uint16_t cur_entry;
     uint32_t cur_offset;
     Notifier machine_ready;
+
+    bool dma_enabled;
+    AddressSpace *dma_as;
+    dma_addr_t dma_addr;
 };
 
 struct FWCfgIoState {
@@ -75,7 +90,7 @@ struct FWCfgMemState {
     FWCfgState parent_obj;
     /*< public >*/
 
-    MemoryRegion ctl_iomem, data_iomem;
+    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
     uint32_t data_width;
     MemoryRegionOps wide_data_ops;
 };
@@ -294,6 +309,142 @@ 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;
+    uint8_t *ptr;
+    void *addr;
+    FWCfgDmaAccess dma;
+    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
+    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
+
+    len = sizeof(dma);
+    addr = dma_memory_map(s->dma_as, s->dma_addr, &len,
+                                DMA_DIRECTION_FROM_DEVICE);
+
+    s->dma_addr = 0;
+
+    if (!addr || !len) {
+        return;
+    }
+
+    dma.address = be64_to_cpu(*(uint64_t *)(addr +
+                    offsetof(FWCfgDmaAccess, address)));
+    dma.length = be32_to_cpu(*(uint32_t *)(addr +
+                    offsetof(FWCfgDmaAccess, length)));
+    dma.control = be32_to_cpu(*(uint32_t *)(addr +
+                    offsetof(FWCfgDmaAccess, control)));
+
+    if (dma.control & FW_CFG_DMA_CTL_ERROR) {
+        goto out;
+    }
+
+    if (!(dma.control & (FW_CFG_DMA_CTL_READ | FW_CFG_DMA_CTL_SKIP))) {
+        goto out;
+    }
+
+    while (dma.length > 0) {
+        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 (dma.control & FW_CFG_DMA_CTL_READ) {
+                ptr = dma_memory_map(s->dma_as, dma.address, &len,
+                                     DMA_DIRECTION_FROM_DEVICE);
+                if (!ptr || !len) {
+                    dma.control |= FW_CFG_DMA_CTL_ERROR;
+                    goto out;
+                }
+
+                memset(ptr, 0, len);
+
+                dma_memory_unmap(s->dma_as, ptr, len,
+                                 DMA_DIRECTION_FROM_DEVICE, len);
+            }
+
+        } else {
+            if (dma.length <= e->len) {
+                len = dma.length;
+            } else {
+                len = e->len;
+            }
+
+            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 (dma.control & FW_CFG_DMA_CTL_READ) {
+                ptr = dma_memory_map(s->dma_as, dma.address, &len,
+                                     DMA_DIRECTION_FROM_DEVICE);
+                if (!ptr || !len) {
+                    dma.control |= FW_CFG_DMA_CTL_ERROR;
+                    goto out;
+                }
+
+                memcpy(ptr, &e->data[s->cur_offset], len);
+
+                dma_memory_unmap(s->dma_as, ptr, len,
+                                 DMA_DIRECTION_FROM_DEVICE, len);
+            }
+
+            s->cur_offset += len;
+
+        }
+
+        dma.address += len;
+        dma.length  -= len;
+        dma.control = 0;
+
+        *(uint64_t *)(addr + offsetof(FWCfgDmaAccess, address)) =
+                                                cpu_to_be64(dma.address);
+        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, length)) =
+                                                cpu_to_be32(dma.length);
+        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, control)) =
+                                                cpu_to_be32(dma.control);
+    }
+
+    trace_fw_cfg_read(s, 0);
+
+out:
+    dma_memory_unmap(s->dma_as, addr, sizeof(dma),
+                                DMA_DIRECTION_FROM_DEVICE, sizeof(dma));
+    return;
+
+}
+
+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 = (0xFFFFFFFF & 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)
 {
@@ -321,20 +472,35 @@ static uint64_t fw_cfg_comb_read(void *opaque, hwaddr addr,
 static void fw_cfg_comb_write(void *opaque, hwaddr addr,
                               uint64_t value, unsigned size)
 {
-    switch (size) {
+    FWCfgState *s;
+
+    switch (addr) {
+    case 0:
+        fw_cfg_select(opaque, (uint16_t)value);
+        break;
     case 1:
         fw_cfg_write(opaque, (uint8_t)value);
         break;
-    case 2:
-        fw_cfg_select(opaque, (uint16_t)value);
+    case 4:
+        /* FWCfgDmaAccess low address */
+        s = opaque;
+        s->dma_addr |= value;
+        fw_cfg_dma_transfer(s);
         break;
+    case 8:
+        /* FWCfgDmaAccess high address */
+        s = opaque;
+        s->dma_addr = (0xFFFFFFFF & value) << 32;
     }
 }
 
 static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
-    return (size == 1) || (is_write && size == 2);
+    FWCfgState *s = opaque;
+
+    return (size == 1) || (is_write && size == 2) ||
+            (s->dma_enabled && is_write && addr >= 4);
 }
 
 static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
@@ -361,6 +527,12 @@ 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,
+};
+
 static void fw_cfg_reset(DeviceState *d)
 {
     FWCfgState *s = FW_CFG(d);
@@ -401,6 +573,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 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,
@@ -410,6 +598,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,
     }
 };
 
@@ -595,7 +787,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);
@@ -607,22 +798,45 @@ 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, AddressSpace *dma_as)
 {
     DeviceState *dev;
+    FWCfgState *s;
+    uint32_t version = FW_CFG_VERSION;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_IO);
     qdev_prop_set_uint32(dev, "iobase", iobase);
+
     fw_cfg_init1(dev);
+    s = FW_CFG(dev);
+
+    if (dma_as) {
+        /* 64 bits for the address field */
+        s->dma_as = dma_as;
+        s->dma_enabled = true;
+        s->dma_addr = 0;
+
+        version |= FW_CFG_VERSION_DMA;
+    }
+
+    fw_cfg_add_i32(s, FW_CFG_ID, version);
 
-    return FW_CFG(dev);
+    return s;
 }
 
-FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
-                                 uint32_t data_width)
+FWCfgState *fw_cfg_init_io(uint32_t iobase)
+{
+    return fw_cfg_init_io_dma(iobase, NULL);
+}
+
+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;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
     qdev_prop_set_uint32(dev, "data_width", data_width);
@@ -633,13 +847,26 @@ 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_addr && dma_as) {
+        s->dma_as = dma_as;
+        s->dma_enabled = true;
+        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);
 }
 
 
@@ -675,7 +902,7 @@ 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_IO_SIZE);
     sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
 }
 
@@ -707,7 +934,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) {
@@ -725,6 +952,10 @@ 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);
+
+    memory_region_init_io(&s->dma_iomem, OBJECT(s), &fw_cfg_dma_mem_ops,
+                          FW_CFG(s), "fwcfg.dma", sizeof(dma_addr_t));
+    sysbus_init_mmio(sbd, &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..d0cbc88 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,12 @@ 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, 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);
 
-- 
2.4.3

[Qemu-devel] [PATCH v2 4/5] Enable fw_cfg DMA interface for ARM

[Marc Marí]

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>
---
 hw/arm/virt.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b88c104..54d5f54 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -111,7 +111,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
-    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
+    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -614,13 +614,13 @@ static void create_flash(const VirtBoardInfo *vbi)
     g_free(nodename);
 }
 
-static void create_fw_cfg(const VirtBoardInfo *vbi)
+static void create_fw_cfg(AddressSpace *as, const VirtBoardInfo *vbi)
 {
     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);
@@ -808,6 +808,7 @@ static void machvirt_init(MachineState *machine)
     VirtGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
     VirtGuestInfo *guest_info = &guest_info_state->info;
     char **cpustr;
+    AddressSpace *as = NULL;
 
     if (!cpu_model) {
         cpu_model = "cortex-a15";
@@ -845,6 +846,10 @@ static void machvirt_init(MachineState *machine)
         }
         cpuobj = object_new(object_class_get_name(oc));
 
+        if (!as) {
+            as = CPU(cpuobj)->as;
+        }
+
         /* Handle any CPU options specified by the user */
         cc->parse_features(CPU(cpuobj), cpuopts, &err);
         g_free(cpuopts);
@@ -897,7 +902,7 @@ static void machvirt_init(MachineState *machine)
      */
     create_virtio_devices(vbi, pic);
 
-    create_fw_cfg(vbi);
+    create_fw_cfg(as, vbi);
     rom_set_fw(fw_cfg_find());
 
     guest_info->smp_cpus = smp_cpus;
-- 
2.4.3

[Qemu-devel] [PATCH v2 5/5] Enable fw_cfg DMA interface for x86

[Marc Marí]

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>
---
 hw/i386/pc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 9f2924e..c6dc84f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -753,14 +753,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, 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
@@ -1316,6 +1317,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
     MemoryRegion *ram_below_4g, *ram_above_4g;
     FWCfgState *fw_cfg;
     MachineState *machine = MACHINE(pcms);
+    AddressSpace *as;
 
     assert(machine->ram_size == pcms->below_4g_mem_size +
                                 pcms->above_4g_mem_size);
@@ -1407,7 +1409,10 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = bochs_bios_init();
+    as = g_malloc(sizeof(*as));
+    address_space_init(as, ram_below_4g, "pc.as");
+    fw_cfg = bochs_bios_init(as);
+
     rom_set_fw(fw_cfg);
 
     if (guest_info->has_reserved_memory && pcms->hotplug_memory.base) {
-- 
2.4.3

[PATCH v2] QEMU fw_cfg DMA interface documentation

[Marc Marí]

Add fw_cfg DMA interface specfication in the fw_cfg documentation.

Signed-off-by: Marc Marí <markmb@redhat.com>
---
 Documentation/devicetree/bindings/arm/fw-cfg.txt | 51 +++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
index 953fb64..766ddbe 100644
--- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
+++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
@@ -45,6 +45,53 @@ blob to be read from the data register has size 4, and it is to be interpreted
 as a uint32_t value in little endian byte order. The current value
 (corresponding to the above outer protocol) is zero.
 
+If bit 1 of the feature bitmap is set, the DMA interface is present. This
+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 lower half 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 higher part.
+
+In this register, a physical RAM address to a FWCfgDmaAccess structure 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
+
+When an operation is triggered, 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 address specified by the "address" field.
+
+If the control field has only bit 2 set, a skip operation will be perfomed.
+The offset for the current selector will be advanced "length" bytes.
+
+To check 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).
+
+Target address goes up and transfer length goes down as the transfer happens,
+so after a successful transfer the length field is zero and the address field
+points right after the memory block written.
+
+If a partial transfer happened before an error occured the address and
+length registers indicate how much data has been transfered successfully.
+
 The guest kernel is not expected to use these registers (although it is
 certainly allowed to); the device tree bindings are documented here because
 this is where device tree bindings reside in general.
@@ -56,6 +103,8 @@ Required properties:
 - reg: the MMIO region used by the device.
   * Bytes 0x0 to 0x7 cover the data register.
   * Bytes 0x8 to 0x9 cover the selector register.
+  * With DMA interface enabled: Bytes 0xc to 0x13 cover the DMA address
+    register.
   * Further registers may be appended to the region in case of future interface
     revisions / feature bits.
 
@@ -66,7 +115,7 @@ Example:
 	#address-cells = <0x2>;
 
 	fw-cfg@9020000 {
+		reg = <0x0 0x9020000 0x0 0x14>;
 		compatible = "qemu,fw-cfg-mmio";
-		reg = <0x0 0x9020000 0x0 0xa>;
 	};
 };
-- 
2.4.3

[PATCH v2] QEMU fw_cfg DMA interface documentation

[Marc Marí]

Add fw_cfg DMA interface specfication in the fw_cfg documentation.

Signed-off-by: Marc Marí <markmb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/fw-cfg.txt | 51 +++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
index 953fb64..766ddbe 100644
--- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
+++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
@@ -45,6 +45,53 @@ blob to be read from the data register has size 4, and it is to be interpreted
 as a uint32_t value in little endian byte order. The current value
 (corresponding to the above outer protocol) is zero.
 
+If bit 1 of the feature bitmap is set, the DMA interface is present. This
+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 lower half 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 higher part.
+
+In this register, a physical RAM address to a FWCfgDmaAccess structure 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
+
+When an operation is triggered, 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 address specified by the "address" field.
+
+If the control field has only bit 2 set, a skip operation will be perfomed.
+The offset for the current selector will be advanced "length" bytes.
+
+To check 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).
+
+Target address goes up and transfer length goes down as the transfer happens,
+so after a successful transfer the length field is zero and the address field
+points right after the memory block written.
+
+If a partial transfer happened before an error occured the address and
+length registers indicate how much data has been transfered successfully.
+
 The guest kernel is not expected to use these registers (although it is
 certainly allowed to); the device tree bindings are documented here because
 this is where device tree bindings reside in general.
@@ -56,6 +103,8 @@ Required properties:
 - reg: the MMIO region used by the device.
   * Bytes 0x0 to 0x7 cover the data register.
   * Bytes 0x8 to 0x9 cover the selector register.
+  * With DMA interface enabled: Bytes 0xc to 0x13 cover the DMA address
+    register.
   * Further registers may be appended to the region in case of future interface
     revisions / feature bits.
 
@@ -66,7 +115,7 @@ Example:
 	#address-cells = <0x2>;
 
 	fw-cfg@9020000 {
+		reg = <0x0 0x9020000 0x0 0x14>;
 		compatible = "qemu,fw-cfg-mmio";
-		reg = <0x0 0x9020000 0x0 0xa>;
 	};
 };
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Qemu-devel] [PATCH v2 2/5] fw_cfg DMA interface documentation

[Kevin O'Connor]

On Mon, Aug 31, 2015 at 11:10:14AM +0200, Marc Marí wrote:
> Add fw_cfg DMA interface specification in the documentation.
> 
> Based on Gerd Hoffman's initial implementation.

Thanks for working on this.

[...]
> += 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, as the selector register, is in little-endian format
> +when using IOports, and in big-endian format when using MMIO. The value for
> +the register is 0 at startup and after an operation. A write to the lower
> +half 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
> +higher part.
> +
> +In this register, a physical RAM address to a FWCfgDmaAccess structure 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
> +
> +When an operation is triggered, 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 address specified by the "address" field.
> +
> +If the control field has only bit 2 set, a skip operation will be perfomed.
> +The offset for the current selector will be advanced "length" bytes.
> +
> +To check 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).
> +
> +Target address goes up and transfer length goes down as the transfer happens,
> +so after a successful transfer the length field is zero and the address field
> +points right after the memory block written.
> +
> +If a partial transfer happened before an error occured the address and
> +length registers indicate how much data has been transfered successfully.

If the interface will support asynchronous transfers, then it is
necessary to document which field QEMU will update last.  That way,
the guest will know which particular field to wait on.  It should
probably also be documented that that particular field must have
native alignment (eg, alignment of 4 for a 32bit field).

The reason this is important, is because of a subtle race condition.
For example, if QEMU updates "length" and then "control", but the
guest spins on "length" then it is possible for the guest to exit its
spin loop and use the memory containing "control" for something else
before QEMU finishes updating "control".  This would result in
corruption of that memory when QEMU later overwrites it.

My suggestion would be for QEMU to only ever update "control" and not
modify any other parts of the FWCfgDmaAccess struct.  This makes the
interface and documentation simpler and it sidesteps this issue.

-Kevin

Re: [Qemu-devel] [PATCH v2 3/5] Implement fw_cfg DMA interface

[Kevin O'Connor]

On Mon, Aug 31, 2015 at 11:10:15AM +0200, Marc Marí wrote:
> 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>
> ---
>  hw/arm/virt.c             |   2 +-
>  hw/nvram/fw_cfg.c         | 261 +++++++++++++++++++++++++++++++++++++++++++---
>  include/hw/nvram/fw_cfg.h |  15 ++-
>  3 files changed, 260 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d5a8417..b88c104 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -620,7 +620,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 88481b7..7e5ba96 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,8 @@
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
>  
> -#define FW_CFG_SIZE 2
> +#define FW_CFG_IO_SIZE 12 /* Address aligned to 4 bytes */
> +#define FW_CFG_CTL_SIZE 2
>  #define FW_CFG_NAME "fw_cfg"
>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>  
> @@ -42,6 +44,15 @@
>  #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
> +
>  typedef struct FWCfgEntry {
>      uint32_t len;
>      uint8_t *data;
> @@ -59,6 +70,10 @@ struct FWCfgState {
>      uint16_t cur_entry;
>      uint32_t cur_offset;
>      Notifier machine_ready;
> +
> +    bool dma_enabled;
> +    AddressSpace *dma_as;
> +    dma_addr_t dma_addr;
>  };
>  
>  struct FWCfgIoState {
> @@ -75,7 +90,7 @@ struct FWCfgMemState {
>      FWCfgState parent_obj;
>      /*< public >*/
>  
> -    MemoryRegion ctl_iomem, data_iomem;
> +    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
>      uint32_t data_width;
>      MemoryRegionOps wide_data_ops;
>  };
> @@ -294,6 +309,142 @@ 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;
> +    uint8_t *ptr;
> +    void *addr;
> +    FWCfgDmaAccess dma;
> +    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> +    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
> +
> +    len = sizeof(dma);
> +    addr = dma_memory_map(s->dma_as, s->dma_addr, &len,
> +                                DMA_DIRECTION_FROM_DEVICE);
> +
> +    s->dma_addr = 0;
> +
> +    if (!addr || !len) {
> +        return;
> +    }
> +
> +    dma.address = be64_to_cpu(*(uint64_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, address)));
> +    dma.length = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, length)));
> +    dma.control = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, control)));

I am not that familiar with QEMU, but shouldn't that be
DMA_DIRECTION_TO_DEVICE?

It looks like other drivers use dma_memory_read() which would simplify
this code:

    dma_memory_read(s->dma_as, s->dma_addr, &dma, sizeof(dma));
    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_ERROR) {
> +        goto out;
> +    }
> +
> +    if (!(dma.control & (FW_CFG_DMA_CTL_READ | FW_CFG_DMA_CTL_SKIP))) {
> +        goto out;
> +    }
> +
> +    while (dma.length > 0) {
> +        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 (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memset(ptr, 0, len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);
> +            }
> +
> +        } else {
> +            if (dma.length <= e->len) {
> +                len = dma.length;
> +            } else {
> +                len = e->len;
> +            }
> +
> +            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 (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memcpy(ptr, &e->data[s->cur_offset], len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);
> +            }
> +
> +            s->cur_offset += len;
> +
> +        }
> +
> +        dma.address += len;
> +        dma.length  -= len;
> +        dma.control = 0;
> +
> +        *(uint64_t *)(addr + offsetof(FWCfgDmaAccess, address)) =
> +                                                cpu_to_be64(dma.address);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, length)) =
> +                                                cpu_to_be32(dma.length);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, control)) =
> +                                                cpu_to_be32(dma.control);
> +    }

I don't think it makes sense for this update to be performed within
the loop.

As I mentioned in another email, I think just updating control would
be sufficient.  Looks like include/sysemu/dma.h defines a stl_be_dma()
macro for performing a single 32bit dma write.

Thanks,
-Kevin

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Peter Maydell]

On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote:
> 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>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@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 74351dd..5bc7b96 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,

This doesn't cover fw_cfg_modify_file(); is that intentional?

As an aside, shouldn't this function-level documentation be done
via doc-comments in the header file where the prototypes are
declared? (You don't need to move the docs around in this series,
but it might be nice to do at some point.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Gabriel L. Somlo]

On Tue, Sep 01, 2015 at 06:33:25PM +0100, Peter Maydell wrote:
> On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote:
> > 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>
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > Reviewed-by: Laszlo Ersek <lersek@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 74351dd..5bc7b96 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,
> 
> This doesn't cover fw_cfg_modify_file(); is that intentional?

There should already be a paragraph in there (further down from where
this is supposed to go in) describing fw_cfg_modify_file(), which
isn't affected by this. 
> 
> As an aside, shouldn't this function-level documentation be done
> via doc-comments in the header file where the prototypes are
> declared? (You don't need to move the docs around in this series,
> but it might be nice to do at some point.)

You mean, leave docs/specs/fw_cfg.txt to deal with the guest-side
port/mmio api only, and have the host-side functions simply documented
as comments in include/hw/nvram/fw_cfg.h ?

That should be relatively painless, if that's the agreed-upon
convention...

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH v2 2/5] fw_cfg DMA interface documentation

[Peter Maydell]

On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote:
> Add fw_cfg DMA interface specification in the documentation.
>
> Based on Gerd Hoffman's initial implementation.
>
> Signed-off-by: Marc Marí <markmb@redhat.com>
> ---
>  docs/specs/fw_cfg.txt | 68 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 5bc7b96..06302f6 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: 0x09020000
> +Data Register address:     0x09020008
> +DMA Address address:       0x0902000c

These addresses shouldn't be documented -- the correct API is that the guest
needs to find the base address of the fw_cfg device via device tree
or ACPI table. You can document the layout of the registers within the
device, obviously (ie +0, +4, +8).

>  == 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,58 @@ 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, as the selector register, is in little-endian format
> +when using IOports, and in big-endian format when using MMIO. The value for
> +the register is 0 at startup and after an operation. A write to the lower
> +half triggers an operation. This means, that operations with 32-bit addresses

Delete this comma.

> +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
> +higher part.
> +
> +In this register, a physical RAM address to a FWCfgDmaAccess structure should

"the physical address of a FWCfgDmaAccess structure in RAM"

> +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
> +
> +When an operation is triggered, 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 address specified by the "address" field.
> +
> +If the control field has only bit 2 set, a skip operation will be perfomed.
> +The offset for the current selector will be advanced "length" bytes.

The implication here is that the operation completes before the
guest write to the address register returns,

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

Is there much point in having an async transfer interface which
requires the guest to busy-wait polling the control field?

> +
> +Target address goes up and transfer length goes down as the transfer happens,
> +so after a successful transfer the length field is zero and the address field
> +points right after the memory block written.
> +
> +If a partial transfer happened before an error occured the address and

"occurred".

> +length registers indicate how much data has been transfered successfully.

"transferred".

> +
>  = Host-side API =
>
>  The following functions are available to the QEMU programmer for adding

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/5] fw_cfg DMA interface documentation

[Peter Maydell]

On 1 September 2015 at 18:47, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote:
>> +=== ARM Register Locations ===
>> +
>> +Selector Register address: 0x09020000
>> +Data Register address:     0x09020008
>> +DMA Address address:       0x0902000c
>
> These addresses shouldn't be documented -- the correct API is that the guest
> needs to find the base address of the fw_cfg device via device tree
> or ACPI table. You can document the layout of the registers within the
> device, obviously (ie +0, +4, +8).

...and this is not what your patchset implements anyway. This should be:

 offset 0: data register (8 bytes wide)
 offset 8: selector register (4 bytes wide)
 offset 12: reserved
 offset 16: DMA address register (8 bytes wide)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/5] Enable fw_cfg DMA interface for ARM

[Peter Maydell]

On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote:
> 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>
> ---
>  hw/arm/virt.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b88c104..54d5f54 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -111,7 +111,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
>      [VIRT_UART] =               { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
> -    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
> +    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -614,13 +614,13 @@ static void create_flash(const VirtBoardInfo *vbi)
>      g_free(nodename);
>  }
>
> -static void create_fw_cfg(const VirtBoardInfo *vbi)
> +static void create_fw_cfg(AddressSpace *as, const VirtBoardInfo *vbi)

Please keep vbi as the first argument; this matches the other functions
in this file.

Calling the argument dma_as would probably be a little more informative.

>  {
>      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);
> @@ -808,6 +808,7 @@ static void machvirt_init(MachineState *machine)
>      VirtGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
>      VirtGuestInfo *guest_info = &guest_info_state->info;
>      char **cpustr;
> +    AddressSpace *as = NULL;
>
>      if (!cpu_model) {
>          cpu_model = "cortex-a15";
> @@ -845,6 +846,10 @@ static void machvirt_init(MachineState *machine)
>          }
>          cpuobj = object_new(object_class_get_name(oc));
>
> +        if (!as) {
> +            as = CPU(cpuobj)->as;
> +        }

This seems like a weird thing to set the fw_cfg DMA address
space to. (Either the fw_cfg device shouldn't care which CPU
it is being accessing by and shouldn't use any particular CPU's
address space, or it needs to really care about the CPU
that's doing any particular write to it and use that exact
CPU's address space, selected at runtime. The former is the
most likely and matches what actual DMA hardware devices
will do.)

Why not just use address_space_memory ?

> +
>          /* Handle any CPU options specified by the user */
>          cc->parse_features(CPU(cpuobj), cpuopts, &err);
>          g_free(cpuopts);
> @@ -897,7 +902,7 @@ static void machvirt_init(MachineState *machine)
>       */
>      create_virtio_devices(vbi, pic);
>
> -    create_fw_cfg(vbi);
> +    create_fw_cfg(as, vbi);
>      rom_set_fw(fw_cfg_find());
>
>      guest_info->smp_cpus = smp_cpus;
> --
> 2.4.3

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 3/5] Implement fw_cfg DMA interface

[Peter Maydell]

On 31 August 2015 at 10:10, Marc Marí <markmb@redhat.com> wrote:
> 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>

> @@ -294,6 +309,142 @@ 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;
> +    uint8_t *ptr;
> +    void *addr;
> +    FWCfgDmaAccess dma;
> +    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> +    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
> +
> +    len = sizeof(dma);
> +    addr = dma_memory_map(s->dma_as, s->dma_addr, &len,
> +                                DMA_DIRECTION_FROM_DEVICE);
> +
> +    s->dma_addr = 0;
> +
> +    if (!addr || !len) {
> +        return;
> +    }
> +
> +    dma.address = be64_to_cpu(*(uint64_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, address)));
> +    dma.length = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, length)));
> +    dma.control = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, control)));

There are only three fields in this struct, so rather than
using dma_memory_map/unmap and manual byteswapping, how about:

 dma.control = ldl_be_dma(s->dma_as, s->dma_addr);
 dma.length = ldl_be_dma(s->dma_as, s->dma_addr + 4);
 dma.address = ldq_be_dma(s->dma_as, s->dma_addr + 8);

Kevin's suggestion to use dma_memory_read() would be OK as well,
if you really want to check for the return value from the memory
transaction. But you probably don't care because if the guest has
pointed the dma address register into nowhere then it deserves
whatever it gets as long as we don't fall over; and we need to
not fall over whatever the values of control/length/address are.
In any case, please don't use dma_memory_map/unmap.

There are st*_ versions for writing back updated values.

> +
> +    if (dma.control & FW_CFG_DMA_CTL_ERROR) {
> +        goto out;
> +    }
> +
> +    if (!(dma.control & (FW_CFG_DMA_CTL_READ | FW_CFG_DMA_CTL_SKIP))) {
> +        goto out;
> +    }
> +
> +    while (dma.length > 0) {
> +        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 (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memset(ptr, 0, len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);

dma_memory_write() would be better than map-zero-unmap I think,
though you would need to have a handy buffer of zeroes to pass
to that function, so maybe not.

> +            }
> +
> +        } else {
> +            if (dma.length <= e->len) {
> +                len = dma.length;
> +            } else {
> +                len = e->len;
> +            }
> +
> +            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 (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memcpy(ptr, &e->data[s->cur_offset], len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);

...and dma_memory_write() is definitely better than
map-memcpy-unmap.

> +            }
> +
> +            s->cur_offset += len;
> +
> +        }
> +
> +        dma.address += len;
> +        dma.length  -= len;
> +        dma.control = 0;
> +
> +        *(uint64_t *)(addr + offsetof(FWCfgDmaAccess, address)) =
> +                                                cpu_to_be64(dma.address);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, length)) =
> +                                                cpu_to_be32(dma.length);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, control)) =
> +                                                cpu_to_be32(dma.control);
> +    }
> +
> +    trace_fw_cfg_read(s, 0);
> +
> +out:
> +    dma_memory_unmap(s->dma_as, addr, sizeof(dma),
> +                                DMA_DIRECTION_FROM_DEVICE, sizeof(dma));
> +    return;
> +
> +}
> +
> +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 = (0xFFFFFFFF & value) << 32;

The mask here is unnecessary, since you're shifting by 32 bits.

> +        } 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)
>  {
> @@ -321,20 +472,35 @@ static uint64_t fw_cfg_comb_read(void *opaque, hwaddr addr,
>  static void fw_cfg_comb_write(void *opaque, hwaddr addr,
>                                uint64_t value, unsigned size)
>  {
> -    switch (size) {
> +    FWCfgState *s;
> +
> +    switch (addr) {
> +    case 0:
> +        fw_cfg_select(opaque, (uint16_t)value);
> +        break;
>      case 1:
>          fw_cfg_write(opaque, (uint8_t)value);
>          break;
> -    case 2:
> -        fw_cfg_select(opaque, (uint16_t)value);
> +    case 4:
> +        /* FWCfgDmaAccess low address */
> +        s = opaque;
> +        s->dma_addr |= value;
> +        fw_cfg_dma_transfer(s);
>          break;
> +    case 8:
> +        /* FWCfgDmaAccess high address */
> +        s = opaque;
> +        s->dma_addr = (0xFFFFFFFF & value) << 32;
>      }
>  }
>
>  static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> -    return (size == 1) || (is_write && size == 2);
> +    FWCfgState *s = opaque;
> +
> +    return (size == 1) || (is_write && size == 2) ||
> +            (s->dma_enabled && is_write && addr >= 4);

This doesn't look right, since it will allow 1 and 2 byte
writes to the dma_address port, which doesn't seem like
a great idea.

Maybe it would be easier to make the dma port its own
memory region (and then init and sysbus_add_io() it in
fw_cfg_io_realize()) ? Then you wouldn't need to mess with
the already confusing logic for handling combined config/data
ports (which have to be handled together because of the weird
"1 byte accesses are data and 2 byte accesses are config"
semantics).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Peter Maydell]

On 1 September 2015 at 18:45, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> On Tue, Sep 01, 2015 at 06:33:25PM +0100, Peter Maydell wrote:
>> This doesn't cover fw_cfg_modify_file(); is that intentional?
>
> There should already be a paragraph in there (further down from where
> this is supposed to go in) describing fw_cfg_modify_file(), which
> isn't affected by this.

Sorry, so there is.

>> As an aside, shouldn't this function-level documentation be done
>> via doc-comments in the header file where the prototypes are
>> declared? (You don't need to move the docs around in this series,
>> but it might be nice to do at some point.)
>
> You mean, leave docs/specs/fw_cfg.txt to deal with the guest-side
> port/mmio api only, and have the host-side functions simply documented
> as comments in include/hw/nvram/fw_cfg.h ?
>
> That should be relatively painless, if that's the agreed-upon
> convention...

Yes. I think at the point this file was written we probably hadn't
started using doc-comments for our APIs. (My usual place to crib
the formatting for doc-comments is the extract/deposit APIs in
bitops.h.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Gabriel L. Somlo]

On Tue, Sep 01, 2015 at 07:45:27PM +0100, Peter Maydell wrote:
> On 1 September 2015 at 18:45, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> > On Tue, Sep 01, 2015 at 06:33:25PM +0100, Peter Maydell wrote:
> >> As an aside, shouldn't this function-level documentation be done
> >> via doc-comments in the header file where the prototypes are
> >> declared? (You don't need to move the docs around in this series,
> >> but it might be nice to do at some point.)
> >
> > You mean, leave docs/specs/fw_cfg.txt to deal with the guest-side
> > port/mmio api only, and have the host-side functions simply documented
> > as comments in include/hw/nvram/fw_cfg.h ?
> >
> > That should be relatively painless, if that's the agreed-upon
> > convention...
> 
> Yes. I think at the point this file was written we probably hadn't
> started using doc-comments for our APIs. (My usual place to crib
> the formatting for doc-comments is the extract/deposit APIs in
> bitops.h.)

OK, I guess I'll wait until after the dust settles on fw_cfg/DMA
before further mucking with the doc or header files, but again, this
shouldn't be too painless...

Also, since I'll be tinkering with fw_cfg again, and you mentioned
using DT on arm and ACPI on x86 to auto-detect the presence (and location)
of fw_cfg from the guest-side in a related thread:

Right now, I don't think fw_cfg is listed in ACPI, and I would like it
to be. So what should I do, simply figure out how to add a new node
somewhere in the SSDT and submit a patch for that ? Could it be that
simple, or am I missing something ? :)

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Peter Maydell]

On 1 September 2015 at 20:13, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> Also, since I'll be tinkering with fw_cfg again, and you mentioned
> using DT on arm and ACPI on x86 to auto-detect the presence (and location)
> of fw_cfg from the guest-side in a related thread:

I meant DT or ACPI on ARM, actually. I don't know what the x86
approach is for fw_cfg but I think it's just "known address".
(For ARM we don't I think currently expose fw_cfg in the ACPI
tables, but we probably ought to; only needed for the edge
case where you actually wanted to read fw_cfg in the guest
kernel rather than just the guest firmware, though.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Gabriel L. Somlo]

On Tue, Sep 01, 2015 at 09:10:23PM +0100, Peter Maydell wrote:
> On 1 September 2015 at 20:13, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> > Also, since I'll be tinkering with fw_cfg again, and you mentioned
> > using DT on arm and ACPI on x86 to auto-detect the presence (and location)
> > of fw_cfg from the guest-side in a related thread:
> 
> I meant DT or ACPI on ARM, actually. I don't know what the x86
> approach is for fw_cfg but I think it's just "known address".
> (For ARM we don't I think currently expose fw_cfg in the ACPI
> tables, but we probably ought to; only needed for the edge
> case where you actually wanted to read fw_cfg in the guest
> kernel rather than just the guest firmware, though.)

Right, that's what I'm trying to do -- expose fw_cfg in
/sys/firmware/... on the guest -- which involves figuring out how
to determine if it is present (DT on ARM, blindly poking port 0x510
on x86 for now, hence my interest in having it listed in ACPI).

Now, if we did expose fw_cfg in ACPI on *both* ARM and x86, I wonder
I could get away with an ACPI-only detection scheme across both
architectures... But ACPI is all we have on x86, so on the QEMU side
of things it looks like I'll have to add it regardless...

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Peter Maydell]

On 1 September 2015 at 21:27, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> Right, that's what I'm trying to do -- expose fw_cfg in
> /sys/firmware/... on the guest -- which involves figuring out how
> to determine if it is present (DT on ARM, blindly poking port 0x510
> on x86 for now, hence my interest in having it listed in ACPI).
>
> Now, if we did expose fw_cfg in ACPI on *both* ARM and x86, I wonder
> I could get away with an ACPI-only detection scheme across both
> architectures...

Nope, because there's no requirement for ACPI on ARM -- you could
be booting the kernel via device tree.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Gerd Hoffmann]

On Di, 2015-09-01 at 21:10 +0100, Peter Maydell wrote:
> On 1 September 2015 at 20:13, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> > Also, since I'll be tinkering with fw_cfg again, and you mentioned
> > using DT on arm and ACPI on x86 to auto-detect the presence (and location)
> > of fw_cfg from the guest-side in a related thread:
> 
> I meant DT or ACPI on ARM, actually. I don't know what the x86
> approach is for fw_cfg but I think it's just "known address".

Yes, "known address".  And given that the firmware actually loads the
acpi tables via fw_cfg that is very unlikely to change.  Adding it to
the acpi tables might be useful nevertheless so the guest os knows the
ioports are in use.

I think on arm the acpi situation is the same, but I think firmware
detecting the location via DT should work without chicken&egg problems.

cheers,
  Gerd

Re: [PATCH v2] QEMU fw_cfg DMA interface documentation

[Stefan Hajnoczi]

On Mon, Aug 31, 2015 at 10:11 AM, Marc Marí <markmb@redhat.com> wrote:
> Add fw_cfg DMA interface specfication in the fw_cfg documentation.
>
> Signed-off-by: Marc Marí <markmb@redhat.com>
> ---
>  Documentation/devicetree/bindings/arm/fw-cfg.txt | 51 +++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> index 953fb64..766ddbe 100644
> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> @@ -45,6 +45,53 @@ blob to be read from the data register has size 4, and it is to be interpreted
>  as a uint32_t value in little endian byte order. The current value
>  (corresponding to the above outer protocol) is zero.
>
> +If bit 1 of the feature bitmap is set, the DMA interface is present. This
> +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 lower half 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 higher part.
> +
> +In this register, a physical RAM address to a FWCfgDmaAccess structure should
> +be written. This is the format of the FWCfgDmaAccess structure:
> +
> +typedef struct FWCfgDmaAccess {
> +    uint32_t control;
> +    uint32_t length;
> +    uint64_t address;
> +} FWCfgDmaAccess;

I think including the selector field would be nice to avoid extra
register accesses, but I'm not that familiar with fw_cfg so maybe
there's a reason not to include that field.

> +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
> +
> +When an operation is triggered, 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 address specified by the "address" field.

Minor clarification:
s/address/physical RAM address/

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [PATCH v2] QEMU fw_cfg DMA interface documentation

[Marc Marí]

On Wed, 2 Sep 2015 09:20:12 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Mon, Aug 31, 2015 at 10:11 AM, Marc Marí <markmb@redhat.com> wrote:
> > Add fw_cfg DMA interface specfication in the fw_cfg documentation.
> >
> > Signed-off-by: Marc Marí <markmb@redhat.com>
> > ---
> >  Documentation/devicetree/bindings/arm/fw-cfg.txt | 51
> > +++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1
> > deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> > b/Documentation/devicetree/bindings/arm/fw-cfg.txt index
> > 953fb64..766ddbe 100644 ---
> > a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++
> > b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -45,6 +45,53
> > @@ blob to be read from the data register has size 4, and it is to
> > be interpreted as a uint32_t value in little endian byte order. The
> > current value (corresponding to the above outer protocol) is zero.
> >
> > +If bit 1 of the feature bitmap is set, the DMA interface is
> > present. This +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
> > lower half 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 higher part. +
> > +In this register, a physical RAM address to a FWCfgDmaAccess
> > structure should +be written. This is the format of the
> > FWCfgDmaAccess structure: +
> > +typedef struct FWCfgDmaAccess {
> > +    uint32_t control;
> > +    uint32_t length;
> > +    uint64_t address;
> > +} FWCfgDmaAccess;
> 
> I think including the selector field would be nice to avoid extra
> register accesses, but I'm not that familiar with fw_cfg so maybe
> there's a reason not to include that field.

It's just simplicity. If you want to read a few times from the same
field (like in ACPI tables, read the data size and then the data), you
need a way to enable and disable the selector and manage the current
offset for that entry. This is already provided with the "old"
interface.

Moreover, if, for some reason, both systems are being used
simultaneously, some way of interacting both selectors and offsets is
needed.

I think the overhead of writing the selector apart is not that big,
compared to the trouble of adding a new one.

Thanks
Marc
 
> > +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
> > +
> > +When an operation is triggered, 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 address
> > specified by the "address" field.
> 
> Minor clarification:
> s/address/physical RAM address/
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [PATCH v2] QEMU fw_cfg DMA interface documentation

[Marc Marí]

On Wed, 2 Sep 2015 09:20:12 +0100
Stefan Hajnoczi <stefanha-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Mon, Aug 31, 2015 at 10:11 AM, Marc Marí <markmb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > Add fw_cfg DMA interface specfication in the fw_cfg documentation.
> >
> > Signed-off-by: Marc Marí <markmb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  Documentation/devicetree/bindings/arm/fw-cfg.txt | 51
> > +++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1
> > deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> > b/Documentation/devicetree/bindings/arm/fw-cfg.txt index
> > 953fb64..766ddbe 100644 ---
> > a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++
> > b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -45,6 +45,53
> > @@ blob to be read from the data register has size 4, and it is to
> > be interpreted as a uint32_t value in little endian byte order. The
> > current value (corresponding to the above outer protocol) is zero.
> >
> > +If bit 1 of the feature bitmap is set, the DMA interface is
> > present. This +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
> > lower half 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 higher part. +
> > +In this register, a physical RAM address to a FWCfgDmaAccess
> > structure should +be written. This is the format of the
> > FWCfgDmaAccess structure: +
> > +typedef struct FWCfgDmaAccess {
> > +    uint32_t control;
> > +    uint32_t length;
> > +    uint64_t address;
> > +} FWCfgDmaAccess;
> 
> I think including the selector field would be nice to avoid extra
> register accesses, but I'm not that familiar with fw_cfg so maybe
> there's a reason not to include that field.

It's just simplicity. If you want to read a few times from the same
field (like in ACPI tables, read the data size and then the data), you
need a way to enable and disable the selector and manage the current
offset for that entry. This is already provided with the "old"
interface.

Moreover, if, for some reason, both systems are being used
simultaneously, some way of interacting both selectors and offsets is
needed.

I think the overhead of writing the selector apart is not that big,
compared to the trouble of adding a new one.

Thanks
Marc
 
> > +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
> > +
> > +When an operation is triggered, 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 address
> > specified by the "address" field.
> 
> Minor clarification:
> s/address/physical RAM address/
> 
> Reviewed-by: Stefan Hajnoczi <stefanha-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Laszlo Ersek]

On 09/02/15 10:08, Gerd Hoffmann wrote:
> On Di, 2015-09-01 at 21:10 +0100, Peter Maydell wrote:
>> On 1 September 2015 at 20:13, Gabriel L. Somlo <somlo@cmu.edu> wrote:
>>> Also, since I'll be tinkering with fw_cfg again, and you mentioned
>>> using DT on arm and ACPI on x86 to auto-detect the presence (and location)
>>> of fw_cfg from the guest-side in a related thread:
>>
>> I meant DT or ACPI on ARM, actually. I don't know what the x86
>> approach is for fw_cfg but I think it's just "known address".
> 
> Yes, "known address".  And given that the firmware actually loads the
> acpi tables via fw_cfg that is very unlikely to change.  Adding it to
> the acpi tables might be useful nevertheless so the guest os knows the
> ioports are in use.
> 
> I think on arm the acpi situation is the same, but I think firmware
> detecting the location via DT should work without chicken&egg problems.

First of all I apologize for lagging severely behind this thread; in
practice I can't read emails longer than 20 lines or so. This one
qualifies, so I'll respond:

Yes, on qemu-system-(arm|aarch64) -M virt, detecting the fw_cfg
registers via DT should continue to work without any dependencies.

Thanks
Laszlo

Re: [PATCH v2] QEMU fw_cfg DMA interface documentation

[Gerd Hoffmann]

  Hi,

> It's just simplicity. If you want to read a few times from the same
> field (like in ACPI tables, read the data size and then the data), you
> need a way to enable and disable the selector and manage the current
> offset for that entry. This is already provided with the "old"
> interface.

Could be handled with a 'select' control bit.  Only when set select
entry and reset offset to zero.

Also: would it make sense to allow an *array* of FWCfgDmaAccess structs?
With a 'more' bit in control we could indicate that there are more
entries.  I'm not sure firmware would actually use that though ...

cheers,
  Gerd

Re: [PATCH v2] QEMU fw_cfg DMA interface documentation

[Laszlo Ersek]

On 09/07/15 13:08, Gerd Hoffmann wrote:
>   Hi,
> 
>> It's just simplicity. If you want to read a few times from the same
>> field (like in ACPI tables, read the data size and then the data), you
>> need a way to enable and disable the selector and manage the current
>> offset for that entry. This is already provided with the "old"
>> interface.
> 
> Could be handled with a 'select' control bit.  Only when set select
> entry and reset offset to zero.
> 
> Also: would it make sense to allow an *array* of FWCfgDmaAccess structs?
> With a 'more' bit in control we could indicate that there are more
> entries.  I'm not sure firmware would actually use that though ...

Seems far-fetched.

Thanks
Laszlo

> cheers,
>   Gerd
> 
> 

Re: [PATCH v2] QEMU fw_cfg DMA interface documentation

[Laszlo Ersek]

On 09/07/15 13:08, Gerd Hoffmann wrote:
>   Hi,
> 
>> It's just simplicity. If you want to read a few times from the same
>> field (like in ACPI tables, read the data size and then the data), you
>> need a way to enable and disable the selector and manage the current
>> offset for that entry. This is already provided with the "old"
>> interface.
> 
> Could be handled with a 'select' control bit.  Only when set select
> entry and reset offset to zero.
> 
> Also: would it make sense to allow an *array* of FWCfgDmaAccess structs?
> With a 'more' bit in control we could indicate that there are more
> entries.  I'm not sure firmware would actually use that though ...

Seems far-fetched.

Thanks
Laszlo

> cheers,
>   Gerd
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] QEMU fw_cfg DMA interface documentation

[Kevin O'Connor]

On Mon, Sep 07, 2015 at 01:08:29PM +0200, Gerd Hoffmann wrote:
> > It's just simplicity. If you want to read a few times from the same
> > field (like in ACPI tables, read the data size and then the data), you
> > need a way to enable and disable the selector and manage the current
> > offset for that entry. This is already provided with the "old"
> > interface.
> 
> Could be handled with a 'select' control bit.  Only when set select
> entry and reset offset to zero.

I think two features would help "round off" the new fw_cfg DMA
proposal: add a select bit as you describe (that uses the 16 most
significant bits of the "control" field for the "select entry" when
the bit is set), and define a static signature (eg, "QEMU CFG") when
reading the 64bit MMIO dma register.

Both are optional features that don't change the fundamental
interface; I was thinking of sending them as two patches on top of
Marc's next version of his patch series (if no one else gets to it
first).

-Kevin

Re: [PATCH v2] QEMU fw_cfg DMA interface documentation

[Marc Marí]

On Tue, 8 Sep 2015 12:46:08 -0400
"Kevin O'Connor" <kevin@koconnor.net> wrote:

> On Mon, Sep 07, 2015 at 01:08:29PM +0200, Gerd Hoffmann wrote:
> > > It's just simplicity. If you want to read a few times from the
> > > same field (like in ACPI tables, read the data size and then the
> > > data), you need a way to enable and disable the selector and
> > > manage the current offset for that entry. This is already
> > > provided with the "old" interface.
> > 
> > Could be handled with a 'select' control bit.  Only when set select
> > entry and reset offset to zero.
> 
> I think two features would help "round off" the new fw_cfg DMA
> proposal: add a select bit as you describe (that uses the 16 most
> significant bits of the "control" field for the "select entry" when
> the bit is set), and define a static signature (eg, "QEMU CFG") when
> reading the 64bit MMIO dma register.
> 
> Both are optional features that don't change the fundamental
> interface; I was thinking of sending them as two patches on top of
> Marc's next version of his patch series (if no one else gets to it
> first).
> 

As there will be (at least) another version, I can add those simple
changes in the next version.

Thanks
Marc