Re: [Qemu-devel] [PATCH 08/13] iommu: Introduce IOMMU emulation infrastructure

[Anthony Liguori <anthony@codemonkey.ws>]

On 05/14/2012 08:42 PM, David Gibson wrote:
> On Mon, May 14, 2012 at 07:49:16PM -0500, Anthony Liguori wrote:
> [snip]
>>> +void iommu_wait_for_invalidated_maps(DMAContext *dma,
>>> +                                     dma_addr_t addr, dma_addr_t len)
>>> +{
>>> +    DMAMemoryMap *map;
>>> +    DMAInvalidationState is;
>>> +
>>> +    is.count = 0;
>>> +    qemu_cond_init(&is.cond);
>>> +
>>> +    QLIST_FOREACH(map,&dma->memory_maps, list) {
>>> +        if (ranges_overlap(addr, len, map->addr, map->len)) {
>>> +            is.count++;
>>> +            map->invalidate =&is;
>>> +        }
>>> +    }
>>> +
>>> +    if (is.count) {
>>> +        qemu_cond_wait(&is.cond,&qemu_global_mutex);
>>> +    }
>>> +    assert(is.count == 0);
>>> +}
>>
>> I don't get what's going on here but I don't think it can possibly
>> be right. What is the purpose of this function?
>
> So.  This is a function to be used by individual iommu
> implementations.  When IOMMU mappings are updated on real hardware,
> there may be some lag in th effect, particularly for in-flight DMAs,
> due to shadow TLBs or other things.  But generally, there will be some
> way to synchronize the IOMMU that once completed will ensure that no
> further DMA access to the old translations may occur.  For the sPAPR
> TCE MMU, this actually happens after every PUT_TCE hcall.
>
> In our software implementation this is a problem if existing drivers
> have done a dma_memory_map() and haven't yet done a
> dma_memory_unmap(): they will have a real pointer to the translated
> memory which can't be intercepted.  However, memory maps are supposed
> to be transient, so this helper function invalidates memory maps based
> on an IOVA address range, and blocks until they expire.  This function
> would be called from CPU thread context, the dma_memory_unmap() would
> come from the IO thread (in the only existing case from AIO completion
> callbacks in the block IO code).
>
> This gives the IOMMU implementation a way of blocking the CPU
> initiating a sync operation until it really is safe to assume that no
> further DMA operations may hit the invalidated mappings.  Note that if
> we actually hit the blocking path here, that almost certainly
> indicates the guest has done something wrong, or at least unusual -

So the CPU thread runs in lock-step with the I/O thread.  Dropping the CPU 
thread lock to let the I/O thread run is a dangerous thing to do in a place like 
this.

Also, I think you'd effectively block the CPU until pending DMA operations 
complete?  This could be many, many, milliseconds, no?  That's going to make 
guests very upset.

Regards,

Anthony Liguori

> DMA devices should be stopped before removing their IOMMU mappings
> from under them.  However, one of the points of the IOMMU is the
> ability to be able to forcibly stop DMAs, so we do need to implement
> this behaviour for that case.
>
> With difficulty, I've traced through qemu's difficult-to-follow thread
> synchronization logic and I'm about 75% convinced this works correctly
> with it.
>