Use DMADirection type for dma_bdrv_io

Submitted by David Gibson on Feb. 20, 2012, 3:01 a.m.

Details

Message ID 1329706897-30476-1-git-send-email-david@gibson.dropbear.id.au
State New
Headers show

Commit Message

David Gibson Feb. 20, 2012, 3:01 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.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 dma-helpers.c  |   20 ++++++++++++--------
 dma.h          |    2 +-
 hw/ide/core.c  |    3 ++-
 hw/ide/macio.c |    3 ++-
 4 files changed, 17 insertions(+), 11 deletions(-)

Comments

Alexander Graf Feb. 20, 2012, 10:50 a.m.
On 20.02.2012, at 04:01, 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.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> dma-helpers.c  |   20 ++++++++++++--------
> dma.h          |    2 +-
> hw/ide/core.c  |    3 ++-
> hw/ide/macio.c |    3 ++-
> 4 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index f08cdb5..d4af367 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);

Was the trace wrong before or is it now? I don't see its definition changed anywhere.

Alex
David Gibson Feb. 21, 2012, 1:14 a.m.
On Mon, Feb 20, 2012 at 11:50:40AM +0100, Alexander Graf wrote:
> 
> On 20.02.2012, at 04:01, 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.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > dma-helpers.c  |   20 ++++++++++++--------
> > dma.h          |    2 +-
> > hw/ide/core.c  |    3 ++-
> > hw/ide/macio.c |    3 ++-
> > 4 files changed, 17 insertions(+), 11 deletions(-)
> > 
> > diff --git a/dma-helpers.c b/dma-helpers.c
> > index f08cdb5..d4af367 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);
> 
> Was the trace wrong before or is it now? I don't see its definition changed anywhere.

So, there's no explicit prototype for the trace function anywhere I
could see.  As best as I can tell, it is autogenerated from the
surrounding function definition, and so needs to change when it does.

But I'm certainly no expert on the tracing code.
Paolo Bonzini Feb. 21, 2012, 8:35 a.m.
On 02/20/2012 11:50 AM, Alexander Graf wrote:
>> >     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);
> Was the trace wrong before or is it now? I don't see its definition changed anywhere.

Not sure what you mean. :)

Paolo
Kevin Wolf Feb. 21, 2012, 9:09 a.m.
Am 21.02.2012 09:35, schrieb Paolo Bonzini:
> On 02/20/2012 11:50 AM, Alexander Graf wrote:
>>>>     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);
>> Was the trace wrong before or is it now? I don't see its definition changed anywhere.
> 
> Not sure what you mean. :)

trace-events:

dma_bdrv_io(void *dbs, void *bs, int64_t sector_num, bool to_dev)
"dbs=%p bs=%p sector_num=%" PRId64 " to_dev=%d"

to_dev is declared bool here, and it should also be renamed to dir (the
unfortunate thing about DMADirection is that it swaps 0 and 1 compared
to bool to_dev... We need to check carefully that all occurrences have
been caught.)

Kevin
David Gibson Feb. 22, 2012, 1:11 a.m.
On Tue, Feb 21, 2012 at 10:09:01AM +0100, Kevin Wolf wrote:
> Am 21.02.2012 09:35, schrieb Paolo Bonzini:
> > On 02/20/2012 11:50 AM, Alexander Graf wrote:
> >>>>     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);
> >> Was the trace wrong before or is it now? I don't see its definition changed anywhere.
> > 
> > Not sure what you mean. :)
> 
> trace-events:
> 
> dma_bdrv_io(void *dbs, void *bs, int64_t sector_num, bool to_dev)
> "dbs=%p bs=%p sector_num=%" PRId64 " to_dev=%d"

Ah, damnit.  Didn't find that file.  I'll resubmit with trace-events
updated too.

> to_dev is declared bool here, and it should also be renamed to dir (the
> unfortunate thing about DMADirection is that it swaps 0 and 1 compared
> to bool to_dev... We need to check carefully that all occurrences have
> been caught.)

Yeah.  And even more unfortunate that afaict there's no way to make
gcc warn on enum<->bool conversions.  Still there are only about 4
callers of dma_bdrv_io() - nearly everything uses the
dma_bdrv_{read,write}() wrappers - so I'm confident I got them all.
Andreas Färber Feb. 22, 2012, 9:54 a.m.
Am 22.02.2012 02:11, schrieb David Gibson:
> On Tue, Feb 21, 2012 at 10:09:01AM +0100, Kevin Wolf wrote:
>> Am 21.02.2012 09:35, schrieb Paolo Bonzini:
>>> On 02/20/2012 11:50 AM, Alexander Graf wrote:
>>>>>>     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);
>>>> Was the trace wrong before or is it now? I don't see its definition changed anywhere.
>>>
>>> Not sure what you mean. :)
>>
>> trace-events:
>>
>> dma_bdrv_io(void *dbs, void *bs, int64_t sector_num, bool to_dev)
>> "dbs=%p bs=%p sector_num=%" PRId64 " to_dev=%d"
> 
> Ah, damnit.  Didn't find that file.  I'll resubmit with trace-events
> updated too.
> 
>> to_dev is declared bool here, and it should also be renamed to dir (the
>> unfortunate thing about DMADirection is that it swaps 0 and 1 compared
>> to bool to_dev... We need to check carefully that all occurrences have
>> been caught.)
> 
> Yeah.  And even more unfortunate that afaict there's no way to make
> gcc warn on enum<->bool conversions.  Still there are only about 4
> callers of dma_bdrv_io() - nearly everything uses the
> dma_bdrv_{read,write}() wrappers - so I'm confident I got them all.

Re all occurrences: megasas might be affected by such a change, too
(patch on the list).

Andreas
Hannes Reinecke Feb. 22, 2012, 9:55 a.m.
On 02/22/2012 10:54 AM, Andreas Färber wrote:
> Am 22.02.2012 02:11, schrieb David Gibson:
>> On Tue, Feb 21, 2012 at 10:09:01AM +0100, Kevin Wolf wrote:
>>> Am 21.02.2012 09:35, schrieb Paolo Bonzini:
>>>> On 02/20/2012 11:50 AM, Alexander Graf wrote:
>>>>>>>     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);
>>>>> Was the trace wrong before or is it now? I don't see its definition changed anywhere.
>>>>
>>>> Not sure what you mean. :)
>>>
>>> trace-events:
>>>
>>> dma_bdrv_io(void *dbs, void *bs, int64_t sector_num, bool to_dev)
>>> "dbs=%p bs=%p sector_num=%" PRId64 " to_dev=%d"
>>
>> Ah, damnit.  Didn't find that file.  I'll resubmit with trace-events
>> updated too.
>>
>>> to_dev is declared bool here, and it should also be renamed to dir (the
>>> unfortunate thing about DMADirection is that it swaps 0 and 1 compared
>>> to bool to_dev... We need to check carefully that all occurrences have
>>> been caught.)
>>
>> Yeah.  And even more unfortunate that afaict there's no way to make
>> gcc warn on enum<->bool conversions.  Still there are only about 4
>> callers of dma_bdrv_io() - nearly everything uses the
>> dma_bdrv_{read,write}() wrappers - so I'm confident I got them all.
> 
> Re all occurrences: megasas might be affected by such a change, too
> (patch on the list).
> 
Nope. megasas is using the scsi interface for reading/writing,
so it should be insulated against this kind of change.

Cheers,

Hannes

Patch hide | download patch | download mbox

diff --git a/dma-helpers.c b/dma-helpers.c
index f08cdb5..d4af367 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);
 
     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,12 +197,14 @@  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 a13209d..0847b42 100644
--- a/dma.h
+++ b/dma.h
@@ -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 56b219b..e173117 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 abbc41b..edcf885 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;