Re: [Qemu-devel] [PATCH] virtio: Make memory barriers be memory barriers

[Sasha Levin]

On Thu, 2011-09-01 at 10:37 +0300, Sasha Levin wrote:
> On Thu, 2011-09-01 at 16:09 +1000, David Gibson wrote:
> > The virtio code already has memory barrier wmb() macros in the code.
> > However they are was defined as no-ops.  The comment claims that real
> > barriers are not necessary because the code does not run concurrent.
> > However, with kvm and io-thread enabled, this is not true and this qemu
> > code can indeed run concurrently with the guest kernel.  This does not
> > cause problems on x86 due to it's strongly ordered storage model, but it
> > causes a race leading to virtio errors on POWER which has a relaxed storage
> > ordering model.
> > 
> > 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.
> > 
> > The problem is easy to reproduce on POWER using virtio-net with heavy
> > traffic.
> > 
> > The patch defines wmb() as __sync_synchronize(), a cross platform memory
> > barrier primitive available in sufficiently recent gcc versions (gcc 4.2
> > and after?).  If we care about older gccs then this patch will need to
> > be updated with some sort of fallback.
> > 
> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/virtio.c |   10 ++--------
> >  1 files changed, 2 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/virtio.c b/hw/virtio.c
> > index 13aa0fa..c9f0e75 100644
> > --- a/hw/virtio.c
> > +++ b/hw/virtio.c
> > @@ -21,14 +21,8 @@
> >   * 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")
> > + /* TODO: we may also need rmb()s.  It hasn't bitten us yet, but.. */
> > + #define wmb() __sync_synchronize()
> 
> That asm directive also implicitly provided a compiler barrier, I could

couldn't.

> find whether __sync_synchronize() provides one as well.
> 
> Any idea if it does?
> 
> >  
> >  typedef struct VRingDesc
> >  {
> 

-- 

Sasha.