From patchwork Wed Mar 26 13:38:46 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ian Campbell X-Patchwork-Id: 27134 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-pa0-f69.google.com (mail-pa0-f69.google.com [209.85.220.69]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 3882920062 for ; Wed, 26 Mar 2014 13:44:56 +0000 (UTC) Received: by mail-pa0-f69.google.com with SMTP id fb1sf4703498pad.8 for ; Wed, 26 Mar 2014 06:44:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:from:to:date:message-id:in-reply-to :references:mime-version:cc:subject:precedence:list-id :list-unsubscribe:list-post:list-help:list-subscribe:sender :errors-to:x-original-sender:x-original-authentication-results :mailing-list:list-archive:content-type:content-transfer-encoding; bh=4LVddO0y5cUvmghoCdUDbw9DPSWHoLMwVGf/DX0DaXQ=; b=cp3i8P4uy8jdWPJav+RiuunLZaUkS+0Ok/1ppeU0PUZonRpxn7I6mgz0AHkC3jopRt PCpn/iRxDA+5kEN7GcK49ZL1FYqt1HFU6Lie70R3vdQyTYRFORRxEGErrY+nbsQlBFe/ Mw6J0o8M5vnlFA4sYUIgQ1Wcs7BGTnlAmmT7IDhCmU+FdN1vZls+EBwoqT1NIJF7M1f3 //6L89qoQx0+5d+itF9NhCLzQ124IhY4kuj5askdkaxH2RkAMLGfj6IneF2fq7KNkRJb 4ACr6UcgipD2h7fDJy03Ej4DCuJC2k9yeByxmA2GywYHsDF72HpShHRLmpwFIu+4YDvW l7ww== X-Gm-Message-State: ALoCoQnUoYJ4jMUH90JBa5BXrO+HivPZs11Lg1Lsc/64LodZYMFvG3vWVN44F7H+UqaXQe+7+K6C X-Received: by 10.66.141.231 with SMTP id rr7mr1567959pab.47.1395841495486; Wed, 26 Mar 2014 06:44:55 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.43.134 with SMTP id e6ls525019qga.17.gmail; Wed, 26 Mar 2014 06:44:55 -0700 (PDT) X-Received: by 10.52.51.226 with SMTP id n2mr82862vdo.57.1395841495297; Wed, 26 Mar 2014 06:44:55 -0700 (PDT) Received: from mail-ve0-f172.google.com (mail-ve0-f172.google.com [209.85.128.172]) by mx.google.com with ESMTPS id u5si4625377vdo.184.2014.03.26.06.44.55 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 26 Mar 2014 06:44:55 -0700 (PDT) Received-SPF: neutral (google.com: 209.85.128.172 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) client-ip=209.85.128.172; Received: by mail-ve0-f172.google.com with SMTP id jx11so2352576veb.3 for ; Wed, 26 Mar 2014 06:44:55 -0700 (PDT) X-Received: by 10.58.66.195 with SMTP id h3mr83742vet.57.1395841495190; Wed, 26 Mar 2014 06:44:55 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.220.78.9 with SMTP id i9csp47443vck; Wed, 26 Mar 2014 06:44:54 -0700 (PDT) X-Received: by 10.140.22.148 with SMTP id 20mr62414768qgn.0.1395841494630; Wed, 26 Mar 2014 06:44:54 -0700 (PDT) Received: from lists.xen.org (lists.xen.org. [50.57.142.19]) by mx.google.com with ESMTPS id z66si6994021qgd.180.2014.03.26.06.44.54 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Wed, 26 Mar 2014 06:44:54 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of xen-devel-bounces@lists.xen.org designates 50.57.142.19 as permitted sender) client-ip=50.57.142.19; Received: from localhost ([127.0.0.1] helo=lists.xen.org) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WSo6t-0006JH-HL; Wed, 26 Mar 2014 13:43:15 +0000 Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WSo6r-0006I7-SI for xen-devel@lists.xen.org; Wed, 26 Mar 2014 13:43:14 +0000 Received: from [193.109.254.147:60904] by server-16.bemta-14.messagelabs.com id 43/07-16986-179D2335; Wed, 26 Mar 2014 13:43:13 +0000 X-Env-Sender: Ian.Campbell@citrix.com X-Msg-Ref: server-11.tower-27.messagelabs.com!1395841389!4263670!1 X-Originating-IP: [66.165.176.63] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogNjYuMTY1LjE3Ni42MyA9PiAzMDYwNDg=\n X-StarScan-Received: X-StarScan-Version: 6.11.1; banners=-,-,- X-VirusChecked: Checked Received: (qmail 30176 invoked from network); 26 Mar 2014 13:43:12 -0000 Received: from smtp02.citrix.com (HELO SMTP02.CITRIX.COM) (66.165.176.63) by server-11.tower-27.messagelabs.com with RC4-SHA encrypted SMTP; 26 Mar 2014 13:43:12 -0000 X-IronPort-AV: E=Sophos;i="4.97,735,1389744000"; d="scan'208";a="113734864" Received: from accessns.citrite.net (HELO FTLPEX01CL01.citrite.net) ([10.9.154.239]) by FTLPIPO02.CITRIX.COM with ESMTP; 26 Mar 2014 13:43:09 +0000 Received: from norwich.cam.xci-test.com (10.80.248.129) by smtprelay.citrix.com (10.13.107.78) with Microsoft SMTP Server id 14.2.342.4; Wed, 26 Mar 2014 09:43:07 -0400 Received: from drall.uk.xensource.com ([10.80.16.71] helo=drall.uk.xensource.com.) by norwich.cam.xci-test.com with esmtp (Exim 4.72) (envelope-from ) id 1WSo2f-00074X-Tr; Wed, 26 Mar 2014 13:38:53 +0000 From: Ian Campbell To: Date: Wed, 26 Mar 2014 13:38:46 +0000 Message-ID: <1395841133-2223-11-git-send-email-ian.campbell@citrix.com> X-Mailer: git-send-email 1.7.10.4 In-Reply-To: <1395841009.12547.11.camel@kazak.uk.xensource.com> References: <1395841009.12547.11.camel@kazak.uk.xensource.com> MIME-Version: 1.0 X-DLP: MIA2 Cc: julien.grall@linaro.org, tim@xen.org, Ian Campbell , stefano.stabellini@eu.citrix.com Subject: [Xen-devel] [PATCH v2 11/17] xen: arm64: atomics: fix use of acquire + release for full barrier semantics X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: List-Unsubscribe: , List-Post: , List-Help: , List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: ian.campbell@citrix.com X-Original-Authentication-Results: mx.google.com; spf=neutral (google.com: 209.85.128.172 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Archive: Xen, like Linux, expects full barrier semantics for bitops, atomics and cmpxchgs. This issue was discovered on Linux and we get our implementation of these from Linux so quoting Will Deacon in Linux commit 8e86f0b409a4 for the gory details: Linux requires a number of atomic operations to provide full barrier semantics, that is no memory accesses after the operation can be observed before any accesses up to and including the operation in program order. On arm64, these operations have been incorrectly implemented as follows: // A, B, C are independent memory locations // atomic_op (B) 1: ldaxr x0, [B] // Exclusive load with acquire stlxr w1, x0, [B] // Exclusive store with release cbnz w1, 1b The assumption here being that two half barriers are equivalent to a full barrier, so the only permitted ordering would be A -> B -> C (where B is the atomic operation involving both a load and a store). Unfortunately, this is not the case by the letter of the architecture and, in fact, the accesses to A and C are permitted to pass their nearest half barrier resulting in orderings such as Bl -> A -> C -> Bs or Bl -> C -> A -> Bs (where Bl is the load-acquire on B and Bs is the store-release on B). This is a clear violation of the full barrier requirement. The simple way to fix this is to implement the same algorithm as ARMv7 using explicit barriers: // atomic_op (B) dmb ish // Full barrier 1: ldxr x0, [B] // Exclusive load stxr w1, x0, [B] // Exclusive store cbnz w1, 1b dmb ish // Full barrier but this has the undesirable effect of introducing *two* full barrier instructions. A better approach is actually the following, non-intuitive sequence: // atomic_op (B) 1: ldxr x0, [B] // Exclusive load stlxr w1, x0, [B] // Exclusive store with release cbnz w1, 1b dmb ish // Full barrier The simple observations here are: - The dmb ensures that no subsequent accesses (e.g. the access to C) can enter or pass the atomic sequence. - The dmb also ensures that no prior accesses (e.g. the access to A) can pass the atomic sequence. - Therefore, no prior access can pass a subsequent access, or vice-versa (i.e. A is strictly ordered before C). - The stlxr ensures that no prior access can pass the store component of the atomic operation. The only tricky part remaining is the ordering between the ldxr and the access to A, since the absence of the first dmb means that we're now permitting re-ordering between the ldxr and any prior accesses. From an (arbitrary) observer's point of view, there are two scenarios: 1. We have observed the ldxr. This means that if we perform a store to [B], the ldxr will still return older data. If we can observe the ldxr, then we can potentially observe the permitted re-ordering with the access to A, which is clearly an issue when compared to the dmb variant of the code. Thankfully, the exclusive monitor will save us here since it will be cleared as a result of the store and the ldxr will retry. Notice that any use of a later memory observation to imply observation of the ldxr will also imply observation of the access to A, since the stlxr/dmb ensure strict ordering. 2. We have not observed the ldxr. This means we can perform a store and influence the later ldxr. However, that doesn't actually tell us anything about the access to [A], so we've not lost anything here either when compared to the dmb variant. This patch implements this solution for our barriered atomic operations, ensuring that we satisfy the full barrier requirements where they are needed. Cc: Cc: Peter Zijlstra Signed-off-by: Will Deacon Signed-off-by: Catalin Marinas Signed-off-by: Ian Campbell Acked-by: Julien Grall --- xen/arch/arm/arm64/lib/bitops.S | 3 +- xen/include/asm-arm/arm64/atomic.h | 13 +++++--- xen/include/asm-arm/arm64/system.h | 61 ++++++++++++++++++------------------ 3 files changed, 42 insertions(+), 35 deletions(-) diff --git a/xen/arch/arm/arm64/lib/bitops.S b/xen/arch/arm/arm64/lib/bitops.S index 80cc903..e1ad239 100644 --- a/xen/arch/arm/arm64/lib/bitops.S +++ b/xen/arch/arm/arm64/lib/bitops.S @@ -46,11 +46,12 @@ ENTRY( \name ) mov x2, #1 add x1, x1, x0, lsr #3 // Get word offset lsl x4, x2, x3 // Create mask -1: ldaxr w2, [x1] +1: ldxr w2, [x1] lsr w0, w2, w3 // Save old value of bit \instr w2, w2, w4 // toggle bit stlxr w5, w2, [x1] cbnz w5, 1b + dmb ish and w0, w0, #1 3: ret ENDPROC(\name ) diff --git a/xen/include/asm-arm/arm64/atomic.h b/xen/include/asm-arm/arm64/atomic.h index 6b37945..3f37ed5 100644 --- a/xen/include/asm-arm/arm64/atomic.h +++ b/xen/include/asm-arm/arm64/atomic.h @@ -48,7 +48,7 @@ static inline int atomic_add_return(int i, atomic_t *v) int result; asm volatile("// atomic_add_return\n" -"1: ldaxr %w0, %2\n" +"1: ldxr %w0, %2\n" " add %w0, %w0, %w3\n" " stlxr %w1, %w0, %2\n" " cbnz %w1, 1b" @@ -56,6 +56,7 @@ static inline int atomic_add_return(int i, atomic_t *v) : "Ir" (i) : "cc", "memory"); + smp_mb(); return result; } @@ -80,7 +81,7 @@ static inline int atomic_sub_return(int i, atomic_t *v) int result; asm volatile("// atomic_sub_return\n" -"1: ldaxr %w0, %2\n" +"1: ldxr %w0, %2\n" " sub %w0, %w0, %w3\n" " stlxr %w1, %w0, %2\n" " cbnz %w1, 1b" @@ -88,6 +89,7 @@ static inline int atomic_sub_return(int i, atomic_t *v) : "Ir" (i) : "cc", "memory"); + smp_mb(); return result; } @@ -96,17 +98,20 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new) unsigned long tmp; int oldval; + smp_mb(); + asm volatile("// atomic_cmpxchg\n" -"1: ldaxr %w1, %2\n" +"1: ldxr %w1, %2\n" " cmp %w1, %w3\n" " b.ne 2f\n" -" stlxr %w0, %w4, %2\n" +" stxr %w0, %w4, %2\n" " cbnz %w0, 1b\n" "2:" : "=&r" (tmp), "=&r" (oldval), "+Q" (ptr->counter) : "Ir" (old), "r" (new) : "cc", "memory"); + smp_mb(); return oldval; } diff --git a/xen/include/asm-arm/arm64/system.h b/xen/include/asm-arm/arm64/system.h index 570af5c..0db96e0 100644 --- a/xen/include/asm-arm/arm64/system.h +++ b/xen/include/asm-arm/arm64/system.h @@ -8,49 +8,50 @@ static inline unsigned long __xchg(unsigned long x, volatile void *ptr, int size { unsigned long ret, tmp; - switch (size) { - case 1: - asm volatile("// __xchg1\n" - "1: ldaxrb %w0, %2\n" - " stlxrb %w1, %w3, %2\n" - " cbnz %w1, 1b\n" - : "=&r" (ret), "=&r" (tmp), "+Q" (*(u8 *)ptr) + switch (size) { + case 1: + asm volatile("// __xchg1\n" + "1: ldxrb %w0, %2\n" + " stlxrb %w1, %w3, %2\n" + " cbnz %w1, 1b\n" + : "=&r" (ret), "=&r" (tmp), "+Q" (*(u8 *)ptr) : "r" (x) : "cc", "memory"); - break; - case 2: - asm volatile("// __xchg2\n" - "1: ldaxrh %w0, %2\n" - " stlxrh %w1, %w3, %2\n" - " cbnz %w1, 1b\n" - : "=&r" (ret), "=&r" (tmp), "+Q" (*(u16 *)ptr) + break; + case 2: + asm volatile("// __xchg2\n" + "1: ldxrh %w0, %2\n" + " stlxrh %w1, %w3, %2\n" + " cbnz %w1, 1b\n" + : "=&r" (ret), "=&r" (tmp), "+Q" (*(u16 *)ptr) : "r" (x) : "cc", "memory"); - break; - case 4: - asm volatile("// __xchg4\n" - "1: ldaxr %w0, %2\n" - " stlxr %w1, %w3, %2\n" - " cbnz %w1, 1b\n" - : "=&r" (ret), "=&r" (tmp), "+Q" (*(u32 *)ptr) + break; + case 4: + asm volatile("// __xchg4\n" + "1: ldxr %w0, %2\n" + " stlxr %w1, %w3, %2\n" + " cbnz %w1, 1b\n" + : "=&r" (ret), "=&r" (tmp), "+Q" (*(u32 *)ptr) : "r" (x) : "cc", "memory"); - break; - case 8: - asm volatile("// __xchg8\n" - "1: ldaxr %0, %2\n" - " stlxr %w1, %3, %2\n" - " cbnz %w1, 1b\n" - : "=&r" (ret), "=&r" (tmp), "+Q" (*(u64 *)ptr) + break; + case 8: + asm volatile("// __xchg8\n" + "1: ldxr %0, %2\n" + " stlxr %w1, %3, %2\n" + " cbnz %w1, 1b\n" + : "=&r" (ret), "=&r" (tmp), "+Q" (*(u64 *)ptr) : "r" (x) : "cc", "memory"); break; default: __bad_xchg(ptr, size), ret = 0; break; - } + } - return ret; + smp_mb(); + return ret; } #define xchg(ptr,x) \