Re: [Qemu-devel] [PATCH 07/13] iommu: Make sglists and dma_bdrv helpers use new universal DMA helpers

[Kevin Wolf]

Am 09.03.2012 06:01, schrieb David Gibson:
> dma-helpers.c contains a number of helper functions for doing
> scatter/gather DMA, and various block device related DMA.  Currently,
> these directly access guest memory using cpu_physical_memory_*(),
> assuming no IOMMU translation.
> 
> This patch updates this code to use the new universal DMA helper
> functions.  qemu_sglist_init() now takes a DMAContext * to describe
> the DMA address space in which the scatter/gather will take place.
> 
> We minimally update the callers qemu_sglist_init() to pass NULL
> (i.e. no translation, same as current behaviour).  Some of those
> callers should pass something else in some cases to allow proper IOMMU
> translation in future, but that will be fixed in later patches.
> 
> Cc: Kevin Wolf <address@hidden>
> Cc: Michael S. Tsirkin <address@hidden>
> 
> Signed-off-by: David Gibson <address@hidden>
> ---
>  dma-helpers.c  |   26 ++++++++++++++++++--------
>  dma.h          |    3 ++-
>  hw/ide/ahci.c  |    3 ++-
>  hw/ide/macio.c |    4 ++--
>  hw/pci.h       |    2 +-
>  5 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index 5f19a85..9dcfb2c 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -11,12 +11,13 @@
>  #include "block_int.h"
>  #include "trace.h"
>  
> -void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint)
> +void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint, DMAContext *dma)
>  {
>      qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry));
>      qsg->nsg = 0;
>      qsg->nalloc = alloc_hint;
>      qsg->size = 0;
> +    qsg->dma = dma;
>  }
>  
>  void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
> @@ -75,10 +76,9 @@ static void dma_bdrv_unmap(DMAAIOCB *dbs)
>      int i;
>  
>      for (i = 0; i < dbs->iov.niov; ++i) {
> -        cpu_physical_memory_unmap(dbs->iov.iov[i].iov_base,
> -                                  dbs->iov.iov[i].iov_len,
> -                                  dbs->dir != DMA_DIRECTION_TO_DEVICE,
> -                                  dbs->iov.iov[i].iov_len);
> +        dma_memory_unmap(dbs->sg->dma, dbs->iov.iov[i].iov_base,
> +                         dbs->iov.iov[i].iov_len, dbs->dir,
> +                         dbs->iov.iov[i].iov_len);
>      }
>      qemu_iovec_reset(&dbs->iov);
>  }
> @@ -104,10 +104,20 @@ static void dma_complete(DMAAIOCB *dbs, int ret)
>      }
>  }
>  
> +static void dma_bdrv_cancel(void *opaque)
> +{
> +    DMAAIOCB *dbs = opaque;
> +
> +    bdrv_aio_cancel(dbs->acb);
> +    dma_bdrv_unmap(dbs);
> +    qemu_iovec_destroy(&dbs->iov);
> +    qemu_aio_release(dbs);
> +}

I'm lacking the context to know when this is actually called, but it
looks suspicious. Did you consider that bdrv_aio_cancel() can actually
invoke the completion callback?

What's the difference between the existing dma_aio_cancel() and the
function that you need here?

Kevin