[1/2] virtio: Use global memory barrier macros

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>
---
 hw/virtio.c |   14 +++-----------
 1 files changed, 3 insertions(+), 11 deletions(-)

On 09/19/2011 09:05 PM, 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>

Applied all.  Thanks.

Regards,

Anthony Liguori

> ---
>   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);

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