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
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/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
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/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