From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::143; helo=mail-it1-x143.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x143.google.com (mail-it1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 319FF2119AC14 for ; Sun, 16 Dec 2018 23:46:11 -0800 (PST) Received: by mail-it1-x143.google.com with SMTP id c9so18574807itj.1 for ; Sun, 16 Dec 2018 23:46:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=51KKMWU1D+SqL+L43715a+7xXug1w1hf/z6Zqu7/PhA=; b=gPN56f+tZHN4yrSK4mVyZEUh95X8v5H+jH5kt/s10Hwg0qkfhDDL9Xs0slq6BGIsR+ cNv7xWAO0BnCFzTAO32lKyf5cMmKpHdBISFiiSa35W6PAYj7K46xDMEkUjot4DrM/183 nGV3IJ1dXa4aOthFac6Bt+W+CWmpNZzTOx1/8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=51KKMWU1D+SqL+L43715a+7xXug1w1hf/z6Zqu7/PhA=; b=YJme42Vs4Ei0MbBm+gh2dq0Rvhkz7krnxGhGBDsefhT1p2OkLtYWVw8nZMmlU4P4lq 4oD4+Lfa90AsEOSLHK2Hz9CBoVTY4dBUqUYKCXCK/qu1dIaAzycp8DmDbAh5EeA9aU0X m/HSf/AX0Mfv4lksXGv5Sddyrkb7VLlNgQ6r+K5c+mV6ykM9Ybv/tAPf2uMojDRABwIn yS+1HoAIdPicAVM/cxhynrvWcJ/UWK2JmYKVLgE9ynv2Nd7MB/kcPkVWvqGOmHqNzkEs xCWmGvjXEwRrgKDJkzfZbQ1TdpDUmL3rH0nbQFeBdC/PymjiGk6RyAyHySqzDTaR9mDj dEkQ== X-Gm-Message-State: AA+aEWZ0LxwHSkRa1OosISYvqhW2Oae89uUgmrTQpkouORy+YAacoQ8Z 7QyFN5Ntc+n75BvDEGLP31I/URZmdJ3XV/s51mKEag== X-Google-Smtp-Source: AFSGD/V+YckED8kzcTyEc/V+ggcrBwG0H+hgHzh6ESMfH8ccUcngwYRZHmzKc5t+sxd/18S2cp7Rq8BXKIjyBco75PY= X-Received: by 2002:a02:4c9:: with SMTP id 192mr11006019jab.2.1545032770020; Sun, 16 Dec 2018 23:46:10 -0800 (PST) MIME-Version: 1.0 References: <1544789607-11316-1-git-send-email-jagadeesh.ujja@arm.com> <1544789607-11316-6-git-send-email-jagadeesh.ujja@arm.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E38D42E@SHSMSX104.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503F457847@shsmsx102.ccr.corp.intel.com> In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503F457847@shsmsx102.ccr.corp.intel.com> From: Ard Biesheuvel Date: Mon, 17 Dec 2018 08:45:59 +0100 Message-ID: To: "Yao, Jiewen" , Leif Lindholm Cc: "Gao, Liming" , Jagadeesh Ujja , "edk2-devel@lists.01.org" , "Zhang, Chao B" Subject: Re: [PATCH 05/13] MdePkg/Library/BaseLib/AArch64: Add AsmLfence function X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 X-List-Received-Date: Mon, 17 Dec 2018 07:46:11 -0000 Content-Type: text/plain; charset="UTF-8" On Mon, 17 Dec 2018 at 04:29, Yao, Jiewen wrote: > > I think we have below definition. > -- MemoryFence: Serialize load and store operations. > -- LoadFence: Serialize load operations. > -- StoreFence: Serialize store operations. > > According to IA32 SDM, Intel has MFENCE, LFENCE and SFENCE. > If ARM only has DMB, it is possible to use DMB for MemoryFence, LoadFence or StoreFence. > > Maybe it is better to use LoadFence, instead of AsmLFence? > Then we can align with MemoryFence. > I think using AsmLfence() all over the code to limit speculation was a mistake, and I am disappointed nobody from the ARM side was involved at all when these changes were proposed. The code changes rely on specific semantics of the x86 Lfence instructions, i.e., that beyond load serialization, they ensure that all instructions (not just loads) complete before the lfence completes. This is a much stronger notion than a load barrier, and so the abstraction should have been defined as something like a ExecFence() or pipeline barrier etc, and the x86 specific implementation would have been mapped onto Lfence. For the ARM side, we probably need an ISB instruction here as well as some kind of other barrier. Calling it LoadFence() makes no sense whatsoever. > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Gao, Liming > > Sent: Monday, December 17, 2018 10:04 AM > > To: Ard Biesheuvel ; Jagadeesh Ujja > > ; Leif Lindholm > > Cc: edk2-devel@lists.01.org; Zhang, Chao B > > Subject: Re: [edk2] [PATCH 05/13] MdePkg/Library/BaseLib/AArch64: Add > > AsmLfence function > > > > Ard: > > My first comment is to suggest updating the caller code for the arch > > specific code. But, there are two drivers that have the same usage. This > > way will introduce the duplicated code logic. So, I suggest another way to > > extend AsmLfence() API scope for the different ARCHs. If you think it brings > > the confuse, I just think another way to resolve this case in the caller code. > > > > #if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64) > > AsmLfence(); > > #else > > MemoryFence() > > #endif > > > > Thanks > > Liming > > >-----Original Message----- > > >From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > > >Sent: Friday, December 14, 2018 9:54 PM > > >To: Jagadeesh Ujja ; Leif Lindholm > > > > > >Cc: edk2-devel@lists.01.org; Gao, Liming ; Zhang, > > >Chao B > > >Subject: Re: [PATCH 05/13] MdePkg/Library/BaseLib/AArch64: Add > > >AsmLfence function > > > > > >On Fri, 14 Dec 2018 at 13:13, Jagadeesh Ujja > > >wrote: > > >> > > >> Variable service driver includes a call to AsmLfence. To reuse this > > >> driver on AArch64 based platforms, add an implementation of AsmLfence > > >> that acts as a wrapper on the AArch64 specific MemoryFence function. > > >> > > >> Contributed-under: TianoCore Contribution Agreement 1.1 > > >> Signed-off-by: Jagadeesh Ujja > > >> --- > > >> MdePkg/Include/Library/BaseLib.h | 33 +++++++++------ > > >> MdePkg/Library/BaseLib/AArch64/AsmLfence.S | 42 > > >++++++++++++++++++++ > > >> MdePkg/Library/BaseLib/AArch64/AsmLfence.asm | 41 > > >+++++++++++++++++++ > > >> MdePkg/Library/BaseLib/BaseLib.inf | 2 + > > >> 4 files changed, 105 insertions(+), 13 deletions(-) > > >> > > >> diff --git a/MdePkg/Include/Library/BaseLib.h > > >b/MdePkg/Include/Library/BaseLib.h > > >> index 8cc0869..ca961ee 100644 > > >> --- a/MdePkg/Include/Library/BaseLib.h > > >> +++ b/MdePkg/Include/Library/BaseLib.h > > >> @@ -7697,19 +7697,6 @@ AsmWriteTr ( > > >> ); > > >> > > >> /** > > >> - Performs a serializing operation on all load-from-memory instructions > > that > > >> - were issued prior the AsmLfence function. > > >> - > > >> - Executes a LFENCE instruction. This function is only available on IA-32 > > and > > >x64. > > >> - > > >> -**/ > > >> -VOID > > >> -EFIAPI > > >> -AsmLfence ( > > >> - VOID > > >> - ); > > >> - > > >> -/** > > >> Patch the immediate operand of an IA32 or X64 instruction such that > > the > > >byte, > > >> word, dword or qword operand is encoded at the end of the > > instruction's > > >> binary representation. > > >> @@ -7752,4 +7739,24 @@ PatchInstructionX86 ( > > >> ); > > >> > > >> #endif // defined (MDE_CPU_IA32) || defined (MDE_CPU_X64) > > >> + > > >> +#if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64) || defined > > >(MDE_CPU_AARCH64) > > >> + > > >> +/** > > >> + Performs a serializing operation on all load-from-memory instructions > > that > > >> + were issued prior the AsmLfence function. > > >> + > > >> + In case of IA-32 and x64, Executes a LFENCE instruction. > > >> + > > >> + In case of AArch64 this acts as a wrapper on the AArch64 > > >> + specific MemoryFence function > > >> + > > >> +**/ > > >> +VOID > > >> +EFIAPI > > >> +AsmLfence ( > > >> + VOID > > >> + ); > > >> + > > >> +#endif // defined (MDE_CPU_IA32) || defined (MDE_CPU_X64) || > > >defined (MDE_CPU_AARCH64) > > >> #endif // !defined (__BASE_LIB__) > > >> diff --git a/MdePkg/Library/BaseLib/AArch64/AsmLfence.S > > >b/MdePkg/Library/BaseLib/AArch64/AsmLfence.S > > >> new file mode 100644 > > >> index 0000000..2fd804b > > >> --- /dev/null > > >> +++ b/MdePkg/Library/BaseLib/AArch64/AsmLfence.S > > >> @@ -0,0 +1,42 @@ > > >> +##------------------------------------------------------------------------------ > > >> +# > > >> +# AsmLfence() for AArch64 > > >> +# > > >> +# Copyright (c) 2013-2018, ARM Ltd. All rights reserved. > > >> +# > > >> +# This program and the accompanying materials > > >> +# are licensed and made available under the terms and conditions of the > > >BSD License > > >> +# which accompanies this distribution. The full text of the license may > > be > > >found at > > >> +# http://opensource.org/licenses/bsd-license.php. > > >> +# > > >> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS > > IS" > > >BASIS, > > >> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > > >EXPRESS OR IMPLIED. > > >> +# > > >> +##------------------------------------------------------------------------------ > > >> + > > >> +.text > > >> +.p2align 2 > > >> + > > >> +GCC_ASM_EXPORT(AsmLfence) > > >> + > > >> +# IMPORT > > >> +GCC_ASM_IMPORT(MemoryFence) > > >> + > > >> +#/** > > >> +# Used to serialize load and store operations. > > >> +# > > >> +# All loads and stores that proceed calls to this function are > > guaranteed to > > >be > > >> +# globally visible when this function returns. > > >> +# > > >> +#**/ > > >> +#VOID > > >> +#EFIAPI > > >> +#AsmLfence ( > > >> +# VOID > > >> +# ); > > >> +# > > >> +ASM_PFX(AsmLfence): > > >> + stp x29, x30, [sp, #-16]! > > >> + bl MemoryFence > > >> + ldp x29, x30, [sp], #0x10 > > >> + ret > > > > > >Any reason we can't simply do > > > > > >b MemoryFence > > > > > >here? > > > > > >Also, why I understand the rationale, I still think it would be better > > >to change callers of the [x86 specific] AsmLfence() than to introduce > > >an alias of MemoryFence() for architectures where Lfence is not > > >defined. > > > > > >This is not only about tidiness, but also about potentially having > > >different semantics, which we can't provide in general on ARM, but > > >only in particular cases [such as the code that is modified in this > > >series] > > > > > >In other words, newly introduced occurrences of AsmLfence() now have > > >to be audited for being appropriate on AArc64 if they are added to > > >generic code. > > > > > > > > >> diff --git a/MdePkg/Library/BaseLib/AArch64/AsmLfence.asm > > >b/MdePkg/Library/BaseLib/AArch64/AsmLfence.asm > > >> new file mode 100644 > > >> index 0000000..7dd5659 > > >> --- /dev/null > > >> +++ b/MdePkg/Library/BaseLib/AArch64/AsmLfence.asm > > >> @@ -0,0 +1,41 @@ > > >> +;------------------------------------------------------------------------------ > > >> +; > > >> +; AsmLfence() for AArch64 > > >> +; > > >> +; Copyright (c) 2013-2018, ARM Ltd. All rights reserved. > > >> +; > > >> +; This program and the accompanying materials > > >> +; are licensed and made available under the terms and conditions of the > > >BSD License > > >> +; which accompanies this distribution. The full text of the license may > > be > > >found at > > >> +; http://opensource.org/licenses/bsd-license.php. > > >> +; > > >> +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS > > IS" > > >BASIS, > > >> +; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > > >EXPRESS OR IMPLIED. > > >> +; > > >> +;------------------------------------------------------------------------------ > > >> + > > >> + EXPORT AsmLfence > > >> + AREA BaseLib_LowLevel, CODE, READONLY > > >> + # IMPORT > > >> + GCC_ASM_IMPORT(MemoryFence) > > >> + > > >> +;/** > > >> +; Used to serialize load and store operations. > > >> +; > > >> +; All loads and stores that proceed calls to this function are guaranteed > > to > > >be > > >> +; globally visible when this function returns. > > >> +; > > >> +;**/ > > >> +;VOID > > >> +;EFIAPI > > >> +;AsmLfence ( > > >> +; VOID > > >> +; ); > > >> +; > > >> +AsmLfence > > >> + stp x29, x30, [sp, #-16]! > > >> + bl MemoryFence > > >> + ldp x29, x30, [sp], #0x10 > > >> + ret > > >> + > > >> + END > > >> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf > > >b/MdePkg/Library/BaseLib/BaseLib.inf > > >> index b84e583..b7d7bcb 100644 > > >> --- a/MdePkg/Library/BaseLib/BaseLib.inf > > >> +++ b/MdePkg/Library/BaseLib/BaseLib.inf > > >> @@ -585,6 +585,7 @@ > > >> Math64.c > > >> > > >> AArch64/MemoryFence.S | GCC > > >> + AArch64/AsmLfence.S | GCC > > >> AArch64/SwitchStack.S | GCC > > >> AArch64/EnableInterrupts.S | GCC > > >> AArch64/DisableInterrupts.S | GCC > > >> @@ -593,6 +594,7 @@ > > >> AArch64/CpuBreakpoint.S | GCC > > >> > > >> AArch64/MemoryFence.asm | MSFT > > >> + AArch64/AsmLfence.asm | MSFT > > >> AArch64/SwitchStack.asm | MSFT > > >> AArch64/EnableInterrupts.asm | MSFT > > >> AArch64/DisableInterrupts.asm | MSFT > > >> -- > > >> 2.7.4 > > >> > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel