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 <a...@ozlabs.ru>
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
Acked-by: Michael S. Tsirkin <m...@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
