From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39855) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SU76c-0004VT-2z for qemu-devel@nongnu.org; Mon, 14 May 2012 22:03:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SU76a-0003Ip-4m for qemu-devel@nongnu.org; Mon, 14 May 2012 22:03:17 -0400 Received: from mail-ob0-f173.google.com ([209.85.214.173]:33974) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SU76Z-0003IN-W2 for qemu-devel@nongnu.org; Mon, 14 May 2012 22:03:16 -0400 Received: by obbwd20 with SMTP id wd20so10096337obb.4 for ; Mon, 14 May 2012 19:03:14 -0700 (PDT) Message-ID: <4FB1B95A.20209@codemonkey.ws> Date: Mon, 14 May 2012 21:03:06 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1336625347-10169-1-git-send-email-benh@kernel.crashing.org> <1336625347-10169-9-git-send-email-benh@kernel.crashing.org> <4FB1A80C.1010103@codemonkey.ws> <20120515014204.GE30229@truffala.fritz.box> In-Reply-To: <20120515014204.GE30229@truffala.fritz.box> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 08/13] iommu: Introduce IOMMU emulation infrastructure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Benjamin Herrenschmidt , qemu-devel@nongnu.org, Eduard - Gabriel Munteanu , Richard Henderson , "Michael S. Tsirkin" 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. >