Re: [Qemu-devel] [PATCH v2 0/7] dma-helpers, scsi-block: use SG_IO for all I/O on scsi-block

[Paolo Bonzini <pbonzini@redhat.com>]

On 25/05/2016 00:59, Mark Cave-Ayland wrote:
> I eventually traced the corruption down to this section of code in
> dma_blk_cb() which was incorrectly truncating the unaligned iovecs:
> 
> if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
>     qemu_iovec_discard_back(&dbs->iov, dbs->iov.size &
>         ~BDRV_SECTOR_MASK);
> }
> 
> This was introduced in
> http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg02236.html to
> handle non-sector aligned SG lists, although given that this is a
> restriction of one particular implementation (PRDT) I'm not sure whether
> a plain revert is the correct thing to do or whether an alternative
> solution needs to be found.

Yeah, I have a plan for this bit.  It's related to this code I'm adding
in patch 7:

+    /* This is not supported yet.  It can only happen if the guest does
+     * reads and writes that are not aligned to one logical sectors
+     * _and_ cover multiple MemoryRegions.
+     */
+    assert(offset % s->qdev.blocksize == 0);
+    assert(iov->size % s->qdev.blocksize == 0);

The idea behind the "if" is that the I/O code cannot deal with a number
of bytes that is not a multiple of the logical sector size.  These
assertions could be removed by generalizing the "if" to an arbitrary
mask, in this case s->qdev.blocksize - 1.

There are two things that are wrong however in the logic.  First, the
"if" must be moved before the "if (dbs->iov.size & ~BDRV_SECTOR_MASK)",
because the call to qemu_iovec_discard_back can result in a zero-byte
QEMUIOVector.  Second, the sg_cur_* variables must be rewound too.

Thanks,

Paolo