From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:33218) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RAIuF-00058i-H1 for qemu-devel@nongnu.org; Sun, 02 Oct 2011 06:04:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RAIuE-0008TG-DC for qemu-devel@nongnu.org; Sun, 02 Oct 2011 06:04:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:14178) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RAIuE-0008TC-4b for qemu-devel@nongnu.org; Sun, 02 Oct 2011 06:04:22 -0400 Date: Sun, 2 Oct 2011 12:05:27 +0200 From: "Michael S. Tsirkin" Message-ID: <20111002100527.GB30747@redhat.com> References: <1316484321-6726-1-git-send-email-david@gibson.dropbear.id.au> <1316484321-6726-2-git-send-email-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1316484321-6726-2-git-send-email-david@gibson.dropbear.id.au> Subject: Re: [Qemu-devel] [PATCH 2/2] Barriers in qemu-barrier.h should not be x86 specific List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: aliguori@us.ibm.com, aik@ozlabs.ru, rusty@rustcorp.com.au, agraf@suse.de, qemu-devel@nongnu.org, avi@redhat.com, pbonzini@redhat.com 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 > Signed-off-by: David Gibson Acked-by: Michael S. Tsirkin > --- > 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