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

["Marc Marí" <markmb@redhat.com>]

On Thu, 6 Aug 2015 10:47:21 -0400
"Kevin O'Connor" <kevin@koconnor.net> wrote:

> 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í <markmb@redhat.com>
> > ---
> >  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("/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..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).

This is a PIO port (out), not a MMIO access (write). Maybe I'm wrong,
but I don't think it matters to have the port number aligned with the
write size.

Thanks
Marc