[Qemu-devel] [PATCH] virtio: Make memory barriers be memory barriers

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

The virtio code already has memory barrier wmb() macros in the code.
However they are was defined as no-ops.  The comment claims that real
barriers are not necessary because the code does not run concurrent.
However, with kvm and io-thread enabled, this is not true and this qemu
code can indeed run concurrently with the guest kernel.  This does not
cause problems on x86 due to it's strongly ordered storage model, but it
causes a race leading to virtio errors on POWER which has a relaxed storage
ordering model.

Specifically, the QEMU puts new element into the "used" ring and then
updates the ring free-running counter.  Without a barrier between these
under the right circumstances, the guest linux driver can receive an
interrupt, read the counter change but find the ring element to be handled
still has an old value, leading to an "id %u is not a head!\n" error
message.

The problem is easy to reproduce on POWER using virtio-net with heavy
traffic.

The patch defines wmb() as __sync_synchronize(), a cross platform memory
barrier primitive available in sufficiently recent gcc versions (gcc 4.2
and after?).  If we care about older gccs then this patch will need to
be updated with some sort of fallback.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/virtio.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index 13aa0fa..c9f0e75 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -21,14 +21,8 @@
  * x86 pagesize again. */
 #define VIRTIO_PCI_VRING_ALIGN         4096
 
-/* QEMU doesn't strictly need write barriers since everything runs in
- * lock-step.  We'll leave the calls to wmb() in though to make it obvious for
- * KVM or if kqemu gets SMP support.
- * In any case, we must prevent the compiler from reordering the code.
- * TODO: we likely need some rmb()/mb() as well.
- */
-
-#define wmb() __asm__ __volatile__("": : :"memory")
+ /* TODO: we may also need rmb()s.  It hasn't bitten us yet, but.. */
+ #define wmb() __sync_synchronize()
 
 typedef struct VRingDesc
 {
-- 
1.7.5.4