[Qemu-devel] QEMU fw_cfg DMA interface

[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

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

[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

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: [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: [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] [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: [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] [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 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 --]

[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

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: [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-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: [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-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] 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] 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

[Qemu-devel] QEMU fw_cfg DMA interface

[Marc Marí]

Implementation of the FW CFG DMA interface.

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

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

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

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

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

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

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

[Qemu-devel] 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 16a1b6e and SeaBIOS commit e4d2b8c
QEMU startup time: .080
BIOS startup time: .060
Kernel setup time: .586
Total time: .726

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

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

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

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

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

Changes from v2:
 - Make IOports fw_cfg DMA region a different IO region.
 - Reuse everything for MMIO and IOport DMA regions
 - Make transfer status only based on control field
 - Use DMA helpers instead of direct map/unmap
 - Change ARM fw_cfg DMA address space
 - Change Linux boot process to match linuxboot.S
 - Add select capabilities in the FWCfgDmaAccess struct
 - Update documentation to reflect all these changes

[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 b2312c6 and SeaBIOS commit 423542e
QEMU startup time: .080
BIOS startup time: .060
Kernel setup time: .586
Total time: .726

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

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

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

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

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

Changes from v2:
 - Make IOports fw_cfg DMA region a different IO region.
 - Reuse everything for MMIO and IOport DMA regions
 - Make transfer status only based on control field
 - Use DMA helpers instead of direct map/unmap
 - Change ARM fw_cfg DMA address space
 - Change Linux boot process to match linuxboot.S
 - Add select capabilities in the FWCfgDmaAccess struct
 - Update documentation to reflect all these changes

Changes from v3:
 - Set properly fw_cfg DMA fields in ARM
 - Set fw_cfg DMA boot process properly (by Laszlo Ersek)
 - Add signature to fw_cfg DMA address field (by Kevin O'Connor)
 - Minor nitpicks

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Eric Blake]

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

[meta-comment]

On 10/01/2015 06:14 AM, Marc Marí wrote:
> Implementation of the FW CFG DMA interface.

The subject line is missing "v4" and "0/7". Also, the cover letter is
missing a diffstat.  That makes it harder to see from the cover letter
what the rest of the series is about.  'git format-patch/send-email
--cover-letter' does what you want; you can even 'git config
format.coverletter=auto' to always include a decent cover letter on any
multi-patch series.

-- 
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] QEMU fw_cfg DMA interface

[Eric Blake]

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

On 10/01/2015 10:03 AM, Eric Blake wrote:
> [meta-comment]
> 
> On 10/01/2015 06:14 AM, Marc Marí wrote:
>> Implementation of the FW CFG DMA interface.
> 
> The subject line is missing "v4" and "0/7". Also, the cover letter is
> missing a diffstat.  That makes it harder to see from the cover letter
> what the rest of the series is about.  'git format-patch/send-email
> --cover-letter' does what you want; you can even 'git config
> format.coverletter=auto' to always include a decent cover letter on any
> multi-patch series.

Oh, I see - you sent a meta-cover letter (the one I replied to in this
subthread), and then a patch series including a cover letter (the real
0/7, then 1/7 and friends in-reply-to the 0/7) as a child of the
meta-cover.  It's still a bit awkward for tools that expect the 0/7 as
the start of the thread, and part of my confusion was caused by
out-of-order mail delivery due to the nongnu.org mail server still
recovering from its mail delays.

-- 
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] QEMU fw_cfg DMA interface

[Laszlo Ersek]

On 10/01/15 18:11, Eric Blake wrote:
> On 10/01/2015 10:03 AM, Eric Blake wrote:
>> [meta-comment]
>>
>> On 10/01/2015 06:14 AM, Marc Marí wrote:
>>> Implementation of the FW CFG DMA interface.
>>
>> The subject line is missing "v4" and "0/7". Also, the cover letter is
>> missing a diffstat.  That makes it harder to see from the cover letter
>> what the rest of the series is about.  'git format-patch/send-email
>> --cover-letter' does what you want; you can even 'git config
>> format.coverletter=auto' to always include a decent cover letter on any
>> multi-patch series.
> 
> Oh, I see - you sent a meta-cover letter (the one I replied to in this
> subthread), and then a patch series including a cover letter (the real
> 0/7, then 1/7 and friends in-reply-to the 0/7) as a child of the
> meta-cover.  It's still a bit awkward for tools that expect the 0/7 as
> the start of the thread,

Yep, the pattern I just described doesn't consider those tools. Is that
a bad problem? Maybe the pattern is not so clever then. :)

(I'm allowed to say bad things about it, because I "invented" it
"independently". :))

> and part of my confusion was caused by
> out-of-order mail delivery due to the nongnu.org mail server still
> recovering from its mail delays.
> 

Right, those delays are not helping.

Thanks
Laszlo

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Laszlo Ersek]

On 10/01/15 18:03, Eric Blake wrote:
> [meta-comment]
> 
> On 10/01/2015 06:14 AM, Marc Marí wrote:
>> Implementation of the FW CFG DMA interface.
> 
> The subject line is missing "v4" and "0/7". Also, the cover letter is
> missing a diffstat.  That makes it harder to see from the cover letter
> what the rest of the series is about.  'git format-patch/send-email
> --cover-letter' does what you want; you can even 'git config
> format.coverletter=auto' to always include a decent cover letter on any
> multi-patch series.
> 

This posting follows a little bit different pattern, one that I myself
follow when posting patches for two (or more) components that must work
in sync.

Usually, a top-level blurb is manually cross-posted to all relevant
mailing lists. Then, each separate patch series is posted only to the
relevant mailing list, with its own cover letter (as usual with git),
*in response* to the manually posted blurb.

This has the following benefits:

- in mailing list archives that organize messages into threads *across*
  mailing lists (like Gmane does, for example), the top-level manual
  blurb is a good "root" for referencing the entire posting.

- The same is true for personal mailboxes, if a recipient is explicitly
  CC'd on all of the messages.

Because the top level blurb is parent to several patch series, and those
child series can all have different version numbers (due to different
numbers of respinds), it is not always straightforward to assign a
version number to the top blurb.

Thanks
Laszlo

Re: [Qemu-devel] QEMU fw_cfg DMA interface

[Eric Blake]

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

On 10/01/2015 10:17 AM, Laszlo Ersek wrote:
> On 10/01/15 18:03, Eric Blake wrote:
>> [meta-comment]
>>
>> On 10/01/2015 06:14 AM, Marc Marí wrote:
>>> Implementation of the FW CFG DMA interface.
>>
>> The subject line is missing "v4" and "0/7". Also, the cover letter is
>> missing a diffstat.  That makes it harder to see from the cover letter
>> what the rest of the series is about.  'git format-patch/send-email
>> --cover-letter' does what you want; you can even 'git config
>> format.coverletter=auto' to always include a decent cover letter on any
>> multi-patch series.
>>
> 
> This posting follows a little bit different pattern, one that I myself
> follow when posting patches for two (or more) components that must work
> in sync.

Ok, makes sense. Maybe the only additional suggestions would be to make
it more obvious in the subject line (put the text 'cross-post'
somewhere?) or have the first paragraph of the meta-cover be more
explicit that there are going to be multiple sub-threads, one per
project, where all subthreads must be applied to their corresponding
project for the overall feature to be complete?

[And maybe I should wait a few minutes for the full thread to appear in
my inbox, rather than immediately replying to the first mail while the
series is still incomplete due to mail delays...]

-- 
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] QEMU fw_cfg DMA interface

[Laszlo Ersek]

On 10/01/15 18:21, Eric Blake wrote:
> On 10/01/2015 10:17 AM, Laszlo Ersek wrote:
>> On 10/01/15 18:03, Eric Blake wrote:
>>> [meta-comment]
>>>
>>> On 10/01/2015 06:14 AM, Marc Marí wrote:
>>>> Implementation of the FW CFG DMA interface.
>>>
>>> The subject line is missing "v4" and "0/7". Also, the cover letter is
>>> missing a diffstat.  That makes it harder to see from the cover letter
>>> what the rest of the series is about.  'git format-patch/send-email
>>> --cover-letter' does what you want; you can even 'git config
>>> format.coverletter=auto' to always include a decent cover letter on any
>>> multi-patch series.
>>>
>>
>> This posting follows a little bit different pattern, one that I myself
>> follow when posting patches for two (or more) components that must work
>> in sync.
> 
> Ok, makes sense. Maybe the only additional suggestions would be to make
> it more obvious in the subject line (put the text 'cross-post'
> somewhere?) or have the first paragraph of the meta-cover be more
> explicit that there are going to be multiple sub-threads, one per
> project, where all subthreads must be applied to their corresponding
> project for the overall feature to be complete?

That's a good idea. I think prefixing the main blurb's subject with
[cross-post], and a "standard" first paragraph based on your above
suggestion, would be helpful.

> [And maybe I should wait a few minutes for the full thread to appear in
> my inbox, rather than immediately replying to the first mail while the
> series is still incomplete due to mail delays...]

I'm not patient; it would be unfair from me to expect others to be... :)

Laszlo