| Message ID | 1476031419-6805-2-git-send-email-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote: > The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not > necessarily the case for all platforms. Use this as the default alignment for > all current callers. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > @@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret) > return; > } > > - if (dbs->iov.size & ~BDRV_SECTOR_MASK) { > - qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK); > + if (dbs->iov.size & (dbs->align - 1)) { > + qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & (dbs->align - 1)); Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size, dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)? Semantically it is the same, but the macros make it obvious what the bit-twiddling is doing. Unless you think that needs a tweak, Reviewed-by: Eric Blake <eblake@redhat.com>
On 10/10/16 17:34, Eric Blake wrote: > On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote: >> The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not >> necessarily the case for all platforms. Use this as the default alignment for >> all current callers. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- > >> @@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret) >> return; >> } >> >> - if (dbs->iov.size & ~BDRV_SECTOR_MASK) { >> - qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK); >> + if (dbs->iov.size & (dbs->align - 1)) { >> + qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & (dbs->align - 1)); > > Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size, > dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)? > Semantically it is the same, but the macros make it obvious what the > bit-twiddling is doing. > > Unless you think that needs a tweak, > Reviewed-by: Eric Blake <eblake@redhat.com> I can't say I feel too strongly about it since there are plenty of other examples of this style in the codebase, so I'm happy to go with whatever John/Paolo are most happy with. ATB, Mark.
On 10/10/2016 03:23 PM, Mark Cave-Ayland wrote: > On 10/10/16 17:34, Eric Blake wrote: > >> On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote: >>> The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not >>> necessarily the case for all platforms. Use this as the default alignment for >>> all current callers. >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> --- >> >>> @@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret) >>> return; >>> } >>> >>> - if (dbs->iov.size & ~BDRV_SECTOR_MASK) { >>> - qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK); >>> + if (dbs->iov.size & (dbs->align - 1)) { >>> + qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & (dbs->align - 1)); >> >> Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size, >> dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)? >> Semantically it is the same, but the macros make it obvious what the >> bit-twiddling is doing. >> >> Unless you think that needs a tweak, >> Reviewed-by: Eric Blake <eblake@redhat.com> > > I can't say I feel too strongly about it since there are plenty of other > examples of this style in the codebase, so I'm happy to go with whatever > John/Paolo are most happy with. > > > ATB, > > Mark. > I can't pretend I am consistent, but when in doubt use the macro. Not worth a respin IMO, but I think this falls out of my jurisdiction :) Acked-by: John Snow <jsnow@redhat.com>
Am 11.10.2016 um 17:47 hat John Snow geschrieben: > On 10/10/2016 03:23 PM, Mark Cave-Ayland wrote: > >On 10/10/16 17:34, Eric Blake wrote: > > > >>On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote: > >>>The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not > >>>necessarily the case for all platforms. Use this as the default alignment for > >>>all current callers. > >>> > >>>Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > >>>--- > >> > >>>@@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret) > >>> return; > >>> } > >>> > >>>- if (dbs->iov.size & ~BDRV_SECTOR_MASK) { > >>>- qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK); > >>>+ if (dbs->iov.size & (dbs->align - 1)) { > >>>+ qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & (dbs->align - 1)); > >> > >>Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size, > >>dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)? > >>Semantically it is the same, but the macros make it obvious what the > >>bit-twiddling is doing. > >> > >>Unless you think that needs a tweak, > >>Reviewed-by: Eric Blake <eblake@redhat.com> > > > >I can't say I feel too strongly about it since there are plenty of other > >examples of this style in the codebase, so I'm happy to go with whatever > >John/Paolo are most happy with. > > > > > >ATB, > > > >Mark. > > > > I can't pretend I am consistent, but when in doubt use the macro. > Not worth a respin IMO, but I think this falls out of my > jurisdiction :) > > Acked-by: John Snow <jsnow@redhat.com> dma-helpers.c is officially unmaintained, and as the other patch is clearly IDE, I think the series should go through your tree. Kevin
On 10/12/2016 06:22 AM, Kevin Wolf wrote: > Am 11.10.2016 um 17:47 hat John Snow geschrieben: >> On 10/10/2016 03:23 PM, Mark Cave-Ayland wrote: >>> On 10/10/16 17:34, Eric Blake wrote: >>> >>>> On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote: >>>>> The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not >>>>> necessarily the case for all platforms. Use this as the default alignment for >>>>> all current callers. >>>>> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>> --- >>>> >>>>> @@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret) >>>>> return; >>>>> } >>>>> >>>>> - if (dbs->iov.size & ~BDRV_SECTOR_MASK) { >>>>> - qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK); >>>>> + if (dbs->iov.size & (dbs->align - 1)) { >>>>> + qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & (dbs->align - 1)); >>>> >>>> Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size, >>>> dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)? >>>> Semantically it is the same, but the macros make it obvious what the >>>> bit-twiddling is doing. >>>> >>>> Unless you think that needs a tweak, >>>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> >>> I can't say I feel too strongly about it since there are plenty of other >>> examples of this style in the codebase, so I'm happy to go with whatever >>> John/Paolo are most happy with. >>> >>> >>> ATB, >>> >>> Mark. >>> >> >> I can't pretend I am consistent, but when in doubt use the macro. >> Not worth a respin IMO, but I think this falls out of my >> jurisdiction :) >> >> Acked-by: John Snow <jsnow@redhat.com> > > dma-helpers.c is officially unmaintained, and as the other patch is > clearly IDE, I think the series should go through your tree. > > Kevin > Oh! I was under the impression that Paolo had the dma-helpers. My mistake. I will test and stage this. --js
diff --git a/dma-helpers.c b/dma-helpers.c index 9defc10..3caa2ab 100644 --- a/dma-helpers.c +++ b/dma-helpers.c @@ -73,6 +73,7 @@ typedef struct { AioContext *ctx; BlockAIOCB *acb; QEMUSGList *sg; + uint32_t align; uint64_t offset; DMADirection dir; int sg_cur_index; @@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret) return; } - if (dbs->iov.size & ~BDRV_SECTOR_MASK) { - qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK); + if (dbs->iov.size & (dbs->align - 1)) { + qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & (dbs->align - 1)); } dbs->acb = dbs->io_func(dbs->offset, &dbs->iov, @@ -199,7 +200,7 @@ static const AIOCBInfo dma_aiocb_info = { }; BlockAIOCB *dma_blk_io(AioContext *ctx, - QEMUSGList *sg, uint64_t offset, + QEMUSGList *sg, uint64_t offset, uint32_t align, DMAIOFunc *io_func, void *io_func_opaque, BlockCompletionFunc *cb, void *opaque, DMADirection dir) @@ -212,6 +213,7 @@ BlockAIOCB *dma_blk_io(AioContext *ctx, dbs->sg = sg; dbs->ctx = ctx; dbs->offset = offset; + dbs->align = align; dbs->sg_cur_index = 0; dbs->sg_cur_byte = 0; dbs->dir = dir; @@ -234,11 +236,11 @@ BlockAIOCB *dma_blk_read_io_func(int64_t offset, QEMUIOVector *iov, } BlockAIOCB *dma_blk_read(BlockBackend *blk, - QEMUSGList *sg, uint64_t offset, + QEMUSGList *sg, uint64_t offset, uint32_t align, void (*cb)(void *opaque, int ret), void *opaque) { - return dma_blk_io(blk_get_aio_context(blk), - sg, offset, dma_blk_read_io_func, blk, cb, opaque, + return dma_blk_io(blk_get_aio_context(blk), sg, offset, align, + dma_blk_read_io_func, blk, cb, opaque, DMA_DIRECTION_FROM_DEVICE); } @@ -252,11 +254,11 @@ BlockAIOCB *dma_blk_write_io_func(int64_t offset, QEMUIOVector *iov, } BlockAIOCB *dma_blk_write(BlockBackend *blk, - QEMUSGList *sg, uint64_t offset, + QEMUSGList *sg, uint64_t offset, uint32_t align, void (*cb)(void *opaque, int ret), void *opaque) { - return dma_blk_io(blk_get_aio_context(blk), - sg, offset, dma_blk_write_io_func, blk, cb, opaque, + return dma_blk_io(blk_get_aio_context(blk), sg, offset, align, + dma_blk_write_io_func, blk, cb, opaque, DMA_DIRECTION_TO_DEVICE); } diff --git a/hw/block/nvme.c b/hw/block/nvme.c index cef3bb4..b380142 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -258,8 +258,10 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd, req->has_sg = true; dma_acct_start(n->conf.blk, &req->acct, &req->qsg, acct); req->aiocb = is_write ? - dma_blk_write(n->conf.blk, &req->qsg, data_offset, nvme_rw_cb, req) : - dma_blk_read(n->conf.blk, &req->qsg, data_offset, nvme_rw_cb, req); + dma_blk_write(n->conf.blk, &req->qsg, data_offset, BDRV_SECTOR_SIZE, + nvme_rw_cb, req) : + dma_blk_read(n->conf.blk, &req->qsg, data_offset, BDRV_SECTOR_SIZE, + nvme_rw_cb, req); return NVME_NO_COMPLETE; } diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 63ead21..3c19bda 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1009,6 +1009,7 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs) &ncq_tfs->sglist, BLOCK_ACCT_READ); ncq_tfs->aiocb = dma_blk_read(ide_state->blk, &ncq_tfs->sglist, ncq_tfs->lba << BDRV_SECTOR_BITS, + BDRV_SECTOR_SIZE, ncq_cb, ncq_tfs); break; case WRITE_FPDMA_QUEUED: @@ -1022,6 +1023,7 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs) &ncq_tfs->sglist, BLOCK_ACCT_WRITE); ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist, ncq_tfs->lba << BDRV_SECTOR_BITS, + BDRV_SECTOR_SIZE, ncq_cb, ncq_tfs); break; default: diff --git a/hw/ide/core.c b/hw/ide/core.c index 7291677..43709e5 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -882,15 +882,15 @@ static void ide_dma_cb(void *opaque, int ret) switch (s->dma_cmd) { case IDE_DMA_READ: s->bus->dma->aiocb = dma_blk_read(s->blk, &s->sg, offset, - ide_dma_cb, s); + BDRV_SECTOR_SIZE, ide_dma_cb, s); break; case IDE_DMA_WRITE: s->bus->dma->aiocb = dma_blk_write(s->blk, &s->sg, offset, - ide_dma_cb, s); + BDRV_SECTOR_SIZE, ide_dma_cb, s); break; case IDE_DMA_TRIM: s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk), - &s->sg, offset, + &s->sg, offset, BDRV_SECTOR_SIZE, ide_issue_trim, s->blk, ide_dma_cb, s, DMA_DIRECTION_TO_DEVICE); break; diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 88beaf4..a963191 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -341,6 +341,7 @@ static void scsi_do_read(SCSIDiskReq *r, int ret) r->req.resid -= r->req.sg->size; r->req.aiocb = dma_blk_io(blk_get_aio_context(s->qdev.conf.blk), r->req.sg, r->sector << BDRV_SECTOR_BITS, + BDRV_SECTOR_SIZE, sdc->dma_readv, r, scsi_dma_complete, r, DMA_DIRECTION_FROM_DEVICE); } else { @@ -539,6 +540,7 @@ static void scsi_write_data(SCSIRequest *req) r->req.resid -= r->req.sg->size; r->req.aiocb = dma_blk_io(blk_get_aio_context(s->qdev.conf.blk), r->req.sg, r->sector << BDRV_SECTOR_BITS, + BDRV_SECTOR_SIZE, sdc->dma_writev, r, scsi_dma_complete, r, DMA_DIRECTION_TO_DEVICE); } else { diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h index 34c8eaf..c228c66 100644 --- a/include/sysemu/dma.h +++ b/include/sysemu/dma.h @@ -199,14 +199,14 @@ typedef BlockAIOCB *DMAIOFunc(int64_t offset, QEMUIOVector *iov, void *opaque); BlockAIOCB *dma_blk_io(AioContext *ctx, - QEMUSGList *sg, uint64_t offset, + QEMUSGList *sg, uint64_t offset, uint32_t align, DMAIOFunc *io_func, void *io_func_opaque, BlockCompletionFunc *cb, void *opaque, DMADirection dir); BlockAIOCB *dma_blk_read(BlockBackend *blk, - QEMUSGList *sg, uint64_t offset, + QEMUSGList *sg, uint64_t offset, uint32_t align, BlockCompletionFunc *cb, void *opaque); BlockAIOCB *dma_blk_write(BlockBackend *blk, - QEMUSGList *sg, uint64_t offset, + QEMUSGList *sg, uint64_t offset, uint32_t align, BlockCompletionFunc *cb, void *opaque); uint64_t dma_buf_read(uint8_t *ptr, int32_t len, QEMUSGList *sg); uint64_t dma_buf_write(uint8_t *ptr, int32_t len, QEMUSGList *sg);
The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not necessarily the case for all platforms. Use this as the default alignment for all current callers. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- dma-helpers.c | 20 +++++++++++--------- hw/block/nvme.c | 6 ++++-- hw/ide/ahci.c | 2 ++ hw/ide/core.c | 6 +++--- hw/scsi/scsi-disk.c | 2 ++ include/sysemu/dma.h | 6 +++--- 6 files changed, 25 insertions(+), 17 deletions(-)