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

[Marc Marí]

On Tue, 21 Jul 2015 21:44:49 +0200
Laszlo Ersek <address@hidden> wrote:

> On 07/21/15 18:03, Marc Marí wrote:
> > From: Gerd Hoffmann <address@hidden>
> > 
> > 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 <address@hidden>
> > ---
> >  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.)
> 

Ok. When the changes get accepted, I'll also update that document in
the kernel tree.

> > +
> > +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("/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..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?

I suppose the comment has no goal. It was just left undeleted, and I
haven't seen it.

> 
> >      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?

Somebody with more knowledge in this architecture should answer this
question.

Thanks
Marc