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 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
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.

[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 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
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.

[Qemu-devel] [PATCH 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     |  53 +++++++++++-
 hw/arm/virt.c             |  13 ++-
 hw/i386/pc.c              |  11 ++-
 hw/nvram/fw_cfg.c         | 212 +++++++++++++++++++++++++++++++++++++++++++---
 include/hw/nvram/fw_cfg.h |  12 ++-
 5 files changed, 278 insertions(+), 23 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 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 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 | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 5bc7b96..dc8051e 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -76,6 +76,7 @@ increasing address order, similar to memcpy().
 
 Selector Register IOport: 0x510
 Data Register IOport:     0x511
+DMA Address IOport:       0x512
 
 == Firmware Configuration Items ==
 
@@ -89,8 +90,9 @@ present, the four bytes read will contain the characters "QEMU".
 === Revision (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.
+revision number. If it is set to 1, the interface is the traditional
+selector / data interface. If it is set to 2, the DMA extension is
+also present.
 
 === File Directory (Key 0x0019, FW_CFG_FILE_DIR) ===
 
@@ -132,6 +134,42 @@ 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 =
+
+For revision value 2, the DMA interface is present. This does not replace
+the existing fw_cfg interface, it is an add-on.
+
+When this interface is enabled the DMA Address register can be used to
+write the address of a FWCfgDmaAccess structure:
+
+typedef struct FWCfgDmaAccess {
+    uint64_t address;
+    uint32_t length;
+    uint32_t control;
+} FWCfgDmaAccess;
+
+If "control" has the bit 1 set (FW_CFG_DMA_CTL_READ), a read operation is
+performed on the selected entry. "length" bytes of data in that fw_cfg
+entry are copied to the address specified by "address".
+
+If the field "address" has value 0, the read is considered a skip, and
+the data is not copied anywhere, but the offset is still incremented.
+
+To check result, read the control register:
+   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 register is zero
+and the address register 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 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.

For x86 arch, this addon will use one extra port (0x512). For ARM, it will
use 8 more bytes, following the actual implementation.

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         | 212 +++++++++++++++++++++++++++++++++++++++++++---
 include/hw/nvram/fw_cfg.h |  12 ++-
 3 files changed, 211 insertions(+), 15 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4846892..374660c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -612,7 +612,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..7399008 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,10 +31,13 @@
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 
-#define FW_CFG_SIZE 2
+#define FW_CFG_SIZE 3
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
+#define FW_CFG_VERSION      1
+#define FW_CFG_VERSION_DMA  2
+
 #define TYPE_FW_CFG     "fw_cfg"
 #define TYPE_FW_CFG_IO  "fw_cfg_io"
 #define TYPE_FW_CFG_MEM "fw_cfg_mem"
@@ -42,6 +46,11 @@
 #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_DMA_CONTROL bits */
+#define FW_CFG_DMA_CTL_ERROR   0x01
+#define FW_CFG_DMA_CTL_READ    0x02
+#define FW_CFG_DMA_CTL_MASK    0x03
+
 typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
@@ -59,6 +68,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 +88,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 +307,108 @@ 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;
+    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);
+    dma = dma_memory_map(s->dma_as, s->dma_addr, &len,
+                                DMA_DIRECTION_FROM_DEVICE);
+
+    if (!dma || !len) {
+        return;
+    }
+
+    if (dma->control & FW_CFG_DMA_CTL_ERROR) {
+        return;
+    }
+
+    if (!(dma->control & FW_CFG_DMA_CTL_READ)) {
+        return;
+    }
+
+    while (dma->length > 0) {
+        if (s->cur_entry == FW_CFG_INVALID || !e->data ||
+                                    s->cur_offset >= e->len) {
+            len = dma->length;
+            if (dma->address) {
+                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->address += len;
+            dma->length  -= 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 (dma->address) {
+                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;
+        }
+    }
+
+    trace_fw_cfg_read(s, 0);
+
+out:
+    dma_memory_unmap(s->dma_as, dma, 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;
+
+    s->dma_addr = be64_to_cpu(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 && addr == 0;
+}
+
 static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
@@ -321,12 +436,20 @@ 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);
+        /* FWCfgDmaAccess address */
+        s = opaque;
+        s->dma_addr = le64_to_cpu(value);
+        fw_cfg_dma_transfer(s);
         break;
     }
 }
@@ -334,7 +457,11 @@ static void fw_cfg_comb_write(void *opaque, hwaddr addr,
 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 (is_write && size == 2 && addr == 0) ||
+            (size == 1 && addr == 1) ||
+            (s->dma_enabled && is_write && addr == 2);
 }
 
 static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
@@ -361,6 +488,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 +534,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 +559,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 +748,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 +759,42 @@ 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;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_IO);
     qdev_prop_set_uint32(dev, "iobase", iobase);
+
+    s = FW_CFG(dev);
     fw_cfg_init1(dev);
 
-    return FW_CFG(dev);
+    if (dma_as) {
+        /* 64 bits for the address field */
+        s->dma_as = dma_as;
+        s->dma_enabled = true;
+
+        fw_cfg_add_i32(s, FW_CFG_ID, FW_CFG_VERSION_DMA);
+    } else {
+        fw_cfg_add_i32(s, FW_CFG_ID, FW_CFG_VERSION);
+    }
+
+    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;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
     qdev_prop_set_uint32(dev, "data_width", data_width);
@@ -633,13 +805,25 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
     sysbus_mmio_map(sbd, 0, ctl_addr);
     sysbus_mmio_map(sbd, 1, data_addr);
 
-    return FW_CFG(dev);
+    s = FW_CFG(dev);
+
+    if (dma_addr && dma_as) {
+        s->dma_as = dma_as;
+        s->dma_enabled = true;
+        sysbus_mmio_map(sbd, 2, dma_addr);
+        fw_cfg_add_i32(s, FW_CFG_ID, FW_CFG_VERSION_DMA);
+    } else {
+        fw_cfg_add_i32(s, FW_CFG_ID, FW_CFG_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);
 }
 
 
@@ -725,6 +909,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..4ce51e2 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -61,6 +61,12 @@ typedef struct FWCfgFiles {
     FWCfgFile f[];
 } FWCfgFiles;
 
+typedef struct FWCfgDmaAccess {
+    uint64_t address;
+    uint32_t length;
+    uint32_t control;
+} FWCfgDmaAccess;
+
 typedef void (*FWCfgCallback)(void *opaque, uint8_t *data);
 typedef void (*FWCfgReadCallback)(void *opaque, uint32_t offset);
 
@@ -77,10 +83,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 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 374660c..eaad274 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -109,7 +109,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, 0x00000012 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -606,13 +606,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, 16, as);
 
     nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
@@ -800,6 +800,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";
@@ -837,6 +838,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);
@@ -889,7 +894,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 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 7661ea9..e5ca805 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -718,7 +718,7 @@ static unsigned int pc_apic_id_limit(unsigned int max_cpus)
     return x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
 }
 
-static FWCfgState *bochs_bios_init(void)
+static FWCfgState *bochs_bios_init(AddressSpace *as)
 {
     FWCfgState *fw_cfg;
     uint8_t *smbios_tables, *smbios_anchor;
@@ -727,7 +727,8 @@ static FWCfgState *bochs_bios_init(void)
     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
@@ -1302,6 +1303,7 @@ FWCfgState *pc_memory_init(MachineState *machine,
     MemoryRegion *ram_below_4g, *ram_above_4g;
     FWCfgState *fw_cfg;
     PCMachineState *pcms = PC_MACHINE(machine);
+    AddressSpace *as;
 
     assert(machine->ram_size == below_4g_mem_size + above_4g_mem_size);
 
@@ -1391,7 +1393,10 @@ FWCfgState *pc_memory_init(MachineState *machine,
                                         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] 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 | 36 ++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
index 953fb64..c880eec 100644
--- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
+++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
@@ -49,6 +49,41 @@ 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.
 
+Starting from revision 2, a DMA interface has also been added. This can be used
+through a write-only, 64-bit wide address register.
+
+In this register, a pointer to a FWCfgDmaAccess structure can be written, in
+big endian mode. This is the format of the FWCfgDmaAccess structure:
+
+typedef struct FWCfgDmaAccess {
+    uint64_t address;
+    uint32_t length;
+    uint32_t control;
+} FWCfgDmaAccess;
+
+Once the address to this structure has been written, an DMA operation is
+started. If the "control" field has value 2, a read operation will be performed.
+"length" bytes for the current selector and offset will be mapped into the
+address specified by the "address" field.
+
+If the field "address" has value 0, the read is considered a skip, and
+the data is not copied anywhere, but the offset is still incremented.
+
+To check result, read the control register:
+   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 register is zero
+and the address register 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.
+
 Required properties:
 
 - compatible: "qemu,fw-cfg-mmio".
@@ -56,6 +91,7 @@ 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.
+  * From revision 2: Bytes 0xa to 0x11 cover the DMA address register.
   * Further registers may be appended to the region in case of future interface
     revisions / feature bits.
 
-- 
2.4.3

Re: [PATCH] QEMU fw_cfg DMA interface documentation

[Stefan Hajnoczi]

On Thu, Aug 6, 2015 at 12:03 PM, 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 | 36 ++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> index 953fb64..c880eec 100644
> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> @@ -49,6 +49,41 @@ The guest kernel is not expected to use these registers (although it is

Please update the "=== Revision (Key 0x0001, FW_CFG_ID) ===" section
to say that currently the revision is 2.

>  certainly allowed to); the device tree bindings are documented here because
>  this is where device tree bindings reside in general.
>
> +Starting from revision 2, a DMA interface has also been added. This can be used
> +through a write-only, 64-bit wide address register.
> +
> +In this register, a pointer to a FWCfgDmaAccess structure can be written, in

s/pointer/physical RAM address/ is clearer

> +big endian mode. This is the format of the FWCfgDmaAccess structure:

Please be explicit about the *order* of 32-bit writes to the 64-bit
DMA register.

Big-endian only defines the layout of bits but it doesn't say in which
order the two 32-bit sub-registers need to be written.

> +typedef struct FWCfgDmaAccess {
> +    uint64_t address;
> +    uint32_t length;
> +    uint32_t control;
> +} FWCfgDmaAccess;
> +
> +Once the address to this structure has been written, an DMA operation is
> +started. If the "control" field has value 2, a read operation will be performed.
> +"length" bytes for the current selector and offset will be mapped into the
> +address specified by the "address" field.

"mapped" might be confusing.  "Copied" or "DMAed" is clearer.

> +If the field "address" has value 0, the read is considered a skip, and
> +the data is not copied anywhere, but the offset is still incremented.
> +
> +To check result, read the control register:

FWCfgDmaAccess.control is not a register, it's a field.

s/register/field/

> +   error bit set     ->  something went wrong.

Which bit number is the error bit?

> +   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 register is zero
> +and the address register points right after the memory block written.

s/register/field/

Re: [PATCH] QEMU fw_cfg DMA interface documentation

[Laszlo Ersek]

On 08/06/15 14:12, Stefan Hajnoczi wrote:
> On Thu, Aug 6, 2015 at 12:03 PM, 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 | 36 ++++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
>> index 953fb64..c880eec 100644
>> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
>> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
>> @@ -49,6 +49,41 @@ The guest kernel is not expected to use these registers (although it is
> 
> Please update the "=== Revision (Key 0x0001, FW_CFG_ID) ===" section
> to say that currently the revision is 2.

Sorry I haven't started reading the series yet, but this caught my eye
-- can we agree that this field should be a bitmap instead, and not a
counter-like value? I don't insist of course, because for the current
use case both approaches will work. But, for "future proofing", I think
it is useful to express features independently of each other. (See eg.
virtio feature flags.)

Just an idea.

Thanks
Laszlo

> 
>>  certainly allowed to); the device tree bindings are documented here because
>>  this is where device tree bindings reside in general.
>>
>> +Starting from revision 2, a DMA interface has also been added. This can be used
>> +through a write-only, 64-bit wide address register.
>> +
>> +In this register, a pointer to a FWCfgDmaAccess structure can be written, in
> 
> s/pointer/physical RAM address/ is clearer
> 
>> +big endian mode. This is the format of the FWCfgDmaAccess structure:
> 
> Please be explicit about the *order* of 32-bit writes to the 64-bit
> DMA register.
> 
> Big-endian only defines the layout of bits but it doesn't say in which
> order the two 32-bit sub-registers need to be written.
> 
>> +typedef struct FWCfgDmaAccess {
>> +    uint64_t address;
>> +    uint32_t length;
>> +    uint32_t control;
>> +} FWCfgDmaAccess;
>> +
>> +Once the address to this structure has been written, an DMA operation is
>> +started. If the "control" field has value 2, a read operation will be performed.
>> +"length" bytes for the current selector and offset will be mapped into the
>> +address specified by the "address" field.
> 
> "mapped" might be confusing.  "Copied" or "DMAed" is clearer.
> 
>> +If the field "address" has value 0, the read is considered a skip, and
>> +the data is not copied anywhere, but the offset is still incremented.
>> +
>> +To check result, read the control register:
> 
> FWCfgDmaAccess.control is not a register, it's a field.
> 
> s/register/field/
> 
>> +   error bit set     ->  something went wrong.
> 
> Which bit number is the error bit?
> 
>> +   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 register is zero
>> +and the address register points right after the memory block written.
> 
> s/register/field/
> 

Re: QEMU fw_cfg DMA interface

[Stefan Hajnoczi]

On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com> wrote:
> When running a Linux guest on top of QEMU, using the -kernel options, this
> is the timing improvement for x86:
>
> QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> 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

Impressive results!

Is this a fully-featured QEMU build or did you disable things?

Is this the default SeaBIOS build or did you disable things?

Stefan

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Stefan Hajnoczi]

On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com> wrote:
> When running a Linux guest on top of QEMU, using the -kernel options, this
> is the timing improvement for x86:
>
> QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> 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

Impressive results!

Is this a fully-featured QEMU build or did you disable things?

Is this the default SeaBIOS build or did you disable things?

Stefan

Re: [PATCH] QEMU fw_cfg DMA interface documentation

[Stefan Hajnoczi]

On Thu, Aug 6, 2015 at 1:22 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 08/06/15 14:12, Stefan Hajnoczi wrote:
>> On Thu, Aug 6, 2015 at 12:03 PM, 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 | 36 ++++++++++++++++++++++++
>>>  1 file changed, 36 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
>>> index 953fb64..c880eec 100644
>>> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
>>> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
>>> @@ -49,6 +49,41 @@ The guest kernel is not expected to use these registers (although it is
>>
>> Please update the "=== Revision (Key 0x0001, FW_CFG_ID) ===" section
>> to say that currently the revision is 2.
>
> Sorry I haven't started reading the series yet, but this caught my eye
> -- can we agree that this field should be a bitmap instead, and not a
> counter-like value? I don't insist of course, because for the current
> use case both approaches will work. But, for "future proofing", I think
> it is useful to express features independently of each other. (See eg.
> virtio feature flags.)

Good idea.

Stefan

Re: QEMU fw_cfg DMA interface

[Marc Marí]

On Thu, 6 Aug 2015 13:27:16 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com> wrote:
> > When running a Linux guest on top of QEMU, using the -kernel
> > options, this is the timing improvement for x86:
> >
> > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> > 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
> 
> Impressive results!
> 
> Is this a fully-featured QEMU build or did you disable things?
> 
> Is this the default SeaBIOS build or did you disable things?
> 

This is the default QEMU configuration I get for my system. It's not a
fully-featured QEMU, but it has a lot of things enabled. SeaBIOS
has a default configuration (with debugging disabled).

The QEMU configuration is:
[...]
tcg debug enabled no
gprof enabled     no
sparse enabled    no
strip binaries    yes
profiler          no
static build      no
pixman            system
SDL support       yes
GTK support       yes
GNUTLS support    yes
GNUTLS hash       yes
GNUTLS gcrypt     no
GNUTLS nettle     yes (2.7.1)
VTE support       yes
curses support    yes
curl support      yes
mingw32 support   no
Audio drivers     oss
Block whitelist (rw)
Block whitelist (ro)
VirtFS support    yes
VNC support       yes
VNC TLS support   yes
VNC SASL support  yes
VNC JPEG support  yes
VNC PNG support   yes
xen support       no
brlapi support    yes
bluez  support    yes
Documentation     no
GUEST_BASE        yes
PIE               yes
vde support       yes
netmap support    no
Linux AIO support yes
ATTR/XATTR support yes
Install blobs     yes
KVM support       yes
RDMA support      yes
TCG interpreter   no
fdt support       yes
preadv support    yes
fdatasync         yes
madvise           yes
posix_madvise     yes
sigev_thread_id   yes
uuid support      yes
libcap-ng support yes
vhost-net support yes
vhost-scsi support yes
Trace backends    nop
spice support     yes (0.12.7/0.12.5)
rbd support       yes
xfsctl support    no
nss used          yes
libusb            yes
usb net redir     yes
OpenGL support    no
libiscsi support  yes
libnfs support    no
build guest agent yes
QGA VSS support   no
QGA w32 disk info no
seccomp support   yes
coroutine backend ucontext
coroutine pool    yes
GlusterFS support yes
Archipelago support no
gcov              gcov
gcov enabled      no
TPM support       yes
libssh2 support   yes
TPM passthrough   yes
QOM debugging     yes
vhdx              yes
lzo support       yes
snappy support    yes
bzip2 support     yes
NUMA host support yes
tcmalloc support  no

Marc

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Marc Marí]

On Thu, 6 Aug 2015 13:27:16 +0100
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com> wrote:
> > When running a Linux guest on top of QEMU, using the -kernel
> > options, this is the timing improvement for x86:
> >
> > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> > 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
> 
> Impressive results!
> 
> Is this a fully-featured QEMU build or did you disable things?
> 
> Is this the default SeaBIOS build or did you disable things?
> 

This is the default QEMU configuration I get for my system. It's not a
fully-featured QEMU, but it has a lot of things enabled. SeaBIOS
has a default configuration (with debugging disabled).

The QEMU configuration is:
[...]
tcg debug enabled no
gprof enabled     no
sparse enabled    no
strip binaries    yes
profiler          no
static build      no
pixman            system
SDL support       yes
GTK support       yes
GNUTLS support    yes
GNUTLS hash       yes
GNUTLS gcrypt     no
GNUTLS nettle     yes (2.7.1)
VTE support       yes
curses support    yes
curl support      yes
mingw32 support   no
Audio drivers     oss
Block whitelist (rw)
Block whitelist (ro)
VirtFS support    yes
VNC support       yes
VNC TLS support   yes
VNC SASL support  yes
VNC JPEG support  yes
VNC PNG support   yes
xen support       no
brlapi support    yes
bluez  support    yes
Documentation     no
GUEST_BASE        yes
PIE               yes
vde support       yes
netmap support    no
Linux AIO support yes
ATTR/XATTR support yes
Install blobs     yes
KVM support       yes
RDMA support      yes
TCG interpreter   no
fdt support       yes
preadv support    yes
fdatasync         yes
madvise           yes
posix_madvise     yes
sigev_thread_id   yes
uuid support      yes
libcap-ng support yes
vhost-net support yes
vhost-scsi support yes
Trace backends    nop
spice support     yes (0.12.7/0.12.5)
rbd support       yes
xfsctl support    no
nss used          yes
libusb            yes
usb net redir     yes
OpenGL support    no
libiscsi support  yes
libnfs support    no
build guest agent yes
QGA VSS support   no
QGA w32 disk info no
seccomp support   yes
coroutine backend ucontext
coroutine pool    yes
GlusterFS support yes
Archipelago support no
gcov              gcov
gcov enabled      no
TPM support       yes
libssh2 support   yes
TPM passthrough   yes
QOM debugging     yes
vhdx              yes
lzo support       yes
snappy support    yes
bzip2 support     yes
NUMA host support yes
tcmalloc support  no

Marc

Re: QEMU fw_cfg DMA interface

[Stefan Hajnoczi]

On Thu, Aug 6, 2015 at 1:37 PM, Marc Marí <markmb@redhat.com> wrote:
> On Thu, 6 Aug 2015 13:27:16 +0100
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
>> On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com> wrote:
>> > When running a Linux guest on top of QEMU, using the -kernel
>> > options, this is the timing improvement for x86:
>> >
>> > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
>> > 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
>>
>> Impressive results!
>>
>> Is this a fully-featured QEMU build or did you disable things?
>>
>> Is this the default SeaBIOS build or did you disable things?
>>
>
> This is the default QEMU configuration I get for my system. It's not a
> fully-featured QEMU, but it has a lot of things enabled. SeaBIOS
> has a default configuration (with debugging disabled).

That's great.

Since SeaBIOS is a default configuration, the remaining BIOS startup
time is amenable to more optimizations in the future.

Stefan

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Stefan Hajnoczi]

On Thu, Aug 6, 2015 at 1:37 PM, Marc Marí <markmb@redhat.com> wrote:
> On Thu, 6 Aug 2015 13:27:16 +0100
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
>> On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com> wrote:
>> > When running a Linux guest on top of QEMU, using the -kernel
>> > options, this is the timing improvement for x86:
>> >
>> > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
>> > 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
>>
>> Impressive results!
>>
>> Is this a fully-featured QEMU build or did you disable things?
>>
>> Is this the default SeaBIOS build or did you disable things?
>>
>
> This is the default QEMU configuration I get for my system. It's not a
> fully-featured QEMU, but it has a lot of things enabled. SeaBIOS
> has a default configuration (with debugging disabled).

That's great.

Since SeaBIOS is a default configuration, the remaining BIOS startup
time is amenable to more optimizations in the future.

Stefan

Re: [PATCH] QEMU fw_cfg DMA interface documentation

[Andrew Jones]

On Thu, Aug 06, 2015 at 01:03:07PM +0200, Marc Marí 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 | 36 ++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> index 953fb64..c880eec 100644
> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> @@ -49,6 +49,41 @@ 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.
>  
> +Starting from revision 2, a DMA interface has also been added. This can be used
> +through a write-only, 64-bit wide address register.
> +
> +In this register, a pointer to a FWCfgDmaAccess structure can be written, in
> +big endian mode. This is the format of the FWCfgDmaAccess structure:
s/mode/format/
> +
> +typedef struct FWCfgDmaAccess {
> +    uint64_t address;
> +    uint32_t length;
> +    uint32_t control;
> +} FWCfgDmaAccess;
> +
> +Once the address to this structure has been written, an DMA operation is
> +started. If the "control" field has value 2, a read operation will be performed.
> +"length" bytes for the current selector and offset will be mapped into the
> +address specified by the "address" field.
> +
> +If the field "address" has value 0, the read is considered a skip, and
> +the data is not copied anywhere, but the offset is still incremented.

So we can't DMA to address == 0? That might not generally be a good
idea, but why limit ourselves? Can't we add another control input for skip
instead? Actually, what inputs are accepted now? READ == 2, WRITE? ??

> +
> +To check result, read the control register:
> +   error bit set     ->  something went wrong.
echo Stefan's which bit question, and also what types of errors? If
there are many, then how about an error bit, plus field of bits for
an error code?

> +   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).
I don't think we need to point out that this isn't implemented yet, but
may be in the future. If that's how it may work, then let's just document
it. And why not specify an in-progress bit?

> +
> +Target address goes up and transfer length goes down as the transfer
> +happens, so after a successful transfer the length register is zero
> +and the address register 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.
> +
>  Required properties:
>  
>  - compatible: "qemu,fw-cfg-mmio".
> @@ -56,6 +91,7 @@ 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.
> +  * From revision 2: Bytes 0xa to 0x11 cover the DMA address register.

I think we should naturally align the register, i.e we can reserve bytes
0xa - 0xf, and then put the DMA register at 0x10.

>    * Further registers may be appended to the region in case of future interface
>      revisions / feature bits.
>  
> -- 
> 2.4.3
> 

Thanks,
drew

Re: [PATCH] QEMU fw_cfg DMA interface documentation

[Marc Marí]

On Thu, 6 Aug 2015 16:08:49 +0200
Andrew Jones <drjones@redhat.com> wrote:

> On Thu, Aug 06, 2015 at 01:03:07PM +0200, Marc Marí 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 | 36
> > ++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> > b/Documentation/devicetree/bindings/arm/fw-cfg.txt index
> > 953fb64..c880eec 100644 ---
> > a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++
> > b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -49,6 +49,41
> > @@ 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. +Starting from revision 2, a DMA interface has also
> > been added. This can be used +through a write-only, 64-bit wide
> > address register. +
> > +In this register, a pointer to a FWCfgDmaAccess structure can be
> > written, in +big endian mode. This is the format of the
> > FWCfgDmaAccess structure:
> s/mode/format/
> > +
> > +typedef struct FWCfgDmaAccess {
> > +    uint64_t address;
> > +    uint32_t length;
> > +    uint32_t control;
> > +} FWCfgDmaAccess;
> > +
> > +Once the address to this structure has been written, an DMA
> > operation is +started. If the "control" field has value 2, a read
> > operation will be performed. +"length" bytes for the current
> > selector and offset will be mapped into the +address specified by
> > the "address" field. +
> > +If the field "address" has value 0, the read is considered a skip,
> > and +the data is not copied anywhere, but the offset is still
> > incremented.
> 
> So we can't DMA to address == 0? That might not generally be a good
> idea, but why limit ourselves? Can't we add another control input for
> skip instead? Actually, what inputs are accepted now? READ == 2,
> WRITE? ??
> 

Write was already disabled for PIO:

static void fw_cfg_write(FWCfgState *s, uint8_t value)
{
    /* nothing, write support removed in QEMU v2.4+ */
}

> > +
> > +To check result, read the control register:
> > +   error bit set     ->  something went wrong.
> echo Stefan's which bit question, and also what types of errors? If
> there are many, then how about an error bit, plus field of bits for
> an error code?
> 

Bit 0. At this moment the only error possible is DMA mapping failure.
But its true that it might be useful to have some bits in the control
field or another field to indicate the type of error, in case of future
extensions.

> > +   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).
> I don't think we need to point out that this isn't implemented yet,
> but may be in the future. If that's how it may work, then let's just
> document it. And why not specify an in-progress bit?

Is this a feature we will want?

> > +
> > +Target address goes up and transfer length goes down as the
> > transfer +happens, so after a successful transfer the length
> > register is zero +and the address register 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.
> > +
> >  Required properties:
> >  
> >  - compatible: "qemu,fw-cfg-mmio".
> > @@ -56,6 +91,7 @@ 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.
> > +  * From revision 2: Bytes 0xa to 0x11 cover the DMA address
> > register.
> 
> I think we should naturally align the register, i.e we can reserve
> bytes 0xa - 0xf, and then put the DMA register at 0x10.
> 
> >    * Further registers may be appended to the region in case of
> > future interface revisions / feature bits.
> >  
> > -- 
> > 2.4.3
> > 
> 

Thanks
Marc

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

[Kevin O'Connor]

On Thu, Aug 06, 2015 at 01:01:15PM +0200, Marc Marí 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 | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 5bc7b96..dc8051e 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -76,6 +76,7 @@ increasing address order, similar to memcpy().
>  
>  Selector Register IOport: 0x510
>  Data Register IOport:     0x511
> +DMA Address IOport:       0x512
>  
>  == Firmware Configuration Items ==
>  
> @@ -89,8 +90,9 @@ present, the four bytes read will contain the characters "QEMU".
>  === Revision (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.
> +revision number. If it is set to 1, the interface is the traditional
> +selector / data interface. If it is set to 2, the DMA extension is
> +also present.
>  
>  === File Directory (Key 0x0019, FW_CFG_FILE_DIR) ===
>  
> @@ -132,6 +134,42 @@ 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 =
> +
> +For revision value 2, the DMA interface is present. This does not replace
> +the existing fw_cfg interface, it is an add-on.
> +
> +When this interface is enabled the DMA Address register can be used to
> +write the address of a FWCfgDmaAccess structure:
> +
> +typedef struct FWCfgDmaAccess {
> +    uint64_t address;
> +    uint32_t length;
> +    uint32_t control;
> +} FWCfgDmaAccess;
> +
> +If "control" has the bit 1 set (FW_CFG_DMA_CTL_READ), a read operation is
> +performed on the selected entry. "length" bytes of data in that fw_cfg
> +entry are copied to the address specified by "address".
> +
> +If the field "address" has value 0, the read is considered a skip, and
> +the data is not copied anywhere, but the offset is still incremented.

Thanks!

I have a few suggestions on the interface:

- I think it would be better to place the 'control' field first (ie,
  control/length/address instead of address/length/control).  Placing
  the 'control' field first makes potential future extensions easier -
  that is, it would make it easier for future control bits to change
  the layout of the struct.

- It would be good to add a 'select' field to the struct.  I think
  this could be done by using a 'u16 control; u16 select' instead of
  the current 'u32 control'.  It's common for guests to select a
  fw_cfg entry and read its entire contents - with the 'select' field
  in the struct this could be done with a single guest/host fault.  A
  new control bit could be added (eg, FW_CFG_DMA_CTL_SELECT) to
  determine whether or not the select field is used - iff the bit is
  set then the given fw_cfg entry is selected and the read position is
  reset to the start prior to the DMA data copy.

- Instead of using address==0 to check for skip, I think a new control
  bit would be a little more friendly.  That is, one could add a new
  control bit FW_CFG_DMA_CTL_COPY, and iff it is set then do the DMA
  style copy to the 'address' field.  If it is not set, then 'address'
  is ignored and effectively the read position is just incremented.

-Kevin

Re: [PATCH] QEMU fw_cfg DMA interface documentation

[Andrew Jones]

On Thu, Aug 06, 2015 at 04:19:18PM +0200, Marc Marí wrote:
> On Thu, 6 Aug 2015 16:08:49 +0200
> Andrew Jones <drjones@redhat.com> wrote:
> 
> > On Thu, Aug 06, 2015 at 01:03:07PM +0200, Marc Marí 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 | 36
> > > ++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> > > b/Documentation/devicetree/bindings/arm/fw-cfg.txt index
> > > 953fb64..c880eec 100644 ---
> > > a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++
> > > b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -49,6 +49,41
> > > @@ 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. +Starting from revision 2, a DMA interface has also
> > > been added. This can be used +through a write-only, 64-bit wide
> > > address register. +
> > > +In this register, a pointer to a FWCfgDmaAccess structure can be
> > > written, in +big endian mode. This is the format of the
> > > FWCfgDmaAccess structure:
> > s/mode/format/
> > > +
> > > +typedef struct FWCfgDmaAccess {
> > > +    uint64_t address;
> > > +    uint32_t length;
> > > +    uint32_t control;
> > > +} FWCfgDmaAccess;
> > > +
> > > +Once the address to this structure has been written, an DMA
> > > operation is +started. If the "control" field has value 2, a read
> > > operation will be performed. +"length" bytes for the current
> > > selector and offset will be mapped into the +address specified by
> > > the "address" field. +
> > > +If the field "address" has value 0, the read is considered a skip,
> > > and +the data is not copied anywhere, but the offset is still
> > > incremented.
> > 
> > So we can't DMA to address == 0? That might not generally be a good
> > idea, but why limit ourselves? Can't we add another control input for
> > skip instead? Actually, what inputs are accepted now? READ == 2,
> > WRITE? ??
> > 
> 
> Write was already disabled for PIO:
> 
> static void fw_cfg_write(FWCfgState *s, uint8_t value)
> {
>     /* nothing, write support removed in QEMU v2.4+ */
> }
> 
> > > +
> > > +To check result, read the control register:
> > > +   error bit set     ->  something went wrong.
> > echo Stefan's which bit question, and also what types of errors? If
> > there are many, then how about an error bit, plus field of bits for
> > an error code?
> > 
> 
> Bit 0. At this moment the only error possible is DMA mapping failure.
> But its true that it might be useful to have some bits in the control
> field or another field to indicate the type of error, in case of future
> extensions.
> 
> > > +   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).
> > I don't think we need to point out that this isn't implemented yet,
> > but may be in the future. If that's how it may work, then let's just
> > document it. And why not specify an in-progress bit?
> 
> Is this a feature we will want?

I don't know. Need firmware person like Laszlo to give an opinion. Maybe
he'd want OVMF/AAVMF to be able to output progress while transferring,
to keep users more patient?

> 
> > > +
> > > +Target address goes up and transfer length goes down as the
> > > transfer +happens, so after a successful transfer the length
> > > register is zero +and the address register 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.
> > > +
> > >  Required properties:
> > >  
> > >  - compatible: "qemu,fw-cfg-mmio".
> > > @@ -56,6 +91,7 @@ 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.
> > > +  * From revision 2: Bytes 0xa to 0x11 cover the DMA address
> > > register.
> > 
> > I think we should naturally align the register, i.e we can reserve
> > bytes 0xa - 0xf, and then put the DMA register at 0x10.
> > 
> > >    * Further registers may be appended to the region in case of
> > > future interface revisions / feature bits.
> > >  
> > > -- 
> > > 2.4.3
> > > 
> > 
> 
> Thanks
> Marc

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

[Kevin O'Connor]

On Thu, Aug 06, 2015 at 01:01:16PM +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.
> 
> For x86 arch, this addon will use one extra port (0x512). For ARM, it will
> use 8 more bytes, following the actual implementation.
> 
> 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         | 212 +++++++++++++++++++++++++++++++++++++++++++---
>  include/hw/nvram/fw_cfg.h |  12 ++-
>  3 files changed, 211 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 4846892..374660c 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -612,7 +612,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..7399008 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,10 +31,13 @@
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
>  
> -#define FW_CFG_SIZE 2
> +#define FW_CFG_SIZE 3
>  #define FW_CFG_NAME "fw_cfg"
>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>  
> +#define FW_CFG_VERSION      1
> +#define FW_CFG_VERSION_DMA  2
> +
>  #define TYPE_FW_CFG     "fw_cfg"
>  #define TYPE_FW_CFG_IO  "fw_cfg_io"
>  #define TYPE_FW_CFG_MEM "fw_cfg_mem"
> @@ -42,6 +46,11 @@
>  #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_DMA_CONTROL bits */
> +#define FW_CFG_DMA_CTL_ERROR   0x01
> +#define FW_CFG_DMA_CTL_READ    0x02
> +#define FW_CFG_DMA_CTL_MASK    0x03
> +
>  typedef struct FWCfgEntry {
>      uint32_t len;
>      uint8_t *data;
> @@ -59,6 +68,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 +88,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 +307,108 @@ 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;
> +    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);
> +    dma = dma_memory_map(s->dma_as, s->dma_addr, &len,
> +                                DMA_DIRECTION_FROM_DEVICE);
> +
> +    if (!dma || !len) {
> +        return;
> +    }
> +
> +    if (dma->control & FW_CFG_DMA_CTL_ERROR) {

I think the dma structure would need to be copied to a local instance
of the struct and the fields endianness updated.  (Also, I think the
documentation should state the required endianness of the fields.)
I'm not an expert at this, but if this is not done, I think there
could be problems both with endian and with unaligned memory accesses
in the host.

> +    while (dma->length > 0) {
> +        if (s->cur_entry == FW_CFG_INVALID || !e->data ||
> +                                    s->cur_offset >= e->len) {
> +            len = dma->length;
> +            if (dma->address) {
> +                ptr = dma_memory_map(s->dma_as, dma->address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma->control |= FW_CFG_DMA_CTL_ERROR;

Can you write to "dma->control" if the memory is mapped with
DMA_DIRECTION_FROM_DEVICE?  Also, since the control field update
should be atomic, I think the dma struct should probably always be at
least 4 byte aligned and the documentation should be updated to
reflect that.

[...]
> @@ -321,12 +436,20 @@ 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);
> +        /* FWCfgDmaAccess address */
> +        s = opaque;
> +        s->dma_addr = le64_to_cpu(value);
> +        fw_cfg_dma_transfer(s);
>          break;

There is no ability to perfrom 64bit IO writes on x86 (to the best of
my knowledge).  So, I think this code needs to be able to handle a
32bit write to a high bits address and then store those bits until the
32bit write to the low bits address occurs.  (I'd also recommend that
after every dma transfer the stored high bits are reset to zero so
that the common case of a 32bit address can be performed with a single
32bit write to the low bits field.)

Also, it's very unusual to see 32bit writes to an unaligned IO address
- I think two pad bytes should be added so that the offset for the dma
address is at position 4 (instead of 2).

-Kevin

Re: [PATCH] QEMU fw_cfg DMA interface documentation

[Laszlo Ersek]

On 08/06/15 16:28, Andrew Jones wrote:
> On Thu, Aug 06, 2015 at 04:19:18PM +0200, Marc Marí wrote:
>> On Thu, 6 Aug 2015 16:08:49 +0200
>> Andrew Jones <drjones@redhat.com> wrote:
>>
>>> On Thu, Aug 06, 2015 at 01:03:07PM +0200, Marc Marí 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 | 36
>>>> ++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt
>>>> b/Documentation/devicetree/bindings/arm/fw-cfg.txt index
>>>> 953fb64..c880eec 100644 ---
>>>> a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++
>>>> b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -49,6 +49,41
>>>> @@ 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. +Starting from revision 2, a DMA interface has also
>>>> been added. This can be used +through a write-only, 64-bit wide
>>>> address register. +
>>>> +In this register, a pointer to a FWCfgDmaAccess structure can be
>>>> written, in +big endian mode. This is the format of the
>>>> FWCfgDmaAccess structure:
>>> s/mode/format/
>>>> +
>>>> +typedef struct FWCfgDmaAccess {
>>>> +    uint64_t address;
>>>> +    uint32_t length;
>>>> +    uint32_t control;
>>>> +} FWCfgDmaAccess;
>>>> +
>>>> +Once the address to this structure has been written, an DMA
>>>> operation is +started. If the "control" field has value 2, a read
>>>> operation will be performed. +"length" bytes for the current
>>>> selector and offset will be mapped into the +address specified by
>>>> the "address" field. +
>>>> +If the field "address" has value 0, the read is considered a skip,
>>>> and +the data is not copied anywhere, but the offset is still
>>>> incremented.
>>>
>>> So we can't DMA to address == 0? That might not generally be a good
>>> idea, but why limit ourselves? Can't we add another control input for
>>> skip instead? Actually, what inputs are accepted now? READ == 2,
>>> WRITE? ??
>>>
>>
>> Write was already disabled for PIO:
>>
>> static void fw_cfg_write(FWCfgState *s, uint8_t value)
>> {
>>     /* nothing, write support removed in QEMU v2.4+ */
>> }
>>
>>>> +
>>>> +To check result, read the control register:
>>>> +   error bit set     ->  something went wrong.
>>> echo Stefan's which bit question, and also what types of errors? If
>>> there are many, then how about an error bit, plus field of bits for
>>> an error code?
>>>
>>
>> Bit 0. At this moment the only error possible is DMA mapping failure.
>> But its true that it might be useful to have some bits in the control
>> field or another field to indicate the type of error, in case of future
>> extensions.
>>
>>>> +   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).
>>> I don't think we need to point out that this isn't implemented yet,
>>> but may be in the future. If that's how it may work, then let's just
>>> document it. And why not specify an in-progress bit?
>>
>> Is this a feature we will want?
> 
> I don't know. Need firmware person like Laszlo to give an opinion. Maybe
> he'd want OVMF/AAVMF to be able to output progress while transferring,
> to keep users more patient?

I don't yet know how to use this interface from the firmware.

FWIW the library interface to fw_cfg that we use in OVMF & AAVMF is
synchronous. (It doesn't make sense to do "something else" until the
fw_cfg transfer completes "in the background".)

So I imagined the new interface would behave in one of the following ways:
(1) I perform the "final" MMIO register write, and bam, by the time the
    next instruction is executed, the target memory area is populated.
(2) Or, I perform the same final MMIO register write, and then
    busy-loop on reading some other status register, until it tells me
    that the transfer is done, or there has been an error.

(Under (2), if there's an error, I'll just log an error message, and
hang the firmware. Our internal library interface doesn't return any
status codes (no errors are possible with the MMIO/PIO interfaces, on
ARM and x86 alike), and I don't think this change warrants updating a
zillion call sites, in order to propagate and handle such DMA errors in
all modules that use fw-cfg. (We always check return values -- unless
the return type is VOID. And it is now.))

Regarding progress info: aside from the kernel and the initrd, the
fw-cfg items (files included) are tiny. (Remember, they were meant as
*config* items :)) So, normally, I don't bother about progress info.

The kernel and the initrd are exceptions. So, at the moment the code
that reads them (which is specific Boot Device Selection library code,
not generic fw-cfg client libary code) breaks up the transfer into
chunks of 1MB, and logs progress info in between. That is, the progress
info is not emitted by the direct fw-cfg client, it's printed by the
specific module that relies on the fw-cfg library.

So, regardless of whether I'll have to write (1) or (2), I expect this
same approach to just work with the DMA interface. The DMA thing will be
hidden inside the library, the API will remain synchronous. At best the
module (in BDS) that downloads the kernel/initrd blobs will print those
1MB progress messages "super fast".

Summary: the only thing I need to know is when a transfer has finished,
successfully or unsuccessfully.

(If a transfer can terminate prematurely, but still successfully -- ie.
the firwmare should contine "with the rest" -- then the exact size of
the short transfer should also be returned. But I don't think short
transfers would be very useful.)

Thanks
Laszlo



> 
>>
>>>> +
>>>> +Target address goes up and transfer length goes down as the
>>>> transfer +happens, so after a successful transfer the length
>>>> register is zero +and the address register 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.
>>>> +
>>>>  Required properties:
>>>>  
>>>>  - compatible: "qemu,fw-cfg-mmio".
>>>> @@ -56,6 +91,7 @@ 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.
>>>> +  * From revision 2: Bytes 0xa to 0x11 cover the DMA address
>>>> register.
>>>
>>> I think we should naturally align the register, i.e we can reserve
>>> bytes 0xa - 0xf, and then put the DMA register at 0x10.
>>>
>>>>    * Further registers may be appended to the region in case of
>>>> future interface revisions / feature bits.
>>>>  
>>>> -- 
>>>> 2.4.3
>>>>
>>>
>>
>> Thanks
>> Marc

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

[Marc Marí]

On Thu, 6 Aug 2015 10:47:21 -0400
"Kevin O'Connor" <kevin@koconnor.net> wrote:

> On Thu, Aug 06, 2015 at 01:01:16PM +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.
> > 
> > For x86 arch, this addon will use one extra port (0x512). For ARM,
> > it will use 8 more bytes, following the actual implementation.
> > 
> > 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         | 212
> > +++++++++++++++++++++++++++++++++++++++++++---
> > include/hw/nvram/fw_cfg.h |  12 ++- 3 files changed, 211
> > insertions(+), 15 deletions(-)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 4846892..374660c 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -612,7 +612,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..7399008 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,10 +31,13 @@
> >  #include "qemu/error-report.h"
> >  #include "qemu/config-file.h"
> >  
> > -#define FW_CFG_SIZE 2
> > +#define FW_CFG_SIZE 3
> >  #define FW_CFG_NAME "fw_cfg"
> >  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
> >  
> > +#define FW_CFG_VERSION      1
> > +#define FW_CFG_VERSION_DMA  2
> > +
> >  #define TYPE_FW_CFG     "fw_cfg"
> >  #define TYPE_FW_CFG_IO  "fw_cfg_io"
> >  #define TYPE_FW_CFG_MEM "fw_cfg_mem"
> > @@ -42,6 +46,11 @@
> >  #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_DMA_CONTROL bits */
> > +#define FW_CFG_DMA_CTL_ERROR   0x01
> > +#define FW_CFG_DMA_CTL_READ    0x02
> > +#define FW_CFG_DMA_CTL_MASK    0x03
> > +
> >  typedef struct FWCfgEntry {
> >      uint32_t len;
> >      uint8_t *data;
> > @@ -59,6 +68,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 +88,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 +307,108 @@ 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;
> > +    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);
> > +    dma = dma_memory_map(s->dma_as, s->dma_addr, &len,
> > +                                DMA_DIRECTION_FROM_DEVICE);
> > +
> > +    if (!dma || !len) {
> > +        return;
> > +    }
> > +
> > +    if (dma->control & FW_CFG_DMA_CTL_ERROR) {
> 
> I think the dma structure would need to be copied to a local instance
> of the struct and the fields endianness updated.  (Also, I think the
> documentation should state the required endianness of the fields.)
> I'm not an expert at this, but if this is not done, I think there
> could be problems both with endian and with unaligned memory accesses
> in the host.
> 
> > +    while (dma->length > 0) {
> > +        if (s->cur_entry == FW_CFG_INVALID || !e->data ||
> > +                                    s->cur_offset >= e->len) {
> > +            len = dma->length;
> > +            if (dma->address) {
> > +                ptr = dma_memory_map(s->dma_as, dma->address, &len,
> > +                                     DMA_DIRECTION_FROM_DEVICE);
> > +                if (!ptr || !len) {
> > +                    dma->control |= FW_CFG_DMA_CTL_ERROR;
> 
> Can you write to "dma->control" if the memory is mapped with
> DMA_DIRECTION_FROM_DEVICE?  Also, since the control field update
> should be atomic, I think the dma struct should probably always be at
> least 4 byte aligned and the documentation should be updated to
> reflect that.
> 
> [...]
> > @@ -321,12 +436,20 @@ 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);
> > +        /* FWCfgDmaAccess address */
> > +        s = opaque;
> > +        s->dma_addr = le64_to_cpu(value);
> > +        fw_cfg_dma_transfer(s);
> >          break;
> 
> There is no ability to perfrom 64bit IO writes on x86 (to the best of
> my knowledge).  So, I think this code needs to be able to handle a
> 32bit write to a high bits address and then store those bits until the
> 32bit write to the low bits address occurs.  (I'd also recommend that
> after every dma transfer the stored high bits are reset to zero so
> that the common case of a 32bit address can be performed with a single
> 32bit write to the low bits field.)
> 
> Also, it's very unusual to see 32bit writes to an unaligned IO address
> - I think two pad bytes should be added so that the offset for the dma
> address is at position 4 (instead of 2).

This is a PIO port (out), not a MMIO access (write). Maybe I'm wrong,
but I don't think it matters to have the port number aligned with the
write size.

Thanks
Marc

Re: [PATCH] QEMU fw_cfg DMA interface documentation

[Marc Marí]

On Thu, 6 Aug 2015 16:55:20 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 08/06/15 16:28, Andrew Jones wrote:
> > On Thu, Aug 06, 2015 at 04:19:18PM +0200, Marc Marí wrote:
> >> On Thu, 6 Aug 2015 16:08:49 +0200
> >> Andrew Jones <drjones@redhat.com> wrote:
> >>
> >>> On Thu, Aug 06, 2015 at 01:03:07PM +0200, Marc Marí 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 | 36
> >>>> ++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> >>>> b/Documentation/devicetree/bindings/arm/fw-cfg.txt index
> >>>> 953fb64..c880eec 100644 ---
> >>>> a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++
> >>>> b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -49,6
> >>>> +49,41 @@ 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. +Starting from revision 2, a DMA interface
> >>>> has also been added. This can be used +through a write-only,
> >>>> 64-bit wide address register. +
> >>>> +In this register, a pointer to a FWCfgDmaAccess structure can be
> >>>> written, in +big endian mode. This is the format of the
> >>>> FWCfgDmaAccess structure:
> >>> s/mode/format/
> >>>> +
> >>>> +typedef struct FWCfgDmaAccess {
> >>>> +    uint64_t address;
> >>>> +    uint32_t length;
> >>>> +    uint32_t control;
> >>>> +} FWCfgDmaAccess;
> >>>> +
> >>>> +Once the address to this structure has been written, an DMA
> >>>> operation is +started. If the "control" field has value 2, a read
> >>>> operation will be performed. +"length" bytes for the current
> >>>> selector and offset will be mapped into the +address specified by
> >>>> the "address" field. +
> >>>> +If the field "address" has value 0, the read is considered a
> >>>> skip, and +the data is not copied anywhere, but the offset is
> >>>> still incremented.
> >>>
> >>> So we can't DMA to address == 0? That might not generally be a
> >>> good idea, but why limit ourselves? Can't we add another control
> >>> input for skip instead? Actually, what inputs are accepted now?
> >>> READ == 2, WRITE? ??
> >>>
> >>
> >> Write was already disabled for PIO:
> >>
> >> static void fw_cfg_write(FWCfgState *s, uint8_t value)
> >> {
> >>     /* nothing, write support removed in QEMU v2.4+ */
> >> }
> >>
> >>>> +
> >>>> +To check result, read the control register:
> >>>> +   error bit set     ->  something went wrong.
> >>> echo Stefan's which bit question, and also what types of errors?
> >>> If there are many, then how about an error bit, plus field of
> >>> bits for an error code?
> >>>
> >>
> >> Bit 0. At this moment the only error possible is DMA mapping
> >> failure. But its true that it might be useful to have some bits in
> >> the control field or another field to indicate the type of error,
> >> in case of future extensions.
> >>
> >>>> +   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).
> >>> I don't think we need to point out that this isn't implemented
> >>> yet, but may be in the future. If that's how it may work, then
> >>> let's just document it. And why not specify an in-progress bit?
> >>
> >> Is this a feature we will want?
> > 
> > I don't know. Need firmware person like Laszlo to give an opinion.
> > Maybe he'd want OVMF/AAVMF to be able to output progress while
> > transferring, to keep users more patient?
> 
> I don't yet know how to use this interface from the firmware.
> 
> FWIW the library interface to fw_cfg that we use in OVMF & AAVMF is
> synchronous. (It doesn't make sense to do "something else" until the
> fw_cfg transfer completes "in the background".)
> 
> So I imagined the new interface would behave in one of the following
> ways: (1) I perform the "final" MMIO register write, and bam, by the
> time the next instruction is executed, the target memory area is
> populated. (2) Or, I perform the same final MMIO register write, and
> then busy-loop on reading some other status register, until it tells
> me that the transfer is done, or there has been an error.
> 

It's (2).

If you have library functions such as "fw_cfg_read", just changing
them should leave everything working. You may want to look at the x86
guest (SeaBIOS) to check the few changes necessary:

http://www.seabios.org/pipermail/seabios/2015-August/009605.html

But it's important to note that the host part (QEMU) is implemented
using memcpy. When you implement this interface, you may want to
increase the chunk size.

Thanks
Marc

Re: QEMU fw_cfg DMA interface

[Kevin O'Connor]

On Thu, Aug 06, 2015 at 02:37:45PM +0200, Marc Marí wrote:
> On Thu, 6 Aug 2015 13:27:16 +0100
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> > On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com> wrote:
> > > When running a Linux guest on top of QEMU, using the -kernel
> > > options, this is the timing improvement for x86:
> > >
> > > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> > > 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
> > 
> > Impressive results!
> > 
> > Is this a fully-featured QEMU build or did you disable things?
> > 
> > Is this the default SeaBIOS build or did you disable things?
> > 
> 
> This is the default QEMU configuration I get for my system. It's not a
> fully-featured QEMU, but it has a lot of things enabled. SeaBIOS
> has a default configuration (with debugging disabled).

Thanks!

What qemu command-line did you use during testing?  Also, do you have
a quick primer on how to use the kvm trace stuff to obtain timings?

-Kevin

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Kevin O'Connor]

On Thu, Aug 06, 2015 at 02:37:45PM +0200, Marc Marí wrote:
> On Thu, 6 Aug 2015 13:27:16 +0100
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> > On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com> wrote:
> > > When running a Linux guest on top of QEMU, using the -kernel
> > > options, this is the timing improvement for x86:
> > >
> > > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> > > 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
> > 
> > Impressive results!
> > 
> > Is this a fully-featured QEMU build or did you disable things?
> > 
> > Is this the default SeaBIOS build or did you disable things?
> > 
> 
> This is the default QEMU configuration I get for my system. It's not a
> fully-featured QEMU, but it has a lot of things enabled. SeaBIOS
> has a default configuration (with debugging disabled).

Thanks!

What qemu command-line did you use during testing?  Also, do you have
a quick primer on how to use the kvm trace stuff to obtain timings?

-Kevin

Re: QEMU fw_cfg DMA interface

[Marc Marí]

[-- Attachment #1: Type: text/plain, Size: 2332 bytes --]

On Thu, 6 Aug 2015 11:30:43 -0400
"Kevin O'Connor" <kevin@koconnor.net> wrote:

> On Thu, Aug 06, 2015 at 02:37:45PM +0200, Marc Marí wrote:
> > On Thu, 6 Aug 2015 13:27:16 +0100
> > Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > 
> > > On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com>
> > > wrote:
> > > > When running a Linux guest on top of QEMU, using the -kernel
> > > > options, this is the timing improvement for x86:
> > > >
> > > > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> > > > 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
> > > 
> > > Impressive results!
> > > 
> > > Is this a fully-featured QEMU build or did you disable things?
> > > 
> > > Is this the default SeaBIOS build or did you disable things?
> > > 
> > 
> > This is the default QEMU configuration I get for my system. It's
> > not a fully-featured QEMU, but it has a lot of things enabled.
> > SeaBIOS has a default configuration (with debugging disabled).
> 
> Thanks!
> 
> What qemu command-line did you use during testing?  Also, do you have
> a quick primer on how to use the kvm trace stuff to obtain timings?
> 

The command line I used is:

x86_64-softmmu/qemu-system-x86_64 --enable-kvm \
    -kernel /boot/vmlinuz-4.0.7-300.fc22.x86_64 \
    -L pc-bios/optionrom/ \
    -bios roms/seabios/out/bios.bin -nographic

And I used perf (and two out instructions in the BIOS) to measure the
times:

perf record -a -e kvm:\* -e sched:sched_process_exec

And searching for sched:sched_process_exec, kvm:kvm_entry, pio_write at
0xf5 and pio_write at 0xf4. Out at 0xf5 is the one in "do_boot"
function, and out at 0xf4 is the one just before the jump to the Linux
kernel.

I attach the script I've been using. It can be improved, but it works.
It can be run like this:

sudo ../../results/run_test.sh x86_64-softmmu/qemu-system-x86_64 \
    --enable-kvm -kernel /boot/vmlinuz-4.0.8-300.fc22.x86_64 \
    -L pc-bios/optionrom/ \
    -bios roms/seabios/out/bios.bin -nographic

Thanks
Marc

[-- Attachment #2: run_test.sh --]
[-- Type: application/x-shellscript, Size: 1401 bytes --]

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Marc Marí]

[-- Attachment #1: Type: text/plain, Size: 2332 bytes --]

On Thu, 6 Aug 2015 11:30:43 -0400
"Kevin O'Connor" <kevin@koconnor.net> wrote:

> On Thu, Aug 06, 2015 at 02:37:45PM +0200, Marc Marí wrote:
> > On Thu, 6 Aug 2015 13:27:16 +0100
> > Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > 
> > > On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com>
> > > wrote:
> > > > When running a Linux guest on top of QEMU, using the -kernel
> > > > options, this is the timing improvement for x86:
> > > >
> > > > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> > > > 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
> > > 
> > > Impressive results!
> > > 
> > > Is this a fully-featured QEMU build or did you disable things?
> > > 
> > > Is this the default SeaBIOS build or did you disable things?
> > > 
> > 
> > This is the default QEMU configuration I get for my system. It's
> > not a fully-featured QEMU, but it has a lot of things enabled.
> > SeaBIOS has a default configuration (with debugging disabled).
> 
> Thanks!
> 
> What qemu command-line did you use during testing?  Also, do you have
> a quick primer on how to use the kvm trace stuff to obtain timings?
> 

The command line I used is:

x86_64-softmmu/qemu-system-x86_64 --enable-kvm \
    -kernel /boot/vmlinuz-4.0.7-300.fc22.x86_64 \
    -L pc-bios/optionrom/ \
    -bios roms/seabios/out/bios.bin -nographic

And I used perf (and two out instructions in the BIOS) to measure the
times:

perf record -a -e kvm:\* -e sched:sched_process_exec

And searching for sched:sched_process_exec, kvm:kvm_entry, pio_write at
0xf5 and pio_write at 0xf4. Out at 0xf5 is the one in "do_boot"
function, and out at 0xf4 is the one just before the jump to the Linux
kernel.

I attach the script I've been using. It can be improved, but it works.
It can be run like this:

sudo ../../results/run_test.sh x86_64-softmmu/qemu-system-x86_64 \
    --enable-kvm -kernel /boot/vmlinuz-4.0.8-300.fc22.x86_64 \
    -L pc-bios/optionrom/ \
    -bios roms/seabios/out/bios.bin -nographic

Thanks
Marc

[-- Attachment #2: run_test.sh --]
[-- Type: application/x-shellscript, Size: 1401 bytes --]

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

[Laszlo Ersek]

On 08/06/15 13:01, 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.
> 
> For x86 arch, this addon will use one extra port (0x512). For ARM, it will
> use 8 more bytes, following the actual implementation.
> 
> 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         | 212 +++++++++++++++++++++++++++++++++++++++++++---
>  include/hw/nvram/fw_cfg.h |  12 ++-
>  3 files changed, 211 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 4846892..374660c 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -612,7 +612,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..7399008 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,10 +31,13 @@
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
>  
> -#define FW_CFG_SIZE 2
> +#define FW_CFG_SIZE 3

I see a problem with this. FW_CFG_SIZE controls two things:

- the size of the region that hosts the combined ioport range, for the
  ioport-mapped device. See fw_cfg_io_realize().

- the size of the region that hosts the control (ie. selector)
  register, for the memory-mapped device. See fw_cfg_mem_realize() --
  "fwcfg.ctl".

Setting FW_CFG_SIZE to 3 is incorrect for both cases, but for different
reasons.

- For the ioport-mapped device, the port range should be extended by 4,
  not 1.

  I do realize that the current code works, ie. it is not enforced by
  the memory API that a 4 byte wide outl() at 0x512 actually fit in the
  containing region -- only the start address and the size (treated in
  isolation) matters.

  Sometimes this is even "exploited" for the greater good. For example
  the 4-byte wide ioport at 0xCF8 (PCI config address) happily coexists
  with the 1-byte wide ioport at 0xCF9 (reset control register). The
  overlap doesn't confuse anything because the access is dispatched by
  start address, and the size of the access is "just" auxiliary
  information.

  However, I think this is ugly nonetheless. If we intend for the guest
  to issue outl() against 0x512, then we should extend the region up to
  and including 0x515.

  - Separate problem to be mentioned here: according to the
    documentation, the DMA address register should receive the address
    of the FWCfgDmaAccess structure. Now consider a 16 GB guest that
    allocates that structure above 4GB. There is simply no IO
    instruction ("outq") that could transfer that address in one go.

    However, fw_cfg_comb_write(), even the patched version, insists on
    that. (See it lower down in the patch.) If there's an access at
    0x512, fw_cfg_dma_transfer() is immediately fired off at the
    control structure that exists at the address. Too bad that the four
    most significant bytes of the address will always be zero, because
    the guest has no means to communicate otherwise.

    While this is probably not an issue for SeaBIOS, it certainly would
    be for OVMF. (Not that I intend to hook the feature into OVMF (ie.
    x86) any time soon, but still.) If this is a limitation by design,
    that's fine, but then it should be documented. (In OVMF low memory
    can be easily allocated, but whoever writes that code will have to
    know.)

- FW_CFG_SIZE := 3 is also a problem for the memory mapped device. The
  size of the traditional selector port -- see fw_cfg_mem_realize() --
  should not change.

The upshot is that in this patch series you should split apart the two
uses of FW_CFG_SIZE. In fact, at the moment it should be more correctly
called "FW_CFG_CTL_SIZE". At the moment FW_CFG_SIZE is used for
fw_cfg_io_realize() too because the control register's size dominates /
determines the entire ioport range.

So, you could consider dropping FW_CFG_SIZE altogether, and open-code
the separate sizes in the respective functions (fw_cfg_io_realize,
fw_cfg_mem_realize.)

Let's continue:

>  #define FW_CFG_NAME "fw_cfg"
>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>  
> +#define FW_CFG_VERSION      1
> +#define FW_CFG_VERSION_DMA  2
> +

As discussed, this should be a bitmap of features instead.

>  #define TYPE_FW_CFG     "fw_cfg"
>  #define TYPE_FW_CFG_IO  "fw_cfg_io"
>  #define TYPE_FW_CFG_MEM "fw_cfg_mem"
> @@ -42,6 +46,11 @@
>  #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_DMA_CONTROL bits */
> +#define FW_CFG_DMA_CTL_ERROR   0x01
> +#define FW_CFG_DMA_CTL_READ    0x02
> +#define FW_CFG_DMA_CTL_MASK    0x03
> +
>  typedef struct FWCfgEntry {
>      uint32_t len;
>      uint8_t *data;
> @@ -59,6 +68,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 +88,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 +307,108 @@ 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;
> +    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);
> +    dma = dma_memory_map(s->dma_as, s->dma_addr, &len,
> +                                DMA_DIRECTION_FROM_DEVICE);
> +
> +    if (!dma || !len) {
> +        return;
> +    }
> +
> +    if (dma->control & FW_CFG_DMA_CTL_ERROR) {
> +        return;
> +    }
> +
> +    if (!(dma->control & FW_CFG_DMA_CTL_READ)) {
> +        return;
> +    }
> +
> +    while (dma->length > 0) {
> +        if (s->cur_entry == FW_CFG_INVALID || !e->data ||
> +                                    s->cur_offset >= e->len) {

whitespace damage

I'm skipping the loop body below for now (I'm too tired and I'd like to
focus on the interface first):

> +            len = dma->length;
> +            if (dma->address) {
> +                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->address += len;
> +            dma->length  -= 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 (dma->address) {
> +                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;
> +        }
> +    }
> +
> +    trace_fw_cfg_read(s, 0);
> +
> +out:
> +    dma_memory_unmap(s->dma_as, dma, 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;
> +
> +    s->dma_addr = be64_to_cpu(value);
> +    fw_cfg_dma_transfer(s);
> +}

So, this is similar to the ioport size limitation I described at the
top. Namely,  I think that an Aarch32 guest won't be able to transfer a
64-bit value with a single MMIO access. (I believe double-width store
instructions do exist, but they cannot be virtualized well. They trap,
but the instruction syndrome register won't give enough info to the
hypervisor.)

Therefore, the address of the dma control structure should be passed in
two 32-bit wide accesses, both for the ioport mapping and the mmio
mapping. This can be done in two ways:
- write the two halves to the same register, and use a latch to
  identify each 2nd access
- use different addresses.

The latch sucks, because the guest has no way to bring the register to a
known good state. Therefore:

In the ioport mapped case, the port range should go up to 0x519, and two
outl's are going to be necessary in the guest. The documentation should
spell out which outl (@ 0x512 or @ 0x516) will trigger the actual transfer.

(I vaguely recall that someone already described this, but I can't find
the message!)

In the memory mapped case: same thing.

> +
> +static bool fw_cfg_dma_mem_valid(void *opaque, hwaddr addr,
> +                                  unsigned size, bool is_write)
> +{
> +    return is_write && addr == 0;
> +}
> +

so this should change...

>  static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> @@ -321,12 +436,20 @@ 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);
> +        /* FWCfgDmaAccess address */
> +        s = opaque;
> +        s->dma_addr = le64_to_cpu(value);
> +        fw_cfg_dma_transfer(s);
>          break;
>      }
>  }
> @@ -334,7 +457,11 @@ static void fw_cfg_comb_write(void *opaque, hwaddr addr,
>  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 (is_write && size == 2 && addr == 0) ||
> +            (size == 1 && addr == 1) ||
> +            (s->dma_enabled && is_write && addr == 2);
>  }

I get the idea.
- Write the selector (2 byte wide) at ioport offset 0,
- or read the data (1 byte wide) at ioport offset 1,
- or, if DMA is enabled, write the DMA control struct's address at
  ioport offset 2, "as wide as you can". Problem is, that's not wide
  enough.

Additionally, this incurs a guest visible change, for a corner case.
Before the patch, it used to be valid for the guest to outb() at ioport
offset 0. It was dispatched to fw_cfg_write(), which meant "ignore
silently".

We could have dropped the fw_cfg_write() function earlier (when we
turned it into the currently seen no-op). We didn't, because then such a
write access would have injected a guest-visible fault (at least on some
target architectures).

And that's what the above achieves too (probably involuntarily): an
outb() at ioport offset 0 is now a guest visible trap (because
fw_cfg_comb_valid() rejects it).

Even though this is quite obscure, I think we should preserve the
original behavior.

>  
>  static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
> @@ -361,6 +488,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,
> +};
> +

The docs should say that the address of the DMA control struct is taken
as big endian. (And, in two halves, see above.)

>  static void fw_cfg_reset(DeviceState *d)
>  {
>      FWCfgState *s = FW_CFG(d);
> @@ -401,6 +534,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 +559,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 +748,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 +759,42 @@ 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;
>  
>      dev = qdev_create(NULL, TYPE_FW_CFG_IO);
>      qdev_prop_set_uint32(dev, "iobase", iobase);
> +
> +    s = FW_CFG(dev);
>      fw_cfg_init1(dev);
>  
> -    return FW_CFG(dev);
> +    if (dma_as) {
> +        /* 64 bits for the address field */
> +        s->dma_as = dma_as;
> +        s->dma_enabled = true;
> +
> +        fw_cfg_add_i32(s, FW_CFG_ID, FW_CFG_VERSION_DMA);
> +    } else {
> +        fw_cfg_add_i32(s, FW_CFG_ID, FW_CFG_VERSION);
> +    }

(should be a feature bitmap)

> +
> +    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;
>  
>      dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
>      qdev_prop_set_uint32(dev, "data_width", data_width);
> @@ -633,13 +805,25 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
>      sysbus_mmio_map(sbd, 0, ctl_addr);
>      sysbus_mmio_map(sbd, 1, data_addr);
>  
> -    return FW_CFG(dev);
> +    s = FW_CFG(dev);
> +
> +    if (dma_addr && dma_as) {
> +        s->dma_as = dma_as;
> +        s->dma_enabled = true;
> +        sysbus_mmio_map(sbd, 2, dma_addr);
> +        fw_cfg_add_i32(s, FW_CFG_ID, FW_CFG_VERSION_DMA);
> +    } else {
> +        fw_cfg_add_i32(s, FW_CFG_ID, FW_CFG_VERSION);
> +    }

(should be a feature bitmap)


> +
> +    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);
>  }
>  
>  
> @@ -725,6 +909,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));

I'd prefer (2 * sizeof(uint32_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..4ce51e2 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -61,6 +61,12 @@ typedef struct FWCfgFiles {
>      FWCfgFile f[];
>  } FWCfgFiles;
>  
> +typedef struct FWCfgDmaAccess {
> +    uint64_t address;
> +    uint32_t length;
> +    uint32_t control;
> +} FWCfgDmaAccess;
> +

Should be QEMU_PACKED, if only for documentation purposes.

Thanks
Laszlo

>  typedef void (*FWCfgCallback)(void *opaque, uint8_t *data);
>  typedef void (*FWCfgReadCallback)(void *opaque, uint32_t offset);
>  
> @@ -77,10 +83,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);
>  
> 

Re: [PATCH] QEMU fw_cfg DMA interface documentation

[Laszlo Ersek]

On 08/06/15 13:03, Marc Marí 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 | 36 ++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> index 953fb64..c880eec 100644
> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> @@ -49,6 +49,41 @@ 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.
>  
> +Starting from revision 2, a DMA interface has also been added.

- Should be updated to say "bitmap" etc.

- I think these additions should start one paragraph earlier in the
  text file (ie. before the paragraph starting with "The guest kernel
  is not expected...")

> This can be used
> +through a write-only, 64-bit wide address register.

Should be updated to two, 32-bit wide registers, defining which half is
most significant and least significant, then the byte order within each
half, and also which is the one that fires off the DMA.

(I think Drew mentioned the same, but I can't find the message.)


> +
> +In this register, a pointer to a FWCfgDmaAccess structure can be written, in
> +big endian mode. This is the format of the FWCfgDmaAccess structure:

Ah okay, big endian is mentioned here.

> +
> +typedef struct FWCfgDmaAccess {
> +    uint64_t address;
> +    uint32_t length;
> +    uint32_t control;
> +} FWCfgDmaAccess;
> +
> +Once the address to this structure has been written, an DMA operation is
> +started. If the "control" field has value 2,

in what byte order?

> a read operation will be performed.
> +"length" bytes

in what byte order?

> for the current selector and offset will be mapped into the
> +address specified by the "address" field.

In what byte order? :)

(To wit, I think that the fw_cfg_dma_transfer() function in "[PATCH 3/5]
Implement fw_cfg DMA interface" lacks translation from guest endianness
to host endianness, for the fields of FWCfgDmaAccess.)

> +
> +If the field "address" has value 0, the read is considered a skip, and
> +the data is not copied anywhere, but the offset is still incremented.
> +
> +To check result, read the control register:
> +   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 register is zero
> +and the address register 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.
> +
>  Required properties:
>  
>  - compatible: "qemu,fw-cfg-mmio".
> @@ -56,6 +91,7 @@ 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.
> +  * From revision 2: Bytes 0xa to 0x11 cover the DMA address register.

I agree with Drew about padding being necessary here. I have proposed to
break up the address register into two 32-bit halves, but even for that,
0xa is not good enough. 0x0c..0x13, inclusive, seems better.

(Actually I wonder if we should instead add a separate region ("reg")
for this register, instead of padding. I don't know enough about device
trees to make a suggestion here. Let's go with this approach first, ie.
keep the single reg for now, just add the padding I guess.)

>    * Further registers may be appended to the region in case of future interface
>      revisions / feature bits.
>  
> 

The example DTB should also be refreshed. You can do it like this:
- apply your (updated) patch "[PATCH 4/5] Enable fw_cfg DMA interface
  for ARM" to QEMU

- run

  qemu-system-aarch64 -machine virt,dumpdtb=dumped.dtb ...
  dtc -I dtb -O dts dumped.dtb >dumped.dts

- locate "fw-cfg" in "dumped.dts"

The example should be similar to:

        fw-cfg@9020000 {
                compatible = "qemu,fw-cfg-mmio";
                reg = <0x0 0x9020000 0x0 0x14>;
        };


Thanks
Laszlo

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

[Marc Marí]

On Thu, 6 Aug 2015 22:49:12 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

[...]

> > +static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
> > +                                 uint64_t value, unsigned size)
> > +{
> > +    FWCfgState *s = opaque;
> > +
> > +    s->dma_addr = be64_to_cpu(value);
> > +    fw_cfg_dma_transfer(s);
> > +}
> 
> So, this is similar to the ioport size limitation I described at the
> top. Namely,  I think that an Aarch32 guest won't be able to transfer
> a 64-bit value with a single MMIO access. (I believe double-width
> store instructions do exist, but they cannot be virtualized well.
> They trap, but the instruction syndrome register won't give enough
> info to the hypervisor.)
> 
> Therefore, the address of the dma control structure should be passed
> in two 32-bit wide accesses, both for the ioport mapping and the mmio
> mapping. This can be done in two ways:
> - write the two halves to the same register, and use a latch to
>   identify each 2nd access
> - use different addresses.
> 
> The latch sucks, because the guest has no way to bring the register
> to a known good state. Therefore:
> 
> In the ioport mapped case, the port range should go up to 0x519, and
> two outl's are going to be necessary in the guest. The documentation
> should spell out which outl (@ 0x512 or @ 0x516) will trigger the
> actual transfer.
> 
> (I vaguely recall that someone already described this, but I can't
> find the message!)

Previous answer to this patch, by Kevin O'Connor:

"So, I think this code needs to be able to handle a 32bit write to a
high bits address and then store those bits until the 32bit write to
the low bits address occurs.  (I'd also recommend that after every dma
transfer the stored high bits are reset to zero so that the common case
of a 32bit address can be performed with a single 32bit write to the
low bits field.)"

It's easier to do it this way.

Thanks for you comments
Marc

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

[Laszlo Ersek]

On 08/06/15 23:11, Marc Marí wrote:
> On Thu, 6 Aug 2015 22:49:12 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
> [...]
> 
>>> +static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
>>> +                                 uint64_t value, unsigned size)
>>> +{
>>> +    FWCfgState *s = opaque;
>>> +
>>> +    s->dma_addr = be64_to_cpu(value);
>>> +    fw_cfg_dma_transfer(s);
>>> +}
>>
>> So, this is similar to the ioport size limitation I described at the
>> top. Namely,  I think that an Aarch32 guest won't be able to transfer
>> a 64-bit value with a single MMIO access. (I believe double-width
>> store instructions do exist, but they cannot be virtualized well.
>> They trap, but the instruction syndrome register won't give enough
>> info to the hypervisor.)
>>
>> Therefore, the address of the dma control structure should be passed
>> in two 32-bit wide accesses, both for the ioport mapping and the mmio
>> mapping. This can be done in two ways:
>> - write the two halves to the same register, and use a latch to
>>   identify each 2nd access
>> - use different addresses.
>>
>> The latch sucks, because the guest has no way to bring the register
>> to a known good state. Therefore:
>>
>> In the ioport mapped case, the port range should go up to 0x519, and
>> two outl's are going to be necessary in the guest. The documentation
>> should spell out which outl (@ 0x512 or @ 0x516) will trigger the
>> actual transfer.
>>
>> (I vaguely recall that someone already described this, but I can't
>> find the message!)
> 
> Previous answer to this patch, by Kevin O'Connor:
> 
> "So, I think this code needs to be able to handle a 32bit write to a
> high bits address and then store those bits until the 32bit write to
> the low bits address occurs.  (I'd also recommend that after every dma
> transfer the stored high bits are reset to zero so that the common case
> of a 32bit address can be performed with a single 32bit write to the
> low bits field.)"
> 
> It's easier to do it this way.

Thank you for the pointer. :) I don't remember this answer, at least not
consciously.

But, "this way" is not different from my suggestion. It just has more
details filled in (and therefore it is admittedly a smarter, more
elaborate suggestion than mine).

- I asked for two separate addresses, and Kevin had asked for two
  separate addresses too. (The idea that one half is stored until the
  other half is written was implicit in my suggestion.)

- I requested that the docs spell out which address would trigger the
  dma. Kevin had specifically suggested that the low bits address
  trigger it.

- The clearing of the stored high bits is extra smartness in Kevin's
  suggestion (I'm jealous for not thinking of that :)), but that will
  require more code (one line, probably), which is not "easier". :)

We're in violent agreement.

(If you wanted to poke fun at me, you could say that I just repeated
what Kevin had said, only worse. Thing is, I really don't recall seeing
his message. Let me search my mailbox for a substring from your above
quote... Yep, I don't have that message. My email has been acting up
today. I only have parts of that message *within* one of your emails.
... Which I mostly missed too. Doing too many things at once, sorry.)

Thanks!
Laszlo

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Kevin O'Connor]

Trimming CC list.

On Thu, Aug 06, 2015 at 05:53:26PM +0200, Marc Marí wrote:
> On Thu, 6 Aug 2015 11:30:43 -0400
> "Kevin O'Connor" <kevin@koconnor.net> wrote:
> > On Thu, Aug 06, 2015 at 02:37:45PM +0200, Marc Marí wrote:
> > > On Thu, 6 Aug 2015 13:27:16 +0100
> > > Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > > 
> > > > On Thu, Aug 6, 2015 at 12:00 PM, Marc Marí <markmb@redhat.com>
> > > > wrote:
> > > > > When running a Linux guest on top of QEMU, using the -kernel
> > > > > options, this is the timing improvement for x86:
> > > > >
> > > > > QEMU commit 2be4f242b50a8 and SeaBIOS commit 908a58c1d5ff
> > > > > 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
> > > > 
> > > > Impressive results!
> > > > 
> > > > Is this a fully-featured QEMU build or did you disable things?
> > > > 
> > > > Is this the default SeaBIOS build or did you disable things?
> > > > 
> > > 
> > > This is the default QEMU configuration I get for my system. It's
> > > not a fully-featured QEMU, but it has a lot of things enabled.
> > > SeaBIOS has a default configuration (with debugging disabled).
> > 
> > Thanks!
> > 
> > What qemu command-line did you use during testing?  Also, do you have
> > a quick primer on how to use the kvm trace stuff to obtain timings?
> > 
> 
> The command line I used is:
> 
> x86_64-softmmu/qemu-system-x86_64 --enable-kvm \
>     -kernel /boot/vmlinuz-4.0.7-300.fc22.x86_64 \
>     -L pc-bios/optionrom/ \
>     -bios roms/seabios/out/bios.bin -nographic

Thanks.  One thing that immediately pops up is that even though
"-nographic" is used, it looks like QEMU still emulates a VGA device
and that device has an option rom that takes several milliseconds to
init the display hardware.  Adding "-device VGA,romfile=" to the qemu
command line will further reduce the boot time by avoiding this
hardware init delay.

-Kevin

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

[Marc Marí]

On Thu, 6 Aug 2015 23:32:50 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 08/06/15 23:11, Marc Marí wrote:
> > On Thu, 6 Aug 2015 22:49:12 +0200
> > Laszlo Ersek <lersek@redhat.com> wrote:
> > 
> > [...]
> > 
> >>> +static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
> >>> +                                 uint64_t value, unsigned size)
> >>> +{
> >>> +    FWCfgState *s = opaque;
> >>> +
> >>> +    s->dma_addr = be64_to_cpu(value);
> >>> +    fw_cfg_dma_transfer(s);
> >>> +}
> >>
> >> So, this is similar to the ioport size limitation I described at
> >> the top. Namely,  I think that an Aarch32 guest won't be able to
> >> transfer a 64-bit value with a single MMIO access. (I believe
> >> double-width store instructions do exist, but they cannot be
> >> virtualized well. They trap, but the instruction syndrome register
> >> won't give enough info to the hypervisor.)
> >>
> >> Therefore, the address of the dma control structure should be
> >> passed in two 32-bit wide accesses, both for the ioport mapping
> >> and the mmio mapping. This can be done in two ways:
> >> - write the two halves to the same register, and use a latch to
> >>   identify each 2nd access
> >> - use different addresses.
> >>
> >> The latch sucks, because the guest has no way to bring the register
> >> to a known good state. Therefore:
> >>
> >> In the ioport mapped case, the port range should go up to 0x519,
> >> and two outl's are going to be necessary in the guest. The
> >> documentation should spell out which outl (@ 0x512 or @ 0x516)
> >> will trigger the actual transfer.
> >>
> >> (I vaguely recall that someone already described this, but I can't
> >> find the message!)
> > 
> > Previous answer to this patch, by Kevin O'Connor:
> > 
> > "So, I think this code needs to be able to handle a 32bit write to a
> > high bits address and then store those bits until the 32bit write to
> > the low bits address occurs.  (I'd also recommend that after every
> > dma transfer the stored high bits are reset to zero so that the
> > common case of a 32bit address can be performed with a single 32bit
> > write to the low bits field.)"
> > 
> > It's easier to do it this way.
> 
> Thank you for the pointer. :) I don't remember this answer, at least
> not consciously.
> 
> But, "this way" is not different from my suggestion. It just has more
> details filled in (and therefore it is admittedly a smarter, more
> elaborate suggestion than mine).
> 
> - I asked for two separate addresses, and Kevin had asked for two
>   separate addresses too. (The idea that one half is stored until the
>   other half is written was implicit in my suggestion.)
> 
> - I requested that the docs spell out which address would trigger the
>   dma. Kevin had specifically suggested that the low bits address
>   trigger it.
> 
> - The clearing of the stored high bits is extra smartness in Kevin's
>   suggestion (I'm jealous for not thinking of that :)), but that will
>   require more code (one line, probably), which is not "easier". :)
> 
> We're in violent agreement.
> 
> (If you wanted to poke fun at me, you could say that I just repeated
> what Kevin had said, only worse. Thing is, I really don't recall
> seeing his message. Let me search my mailbox for a substring from
> your above quote... Yep, I don't have that message. My email has been
> acting up today. I only have parts of that message *within* one of
> your emails. ... Which I mostly missed too. Doing too many things at
> once, sorry.)
> 

No fun intended, just complement the answer, because you said "I
vaguely recall that someone already described this, but I can't
find the message!".

And although I did not elaborate my answer (my fault), I wanted to say:
Starting on your idea (which is similar to Kevin's), it is easier for
the guest to have the upper address as 0 and trigger on the lower
address, because this lets 32 bit guests to trigger with just one
access. Of course, this has to be documented, even if not explicitly
stated.

Marc

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

[Marc Marí]

On Thu, 6 Aug 2015 10:20:38 -0400
"Kevin O'Connor" <kevin@koconnor.net> wrote:

> On Thu, Aug 06, 2015 at 01:01:15PM +0200, Marc Marí 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 | 42
> > ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40
> > insertions(+), 2 deletions(-)
> > 
> > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> > index 5bc7b96..dc8051e 100644
> > --- a/docs/specs/fw_cfg.txt
> > +++ b/docs/specs/fw_cfg.txt
> > @@ -76,6 +76,7 @@ increasing address order, similar to memcpy().
> >  
> >  Selector Register IOport: 0x510
> >  Data Register IOport:     0x511
> > +DMA Address IOport:       0x512
> >  
> >  == Firmware Configuration Items ==
> >  
> > @@ -89,8 +90,9 @@ present, the four bytes read will contain the
> > characters "QEMU". === Revision (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.
> > +revision number. If it is set to 1, the interface is the
> > traditional +selector / data interface. If it is set to 2, the DMA
> > extension is +also present.
> >  
> >  === File Directory (Key 0x0019, FW_CFG_FILE_DIR) ===
> >  
> > @@ -132,6 +134,42 @@ 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 =
> > +
> > +For revision value 2, the DMA interface is present. This does not
> > replace +the existing fw_cfg interface, it is an add-on.
> > +
> > +When this interface is enabled the DMA Address register can be
> > used to +write the address of a FWCfgDmaAccess structure:
> > +
> > +typedef struct FWCfgDmaAccess {
> > +    uint64_t address;
> > +    uint32_t length;
> > +    uint32_t control;
> > +} FWCfgDmaAccess;
> > +
> > +If "control" has the bit 1 set (FW_CFG_DMA_CTL_READ), a read
> > operation is +performed on the selected entry. "length" bytes of
> > data in that fw_cfg +entry are copied to the address specified by
> > "address". +
> > +If the field "address" has value 0, the read is considered a skip,
> > and +the data is not copied anywhere, but the offset is still
> > incremented.
> 
> Thanks!
> 
> I have a few suggestions on the interface:
> 
> - I think it would be better to place the 'control' field first (ie,
>   control/length/address instead of address/length/control).  Placing
>   the 'control' field first makes potential future extensions easier -
>   that is, it would make it easier for future control bits to change
>   the layout of the struct.
> 
> - It would be good to add a 'select' field to the struct.  I think
>   this could be done by using a 'u16 control; u16 select' instead of
>   the current 'u32 control'.  It's common for guests to select a
>   fw_cfg entry and read its entire contents - with the 'select' field
>   in the struct this could be done with a single guest/host fault.  A
>   new control bit could be added (eg, FW_CFG_DMA_CTL_SELECT) to
>   determine whether or not the select field is used - iff the bit is
>   set then the given fw_cfg entry is selected and the read position is
>   reset to the start prior to the DMA data copy.

(Problems with email, I got this delivered today)

I don't think there's much problem in using the original fw cfg select
field. Adding a new select field will add complexity to the guest (when
use one select field or the other), and the host (how the two select
fields interact with each other). I think this part is good enough as
it is now.

Thanks
Marc

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

On 08/06/2015 03:32 PM, Laszlo Ersek wrote:

> (If you wanted to poke fun at me, you could say that I just repeated
> what Kevin had said, only worse. Thing is, I really don't recall seeing
> his message. Let me search my mailbox for a substring from your above
> quote... Yep, I don't have that message. My email has been acting up
> today. I only have parts of that message *within* one of your emails.
> ... Which I mostly missed too. Doing too many things at once, sorry.)

I think gnu.org had a hiccup yesterday; I received a number of emails
out of order and several hours later than when they appeared in the
archives, and inspecting the headers made it appear that gnu.org's
servers were the cause of the delayed mail.  It seems to be running
smoother today.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Kevin O'Connor]

On Thu, Aug 06, 2015 at 04:59:15PM +0200, Marc Marí wrote:
> On Thu, 6 Aug 2015 10:47:21 -0400
> "Kevin O'Connor" <kevin@koconnor.net> wrote:
> > Also, it's very unusual to see 32bit writes to an unaligned IO address
> > - I think two pad bytes should be added so that the offset for the dma
> > address is at position 4 (instead of 2).
> 
> This is a PIO port (out), not a MMIO access (write). Maybe I'm wrong,
> but I don't think it matters to have the port number aligned with the
> write size.

There was a thread on misaligned IO accesses recently:

  http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg05054.html

Perhaps Stefan knows what the implications of misaligned IO writes
are.

I suspect it's better to avoid them.

-Kevin

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

[Laszlo Ersek]

On 08/07/15 22:40, Kevin O'Connor wrote:
> On Thu, Aug 06, 2015 at 04:59:15PM +0200, Marc Marí wrote:
>> On Thu, 6 Aug 2015 10:47:21 -0400
>> "Kevin O'Connor" <kevin@koconnor.net> wrote:
>>> Also, it's very unusual to see 32bit writes to an unaligned IO address
>>> - I think two pad bytes should be added so that the offset for the dma
>>> address is at position 4 (instead of 2).
>>
>> This is a PIO port (out), not a MMIO access (write). Maybe I'm wrong,
>> but I don't think it matters to have the port number aligned with the
>> write size.
> 
> There was a thread on misaligned IO accesses recently:
> 
>   http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg05054.html
> 
> Perhaps Stefan knows what the implications of misaligned IO writes
> are.
> 
> I suspect it's better to avoid them.

Unrelated, but I just noticed: in (and since) commit 457ba42878,
ICH9_LPC_GEN_PMCON_1 is read with pci_config_read*w*, but rewritten with
pci_config_write*l*.

Is that intended? (My similar OVMF code writes a word, not a long.)

Thanks
Laszlo

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Gerd Hoffmann]

  Hi,

> > The command line I used is:
> > 
> > x86_64-softmmu/qemu-system-x86_64 --enable-kvm \
> >     -kernel /boot/vmlinuz-4.0.7-300.fc22.x86_64 \
> >     -L pc-bios/optionrom/ \
> >     -bios roms/seabios/out/bios.bin -nographic
> 
> Thanks.  One thing that immediately pops up is that even though
> "-nographic" is used, it looks like QEMU still emulates a VGA device
> and that device has an option rom that takes several milliseconds to
> init the display hardware.  Adding "-device VGA,romfile=" to the qemu
> command line will further reduce the boot time by avoiding this
> hardware init delay.

Same goes for the NIC btw.

cheers,
  Gerd