On Fri, Apr 12, 2024 at 10:06:17AM +0200, Stefan Fritsch wrote: > Commit 99868af3d0 changed the hardcoded constant BDRV_SECTOR_SIZE to a > dynamic field 'align' but introduced a bug. qemu_iovec_discard_back() > is now passed the wanted iov length instead of the actually required > amount that should be removed from the end of the iov. > > The bug can likely only be hit in uncommon configurations, e.g. with > icount enabled or when reading from disk directly to device memory. > > Fixes: 99868af3d0a75cf6 ("dma-helpers: explicitly pass alignment into DMA > helpers") > Signed-off-by: Stefan Fritsch <s...@sfritsch.de> > --- > system/dma-helpers.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-)
Wow, that bug has been latent for a while (2016, v2.8.0). As such, I don't think it's worth holding up 9.0 for, but it is definitely worth cc'ing qemu-stable (done now). > > diff --git a/system/dma-helpers.c b/system/dma-helpers.c > index 9b221cf94e..c9677fd39b 100644 > --- a/system/dma-helpers.c > +++ b/system/dma-helpers.c > @@ -174,8 +174,7 @@ static void dma_blk_cb(void *opaque, int ret) > } > > if (!QEMU_IS_ALIGNED(dbs->iov.size, dbs->align)) { > - qemu_iovec_discard_back(&dbs->iov, > - QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)); > + qemu_iovec_discard_back(&dbs->iov, dbs->iov.size % dbs->align); Before the regression, it was: - if (dbs->iov.size & ~BDRV_SECTOR_MASK) { - qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK); + if (!QEMU_IS_ALIGNED(dbs->iov.size, dbs->align)) { + qemu_iovec_discard_back(&dbs->iov, If dbs->align is always a power of two, we can use '& (dbs->align - 1)' to avoid a hardware division, to match the original code; bug QEMU_IS_ALIGNED does not require a power of two, so your choice of '% dbs->align' seems reasonable. Reviewed-by: Eric Blake <ebl...@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org