[Qemu-devel] QEMU fw_cfg DMA interface

[Qemu-devel] QEMU fw_cfg DMA interface

[Marc Marí]

Implementation of the FW CFG DMA interface.

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

QEMU commit 16a1b6e and SeaBIOS commit e4d2b8c
QEMU startup time: .080
BIOS startup time: .060
Kernel setup time: .586
Total time: .726

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

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

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

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

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

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

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

[Marc Marí]

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

Based on Gerd Hoffman's initial implementation.

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

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

 docs/specs/fw_cfg.txt     |  76 +++++++++++++++-
 hw/arm/virt.c             |   9 +-
 hw/i386/pc.c              |  11 ++-
 hw/nvram/fw_cfg.c         | 228 +++++++++++++++++++++++++++++++++++++++++++---
 include/hw/nvram/fw_cfg.h |  16 +++-
 5 files changed, 314 insertions(+), 26 deletions(-)

-- 
2.4.3

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

[Marc Marí]

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

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

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

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

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

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

[Marc Marí]

Add fw_cfg DMA interface specification in the documentation.

Based on Gerd Hoffman's initial implementation.

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

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

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

[Marc Marí]

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

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

Based on Gerd Hoffman's initial implementation.

Signed-off-by: Marc Marí <markmb@redhat.com>
---
 hw/arm/virt.c             |   2 +-
 hw/nvram/fw_cfg.c         | 228 +++++++++++++++++++++++++++++++++++++++++++---
 include/hw/nvram/fw_cfg.h |  16 +++-
 3 files changed, 230 insertions(+), 16 deletions(-)

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

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

[Marc Marí]

Enable the fw_cfg DMA interface for the ARM virt machine.

Based on Gerd Hoffman's initial implementation.

Signed-off-by: Marc Marí <markmb@redhat.com>
---
 hw/arm/virt.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 3568107..47f4ad3 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -113,7 +113,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
-    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
+    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -651,13 +651,13 @@ static void create_flash(const VirtBoardInfo *vbi)
     g_free(nodename);
 }
 
-static void create_fw_cfg(const VirtBoardInfo *vbi)
+static void create_fw_cfg(const VirtBoardInfo *vbi, AddressSpace *as)
 {
     hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
     hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
     char *nodename;
 
-    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
+    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
 
     nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
@@ -919,6 +919,7 @@ static void machvirt_init(MachineState *machine)
 
     create_fdt(vbi);
 
+
     for (n = 0; n < smp_cpus; n++) {
         ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
         CPUClass *cc = CPU_CLASS(oc);
@@ -984,7 +985,7 @@ static void machvirt_init(MachineState *machine)
      */
     create_virtio_devices(vbi, pic);
 
-    create_fw_cfg(vbi);
+    create_fw_cfg(vbi, &address_space_memory);
     rom_set_fw(fw_cfg_find());
 
     guest_info->smp_cpus = smp_cpus;
-- 
2.4.3

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

[Marc Marí]

Enable the fw_cfg DMA interface for all the x86 platforms.

Based on Gerd Hoffman's initial implementation.

Signed-off-by: Marc Marí <markmb@redhat.com>
---
 hw/i386/pc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 56aecce..6e02061 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -753,14 +753,15 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
     }
 }
 
-static FWCfgState *bochs_bios_init(void)
+static FWCfgState *bochs_bios_init(AddressSpace *as)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;
     int i, j;
     unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
 
-    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
+    fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 4, as);
+
     /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
      *
      * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU hotplug
@@ -1316,6 +1317,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
     MemoryRegion *ram_below_4g, *ram_above_4g;
     FWCfgState *fw_cfg;
     MachineState *machine = MACHINE(pcms);
+    AddressSpace *as;
 
     assert(machine->ram_size == pcms->below_4g_mem_size +
                                 pcms->above_4g_mem_size);
@@ -1407,7 +1409,10 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = bochs_bios_init();
+    as = g_malloc(sizeof(*as));
+    address_space_init(as, ram_below_4g, "pc.as");
+    fw_cfg = bochs_bios_init(as);
+
     rom_set_fw(fw_cfg);
 
     if (guest_info->has_reserved_memory && pcms->hotplug_memory.base) {
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3 5/5] Enable fw_cfg DMA interface for x86

[Gerd Hoffmann]

  Hi,

> -    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
> +    fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 4, as);

> +    as = g_malloc(sizeof(*as));
> +    address_space_init(as, ram_below_4g, "pc.as");
> +    fw_cfg = bochs_bios_init(as);

I think we can use address_space_memory on x86 too (and can remove the
AddressSpace argument to fw_cfg_init_io_dma).

Otherwise the series looks fine to me (and thanks for picking it up).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3 5/5] Enable fw_cfg DMA interface for x86

[Peter Maydell]

On 18 September 2015 at 11:58, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>> -    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
>> +    fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 4, as);
>
>> +    as = g_malloc(sizeof(*as));
>> +    address_space_init(as, ram_below_4g, "pc.as");
>> +    fw_cfg = bochs_bios_init(as);
>
> I think we can use address_space_memory on x86 too (and can remove the
> AddressSpace argument to fw_cfg_init_io_dma).

I would prefer to keep the AS argument to fw_cfg_init_io_dma().
Random devices shouldn't have their own ideas about what buses
they're plugged into to do DMA (which is effectively what this
is doing) -- the board should be responsible for wiring them
up appropriately.

thanks
-- PMM

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

[Peter Maydell]

On 18 September 2015 at 09:58, Marc Marí <markmb@redhat.com> wrote:
> Add fw_cfg DMA interface specification in the documentation.
>
> Based on Gerd Hoffman's initial implementation.
>
> Signed-off-by: Marc Marí <markmb@redhat.com>
> ---
>  docs/specs/fw_cfg.txt | 65 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 61 insertions(+), 4 deletions(-)

> +To check result, read the "control" field:

"check the result"

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Peter Maydell]

On 18 September 2015 at 09:58, Marc Marí <markmb@redhat.com> wrote:
> Enable the fw_cfg DMA interface for the ARM virt machine.
>
> Based on Gerd Hoffman's initial implementation.
>
> Signed-off-by: Marc Marí <markmb@redhat.com>
> ---
>  hw/arm/virt.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 3568107..47f4ad3 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -113,7 +113,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
>      [VIRT_UART] =               { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
> -    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
> +    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -651,13 +651,13 @@ static void create_flash(const VirtBoardInfo *vbi)
>      g_free(nodename);
>  }
>
> -static void create_fw_cfg(const VirtBoardInfo *vbi)
> +static void create_fw_cfg(const VirtBoardInfo *vbi, AddressSpace *as)
>  {
>      hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
>      hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
>      char *nodename;
>
> -    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
> +    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
>
>      nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>      qemu_fdt_add_subnode(vbi->fdt, nodename);
> @@ -919,6 +919,7 @@ static void machvirt_init(MachineState *machine)
>
>      create_fdt(vbi);
>
> +

Stray whitespace change.

>      for (n = 0; n < smp_cpus; n++) {
>          ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
>          CPUClass *cc = CPU_CLASS(oc);
> @@ -984,7 +985,7 @@ static void machvirt_init(MachineState *machine)
>       */
>      create_virtio_devices(vbi, pic);
>
> -    create_fw_cfg(vbi);
> +    create_fw_cfg(vbi, &address_space_memory);
>      rom_set_fw(fw_cfg_find());
>
>      guest_info->smp_cpus = smp_cpus;
> --
> 2.4.3

Otherwise:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Peter Maydell]

On 18 September 2015 at 09:58, Marc Marí <markmb@redhat.com> wrote:
> Based on the specifications on docs/specs/fw_cfg.txt
>
> This interface is an addon. The old interface can still be used as usual.
>
> Based on Gerd Hoffman's initial implementation.
>
> Signed-off-by: Marc Marí <markmb@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 0/5] fw_cfg DMA interface

[Kevin O'Connor]

On Fri, Sep 18, 2015 at 10:58:44AM +0200, Marc Marí wrote:
> Implement host-side of the FW CFG DMA interface both for x86 and ARM.
> 
> Based on Gerd Hoffman's initial implementation.

Thanks for working on this Marc!

Any chance you could add the patch below to the series (or merge it
into your series)?

The patch adds a signature to the DMA address IO register.  With the
current implementation, a future firmware would have to implement the
V1 fw_cfg interface just to probe for the dma interface.  It might be
useful if future firmwares (that don't care about backwards
compatibility with old versions of qemu) could probe for the dma
fw_cfg interface by just checking for a signature (and therefore not
require all the V1 code just to probe).

-Kevin


commit ae6d8df012ef9b21ae17bfb0383d116f71ba1d58
Author: Kevin O'Connor <kevin@koconnor.net>
Date:   Fri Sep 18 14:14:55 2015 -0400

    fw_cfg: Define a static signature to be returned on DMA port reads
    
    Return a static signature ("QEMU CFG") if the guest does a read to the
    DMA address io register.
    
    Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index d5f9ddd..5bf3f65 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -93,6 +93,10 @@ by selecting the "signature" item using key 0x0000 (FW_CFG_SIGNATU
RE),
 and reading four bytes from the data register. If the fw_cfg device is
 present, the four bytes read will contain the characters "QEMU".
 
+Additionaly, if the DMA interface is available then a read to the DMA
+Address will return 0x51454d5520434647 ("QEMU CFG" in big-endian
+format).
+
 === Revision / feature bitmap (Key 0x0001, FW_CFG_ID) ===
 
 A 32-bit little-endian unsigned int, this item is used to check for enabled
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index d11d8c5..d95075d 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -53,6 +53,8 @@
 #define FW_CFG_DMA_CTL_SKIP    0x04
 #define FW_CFG_DMA_CTL_SELECT  0x08
 
+#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647 /* "QEMU CFG" */
+
 typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
@@ -393,6 +395,12 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
     trace_fw_cfg_read(s, 0);
 }
 
+static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr,
+                                    unsigned size)
+{
+    return FW_CFG_DMA_SIGNATURE >> ((8 - addr - size) * 8);
+}
+
 static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
                                  uint64_t value, unsigned size)
 {
@@ -416,8 +424,8 @@ static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
 static bool fw_cfg_dma_mem_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
-    return is_write && ((size == 4 && (addr == 0 || addr == 4)) ||
-                        (size == 8 && addr == 0));
+    return !is_write || ((size == 4 && (addr == 0 || addr == 4)) ||
+                         (size == 8 && addr == 0));
 }
 
 static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
@@ -488,6 +496,7 @@ static const MemoryRegionOps fw_cfg_comb_mem_ops = {
 };
 
 static const MemoryRegionOps fw_cfg_dma_mem_ops = {
+    .read = fw_cfg_dma_mem_read,
     .write = fw_cfg_dma_mem_write,
     .endianness = DEVICE_BIG_ENDIAN,
     .valid.accepts = fw_cfg_dma_mem_valid,

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

[Kevin O'Connor]

On Fri, Sep 18, 2015 at 10:58:47AM +0200, Marc Marí wrote:
> Based on the specifications on docs/specs/fw_cfg.txt
> 
> This interface is an addon. The old interface can still be used as usual.
> 
> Based on Gerd Hoffman's initial implementation.
> 
> Signed-off-by: Marc Marí <markmb@redhat.com>
> ---
>  hw/arm/virt.c             |   2 +-
>  hw/nvram/fw_cfg.c         | 228 +++++++++++++++++++++++++++++++++++++++++++---
>  include/hw/nvram/fw_cfg.h |  16 +++-
>  3 files changed, 230 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e9324f5..3568107 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -657,7 +657,7 @@ static void create_fw_cfg(const VirtBoardInfo *vbi)
>      hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
>      char *nodename;
>  
> -    fw_cfg_init_mem_wide(base + 8, base, 8);
> +    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
>  
>      nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>      qemu_fdt_add_subnode(vbi->fdt, nodename);
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 658f8c4..d11d8c5 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -23,6 +23,7 @@
>   */
>  #include "hw/hw.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/dma.h"
>  #include "hw/isa/isa.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/sysbus.h"
> @@ -30,7 +31,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
>  
> -#define FW_CFG_SIZE 2
> +#define FW_CFG_CTL_SIZE 2
>  #define FW_CFG_NAME "fw_cfg"
>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>  
> @@ -42,6 +43,16 @@
>  #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
>  #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
>  
> +/* FW_CFG_VERSION bits */
> +#define FW_CFG_VERSION      0x01
> +#define FW_CFG_VERSION_DMA  0x02
> +
> +/* FW_CFG_DMA_CONTROL bits */
> +#define FW_CFG_DMA_CTL_ERROR   0x01
> +#define FW_CFG_DMA_CTL_READ    0x02
> +#define FW_CFG_DMA_CTL_SKIP    0x04
> +#define FW_CFG_DMA_CTL_SELECT  0x08
> +
>  typedef struct FWCfgEntry {
>      uint32_t len;
>      uint8_t *data;
> @@ -59,6 +70,10 @@ struct FWCfgState {
>      uint16_t cur_entry;
>      uint32_t cur_offset;
>      Notifier machine_ready;
> +
> +    bool dma_enabled;
> +    AddressSpace *dma_as;
> +    dma_addr_t dma_addr;
>  };
>  
>  struct FWCfgIoState {
> @@ -66,8 +81,8 @@ struct FWCfgIoState {
>      FWCfgState parent_obj;
>      /*< public >*/
>  
> -    MemoryRegion comb_iomem;
> -    uint32_t iobase;
> +    MemoryRegion comb_iomem, dma_iomem;
> +    uint32_t iobase, dma_iobase;
>  };
>  
>  struct FWCfgMemState {
> @@ -75,7 +90,7 @@ struct FWCfgMemState {
>      FWCfgState parent_obj;
>      /*< public >*/
>  
> -    MemoryRegion ctl_iomem, data_iomem;
> +    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
>      uint32_t data_width;
>      MemoryRegionOps wide_data_ops;
>  };
> @@ -292,6 +307,119 @@ static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
>      } while (i);
>  }
>  
> +static void fw_cfg_dma_transfer(FWCfgState *s)
> +{
> +    dma_addr_t len;
> +    FWCfgDmaAccess dma;
> +    int arch;
> +    FWCfgEntry *e;
> +    int read;
> +    dma_addr_t dma_addr;
> +
> +    /* Reset the address before the next access */
> +    dma_addr = s->dma_addr;
> +    s->dma_addr = 0;
> +
> +    dma.address = ldq_be_dma(s->dma_as,
> +                            dma_addr + offsetof(FWCfgDmaAccess, address));
> +    dma.length = ldl_be_dma(s->dma_as,
> +                            dma_addr + offsetof(FWCfgDmaAccess, length));
> +    dma.control = ldl_be_dma(s->dma_as,
> +                            dma_addr + offsetof(FWCfgDmaAccess, control));
> +
> +    if (dma.control & FW_CFG_DMA_CTL_SELECT) {
> +        fw_cfg_select(s, dma.control >> 16);
> +    }
> +
> +    arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> +    e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
> +
> +    if (dma.control & FW_CFG_DMA_CTL_READ) {
> +        read = 1;
> +    } else if (dma.control & FW_CFG_DMA_CTL_SKIP) {
> +        read = 0;
> +    } else {
> +        return;

Minor nit - if FW_CFG_DMA_CTL_SELECT is set but not _READ nor _SKIP
then this return will exit without writing back the updated control
field.  I think the "return" could be replaced with "dma.length = 0"
to fix this.

Otherwise, the series looks good to me.

-Kevin

Re: [Qemu-devel] [PATCH v3 0/5] fw_cfg DMA interface

[Marc Marí]

On Fri, 18 Sep 2015 14:25:09 -0400
"Kevin O'Connor" <kevin@koconnor.net> wrote:

> On Fri, Sep 18, 2015 at 10:58:44AM +0200, Marc Marí wrote:
> > Implement host-side of the FW CFG DMA interface both for x86 and
> > ARM.
> > 
> > Based on Gerd Hoffman's initial implementation.
> 
> Thanks for working on this Marc!
> 
> Any chance you could add the patch below to the series (or merge it
> into your series)?

Unless it is decided to merge the series as is, I'll send another
version with the little nitpicks corrected. I'll add this patch too.

Thank you also for all the comments!

Marc

> The patch adds a signature to the DMA address IO register.  With the
> current implementation, a future firmware would have to implement the
> V1 fw_cfg interface just to probe for the dma interface.  It might be
> useful if future firmwares (that don't care about backwards
> compatibility with old versions of qemu) could probe for the dma
> fw_cfg interface by just checking for a signature (and therefore not
> require all the V1 code just to probe).
> 
> -Kevin
> 
> 
> commit ae6d8df012ef9b21ae17bfb0383d116f71ba1d58
> Author: Kevin O'Connor <kevin@koconnor.net>
> Date:   Fri Sep 18 14:14:55 2015 -0400
> 
>     fw_cfg: Define a static signature to be returned on DMA port reads
>     
>     Return a static signature ("QEMU CFG") if the guest does a read
> to the DMA address io register.
>     
>     Signed-off-by: Kevin O'Connor <kevin@koconnor.net>
> 
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index d5f9ddd..5bf3f65 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -93,6 +93,10 @@ by selecting the "signature" item using key 0x0000
> (FW_CFG_SIGNATU RE),
>  and reading four bytes from the data register. If the fw_cfg device
> is present, the four bytes read will contain the characters "QEMU".
>  
> +Additionaly, if the DMA interface is available then a read to the DMA
> +Address will return 0x51454d5520434647 ("QEMU CFG" in big-endian
> +format).
> +
>  === Revision / feature bitmap (Key 0x0001, FW_CFG_ID) ===
>  
>  A 32-bit little-endian unsigned int, this item is used to check for
> enabled diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index d11d8c5..d95075d 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -53,6 +53,8 @@
>  #define FW_CFG_DMA_CTL_SKIP    0x04
>  #define FW_CFG_DMA_CTL_SELECT  0x08
>  
> +#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647 /* "QEMU CFG" */
> +
>  typedef struct FWCfgEntry {
>      uint32_t len;
>      uint8_t *data;
> @@ -393,6 +395,12 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
>      trace_fw_cfg_read(s, 0);
>  }
>  
> +static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr,
> +                                    unsigned size)
> +{
> +    return FW_CFG_DMA_SIGNATURE >> ((8 - addr - size) * 8);
> +}
> +
>  static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
>                                   uint64_t value, unsigned size)
>  {
> @@ -416,8 +424,8 @@ static void fw_cfg_dma_mem_write(void *opaque,
> hwaddr addr, static bool fw_cfg_dma_mem_valid(void *opaque, hwaddr
> addr, unsigned size, bool is_write)
>  {
> -    return is_write && ((size == 4 && (addr == 0 || addr == 4)) ||
> -                        (size == 8 && addr == 0));
> +    return !is_write || ((size == 4 && (addr == 0 || addr == 4)) ||
> +                         (size == 8 && addr == 0));
>  }
>  
>  static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
> @@ -488,6 +496,7 @@ static const MemoryRegionOps fw_cfg_comb_mem_ops
> = { };
>  
>  static const MemoryRegionOps fw_cfg_dma_mem_ops = {
> +    .read = fw_cfg_dma_mem_read,
>      .write = fw_cfg_dma_mem_write,
>      .endianness = DEVICE_BIG_ENDIAN,
>      .valid.accepts = fw_cfg_dma_mem_valid,
> 

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

[Laszlo Ersek]

On 09/18/15 10:58, Marc Marí wrote:
> Enable the fw_cfg DMA interface for the ARM virt machine.
> 
> Based on Gerd Hoffman's initial implementation.
> 
> Signed-off-by: Marc Marí <markmb@redhat.com>
> ---
>  hw/arm/virt.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 3568107..47f4ad3 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -113,7 +113,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
>      [VIRT_UART] =               { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
> -    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
> +    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -651,13 +651,13 @@ static void create_flash(const VirtBoardInfo *vbi)
>      g_free(nodename);
>  }
>  
> -static void create_fw_cfg(const VirtBoardInfo *vbi)
> +static void create_fw_cfg(const VirtBoardInfo *vbi, AddressSpace *as)
>  {
>      hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
>      hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
>      char *nodename;
>  
> -    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
> +    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
>  
>      nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>      qemu_fdt_add_subnode(vbi->fdt, nodename);
> @@ -919,6 +919,7 @@ static void machvirt_init(MachineState *machine)
>  
>      create_fdt(vbi);
>  
> +
>      for (n = 0; n < smp_cpus; n++) {
>          ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
>          CPUClass *cc = CPU_CLASS(oc);
> @@ -984,7 +985,7 @@ static void machvirt_init(MachineState *machine)
>       */
>      create_virtio_devices(vbi, pic);
>  
> -    create_fw_cfg(vbi);
> +    create_fw_cfg(vbi, &address_space_memory);
>      rom_set_fw(fw_cfg_find());
>  
>      guest_info->smp_cpus = smp_cpus;
> 

I got excited that the work got this far (thanks a lot for it, and I apologize on falling back on the review), so I wanted to start writing the edk2 / ArmVirtPkg client code for it.

I applied your v3 series on top of current master (b12a84ce3c27e42c8f51c436aa196938d5cc2c71). First I wanted to see a new DTB:

$ qemu-system-aarch64 -machine virt,dumpdtb=xx.dtb

Unfortunately it crashes with a failed assertion:

qemu-system-aarch64: hw/core/sysbus.c:130: sysbus_mmio_map_common: Assertion `n >= 0 && n < dev->num_mmio' failed.

The problem is that you have a third (conditional) sysbus_mmio_map() in fw_cfg_init_mem_wide(), from patch #3, which would depend on the similarly conditional sysbus_init_mmio() call in fw_cfg_mem_realize().

However, that prerequisite sysbus_init_mmio() is never executed in fw_cfg_mem_realize(), because it would depend on the (FW_CFG(s)->dma_enabled) field, which at that point *cannot* have been set at all.

So you have to set it through a property, because that's the only way you can pass it to the realize method.

Please squash the following patch into patch #3:

> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index d11d8c5..946abb5 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -799,9 +799,11 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>      SysBusDevice *sbd;
>      FWCfgState *s;
>      uint32_t version = FW_CFG_VERSION;
> +    bool dma_enabled = dma_addr && dma_as;
>  
>      dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
>      qdev_prop_set_uint32(dev, "data_width", data_width);
> +    qdev_prop_set_bit(dev, "dma_enabled", dma_enabled);
>  
>      fw_cfg_init1(dev);
>  
> @@ -811,9 +813,8 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>  
>      s = FW_CFG(dev);
>  
> -    if (dma_addr && dma_as) {
> +    if (dma_enabled) {
>          s->dma_as = dma_as;
> -        s->dma_enabled = true;
>          s->dma_addr = 0;
>          sysbus_mmio_map(sbd, 2, dma_addr);
>          version |= FW_CFG_VERSION_DMA;
> @@ -891,6 +892,8 @@ static const TypeInfo fw_cfg_io_info = {
>  
>  static Property fw_cfg_mem_properties[] = {
>      DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
> +    DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
> +                     false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  

Thanks
Laszlo

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

[Laszlo Ersek]

On 09/18/15 10:58, Marc Marí wrote:
> Enable the fw_cfg DMA interface for the ARM virt machine.
> 
> Based on Gerd Hoffman's initial implementation.
> 
> Signed-off-by: Marc Marí <markmb@redhat.com>
> ---
>  hw/arm/virt.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 3568107..47f4ad3 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -113,7 +113,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
>      [VIRT_UART] =               { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
> -    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
> +    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },

Okay, Laszlo is the hateful reviewer. Sorry about that. I'm late, yes.

But: this says 0x00000014, ie 20 bytes in decimal. I don't think that's
correct; it should be 0x18 -- 24 bytes in decimal. From patch #2:
"DMA Address address:       Base + 16 (8 bytes)".

Thanks (and I'm sorry about being late!)
Laszlo

>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -651,13 +651,13 @@ static void create_flash(const VirtBoardInfo *vbi)
>      g_free(nodename);
>  }
>  
> -static void create_fw_cfg(const VirtBoardInfo *vbi)
> +static void create_fw_cfg(const VirtBoardInfo *vbi, AddressSpace *as)
>  {
>      hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
>      hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
>      char *nodename;
>  
> -    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
> +    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
>  
>      nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>      qemu_fdt_add_subnode(vbi->fdt, nodename);
> @@ -919,6 +919,7 @@ static void machvirt_init(MachineState *machine)
>  
>      create_fdt(vbi);
>  
> +
>      for (n = 0; n < smp_cpus; n++) {
>          ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
>          CPUClass *cc = CPU_CLASS(oc);
> @@ -984,7 +985,7 @@ static void machvirt_init(MachineState *machine)
>       */
>      create_virtio_devices(vbi, pic);
>  
> -    create_fw_cfg(vbi);
> +    create_fw_cfg(vbi, &address_space_memory);
>      rom_set_fw(fw_cfg_find());
>  
>      guest_info->smp_cpus = smp_cpus;
> 

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

[Marc Marí]

On Fri, 18 Sep 2015 22:16:46 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 09/18/15 10:58, Marc Marí wrote:
> > Enable the fw_cfg DMA interface for the ARM virt machine.
> > 
> > Based on Gerd Hoffman's initial implementation.
> > 
> > Signed-off-by: Marc Marí <markmb@redhat.com>
> > ---
> >  hw/arm/virt.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 3568107..47f4ad3 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -113,7 +113,7 @@ static const MemMapEntry a15memmap[] = {
> >      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
> >      [VIRT_UART] =               { 0x09000000, 0x00001000 },
> >      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
> > -    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
> > +    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },
> 
> Okay, Laszlo is the hateful reviewer. Sorry about that. I'm late, yes.
> 
> But: this says 0x00000014, ie 20 bytes in decimal. I don't think
> that's correct; it should be 0x18 -- 24 bytes in decimal. From patch
> #2: "DMA Address address:       Base + 16 (8 bytes)".

It's not your problem if I don't know how to count. So don't
apologize :).

And it's better to catch this stupid little mistakes now.

Thanks
Marc

> Thanks (and I'm sorry about being late!)
> Laszlo
> 
> >      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
> >      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of
> > that size */ [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> > @@ -651,13 +651,13 @@ static void create_flash(const VirtBoardInfo
> > *vbi) g_free(nodename);
> >  }
> >  
> > -static void create_fw_cfg(const VirtBoardInfo *vbi)
> > +static void create_fw_cfg(const VirtBoardInfo *vbi, AddressSpace
> > *as) {
> >      hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
> >      hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
> >      char *nodename;
> >  
> > -    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
> > +    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
> >  
> >      nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
> >      qemu_fdt_add_subnode(vbi->fdt, nodename);
> > @@ -919,6 +919,7 @@ static void machvirt_init(MachineState *machine)
> >  
> >      create_fdt(vbi);
> >  
> > +
> >      for (n = 0; n < smp_cpus; n++) {
> >          ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU,
> > cpustr[0]); CPUClass *cc = CPU_CLASS(oc);
> > @@ -984,7 +985,7 @@ static void machvirt_init(MachineState *machine)
> >       */
> >      create_virtio_devices(vbi, pic);
> >  
> > -    create_fw_cfg(vbi);
> > +    create_fw_cfg(vbi, &address_space_memory);
> >      rom_set_fw(fw_cfg_find());
> >  
> >      guest_info->smp_cpus = smp_cpus;
> > 
> 

Re: [Qemu-devel] [PATCH v3 0/5] fw_cfg DMA interface

[Peter Maydell]

On 18 September 2015 at 19:25, Kevin O'Connor <kevin@koconnor.net> wrote:
> On Fri, Sep 18, 2015 at 10:58:44AM +0200, Marc Marí wrote:
>> Implement host-side of the FW CFG DMA interface both for x86 and ARM.
>>
>> Based on Gerd Hoffman's initial implementation.
>
> Thanks for working on this Marc!
>
> Any chance you could add the patch below to the series (or merge it
> into your series)?
>
> The patch adds a signature to the DMA address IO register.  With the
> current implementation, a future firmware would have to implement the
> V1 fw_cfg interface just to probe for the dma interface.  It might be
> useful if future firmwares (that don't care about backwards
> compatibility with old versions of qemu) could probe for the dma
> fw_cfg interface by just checking for a signature (and therefore not
> require all the V1 code just to probe).
>
> -Kevin
>
>
> commit ae6d8df012ef9b21ae17bfb0383d116f71ba1d58
> Author: Kevin O'Connor <kevin@koconnor.net>
> Date:   Fri Sep 18 14:14:55 2015 -0400
>
>     fw_cfg: Define a static signature to be returned on DMA port reads
>
>     Return a static signature ("QEMU CFG") if the guest does a read to the
>     DMA address io register.
>
>     Signed-off-by: Kevin O'Connor <kevin@koconnor.net>
>
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index d5f9ddd..5bf3f65 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -93,6 +93,10 @@ by selecting the "signature" item using key 0x0000 (FW_CFG_SIGNATU
> RE),
>  and reading four bytes from the data register. If the fw_cfg device is
>  present, the four bytes read will contain the characters "QEMU".
>
> +Additionaly, if the DMA interface is available then a read to the DMA
> +Address will return 0x51454d5520434647 ("QEMU CFG" in big-endian
> +format).
> +

I don't think I understand this. If you know the DMA Address
port or register exists, then you know (by definition) that
the DMA interface is available. If you don't know that the
DMA interface is available then you can't read from the DMA
Address port or register because it might not exist and could
therefore cause you to blow up.

If you want to be able to tell without doing the "use the
old-style interface to query the version" thing, then you
need to look in the ACPI or device tree tables (and those
tables need to be such that you can tell the difference,
which is the case for at least device tree; haven't checked
ACPI.)

thanks
-- PMM

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

[Laszlo Ersek]

On 09/18/15 22:24, Marc Marí wrote:
> On Fri, 18 Sep 2015 22:16:46 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 09/18/15 10:58, Marc Marí wrote:
>>> Enable the fw_cfg DMA interface for the ARM virt machine.
>>>
>>> Based on Gerd Hoffman's initial implementation.
>>>
>>> Signed-off-by: Marc Marí <markmb@redhat.com>
>>> ---
>>>  hw/arm/virt.c | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 3568107..47f4ad3 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -113,7 +113,7 @@ static const MemMapEntry a15memmap[] = {
>>>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
>>>      [VIRT_UART] =               { 0x09000000, 0x00001000 },
>>>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
>>> -    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
>>> +    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },
>>
>> Okay, Laszlo is the hateful reviewer. Sorry about that. I'm late, yes.
>>
>> But: this says 0x00000014, ie 20 bytes in decimal. I don't think
>> that's correct; it should be 0x18 -- 24 bytes in decimal. From patch
>> #2: "DMA Address address:       Base + 16 (8 bytes)".
> 
> It's not your problem if I don't know how to count. So don't
> apologize :).
> 
> And it's better to catch this stupid little mistakes now.

Got some good news: with those two fixups in place (register block size
corrected, and dma_enabled set via device property), I could test the
AAVMF / ArmVirtPkg / <insert your favorite synonym here> patches.

On my APM Mustang, downloading a decompressed kernel (14,475,776 bytes),
a decompressed initrd (18,177,264), and a cmdline (104 bytes :)), in
total 32,653,144 bytes, takes approx. 24 seconds with the 8-byte wide
MMIO data register. (Yeah, it's *really* slow.)

Using the DMA interface, the same takes about 52 milliseconds, and that
still includes one progress message per 1 MB downloaded :)

It's a factor of approx. 450. Not bad. Not bad. :)

Thanks
Laszlo


> Thanks
> Marc
> 
>> Thanks (and I'm sorry about being late!)
>> Laszlo
>>
>>>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>>>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of
>>> that size */ [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
>>> @@ -651,13 +651,13 @@ static void create_flash(const VirtBoardInfo
>>> *vbi) g_free(nodename);
>>>  }
>>>  
>>> -static void create_fw_cfg(const VirtBoardInfo *vbi)
>>> +static void create_fw_cfg(const VirtBoardInfo *vbi, AddressSpace
>>> *as) {
>>>      hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
>>>      hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
>>>      char *nodename;
>>>  
>>> -    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
>>> +    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
>>>  
>>>      nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>>>      qemu_fdt_add_subnode(vbi->fdt, nodename);
>>> @@ -919,6 +919,7 @@ static void machvirt_init(MachineState *machine)
>>>  
>>>      create_fdt(vbi);
>>>  
>>> +
>>>      for (n = 0; n < smp_cpus; n++) {
>>>          ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU,
>>> cpustr[0]); CPUClass *cc = CPU_CLASS(oc);
>>> @@ -984,7 +985,7 @@ static void machvirt_init(MachineState *machine)
>>>       */
>>>      create_virtio_devices(vbi, pic);
>>>  
>>> -    create_fw_cfg(vbi);
>>> +    create_fw_cfg(vbi, &address_space_memory);
>>>      rom_set_fw(fw_cfg_find());
>>>  
>>>      guest_info->smp_cpus = smp_cpus;
>>>
>>
> 

Re: [Qemu-devel] [PATCH v3 0/5] fw_cfg DMA interface

[Kevin O'Connor]

On Fri, Sep 18, 2015 at 11:47:52PM +0100, Peter Maydell wrote:
> On 18 September 2015 at 19:25, Kevin O'Connor <kevin@koconnor.net> wrote:
> > +Additionaly, if the DMA interface is available then a read to the DMA
> > +Address will return 0x51454d5520434647 ("QEMU CFG" in big-endian
> > +format).
> > +
> 
> I don't think I understand this. If you know the DMA Address
> port or register exists, then you know (by definition) that
> the DMA interface is available. If you don't know that the
> DMA interface is available then you can't read from the DMA
> Address port or register because it might not exist and could
> therefore cause you to blow up.
> 
> If you want to be able to tell without doing the "use the
> old-style interface to query the version" thing, then you
> need to look in the ACPI or device tree tables (and those
> tables need to be such that you can tell the difference,
> which is the case for at least device tree; haven't checked
> ACPI.)

Hi Peter,

On x86 the firmware can't use acpi (nor device tree) to find fw_cfg
because fw_cfg is what is used to transfer acpi to the firmware.  So,
the firmware just hard codes the address.  As a "sanity check", the
firmware currently checks for a signature before using fw_cfg to
verify everything is working correctly (outw(0x0000, 0x510);
inb(0x511) == 'Q'; inb(0x511) == 'E'; ...).  A check for the new dma
interface involves an additional query (outw(0x0001, 0x510);
inb(0x511) == 3; ...).

I'm proposing that a future firmware (that didn't need to support old
versions of QEMU) could use a simpler sanity check instead (inl(0x514)
== "QEMU"; inl(0x518) == " CFG").

Granted, both the old check and the new proposed check would not be
needed on platforms that have a device tree transmitted separately
from fw_cfg.  Though, even on those platforms, there is no harm in
defining what happens on a read event.

-Kevin

Re: [Qemu-devel] [PATCH v3 0/5] fw_cfg DMA interface

[Peter Maydell]

On 19 September 2015 at 00:43, Kevin O'Connor <kevin@koconnor.net> wrote:
> On x86 the firmware can't use acpi (nor device tree) to find fw_cfg
> because fw_cfg is what is used to transfer acpi to the firmware.  So,
> the firmware just hard codes the address.  As a "sanity check", the
> firmware currently checks for a signature before using fw_cfg to
> verify everything is working correctly (outw(0x0000, 0x510);
> inb(0x511) == 'Q'; inb(0x511) == 'E'; ...).  A check for the new dma
> interface involves an additional query (outw(0x0001, 0x510);
> inb(0x511) == 3; ...).
>
> I'm proposing that a future firmware (that didn't need to support old
> versions of QEMU) could use a simpler sanity check instead (inl(0x514)
> == "QEMU"; inl(0x518) == " CFG").

But what happens if you try this on an old QEMU? Won't it not
have the newer ports present and so do bad things? At least
on ARM trying to read from something you don't know for certain
to exist is a bad idea because you're likely to get a fault.

thanks
-- PMM

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

[Marc Marí]

On Sat, 19 Sep 2015 01:10:46 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 09/18/15 22:24, Marc Marí wrote:
> > On Fri, 18 Sep 2015 22:16:46 +0200
> > Laszlo Ersek <lersek@redhat.com> wrote:
> > 
> >> On 09/18/15 10:58, Marc Marí wrote:
> >>> Enable the fw_cfg DMA interface for the ARM virt machine.
> >>>
> >>> Based on Gerd Hoffman's initial implementation.
> >>>
> >>> Signed-off-by: Marc Marí <markmb@redhat.com>
> >>> ---
> >>>  hw/arm/virt.c | 9 +++++----
> >>>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >>> index 3568107..47f4ad3 100644
> >>> --- a/hw/arm/virt.c
> >>> +++ b/hw/arm/virt.c
> >>> @@ -113,7 +113,7 @@ static const MemMapEntry a15memmap[] = {
> >>>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
> >>>      [VIRT_UART] =               { 0x09000000, 0x00001000 },
> >>>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
> >>> -    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
> >>> +    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },
> >>
> >> Okay, Laszlo is the hateful reviewer. Sorry about that. I'm late,
> >> yes.
> >>
> >> But: this says 0x00000014, ie 20 bytes in decimal. I don't think
> >> that's correct; it should be 0x18 -- 24 bytes in decimal. From
> >> patch #2: "DMA Address address:       Base + 16 (8 bytes)".
> > 
> > It's not your problem if I don't know how to count. So don't
> > apologize :).
> > 
> > And it's better to catch this stupid little mistakes now.
> 
> Got some good news: with those two fixups in place (register block
> size corrected, and dma_enabled set via device property), I could
> test the AAVMF / ArmVirtPkg / <insert your favorite synonym here>
> patches.
> 
> On my APM Mustang, downloading a decompressed kernel (14,475,776
> bytes), a decompressed initrd (18,177,264), and a cmdline (104
> bytes :)), in total 32,653,144 bytes, takes approx. 24 seconds with
> the 8-byte wide MMIO data register. (Yeah, it's *really* slow.)
> 
> Using the DMA interface, the same takes about 52 milliseconds, and
> that still includes one progress message per 1 MB downloaded :)
> 
> It's a factor of approx. 450. Not bad. Not bad. :)

Not bad. Not bad :). In x86 the speedup is high but not so brutal. I'm
really happy that it works so well.

Thanks
Marc

> Thanks
> Laszlo
> 
> 
> > Thanks
> > Marc
> > 
> >> Thanks (and I'm sorry about being late!)
> >> Laszlo
> >>
> >>>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
> >>>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of
> >>> that size */ [VIRT_PLATFORM_BUS] =       { 0x0c000000,
> >>> 0x02000000 }, @@ -651,13 +651,13 @@ static void
> >>> create_flash(const VirtBoardInfo *vbi) g_free(nodename);
> >>>  }
> >>>  
> >>> -static void create_fw_cfg(const VirtBoardInfo *vbi)
> >>> +static void create_fw_cfg(const VirtBoardInfo *vbi, AddressSpace
> >>> *as) {
> >>>      hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
> >>>      hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
> >>>      char *nodename;
> >>>  
> >>> -    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
> >>> +    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
> >>>  
> >>>      nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
> >>>      qemu_fdt_add_subnode(vbi->fdt, nodename);
> >>> @@ -919,6 +919,7 @@ static void machvirt_init(MachineState
> >>> *machine) 
> >>>      create_fdt(vbi);
> >>>  
> >>> +
> >>>      for (n = 0; n < smp_cpus; n++) {
> >>>          ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU,
> >>> cpustr[0]); CPUClass *cc = CPU_CLASS(oc);
> >>> @@ -984,7 +985,7 @@ static void machvirt_init(MachineState
> >>> *machine) */
> >>>      create_virtio_devices(vbi, pic);
> >>>  
> >>> -    create_fw_cfg(vbi);
> >>> +    create_fw_cfg(vbi, &address_space_memory);
> >>>      rom_set_fw(fw_cfg_find());
> >>>  
> >>>      guest_info->smp_cpus = smp_cpus;
> >>>
> >>
> > 
> 

Re: [Qemu-devel] [PATCH v3 0/5] fw_cfg DMA interface

[Kevin O'Connor]

On Sat, Sep 19, 2015 at 10:48:37AM +0100, Peter Maydell wrote:
> On 19 September 2015 at 00:43, Kevin O'Connor <kevin@koconnor.net> wrote:
> > On x86 the firmware can't use acpi (nor device tree) to find fw_cfg
> > because fw_cfg is what is used to transfer acpi to the firmware.  So,
> > the firmware just hard codes the address.  As a "sanity check", the
> > firmware currently checks for a signature before using fw_cfg to
> > verify everything is working correctly (outw(0x0000, 0x510);
> > inb(0x511) == 'Q'; inb(0x511) == 'E'; ...).  A check for the new dma
> > interface involves an additional query (outw(0x0001, 0x510);
> > inb(0x511) == 3; ...).
> >
> > I'm proposing that a future firmware (that didn't need to support old
> > versions of QEMU) could use a simpler sanity check instead (inl(0x514)
> > == "QEMU"; inl(0x518) == " CFG").
> 
> But what happens if you try this on an old QEMU? Won't it not
> have the newer ports present and so do bad things? At least
> on ARM trying to read from something you don't know for certain
> to exist is a bad idea because you're likely to get a fault.

Not on x86 - it used to be the norm to probe for old ISA devices via
io port reads and writes (eg, serial ports and lpt ports were detected
that way).  Here's what adding this to seabios:

  dprintf(1, "outl: %x %x\n", inl(0x514), inl(0x518));

reports on qemu v2.3 and earlier:

  outl: ffffffff ffffffff

on latest qemu with Marc's patches:

  outl: 0 0

and with my additional patch:

  outl: 554d4551 47464320

It's not a huge deal if you don't want to include the additional
signature.  It's not required as the v1 signature check still works
(see docs/specs/fw_cfg.txt), but the v1 check is a bit ugly and a new
additional simpler signature didn't seem like it would hurt.

-Kevin

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

[Gabriel L. Somlo]

On Sat, 19 Sep 2015, Laszlo Ersek wrote:
> Got some good news: with those two fixups in place (register block
> size corrected, and dma_enabled set via device property), I could
> test the AAVMF / ArmVirtPkg / <insert your favorite synonym here>
> patches.
>
> On my APM Mustang, downloading a decompressed kernel (14,475,776
> bytes), a decompressed initrd (18,177,264), and a cmdline (104 bytes :)),
> in total 32,653,144 bytes, takes approx. 24 seconds with the 8-byte wide
> MMIO data register. (Yeah, it's *really* slow.)
>
> Using the DMA interface, the same takes about 52 milliseconds, and
> that still includes one progress message per 1 MB downloaded :)
>
> It's a factor of approx. 450. Not bad. Not bad. :)

So I've been catching up (after a several-week-long day-job related detour :)
with the latest developments in fw_cfg -- and the DMA stuff looks good, and
makes for a very educational read!

I was re-reading the documentation for fw_cfg_add_file_callback(),
and noticed that non-dma read operations check for the presence
of a callback (and call it if present) for *every* *single* *byte*,
even on 64-bit MMIO reads. That's also what the documentation says
(in docs/specs/fw_cfg.txt, being moved into fw_cfg.h as per
 http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05315.html).

During DMA reads, however, the callback is only checked once before
each chunk, effectively once per DMA read operation.

Now, typical callbacks I found throughout the qemu source tend to return
immediately except for the first time they're invoked, but I wonder if
skipping over all those extra "do I have a callback, if so call it,
mostly so it can return without doing anything" per-byte operations
account in some significant part for the dramatically faster transfers?

Not sure how I'd test for that -- besides my not having anything
resembling a viable ARM setup, I'm not sure if limiting the callbacks
to only be invoked if (s->cur_offset == 0) would make sense, just as a
test ?

Either way, I'll send out a v2 of my fw_cfg function-call doc patch
to additionally say something like:

  * structure residing at key value FW_CFG_FILE_DIR, containing the
  * item name,
  * data size, and assigned selector key value.
  * Additionally, set a callback function (and argument) to be called
  * each
- * time a byte is read by the guest from this particular item.
+ * time a byte is read by the guest from this particular item, or once per
+ * each DMA guest read operation.
  * NOTE: In addition to the opaque argument set here, the callback
  * function
  * takes the current data offset as an additional argument, allowing
  * it the
  * option of only acting upon specific offset values (e.g., 0, before
  * the

Let me know what you all think...

Thanks much,
--Gabriel

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

[Stefan Hajnoczi]

On Thu, Oct 22, 2015 at 05:22:16PM -0400, Gabriel L. Somlo wrote:
> I was re-reading the documentation for fw_cfg_add_file_callback(),
> and noticed that non-dma read operations check for the presence
> of a callback (and call it if present) for *every* *single* *byte*,
> even on 64-bit MMIO reads. That's also what the documentation says
> (in docs/specs/fw_cfg.txt, being moved into fw_cfg.h as per
>  http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05315.html).
> 
> During DMA reads, however, the callback is only checked once before
> each chunk, effectively once per DMA read operation.
> 
> Now, typical callbacks I found throughout the qemu source tend to return
> immediately except for the first time they're invoked, but I wonder if
> skipping over all those extra "do I have a callback, if so call it,
> mostly so it can return without doing anything" per-byte operations
> account in some significant part for the dramatically faster transfers?
> 
> Not sure how I'd test for that -- besides my not having anything
> resembling a viable ARM setup, I'm not sure if limiting the callbacks
> to only be invoked if (s->cur_offset == 0) would make sense, just as a
> test ?

I think Marc came to the conclusion that it's safe and therefore made
that optimization for DMA.

The same can be done for PIO.

Stefan

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

[Gabriel L. Somlo]

On Mon, Oct 26, 2015 at 10:48:08AM +0000, Stefan Hajnoczi wrote:
> On Thu, Oct 22, 2015 at 05:22:16PM -0400, Gabriel L. Somlo wrote:
> > I was re-reading the documentation for fw_cfg_add_file_callback(),
> > and noticed that non-dma read operations check for the presence
> > of a callback (and call it if present) for *every* *single* *byte*,
> > even on 64-bit MMIO reads. That's also what the documentation says
> > (in docs/specs/fw_cfg.txt, being moved into fw_cfg.h as per
> >  http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05315.html).
> > 
> > During DMA reads, however, the callback is only checked once before
> > each chunk, effectively once per DMA read operation.
> > 
> > Now, typical callbacks I found throughout the qemu source tend to return
> > immediately except for the first time they're invoked, but I wonder if
> > skipping over all those extra "do I have a callback, if so call it,
> > mostly so it can return without doing anything" per-byte operations
> > account in some significant part for the dramatically faster transfers?
> > 
> > Not sure how I'd test for that -- besides my not having anything
> > resembling a viable ARM setup, I'm not sure if limiting the callbacks
> > to only be invoked if (s->cur_offset == 0) would make sense, just as a
> > test ?
> 
> I think Marc came to the conclusion that it's safe and therefore made
> that optimization for DMA.
> 
> The same can be done for PIO.

OK, so at the risk of over-reaching here, would it make sense to
rewrite the fw_cfg spec to say "If present, a callback will be
executed *once* before each time a blob is read" ?

My hypothesis (which I guess I'm volunteering to verify, unless we
end up rejecting this immediately as a bad idea, for some reason that
I have missed), is that current functionality wouldn't change, given
the way existing callbacks work right now, and that we could run the
callback each time a blob is *selected*, rather than hooking into the
(dma/mmio/pio) read methods.

Thanks,
--Gabriel

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

[Laszlo Ersek]

On 10/26/15 13:49, Gabriel L. Somlo wrote:
> On Mon, Oct 26, 2015 at 10:48:08AM +0000, Stefan Hajnoczi wrote:
>> On Thu, Oct 22, 2015 at 05:22:16PM -0400, Gabriel L. Somlo wrote:
>>> I was re-reading the documentation for fw_cfg_add_file_callback(),
>>> and noticed that non-dma read operations check for the presence
>>> of a callback (and call it if present) for *every* *single* *byte*,
>>> even on 64-bit MMIO reads. That's also what the documentation says
>>> (in docs/specs/fw_cfg.txt, being moved into fw_cfg.h as per
>>>  http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05315.html).
>>>
>>> During DMA reads, however, the callback is only checked once before
>>> each chunk, effectively once per DMA read operation.
>>>
>>> Now, typical callbacks I found throughout the qemu source tend to return
>>> immediately except for the first time they're invoked, but I wonder if
>>> skipping over all those extra "do I have a callback, if so call it,
>>> mostly so it can return without doing anything" per-byte operations
>>> account in some significant part for the dramatically faster transfers?
>>>
>>> Not sure how I'd test for that -- besides my not having anything
>>> resembling a viable ARM setup, I'm not sure if limiting the callbacks
>>> to only be invoked if (s->cur_offset == 0) would make sense, just as a
>>> test ?
>>
>> I think Marc came to the conclusion that it's safe and therefore made
>> that optimization for DMA.
>>
>> The same can be done for PIO.
> 
> OK, so at the risk of over-reaching here, would it make sense to
> rewrite the fw_cfg spec to say "If present, a callback will be
> executed *once* before each time a blob is read" ?
> 
> My hypothesis (which I guess I'm volunteering to verify, unless we
> end up rejecting this immediately as a bad idea, for some reason that
> I have missed), is that current functionality wouldn't change, given
> the way existing callbacks work right now, and that we could run the
> callback each time a blob is *selected*, rather than hooking into the
> (dma/mmio/pio) read methods.

Callback executed on first read only sounds okay to me, callback
executed on selection... hm... don't like it. :)

Thanks
Laszlo

> 
> Thanks,
> --Gabriel
> 

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

[Gabriel L. Somlo]

On Mon, Oct 26, 2015 at 02:38:11PM +0100, Laszlo Ersek wrote:
> On 10/26/15 13:49, Gabriel L. Somlo wrote:
> > On Mon, Oct 26, 2015 at 10:48:08AM +0000, Stefan Hajnoczi wrote:
> >> On Thu, Oct 22, 2015 at 05:22:16PM -0400, Gabriel L. Somlo wrote:
> >>> I was re-reading the documentation for fw_cfg_add_file_callback(),
> >>> and noticed that non-dma read operations check for the presence
> >>> of a callback (and call it if present) for *every* *single* *byte*,
> >>> even on 64-bit MMIO reads. That's also what the documentation says
> >>> (in docs/specs/fw_cfg.txt, being moved into fw_cfg.h as per
> >>>  http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05315.html).
> >>>
> >>> During DMA reads, however, the callback is only checked once before
> >>> each chunk, effectively once per DMA read operation.
> >>>
> >>> Now, typical callbacks I found throughout the qemu source tend to return
> >>> immediately except for the first time they're invoked, but I wonder if
> >>> skipping over all those extra "do I have a callback, if so call it,
> >>> mostly so it can return without doing anything" per-byte operations
> >>> account in some significant part for the dramatically faster transfers?
> >>>
> >>> Not sure how I'd test for that -- besides my not having anything
> >>> resembling a viable ARM setup, I'm not sure if limiting the callbacks
> >>> to only be invoked if (s->cur_offset == 0) would make sense, just as a
> >>> test ?
> >>
> >> I think Marc came to the conclusion that it's safe and therefore made
> >> that optimization for DMA.
> >>
> >> The same can be done for PIO.
> > 
> > OK, so at the risk of over-reaching here, would it make sense to
> > rewrite the fw_cfg spec to say "If present, a callback will be
> > executed *once* before each time a blob is read" ?
> > 
> > My hypothesis (which I guess I'm volunteering to verify, unless we
> > end up rejecting this immediately as a bad idea, for some reason that
> > I have missed), is that current functionality wouldn't change, given
> > the way existing callbacks work right now, and that we could run the
> > callback each time a blob is *selected*, rather than hooking into the
> > (dma/mmio/pio) read methods.
> 
> Callback executed on first read only sounds okay to me, callback
> executed on selection... hm... don't like it. :)

I figured there's different code paths for the different read methods,
so instead of checking for (and calling) the callback in each of them,
(and additionally looking at whether the current read offset is 0 if
we're to only call it on first read only), I could maybe factor it out
a bit further. Since the only reason you'd want select something is to
then read from it, that sort-of made sense to me, at the time... :)

I don't have strong feelings about it, though... :)

Thanks,
--Gabriel

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

[Gerd Hoffmann]

  Hi,

> > My hypothesis (which I guess I'm volunteering to verify, unless we
> > end up rejecting this immediately as a bad idea, for some reason that
> > I have missed), is that current functionality wouldn't change, given
> > the way existing callbacks work right now, and that we could run the
> > callback each time a blob is *selected*, rather than hooking into the
> > (dma/mmio/pio) read methods.
> 
> Callback executed on first read only sounds okay to me, callback
> executed on selection... hm... don't like it. :)

Care to explain why?

I think callback on selection would be better.  Interface is more clear
then, I don't like read having different behavior depending on hidden
state (current offset).  And in practice selection and read will always
be called together, so there shouldn't be a difference in practice ...

cheers,
  Gerd

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

[Laszlo Ersek]

On 10/27/15 12:11, Gerd Hoffmann wrote:
>   Hi,
> 
>>> My hypothesis (which I guess I'm volunteering to verify, unless we
>>> end up rejecting this immediately as a bad idea, for some reason that
>>> I have missed), is that current functionality wouldn't change, given
>>> the way existing callbacks work right now, and that we could run the
>>> callback each time a blob is *selected*, rather than hooking into the
>>> (dma/mmio/pio) read methods.
>>
>> Callback executed on first read only sounds okay to me, callback
>> executed on selection... hm... don't like it. :)
> 
> Care to explain why?
> 
> I think callback on selection would be better.  Interface is more clear
> then, I don't like read having different behavior depending on hidden
> state (current offset).

> And in practice selection and read will always
> be called together,

This is what I think you cannot guarantee on the host side, without
auditing all guest code. The behavior of callbacks has been specified
under fw_cfg_add_file_callback(), in docs/specs/fw_cfg.txt, and guest
code is allowed to work off that.

> so there shouldn't be a difference in practice ...

I guess I have no choice but to audit all QemuFwCfgSelectItem calls in
edk2...

Right, here's what I've had in the back of my mind: see the
DetectSmbiosVersion() function in
"OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c". It selects
the key that belongs to the "etc/smbios/smbios-anchor" fw_cfg file, but
the switch statement right after it can jump to the "default" label, and
under that label *nothing* is read from fw_cfg.

This is valid guest code according to the current specs. Its behavior
would change (however obscurely) if there was a callback on the
"etc/smbios/smbios-anchor" file, and the callback was executed on
selection, not read.

... This one instance wouldn't be particularly hard to patch in edk2,
but in general our specs are useless if we don't stick to them.

Thanks
Laszlo

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

[Gabriel L. Somlo]

On Tue, Oct 27, 2015 at 01:43:39PM +0100, Laszlo Ersek wrote:
> On 10/27/15 12:11, Gerd Hoffmann wrote:
> >   Hi,
> > 
> >>> My hypothesis (which I guess I'm volunteering to verify, unless we
> >>> end up rejecting this immediately as a bad idea, for some reason that
> >>> I have missed), is that current functionality wouldn't change, given
> >>> the way existing callbacks work right now, and that we could run the
> >>> callback each time a blob is *selected*, rather than hooking into the
> >>> (dma/mmio/pio) read methods.
> >>
> >> Callback executed on first read only sounds okay to me, callback
> >> executed on selection... hm... don't like it. :)
> > 
> > Care to explain why?
> > 
> > I think callback on selection would be better.  Interface is more clear
> > then, I don't like read having different behavior depending on hidden
> > state (current offset).
> 
> > And in practice selection and read will always
> > be called together,
> 
> This is what I think you cannot guarantee on the host side, without
> auditing all guest code. The behavior of callbacks has been specified
> under fw_cfg_add_file_callback(), in docs/specs/fw_cfg.txt, and guest
> code is allowed to work off that.
> 
> > so there shouldn't be a difference in practice ...
> 
> I guess I have no choice but to audit all QemuFwCfgSelectItem calls in
> edk2...
> 
> Right, here's what I've had in the back of my mind: see the
> DetectSmbiosVersion() function in
> "OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c". It selects
> the key that belongs to the "etc/smbios/smbios-anchor" fw_cfg file, but
> the switch statement right after it can jump to the "default" label, and
> under that label *nothing* is read from fw_cfg.
> 
> This is valid guest code according to the current specs. Its behavior
> would change (however obscurely) if there was a callback on the
> "etc/smbios/smbios-anchor" file, and the callback was executed on
> selection, not read.

OK, but none of "etc/smbios/*" blobs actually have a callback at all.

After some grepping, the only places inserting callback-equipped blobs
are:

	- hw/i386/acpi-build.c (via rom_add_blob or directly by calling
				fw_cfg_add_file_callback)

		- files added are
			"/etc/acpi/rsdp",
			"/etc/acpi/tables", and
			"/etc/table-loader".

		- all using the same callback: acpi_build_update()

	- hw/arm/virt-acpi-build.c (via rom_add_blob only)

		- same three files as on i386

		- all using the same callback: virt_acpi_build_update()

Both of these callbacks are a one-shot deal, i.e. they both
contain something along these lines:

    /* No state to update or already patched? Nothing to do. */
    if (!build_state || build_state->patched) {
        return;
    }
    build_state->patched = 1;

So, they do something *once* before the first byte is ever read, and
never again after that.

> ... This one instance wouldn't be particularly hard to patch in edk2,
> but in general our specs are useless if we don't stick to them.

OK, so I was proposing to amend the specs (now, while externally
visible behavior won't be affected), and *THEN* stick to them :)

We're already giving up on the letter of the specs (right now, they
say once per byte read, but DMA is only doing once per chunk transfered,
which in practice amounts to once each time a whole blob is read).

Of course, if you (or anyone else with much more clue than me) expect
a future scenario where we'd need the opportunity to run the callback
more than once *before* reading anything from the blob, or (as is the
case with smbios) wish to select (but not read from) blobs, and the
blobs will be callback-enabled, but running the callback will be a bad
thing when no read follows, then by all means, let's stick with
hooking into each individual read operation.

As it is right now, the ammended spec I'm proposing (if set, callback
runs on select, whether a read follows or not) is a NOP w.r.t. currently
visible behavior. It allows simplifying things, at the price of removing
theoretical future flexibility (but also unnecessary slowness as well).

Thanks for helping me think this through !

--Gabriel

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

[Laszlo Ersek]

On 10/28/15 02:12, Gabriel L. Somlo wrote:
> On Tue, Oct 27, 2015 at 01:43:39PM +0100, Laszlo Ersek wrote:
>> On 10/27/15 12:11, Gerd Hoffmann wrote:
>>>   Hi,
>>>
>>>>> My hypothesis (which I guess I'm volunteering to verify, unless we
>>>>> end up rejecting this immediately as a bad idea, for some reason that
>>>>> I have missed), is that current functionality wouldn't change, given
>>>>> the way existing callbacks work right now, and that we could run the
>>>>> callback each time a blob is *selected*, rather than hooking into the
>>>>> (dma/mmio/pio) read methods.
>>>>
>>>> Callback executed on first read only sounds okay to me, callback
>>>> executed on selection... hm... don't like it. :)
>>>
>>> Care to explain why?
>>>
>>> I think callback on selection would be better.  Interface is more clear
>>> then, I don't like read having different behavior depending on hidden
>>> state (current offset).
>>
>>> And in practice selection and read will always
>>> be called together,
>>
>> This is what I think you cannot guarantee on the host side, without
>> auditing all guest code. The behavior of callbacks has been specified
>> under fw_cfg_add_file_callback(), in docs/specs/fw_cfg.txt, and guest
>> code is allowed to work off that.
>>
>>> so there shouldn't be a difference in practice ...
>>
>> I guess I have no choice but to audit all QemuFwCfgSelectItem calls in
>> edk2...
>>
>> Right, here's what I've had in the back of my mind: see the
>> DetectSmbiosVersion() function in
>> "OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c". It selects
>> the key that belongs to the "etc/smbios/smbios-anchor" fw_cfg file, but
>> the switch statement right after it can jump to the "default" label, and
>> under that label *nothing* is read from fw_cfg.
>>
>> This is valid guest code according to the current specs. Its behavior
>> would change (however obscurely) if there was a callback on the
>> "etc/smbios/smbios-anchor" file, and the callback was executed on
>> selection, not read.
> 
> OK, but none of "etc/smbios/*" blobs actually have a callback at all.
> 
> After some grepping, the only places inserting callback-equipped blobs
> are:
> 
> 	- hw/i386/acpi-build.c (via rom_add_blob or directly by calling
> 				fw_cfg_add_file_callback)
> 
> 		- files added are
> 			"/etc/acpi/rsdp",
> 			"/etc/acpi/tables", and
> 			"/etc/table-loader".
> 
> 		- all using the same callback: acpi_build_update()
> 
> 	- hw/arm/virt-acpi-build.c (via rom_add_blob only)
> 
> 		- same three files as on i386
> 
> 		- all using the same callback: virt_acpi_build_update()
> 
> Both of these callbacks are a one-shot deal, i.e. they both
> contain something along these lines:
> 
>     /* No state to update or already patched? Nothing to do. */
>     if (!build_state || build_state->patched) {
>         return;
>     }
>     build_state->patched = 1;
> 
> So, they do something *once* before the first byte is ever read, and
> never again after that.
> 
>> ... This one instance wouldn't be particularly hard to patch in edk2,
>> but in general our specs are useless if we don't stick to them.
> 
> OK, so I was proposing to amend the specs (now, while externally
> visible behavior won't be affected), and *THEN* stick to them :)
> 
> We're already giving up on the letter of the specs (right now, they
> say once per byte read, but DMA is only doing once per chunk transfered,
> which in practice amounts to once each time a whole blob is read).

Good point.

> Of course, if you (or anyone else with much more clue than me) expect
> a future scenario where we'd need the opportunity to run the callback
> more than once *before* reading anything from the blob, or (as is the
> case with smbios) wish to select (but not read from) blobs, and the
> blobs will be callback-enabled, but running the callback will be a bad
> thing when no read follows, then by all means, let's stick with
> hooking into each individual read operation.
> 
> As it is right now, the ammended spec I'm proposing (if set, callback
> runs on select, whether a read follows or not) is a NOP w.r.t. currently
> visible behavior. It allows simplifying things, at the price of removing
> theoretical future flexibility (but also unnecessary slowness as well).

Okay. Please go ahead with the change, as far as I'm concerned.

Thanks
Laszlo

> Thanks for helping me think this through !
> 
> --Gabriel
>