[v2,3/5] Implement fw_cfg DMA interface

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         | 261 +++++++++++++++++++++++++++++++++++++++++++---
 include/hw/nvram/fw_cfg.h |  15 ++-
 3 files changed, 260 insertions(+), 18 deletions(-)

On Mon, Aug 31, 2015 at 11:10:15AM +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         | 261 +++++++++++++++++++++++++++++++++++++++++++---
>  include/hw/nvram/fw_cfg.h |  15 ++-
>  3 files changed, 260 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d5a8417..b88c104 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -620,7 +620,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..7e5ba96 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,8 @@
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
>  
> -#define FW_CFG_SIZE 2
> +#define FW_CFG_IO_SIZE 12 /* Address aligned to 4 bytes */
> +#define FW_CFG_CTL_SIZE 2
>  #define FW_CFG_NAME "fw_cfg"
>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>  
> @@ -42,6 +44,15 @@
>  #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
> +
>  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 {
> @@ -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;
>  };
> @@ -294,6 +309,142 @@ 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;
> +    void *addr;
> +    FWCfgDmaAccess dma;
> +    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> +    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
> +
> +    len = sizeof(dma);
> +    addr = dma_memory_map(s->dma_as, s->dma_addr, &len,
> +                                DMA_DIRECTION_FROM_DEVICE);
> +
> +    s->dma_addr = 0;
> +
> +    if (!addr || !len) {
> +        return;
> +    }
> +
> +    dma.address = be64_to_cpu(*(uint64_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, address)));
> +    dma.length = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, length)));
> +    dma.control = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, control)));

I am not that familiar with QEMU, but shouldn't that be
DMA_DIRECTION_TO_DEVICE?

It looks like other drivers use dma_memory_read() which would simplify
this code:

    dma_memory_read(s->dma_as, s->dma_addr, &dma, sizeof(dma));
    dma.address = be64_to_cpu(dma.address);
    dma.length = be32_to_cpu(dma.length);
    dma.control = be32_to_cpu(dma.control);

> +    if (dma.control & FW_CFG_DMA_CTL_ERROR) {
> +        goto out;
> +    }
> +
> +    if (!(dma.control & (FW_CFG_DMA_CTL_READ | FW_CFG_DMA_CTL_SKIP))) {
> +        goto out;
> +    }
> +
> +    while (dma.length > 0) {
> +        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 (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memset(ptr, 0, len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);
> +            }
> +
> +        } else {
> +            if (dma.length <= e->len) {
> +                len = dma.length;
> +            } else {
> +                len = e->len;
> +            }
> +
> +            if (e->read_callback) {
> +                e->read_callback(e->callback_opaque, s->cur_offset);
> +            }
> +
> +            /* If the access is not a read access, it will be a skip access,
> +             * tested before.
> +             */
> +            if (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memcpy(ptr, &e->data[s->cur_offset], len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);
> +            }
> +
> +            s->cur_offset += len;
> +
> +        }
> +
> +        dma.address += len;
> +        dma.length  -= len;
> +        dma.control = 0;
> +
> +        *(uint64_t *)(addr + offsetof(FWCfgDmaAccess, address)) =
> +                                                cpu_to_be64(dma.address);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, length)) =
> +                                                cpu_to_be32(dma.length);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, control)) =
> +                                                cpu_to_be32(dma.control);
> +    }

I don't think it makes sense for this update to be performed within
the loop.

As I mentioned in another email, I think just updating control would
be sufficient.  Looks like include/sysemu/dma.h defines a stl_be_dma()
macro for performing a single 32bit dma write.

Thanks,
-Kevin

On 31 August 2015 at 10:10, 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>

> @@ -294,6 +309,142 @@ 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;
> +    void *addr;
> +    FWCfgDmaAccess dma;
> +    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> +    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
> +
> +    len = sizeof(dma);
> +    addr = dma_memory_map(s->dma_as, s->dma_addr, &len,
> +                                DMA_DIRECTION_FROM_DEVICE);
> +
> +    s->dma_addr = 0;
> +
> +    if (!addr || !len) {
> +        return;
> +    }
> +
> +    dma.address = be64_to_cpu(*(uint64_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, address)));
> +    dma.length = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, length)));
> +    dma.control = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, control)));

There are only three fields in this struct, so rather than
using dma_memory_map/unmap and manual byteswapping, how about:

 dma.control = ldl_be_dma(s->dma_as, s->dma_addr);
 dma.length = ldl_be_dma(s->dma_as, s->dma_addr + 4);
 dma.address = ldq_be_dma(s->dma_as, s->dma_addr + 8);

Kevin's suggestion to use dma_memory_read() would be OK as well,
if you really want to check for the return value from the memory
transaction. But you probably don't care because if the guest has
pointed the dma address register into nowhere then it deserves
whatever it gets as long as we don't fall over; and we need to
not fall over whatever the values of control/length/address are.
In any case, please don't use dma_memory_map/unmap.

There are st*_ versions for writing back updated values.

> +
> +    if (dma.control & FW_CFG_DMA_CTL_ERROR) {
> +        goto out;
> +    }
> +
> +    if (!(dma.control & (FW_CFG_DMA_CTL_READ | FW_CFG_DMA_CTL_SKIP))) {
> +        goto out;
> +    }
> +
> +    while (dma.length > 0) {
> +        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 (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memset(ptr, 0, len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);

dma_memory_write() would be better than map-zero-unmap I think,
though you would need to have a handy buffer of zeroes to pass
to that function, so maybe not.

> +            }
> +
> +        } else {
> +            if (dma.length <= e->len) {
> +                len = dma.length;
> +            } else {
> +                len = e->len;
> +            }
> +
> +            if (e->read_callback) {
> +                e->read_callback(e->callback_opaque, s->cur_offset);
> +            }
> +
> +            /* If the access is not a read access, it will be a skip access,
> +             * tested before.
> +             */
> +            if (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memcpy(ptr, &e->data[s->cur_offset], len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);

...and dma_memory_write() is definitely better than
map-memcpy-unmap.

> +            }
> +
> +            s->cur_offset += len;
> +
> +        }
> +
> +        dma.address += len;
> +        dma.length  -= len;
> +        dma.control = 0;
> +
> +        *(uint64_t *)(addr + offsetof(FWCfgDmaAccess, address)) =
> +                                                cpu_to_be64(dma.address);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, length)) =
> +                                                cpu_to_be32(dma.length);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, control)) =
> +                                                cpu_to_be32(dma.control);
> +    }
> +
> +    trace_fw_cfg_read(s, 0);
> +
> +out:
> +    dma_memory_unmap(s->dma_as, addr, sizeof(dma),
> +                                DMA_DIRECTION_FROM_DEVICE, sizeof(dma));
> +    return;
> +
> +}
> +
> +static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
> +                                 uint64_t value, unsigned size)
> +{
> +    FWCfgState *s = opaque;
> +
> +    if (size == 4) {
> +        if (addr == 0) {
> +            /* FWCfgDmaAccess high address */
> +            s->dma_addr = (0xFFFFFFFF & value) << 32;

The mask here is unnecessary, since you're shifting by 32 bits.

> +        } 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)
>  {
> @@ -321,20 +472,35 @@ static uint64_t fw_cfg_comb_read(void *opaque, hwaddr addr,
>  static void fw_cfg_comb_write(void *opaque, hwaddr addr,
>                                uint64_t value, unsigned size)
>  {
> -    switch (size) {
> +    FWCfgState *s;
> +
> +    switch (addr) {
> +    case 0:
> +        fw_cfg_select(opaque, (uint16_t)value);
> +        break;
>      case 1:
>          fw_cfg_write(opaque, (uint8_t)value);
>          break;
> -    case 2:
> -        fw_cfg_select(opaque, (uint16_t)value);
> +    case 4:
> +        /* FWCfgDmaAccess low address */
> +        s = opaque;
> +        s->dma_addr |= value;
> +        fw_cfg_dma_transfer(s);
>          break;
> +    case 8:
> +        /* FWCfgDmaAccess high address */
> +        s = opaque;
> +        s->dma_addr = (0xFFFFFFFF & value) << 32;
>      }
>  }
>
>  static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> -    return (size == 1) || (is_write && size == 2);
> +    FWCfgState *s = opaque;
> +
> +    return (size == 1) || (is_write && size == 2) ||
> +            (s->dma_enabled && is_write && addr >= 4);

This doesn't look right, since it will allow 1 and 2 byte
writes to the dma_address port, which doesn't seem like
a great idea.

Maybe it would be easier to make the dma port its own
memory region (and then init and sysbus_add_io() it in
fw_cfg_io_realize()) ? Then you wouldn't need to mess with
the already confusing logic for handling combined config/data
ports (which have to be handled together because of the weird
"1 byte accesses are data and 2 byte accesses are config"
semantics).

thanks
-- PMM