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

[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?

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