Re: [Qemu-devel] [PATCH 1/2] virtio: Use global memory barrier macros

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Sep 20, 2011 at 12:05:20PM +1000, David Gibson wrote:
> The virtio code uses wmb() macros in several places, as required by the
> SMP-aware virtio protocol.  However the wmb() macro is locally defined
> to be a compiler barrier only.  This is probably sufficient on x86
> due to its strong storage ordering model, but it certainly isn't on other
> platforms, such as ppc.
> 
> In any case, qemu already has some globally defined memory barrier macros
> in qemu-barrier.h.  This patch, therefore converts virtio.c to use those
> barrier macros.  The macros in qemu-barrier.h are also wrong (or at least,
> safe for x86 only) but this way at least there's only one place to fix
> them.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/virtio.c |   14 +++-----------
>  1 files changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 946d911..9663294 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -16,20 +16,12 @@
>  #include "trace.h"
>  #include "qemu-error.h"
>  #include "virtio.h"
> +#include "qemu-barrier.h"
>  
>  /* The alignment to use between consumer and producer parts of vring.
>   * 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")
> -
>  typedef struct VRingDesc
>  {
>      uint64_t addr;
> @@ -264,7 +256,7 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
>  {
>      uint16_t old, new;
>      /* Make sure buffer is written before we update index. */
> -    wmb();
> +    smp_wmb();
>      trace_virtqueue_flush(vq, count);
>      old = vring_used_idx(vq);
>      new = old + count;
> @@ -324,7 +316,7 @@ static unsigned virtqueue_next_desc(target_phys_addr_t desc_pa,
>      /* Check they're not leading us off end of descriptors. */
>      next = vring_desc_next(desc_pa, i);
>      /* Make sure compiler knows to grab that: we don't want it changing! */
> -    wmb();
> +    smp_wmb();
>  
>      if (next >= max) {
>          error_report("Desc next is %u", next);
> -- 
> 1.7.5.4