Patchwork Use DMADirection type for dma_bdrv_io

login
register
mail settings
Submitter David Gibson
Date Feb. 20, 2012, 3:01 a.m.
Message ID <1329706897-30476-1-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/142104/
State New
Headers show

Comments

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(-)
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

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;