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