adding John Snow to CC because he investigated this in 2020. On Fri, 12 Apr 2024, Eric Blake wrote:
> 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). we had some more internal discussions and did some more googling, and it turns out that this is actually part of a known issue: https://lists.nongnu.org/archive/html/qemu-block/2020-07/msg01866.html https://gitlab.com/qemu-project/qemu/-/issues/259 In the above mail to qemu-block, John Snow listed 4 problems in the code but my patch only fixes the first one. Considering that the code may also write data to the wrong place (problem 2), I wonder if an assert in the same place would be better until the underlying issues have been fixed? > > > > 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> > >