[Qemu-devel] [RFC 0/7] fw_cfg dma interface

[Qemu-devel] [RFC 0/7] fw_cfg dma interface

[Marc Marí]

Work done by Gerd Hoffmann for ARM. Extended to be used in x86 platforms

These patches, with the support in SeaBIOS (patches in the SeaBIOS
mailing list) make the Linux kernel setup and boot approximately 10x
faster.

TODO:
 * Optimizations. For example, use memcpy instead of a loop if the
   transaction is big
 * Is 0xfef00000 a good address? Is there any better address? Is it
   possible to not make it hardcoded (and can be read by the BIOS)?
 * Maybe have both DMA and IO interfaces enabled, to improve
   compatibility?

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

Gerd Hoffmann (4):
  fw_cfg dma interface
  fw_cfg dma: adapt to vmstate changes
  enable fw_cfg dma for arm virt
  fw_cfg file sort

Marc Marí (2):
  Add offset register to fw_cfg DMA interface
  fw_cfg DMA for x86

 docs/specs/fw_cfg.txt     |  52 ++++++++++++
 hw/arm/virt.c             |  11 ++-
 hw/i386/pc.c              |  21 ++++-
 hw/i386/pc_piix.c         |  25 +++++-
 hw/i386/pc_q35.c          |  26 +++++-
 hw/nvram/fw_cfg.c         | 199 +++++++++++++++++++++++++++++++++++++++++++---
 include/hw/i386/pc.h      |   1 +
 include/hw/nvram/fw_cfg.h |   5 +-
 8 files changed, 318 insertions(+), 22 deletions(-)

-- 
2.4.3

[Qemu-devel] [RFC 1/7] 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>
---
 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

Re: [Qemu-devel] [RFC 1/7] fw_cfg: document fw_cfg_modify_iXX() update functions

[Laszlo Ersek]

On 07/21/15 18:03, Marc Marí wrote:
> From: "Gabriel L. Somlo" <somlo@cmu.edu>
> 
> Document the behavior of fw_cfg_modify_iXX() for leak-less updating
> of integer-type blobs.
> 
> Currently only fw_cfg_modify_i16() is coded, but 32- and 64-bit versions
> may be added later if necessary..
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  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,
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Marc Marí]

From: Gerd Hoffmann <kraxel@redhat.com>

First draft of a fw_cfg dma interface.  Designed as add-on to the
extisting fw_cfg interface, i.e. there is no select register.  There
are four 32bit registers:  Target address (low and high bits), transfer
length, control register.

See docs/specs/fw_cfg.txt update for details.

Possible improvements (can be done incremental):

 * Better error reporting.  Right now we throw errors on invalid
   addresses only.  We could also throw errors on invalid reads/writes
   instead of using fw_cfg_read (return zeros on error) behavior.
 * Use memcpy instead of calling fw_cfg_read in a loop.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 docs/specs/fw_cfg.txt     |  35 ++++++++++
 hw/arm/virt.c             |   2 +-
 hw/nvram/fw_cfg.c         | 159 ++++++++++++++++++++++++++++++++++++++++++++--
 include/hw/nvram/fw_cfg.h |   5 +-
 4 files changed, 194 insertions(+), 7 deletions(-)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 5bc7b96..64d9192 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -132,6 +132,41 @@ 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 =
+
+== Registers ==
+
+This does not replace the existing fw_cfg interface, it is an add-on.
+Specifically there is no select register in the dma interface, guests
+must use the select register of the classic fw_cfg interface instead.
+
+There are four 32bit registers: Target address (low and high bits),
+transfer length, control register.
+
+== Protocol ==
+
+Probing for dma support being available: Write target address, read it
+back, verify you got back the value you wrote.
+
+Kick a transfer: Write select, target address and transfer length
+registers (order doesn't matter).  Then flip read bit in the control
+register to '1'.  Also make sure the error bit is cleared.
+
+Check result:  Read 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 successfull 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
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..5bcd0e0 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"
@@ -42,6 +43,18 @@
 #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 registers */
+#define FW_CFG_DMA_ADDR_LO        0
+#define FW_CFG_DMA_ADDR_HI        4
+#define FW_CFG_DMA_LENGTH         8
+#define FW_CFG_DMA_CONTROL       12
+#define FW_CFG_DMA_SIZE          16
+
+/* 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 +72,12 @@ struct FWCfgState {
     uint16_t cur_entry;
     uint32_t cur_offset;
     Notifier machine_ready;
+
+    bool       dma_enabled;
+    AddressSpace *dma_as;
+    dma_addr_t dma_addr;
+    uint32_t   dma_len;
+    uint32_t   dma_ctl;
 };
 
 struct FWCfgIoState {
@@ -75,7 +94,10 @@ struct FWCfgMemState {
     FWCfgState parent_obj;
     /*< public >*/
 
-    MemoryRegion ctl_iomem, data_iomem;
+    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
+#if 0
+    hwaddr ctl_addr, data_addr, dma_addr;
+#endif
     uint32_t data_width;
     MemoryRegionOps wide_data_ops;
 };
@@ -294,6 +316,88 @@ 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;
+    uint32_t i;
+
+    if (s->dma_ctl & FW_CFG_DMA_CTL_ERROR) {
+        return;
+    }
+    if (!(s->dma_ctl & FW_CFG_DMA_CTL_READ)) {
+        return;
+    }
+
+    while (s->dma_len > 0) {
+        len = s->dma_len;
+        ptr = dma_memory_map(s->dma_as, s->dma_addr, &len,
+                             DMA_DIRECTION_FROM_DEVICE);
+        if (!ptr || !len) {
+            s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
+            return;
+        }
+
+        for (i = 0; i < len; i++) {
+            ptr[i] = fw_cfg_read(s);
+        }
+
+        s->dma_addr += i;
+        s->dma_len  -= i;
+        dma_memory_unmap(s->dma_as, ptr, len,
+                         DMA_DIRECTION_FROM_DEVICE, i);
+    }
+    s->dma_ctl = 0;
+}
+
+static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr,
+                                    unsigned size)
+{
+    FWCfgState *s = opaque;
+    uint64_t ret = 0;
+
+    switch (addr) {
+    case FW_CFG_DMA_ADDR_LO:
+        ret = s->dma_addr & 0xffffffff;
+        break;
+    case FW_CFG_DMA_ADDR_HI:
+        ret = (s->dma_addr >> 32) & 0xffffffff;
+        break;
+    case FW_CFG_DMA_LENGTH:
+        ret = s->dma_len;
+        break;
+    case FW_CFG_DMA_CONTROL:
+        ret = s->dma_ctl;
+        break;
+    }
+    return ret;
+}
+
+static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
+                                 uint64_t value, unsigned size)
+{
+    FWCfgState *s = opaque;
+
+    switch (addr) {
+    case FW_CFG_DMA_ADDR_LO:
+        s->dma_addr &= ~((dma_addr_t)0xffffffff);
+        s->dma_addr |= value;
+        break;
+    case FW_CFG_DMA_ADDR_HI:
+        s->dma_addr &= ~(((dma_addr_t)0xffffffff) << 32);
+        s->dma_addr |= ((dma_addr_t)value) << 32;
+        break;
+    case FW_CFG_DMA_LENGTH:
+        s->dma_len = value;
+        break;
+    case FW_CFG_DMA_CONTROL:
+        value &= FW_CFG_DMA_CTL_MASK;
+        s->dma_ctl = value;
+        fw_cfg_dma_transfer(s);
+        break;
+    }
+}
+
 static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
@@ -361,6 +465,16 @@ static const MemoryRegionOps fw_cfg_comb_mem_ops = {
     .valid.accepts = fw_cfg_comb_valid,
 };
 
+static const MemoryRegionOps fw_cfg_dma_mem_ops = {
+    .read = fw_cfg_dma_mem_read,
+    .write = fw_cfg_dma_mem_write,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
 static void fw_cfg_reset(DeviceState *d)
 {
     FWCfgState *s = FW_CFG(d);
@@ -401,6 +515,23 @@ static bool is_version_1(void *opaque, int version_id)
     return version_id == 1;
 }
 
+static VMStateDescription vmstate_fw_cfg_dma = {
+    .name = "fw_cfg/dma",
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(dma_addr, FWCfgState),
+        VMSTATE_UINT32(dma_len, FWCfgState),
+        VMSTATE_UINT32(dma_ctl, FWCfgState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static bool fw_cfg_dma_enabled(void *opaque)
+{
+    FWCfgState *s = opaque;
+
+    return s->dma_enabled;
+}
+
 static const VMStateDescription vmstate_fw_cfg = {
     .name = "fw_cfg",
     .version_id = 2,
@@ -410,6 +541,14 @@ 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 = (VMStateSubsection[]) {
+        {
+            .vmsd   = &vmstate_fw_cfg_dma,
+            .needed = fw_cfg_dma_enabled,
+        }, {
+            /* end of list */
+        }
     }
 };
 
@@ -618,8 +757,9 @@ FWCfgState *fw_cfg_init_io(uint32_t iobase)
     return FW_CFG(dev);
 }
 
-FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
-                                 uint32_t data_width)
+FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
+                                 hwaddr data_addr, uint32_t data_width,
+                                 hwaddr dma_addr, AddressSpace *dma_as)
 {
     DeviceState *dev;
     SysBusDevice *sbd;
@@ -633,13 +773,20 @@ 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);
 
+    if (dma_addr && dma_as) {
+        FW_CFG(dev)->dma_as = dma_as;
+        FW_CFG(dev)->dma_enabled = true;
+        sysbus_mmio_map(sbd, 1, dma_addr);
+    }
+
     return FW_CFG(dev);
 }
 
 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 +872,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", FW_CFG_DMA_SIZE);
+    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..2fe31ea 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -79,8 +79,9 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
                          size_t len);
 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] [RFC 2/7] fw_cfg dma interface

[Laszlo Ersek]

On 07/21/15 18:03, Marc Marí wrote:
> From: Gerd Hoffmann <kraxel@redhat.com>
> 
> First draft of a fw_cfg dma interface.  Designed as add-on to the
> extisting fw_cfg interface, i.e. there is no select register.  There
> are four 32bit registers:  Target address (low and high bits), transfer
> length, control register.
> 
> See docs/specs/fw_cfg.txt update for details.
> 
> Possible improvements (can be done incremental):
> 
>  * Better error reporting.  Right now we throw errors on invalid
>    addresses only.  We could also throw errors on invalid reads/writes
>    instead of using fw_cfg_read (return zeros on error) behavior.
>  * Use memcpy instead of calling fw_cfg_read in a loop.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  docs/specs/fw_cfg.txt     |  35 ++++++++++
>  hw/arm/virt.c             |   2 +-
>  hw/nvram/fw_cfg.c         | 159 ++++++++++++++++++++++++++++++++++++++++++++--
>  include/hw/nvram/fw_cfg.h |   5 +-
>  4 files changed, 194 insertions(+), 7 deletions(-)
> 
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 5bc7b96..64d9192 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -132,6 +132,41 @@ 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 =
> +
> +== Registers ==
> +
> +This does not replace the existing fw_cfg interface, it is an add-on.
> +Specifically there is no select register in the dma interface, guests
> +must use the select register of the classic fw_cfg interface instead.
> +
> +There are four 32bit registers: Target address (low and high bits),
> +transfer length, control register.
> +
> +== Protocol ==
> +
> +Probing for dma support being available: Write target address, read it
> +back, verify you got back the value you wrote.

In the kernel tree, in
"Documentation/devicetree/bindings/arm/fw-cfg.txt", we said

> The outermost protocol (involving the write / read sequences of the
> control and data registers) is expected to be versioned, and/or
> described by feature bits. The interface revision / feature bitmap can
> be retrieved with key 0x0001. The blob to be read from the data
> register has size 4, and it is to be interpreted as a uint32_t value
> in little endian byte order. The current value (corresponding to the
> above outer protocol) is zero.

So, I think the DMA interface should be advertised as a new bit in the
bitmap. (Bit #0 with value 1 should not be used, because that's already
used -- for nothing, actually -- on x86 targets. But bit #1 with value 2
should be okay for this.)

> +
> +Kick a transfer: Write select, target address and transfer length
> +registers (order doesn't matter).  Then flip read bit in the control
> +register to '1'.  Also make sure the error bit is cleared.
> +
> +Check result:  Read 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 successfull

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
> 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..5bcd0e0 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"
> @@ -42,6 +43,18 @@
>  #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 registers */
> +#define FW_CFG_DMA_ADDR_LO        0
> +#define FW_CFG_DMA_ADDR_HI        4
> +#define FW_CFG_DMA_LENGTH         8
> +#define FW_CFG_DMA_CONTROL       12
> +#define FW_CFG_DMA_SIZE          16
> +
> +/* 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 +72,12 @@ struct FWCfgState {
>      uint16_t cur_entry;
>      uint32_t cur_offset;
>      Notifier machine_ready;
> +
> +    bool       dma_enabled;
> +    AddressSpace *dma_as;
> +    dma_addr_t dma_addr;
> +    uint32_t   dma_len;
> +    uint32_t   dma_ctl;
>  };
>  
>  struct FWCfgIoState {
> @@ -75,7 +94,10 @@ struct FWCfgMemState {
>      FWCfgState parent_obj;
>      /*< public >*/
>  
> -    MemoryRegion ctl_iomem, data_iomem;
> +    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
> +#if 0
> +    hwaddr ctl_addr, data_addr, dma_addr;
> +#endif

What's the goal of this?

>      uint32_t data_width;
>      MemoryRegionOps wide_data_ops;
>  };
> @@ -294,6 +316,88 @@ 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;
> +    uint32_t i;
> +
> +    if (s->dma_ctl & FW_CFG_DMA_CTL_ERROR) {
> +        return;
> +    }
> +    if (!(s->dma_ctl & FW_CFG_DMA_CTL_READ)) {
> +        return;
> +    }
> +
> +    while (s->dma_len > 0) {
> +        len = s->dma_len;
> +        ptr = dma_memory_map(s->dma_as, s->dma_addr, &len,
> +                             DMA_DIRECTION_FROM_DEVICE);
> +        if (!ptr || !len) {
> +            s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
> +            return;
> +        }
> +
> +        for (i = 0; i < len; i++) {
> +            ptr[i] = fw_cfg_read(s);
> +        }
> +
> +        s->dma_addr += i;
> +        s->dma_len  -= i;
> +        dma_memory_unmap(s->dma_as, ptr, len,
> +                         DMA_DIRECTION_FROM_DEVICE, i);
> +    }
> +    s->dma_ctl = 0;
> +}

On Aarch64 KVM, is this going to show the same cache coherence problems
as VGA memory access?

... I definitely don't claim that I understand the rest of the patch,
but for now I don't have more questions. :)

Thanks!
Laszlo

> +
> +static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr,
> +                                    unsigned size)
> +{
> +    FWCfgState *s = opaque;
> +    uint64_t ret = 0;
> +
> +    switch (addr) {
> +    case FW_CFG_DMA_ADDR_LO:
> +        ret = s->dma_addr & 0xffffffff;
> +        break;
> +    case FW_CFG_DMA_ADDR_HI:
> +        ret = (s->dma_addr >> 32) & 0xffffffff;
> +        break;
> +    case FW_CFG_DMA_LENGTH:
> +        ret = s->dma_len;
> +        break;
> +    case FW_CFG_DMA_CONTROL:
> +        ret = s->dma_ctl;
> +        break;
> +    }
> +    return ret;
> +}
> +
> +static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
> +                                 uint64_t value, unsigned size)
> +{
> +    FWCfgState *s = opaque;
> +
> +    switch (addr) {
> +    case FW_CFG_DMA_ADDR_LO:
> +        s->dma_addr &= ~((dma_addr_t)0xffffffff);
> +        s->dma_addr |= value;
> +        break;
> +    case FW_CFG_DMA_ADDR_HI:
> +        s->dma_addr &= ~(((dma_addr_t)0xffffffff) << 32);
> +        s->dma_addr |= ((dma_addr_t)value) << 32;
> +        break;
> +    case FW_CFG_DMA_LENGTH:
> +        s->dma_len = value;
> +        break;
> +    case FW_CFG_DMA_CONTROL:
> +        value &= FW_CFG_DMA_CTL_MASK;
> +        s->dma_ctl = value;
> +        fw_cfg_dma_transfer(s);
> +        break;
> +    }
> +}
> +
>  static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> @@ -361,6 +465,16 @@ static const MemoryRegionOps fw_cfg_comb_mem_ops = {
>      .valid.accepts = fw_cfg_comb_valid,
>  };
>  
> +static const MemoryRegionOps fw_cfg_dma_mem_ops = {
> +    .read = fw_cfg_dma_mem_read,
> +    .write = fw_cfg_dma_mem_write,
> +    .endianness = DEVICE_BIG_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
>  static void fw_cfg_reset(DeviceState *d)
>  {
>      FWCfgState *s = FW_CFG(d);
> @@ -401,6 +515,23 @@ static bool is_version_1(void *opaque, int version_id)
>      return version_id == 1;
>  }
>  
> +static VMStateDescription vmstate_fw_cfg_dma = {
> +    .name = "fw_cfg/dma",
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(dma_addr, FWCfgState),
> +        VMSTATE_UINT32(dma_len, FWCfgState),
> +        VMSTATE_UINT32(dma_ctl, FWCfgState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static bool fw_cfg_dma_enabled(void *opaque)
> +{
> +    FWCfgState *s = opaque;
> +
> +    return s->dma_enabled;
> +}
> +
>  static const VMStateDescription vmstate_fw_cfg = {
>      .name = "fw_cfg",
>      .version_id = 2,
> @@ -410,6 +541,14 @@ 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 = (VMStateSubsection[]) {
> +        {
> +            .vmsd   = &vmstate_fw_cfg_dma,
> +            .needed = fw_cfg_dma_enabled,
> +        }, {
> +            /* end of list */
> +        }
>      }
>  };
>  
> @@ -618,8 +757,9 @@ FWCfgState *fw_cfg_init_io(uint32_t iobase)
>      return FW_CFG(dev);
>  }
>  
> -FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
> -                                 uint32_t data_width)
> +FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
> +                                 hwaddr data_addr, uint32_t data_width,
> +                                 hwaddr dma_addr, AddressSpace *dma_as)
>  {
>      DeviceState *dev;
>      SysBusDevice *sbd;
> @@ -633,13 +773,20 @@ 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);
>  
> +    if (dma_addr && dma_as) {
> +        FW_CFG(dev)->dma_as = dma_as;
> +        FW_CFG(dev)->dma_enabled = true;
> +        sysbus_mmio_map(sbd, 1, dma_addr);
> +    }
> +
>      return FW_CFG(dev);
>  }
>  
>  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 +872,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", FW_CFG_DMA_SIZE);
> +    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..2fe31ea 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -79,8 +79,9 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
>                           size_t len);
>  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] [RFC 2/7] fw_cfg dma interface

[Marc Marí]

On Tue, 21 Jul 2015 21:44:49 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 07/21/15 18:03, Marc Marí wrote:
> > From: Gerd Hoffmann <kraxel@redhat.com>
> > 
> > First draft of a fw_cfg dma interface.  Designed as add-on to the
> > extisting fw_cfg interface, i.e. there is no select register.  There
> > are four 32bit registers:  Target address (low and high bits),
> > transfer length, control register.
> > 
> > See docs/specs/fw_cfg.txt update for details.
> > 
> > Possible improvements (can be done incremental):
> > 
> >  * Better error reporting.  Right now we throw errors on invalid
> >    addresses only.  We could also throw errors on invalid
> > reads/writes instead of using fw_cfg_read (return zeros on error)
> > behavior.
> >  * Use memcpy instead of calling fw_cfg_read in a loop.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  docs/specs/fw_cfg.txt     |  35 ++++++++++
> >  hw/arm/virt.c             |   2 +-
> >  hw/nvram/fw_cfg.c         | 159
> > ++++++++++++++++++++++++++++++++++++++++++++--
> > include/hw/nvram/fw_cfg.h |   5 +- 4 files changed, 194
> > insertions(+), 7 deletions(-)
> > 
> > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> > index 5bc7b96..64d9192 100644
> > --- a/docs/specs/fw_cfg.txt
> > +++ b/docs/specs/fw_cfg.txt
> > @@ -132,6 +132,41 @@ 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 =
> > +
> > +== Registers ==
> > +
> > +This does not replace the existing fw_cfg interface, it is an
> > add-on. +Specifically there is no select register in the dma
> > interface, guests +must use the select register of the classic
> > fw_cfg interface instead. +
> > +There are four 32bit registers: Target address (low and high bits),
> > +transfer length, control register.
> > +
> > +== Protocol ==
> > +
> > +Probing for dma support being available: Write target address,
> > read it +back, verify you got back the value you wrote.
> 
> In the kernel tree, in
> "Documentation/devicetree/bindings/arm/fw-cfg.txt", we said
> 
> > The outermost protocol (involving the write / read sequences of the
> > control and data registers) is expected to be versioned, and/or
> > described by feature bits. The interface revision / feature bitmap
> > can be retrieved with key 0x0001. The blob to be read from the data
> > register has size 4, and it is to be interpreted as a uint32_t value
> > in little endian byte order. The current value (corresponding to the
> > above outer protocol) is zero.
> 
> So, I think the DMA interface should be advertised as a new bit in the
> bitmap. (Bit #0 with value 1 should not be used, because that's
> already used -- for nothing, actually -- on x86 targets. But bit #1
> with value 2 should be okay for this.)
> 

Ok. When the changes get accepted, I'll also update that document in
the kernel tree.

> > +
> > +Kick a transfer: Write select, target address and transfer length
> > +registers (order doesn't matter).  Then flip read bit in the
> > control +register to '1'.  Also make sure the error bit is cleared.
> > +
> > +Check result:  Read 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 successfull
> 
> 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 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..5bcd0e0 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"
> > @@ -42,6 +43,18 @@
> >  #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 registers */
> > +#define FW_CFG_DMA_ADDR_LO        0
> > +#define FW_CFG_DMA_ADDR_HI        4
> > +#define FW_CFG_DMA_LENGTH         8
> > +#define FW_CFG_DMA_CONTROL       12
> > +#define FW_CFG_DMA_SIZE          16
> > +
> > +/* 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 +72,12 @@ struct FWCfgState {
> >      uint16_t cur_entry;
> >      uint32_t cur_offset;
> >      Notifier machine_ready;
> > +
> > +    bool       dma_enabled;
> > +    AddressSpace *dma_as;
> > +    dma_addr_t dma_addr;
> > +    uint32_t   dma_len;
> > +    uint32_t   dma_ctl;
> >  };
> >  
> >  struct FWCfgIoState {
> > @@ -75,7 +94,10 @@ struct FWCfgMemState {
> >      FWCfgState parent_obj;
> >      /*< public >*/
> >  
> > -    MemoryRegion ctl_iomem, data_iomem;
> > +    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
> > +#if 0
> > +    hwaddr ctl_addr, data_addr, dma_addr;
> > +#endif
> 
> What's the goal of this?

I suppose the comment has no goal. It was just left undeleted, and I
haven't seen it.

> 
> >      uint32_t data_width;
> >      MemoryRegionOps wide_data_ops;
> >  };
> > @@ -294,6 +316,88 @@ 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;
> > +    uint32_t i;
> > +
> > +    if (s->dma_ctl & FW_CFG_DMA_CTL_ERROR) {
> > +        return;
> > +    }
> > +    if (!(s->dma_ctl & FW_CFG_DMA_CTL_READ)) {
> > +        return;
> > +    }
> > +
> > +    while (s->dma_len > 0) {
> > +        len = s->dma_len;
> > +        ptr = dma_memory_map(s->dma_as, s->dma_addr, &len,
> > +                             DMA_DIRECTION_FROM_DEVICE);
> > +        if (!ptr || !len) {
> > +            s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
> > +            return;
> > +        }
> > +
> > +        for (i = 0; i < len; i++) {
> > +            ptr[i] = fw_cfg_read(s);
> > +        }
> > +
> > +        s->dma_addr += i;
> > +        s->dma_len  -= i;
> > +        dma_memory_unmap(s->dma_as, ptr, len,
> > +                         DMA_DIRECTION_FROM_DEVICE, i);
> > +    }
> > +    s->dma_ctl = 0;
> > +}
> 
> On Aarch64 KVM, is this going to show the same cache coherence
> problems as VGA memory access?

Somebody with more knowledge in this architecture should answer this
question.

Thanks
Marc

Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Laszlo Ersek]

On 07/22/15 10:19, Marc Marí wrote:
> On Tue, 21 Jul 2015 21:44:49 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 07/21/15 18:03, Marc Marí wrote:
>>> From: Gerd Hoffmann <kraxel@redhat.com>
>>>
>>> First draft of a fw_cfg dma interface.  Designed as add-on to the
>>> extisting fw_cfg interface, i.e. there is no select register.  There
>>> are four 32bit registers:  Target address (low and high bits),
>>> transfer length, control register.
>>>
>>> See docs/specs/fw_cfg.txt update for details.
>>>
>>> Possible improvements (can be done incremental):
>>>
>>>  * Better error reporting.  Right now we throw errors on invalid
>>>    addresses only.  We could also throw errors on invalid
>>> reads/writes instead of using fw_cfg_read (return zeros on error)
>>> behavior.
>>>  * Use memcpy instead of calling fw_cfg_read in a loop.
>>>
>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>> ---
>>>  docs/specs/fw_cfg.txt     |  35 ++++++++++
>>>  hw/arm/virt.c             |   2 +-
>>>  hw/nvram/fw_cfg.c         | 159
>>> ++++++++++++++++++++++++++++++++++++++++++++--
>>> include/hw/nvram/fw_cfg.h |   5 +- 4 files changed, 194
>>> insertions(+), 7 deletions(-)
>>>
>>> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
>>> index 5bc7b96..64d9192 100644
>>> --- a/docs/specs/fw_cfg.txt
>>> +++ b/docs/specs/fw_cfg.txt
>>> @@ -132,6 +132,41 @@ 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 =
>>> +
>>> +== Registers ==
>>> +
>>> +This does not replace the existing fw_cfg interface, it is an
>>> add-on. +Specifically there is no select register in the dma
>>> interface, guests +must use the select register of the classic
>>> fw_cfg interface instead. +
>>> +There are four 32bit registers: Target address (low and high bits),
>>> +transfer length, control register.
>>> +
>>> +== Protocol ==
>>> +
>>> +Probing for dma support being available: Write target address,
>>> read it +back, verify you got back the value you wrote.
>>
>> In the kernel tree, in
>> "Documentation/devicetree/bindings/arm/fw-cfg.txt", we said
>>
>>> The outermost protocol (involving the write / read sequences of the
>>> control and data registers) is expected to be versioned, and/or
>>> described by feature bits. The interface revision / feature bitmap
>>> can be retrieved with key 0x0001. The blob to be read from the data
>>> register has size 4, and it is to be interpreted as a uint32_t value
>>> in little endian byte order. The current value (corresponding to the
>>> above outer protocol) is zero.
>>
>> So, I think the DMA interface should be advertised as a new bit in the
>> bitmap. (Bit #0 with value 1 should not be used, because that's
>> already used -- for nothing, actually -- on x86 targets. But bit #1
>> with value 2 should be okay for this.)
>>
> 
> Ok. When the changes get accepted, I'll also update that document in
> the kernel tree.

The kernel documentation is more or less a specification for this
device. The documentation there dictates the implementation here.
Getting the kernel devs with jurisdiction to accept changes for the
binding docs is actually harder than getting the QEMU patches in. I
suggest to start there (or to do it in parallel).

Obviously, I'm not trying to "enforce" such a workflow, I'm just
explaining the method we used last time, when I worked on this device
(and wrote "Documentation/devicetree/bindings/arm/fw-cfg.txt", as Peter
asked).

Thanks
Laszlo

> 
>>> +
>>> +Kick a transfer: Write select, target address and transfer length
>>> +registers (order doesn't matter).  Then flip read bit in the
>>> control +register to '1'.  Also make sure the error bit is cleared.
>>> +
>>> +Check result:  Read 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 successfull
>>
>> 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 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..5bcd0e0 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"
>>> @@ -42,6 +43,18 @@
>>>  #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 registers */
>>> +#define FW_CFG_DMA_ADDR_LO        0
>>> +#define FW_CFG_DMA_ADDR_HI        4
>>> +#define FW_CFG_DMA_LENGTH         8
>>> +#define FW_CFG_DMA_CONTROL       12
>>> +#define FW_CFG_DMA_SIZE          16
>>> +
>>> +/* 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 +72,12 @@ struct FWCfgState {
>>>      uint16_t cur_entry;
>>>      uint32_t cur_offset;
>>>      Notifier machine_ready;
>>> +
>>> +    bool       dma_enabled;
>>> +    AddressSpace *dma_as;
>>> +    dma_addr_t dma_addr;
>>> +    uint32_t   dma_len;
>>> +    uint32_t   dma_ctl;
>>>  };
>>>  
>>>  struct FWCfgIoState {
>>> @@ -75,7 +94,10 @@ struct FWCfgMemState {
>>>      FWCfgState parent_obj;
>>>      /*< public >*/
>>>  
>>> -    MemoryRegion ctl_iomem, data_iomem;
>>> +    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
>>> +#if 0
>>> +    hwaddr ctl_addr, data_addr, dma_addr;
>>> +#endif
>>
>> What's the goal of this?
> 
> I suppose the comment has no goal. It was just left undeleted, and I
> haven't seen it.
> 
>>
>>>      uint32_t data_width;
>>>      MemoryRegionOps wide_data_ops;
>>>  };
>>> @@ -294,6 +316,88 @@ 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;
>>> +    uint32_t i;
>>> +
>>> +    if (s->dma_ctl & FW_CFG_DMA_CTL_ERROR) {
>>> +        return;
>>> +    }
>>> +    if (!(s->dma_ctl & FW_CFG_DMA_CTL_READ)) {
>>> +        return;
>>> +    }
>>> +
>>> +    while (s->dma_len > 0) {
>>> +        len = s->dma_len;
>>> +        ptr = dma_memory_map(s->dma_as, s->dma_addr, &len,
>>> +                             DMA_DIRECTION_FROM_DEVICE);
>>> +        if (!ptr || !len) {
>>> +            s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
>>> +            return;
>>> +        }
>>> +
>>> +        for (i = 0; i < len; i++) {
>>> +            ptr[i] = fw_cfg_read(s);
>>> +        }
>>> +
>>> +        s->dma_addr += i;
>>> +        s->dma_len  -= i;
>>> +        dma_memory_unmap(s->dma_as, ptr, len,
>>> +                         DMA_DIRECTION_FROM_DEVICE, i);
>>> +    }
>>> +    s->dma_ctl = 0;
>>> +}
>>
>> On Aarch64 KVM, is this going to show the same cache coherence
>> problems as VGA memory access?
> 
> Somebody with more knowledge in this architecture should answer this
> question.
> 
> Thanks
> Marc
> 

Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Andrew Jones]

On Tue, Jul 21, 2015 at 09:44:49PM +0200, Laszlo Ersek wrote:
> On 07/21/15 18:03, Marc Marí wrote:
> > +static void fw_cfg_dma_transfer(FWCfgState *s)
> > +{
> > +    dma_addr_t len;
> > +    uint8_t *ptr;
> > +    uint32_t i;
> > +
> > +    if (s->dma_ctl & FW_CFG_DMA_CTL_ERROR) {
> > +        return;
> > +    }
> > +    if (!(s->dma_ctl & FW_CFG_DMA_CTL_READ)) {
> > +        return;
> > +    }
> > +
> > +    while (s->dma_len > 0) {
> > +        len = s->dma_len;
> > +        ptr = dma_memory_map(s->dma_as, s->dma_addr, &len,
> > +                             DMA_DIRECTION_FROM_DEVICE);
> > +        if (!ptr || !len) {
> > +            s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
> > +            return;
> > +        }
> > +
> > +        for (i = 0; i < len; i++) {
> > +            ptr[i] = fw_cfg_read(s);
> > +        }
> > +
> > +        s->dma_addr += i;
> > +        s->dma_len  -= i;
> > +        dma_memory_unmap(s->dma_as, ptr, len,
> > +                         DMA_DIRECTION_FROM_DEVICE, i);
> > +    }
> > +    s->dma_ctl = 0;
> > +}
> 
> On Aarch64 KVM, is this going to show the same cache coherence problems
> as VGA memory access?
>

Does the guest (AAVMF) need to map all regions it DMAs to/from as
noncacheable? If so, then yes. If not, then I don't think it should
map fw-cfg DMA regions that way. fw-cfg is a paravirt device, so I
don't think we should have to treat it like a real one.

drew 

Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Laszlo Ersek]

On 07/22/15 13:30, Andrew Jones wrote:
> On Tue, Jul 21, 2015 at 09:44:49PM +0200, Laszlo Ersek wrote:
>> On 07/21/15 18:03, Marc Marí wrote:
>>> +static void fw_cfg_dma_transfer(FWCfgState *s)
>>> +{
>>> +    dma_addr_t len;
>>> +    uint8_t *ptr;
>>> +    uint32_t i;
>>> +
>>> +    if (s->dma_ctl & FW_CFG_DMA_CTL_ERROR) {
>>> +        return;
>>> +    }
>>> +    if (!(s->dma_ctl & FW_CFG_DMA_CTL_READ)) {
>>> +        return;
>>> +    }
>>> +
>>> +    while (s->dma_len > 0) {
>>> +        len = s->dma_len;
>>> +        ptr = dma_memory_map(s->dma_as, s->dma_addr, &len,
>>> +                             DMA_DIRECTION_FROM_DEVICE);
>>> +        if (!ptr || !len) {
>>> +            s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
>>> +            return;
>>> +        }
>>> +
>>> +        for (i = 0; i < len; i++) {
>>> +            ptr[i] = fw_cfg_read(s);
>>> +        }
>>> +
>>> +        s->dma_addr += i;
>>> +        s->dma_len  -= i;
>>> +        dma_memory_unmap(s->dma_as, ptr, len,
>>> +                         DMA_DIRECTION_FROM_DEVICE, i);
>>> +    }
>>> +    s->dma_ctl = 0;
>>> +}
>>
>> On Aarch64 KVM, is this going to show the same cache coherence problems
>> as VGA memory access?
>>
> 
> Does the guest (AAVMF) need to map all regions it DMAs to/from as
> noncacheable? If so, then yes. If not, then I don't think it should
> map fw-cfg DMA regions that way. fw-cfg is a paravirt device, so I
> don't think we should have to treat it like a real one.

I'd just point the address registers (or the address fields in the
descriptor structure) to a block of normal guest RAM, before flipping
the appropriate bit in the MMIO register that actually fires off the
transfer.

... Seems like that should work, then. Thanks!

Laszlo

Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Kevin O'Connor]

On Tue, Jul 21, 2015 at 06:03:41PM +0200, Marc Marí wrote:
> From: Gerd Hoffmann <kraxel@redhat.com>
> 
> First draft of a fw_cfg dma interface.  Designed as add-on to the
> extisting fw_cfg interface, i.e. there is no select register.  There
> are four 32bit registers:  Target address (low and high bits), transfer
> length, control register.

If I read this interface correctly, a guest will have at least six
faults to complete a typical fw_cfg dma transfer (select, target low,
target high, transfer length, control register write, control register
read).  I wonder if using a DMA transfer descriptor might be more
efficient.

That is, if a transfer descriptor was defined with something like:

struct fwcfg_dma {
    u16 command;
    u16 select;
    u32 transfer_length;
    u64 target_addr;
} PACKED;

and QEMU was informed of the transfer descriptor address (one fault on
32bit addresses; two faults on 64bit addresses) then QEMU could read
the transfer descriptor, perform the requested operation, and then
update the transfer descriptor on completion.  This would reduce the
total number of faults between QEMU and the firmware, and allow for a
more flexible interface for future growth.

-Kevin

Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Marc Marí]

On Wed, 22 Jul 2015 00:24:34 -0400
"Kevin O'Connor" <kevin@koconnor.net> wrote:

> On Tue, Jul 21, 2015 at 06:03:41PM +0200, Marc Marí wrote:
> > From: Gerd Hoffmann <kraxel@redhat.com>
> > 
> > First draft of a fw_cfg dma interface.  Designed as add-on to the
> > extisting fw_cfg interface, i.e. there is no select register.  There
> > are four 32bit registers:  Target address (low and high bits),
> > transfer length, control register.
> 
> If I read this interface correctly, a guest will have at least six
> faults to complete a typical fw_cfg dma transfer (select, target low,
> target high, transfer length, control register write, control register
> read).  I wonder if using a DMA transfer descriptor might be more
> efficient.
> 
> That is, if a transfer descriptor was defined with something like:
> 
> struct fwcfg_dma {
>     u16 command;
>     u16 select;
>     u32 transfer_length;
>     u64 target_addr;
> } PACKED;
> 
> and QEMU was informed of the transfer descriptor address (one fault on
> 32bit addresses; two faults on 64bit addresses) then QEMU could read
> the transfer descriptor, perform the requested operation, and then
> update the transfer descriptor on completion.  This would reduce the
> total number of faults between QEMU and the firmware, and allow for a
> more flexible interface for future growth.
> 
> -Kevin
> 

That is probably faster and more flexible. I think it's a good step
forward. But, on the other side, is this a too big break of what is
actually implemented in MMIO?

At the end we will have MMIO version, with a select and a data
register, and DMA version, with a 64 bit address register. How can we
differentiate between versions? Laszlo talks about the ID, but this is
already in one of the fields (so you need to know the protocol to get
the protocol). How could we do this transition more seamless?

Thanks
Marc

Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Kevin O'Connor]

On Wed, Jul 22, 2015 at 10:31:12AM +0200, Marc Marí wrote:
> On Wed, 22 Jul 2015 00:24:34 -0400
> "Kevin O'Connor" <kevin@koconnor.net> wrote:
> > On Tue, Jul 21, 2015 at 06:03:41PM +0200, Marc Marí wrote:
> > > From: Gerd Hoffmann <kraxel@redhat.com>
> > > 
> > > First draft of a fw_cfg dma interface.  Designed as add-on to the
> > > extisting fw_cfg interface, i.e. there is no select register.  There
> > > are four 32bit registers:  Target address (low and high bits),
> > > transfer length, control register.
> > 
> > If I read this interface correctly, a guest will have at least six
> > faults to complete a typical fw_cfg dma transfer (select, target low,
> > target high, transfer length, control register write, control register
> > read).  I wonder if using a DMA transfer descriptor might be more
> > efficient.
[...]
> That is probably faster and more flexible. I think it's a good step
> forward. But, on the other side, is this a too big break of what is
> actually implemented in MMIO?
> 
> At the end we will have MMIO version, with a select and a data
> register, and DMA version, with a 64 bit address register. How can we
> differentiate between versions? Laszlo talks about the ID, but this is
> already in one of the fields (so you need to know the protocol to get
> the protocol). How could we do this transition more seamless?

Well, one way would be to place a 64bit MMIO (or IO) register at a
magic address.  The firmware could then read the magic address and
check it against a signature (eg, "QEMU CFG").  If the signature
matches then it would know it could use the DMA version of the fw_cfg
interface (by issuing writes, instead of reads, to the magic address).
The firmware would then not have to use the older select/data fw_cfg
interface at all.

Another possibility would be to place the new fw_cfg dma register
address into a named fw_cfg "file" (eg, "fw_cfg_dma").  The firmware
could then use the existing select/data fw_cfg interface to check if
the new dma interface is available by scanning for that "fw_cfg_dma"
file.  This has the advantage of not requiring a new "magic address",
but has the disadvantage of a more complex probe.

Just a thought,
-Kevin

Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Laszlo Ersek]

On 07/22/15 19:18, Kevin O'Connor wrote:
> On Wed, Jul 22, 2015 at 10:31:12AM +0200, Marc Marí wrote:
>> On Wed, 22 Jul 2015 00:24:34 -0400
>> "Kevin O'Connor" <kevin@koconnor.net> wrote:
>>> On Tue, Jul 21, 2015 at 06:03:41PM +0200, Marc Marí wrote:
>>>> From: Gerd Hoffmann <kraxel@redhat.com>
>>>>
>>>> First draft of a fw_cfg dma interface.  Designed as add-on to the
>>>> extisting fw_cfg interface, i.e. there is no select register.  There
>>>> are four 32bit registers:  Target address (low and high bits),
>>>> transfer length, control register.
>>>
>>> If I read this interface correctly, a guest will have at least six
>>> faults to complete a typical fw_cfg dma transfer (select, target low,
>>> target high, transfer length, control register write, control register
>>> read).  I wonder if using a DMA transfer descriptor might be more
>>> efficient.
> [...]
>> That is probably faster and more flexible. I think it's a good step
>> forward. But, on the other side, is this a too big break of what is
>> actually implemented in MMIO?
>>
>> At the end we will have MMIO version, with a select and a data
>> register, and DMA version, with a 64 bit address register. How can we
>> differentiate between versions? Laszlo talks about the ID, but this is
>> already in one of the fields (so you need to know the protocol to get
>> the protocol). How could we do this transition more seamless?
> 
> Well, one way would be to place a 64bit MMIO (or IO) register at a
> magic address.  The firmware could then read the magic address and
> check it against a signature (eg, "QEMU CFG").  If the signature
> matches then it would know it could use the DMA version of the fw_cfg
> interface (by issuing writes, instead of reads, to the magic address).
> The firmware would then not have to use the older select/data fw_cfg
> interface at all.

So, with regard to this patchset, I guess I'm in the position of the
"hateful reviewer". I cannot give constructive ideas simply because I'm
not knowledgeable enough about "anything DMA" in QEMU. I can only
express my negative thoughs, whenever I have such. :)

In any case, I wouldn't recommend a signature check -- the fw-cfg
interface already has a signature mechanism (independently of DMA -- see
FW_CFG_SIGNATURE in docs/specs/fw_cfg.txt). Adding another signature
(for a sub-feature) feels unclean, especially since we have a feature
bitmap already, in one of the preexistent fw_cfg keys. (See FW_CFG_ID.)

So, the "slow" mechanism could be used to retrieve FW_CFG_ID. Then,
based on bit #1 (value 2), presence of he DMA interface could be deduced.

> Another possibility would be to place the new fw_cfg dma register
> address into a named fw_cfg "file" (eg, "fw_cfg_dma").  The firmware
> could then use the existing select/data fw_cfg interface to check if
> the new dma interface is available by scanning for that "fw_cfg_dma"
> file.  This has the advantage of not requiring a new "magic address",
> but has the disadvantage of a more complex probe.

I like this one so much that I'm worried I'm missing some details. :)

The more complex probe shouldn't be an issue. The DMA interface would
bring such huge savings (*) that the probe's cost would be "amortized".

(*) ... at least in the environment where I really care about this: on
Aarch64 KVM, libguestfs loads a kernel and an initrd through *extremely*
slow MMIO.

Shaving off a few milliseconds for x86 SeaBIOS, so that this stack can
"compete" with Docker (or whatever) for short-lived containers, is
perhaps a valid goal, but personally, meh. :) I intend to write client
code for this DMA interface only in AAVMF, and not in OVMF -- OVMF is
unavoidably too "busy" to be considered for containers, so the DMA
interface would just be a complication. But, for libguestfs on Aarch64
KVM, it is a game changer.

Thanks
Laszlo

Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Peter Maydell]

On 23 July 2015 at 14:13, Laszlo Ersek <lersek@redhat.com> wrote:
> On 07/22/15 19:18, Kevin O'Connor wrote:
>> Another possibility would be to place the new fw_cfg dma register
>> address into a named fw_cfg "file" (eg, "fw_cfg_dma").  The firmware
>> could then use the existing select/data fw_cfg interface to check if
>> the new dma interface is available by scanning for that "fw_cfg_dma"
>> file.  This has the advantage of not requiring a new "magic address",
>> but has the disadvantage of a more complex probe.
>
> I like this one so much that I'm worried I'm missing some details. :)

This requires the device itself to know its own address, which
is in QEMU possible but ugly enough to be worth avoiding.

For ARM MMIO the obvious answer is "the new register should
just go next to the first one". Does x86 do something that
means we can't put it somewhere equally straightforward
or do discovery via whatever x86 uses for discovering MMIO?

-- PMM

Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Laszlo Ersek]

On 07/23/15 15:35, Peter Maydell wrote:
> On 23 July 2015 at 14:13, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 07/22/15 19:18, Kevin O'Connor wrote:
>>> Another possibility would be to place the new fw_cfg dma register
>>> address into a named fw_cfg "file" (eg, "fw_cfg_dma").  The firmware
>>> could then use the existing select/data fw_cfg interface to check if
>>> the new dma interface is available by scanning for that "fw_cfg_dma"
>>> file.  This has the advantage of not requiring a new "magic address",
>>> but has the disadvantage of a more complex probe.
>>
>> I like this one so much that I'm worried I'm missing some details. :)
> 
> This requires the device itself to know its own address, which
> is in QEMU possible but ugly enough to be worth avoiding.
> 
> For ARM MMIO the obvious answer is "the new register should
> just go next to the first one". Does x86 do something that
> means we can't put it somewhere equally straightforward
> or do discovery via whatever x86 uses for discovering MMIO?

I don't know how x86 determines the MMIO mapping. As far as I gather
from the SeaBIOS patches and this QEMU series, 0xfef00000 is a
hand-picked fixed address. (See BIOS_CFG_DMA_ADDR in 7/7.)

0xfef00000 seems to fall right above the 1MB LAPIC range; I guess
there's no conflict with anything else...

Thanks
Laszlo

Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Marc Marí]

On Thu, 23 Jul 2015 15:45:25 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 07/23/15 15:35, Peter Maydell wrote:
> > On 23 July 2015 at 14:13, Laszlo Ersek <lersek@redhat.com> wrote:
> >> On 07/22/15 19:18, Kevin O'Connor wrote:
> >>> Another possibility would be to place the new fw_cfg dma register
> >>> address into a named fw_cfg "file" (eg, "fw_cfg_dma").  The
> >>> firmware could then use the existing select/data fw_cfg interface
> >>> to check if the new dma interface is available by scanning for
> >>> that "fw_cfg_dma" file.  This has the advantage of not requiring
> >>> a new "magic address", but has the disadvantage of a more complex
> >>> probe.
> >>
> >> I like this one so much that I'm worried I'm missing some
> >> details. :)
> > 
> > This requires the device itself to know its own address, which
> > is in QEMU possible but ugly enough to be worth avoiding.
> > 
> > For ARM MMIO the obvious answer is "the new register should
> > just go next to the first one". Does x86 do something that
> > means we can't put it somewhere equally straightforward
> > or do discovery via whatever x86 uses for discovering MMIO?
> 
> I don't know how x86 determines the MMIO mapping. As far as I gather
> from the SeaBIOS patches and this QEMU series, 0xfef00000 is a
> hand-picked fixed address. (See BIOS_CFG_DMA_ADDR in 7/7.)
> 
> 0xfef00000 seems to fall right above the 1MB LAPIC range; I guess
> there's no conflict with anything else...
> 

For the x86 part, I chose a completely arbitrary "magic address".
Ideally, this address should be discoverable by the BIOS, so it can be
moved around if some specification is changed. But a lot of device
configuration used in SeaBIOS comes from fw_cfg. That might be the
reason why the IO port was also hardcoded.

It would be good if somebody comes with an idea on how to make it
discoverable by the BIOS in x86.

Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Kevin O'Connor]

On Thu, Jul 23, 2015 at 02:35:05PM +0100, Peter Maydell wrote:
> On 23 July 2015 at 14:13, Laszlo Ersek <lersek@redhat.com> wrote:
> > On 07/22/15 19:18, Kevin O'Connor wrote:
> >> Another possibility would be to place the new fw_cfg dma register
> >> address into a named fw_cfg "file" (eg, "fw_cfg_dma").  The firmware
> >> could then use the existing select/data fw_cfg interface to check if
> >> the new dma interface is available by scanning for that "fw_cfg_dma"
> >> file.  This has the advantage of not requiring a new "magic address",
> >> but has the disadvantage of a more complex probe.
> >
> > I like this one so much that I'm worried I'm missing some details. :)
> 
> This requires the device itself to know its own address, which
> is in QEMU possible but ugly enough to be worth avoiding.
> 
> For ARM MMIO the obvious answer is "the new register should
> just go next to the first one". Does x86 do something that
> means we can't put it somewhere equally straightforward
> or do discovery via whatever x86 uses for discovering MMIO?

On x86 the fw_cfg select/data registers are in IO space at address
0x510 and 0x511.  I suspect one could put a "transfer descriptor
address" register in IO space at 0x514-0x51c (or 0x518-0x520 if
alignment is a concern, or even 0x510-0x518 if willing to play tricks
with read/write size).  I didn't see any conflicts after a quick
search - does anyone know if it would be a problem?

-Kevin

Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Stefan Hajnoczi]

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

On Wed, Jul 22, 2015 at 12:24:34AM -0400, Kevin O'Connor wrote:
> On Tue, Jul 21, 2015 at 06:03:41PM +0200, Marc Marí wrote:
> > From: Gerd Hoffmann <kraxel@redhat.com>
> > 
> > First draft of a fw_cfg dma interface.  Designed as add-on to the
> > extisting fw_cfg interface, i.e. there is no select register.  There
> > are four 32bit registers:  Target address (low and high bits), transfer
> > length, control register.
> 
> If I read this interface correctly, a guest will have at least six
> faults to complete a typical fw_cfg dma transfer (select, target low,
> target high, transfer length, control register write, control register
> read).  I wonder if using a DMA transfer descriptor might be more
> efficient.
> 
> That is, if a transfer descriptor was defined with something like:
> 
> struct fwcfg_dma {
>     u16 command;
>     u16 select;
>     u32 transfer_length;
>     u64 target_addr;
> } PACKED;
> 
> and QEMU was informed of the transfer descriptor address (one fault on
> 32bit addresses; two faults on 64bit addresses) then QEMU could read
> the transfer descriptor, perform the requested operation, and then
> update the transfer descriptor on completion.  This would reduce the
> total number of faults between QEMU and the firmware, and allow for a
> more flexible interface for future growth.

I like the idea.

In Marc's case of benchmarking -kernel/-initrd times it may not make a
big difference, but if we're going to add a new interface it might as
well be optimal.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [RFC 3/7] fw_cfg dma: adapt to vmstate changes

[Marc Marí]

From: Gerd Hoffmann <kraxel@redhat.com>

---
 hw/nvram/fw_cfg.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 5bcd0e0..0f35931 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -515,8 +515,16 @@ 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_UINT32(dma_len, FWCfgState),
@@ -525,13 +533,6 @@ static VMStateDescription vmstate_fw_cfg_dma = {
     },
 };
 
-static bool fw_cfg_dma_enabled(void *opaque)
-{
-    FWCfgState *s = opaque;
-
-    return s->dma_enabled;
-}
-
 static const VMStateDescription vmstate_fw_cfg = {
     .name = "fw_cfg",
     .version_id = 2,
@@ -542,13 +543,9 @@ static const VMStateDescription vmstate_fw_cfg = {
         VMSTATE_UINT32_V(cur_offset, FWCfgState, 2),
         VMSTATE_END_OF_LIST()
     },
-    .subsections = (VMStateSubsection[]) {
-        {
-            .vmsd   = &vmstate_fw_cfg_dma,
-            .needed = fw_cfg_dma_enabled,
-        }, {
-            /* end of list */
-        }
+    .subsections = (const VMStateDescription*[]) {
+        &vmstate_fw_cfg_dma,
+        NULL,
     }
 };
 
-- 
2.4.3

Re: [Qemu-devel] [RFC 3/7] fw_cfg dma: adapt to vmstate changes

[Stefan Hajnoczi]

On Tue, Jul 21, 2015 at 5:03 PM, Marc Marí <markmb@redhat.com> wrote:
> From: Gerd Hoffmann <kraxel@redhat.com>

This patch should be squashed into the patch that adds fw_cfg_dma_enabled().

[Qemu-devel] [RFC 4/7] enable fw_cfg dma for arm virt

[Marc Marí]

From: Gerd Hoffmann <kraxel@redhat.com>

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 docs/specs/fw_cfg.txt |  6 ++++++
 hw/arm/virt.c         | 11 ++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 64d9192..eac83a1 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -167,6 +167,12 @@ If a partial transfer happened before an error occured the address and
 length registers indicate how much data has been transfered
 successfully.
 
+== Register Locations ==
+
+=== arm ===
+
+classic fw_cfg base + 16
+
 = Host-side API =
 
 The following functions are available to the QEMU programmer for adding
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 374660c..d05f6a4 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -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

Re: [Qemu-devel] [RFC 4/7] enable fw_cfg dma for arm virt

[Peter Maydell]

On 21 July 2015 at 17:03, Marc Marí <markmb@redhat.com> wrote:
> From: Gerd Hoffmann <kraxel@redhat.com>
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  docs/specs/fw_cfg.txt |  6 ++++++
>  hw/arm/virt.c         | 11 ++++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 64d9192..eac83a1 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -167,6 +167,12 @@ If a partial transfer happened before an error occured the address and
>  length registers indicate how much data has been transfered
>  successfully.
>
> +== Register Locations ==
> +
> +=== arm ===
> +
> +classic fw_cfg base + 16
> +
>  = Host-side API =
>
>  The following functions are available to the QEMU programmer for adding
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 374660c..d05f6a4 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -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);

Don't we need to also update the device tree (and the spec for
that!) to indicate that we're providing a with-DMA fw-cfg
device ?

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

Is this really the right AddressSpace to use? Fishing
around in the CPU object seems a bit dubious...

thanks
-- PMM

Re: [Qemu-devel] [RFC 4/7] enable fw_cfg dma for arm virt

[Laszlo Ersek]

On 07/21/15 19:04, Peter Maydell wrote:
> On 21 July 2015 at 17:03, Marc Marí <markmb@redhat.com> wrote:
>> From: Gerd Hoffmann <kraxel@redhat.com>
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  docs/specs/fw_cfg.txt |  6 ++++++
>>  hw/arm/virt.c         | 11 ++++++++---
>>  2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
>> index 64d9192..eac83a1 100644
>> --- a/docs/specs/fw_cfg.txt
>> +++ b/docs/specs/fw_cfg.txt
>> @@ -167,6 +167,12 @@ If a partial transfer happened before an error occured the address and
>>  length registers indicate how much data has been transfered
>>  successfully.
>>
>> +== Register Locations ==
>> +
>> +=== arm ===
>> +
>> +classic fw_cfg base + 16
>> +
>>  = Host-side API =
>>
>>  The following functions are available to the QEMU programmer for adding
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 374660c..d05f6a4 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -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);
> 
> Don't we need to also update the device tree

Yes, we do (a15memmap[VIRT_FW_CFG]).

> (and the spec for
> that!)

Yes, we do ("Documentation/devicetree/bindings/arm/fw-cfg.txt").

> to indicate that we're providing a with-DMA fw-cfg
> device ?

Thanks
Laszlo

> 
>> @@ -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;
>> +        }
> 
> Is this really the right AddressSpace to use? Fishing
> around in the CPU object seems a bit dubious...
> 
> thanks
> -- PMM
> 
> 

Re: [Qemu-devel] [RFC 4/7] enable fw_cfg dma for arm virt

[Marc Marí]

On Tue, 21 Jul 2015 18:04:31 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 21 July 2015 at 17:03, Marc Marí <markmb@redhat.com> wrote:
> > From: Gerd Hoffmann <kraxel@redhat.com>
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  docs/specs/fw_cfg.txt |  6 ++++++
> >  hw/arm/virt.c         | 11 ++++++++---
> >  2 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> > index 64d9192..eac83a1 100644
> > --- a/docs/specs/fw_cfg.txt
> > +++ b/docs/specs/fw_cfg.txt
> > @@ -167,6 +167,12 @@ If a partial transfer happened before an error
> > occured the address and length registers indicate how much data has
> > been transfered successfully.
> >
> > +== Register Locations ==
> > +
> > +=== arm ===
> > +
> > +classic fw_cfg base + 16
> > +
> >  = Host-side API =
> >
> >  The following functions are available to the QEMU programmer for
> > adding diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 374660c..d05f6a4 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -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);
> 
> Don't we need to also update the device tree (and the spec for
> that!) to indicate that we're providing a with-DMA fw-cfg
> device ?
> 
> > @@ -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;
> > +        }
> 
> Is this really the right AddressSpace to use? Fishing
> around in the CPU object seems a bit dubious...
> 

I have not touched this part, I touched the x86 part, but the same
comment applies in both places:

I don't know much about how and why address spaces are used in QEMU, so
I'm not really sure which address space should I use for this purpose.
I'd appreciate some help in the issue.

Thanks
Marc

[Qemu-devel] [RFC 5/7] fw_cfg file sort

[Marc Marí]

From: Gerd Hoffmann <kraxel@redhat.com>

This is what it takes to have a sorted fw_cfg file directory.
Entries are inserted at the correct place instead of being
appended to the end in case sorting is enabled.

Compatibility fluff (enable sorting for new machine types only)
isn't there yet.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/nvram/fw_cfg.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 0f35931..83205e0 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -639,7 +639,7 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
                               FWCfgReadCallback callback, void *callback_opaque,
                               void *data, size_t len)
 {
-    int i, index;
+    int i, index, count;
     size_t dsize;
 
     if (!s->files) {
@@ -648,13 +648,31 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
         fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
     }
 
-    index = be32_to_cpu(s->files->count);
-    assert(index < FW_CFG_FILE_SLOTS);
+    count = be32_to_cpu(s->files->count);
+    assert(count < FW_CFG_FILE_SLOTS);
+
+    index = count;
+    if (1 /* sort entries */) {
+        while (index > 0 && strcmp(filename, s->files->f[index-1].name) < 0) {
+            s->files->f[index] =
+                s->files->f[index - 1];
+            s->files->f[index].select =
+                cpu_to_be16(FW_CFG_FILE_FIRST + index);
+            s->entries[0][FW_CFG_FILE_FIRST + index] =
+                s->entries[0][FW_CFG_FILE_FIRST + index - 1];
+            index--;
+        }
+        memset(&s->files->f[index],
+               0, sizeof(FWCfgFile));
+        memset(&s->entries[0][FW_CFG_FILE_FIRST + index],
+               0, sizeof(FWCfgEntry));
+    }
 
     pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name),
             filename);
-    for (i = 0; i < index; i++) {
-        if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
+    for (i = 0; i <= count; i++) {
+        if (i != index &&
+            strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
             error_report("duplicate fw_cfg file name: %s",
                          s->files->f[index].name);
             exit(1);
@@ -668,7 +686,7 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
     s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
     trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
 
-    s->files->count = cpu_to_be32(index+1);
+    s->files->count = cpu_to_be32(count+1);
 }
 
 void fw_cfg_add_file(FWCfgState *s,  const char *filename,
-- 
2.4.3

Re: [Qemu-devel] [RFC 5/7] fw_cfg file sort

[Stefan Hajnoczi]

On Tue, Jul 21, 2015 at 5:03 PM, Marc Marí <markmb@redhat.com> wrote:
> From: Gerd Hoffmann <kraxel@redhat.com>
>
> This is what it takes to have a sorted fw_cfg file directory.
> Entries are inserted at the correct place instead of being
> appended to the end in case sorting is enabled.
>
> Compatibility fluff (enable sorting for new machine types only)
> isn't there yet.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/nvram/fw_cfg.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)

Probably best to drop this patch from this series, unless it's needed.

I guess sorting files allows for binary search but it's unrelated to
DMA and needs to be accompanied with guest code (e.g. SeaBIOS) that
actually uses the feature.

Re: [Qemu-devel] [RFC 5/7] fw_cfg file sort

[Laszlo Ersek]

On 07/21/15 18:18, Stefan Hajnoczi wrote:
> On Tue, Jul 21, 2015 at 5:03 PM, Marc Marí <markmb@redhat.com> wrote:
>> From: Gerd Hoffmann <kraxel@redhat.com>
>>
>> This is what it takes to have a sorted fw_cfg file directory.
>> Entries are inserted at the correct place instead of being
>> appended to the end in case sorting is enabled.
>>
>> Compatibility fluff (enable sorting for new machine types only)
>> isn't there yet.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  hw/nvram/fw_cfg.c | 30 ++++++++++++++++++++++++------
>>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> Probably best to drop this patch from this series, unless it's needed.
> 
> I guess sorting files allows for binary search but it's unrelated to
> DMA and needs to be accompanied with guest code (e.g. SeaBIOS) that
> actually uses the feature.

I recall another discussion where the sorting was considered under a
migration aspect (not for the sake of binary search on the guest side).

.... Yes, here it is:

http://thread.gmane.org/gmane.comp.emulators.qemu/340424/focus=342398

The commit message should give a full rationale, preferably.

Thanks!
Laszlo

Re: [Qemu-devel] [RFC 5/7] fw_cfg file sort

[Marc Marí]

On Tue, 21 Jul 2015 21:53:06 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 07/21/15 18:18, Stefan Hajnoczi wrote:
> > On Tue, Jul 21, 2015 at 5:03 PM, Marc Marí <markmb@redhat.com>
> > wrote:
> >> From: Gerd Hoffmann <kraxel@redhat.com>
> >>
> >> This is what it takes to have a sorted fw_cfg file directory.
> >> Entries are inserted at the correct place instead of being
> >> appended to the end in case sorting is enabled.
> >>
> >> Compatibility fluff (enable sorting for new machine types only)
> >> isn't there yet.
> >>
> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >> ---
> >>  hw/nvram/fw_cfg.c | 30 ++++++++++++++++++++++++------
> >>  1 file changed, 24 insertions(+), 6 deletions(-)
> > 
> > Probably best to drop this patch from this series, unless it's
> > needed.
> > 
> > I guess sorting files allows for binary search but it's unrelated to
> > DMA and needs to be accompanied with guest code (e.g. SeaBIOS) that
> > actually uses the feature.
> 
> I recall another discussion where the sorting was considered under a
> migration aspect (not for the sake of binary search on the guest
> side).
> 
> .... Yes, here it is:
> 
> http://thread.gmane.org/gmane.comp.emulators.qemu/340424/focus=342398
> 
> The commit message should give a full rationale, preferably.
> 

I've not worked on it, it was already on the patches. So I'll drop it
for now, and it can be retaken in the future.

Thanks
Marc

[Qemu-devel] [RFC 6/7] Add offset register to fw_cfg DMA interface

[Marc Marí]

Signed-off-by: Marc Marí <markmb@redhat.com>
---
 hw/nvram/fw_cfg.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 83205e0..9a39d45 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -47,8 +47,9 @@
 #define FW_CFG_DMA_ADDR_LO        0
 #define FW_CFG_DMA_ADDR_HI        4
 #define FW_CFG_DMA_LENGTH         8
-#define FW_CFG_DMA_CONTROL       12
-#define FW_CFG_DMA_SIZE          16
+#define FW_CFG_DMA_OFFSET        12
+#define FW_CFG_DMA_CONTROL       16
+#define FW_CFG_DMA_SIZE          20
 
 /* FW_CFG_DMA_CONTROL bits */
 #define FW_CFG_DMA_CTL_ERROR   0x01
@@ -78,6 +79,7 @@ struct FWCfgState {
     dma_addr_t dma_addr;
     uint32_t   dma_len;
     uint32_t   dma_ctl;
+    uint32_t   dma_off;
 };
 
 struct FWCfgIoState {
@@ -338,6 +340,10 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
             return;
         }
 
+        for (i = 0; i < s->dma_off; ++i) {
+            fw_cfg_read(s);
+        }
+
         for (i = 0; i < len; i++) {
             ptr[i] = fw_cfg_read(s);
         }
@@ -366,6 +372,9 @@ static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr,
     case FW_CFG_DMA_LENGTH:
         ret = s->dma_len;
         break;
+    case FW_CFG_DMA_OFFSET:
+        ret = s->dma_off;
+        break;
     case FW_CFG_DMA_CONTROL:
         ret = s->dma_ctl;
         break;
@@ -390,6 +399,9 @@ static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
     case FW_CFG_DMA_LENGTH:
         s->dma_len = value;
         break;
+    case FW_CFG_DMA_OFFSET:
+        s->dma_off = value;
+        break;
     case FW_CFG_DMA_CONTROL:
         value &= FW_CFG_DMA_CTL_MASK;
         s->dma_ctl = value;
@@ -528,6 +540,7 @@ static VMStateDescription vmstate_fw_cfg_dma = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(dma_addr, FWCfgState),
         VMSTATE_UINT32(dma_len, FWCfgState),
+        VMSTATE_UINT32(dma_off, FWCfgState),
         VMSTATE_UINT32(dma_ctl, FWCfgState),
         VMSTATE_END_OF_LIST()
     },
@@ -791,7 +804,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
     if (dma_addr && dma_as) {
         FW_CFG(dev)->dma_as = dma_as;
         FW_CFG(dev)->dma_enabled = true;
-        sysbus_mmio_map(sbd, 1, dma_addr);
+        sysbus_mmio_map(sbd, 2, dma_addr);
     }
 
     return FW_CFG(dev);
-- 
2.4.3

Re: [Qemu-devel] [RFC 6/7] Add offset register to fw_cfg DMA interface

[Stefan Hajnoczi]

On Tue, Jul 21, 2015 at 5:03 PM, Marc Marí <markmb@redhat.com> wrote:
> Signed-off-by: Marc Marí <markmb@redhat.com>
> ---
>  hw/nvram/fw_cfg.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

No commit description, no docs/specs/fw_cfg.txt documentation.

I understand how the offset is supposed to work, but why is it
necessary?  No one needed it before so there must be a reason why you
decided to add it now.

Re: [Qemu-devel] [RFC 6/7] Add offset register to fw_cfg DMA interface

[Laszlo Ersek]

On 07/21/15 18:26, Stefan Hajnoczi wrote:
> On Tue, Jul 21, 2015 at 5:03 PM, Marc Marí <markmb@redhat.com> wrote:
>> Signed-off-by: Marc Marí <markmb@redhat.com>
>> ---
>>  hw/nvram/fw_cfg.c | 19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> No commit description, no docs/specs/fw_cfg.txt documentation.

Yes, those would be nice.

Also, I think this patch should be squashed into the main fw_cfg patch.

> I understand how the offset is supposed to work, but why is it
> necessary?  No one needed it before so there must be a reason why you
> decided to add it now.

I guess because of
<http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/9496/focus=9554>.

For me chunked transfers would be important (ie. transfering I+J=K bytes
from the same fw_cfg file should be possible as two separate accesses,
with I & J sizes), but I believe the offset register would not be
necessary just for that. So I think it's solely directed at Kevin's
feedback (see link above).

Thanks
Laszlo

Re: [Qemu-devel] [RFC 6/7] Add offset register to fw_cfg DMA interface

[Kevin O'Connor]

On Tue, Jul 21, 2015 at 10:06:51PM +0200, Laszlo Ersek wrote:
> On 07/21/15 18:26, Stefan Hajnoczi wrote:
> > On Tue, Jul 21, 2015 at 5:03 PM, Marc Marí <markmb@redhat.com> wrote:
> >> Signed-off-by: Marc Marí <markmb@redhat.com>
> >> ---
> >>  hw/nvram/fw_cfg.c | 19 ++++++++++++++++---
> >>  1 file changed, 16 insertions(+), 3 deletions(-)
> > 
> > No commit description, no docs/specs/fw_cfg.txt documentation.
> 
> Yes, those would be nice.
> 
> Also, I think this patch should be squashed into the main fw_cfg patch.
> 
> > I understand how the offset is supposed to work, but why is it
> > necessary?  No one needed it before so there must be a reason why you
> > decided to add it now.
> 
> I guess because of
> <http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/9496/focus=9554>.
> 
> For me chunked transfers would be important (ie. transfering I+J=K bytes
> from the same fw_cfg file should be possible as two separate accesses,
> with I & J sizes), but I believe the offset register would not be
> necessary just for that. So I think it's solely directed at Kevin's
> feedback (see link above).

The reason the "offset" field is useful is because several of the
fw_cfg files have headers, and it's necessary to read the header into
one area of memory and the payload into another area of memory.  So,
basically we want to be able to read a chunk of a fw_cfg entry to one
memory address and then read another chunk to another memory address.

Not sure what you mean by "I+J=K" but I suspect we have similar
requirements - the ability to read different parts of a fw_cfg entry
in different calls.  Without this ability we'd need to read the entire
entry into a big linear area of memory and then memmove that around.
If there is a way to accomplish this without an "offset" field then
that's fine too.

Cheers,
-Kevin

Re: [Qemu-devel] [RFC 6/7] Add offset register to fw_cfg DMA interface

[Laszlo Ersek]

On 07/21/15 22:16, Kevin O'Connor wrote:
> On Tue, Jul 21, 2015 at 10:06:51PM +0200, Laszlo Ersek wrote:
>> On 07/21/15 18:26, Stefan Hajnoczi wrote:
>>> On Tue, Jul 21, 2015 at 5:03 PM, Marc Marí <markmb@redhat.com> wrote:
>>>> Signed-off-by: Marc Marí <markmb@redhat.com>
>>>> ---
>>>>  hw/nvram/fw_cfg.c | 19 ++++++++++++++++---
>>>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> No commit description, no docs/specs/fw_cfg.txt documentation.
>>
>> Yes, those would be nice.
>>
>> Also, I think this patch should be squashed into the main fw_cfg patch.
>>
>>> I understand how the offset is supposed to work, but why is it
>>> necessary?  No one needed it before so there must be a reason why you
>>> decided to add it now.
>>
>> I guess because of
>> <http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/9496/focus=9554>.
>>
>> For me chunked transfers would be important (ie. transfering I+J=K bytes
>> from the same fw_cfg file should be possible as two separate accesses,
>> with I & J sizes), but I believe the offset register would not be
>> necessary just for that. So I think it's solely directed at Kevin's
>> feedback (see link above).
> 
> The reason the "offset" field is useful is because several of the
> fw_cfg files have headers, and it's necessary to read the header into
> one area of memory and the payload into another area of memory.  So,
> basically we want to be able to read a chunk of a fw_cfg entry to one
> memory address and then read another chunk to another memory address.
> 
> Not sure what you mean by "I+J=K" but I suspect we have similar
> requirements - the ability to read different parts of a fw_cfg entry
> in different calls.

Yes.

> Without this ability we'd need to read the entire
> entry into a big linear area of memory and then memmove that around.
> If there is a way to accomplish this without an "offset" field then
> that's fine too.

What I meant by "I+J=K" is that the qemu-internal offset into the
selected fw_cfg blob is not (and should not) be reset between calls. So,
if you want to implement a "scatter read" in the firmware, just break up
the read into appropriately sized, smaller transfers, and pass different
target addresses. The source offset should be carried forward from one
call to the other.

If you look at patch #2 in this RFC series, the only qemu-internal
fw-cfg API call that it exercises is fw_cfg_read(). That function
advances / maintains the "cur_offset" field of FWCfgState. Nothing in
the patch re-sets "cur_offset" -- that happens only when the firmware
writes to the selector register, and fw_cfg_select() gets called.

"docs/specs/fw_cfg.txt" also states,

> Initially following a write to the selector register, the data offset
> will be set to zero. Each successful access to the data register will
> increment the data offset by the appropriate access width.

This is a very nice property of fw_cfg (allowing for chunked and scatter
reads, if you like), and I think patch #2 preserves that property. If
we'd like a readv()-like pattern in the firmware, we can decompose that
into read()-like calls; a pread()-like interface is not necessary.

So, I think patch #6 is not actually needed.

Thanks
Laszlo

Re: [Qemu-devel] [RFC 6/7] Add offset register to fw_cfg DMA interface

[Kevin O'Connor]

On Tue, Jul 21, 2015 at 10:36:39PM +0200, Laszlo Ersek wrote:
> On 07/21/15 22:16, Kevin O'Connor wrote:
> > Without this ability we'd need to read the entire
> > entry into a big linear area of memory and then memmove that around.
> > If there is a way to accomplish this without an "offset" field then
> > that's fine too.
> 
> What I meant by "I+J=K" is that the qemu-internal offset into the
> selected fw_cfg blob is not (and should not) be reset between calls.

I missed that in my earlier review of the code.  With that in place, I
agree that the offset isn't needed.

-Kevin

Re: [Qemu-devel] [RFC 6/7] Add offset register to fw_cfg DMA interface

[Marc Marí]

On Tue, 21 Jul 2015 22:36:39 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 07/21/15 22:16, Kevin O'Connor wrote:
> > On Tue, Jul 21, 2015 at 10:06:51PM +0200, Laszlo Ersek wrote:
> >> On 07/21/15 18:26, Stefan Hajnoczi wrote:
> >>> On Tue, Jul 21, 2015 at 5:03 PM, Marc Marí <markmb@redhat.com>
> >>> wrote:
> >>>> Signed-off-by: Marc Marí <markmb@redhat.com>
> >>>> ---
> >>>>  hw/nvram/fw_cfg.c | 19 ++++++++++++++++---
> >>>>  1 file changed, 16 insertions(+), 3 deletions(-)
> >>>
> >>> No commit description, no docs/specs/fw_cfg.txt documentation.
> >>
> >> Yes, those would be nice.
> >>
> >> Also, I think this patch should be squashed into the main fw_cfg
> >> patch.
> >>
> >>> I understand how the offset is supposed to work, but why is it
> >>> necessary?  No one needed it before so there must be a reason why
> >>> you decided to add it now.
> >>
> >> I guess because of
> >> <http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/9496/focus=9554>.

Yes, it's for that. Sorry for the short description, I don't know why I
could think it was enough.

> >>
> >> For me chunked transfers would be important (ie. transfering I+J=K
> >> bytes from the same fw_cfg file should be possible as two separate
> >> accesses, with I & J sizes), but I believe the offset register
> >> would not be necessary just for that. So I think it's solely
> >> directed at Kevin's feedback (see link above).
> > 
> > The reason the "offset" field is useful is because several of the
> > fw_cfg files have headers, and it's necessary to read the header
> > into one area of memory and the payload into another area of
> > memory.  So, basically we want to be able to read a chunk of a
> > fw_cfg entry to one memory address and then read another chunk to
> > another memory address.
> > 
> > Not sure what you mean by "I+J=K" but I suspect we have similar
> > requirements - the ability to read different parts of a fw_cfg entry
> > in different calls.
> 
> Yes.
> 
> > Without this ability we'd need to read the entire
> > entry into a big linear area of memory and then memmove that around.
> > If there is a way to accomplish this without an "offset" field then
> > that's fine too.
> 
> What I meant by "I+J=K" is that the qemu-internal offset into the
> selected fw_cfg blob is not (and should not) be reset between calls.
> So, if you want to implement a "scatter read" in the firmware, just
> break up the read into appropriately sized, smaller transfers, and
> pass different target addresses. The source offset should be carried
> forward from one call to the other.
> 
> If you look at patch #2 in this RFC series, the only qemu-internal
> fw-cfg API call that it exercises is fw_cfg_read(). That function
> advances / maintains the "cur_offset" field of FWCfgState. Nothing in
> the patch re-sets "cur_offset" -- that happens only when the firmware
> writes to the selector register, and fw_cfg_select() gets called.
> 
> "docs/specs/fw_cfg.txt" also states,
> 
> > Initially following a write to the selector register, the data
> > offset will be set to zero. Each successful access to the data
> > register will increment the data offset by the appropriate access
> > width.
> 
> This is a very nice property of fw_cfg (allowing for chunked and
> scatter reads, if you like), and I think patch #2 preserves that
> property. If we'd like a readv()-like pattern in the firmware, we can
> decompose that into read()-like calls; a pread()-like interface is
> not necessary.
> 
> So, I think patch #6 is not actually needed.

It's true that I missed that point in the guest side (SeaBIOS support
for these patches). This means, the actual guest code can be improved a
lot.

But I still think this is useful. With the DMA interface, every read
operation copies the data into the memory region provided. If you want
to discard a big chunk of data, you need a big chunk of memory to place
it, unless you want to corrupt the memory. In the IO interface, you
could just read and not save until you are where you want to read.

This doesn't mean to leave the offset. Probably there's another
solution. I can think of using address NULL to discard the data.
Probably there are other good or better options.

Of course, you can always read the size of the region you have
allocated as many times as necessary to reach the desired offset, but
it's better to do it in just one operation.

Thanks
Marc

Re: [Qemu-devel] [RFC 6/7] Add offset register to fw_cfg DMA interface

[Stefan Hajnoczi]

On Tue, Jul 21, 2015 at 5:03 PM, Marc Marí <markmb@redhat.com> wrote:
> @@ -338,6 +340,10 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
>              return;
>          }
>
> +        for (i = 0; i < s->dma_off; ++i) {
> +            fw_cfg_read(s);
> +        }
> +
>          for (i = 0; i < len; i++) {
>              ptr[i] = fw_cfg_read(s);
>          }

Please consume s->dma_off bytes outside the while loop so we don't
repeat this multiple times if the memory map is smaller than expected.

It would also be useful to set s->dma_off to 0 after consuming the
bytes.  That way the next request doesn't need to write to the
register (unless it wants to use an offset).  This should be
documented in docs/specs/fw_cfg.txt.

Please limit dma_off to a maximum of e->len so that an out-of-bounds
value doesn't burn CPU needlessly.

[Qemu-devel] [RFC 7/7] fw_cfg DMA for x86

[Marc Marí]

Enable fw_cfg for x86 machines. Create new machine to avoid
incompatibilites.

Signed-off-by: Marc Marí <markmb@redhat.com>
---
 hw/i386/pc.c         | 21 ++++++++++++++++++---
 hw/i386/pc_piix.c    | 25 +++++++++++++++++++++++--
 hw/i386/pc_q35.c     | 26 ++++++++++++++++++++++++--
 include/hw/i386/pc.h |  1 +
 4 files changed, 66 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7661ea9..5b202fa 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -87,6 +87,7 @@ void pc_set_legacy_acpi_data_size(void)
 }
 
 #define BIOS_CFG_IOPORT 0x510
+#define BIOS_CFG_DMA_ADDR 0xfef00000
 #define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
 #define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
 #define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
@@ -718,7 +719,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, hwaddr fw_cfg_addr)
 {
     FWCfgState *fw_cfg;
     uint8_t *smbios_tables, *smbios_anchor;
@@ -727,7 +728,13 @@ 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);
+    if (as && fw_cfg_addr) {
+        fw_cfg = fw_cfg_init_mem_wide(fw_cfg_addr + 8, fw_cfg_addr, 8,
+                                                    fw_cfg_addr + 10, as);
+    } else {
+        fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
+    }
+
     /* 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 +1309,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 +1399,14 @@ FWCfgState *pc_memory_init(MachineState *machine,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = bochs_bios_init();
+    if (guest_info->fw_cfg_dma) {
+        as = g_malloc(sizeof(*as));
+        address_space_init(as, ram_below_4g, "pc.as");
+        fw_cfg = bochs_bios_init(as, BIOS_CFG_DMA_ADDR);
+    } else {
+        fw_cfg = bochs_bios_init(NULL, 0);
+    }
+
     rom_set_fw(fw_cfg);
 
     if (guest_info->has_reserved_memory && pcms->hotplug_memory.base) {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 8167b12..bb94c87 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -74,6 +74,7 @@ static bool smbios_uuid_encoded = true;
 static bool gigabyte_align = true;
 static bool has_reserved_memory = true;
 static bool kvmclock_enabled = true;
+static bool fw_cfg_dma = true;
 
 /* PC hardware initialisation */
 static void pc_init1(MachineState *machine)
@@ -168,6 +169,7 @@ static void pc_init1(MachineState *machine)
     guest_info->isapc_ram_fw = !pci_enabled;
     guest_info->has_reserved_memory = has_reserved_memory;
     guest_info->rsdp_in_ram = rsdp_in_ram;
+    guest_info->fw_cfg_dma = fw_cfg_dma;
 
     if (smbios_defaults) {
         MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -304,9 +306,16 @@ static void pc_init1(MachineState *machine)
     }
 }
 
+static void pc_compat_2_4(MachineState *machine)
+{
+    fw_cfg_dma = false;
+}
+
 static void pc_compat_2_3(MachineState *machine)
 {
     PCMachineState *pcms = PC_MACHINE(machine);
+
+    pc_compat_2_4(machine);
     savevm_skip_section_footers();
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
@@ -477,7 +486,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
     m->hot_add_cpu = pc_hot_add_cpu;
 }
 
-static void pc_i440fx_2_4_machine_options(MachineClass *m)
+static void pc_i440fx_2_5_machine_options(MachineClass *m)
 {
     pc_i440fx_machine_options(m);
     m->default_machine_opts = "firmware=bios-256k.bin";
@@ -486,7 +495,19 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
     m->is_default = 1;
 }
 
-DEFINE_I440FX_MACHINE(v2_4, "pc-i440fx-2.4", NULL,
+DEFINE_I440FX_MACHINE(v2_5, "pc-i440fx-2.5", NULL,
+                      pc_i440fx_2_5_machine_options)
+
+static void pc_i440fx_2_4_machine_options(MachineClass *m)
+{
+    pc_i440fx_machine_options(m);
+    m->default_machine_opts = "firmware=bios-256k.bin";
+    m->default_display = "std";
+    m->alias = NULL;
+    m->is_default = 0;
+}
+
+DEFINE_I440FX_MACHINE(v2_4, "pc-i440fx-2.4", pc_compat_2_4,
                       pc_i440fx_2_4_machine_options)
 
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 974aead..7230da6 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -61,6 +61,7 @@ static bool smbios_uuid_encoded = true;
  */
 static bool gigabyte_align = true;
 static bool has_reserved_memory = true;
+static bool fw_cfg_dma = true;
 
 /* PC hardware initialisation */
 static void pc_q35_init(MachineState *machine)
@@ -156,6 +157,7 @@ static void pc_q35_init(MachineState *machine)
     guest_info->has_acpi_build = has_acpi_build;
     guest_info->has_reserved_memory = has_reserved_memory;
     guest_info->rsdp_in_ram = rsdp_in_ram;
+    guest_info->fw_cfg_dma = fw_cfg_dma;
 
     /* Migration was not supported in 2.0 for Q35, so do not bother
      * with this hack (see hw/i386/acpi-build.c).
@@ -287,9 +289,16 @@ static void pc_q35_init(MachineState *machine)
     }
 }
 
+static void pc_compat_2_4(MachineState *machine)
+{
+    fw_cfg_dma = false;
+}
+
 static void pc_compat_2_3(MachineState *machine)
 {
     PCMachineState *pcms = PC_MACHINE(machine);
+
+    pc_compat_2_4(machine);
     savevm_skip_section_footers();
     if (kvm_enabled()) {
         pcms->smm = ON_OFF_AUTO_OFF;
@@ -392,7 +401,7 @@ static void pc_q35_machine_options(MachineClass *m)
     m->units_per_default_bus = 1;
 }
 
-static void pc_q35_2_4_machine_options(MachineClass *m)
+static void pc_q35_2_5_machine_options(MachineClass *m)
 {
     pc_q35_machine_options(m);
     m->default_machine_opts = "firmware=bios-256k.bin";
@@ -402,7 +411,20 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
     m->alias = "q35";
 }
 
-DEFINE_Q35_MACHINE(v2_4, "pc-q35-2.4", NULL,
+DEFINE_Q35_MACHINE(v2_5, "pc-q35-2.5", NULL,
+                   pc_q35_2_5_machine_options);
+
+static void pc_q35_2_4_machine_options(MachineClass *m)
+{
+    pc_q35_machine_options(m);
+    m->default_machine_opts = "firmware=bios-256k.bin";
+    m->default_display = "std";
+    m->no_floppy = 1;
+    m->no_tco = 0;
+    m->alias = NULL;
+}
+
+DEFINE_Q35_MACHINE(v2_4, "pc-q35-2.4", pc_compat_2_4,
                    pc_q35_2_4_machine_options);
 
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 15e3352..6c7eb65 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -103,6 +103,7 @@ struct PcGuestInfo {
     bool has_acpi_build;
     bool has_reserved_memory;
     bool rsdp_in_ram;
+    bool fw_cfg_dma;
 };
 
 /* parallel.c */
-- 
2.4.3

Re: [Qemu-devel] [RFC 7/7] fw_cfg DMA for x86

[Peter Maydell]

On 21 July 2015 at 17:03, Marc Marí <markmb@redhat.com> wrote:
> Enable fw_cfg for x86 machines. Create new machine to avoid
> incompatibilites.

> @@ -1391,7 +1399,14 @@ FWCfgState *pc_memory_init(MachineState *machine,
>                                          option_rom_mr,
>                                          1);
>
> -    fw_cfg = bochs_bios_init();
> +    if (guest_info->fw_cfg_dma) {
> +        as = g_malloc(sizeof(*as));
> +        address_space_init(as, ram_below_4g, "pc.as");
> +        fw_cfg = bochs_bios_init(as, BIOS_CFG_DMA_ADDR);
> +    } else {
> +        fw_cfg = bochs_bios_init(NULL, 0);
> +    }
> +

As with ARM: what's the rationale for choosing this particular
AddressSpace as the target for fw_cfg DMA? (I'm not saying it's
wrong, necessarily -- I was just surprised we didn't just use
the system address space.)

thanks
-- PMM

Re: [Qemu-devel] [RFC 7/7] fw_cfg DMA for x86

[Marc Marí]

On Tue, 21 Jul 2015 18:14:40 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 21 July 2015 at 17:03, Marc Marí <markmb@redhat.com> wrote:
> > Enable fw_cfg for x86 machines. Create new machine to avoid
> > incompatibilites.
> 
> > @@ -1391,7 +1399,14 @@ FWCfgState *pc_memory_init(MachineState
> > *machine, option_rom_mr,
> >                                          1);
> >
> > -    fw_cfg = bochs_bios_init();
> > +    if (guest_info->fw_cfg_dma) {
> > +        as = g_malloc(sizeof(*as));
> > +        address_space_init(as, ram_below_4g, "pc.as");
> > +        fw_cfg = bochs_bios_init(as, BIOS_CFG_DMA_ADDR);
> > +    } else {
> > +        fw_cfg = bochs_bios_init(NULL, 0);
> > +    }
> > +
> 
> As with ARM: what's the rationale for choosing this particular
> AddressSpace as the target for fw_cfg DMA? (I'm not saying it's
> wrong, necessarily -- I was just surprised we didn't just use
> the system address space.)
> 
> thanks
> -- PMM

I took something that seemed logical for me, as the BIOS will be
working in 32 bits. But, as with ARM, I'm not really sure which
address space should I use for this purpose. I'd appreciate some help
in the issue.

Thanks
Marc