Re: [Qemu-devel] [PATCH 13/13] iommu: Add a memory barrier to DMA RW function

[David Gibson <david@gibson.dropbear.id.au>]

On Mon, May 14, 2012 at 07:52:15PM -0500, Anthony Liguori wrote:
> On 05/09/2012 11:49 PM, Benjamin Herrenschmidt wrote:
> >From: David Gibson<david@gibson.dropbear.id.au>
> >
> >The emulated devices can run simultaneously with the guest, so
> >we need to be careful with ordering of load and stores done by
> >them to the guest system memory, which need to be observed in
> >the right order by the guest operating system.
> >
> >The simplest way for now to address that is to stick a memory
> >barrier in the main DMA read/write function of the iommu layer,
> >this will make everything using that layer hopefully "just work".
> >
> >We don't emulate devices supporting the relaxed ordering PCIe
> >feature nor do we want to look at doing more fine grained
> >barriers for now as it could quickly become too complex and not
> >worth the cost.
> >
> >Note that this will not help devices using the map/unmap APIs,
> >those will need to use explicit barriers, similar to what
> >virtio does.
> >
> >Signed-off-by: David Gibson<david@gibson.dropbear.id.au>
> >Signed-off-by: Benjamin Herrenschmidt<benh@kernel.crashing.org>
> >---
> >  dma-helpers.c |    3 +++
> >  1 file changed, 3 insertions(+)
> >
> >diff --git a/dma-helpers.c b/dma-helpers.c
> >index 36fa963..4350cdf 100644
> >--- a/dma-helpers.c
> >+++ b/dma-helpers.c
> >@@ -312,6 +312,9 @@ int iommu_dma_memory_rw(DMAContext *dma, dma_addr_t addr,
> >          buf += plen;
> >      }
> >
> >+    /* HACK: full memory barrier here */
> >+    __sync_synchronize();
> 
> I thought you were going to limit this to the TCE iommu?

So, it wasn't my intention to send this one with the rest, but I
forgot to explain that to Ben when he resent.  As the comment
suggests, this is a hack we have been using internally to see if
certain ordering problems were what we thought they were.  If that
turned out to be the case (and it now looks like it is), we need to
work out where to correctly place this barrier.  As Ben says, this
should probably really be in the PCI accessors, and we should use the
finer grained primitives from qemu-barrier.h rather than the brute
force __sync_synchronize().

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson