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

[Eric Blake]

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org