Re: [Qemu-devel] [PATCH 2/7] dma-helpers: change BlockBackend to opaque value in DMAIOFunc

[Fam Zheng <famz@redhat.com>]

On Mon, 05/23 14:54, Paolo Bonzini wrote:
> Callers of dma_blk_io have no way to pass extra data to the DMAIOFunc,
> because the original callback and opaque are gone by the time DMAIOFunc
> is called.  On the other hand, the BlockBackend is usually derived
> from those extra data that you could pass to the DMAIOFunc (in the
> next patch, that would be the SCSIRequest).
> 
> So change DMAIOFunc's prototype, decoupling it from blk_aio_readv
> and blk_aio_writev's.  The new prototype loses the BlockBackend
> and gains an extra opaque value which, in the case of dma_blk_readv
> and dma_blk_writev, is of course used for the BlockBackend.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  dma-helpers.c        | 48 +++++++++++++++++++++++++++++++++++-------------
>  hw/ide/core.c        | 14 ++++++++------
>  hw/ide/internal.h    |  6 +++---
>  hw/ide/macio.c       |  2 +-
>  include/sysemu/dma.h | 12 ++++++------
>  5 files changed, 53 insertions(+), 29 deletions(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index af07aab..92c5d55 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -70,7 +70,7 @@ void qemu_sglist_destroy(QEMUSGList *qsg)
>  
>  typedef struct {
>      BlockAIOCB common;
> -    BlockBackend *blk;
> +    AioContext *ctx;
>      BlockAIOCB *acb;
>      QEMUSGList *sg;
>      uint64_t offset;
> @@ -80,6 +80,7 @@ typedef struct {
>      QEMUIOVector iov;
>      QEMUBH *bh;
>      DMAIOFunc *io_func;
> +    void *io_func_opaque;
>  } DMAAIOCB;
>  
>  static void dma_blk_cb(void *opaque, int ret);
> @@ -154,8 +155,7 @@ static void dma_blk_cb(void *opaque, int ret)
>  
>      if (dbs->iov.size == 0) {
>          trace_dma_map_wait(dbs);
> -        dbs->bh = aio_bh_new(blk_get_aio_context(dbs->blk),
> -                             reschedule_dma, dbs);
> +        dbs->bh = aio_bh_new(dbs->ctx, reschedule_dma, dbs);
>          cpu_register_map_client(dbs->bh);
>          return;
>      }
> @@ -164,8 +164,8 @@ static void dma_blk_cb(void *opaque, int ret)
>          qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
>      }
>  
> -    dbs->acb = dbs->io_func(dbs->blk, dbs->offset, &dbs->iov, 0,
> -                            dma_blk_cb, dbs);
> +    dbs->acb = dbs->io_func(dbs->offset, &dbs->iov,
> +                            dma_blk_cb, dbs, dbs->io_func_opaque);
>      assert(dbs->acb);
>  }
>  
> @@ -191,23 +191,25 @@ static const AIOCBInfo dma_aiocb_info = {
>      .cancel_async       = dma_aio_cancel,
>  };
>  
> -BlockAIOCB *dma_blk_io(
> -    BlockBackend *blk, QEMUSGList *sg, uint64_t offset,
> -    DMAIOFunc *io_func, BlockCompletionFunc *cb,
> +BlockAIOCB *dma_blk_io(AioContext *ctx,
> +    QEMUSGList *sg, uint64_t opaque,

As pointed out by Mark, s/opaque/offset/

> +    DMAIOFunc *io_func, void *io_func_opaque,
> +    BlockCompletionFunc *cb,
>      void *opaque, DMADirection dir)
>  {
> -    DMAAIOCB *dbs = blk_aio_get(&dma_aiocb_info, blk, cb, opaque);
> +    DMAAIOCB *dbs = qemu_aio_get(&dma_aiocb_info, NULL, cb, opaque);
>  
> -    trace_dma_blk_io(dbs, blk, offset, (dir == DMA_DIRECTION_TO_DEVICE));
> +    trace_dma_blk_io(dbs, io_func_opaque, offset, (dir == DMA_DIRECTION_TO_DEVICE));
>  
>      dbs->acb = NULL;
> -    dbs->blk = blk;
>      dbs->sg = sg;
> +    dbs->ctx = ctx;
>      dbs->offset = offset;
>      dbs->sg_cur_index = 0;
>      dbs->sg_cur_byte = 0;
>      dbs->dir = dir;
>      dbs->io_func = io_func;
> +    dbs->io_func_opaque = io_func_opaque;
>      dbs->bh = NULL;
>      qemu_iovec_init(&dbs->iov, sg->nsg);
>      dma_blk_cb(dbs, 0);
> @@ -215,19 +217,39 @@ BlockAIOCB *dma_blk_io(
>  }
>  
>  
> +static
> +BlockAIOCB *dma_blk_read_io_func(int64_t offset, QEMUIOVector *iov,
> +                                 BlockCompletionFunc *cb, void *cb_opaque,
> +                                 void *opaque)
> +{
> +    BlockBackend *blk = opaque;
> +    return blk_aio_preadv(blk, offset, iov, 0, cb, cb_opaque);
> +}
> +
>  BlockAIOCB *dma_blk_read(BlockBackend *blk,
>                           QEMUSGList *sg, uint64_t offset,
>                           void (*cb)(void *opaque, int ret), void *opaque)
>  {
> -    return dma_blk_io(blk, sg, offset, blk_aio_preadv, cb, opaque,
> +    return dma_blk_io(blk_get_aio_context(blk),
> +                      sg, offset, dma_blk_read_io_func, blk, cb, opaque,
>                        DMA_DIRECTION_FROM_DEVICE);
>  }
>  
> +static
> +BlockAIOCB *dma_blk_write_io_func(int64_t offset, QEMUIOVector *iov,
> +                                  BlockCompletionFunc *cb, void *cb_opaque,
> +                                  void *opaque)
> +{
> +    BlockBackend *blk = opaque;
> +    return blk_aio_pwritev(blk, offset, iov, 0, cb, cb_opaque);
> +}
> +
>  BlockAIOCB *dma_blk_write(BlockBackend *blk,
>                            QEMUSGList *sg, uint64_t offset,
>                            void (*cb)(void *opaque, int ret), void *opaque)
>  {
> -    return dma_blk_io(blk, sg, offset, blk_aio_pwritev, cb, opaque,
> +    return dma_blk_io(blk_get_aio_context(blk),
> +                      sg, offset, dma_blk_write_io_func, blk, cb, opaque,
>                        DMA_DIRECTION_TO_DEVICE);
>  }
>  
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 7326189..029f6b9 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -441,13 +441,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>      }
>  }
>  
> -BlockAIOCB *ide_issue_trim(BlockBackend *blk,
> -        int64_t offset, QEMUIOVector *qiov, BdrvRequestFlags flags,
> -        BlockCompletionFunc *cb, void *opaque)
> +BlockAIOCB *ide_issue_trim(
> +        int64_t offset, QEMUIOVector *qiov,
> +        BlockCompletionFunc *cb, void *cb_opaque, void *opaque)
>  {
> +    BlockBackend *blk = opaque;
>      TrimAIOCB *iocb;
>  
> -    iocb = blk_aio_get(&trim_aiocb_info, blk, cb, opaque);
> +    iocb = blk_aio_get(&trim_aiocb_info, blk, cb, cb_opaque);
>      iocb->blk = blk;
>      iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
>      iocb->ret = 0;
> @@ -871,8 +872,9 @@ static void ide_dma_cb(void *opaque, int ret)
>                                             ide_dma_cb, s);
>          break;
>      case IDE_DMA_TRIM:
> -        s->bus->dma->aiocb = dma_blk_io(s->blk, &s->sg, offset,
> -                                        ide_issue_trim, ide_dma_cb, s,
> +        s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk),
> +                                        &s->sg, offset,
> +                                        ide_issue_trim, s->blk, ide_dma_cb, s,
>                                          DMA_DIRECTION_TO_DEVICE);
>          break;
>      default:
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index ceb9e59..773928a 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -613,9 +613,9 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
>                          EndTransferFunc *end_transfer_func);
>  void ide_transfer_stop(IDEState *s);
>  void ide_set_inactive(IDEState *s, bool more);
> -BlockAIOCB *ide_issue_trim(BlockBackend *blk,
> -        int64_t offset, QEMUIOVector *qiov, BdrvRequestFlags flags,
> -        BlockCompletionFunc *cb, void *opaque);
> +BlockAIOCB *ide_issue_trim(
> +        int64_t offset, QEMUIOVector *qiov,
> +        BlockCompletionFunc *cb, void *cb_opaque, void *opaque);
>  BlockAIOCB *ide_buffered_readv(IDEState *s, int64_t sector_num,
>                                 QEMUIOVector *iov, int nb_sectors,
>                                 BlockCompletionFunc *cb, void *opaque);
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index d7d9c0f..42ad68a 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -230,7 +230,7 @@ static void pmac_dma_trim(BlockBackend *blk,
>      s->io_buffer_index += io->len;
>      io->len = 0;
>  
> -    s->bus->dma->aiocb = ide_issue_trim(blk, offset, &io->iov, 0, cb, io);
> +    s->bus->dma->aiocb = ide_issue_trim(offset, &io->iov, cb, io, blk);
>  }
>  
>  static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index f0e0c30..34c8eaf 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -194,14 +194,14 @@ void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len);
>  void qemu_sglist_destroy(QEMUSGList *qsg);
>  #endif
>  
> -typedef BlockAIOCB *DMAIOFunc(BlockBackend *blk, int64_t offset,
> -                              QEMUIOVector *iov, BdrvRequestFlags flags,
> -                              BlockCompletionFunc *cb, void *opaque);

Wasn't flags parameter added for a reason in d4f510eb3f? Would it be useful for
things like offloading FUA?

Fam

> +typedef BlockAIOCB *DMAIOFunc(int64_t offset, QEMUIOVector *iov,
> +                              BlockCompletionFunc *cb, void *cb_opaque,
> +                              void *opaque);
>  
> -BlockAIOCB *dma_blk_io(BlockBackend *blk,
> +BlockAIOCB *dma_blk_io(AioContext *ctx,
>                         QEMUSGList *sg, uint64_t offset,
> -                       DMAIOFunc *io_func, BlockCompletionFunc *cb,
> -                       void *opaque, DMADirection dir);
> +                       DMAIOFunc *io_func, void *io_func_opaque,
> +                       BlockCompletionFunc *cb, void *opaque, DMADirection dir);
>  BlockAIOCB *dma_blk_read(BlockBackend *blk,
>                           QEMUSGList *sg, uint64_t offset,
>                           BlockCompletionFunc *cb, void *opaque);
> -- 
> 2.5.5
> 
> 
>