Re: [Qemu-devel] [RFC 1/1] pci-dma-api-v2

[Anthony Liguori <anthony@codemonkey.ws>]

Avi Kivity wrote:
> Anthony Liguori wrote:
>>
>> I think you need something more sophisticated that can split up a 
>> scatter/gather list into a set of partial bounce buffers and partial 
>> direct copies.  Just checking the vector once is a bit hackish.
>>
>
> That code path would never be used or tested.  As it is, bouncing will 
> only be invoked by dma to or from mmio, and I expect most guests never 
> to invoke it.  Why would we complicate the code even more?

I'm not sure it's strictly required but I think it would be a 
simplificaction.

>>
>> What should be fixed?  Are these emulated functions wrong?
>>
>> There's a lack of symmetry here.  We should have a bdrv_readv and 
>> bdrv_aio_readv.  bdrv_read and bdrv_aio_read should disappear.  We 
>> can maintain wrappers that create a compatible interface for older 
>> code but just adding a new API is wrong.
>>
>
> It was understood a real aio readv/writev was being worked on, so the 
> emulation could be a temporary step.

Yes, but that's orthogonal to what I'm saying here.  I'm saying that 
instead of adding an optional ->aio_readv() member, we should eliminate 
the ->aio_read() member and replace it with ->aio_readv().  There are 
only three or four places in the code that implement aio so it's not a 
big change.  It avoids introducing a new API too.

I also think we should have complimentary synchronous vector functions 
although I'd like to see the synchronous API disappear completely.

>>> +BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t 
>>> sector_num,
>>> +                 struct iovec *iov, int iovcnt, size_t len,
>>> +                 BlockDriverCompletionFunc *cb, void *opaque)
>>> +{
>>>   
>>
>> struct iovec does not exist on Windows.  I also don't think you've 
>> got the abstraction right.  Reading and writing may require actions 
>> to happen.  I don't think you can just introduce something as simple 
>> as an iovec here.  I think we need something more active.
>>
>
> Can you elaborate?  Actions happen in the completion callback.  This 
> is just an straightforward extension of the block API, I don't think a 
> dma api should materially change the block api.

If we're not going to try and fold in the IOMMU/PCI BUS API at this 
pass, then this is less important.  But to implement a proper PCI bus 
API, I think there has to be function pointers associated with each 
element of the scatter/gather list that control how data is copied in 
and out of each element.

>>
>> I think you missed the mark here.  This API needs to go through the 
>> PCIBus and be pluggable at that level.  There can be a default 
>> implementation but it may differ for each PCI bus.
>>
>
> I think this can serve as the default implementation.  Perhaps when a 
> concrete user of pluggable dma emerges we can understand the 
> requirements better.

I think if we drop the pci_ prefixes and just treat it as a basic DMA 
API as Blue Swirl suggested, then this is closer to what we need.

Regards,

Anthony Liguori