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
find whether __sync_synchronize() provides one as well.
Any idea if it does?
>
> typedef struct VRingDesc
> {
--
Sasha.
