| Message ID | 1329706897-30476-1-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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
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.
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
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
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;
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(-)