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

[Mark Cave-Ayland]

On 25/05/16 09:45, Paolo Bonzini wrote:

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

Great - as long as you're aware of this I'll wait for the next version
of the patchset to come out, and then at least I can re-run the macio
installation test to ensure that there are no further corruption issues
with unaligned DMA accesses.

I'd like to add my thanks to everyone who has worked hard on allowing
byte-based accesses in the I/O layer since on my local test case it
makes a significant improvement to the macio emulation. With my basic
macio conversion applied on top of this patchset, the installation time
for a complete Darwin PPC install drops from 25 mins to about 9 mins on
my laptop which is a clear 2.5x speed increase over the existing code.


ATB,

Mark.