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

[Laszlo Ersek <lersek@redhat.com>]

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

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

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

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

> +
> +Kick a transfer: Write select, target address and transfer length
> +registers (order doesn't matter).  Then flip read bit in the control
> +register to '1'.  Also make sure the error bit is cleared.
> +
> +Check result:  Read control register:
> +   error bit set     ->  something went wrong.
> +   all bits cleared  ->  transfer finished successfully.
> +   otherwise         ->  transfer still in progress (doesn't happen
> +                         today due to implementation not being async,
> +                         but may in the future).
> +
> +Target address goes up and transfer length goes down as the transfer
> +happens, so after a successfull

successful[]

> transfer the length register is zero
> +and the address register points right after the memory block written.
> +
> +If a partial transfer happened before an error occured the address and
> +length registers indicate how much data has been transfered
> +successfully.
> +
>  = Host-side API =
>  
>  The following functions are available to the QEMU programmer for adding
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 4846892..374660c 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -612,7 +612,7 @@ static void create_fw_cfg(const VirtBoardInfo *vbi)
>      hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
>      char *nodename;
>  
> -    fw_cfg_init_mem_wide(base + 8, base, 8);
> +    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
>  
>      nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>      qemu_fdt_add_subnode(vbi->fdt, nodename);
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 88481b7..5bcd0e0 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -23,6 +23,7 @@
>   */
>  #include "hw/hw.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/dma.h"
>  #include "hw/isa/isa.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/sysbus.h"
> @@ -42,6 +43,18 @@
>  #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
>  #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
>  
> +/* fw_cfg dma registers */
> +#define FW_CFG_DMA_ADDR_LO        0
> +#define FW_CFG_DMA_ADDR_HI        4
> +#define FW_CFG_DMA_LENGTH         8
> +#define FW_CFG_DMA_CONTROL       12
> +#define FW_CFG_DMA_SIZE          16
> +
> +/* FW_CFG_DMA_CONTROL bits */
> +#define FW_CFG_DMA_CTL_ERROR   0x01
> +#define FW_CFG_DMA_CTL_READ    0x02
> +#define FW_CFG_DMA_CTL_MASK    0x03
> +
>  typedef struct FWCfgEntry {
>      uint32_t len;
>      uint8_t *data;
> @@ -59,6 +72,12 @@ struct FWCfgState {
>      uint16_t cur_entry;
>      uint32_t cur_offset;
>      Notifier machine_ready;
> +
> +    bool       dma_enabled;
> +    AddressSpace *dma_as;
> +    dma_addr_t dma_addr;
> +    uint32_t   dma_len;
> +    uint32_t   dma_ctl;
>  };
>  
>  struct FWCfgIoState {
> @@ -75,7 +94,10 @@ struct FWCfgMemState {
>      FWCfgState parent_obj;
>      /*< public >*/
>  
> -    MemoryRegion ctl_iomem, data_iomem;
> +    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
> +#if 0
> +    hwaddr ctl_addr, data_addr, dma_addr;
> +#endif

What's the goal of this?

>      uint32_t data_width;
>      MemoryRegionOps wide_data_ops;
>  };
> @@ -294,6 +316,88 @@ static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
>      } while (i);
>  }
>  
> +static void fw_cfg_dma_transfer(FWCfgState *s)
> +{
> +    dma_addr_t len;
> +    uint8_t *ptr;
> +    uint32_t i;
> +
> +    if (s->dma_ctl & FW_CFG_DMA_CTL_ERROR) {
> +        return;
> +    }
> +    if (!(s->dma_ctl & FW_CFG_DMA_CTL_READ)) {
> +        return;
> +    }
> +
> +    while (s->dma_len > 0) {
> +        len = s->dma_len;
> +        ptr = dma_memory_map(s->dma_as, s->dma_addr, &len,
> +                             DMA_DIRECTION_FROM_DEVICE);
> +        if (!ptr || !len) {
> +            s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
> +            return;
> +        }
> +
> +        for (i = 0; i < len; i++) {
> +            ptr[i] = fw_cfg_read(s);
> +        }
> +
> +        s->dma_addr += i;
> +        s->dma_len  -= i;
> +        dma_memory_unmap(s->dma_as, ptr, len,
> +                         DMA_DIRECTION_FROM_DEVICE, i);
> +    }
> +    s->dma_ctl = 0;
> +}

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

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

Thanks!
Laszlo

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