Re: [Qemu-devel] [PATCH 1/9] dma-helpers: Expose the sg mapping logic

[Paolo Bonzini <pbonzini@redhat.com>]

On 19/02/2016 12:50, Alex Pyrgiotis wrote:
> QEMU/Hardware space:
> 5. The SCSI controller code will create a QEMUSGList that points to
>    the memory regions of the SCSI request. This QEMUSGList will also
>    include the MMIO regions.
> 6. The QEMU device implementation, e.g. scsi-block, chooses to use
>    the dma_* interface.
> 7. The dma_blk_read/write() code will ultimately attempt to map all the
>    memory regions pointed by the QEMUSGList in order to create a
>    QEMUIOVector.
> 8. At some point during the mapping loop, the code will encounter an
>    MMIO region. Since reading and writing from/to an MMIO region
>    requires  special handling, e.g., we need to call
>    MemoryRegion->ops->write(), we cannot include it in our read/write
>    system call to the host kernel.
> 9. This leads to a partial read/write and the mapping loop will resume
>    once the partial read/write() has finished.
> 
> Are we in the same page so far?

Yes.

> Are the above OK? If so, I have some questions:
> 
> a) Is an MMIO region one of the reasons why we can't map an sg?

Yes, the only one pretty much.

> b) At which point will the relevant ops->write() method for the MMIO
>    region be called when we have to DMA into the region?? Is it handled
>    implicitly in dma_memory_map()?

It's in address_space_unmap:

    if (is_write) {
        address_space_write(as, bounce.addr, MEMTXATTRS_UNSPECIFIED,
                            bounce.buffer, access_len);
    }

Likewise, address_space_map does the ops->read call through
address_space_read.

> c) I'm not quite sure about the logic of the "nothing mapped" section.
>    Correct me if I'm wrong, but what I think it does is that it
>    registers a callback (reschedule_dma) once some sort of mapping has
>    completed. What kind of mapping is this? Is there anything more to
>    it?

Once something (presumably a concurrent user of dma-helpers.c) calls
address_space_unmap to free the mapping (the bounce.buffer in the above
address_space_write call), reschedule_dma is called.

>> However, it is not possible to do the same for ioctls.  This is actually
>> the reason why no one has ever tried to make scsi-generic do anything
>> but bounce-buffering. I think that your code breaks horribly in this
>> case, and I don't see a way to fix it, except for reverting to bounce
>> buffering.
>>
>> This would require major changes in your patches, and I'm not sure
>> whether they are worth it for the single use case of tape devices...
> 
> Well, I wouldn't narrow it down to tape devices. The scsi-generic
> interface is the universal interface for all SCSI devices and the only
> interface that is fully passthrough.

Sure, but what's the advantage of a fully passthrough interface over
scsi-block?

> So this patch would effectively
> boost the baseline performance of SCSI devices. I think it's worth a try.

I think the first step is understanding what to do about the weird "&
~BDRV_SECTOR_MASK" case, then.

Paolo