Re: [PATCH] virtio: Try to untangle DMA coherency

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Feb 08, 2017 at 01:58:10PM +0100, Alexander Graf wrote:
> On 02/01/2017 08:19 PM, Michael S. Tsirkin wrote:
> > On Wed, Feb 01, 2017 at 06:27:09PM +0000, Will Deacon wrote:
> > > 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).
> > 
> > I'm not ignoring that, but I need to understand that part a bit better.
> > I'll reply to that patch in a day or two after looking at how _CCA is
> > supposed to work.
> > 
> > > We can't detect the fastmodel,
> > 
> > Surely, it puts a hardware id somewhere? I think you mean
> > fastmodel isn't always affected, right?
> > 
> > > 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
> > 
> > I'd like to see basically
> > 
> > if (fastmodel)
> > 	a pile of special work-arounds
> > else
> > 	not less hacky but more common virtio work-arounds
> > 
> > :)
> > 
> > And then I can apply whatever comes from @arm.com and not
> > worry about breaking actual hardware.
> 
> I'm actually seeing the exact same breakage in QEMU right now, so it's not
> fast model related at all. In QEMU we also don't properly set the
> dma-coherent flag, so we run into cache coherency problems.
> 
> 
> Alex

But with latest revert QEMU should now work, right?

-- 
MST