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

[Sasha Levin]

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
find whether __sync_synchronize() provides one as well.

Any idea if it does?

>  
>  typedef struct VRingDesc
>  {

-- 

Sasha.