By forcing on DMA API usage for ARM systems, we have inadvertently
kicked open a hornets' nest in terms of cache-coherency. Namely that
unless the virtio device is explicitly described as capable of coherent
DMA by firmware, the DMA APIs on ARM and other DT-based platforms will
assume it is non-coherent. This turns out to cause a big problem for the
likes of QEMU and kvmtool, which generate virtio-mmio devices in their
guest DTs but neglect to add the often-overlooked "dma-coherent"
property; as a result, we end up with the guest making non-cacheable
accesses to the vring, the host doing so cacheably, both talking past
each other and things going horribly wrong.
To prevent regressing those existing use cases relying on implicit
coherency, but still fixing the original problem of (coherent PCI)
legacy devices behind IOMMUs, take the more conservative approach of
only hitting the DMA API switch for coherent devices, where we can be
sure it is safe, and preserving the old non-DMA behaviour otherwise.
This does not affect devices setting the VIRTIO_F_IOMMU_PLATFORM flag,
which as before are still at the mercy of architecture code correctly
knowing their coherency, so explicitly call this out in the virtio-mmio
DT binding in the hope of heading off any further workarounds for future
firmware mishaps.
Fixes: c7070619f340 ("vring: Force use of DMA API for ARM-based systems
with legacy devices")
Signed-off-by: Robin Murphy <robin.murphy at arm.com>
---
Documentation/devicetree/bindings/virtio/mmio.txt | 3 +++
drivers/virtio/virtio_ring.c | 11 ++++++++---
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/virtio/mmio.txt
b/Documentation/devicetree/bindings/virtio/mmio.txt
index 5069c1b8e193..8f2a981e1010 100644
--- a/Documentation/devicetree/bindings/virtio/mmio.txt
+++ b/Documentation/devicetree/bindings/virtio/mmio.txt
@@ -7,6 +7,8 @@ Required properties:
- compatible: "virtio,mmio" compatibility string
- reg: control registers base address and size including configuration space
- interrupts: interrupt generated by the device
+- dma-coherent: required if the device (or host emulation) accesses memory
+ cache-coherently, absent otherwise
Example:
@@ -14,4 +16,5 @@ Example:
compatible = "virtio,mmio";
reg = <0x3000 0x100>;
interrupts = <41>;
+ dma-coherent;
}
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 <linux/virtio_ring.h>
#include <linux/virtio_config.h>
#include <linux/device.h>
+#include <linux/property.h>
#include <linux/slab.h>
#include <linux/module.h>
#include <linux/hrtimer.h>
@@ -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;
--
2.11.0.dirty
On Wed, Feb 01, 2017 at 12:25:57PM +0000, Robin Murphy wrote:> By forcing on DMA API usage for ARM systems, we have inadvertently > kicked open a hornets' nest in terms of cache-coherency. Namely that > unless the virtio device is explicitly described as capable of coherent > DMA by firmware, the DMA APIs on ARM and other DT-based platforms will > assume it is non-coherent. This turns out to cause a big problem for the > likes of QEMU and kvmtool, which generate virtio-mmio devices in their > guest DTs but neglect to add the often-overlooked "dma-coherent" > property; as a result, we end up with the guest making non-cacheable > accesses to the vring, the host doing so cacheably, both talking past > each other and things going horribly wrong. > > To prevent regressing those existing use cases relying on implicit > coherency, but still fixing the original problem of (coherent PCI) > legacy devices behind IOMMUs, take the more conservative approach of > only hitting the DMA API switch for coherent devices, where we can be > sure it is safe, and preserving the old non-DMA behaviour otherwise. > This does not affect devices setting the VIRTIO_F_IOMMU_PLATFORM flag, > which as before are still at the mercy of architecture code correctly > knowing their coherency, so explicitly call this out in the virtio-mmio > DT binding in the hope of heading off any further workarounds for future > firmware mishaps. > > Fixes: c7070619f340 ("vring: Force use of DMA API for ARM-based systems with legacy devices") > Signed-off-by: Robin Murphy <robin.murphy at arm.com> > --- > Documentation/devicetree/bindings/virtio/mmio.txt | 3 +++ > drivers/virtio/virtio_ring.c | 11 ++++++++--- > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/virtio/mmio.txt b/Documentation/devicetree/bindings/virtio/mmio.txt > index 5069c1b8e193..8f2a981e1010 100644 > --- a/Documentation/devicetree/bindings/virtio/mmio.txt > +++ b/Documentation/devicetree/bindings/virtio/mmio.txt > @@ -7,6 +7,8 @@ Required properties: > - compatible: "virtio,mmio" compatibility string > - reg: control registers base address and size including configuration space > - interrupts: interrupt generated by the device > +- dma-coherent: required if the device (or host emulation) accesses memory > + cache-coherently, absent otherwiseSo QEMU is getting with not providing this property at the moment and this patch continues to ensure that works coherently, which is the right thing to do. However, if QEMU ever emulates devices with VIRTIO_F_IOMMU_PLATFORM, then it will need to provide this property for coherent virtio-mmio devices upstream of the IOMMU otherwise things won't work. I think that's acceptable given that VIRTIO_F_IOMMU_PLATFORM has never done the right thing with respect to coherency if "dma-coherent" is not present, but it would be nice to call that out somewhere so that QEMU developers can be aware of this pitfall. Could we add something like: Linux implementation note: virtio-mmio devices that do not advertise the VIRTIO_F_IOMMU_PLATFORM feature are treated as cache-coherent irrespective of the "dma-coherent" property. If VIRTIO_F_IOMMU_PLATFORM is advertised, then the "dma-coherent" property must accurately reflect the coherency of the device. ? I know that the binding documentation needs to be OS agnostic, but I think it's useful to describe the Linux behaviour here given that QEMU is technically in violation of the binding after this patch is applied. Will
On Wed, Feb 01, 2017 at 02:57:11PM +0000, Will Deacon wrote:> On Wed, Feb 01, 2017 at 12:25:57PM +0000, Robin Murphy wrote: > > By forcing on DMA API usage for ARM systems, we have inadvertently > > kicked open a hornets' nest in terms of cache-coherency. Namely that > > unless the virtio device is explicitly described as capable of coherent > > DMA by firmware, the DMA APIs on ARM and other DT-based platforms will > > assume it is non-coherent. This turns out to cause a big problem for the > > likes of QEMU and kvmtool, which generate virtio-mmio devices in their > > guest DTs but neglect to add the often-overlooked "dma-coherent" > > property; as a result, we end up with the guest making non-cacheable > > accesses to the vring, the host doing so cacheably, both talking past > > each other and things going horribly wrong. > > > > To prevent regressing those existing use cases relying on implicit > > coherency, but still fixing the original problem of (coherent PCI) > > legacy devices behind IOMMUs, take the more conservative approach of > > only hitting the DMA API switch for coherent devices, where we can be > > sure it is safe, and preserving the old non-DMA behaviour otherwise. > > This does not affect devices setting the VIRTIO_F_IOMMU_PLATFORM flag, > > which as before are still at the mercy of architecture code correctly > > knowing their coherency, so explicitly call this out in the virtio-mmio > > DT binding in the hope of heading off any further workarounds for future > > firmware mishaps. > > > > Fixes: c7070619f340 ("vring: Force use of DMA API for ARM-based systems with legacy devices") > > Signed-off-by: Robin Murphy <robin.murphy at arm.com> > > --- > > Documentation/devicetree/bindings/virtio/mmio.txt | 3 +++ > > drivers/virtio/virtio_ring.c | 11 ++++++++--- > > 2 files changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/virtio/mmio.txt b/Documentation/devicetree/bindings/virtio/mmio.txt > > index 5069c1b8e193..8f2a981e1010 100644 > > --- a/Documentation/devicetree/bindings/virtio/mmio.txt > > +++ b/Documentation/devicetree/bindings/virtio/mmio.txt > > @@ -7,6 +7,8 @@ Required properties: > > - compatible: "virtio,mmio" compatibility string > > - reg: control registers base address and size including configuration space > > - interrupts: interrupt generated by the device > > +- dma-coherent: required if the device (or host emulation) accesses memory > > + cache-coherently, absent otherwise > > So QEMU is getting with not providing this property at the moment and this > patch continues to ensure that works coherently, which is the right thing > to do. However, if QEMU ever emulates devices with VIRTIO_F_IOMMU_PLATFORM, > then it will need to provide this property for coherent virtio-mmio > devices upstream of the IOMMU otherwise things won't work. > > I think that's acceptable given that VIRTIO_F_IOMMU_PLATFORM has never > done the right thing with respect to coherency if "dma-coherent" is not > present, but it would be nice to call that out somewhere so that QEMU > developers can be aware of this pitfall. > > Could we add something like: > > > Linux implementation note: > > virtio-mmio devices that do not advertise the VIRTIO_F_IOMMU_PLATFORM > feature are treated as cache-coherent irrespective of the "dma-coherent" > property. If VIRTIO_F_IOMMU_PLATFORM is advertised, then the > "dma-coherent" property must accurately reflect the coherency of the > device. > > > ? > > I know that the binding documentation needs to be OS agnostic, but I think > it's useful to describe the Linux behaviour here given that QEMU is > technically in violation of the binding after this patch is applied.Sounds fine to me. Rob
On Wed, Feb 01, 2017 at 12:25:57PM +0000, Robin Murphy wrote:> By forcing on DMA API usage for ARM systems, we have inadvertently > kicked open a hornets' nest in terms of cache-coherency. Namely that > unless the virtio device is explicitly described as capable of coherent > DMA by firmware, the DMA APIs on ARM and other DT-based platforms will > assume it is non-coherent. This turns out to cause a big problem for the > likes of QEMU and kvmtool, which generate virtio-mmio devices in their > guest DTs but neglect to add the often-overlooked "dma-coherent" > property; as a result, we end up with the guest making non-cacheable > accesses to the vring, the host doing so cacheably, both talking past > each other and things going horribly wrong. > > To prevent regressing those existing use cases relying on implicit > coherency, but still fixing the original problem of (coherent PCI) > legacy devices behind IOMMUs, take the more conservative approach of > only hitting the DMA API switch for coherent devices, where we can be > sure it is safe, and preserving the old non-DMA behaviour otherwise. > This does not affect devices setting the VIRTIO_F_IOMMU_PLATFORM flag, > which as before are still at the mercy of architecture code correctly > knowing their coherency, so explicitly call this out in the virtio-mmio > DT binding in the hope of heading off any further workarounds for future > firmware mishaps. > > Fixes: c7070619f340 ("vring: Force use of DMA API for ARM-based systems with legacy devices") > Signed-off-by: Robin Murphy <robin.murphy at arm.com> > --- > Documentation/devicetree/bindings/virtio/mmio.txt | 3 +++ > drivers/virtio/virtio_ring.c | 11 ++++++++--- > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/virtio/mmio.txt b/Documentation/devicetree/bindings/virtio/mmio.txt > index 5069c1b8e193..8f2a981e1010 100644 > --- a/Documentation/devicetree/bindings/virtio/mmio.txt > +++ b/Documentation/devicetree/bindings/virtio/mmio.txt > @@ -7,6 +7,8 @@ Required properties: > - compatible: "virtio,mmio" compatibility string > - reg: control registers base address and size including configuration space > - interrupts: interrupt generated by the device > +- dma-coherent: required if the device (or host emulation) accesses memory > + cache-coherently, absent otherwise > > Example: > > @@ -14,4 +16,5 @@ Example: > compatible = "virtio,mmio"; > reg = <0x3000 0x100>; > interrupts = <41>; > + dma-coherent; > } > 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 <linux/virtio_ring.h> > #include <linux/virtio_config.h> > #include <linux/device.h> > +#include <linux/property.h> > #include <linux/slab.h> > #include <linux/module.h> > #include <linux/hrtimer.h> > @@ -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. Could we identify fastboot and do the special dance just for it? 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.> -- > 2.11.0.dirty
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 <linux/virtio_ring.h> > > #include <linux/virtio_config.h> > > #include <linux/device.h> > > +#include <linux/property.h> > > #include <linux/slab.h> > > #include <linux/module.h> > > #include <linux/hrtimer.h> > > @@ -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 2001From: Will Deacon <will.deacon at arm.com> 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 <lorenzo.pieralisi at arm.com> Cc: Mark Rutland <mark.rutland at arm.com> Signed-off-by: Will Deacon <will.deacon at arm.com> --- 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; } -- 2.1.4