Re: [Qemu-devel] [PATCH 07/13] iommu: Make sglists and dma_bdrv helpers use new universal DMA helpers

[Kevin Wolf]

Am 13.03.2012 11:42, schrieb David Gibson:
> On Fri, Mar 09, 2012 at 10:43:35AM +0100, Kevin Wolf wrote:
>> Am 09.03.2012 06:01, schrieb David Gibson:
> [snip]
>>> @@ -104,10 +104,20 @@ static void dma_complete(DMAAIOCB *dbs, int ret)
>>>      }
>>>  }
>>>  
>>> +static void dma_bdrv_cancel(void *opaque)
>>> +{
>>> +    DMAAIOCB *dbs = opaque;
>>> +
>>> +    bdrv_aio_cancel(dbs->acb);
>>> +    dma_bdrv_unmap(dbs);
>>> +    qemu_iovec_destroy(&dbs->iov);
>>> +    qemu_aio_release(dbs);
>>> +}
>>
>> I'm lacking the context to know when this is actually called, but it
>> looks suspicious. Did you consider that bdrv_aio_cancel() can actually
>> invoke the completion callback?
>>
>> What's the difference between the existing dma_aio_cancel() and the
>> function that you need here?
> 
> So, first thing to note is that as I said in another sub-thread, there
> are several approaches we could take for handling invalidation of
> IOMMU mappings while they're in use by drivers, and I'm not sure which
> is best yet.
> 
> Second is that this piece of code comes from Eduard - Gabriel's
> original and I haven't actually understood it as well as I should have
> :).
> 
> So, examining in more detail it looks like dma_aio_cancel() is the
> right thing to do (I don't know if it existed when Eduard - Gabriel
> wrote the initial version).

I'm pretty sure that it's older than these patches.

> The semantics of the callback are that after it's complete, there
> should be no further access to the dma_memory_map()ed memory areas.  I
> haven't yet understood the bdrv stuff sufficiently well to be
> completely sure that's true.

Yes, I think it is. Once bdrv_aio_cancel() returns, the block layer
doesn't do anything with the request any more. The caller (e.g. IDE)
can't use the memory areas either because it doesn't even know them.

Kevin