From patchwork Thu Sep 1 06:09:49 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Gibson X-Patchwork-Id: 112819 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id D87E9B6F7C for ; Thu, 1 Sep 2011 16:10:23 +1000 (EST) Received: from localhost ([::1]:37990 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qz0Ta-0000U5-K5 for incoming@patchwork.ozlabs.org; Thu, 01 Sep 2011 02:10:10 -0400 Received: from eggs.gnu.org ([140.186.70.92]:51062) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qz0TU-0000Se-Iv for qemu-devel@nongnu.org; Thu, 01 Sep 2011 02:10:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qz0TT-0007sM-3y for qemu-devel@nongnu.org; Thu, 01 Sep 2011 02:10:04 -0400 Received: from ozlabs.org ([203.10.76.45]:60239) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qz0TS-0007qc-L4 for qemu-devel@nongnu.org; Thu, 01 Sep 2011 02:10:03 -0400 Received: by ozlabs.org (Postfix, from userid 1007) id AEF7BB6F76; Thu, 1 Sep 2011 16:09:58 +1000 (EST) From: David Gibson To: aliguori@us.ibm.com Date: Thu, 1 Sep 2011 16:09:49 +1000 Message-Id: <1314857389-13363-1-git-send-email-david@gibson.dropbear.id.au> X-Mailer: git-send-email 1.7.5.4 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 203.10.76.45 Cc: aik@ozlabs.ru, pbonzini@redhat.com, rusty@rustcorp.com.au, qemu-devel@nongnu.org, agraf@suse.de Subject: [Qemu-devel] [PATCH] virtio: Make memory barriers be memory barriers X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 Signed-off-by: David Gibson Acked-by: Paolo Bonzini --- 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() typedef struct VRingDesc {