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

[Anthony Liguori]

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<address@hidden>
> Signed-off-by: David Gibson<address@hidden>

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