[Qemu-devel] [PATCH-RFC 1/3] qemu: add barriers.h header

[Qemu-devel] [PATCH-RFC 1/3] qemu: add barriers.h header

[Michael S. Tsirkin]

With qemu-kvm and with iothread, guest might read in-memory structures
on one host CPU while it is being updated on another one, and vice
versa.  Therefore we must use memory barriers to ensure proper memory
read/write ordering.

The following implements memory barriers with API matching
that of the linux kernel, and implementation based on
a header from libibverbs infiniband library
(which happened to be pretty clean and which I am familiar with).

Since that library uses dual GPLv2/BSD I think it's nice to keep BSD license
option around just for this header, so that the original can borrow back
from us.

It seems that work is ongoing to implement a portable atomics
library - no option seems to be mature at the moment,
but if that becomes available, we'll be able to use it,
and replace the header in qemu. All of the upcoming
variants seem to be API compatible with linux,
so it will be easy.

Barriers aren't currently required with tcg because tcg does not
implement atomics, and so it runs iothread in lockstep
with tcg thread. However, it's better to keep it simple
and insert barriers always.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/barriers.h |  131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 131 insertions(+), 0 deletions(-)
 create mode 100644 hw/barriers.h

diff --git a/hw/barriers.h b/hw/barriers.h
new file mode 100644
index 0000000..e1dbb70
--- /dev/null
+++ b/hw/barriers.h
@@ -0,0 +1,131 @@
+/*
+ * Copyright (c) 2005 Topspin Communications.  All rights reserved.
+ * Copyright (c) 2009 Red Hat Inc.  All rights reserved.
+ *
+ * Author: Michael S. Tsirkin <mst@redhat.com>
+ * Based on libibverbs library by Roland Dreier <rolandd@cisco.com>
+ *
+ * Note: Please be nice and keep the BSD license option on this file around, so
+ * that libibverbs library can reuse this code if it wants to.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef QEMU_BARRIERS_H
+#define QEMU_BARRIERS_H
+
+/* Compiler barrier. Prevents compiler from re-ordering
+ * memory operations, but does not prevent CPU from doing this. */
+#define barrier() asm volatile("" ::: "memory")
+
+/*
+ * Host-architecture-specific defines.  Currently, an architecture is
+ * required to implement the following operations:
+ *
+ * mb() - memory barrier.  No loads or stores may be reordered across
+ *     this macro by either the compiler or the CPU.
+ * rmb() - read memory barrier.  No loads may be reordered across this
+ *     macro by either the compiler or the CPU.
+ * wmb() - write memory barrier.  No stores may be reordered across
+ *     this macro by either the compiler or the CPU.
+ */
+
+#if defined(__i386__)
+
+/* We use GCC builtin if it's available, as that can use
+ * mfence on 32 bit as well, e.g. if built with -march=pentium-m.
+ * However, on i386, there seem to be known bugs as recently as 4.3. */
+#if defined(_GNUC__) && __GNUC__ >= 4 && __GNUC_MINOR__ >= 4
+#define mb() __sync_synchronize()
+#else
+#define mb() asm volatile("lock; addl $0,0(%%esp) " ::: "memory")
+#endif
+#define rmb() mb()
+/*
+ * We use a compiler barrier for wmb() because we don't care about
+ * ordering against non-temporal/string stores (for now at least).
+ */
+#define wmb() barrier()
+
+#elif defined(__x86_64__)
+
+/*
+ * We use a compiler barrier for wmb() because we don't care about
+ * ordering against non-temporal/string stores (for now at least).
+ */
+#define mb() asm volatile("mfence" ::: "memory")
+#define rmb() asm volatile("lfence" ::: "memory")
+#define wmb() barrier()
+
+#elif defined(__PPC64__)
+
+#define mb() asm volatile("sync" ::: "memory")
+#define rmb() asm volatile("lwsync" ::: "memory")
+#define wmb() mb()
+
+#elif defined(__ia64__)
+
+#define mb() asm volatile("mf" ::: "memory")
+#define rmb() mb()
+#define wmb() mb()
+
+#elif defined(__PPC__)
+
+#define mb() asm volatile("sync" ::: "memory")
+#define rmb() mb()
+#define wmb() asm volatile("eieio" ::: "memory")
+
+#elif defined(__sparc_v9__)
+
+#define mb() asm volatile("membar #LoadLoad | #LoadStore | " \
+                          "#StoreStore | #StoreLoad" ::: "memory")
+#define rmb() asm volatile("membar #LoadLoad" ::: "memory")
+#define wmb() asm volatile("membar #StoreStore" ::: "memory")
+
+#elif defined(__sparc__)
+
+#define mb() barrier()
+#define rmb() mb()
+#define wmb() mb()
+
+#else
+
+#warning No architecture specific defines found.  Using generic implementation.
+
+/* Use GCC builtin if it's available. */
+#if defined(_GNUC__) && __GNUC__ >= 4 && __GNUC_MINOR__ >= 1
+#define mb() __sync_synchronize()
+#else
+#define mb() barrier()
+#endif
+#define rmb() mb()
+#define wmb() mb()
+
+#endif
+#endif /* QEMU_BARRIERS_H */
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] [PATCH-RFC 2/3] virtio: use a real wmb

[Michael S. Tsirkin]

include barriers.h and remove a non-portable
wmb implementation from virtio.c (it will work
for intel but not for other architectures).

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index 1f92171..9f020cf 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -15,20 +15,12 @@
 
 #include "virtio.h"
 #include "sysemu.h"
+#include "barriers.h"
 
 /* The alignment to use between consumer and producer parts of vring.
  * 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")
-
 typedef struct VRingDesc
 {
     uint64_t addr;
-- 
1.6.5.2.143.g8cc62

[Qemu-devel] [PATCH-RFC 3/3] virtio: add missing barriers

[Michael S. Tsirkin]

Add missing memory barriers in virtio:
- before checking interrupt enabled bit, make sure
  data has been written to memory
- make sure ring index has been read before reading ring data

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index 9f020cf..bbd7b51 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -119,8 +119,12 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
 static inline uint16_t vring_avail_idx(VirtQueue *vq)
 {
     target_phys_addr_t pa;
+    uint16_t idx;
     pa = vq->vring.avail + offsetof(VRingAvail, idx);
-    return lduw_phys(pa);
+    idx = lduw_phys(pa);
+    /* Make sure data read happens after index read */
+    rmb();
+    return idx;
 }
 
 static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
@@ -588,6 +592,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
 
 void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
 {
+    /* Make sure used ring is updated before we check NO_INTERRUPT. */
+    mb();
     /* Always notify when queue is empty (when feature acknowledge) */
     if ((vring_avail_flags(vq) & VRING_AVAIL_F_NO_INTERRUPT) &&
         (!(vdev->features & (1 << VIRTIO_F_NOTIFY_ON_EMPTY)) ||
-- 
1.6.5.2.143.g8cc62