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í <address@hidden>
> ---
>  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("/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 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