Re: [Qemu-devel] [PATCH v5 00/11] fw_cfg, bootorder, and UEFI+'-kernel' on arm/virt

[Gerd Hoffmann <kraxel@redhat.com>]

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

  Hi,

Attached is a patch drafting a dma interface for fw_cfg, lousily based
on the ideas I've outlined a few days ago on this list.  Applies on top
of this patch series.

Host side only, unfinished, untested.  Mainly sending this out as
notification that I've looked into this, to avoid duplicating work.
I'll go disappear into xmas vacation in a few hours and will not
continue with this until next year.

My plan is to coordinate in January with Laszlo how to go forward.  If
someone feels bored during the holidays feel free to grab it and run
with it.

cheers,
  Gerd


[-- Attachment #2: Type: text/x-patch, Size: 9050 bytes --]

>From 679b076610ada4229a735caf01dd1390ef336788 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu, 18 Dec 2014 17:03:52 +0100
Subject: [PATCH] [wip] fw_cfg dma interface

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.

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 or write 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.

Todo list:

 * figure which address space to use.
 * wind up in boards.
 * write guest support.
 * test the whole thing.

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) and fw_cfg_write
   (silently discard data on error) behavior.
 * Use memcpy instead of calling fw_cfg_read in a loop.

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 33ffd0f..75ce403 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,19 @@
 #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_READ    0x01
+#define FW_CFG_DMA_CTL_WRITE   0x02
+#define FW_CFG_DMA_CTL_ERROR   0x04
+#define FW_CFG_DMA_CTL_MASK    0x07
+
 typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
@@ -60,6 +74,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 {
@@ -76,8 +96,8 @@ struct FWCfgMemState {
     FWCfgState parent_obj;
     /*< public >*/
 
-    MemoryRegion ctl_iomem, data_iomem;
-    hwaddr ctl_addr, data_addr;
+    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
+    hwaddr ctl_addr, data_addr, dma_addr;
     uint32_t data_width;
     MemoryRegionOps wide_data_ops;
 };
@@ -335,6 +355,102 @@ static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
     }
 }
 
+static void fw_cfg_dma_transfer(FWCfgState *s)
+{
+    DMADirection direction;
+    dma_addr_t len;
+    uint8_t *ptr;
+    uint32_t i;
+
+    if ((s->dma_ctl & FW_CFG_DMA_CTL_READ) &&
+        (s->dma_ctl & FW_CFG_DMA_CTL_WRITE)) {
+        s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
+        return;
+    }
+
+    if (s->dma_ctl & FW_CFG_DMA_CTL_ERROR) {
+        return;
+    } else if (s->dma_ctl & FW_CFG_DMA_CTL_READ) {
+        direction = DMA_DIRECTION_FROM_DEVICE;
+    } else if (s->dma_ctl & FW_CFG_DMA_CTL_WRITE) {
+        direction = DMA_DIRECTION_TO_DEVICE;
+    } else {
+        return;
+    }
+
+    while (s->dma_len > 0) {
+        len = s->dma_len;
+        ptr = dma_memory_map(s->dma_as, s->dma_addr, &len, direction);
+        if (!ptr || !len) {
+            s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
+            return;
+        }
+
+        if (direction == DMA_DIRECTION_FROM_DEVICE) {
+            for (i = 0; i < len; i++) {
+                ptr[i] = fw_cfg_read(s);
+            }
+        } else {
+            for (i = 0; i < len; i++) {
+                fw_cfg_write(s, ptr[i]);
+            }
+        }
+
+        s->dma_addr += i;
+        s->dma_len  -= i;
+        dma_memory_unmap(s->dma_as, ptr, len, direction, 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)
 {
@@ -402,6 +518,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);
@@ -442,6 +568,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,
@@ -451,6 +594,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 */
+        }
     }
 };
 
@@ -744,6 +895,7 @@ static Property fw_cfg_mem_properties[] = {
     DEFINE_PROP_UINT64("ctl_addr", FWCfgMemState, ctl_addr, -1),
     DEFINE_PROP_UINT64("data_addr", FWCfgMemState, data_addr, -1),
     DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
+    DEFINE_PROP_UINT64("dma_addr", FWCfgMemState, dma_addr, -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -774,6 +926,17 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
                           "fwcfg.data", data_ops->valid.max_access_size);
     sysbus_init_mmio(sbd, &s->data_iomem);
     sysbus_mmio_map(sbd, 1, s->data_addr);
+
+    if (s->dma_addr != -1) {
+#if 0 /* FIXME */
+        FW_CFG(s)->dma_as = /* Hmm, which AddressSpace ??? */;
+#endif
+        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);
+        sysbus_mmio_map(sbd, 1, s->dma_addr);
+        FW_CFG(s)->dma_enabled = true;
+    }
 }
 
 static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
-- 
1.8.3.1