Re: [Qemu-devel] [PATCH 2/2] Barriers in qemu-barrier.h should not be x86 specific

[Michael S. Tsirkin]

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 <address@hidden>
> Signed-off-by: David Gibson <address@hidden>


Acked-by: Michael S. Tsirkin <address@hidden>

> ---
>  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