Patchwork Use DMADirection type for dma_bdrv_io

login
register
mail settings
Submitter David Gibson
Date March 27, 2012, 2:42 a.m.
Message ID <1332816143-4989-1-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/148853/
State New
Headers show

Comments

David Gibson - March 27, 2012, 2:42 a.m.
Currently dma_bdrv_io() takes a 'to_dev' boolean parameter to
determine the direction of DMA it is emulating.  We already have a
DMADirection enum designed specifically to encode DMA directions.
This patch uses it for dma_bdrv_io() as well.  This involves removing
the DMADirection definition from the #ifdef it was inside, but since that
only existed to protect the definition of dma_addr_t from places where
config.h is not included, there wasn't any reason for it to be there in
the first place.

Cc: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 dma-helpers.c  |   20 ++++++++++++--------
 dma.h          |   12 ++++++------
 hw/ide/core.c  |    3 ++-
 hw/ide/macio.c |    3 ++-
 4 files changed, 22 insertions(+), 16 deletions(-)
Stefan Hajnoczi - March 27, 2012, 7:17 a.m.
On Tue, Mar 27, 2012 at 01:42:23PM +1100, David Gibson wrote:
> Currently dma_bdrv_io() takes a 'to_dev' boolean parameter to
> determine the direction of DMA it is emulating.  We already have a
> DMADirection enum designed specifically to encode DMA directions.
> This patch uses it for dma_bdrv_io() as well.  This involves removing
> the DMADirection definition from the #ifdef it was inside, but since that
> only existed to protect the definition of dma_addr_t from places where
> config.h is not included, there wasn't any reason for it to be there in
> the first place.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  dma-helpers.c  |   20 ++++++++++++--------
>  dma.h          |   12 ++++++------
>  hw/ide/core.c  |    3 ++-
>  hw/ide/macio.c |    3 ++-
>  4 files changed, 22 insertions(+), 16 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Andreas Färber - March 27, 2012, 2:30 p.m.
Am 27.03.2012 04:42, schrieb David Gibson:
> Currently dma_bdrv_io() takes a 'to_dev' boolean parameter to
> determine the direction of DMA it is emulating.  We already have a
> DMADirection enum designed specifically to encode DMA directions.
> This patch uses it for dma_bdrv_io() as well.  This involves removing
> the DMADirection definition from the #ifdef it was inside, but since that
> only existed to protect the definition of dma_addr_t from places where
> config.h is not included, there wasn't any reason for it to be there in
> the first place.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Including for ppc macio in absence of Alex,

Reviewed-by: Andreas Färber <afaerber@suse.de>

(Expecting this to go through IDE/Kevin's tree.)

Andreas

> ---
>  dma-helpers.c  |   20 ++++++++++++--------
>  dma.h          |   12 ++++++------
>  hw/ide/core.c  |    3 ++-
>  hw/ide/macio.c |    3 ++-
>  4 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index c29ea6d..5f19a85 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -42,7 +42,7 @@ typedef struct {
>      BlockDriverAIOCB *acb;
>      QEMUSGList *sg;
>      uint64_t sector_num;
> -    bool to_dev;
> +    DMADirection dir;
>      bool in_cancel;
>      int sg_cur_index;
>      dma_addr_t sg_cur_byte;
> @@ -76,7 +76,8 @@ static void dma_bdrv_unmap(DMAAIOCB *dbs)
>  
>      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->to_dev,
> +                                  dbs->iov.iov[i].iov_len,
> +                                  dbs->dir != DMA_DIRECTION_TO_DEVICE,
>                                    dbs->iov.iov[i].iov_len);
>      }
>      qemu_iovec_reset(&dbs->iov);
> @@ -123,7 +124,8 @@ static void dma_bdrv_cb(void *opaque, int ret)
>      while (dbs->sg_cur_index < dbs->sg->nsg) {
>          cur_addr = dbs->sg->sg[dbs->sg_cur_index].base + dbs->sg_cur_byte;
>          cur_len = dbs->sg->sg[dbs->sg_cur_index].len - dbs->sg_cur_byte;
> -        mem = cpu_physical_memory_map(cur_addr, &cur_len, !dbs->to_dev);
> +        mem = cpu_physical_memory_map(cur_addr, &cur_len,
> +                                      dbs->dir != DMA_DIRECTION_TO_DEVICE);
>          if (!mem)
>              break;
>          qemu_iovec_add(&dbs->iov, mem, cur_len);
> @@ -170,11 +172,11 @@ static AIOPool dma_aio_pool = {
>  BlockDriverAIOCB *dma_bdrv_io(
>      BlockDriverState *bs, QEMUSGList *sg, uint64_t sector_num,
>      DMAIOFunc *io_func, BlockDriverCompletionFunc *cb,
> -    void *opaque, bool to_dev)
> +    void *opaque, DMADirection dir)
>  {
>      DMAAIOCB *dbs = qemu_aio_get(&dma_aio_pool, bs, cb, opaque);
>  
> -    trace_dma_bdrv_io(dbs, bs, sector_num, to_dev);
> +    trace_dma_bdrv_io(dbs, bs, sector_num, (dir == DMA_DIRECTION_TO_DEVICE));
>  
>      dbs->acb = NULL;
>      dbs->bs = bs;
> @@ -182,7 +184,7 @@ BlockDriverAIOCB *dma_bdrv_io(
>      dbs->sector_num = sector_num;
>      dbs->sg_cur_index = 0;
>      dbs->sg_cur_byte = 0;
> -    dbs->to_dev = to_dev;
> +    dbs->dir = dir;
>      dbs->io_func = io_func;
>      dbs->bh = NULL;
>      qemu_iovec_init(&dbs->iov, sg->nsg);
> @@ -195,14 +197,16 @@ BlockDriverAIOCB *dma_bdrv_read(BlockDriverState *bs,
>                                  QEMUSGList *sg, uint64_t sector,
>                                  void (*cb)(void *opaque, int ret), void *opaque)
>  {
> -    return dma_bdrv_io(bs, sg, sector, bdrv_aio_readv, cb, opaque, false);
> +    return dma_bdrv_io(bs, sg, sector, bdrv_aio_readv, cb, opaque,
> +                       DMA_DIRECTION_FROM_DEVICE);
>  }
>  
>  BlockDriverAIOCB *dma_bdrv_write(BlockDriverState *bs,
>                                   QEMUSGList *sg, uint64_t sector,
>                                   void (*cb)(void *opaque, int ret), void *opaque)
>  {
> -    return dma_bdrv_io(bs, sg, sector, bdrv_aio_writev, cb, opaque, true);
> +    return dma_bdrv_io(bs, sg, sector, bdrv_aio_writev, cb, opaque,
> +                       DMA_DIRECTION_TO_DEVICE);
>  }
>  
>  
> diff --git a/dma.h b/dma.h
> index 20e86d2..05ac325 100644
> --- a/dma.h
> +++ b/dma.h
> @@ -17,6 +17,11 @@
>  
>  typedef struct ScatterGatherEntry ScatterGatherEntry;
>  
> +typedef enum {
> +    DMA_DIRECTION_TO_DEVICE = 0,
> +    DMA_DIRECTION_FROM_DEVICE = 1,
> +} DMADirection;
> +
>  struct QEMUSGList {
>      ScatterGatherEntry *sg;
>      int nsg;
> @@ -29,11 +34,6 @@ typedef target_phys_addr_t dma_addr_t;
>  
>  #define DMA_ADDR_FMT TARGET_FMT_plx
>  
> -typedef enum {
> -    DMA_DIRECTION_TO_DEVICE = 0,
> -    DMA_DIRECTION_FROM_DEVICE = 1,
> -} DMADirection;
> -
>  struct ScatterGatherEntry {
>      dma_addr_t base;
>      dma_addr_t len;
> @@ -51,7 +51,7 @@ typedef BlockDriverAIOCB *DMAIOFunc(BlockDriverState *bs, int64_t sector_num,
>  BlockDriverAIOCB *dma_bdrv_io(BlockDriverState *bs,
>                                QEMUSGList *sg, uint64_t sector_num,
>                                DMAIOFunc *io_func, BlockDriverCompletionFunc *cb,
> -                              void *opaque, bool to_dev);
> +                              void *opaque, DMADirection dir);
>  BlockDriverAIOCB *dma_bdrv_read(BlockDriverState *bs,
>                                  QEMUSGList *sg, uint64_t sector,
>                                  BlockDriverCompletionFunc *cb, void *opaque);
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 4d568ac..43da841 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -604,7 +604,8 @@ void ide_dma_cb(void *opaque, int ret)
>          break;
>      case IDE_DMA_TRIM:
>          s->bus->dma->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
> -                                         ide_issue_trim, ide_dma_cb, s, true);
> +                                         ide_issue_trim, ide_dma_cb, s,
> +                                         DMA_DIRECTION_TO_DEVICE);
>          break;
>      }
>      return;
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index a4df244..7b38d9e 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -149,7 +149,8 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>          break;
>      case IDE_DMA_TRIM:
>          m->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
> -                               ide_issue_trim, pmac_ide_transfer_cb, s, true);
> +                               ide_issue_trim, pmac_ide_transfer_cb, s,
> +                               DMA_DIRECTION_TO_DEVICE);
>          break;
>      }
>      return;
Kevin Wolf - March 27, 2012, 2:43 p.m.
Am 27.03.2012 16:30, schrieb Andreas Färber:
> Am 27.03.2012 04:42, schrieb David Gibson:
>> Currently dma_bdrv_io() takes a 'to_dev' boolean parameter to
>> determine the direction of DMA it is emulating.  We already have a
>> DMADirection enum designed specifically to encode DMA directions.
>> This patch uses it for dma_bdrv_io() as well.  This involves removing
>> the DMADirection definition from the #ifdef it was inside, but since that
>> only existed to protect the definition of dma_addr_t from places where
>> config.h is not included, there wasn't any reason for it to be there in
>> the first place.
>>
>> Cc: Kevin Wolf <kwolf@redhat.com>
>>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Including for ppc macio in absence of Alex,
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> 
> (Expecting this to go through IDE/Kevin's tree.)

So this isn't part of another series any more?

Kevin
David Gibson - March 27, 2012, 3:14 p.m.
On Tue, Mar 27, 2012 at 04:43:25PM +0200, Kevin Wolf wrote:
> Am 27.03.2012 16:30, schrieb Andreas Färber:
> > Am 27.03.2012 04:42, schrieb David Gibson:
> >> Currently dma_bdrv_io() takes a 'to_dev' boolean parameter to
> >> determine the direction of DMA it is emulating.  We already have a
> >> DMADirection enum designed specifically to encode DMA directions.
> >> This patch uses it for dma_bdrv_io() as well.  This involves removing
> >> the DMADirection definition from the #ifdef it was inside, but since that
> >> only existed to protect the definition of dma_addr_t from places where
> >> config.h is not included, there wasn't any reason for it to be there in
> >> the first place.
> >>
> >> Cc: Kevin Wolf <kwolf@redhat.com>
> >>
> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > Including for ppc macio in absence of Alex,
> > 
> > Reviewed-by: Andreas Färber <afaerber@suse.de>
> > 
> > (Expecting this to go through IDE/Kevin's tree.)
> 
> So this isn't part of another series any more?

There's still some stuff to work on in that series, but these early
patches from it are ready to go, so I thought I'd try to get them in
now.
Andreas Färber - March 27, 2012, 3:17 p.m.
Am 27.03.2012 16:43, schrieb Kevin Wolf:
> Am 27.03.2012 16:30, schrieb Andreas Färber:
>> Am 27.03.2012 04:42, schrieb David Gibson:
>>> Currently dma_bdrv_io() takes a 'to_dev' boolean parameter to
>>> determine the direction of DMA it is emulating.  We already have a
>>> DMADirection enum designed specifically to encode DMA directions.
>>> This patch uses it for dma_bdrv_io() as well.  This involves removing
>>> the DMADirection definition from the #ifdef it was inside, but since that
>>> only existed to protect the definition of dma_addr_t from places where
>>> config.h is not included, there wasn't any reason for it to be there in
>>> the first place.
>>>
>>> Cc: Kevin Wolf <kwolf@redhat.com>
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>
>> Including for ppc macio in absence of Alex,
>>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>
>> (Expecting this to go through IDE/Kevin's tree.)
> 
> So this isn't part of another series any more?

AFAICS it's a single patch on qemu-devel, split out of one of the
"Various fixes David has piled up" series on Alex' suggestion.

Andreas
Kevin Wolf - March 27, 2012, 3:23 p.m.
Am 27.03.2012 17:17, schrieb Andreas Färber:
> Am 27.03.2012 16:43, schrieb Kevin Wolf:
>> Am 27.03.2012 16:30, schrieb Andreas Färber:
>>> Am 27.03.2012 04:42, schrieb David Gibson:
>>>> Currently dma_bdrv_io() takes a 'to_dev' boolean parameter to
>>>> determine the direction of DMA it is emulating.  We already have a
>>>> DMADirection enum designed specifically to encode DMA directions.
>>>> This patch uses it for dma_bdrv_io() as well.  This involves removing
>>>> the DMADirection definition from the #ifdef it was inside, but since that
>>>> only existed to protect the definition of dma_addr_t from places where
>>>> config.h is not included, there wasn't any reason for it to be there in
>>>> the first place.
>>>>
>>>> Cc: Kevin Wolf <kwolf@redhat.com>
>>>>
>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>
>>> Including for ppc macio in absence of Alex,
>>>
>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>
>>> (Expecting this to go through IDE/Kevin's tree.)
>>
>> So this isn't part of another series any more?
> 
> AFAICS it's a single patch on qemu-devel, split out of one of the
> "Various fixes David has piled up" series on Alex' suggestion.

Okay. I don't have the time to check the details myself, but two
Reviewed-bys are enough, so I just applied it to the block branch.

Kevin
David Gibson - March 28, 2012, 12:42 a.m.
On Tue, Mar 27, 2012 at 05:17:23PM +0200, Andreas Färber wrote:
> Am 27.03.2012 16:43, schrieb Kevin Wolf:
> > Am 27.03.2012 16:30, schrieb Andreas Färber:
> >> Am 27.03.2012 04:42, schrieb David Gibson:
> >>> Currently dma_bdrv_io() takes a 'to_dev' boolean parameter to
> >>> determine the direction of DMA it is emulating.  We already have a
> >>> DMADirection enum designed specifically to encode DMA directions.
> >>> This patch uses it for dma_bdrv_io() as well.  This involves removing
> >>> the DMADirection definition from the #ifdef it was inside, but since that
> >>> only existed to protect the definition of dma_addr_t from places where
> >>> config.h is not included, there wasn't any reason for it to be there in
> >>> the first place.
> >>>
> >>> Cc: Kevin Wolf <kwolf@redhat.com>
> >>>
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>
> >> Including for ppc macio in absence of Alex,
> >>
> >> Reviewed-by: Andreas Färber <afaerber@suse.de>
> >>
> >> (Expecting this to go through IDE/Kevin's tree.)
> > 
> > So this isn't part of another series any more?
> 
> AFAICS it's a single patch on qemu-devel, split out of one of the
> "Various fixes David has piled up" series on Alex' suggestion.

Actually in this case split out from "patches for IOMMU support"
rather than one of the various fixes series.

Patch

diff --git a/dma-helpers.c b/dma-helpers.c
index c29ea6d..5f19a85 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -42,7 +42,7 @@  typedef struct {
     BlockDriverAIOCB *acb;
     QEMUSGList *sg;
     uint64_t sector_num;
-    bool to_dev;
+    DMADirection dir;
     bool in_cancel;
     int sg_cur_index;
     dma_addr_t sg_cur_byte;
@@ -76,7 +76,8 @@  static void dma_bdrv_unmap(DMAAIOCB *dbs)
 
     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->to_dev,
+                                  dbs->iov.iov[i].iov_len,
+                                  dbs->dir != DMA_DIRECTION_TO_DEVICE,
                                   dbs->iov.iov[i].iov_len);
     }
     qemu_iovec_reset(&dbs->iov);
@@ -123,7 +124,8 @@  static void dma_bdrv_cb(void *opaque, int ret)
     while (dbs->sg_cur_index < dbs->sg->nsg) {
         cur_addr = dbs->sg->sg[dbs->sg_cur_index].base + dbs->sg_cur_byte;
         cur_len = dbs->sg->sg[dbs->sg_cur_index].len - dbs->sg_cur_byte;
-        mem = cpu_physical_memory_map(cur_addr, &cur_len, !dbs->to_dev);
+        mem = cpu_physical_memory_map(cur_addr, &cur_len,
+                                      dbs->dir != DMA_DIRECTION_TO_DEVICE);
         if (!mem)
             break;
         qemu_iovec_add(&dbs->iov, mem, cur_len);
@@ -170,11 +172,11 @@  static AIOPool dma_aio_pool = {
 BlockDriverAIOCB *dma_bdrv_io(
     BlockDriverState *bs, QEMUSGList *sg, uint64_t sector_num,
     DMAIOFunc *io_func, BlockDriverCompletionFunc *cb,
-    void *opaque, bool to_dev)
+    void *opaque, DMADirection dir)
 {
     DMAAIOCB *dbs = qemu_aio_get(&dma_aio_pool, bs, cb, opaque);
 
-    trace_dma_bdrv_io(dbs, bs, sector_num, to_dev);
+    trace_dma_bdrv_io(dbs, bs, sector_num, (dir == DMA_DIRECTION_TO_DEVICE));
 
     dbs->acb = NULL;
     dbs->bs = bs;
@@ -182,7 +184,7 @@  BlockDriverAIOCB *dma_bdrv_io(
     dbs->sector_num = sector_num;
     dbs->sg_cur_index = 0;
     dbs->sg_cur_byte = 0;
-    dbs->to_dev = to_dev;
+    dbs->dir = dir;
     dbs->io_func = io_func;
     dbs->bh = NULL;
     qemu_iovec_init(&dbs->iov, sg->nsg);
@@ -195,14 +197,16 @@  BlockDriverAIOCB *dma_bdrv_read(BlockDriverState *bs,
                                 QEMUSGList *sg, uint64_t sector,
                                 void (*cb)(void *opaque, int ret), void *opaque)
 {
-    return dma_bdrv_io(bs, sg, sector, bdrv_aio_readv, cb, opaque, false);
+    return dma_bdrv_io(bs, sg, sector, bdrv_aio_readv, cb, opaque,
+                       DMA_DIRECTION_FROM_DEVICE);
 }
 
 BlockDriverAIOCB *dma_bdrv_write(BlockDriverState *bs,
                                  QEMUSGList *sg, uint64_t sector,
                                  void (*cb)(void *opaque, int ret), void *opaque)
 {
-    return dma_bdrv_io(bs, sg, sector, bdrv_aio_writev, cb, opaque, true);
+    return dma_bdrv_io(bs, sg, sector, bdrv_aio_writev, cb, opaque,
+                       DMA_DIRECTION_TO_DEVICE);
 }
 
 
diff --git a/dma.h b/dma.h
index 20e86d2..05ac325 100644
--- a/dma.h
+++ b/dma.h
@@ -17,6 +17,11 @@ 
 
 typedef struct ScatterGatherEntry ScatterGatherEntry;
 
+typedef enum {
+    DMA_DIRECTION_TO_DEVICE = 0,
+    DMA_DIRECTION_FROM_DEVICE = 1,
+} DMADirection;
+
 struct QEMUSGList {
     ScatterGatherEntry *sg;
     int nsg;
@@ -29,11 +34,6 @@  typedef target_phys_addr_t dma_addr_t;
 
 #define DMA_ADDR_FMT TARGET_FMT_plx
 
-typedef enum {
-    DMA_DIRECTION_TO_DEVICE = 0,
-    DMA_DIRECTION_FROM_DEVICE = 1,
-} DMADirection;
-
 struct ScatterGatherEntry {
     dma_addr_t base;
     dma_addr_t len;
@@ -51,7 +51,7 @@  typedef BlockDriverAIOCB *DMAIOFunc(BlockDriverState *bs, int64_t sector_num,
 BlockDriverAIOCB *dma_bdrv_io(BlockDriverState *bs,
                               QEMUSGList *sg, uint64_t sector_num,
                               DMAIOFunc *io_func, BlockDriverCompletionFunc *cb,
-                              void *opaque, bool to_dev);
+                              void *opaque, DMADirection dir);
 BlockDriverAIOCB *dma_bdrv_read(BlockDriverState *bs,
                                 QEMUSGList *sg, uint64_t sector,
                                 BlockDriverCompletionFunc *cb, void *opaque);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 4d568ac..43da841 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -604,7 +604,8 @@  void ide_dma_cb(void *opaque, int ret)
         break;
     case IDE_DMA_TRIM:
         s->bus->dma->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
-                                         ide_issue_trim, ide_dma_cb, s, true);
+                                         ide_issue_trim, ide_dma_cb, s,
+                                         DMA_DIRECTION_TO_DEVICE);
         break;
     }
     return;
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index a4df244..7b38d9e 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -149,7 +149,8 @@  static void pmac_ide_transfer_cb(void *opaque, int ret)
         break;
     case IDE_DMA_TRIM:
         m->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
-                               ide_issue_trim, pmac_ide_transfer_cb, s, true);
+                               ide_issue_trim, pmac_ide_transfer_cb, s,
+                               DMA_DIRECTION_TO_DEVICE);
         break;
     }
     return;