From patchwork Mon May 21 01:56:45 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Benjamin Herrenschmidt X-Patchwork-Id: 160305 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 8B86BB6FA2 for ; Mon, 21 May 2012 11:57:08 +1000 (EST) Received: from localhost ([::1]:54840 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SWHru-00051Q-Fi for incoming@patchwork.ozlabs.org; Sun, 20 May 2012 21:57:06 -0400 Received: from eggs.gnu.org ([208.118.235.92]:56757) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SWHrm-00050v-El for qemu-devel@nongnu.org; Sun, 20 May 2012 21:56:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SWHrk-0007OV-9v for qemu-devel@nongnu.org; Sun, 20 May 2012 21:56:58 -0400 Received: from gate.crashing.org ([63.228.1.57]:40865) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SWHrk-0007OI-0G for qemu-devel@nongnu.org; Sun, 20 May 2012 21:56:56 -0400 Received: from [127.0.0.1] (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.13.8) with ESMTP id q4L1ujF8016571; Sun, 20 May 2012 20:56:46 -0500 Message-ID: <1337565405.2458.12.camel@pasglop> From: Benjamin Herrenschmidt To: Paolo Bonzini Date: Mon, 21 May 2012 11:56:45 +1000 In-Reply-To: <1337549768.2458.0.camel@pasglop> References: <1336625347-10169-1-git-send-email-benh@kernel.crashing.org> <1336625347-10169-14-git-send-email-benh@kernel.crashing.org> <4FB1A8BF.7060503@codemonkey.ws> <20120515014449.GF30229@truffala.fritz.box> <1337142938.6727.122.camel@pasglop> <4FB4028F.7070003@codemonkey.ws> <1337213257.30558.22.camel@pasglop> <1337214293.30558.25.camel@pasglop> <4FB5F1FD.9020009@redhat.com> <1337329136.2513.7.camel@pasglop> <4FB60EFF.6070205@redhat.com> <1337379992.2513.17.camel@pasglop> <4FB74AB0.7090608@redhat.com> <1337549768.2458.0.camel@pasglop> X-Mailer: Evolution 3.2.3-0ubuntu6 Mime-Version: 1.0 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 63.228.1.57 Cc: David Gibson , qemu-devel@nongnu.org, Anthony Liguori , "Michael S. Tsirkin" Subject: [Qemu-devel] [PATCH] Add a memory barrier to guest memory access functions 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 emulated devices can run simultaneously with the guest, so we need to be careful with ordering of load and stores done by them to the guest system memory, which need to be observed in the right order by the guest operating system. This adds barriers to some standard guest memory access functions along with a comment explaining the semantics to exec.c Signed-off-by: Benjamin Herrenschmidt --- So after the discussion with Paolo, I removed the specific accessors, just used a normal smp_mb() in only two places, cpu_physical_memory_rw and cpu_physical_memory_map. I don't see an obvious need to provide a "relaxed" variant of the later at this stage, a quick grep doesn't seem to show that most cases where it's used are either not performance sensitive or the barrier makes sense, but feel free to prove me wrong :-) If we really want that, my suggestion would be to change the "is_write" flag into a proper bitmask of direction and relaxed attribute (which we can use for more attributes in the future if needed). Also, we probably want an smp_mb() when shooting MSIs (not LSIs, those are not ordered, that's why the guest driver needs to do an MMIO read after an LSI, but MSIs are). I haven't looked at that yet, we can do it from a separate patch if needed. exec.c | 37 +++++++++++++++++++++++++++++++++++++ 1 files changed, 37 insertions(+), 0 deletions(-) diff --git a/exec.c b/exec.c index 363ec98..997dbb0 100644 --- a/exec.c +++ b/exec.c @@ -16,6 +16,34 @@ * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, see . */ + +/* + * Note on memory barriers usage: + * + * In order for emulated devices "DMA" operations to appear + * with a consistent ordering to the guest, we provide some + * amount of ordering guarantees: + * + * cpy_physical_memory_rw() (and all functions using it) is + * ordered vs. all previous accesses (it begins with a full + * memory barrier) + * + * This include all the new dma_* accessors. + * + * The old variants of ld* and st* that have not been convered + * to dma_ are not ordered. Users are reponsible for using their + * own ordering. + * + * cpu_physical_memory_map() provides a memory barrier vs. all + * previous accesses. There is no implicit barrier on unmap. + * If ordering is required between accessed within the map/unmmap + * sequence, then it needs to be done explicitely. + * + * This means that a typical block driver using map/unmap accross + * the transfer of a block followed by dma_ writes to signal + * completion or interrupt shouldn't require the addition of + * explicit barriers. + */ #include "config.h" #ifdef _WIN32 #include @@ -25,6 +53,7 @@ #endif #include "qemu-common.h" +#include "qemu-barrier.h" #include "cpu.h" #include "tcg.h" #include "hw/hw.h" @@ -3516,6 +3545,10 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf, target_phys_addr_t page; MemoryRegionSection *section; + /* Provides ordering vs. previous accesses, see comments + * at the top of this file */ + smp_mb(); + while (len > 0) { page = addr & TARGET_PAGE_MASK; l = (page + TARGET_PAGE_SIZE) - addr; @@ -3713,6 +3746,10 @@ void *cpu_physical_memory_map(target_phys_addr_t addr, ram_addr_t rlen; void *ret; + /* Provides ordering vs. previous accesses, see comments + * at the top of this file */ + smp_mb(); + while (len > 0) { page = addr & TARGET_PAGE_MASK; l = (page + TARGET_PAGE_SIZE) - addr;