* [Qemu-devel] [PATCH 2/2] Barriers in qemu-barrier.h should not be x86 specific
2011-09-20 2:05 [Qemu-devel] [PATCH 1/2] virtio: Use global memory barrier macros David Gibson
@ 2011-09-20 2:05 ` David Gibson
2011-10-02 10:05 ` Michael S. Tsirkin
2011-09-23 18:51 ` [Qemu-devel] [PATCH 1/2] virtio: Use global memory barrier macros Anthony Liguori
2011-10-02 10:04 ` Michael S. Tsirkin
2 siblings, 1 reply; 5+ messages in thread
From: David Gibson @ 2011-09-20 2:05 UTC (permalink / raw)
To: aliguori, avi; +Cc: mst, aik, rusty, qemu-devel, agraf, pbonzini
qemu-barrier.h contains a few macros implementing memory barrier
primitives used in several places throughout qemu. However, apart
from the compiler-only barrier, the defined wmb() is correct only for
x86, or platforms which are similarly strongly ordered.
This patch addresses the FIXME about this by making the wmb() macro
arch dependent. On x86, it remains a compiler barrier only, but with
a comment explaining in more detail the conditions under which this is
correct. On weakly-ordered powerpc, an "eieio" instruction is used,
again with explanation of the conditions under which it is sufficient.
On other platforms, we use the __sync_synchronize() primitive,
available in sufficiently recent gcc (4.2 and after?). This should
implement a full barrier which will be sufficient on all platforms,
although it may be overkill in some cases. Other platforms can add
optimized versions in future if it's worth it for them.
Without proper memory barriers, it is easy to reproduce ordering
problems with virtio on powerpc; 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.
Similar problems are likely to be possible with kvm on other weakly
ordered platforms.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
qemu-barrier.h | 34 +++++++++++++++++++++++++++++++---
1 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/qemu-barrier.h b/qemu-barrier.h
index b77fce2..735eea6 100644
--- a/qemu-barrier.h
+++ b/qemu-barrier.h
@@ -1,10 +1,38 @@
#ifndef __QEMU_BARRIER_H
#define __QEMU_BARRIER_H 1
-/* FIXME: arch dependant, x86 version */
-#define smp_wmb() asm volatile("" ::: "memory")
-
/* Compiler barrier */
#define barrier() asm volatile("" ::: "memory")
+#if defined(__i386__) || defined(__x86_64__)
+
+/*
+ * Because of the strongly ordered x86 storage model, wmb() is a nop
+ * on x86(well, a compiler barrier only). Well, at least as long as
+ * qemu doesn't do accesses to write-combining memory or non-temporal
+ * load/stores from C code.
+ */
+#define smp_wmb() barrier()
+
+#elif defined(__powerpc__)
+
+/*
+ * We use an eieio() for a wmb() on powerpc. This assumes we don't
+ * need to order cacheable and non-cacheable stores with respect to
+ * each other
+ */
+#define smp_wmb() asm volatile("eieio" ::: "memory")
+
+#else
+
+/*
+ * For (host) platforms we don't have explicit barrier definitions
+ * for, we use the gcc __sync_synchronize() primitive to generate a
+ * full barrier. This should be safe on all platforms, though it may
+ * be overkill.
+ */
+#define smp_wmb() __sync_synchronize()
+
+#endif
+
#endif
--
1.7.5.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH 2/2] Barriers in qemu-barrier.h should not be x86 specific
2011-09-20 2:05 ` [Qemu-devel] [PATCH 2/2] Barriers in qemu-barrier.h should not be x86 specific David Gibson
@ 2011-10-02 10:05 ` Michael S. Tsirkin
0 siblings, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2011-10-02 10:05 UTC (permalink / raw)
To: David Gibson; +Cc: aliguori, aik, rusty, agraf, qemu-devel, avi, pbonzini
On Tue, Sep 20, 2011 at 12:05:21PM +1000, David Gibson wrote:
> qemu-barrier.h contains a few macros implementing memory barrier
> primitives used in several places throughout qemu. However, apart
> from the compiler-only barrier, the defined wmb() is correct only for
> x86, or platforms which are similarly strongly ordered.
>
> This patch addresses the FIXME about this by making the wmb() macro
> arch dependent. On x86, it remains a compiler barrier only, but with
> a comment explaining in more detail the conditions under which this is
> correct. On weakly-ordered powerpc, an "eieio" instruction is used,
> again with explanation of the conditions under which it is sufficient.
>
> On other platforms, we use the __sync_synchronize() primitive,
> available in sufficiently recent gcc (4.2 and after?). This should
> implement a full barrier which will be sufficient on all platforms,
> although it may be overkill in some cases. Other platforms can add
> optimized versions in future if it's worth it for them.
>
> Without proper memory barriers, it is easy to reproduce ordering
> problems with virtio on powerpc; 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.
> Similar problems are likely to be possible with kvm on other weakly
> ordered platforms.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> qemu-barrier.h | 34 +++++++++++++++++++++++++++++++---
> 1 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/qemu-barrier.h b/qemu-barrier.h
> index b77fce2..735eea6 100644
> --- a/qemu-barrier.h
> +++ b/qemu-barrier.h
> @@ -1,10 +1,38 @@
> #ifndef __QEMU_BARRIER_H
> #define __QEMU_BARRIER_H 1
>
> -/* FIXME: arch dependant, x86 version */
> -#define smp_wmb() asm volatile("" ::: "memory")
> -
> /* Compiler barrier */
> #define barrier() asm volatile("" ::: "memory")
>
> +#if defined(__i386__) || defined(__x86_64__)
> +
> +/*
> + * Because of the strongly ordered x86 storage model, wmb() is a nop
> + * on x86(well, a compiler barrier only). Well, at least as long as
> + * qemu doesn't do accesses to write-combining memory or non-temporal
> + * load/stores from C code.
> + */
> +#define smp_wmb() barrier()
> +
> +#elif defined(__powerpc__)
> +
> +/*
> + * We use an eieio() for a wmb() on powerpc. This assumes we don't
> + * need to order cacheable and non-cacheable stores with respect to
> + * each other
> + */
> +#define smp_wmb() asm volatile("eieio" ::: "memory")
> +
> +#else
> +
> +/*
> + * For (host) platforms we don't have explicit barrier definitions
> + * for, we use the gcc __sync_synchronize() primitive to generate a
> + * full barrier. This should be safe on all platforms, though it may
> + * be overkill.
> + */
> +#define smp_wmb() __sync_synchronize()
> +
> +#endif
> +
> #endif
> --
> 1.7.5.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] virtio: Use global memory barrier macros
2011-09-20 2:05 [Qemu-devel] [PATCH 1/2] virtio: Use global memory barrier macros David Gibson
2011-09-20 2:05 ` [Qemu-devel] [PATCH 2/2] Barriers in qemu-barrier.h should not be x86 specific David Gibson
@ 2011-09-23 18:51 ` Anthony Liguori
2011-10-02 10:04 ` Michael S. Tsirkin
2 siblings, 0 replies; 5+ messages in thread
From: Anthony Liguori @ 2011-09-23 18:51 UTC (permalink / raw)
To: David Gibson; +Cc: mst, aik, rusty, agraf, qemu-devel, avi, pbonzini
On 09/19/2011 09:05 PM, David Gibson wrote:
> The virtio code uses wmb() macros in several places, as required by the
> SMP-aware virtio protocol. However the wmb() macro is locally defined
> to be a compiler barrier only. This is probably sufficient on x86
> due to its strong storage ordering model, but it certainly isn't on other
> platforms, such as ppc.
>
> In any case, qemu already has some globally defined memory barrier macros
> in qemu-barrier.h. This patch, therefore converts virtio.c to use those
> barrier macros. The macros in qemu-barrier.h are also wrong (or at least,
> safe for x86 only) but this way at least there's only one place to fix
> them.
>
> Signed-off-by: Alexey Kardashevskiy<aik@ozlabs.ru>
> Signed-off-by: David Gibson<david@gibson.dropbear.id.au>
Applied all. Thanks.
Regards,
Anthony Liguori
> ---
> hw/virtio.c | 14 +++-----------
> 1 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 946d911..9663294 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -16,20 +16,12 @@
> #include "trace.h"
> #include "qemu-error.h"
> #include "virtio.h"
> +#include "qemu-barrier.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;
> @@ -264,7 +256,7 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
> {
> uint16_t old, new;
> /* Make sure buffer is written before we update index. */
> - wmb();
> + smp_wmb();
> trace_virtqueue_flush(vq, count);
> old = vring_used_idx(vq);
> new = old + count;
> @@ -324,7 +316,7 @@ static unsigned virtqueue_next_desc(target_phys_addr_t desc_pa,
> /* Check they're not leading us off end of descriptors. */
> next = vring_desc_next(desc_pa, i);
> /* Make sure compiler knows to grab that: we don't want it changing! */
> - wmb();
> + smp_wmb();
>
> if (next>= max) {
> error_report("Desc next is %u", next);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] virtio: Use global memory barrier macros
2011-09-20 2:05 [Qemu-devel] [PATCH 1/2] virtio: Use global memory barrier macros David Gibson
2011-09-20 2:05 ` [Qemu-devel] [PATCH 2/2] Barriers in qemu-barrier.h should not be x86 specific David Gibson
2011-09-23 18:51 ` [Qemu-devel] [PATCH 1/2] virtio: Use global memory barrier macros Anthony Liguori
@ 2011-10-02 10:04 ` Michael S. Tsirkin
2 siblings, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2011-10-02 10:04 UTC (permalink / raw)
To: David Gibson; +Cc: aliguori, aik, rusty, agraf, qemu-devel, avi, pbonzini
On Tue, Sep 20, 2011 at 12:05:20PM +1000, David Gibson wrote:
> The virtio code uses wmb() macros in several places, as required by the
> SMP-aware virtio protocol. However the wmb() macro is locally defined
> to be a compiler barrier only. This is probably sufficient on x86
> due to its strong storage ordering model, but it certainly isn't on other
> platforms, such as ppc.
>
> In any case, qemu already has some globally defined memory barrier macros
> in qemu-barrier.h. This patch, therefore converts virtio.c to use those
> barrier macros. The macros in qemu-barrier.h are also wrong (or at least,
> safe for x86 only) but this way at least there's only one place to fix
> them.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/virtio.c | 14 +++-----------
> 1 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 946d911..9663294 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -16,20 +16,12 @@
> #include "trace.h"
> #include "qemu-error.h"
> #include "virtio.h"
> +#include "qemu-barrier.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;
> @@ -264,7 +256,7 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
> {
> uint16_t old, new;
> /* Make sure buffer is written before we update index. */
> - wmb();
> + smp_wmb();
> trace_virtqueue_flush(vq, count);
> old = vring_used_idx(vq);
> new = old + count;
> @@ -324,7 +316,7 @@ static unsigned virtqueue_next_desc(target_phys_addr_t desc_pa,
> /* Check they're not leading us off end of descriptors. */
> next = vring_desc_next(desc_pa, i);
> /* Make sure compiler knows to grab that: we don't want it changing! */
> - wmb();
> + smp_wmb();
>
> if (next >= max) {
> error_report("Desc next is %u", next);
> --
> 1.7.5.4
^ permalink raw reply [flat|nested] 5+ messages in thread