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

[Kevin O'Connor]

On Thu, Aug 06, 2015 at 01:01:16PM +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.
> 
> For x86 arch, this addon will use one extra port (0x512). For ARM, it will
> use 8 more bytes, following the actual implementation.
> 
> Based on Gerd Hoffman's initial implementation.
> 
> Signed-off-by: Marc Marí <address@hidden>
> ---
>  hw/arm/virt.c             |   2 +-
>  hw/nvram/fw_cfg.c         | 212 
> +++++++++++++++++++++++++++++++++++++++++++---
>  include/hw/nvram/fw_cfg.h |  12 ++-
>  3 files changed, 211 insertions(+), 15 deletions(-)
> 
> 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("/address@hidden" 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..7399008 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,10 +31,13 @@
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
>  
> -#define FW_CFG_SIZE 2
> +#define FW_CFG_SIZE 3
>  #define FW_CFG_NAME "fw_cfg"
>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>  
> +#define FW_CFG_VERSION      1
> +#define FW_CFG_VERSION_DMA  2
> +
>  #define TYPE_FW_CFG     "fw_cfg"
>  #define TYPE_FW_CFG_IO  "fw_cfg_io"
>  #define TYPE_FW_CFG_MEM "fw_cfg_mem"
> @@ -42,6 +46,11 @@
>  #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_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 +68,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 +88,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 +307,108 @@ 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;
> +    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);
> +    dma = dma_memory_map(s->dma_as, s->dma_addr, &len,
> +                                DMA_DIRECTION_FROM_DEVICE);
> +
> +    if (!dma || !len) {
> +        return;
> +    }
> +
> +    if (dma->control & FW_CFG_DMA_CTL_ERROR) {

I think the dma structure would need to be copied to a local instance
of the struct and the fields endianness updated.  (Also, I think the
documentation should state the required endianness of the fields.)
I'm not an expert at this, but if this is not done, I think there
could be problems both with endian and with unaligned memory accesses
in the host.

> +    while (dma->length > 0) {
> +        if (s->cur_entry == FW_CFG_INVALID || !e->data ||
> +                                    s->cur_offset >= e->len) {
> +            len = dma->length;
> +            if (dma->address) {
> +                ptr = dma_memory_map(s->dma_as, dma->address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma->control |= FW_CFG_DMA_CTL_ERROR;

Can you write to "dma->control" if the memory is mapped with
DMA_DIRECTION_FROM_DEVICE?  Also, since the control field update
should be atomic, I think the dma struct should probably always be at
least 4 byte aligned and the documentation should be updated to
reflect that.

[...]
> @@ -321,12 +436,20 @@ 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);
> +        /* FWCfgDmaAccess address */
> +        s = opaque;
> +        s->dma_addr = le64_to_cpu(value);
> +        fw_cfg_dma_transfer(s);
>          break;

There is no ability to perfrom 64bit IO writes on x86 (to the best of
my knowledge).  So, I think this code needs to be able to handle a
32bit write to a high bits address and then store those bits until the
32bit write to the low bits address occurs.  (I'd also recommend that
after every dma transfer the stored high bits are reset to zero so
that the common case of a 32bit address can be performed with a single
32bit write to the low bits field.)

Also, it's very unusual to see 32bit writes to an unaligned IO address
- I think two pad bytes should be added so that the offset for the dma
address is at position 4 (instead of 2).

-Kevin