[Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers
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
---
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(-)
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)
Re: [Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers
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
> ---
> @@ -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
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers
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
>> ---
>
>> @@ -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
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.
Re: [Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers
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
---
@@ -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
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
Re: [Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers
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
> >>>---
> >>
> >>>@@ -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
> >
> >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
dma-helpers.c is officially unmaintained, and as the other patch is
clearly IDE, I think the series should go through your tree.
Kevin
Re: [Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers
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
---
@@ -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
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
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
