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@xxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
