Re: [PATCH] dma-helpers: Fix iovec alignment

[Michael Tokarev]

11.08.2024 20:47, Michael Tokarev пишет:
> 12.04.2024 18:25, 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.
>
> Hi!
>
> Has this change (proposed for 9.0) been forgotten or is it not needed
> anymore?

Ping #2?

Thanks,

/mjt

> > > Fixes: 99868af3d0a75cf6 ("dma-helpers: explicitly pass alignment into DMA 
> > > helpers")
> > > Signed-off-by: Stefan Fritsch <sf@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 <eblake@redhat.com>


--
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 
ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 
8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt