[PATCH] virtio: Try to untangle DMA coherency

[Rob Herring]

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