From patchwork Wed Feb 1 18:27:09 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Will Deacon X-Patchwork-Id: 9550537 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 00BE3604A7 for ; Wed, 1 Feb 2017 18:27:38 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E6A8728451 for ; Wed, 1 Feb 2017 18:27:37 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id DB60128462; Wed, 1 Feb 2017 18:27:37 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00 autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 4E9BD2845B for ; Wed, 1 Feb 2017 18:27:37 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1cYzcz-0006rY-Sy; Wed, 01 Feb 2017 18:27:33 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1cYzcw-0006nY-5f for linux-arm-kernel@lists.infradead.org; Wed, 01 Feb 2017 18:27:31 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 1DB1BAD7; Wed, 1 Feb 2017 10:27:10 -0800 (PST) Received: from edgewater-inn.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id E55BA3F24D; Wed, 1 Feb 2017 10:27:09 -0800 (PST) Received: by edgewater-inn.cambridge.arm.com (Postfix, from userid 1000) id 941AE1AE0BDA; Wed, 1 Feb 2017 18:27:09 +0000 (GMT) Date: Wed, 1 Feb 2017 18:27:09 +0000 From: Will Deacon To: "Michael S. Tsirkin" Subject: Re: [PATCH] virtio: Try to untangle DMA coherency Message-ID: <20170201182659.GM8177@arm.com> References: <8a6494f6409c20b4609cd6bdcdd751f68b5c0564.1485951731.git.robin.murphy@arm.com> <20170201195732-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170201195732-mutt-send-email-mst@kernel.org> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170201_102730_234633_48C7E84A X-CRM114-Status: GOOD ( 28.08 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, pawel.moll@arm.com, jasowang@redhat.com, virtualization@lists.linux-foundation.org, robh+dt@kernel.org, Robin Murphy , linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Feb 01, 2017 at 08:09:21PM +0200, Michael S. Tsirkin wrote: > On Wed, Feb 01, 2017 at 12:25:57PM +0000, Robin Murphy wrote: > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 7e38ed79c3fc..961af25b385c 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -20,6 +20,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -160,10 +161,14 @@ static bool vring_use_dma_api(struct virtio_device *vdev) > > return true; > > > > /* > > - * On ARM-based machines, the DMA ops will do the right thing, > > - * so always use them with legacy devices. > > + * On ARM-based machines, the coherent DMA ops will do the right > > + * thing, so always use them with legacy devices. However, using > > + * non-coherent DMA when the host *is* actually coherent, but has > > + * forgotten to tell us, is going to break badly; since this situation > > + * already exists in the wild, maintain the old behaviour there. > > */ > > - if (IS_ENABLED(CONFIG_ARM) || IS_ENABLED(CONFIG_ARM64)) > > + if ((IS_ENABLED(CONFIG_ARM) || IS_ENABLED(CONFIG_ARM64)) && > > + device_get_dma_attr(&vdev->dev) == DEV_DMA_COHERENT) > > return !virtio_has_feature(vdev, VIRTIO_F_VERSION_1); > > > > return false; > > This is exactly what I feared. Yes, sorry about this. It works fine for virtio-pci (where "dma-coherent" is used) and it also works on the fastmodel if you disable cache-modelling (which is needed to make the thing run at a usable pace) so we didn't spot this in testing. > Could we identify fastboot and do the special dance just for it? [assuming you mean fastmodel instead of fastboot] > I'd like to do that instead. It's fastboot doing the unreasonable thing > here and deviating from what every other legacy device without exception > did for years. If this means fastboot will need to update to virtio 1, > all the better. The problem still exists with virtio 1, unless we require that the "dma-coherent" property is set/unset correctly when VIRTIO_F_IOMMU_PLATFORM is advertised by the device (which is what I suggested in my reply). We can't detect the fastmodel, but we could implicitly treat virtio-mmio devices as cache-coherent regardless of the "dma-coherent" flag. I already prototyped this, but I suspect the devicetree people will push back (and there's a similar patch needed for ACPI). See below. Do you prefer this approach? Will --->8 From f6ad4e331c26e7ba53132c8cc74e26f782391570 Mon Sep 17 00:00:00 2001 From: Will Deacon Date: Mon, 30 Jan 2017 17:28:31 +0000 Subject: [PATCH] of/address: Allow devices to report DMA coherency based on compatible string Some devices (e.g. virtio-mmio) are implicitly cache coherent with respect to DMA operations and therefore do not mandate the use of "dma-coherent" in their devicetree bindings. In order to ensure that these devices work correctly when using the DMA API, we need to treat them specially in of_dma_is_coherent by identifying them as unconditionally coherent. This patch adds a static, table-based search against the compatible string for the device in of_dma_is_coherent before walking the hierarchy looking for "dma-coherent". This allows existing virtio-mmio devices (e.g. those emulated by QEMU) to function correctly when placed behind an IOMMU that requires use of the DMA ops to map the vring. Cc: Lorenzo Pieralisi Cc: Mark Rutland Signed-off-by: Will Deacon --- drivers/of/address.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/drivers/of/address.c b/drivers/of/address.c index 02b2903fe9d2..af29b115b8aa 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -891,19 +891,47 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz } EXPORT_SYMBOL_GPL(of_dma_get_range); +/* + * DMA from some device types is always cache-coherent, and in some unfortunate + * cases the "dma-coherent" property is not used. + */ +static const char *of_device_dma_coherent_tbl[] = { + /* + * Virtio MMIO devices are assumed to be cache-coherent when accessing + * main memory. Neither QEMU nor kvmtool emit "dma-coherent" properties + * for their generated virtio MMIO device nodes, and the binding + * documentation doesn't mention them either. When using the DMA API + * (e.g. because there is an IOMMU in the system), we must report true + * here to avoid lockups where writes to the vring via a non-coherent + * mapping are not made visible to the device emulation. + */ + "virtio,mmio", + NULL, +}; + /** * of_dma_is_coherent - Check if device is coherent * @np: device node * * It returns true if "dma-coherent" property was found - * for this device in DT. + * for this device in DT or the device is statically known to be + * coherent. */ bool of_dma_is_coherent(struct device_node *np) { struct device_node *node = of_node_get(np); + /* + * Check for implicit DMA coherence first, since we don't want + * to inherit this. + */ + if (of_device_compatible_match(np, of_device_dma_coherent_tbl)) { + of_node_put(node); + return true; + } + while (node) { - if (of_property_read_bool(node, "dma-coherent")) { + if (of_property_read_bool(node, "dma-coherent")){ of_node_put(node); return true; }