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

[Mark Cave-Ayland]

On 23/05/16 20:36, Mark Cave-Ayland wrote:

> On 23/05/16 13:54, Paolo Bonzini wrote:
> 
>> scsi-block uses the block layer for reads and writes in order to avoid
>> allocating bounce buffers as big as the transferred data.  We know how
>> to split a large transfer to multiple reads and writes, and thus we can
>> use scsi-disk.c's existing code to do I/O in multiple chunks (for non-s/g
>> SCSI hosts) or through the DMA helpers (for s/g SCSI hosts).
>>
>> Unfortunately, this has the side effect of eating the SCSI status except
>> in the very few cases where we can convert an errno code back to a SCSI
>> status.  It puts a big wrench in persistent reservations support in the
>> guest, for example.
>>
>> Luckily, splitting a large transfer into multiple SBC commands is just as
>> easy, and this is what the last patch does.  It takes the original CDB,
>> patches in a modified starting sector and sector count, and executes the
>> SCSI command through blk_aio_ioctl.  It is also easy to pass a QEMUIOVector
>> to SG_IO, so that s/g SCSI hosts keep the performance.
>>
>> This rebases the patches on top of Eric's changes for byte-based
>> BlockBackend access and fixes a few bugs I knew about in the RFC.
>>
>> Patches 1, 5 and 6 are new.
>>
>> Paolo
>>
>> Paolo Bonzini (7):
>>   dma-helpers: change interface to byte-based
>>   dma-helpers: change BlockBackend to opaque value in DMAIOFunc
>>   scsi-disk: introduce a common base class
>>   scsi-disk: introduce dma_readv and dma_writev
>>   scsi-disk: add need_fua_emulation to SCSIDiskClass
>>   scsi-disk: introduce scsi_disk_req_check_error
>>   scsi-block: always use SG_IO
>>
>>  dma-helpers.c        |  54 +++++--
>>  hw/block/nvme.c      |   6 +-
>>  hw/ide/ahci.c        |   6 +-
>>  hw/ide/core.c        |  20 ++-
>>  hw/ide/internal.h    |   6 +-
>>  hw/ide/macio.c       |   2 +-
>>  hw/scsi/scsi-disk.c  | 409 
>> +++++++++++++++++++++++++++++++++++++--------------
>>  include/sysemu/dma.h |  20 +--
>>  trace-events         |   2 +-
>>  9 files changed, 371 insertions(+), 154 deletions(-)
> 
> Hi Paolo,
> 
> I thought I'd give this patchset a spin with a view to seeing whether I
> could switch the macio device back to the now byte-based dma-helpers,
> but came up with a couple of compile errors. Attached is the minor diff
> I applied in order to get a successful compile (apologies for not being
> inline, however I couldn't get my mail client to stop wrapping incorrectly).

After some head-scratching, I've finally managed to install and boot
Darwin PPC using the new byte-based DMA helpers facilitated by this
patch in the macio IDE driver. Just switching over to the new helpers
provided by this patch seemingly allowed the installation to proceed,
but the resulting image was corrupt and refused to boot.

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.


ATB,

Mark.