From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.151; helo=mga17.intel.com; envelope-from=jiewen.yao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 08E792119AC14 for ; Sun, 16 Dec 2018 19:29:21 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Dec 2018 19:29:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,363,1539673200"; d="scan'208";a="118856203" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga002.jf.intel.com with ESMTP; 16 Dec 2018 19:29:21 -0800 Received: from fmsmsx116.amr.corp.intel.com (10.18.116.20) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sun, 16 Dec 2018 19:29:20 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx116.amr.corp.intel.com (10.18.116.20) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sun, 16 Dec 2018 19:29:20 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.182]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.222]) with mapi id 14.03.0415.000; Mon, 17 Dec 2018 11:29:10 +0800 From: "Yao, Jiewen" To: "Gao, Liming" , Ard Biesheuvel , Jagadeesh Ujja , "Leif Lindholm" CC: "edk2-devel@lists.01.org" , "Zhang, Chao B" Thread-Topic: [PATCH 05/13] MdePkg/Library/BaseLib/AArch64: Add AsmLfence function Thread-Index: AQHUlazh9asijuz/jUi0/lqlY+s7lKWCOEig Date: Mon, 17 Dec 2018 03:29:09 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F457847@shsmsx102.ccr.corp.intel.com> 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> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E38D42E@SHSMSX104.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNWVjNmMyYTAtOGI4NC00MDBjLWFjOGEtN2RkNGQxYjlmZWVmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiMitxbldodVk4VDhRZ3JTVFQ0RnZZNnp6RHUzYnRJNitER3hTOERWQ0lxRndzKzJRNmtKaHYrUU8xVUhPVEdxQyJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: request-justification,no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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 03:29:22 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 o= r StoreFence. Maybe it is better to use LoadFence, instead of AsmLFence? Then we can align with MemoryFence. Thank you Yao Jiewen > -----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 >=20 > 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 t= o > extend AsmLfence() API scope for the different ARCHs. If you think it br= ings > the confuse, I just think another way to resolve this case in the caller = code. >=20 > #if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64) > AsmLfence(); > #else > MemoryFence() > #endif >=20 > 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 instructio= ns > that > >> - were issued prior the AsmLfence function. > >> - > >> - Executes a LFENCE instruction. This function is only available on I= A-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 instructio= ns > 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 t= he > >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 t= he > >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 guara= nteed > 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