From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=jagadeesh.ujja@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id 24D2421A00AE6 for ; Wed, 2 Jan 2019 05:19:35 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 7EB1C80D for ; Wed, 2 Jan 2019 05:19:35 -0800 (PST) Received: from mail-it1-f179.google.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 56F903F5CF for ; Wed, 2 Jan 2019 05:19:35 -0800 (PST) Received: by mail-it1-f179.google.com with SMTP id g76so40785948itg.2 for ; Wed, 02 Jan 2019 05:19:35 -0800 (PST) X-Gm-Message-State: AA+aEWbeMS6Kuue9+A6QPvxxkB2e/2tIFW093nv0vUUWjI2qS7YvgGRr tT3iBGZBzcL7ZCe4lJdt8hU44P7a0QQhs5Cxurw= X-Google-Smtp-Source: ALg8bN5+GCKCds2TrARhQUwU1Yl+pb/xw8S3uYESLRkyE8bDykWAro4HYEkG/qhPQQpAwDFrfmxmZ3xKTnOOrXgoZO0= X-Received: by 2002:a24:f8c2:: with SMTP id a185mr23134530ith.145.1546435174537; Wed, 02 Jan 2019 05:19:34 -0800 (PST) MIME-Version: 1.0 References: <1544789607-11316-1-git-send-email-jagadeesh.ujja@arm.com> In-Reply-To: From: Jagadeesh Ujja Date: Wed, 2 Jan 2019 18:49:23 +0530 X-Gmail-Original-Message-ID: Message-ID: To: "Wang, Jian J" Cc: "edk2-devel@lists.01.org" , "Gao, Liming" , "Zhang, Chao B" , "leif.lindholm@linaro.org" , "ard.biesheuvel@linaro.org" Subject: Re: [PATCH 00/13] Extend secure variable service to be usable from Standalone MM 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: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 X-List-Received-Date: Wed, 02 Jan 2019 13:19:36 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable hi Jian, On Fri, Dec 21, 2018 at 8:27 AM Wang, Jian J wrote: > > Jagadeesh, > > There're many places in this patch series where code similar to following= is added. > It'd better to wrap them into module private functions or even a library,= if necessary. > This can make the code cleaner (no if/else) and easier (central place) to= maintain in > the future. > > + if (!PcdGetBool (PcdStandaloneMmVariableEnabled)) { > + Status =3D gSmst->SmmLocateProtocol ( > + &gEfiSmmSwapAddressRangeProtocolGuid, > + NULL, > + SarProtocol > + ); > + } else { > + Status =3D gMmst->MmLocateProtocol ( > + &gEfiSmmSwapAddressRangeProtocolGuid, > + NULL, > + SarProtocol > + ); > + } > Thank you for your comment. This patch series try to reuse code as much as possible between MM and non-MM code. So, in some changes, if..else was used which helps to reuse most of the other bits of code. To address your comment, can you please let me know how we could avoid this if..else without duplicating the too much code. I am not clear about " module private functions or even a library" comment that you have made. Can you please help me with this. Thanks, Jagadeesh. > Regards, > Jian > > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Jagadeesh Ujja > > Sent: Friday, December 14, 2018 8:13 PM > > To: edk2-devel@lists.01.org; Gao, Liming ; Zhang,= Chao > > B ; leif.lindholm@linaro.org; > > ard.biesheuvel@linaro.org > > Subject: [edk2] [PATCH 00/13] Extend secure variable service to be usab= le from > > Standalone MM > > > > Changes since RFC v4: > > - Addressed all the comments from Liming Gao > > - Added an additional PCD 'PcdStandaloneMmCodeEnabled' to indicate > > presence of StandaloneMM support. > > - MdePkg.dec file updated to include StandaloneMmServiceTableLib and > > StandaloneMmRuntimeDxe library. > > - Platform specific changes will be posted in a seperate patchset. > > - AsmLfence wrapper function is supported for AArch64 platforms. > > - All the patches in this series can be pulled from > > https://github.com/jagadeeshujja/edk2 (branch: topics/aarch64_secur= e_vars) > > > > Changes since RFC v3: > > - Addressed all the comments from Liming Gao > > - Added a AArch64 implementation of AsmLfence which is a wrapper for > > MemoryFence. The changes in variable service driver in v3 of this > > patchset that used MemoryFence instead of AsmLfence have been remov= ed. > > - Added StandaloneMmServicesTableLib.h and StandaloneMmRuntimeDxe > > library into MdePkg. > > - Renamed PcdStandaloneMmEnable as PcdStandaloneMmVariableEnabled and > > added to in to MdePkg. > > - Now with above changes, edk2 packages don't need to depend on > > StandaloneMmPkg/StandaloneMmPkg.dec > > - Addressed comments from Ting Ye > > - Removed the hacks in the v3 version. > > - Will relook into the =E2=80=9CTimerWrapp.c=E2=80=9D file and add a = appropriate > > implementation of this for MM Standalone mode code. > > > > Changes since RFC v2: > > - Added 'Contributed-under' tag, removed Change-ID tag and > > maintained a single signed-off-by for the all the patches. > > > > Changes since RFC v1: > > - Addressed all the comments from Liming Gao > > - Removed the use of #ifdef/#else/#endif and used a Pcd instead to > > select between MM and non-MM paths. > > - Removed all dependencies on edk2-platforms. > > - Dropped the use of mMmst and used gSmst instead. > > - Added a dummy implementation UefiRuntimeServiceTableLib for > > MM_STANDALONE usage > > - Replaced all uses of AsmLfence with MemoryFence from variable > > service code. > > - Add a new StandaloneMmRuntimeDxe library to for use by non-MM code. > > > > This patch series extends the existing secure variable service support = for > > use with Standalone MM. This is applicable to paltforms that use Standa= lone > > Management Mode to protect access to non-volatile memory (NOR flash in = case > > of these patches) used to store the secure EFI variables. > > > > The first patch pulls in additional libraries from the staging branch o= f > > StandaloneMmPkg into the edk2's StandaloneMmPkg. The existing secure > > variable > > service implementation supports only the traditional MM mode and so the= rest > > of the patches extends the existing secure variable service support to = be > > useable with Standalone MM mode as well. > > > > Jagadeesh Ujja (13): > > StandaloneMmPkg: Pull in additonal libraries from staging branch > > MdePkg: Add a PCD that indicates presence of Standalone MM mode > > MdeModulePkg: Add a PCD to indicate Standalone MM supports secure > > variable > > MdePkg/Include: add StandaloneMmServicesTableLib header file > > MdePkg/Library/BaseLib/AArch64: Add AsmLfence function > > MdePkg/Library: Add StandaloneMmRuntimeDxe library > > MdeModulePkg/FaultTolerantWriteDxe: allow reusability as a MM driver > > MdeModulePkg/Variable/RuntimeDxe: adapt for usability with MM > > Standalone > > MdeModulePkg/Variable/RuntimeDxe: adapt as a MM Standalone driver > > MdeModulePkg/VarCheckLib: allow MM_STANDALONE drivers to use this > > library > > ArmPlatformPkg/NorFlashDxe: allow reusability as a MM driver > > SecurityPkg/AuthVariableLib: allow MM_STANDALONE drivers to use this > > library > > CryptoPkg/BaseCryptLib: allow MM_STANDALONE drivers to use this > > library > > > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashBlockIoDxe.c > > | 2 +- > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c = | > > 210 ++++- > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h = | > > 5 +- > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf = | > > 2 + > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c > > | 96 +-- > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > > | 76 ++ > > CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf = | 7 > > +- > > CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf = | 4 > > + > > CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c = | > > 15 +- > > MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf = | > > 5 +- > > MdeModulePkg/MdeModulePkg.dec = | 5 + > > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf > > | 1 + > > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c > > | 203 +++-- > > > > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandalo= n > > eMm.inf | 101 +++ > > MdeModulePkg/Universal/FaultTolerantWriteDxe/UpdateWorkingBlock.c > > | 27 +- > > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > | 37 +- > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > > | 1 + > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c > > | 201 ++++- > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c > > | 31 +- > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf > > | 3 + > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf > > | 132 ++++ > > MdePkg/Include/Library/BaseLib.h = | 33 +- > > MdePkg/Include/Library/StandaloneMmRuntimeDxe.h > > | 39 + > > MdePkg/Include/Library/StandaloneMmServicesTableLib.h > > | 25 + > > MdePkg/Library/BaseLib/AArch64/AsmLfence.S = | 42 > > + > > MdePkg/Library/BaseLib/AArch64/AsmLfence.asm = | > > 41 + > > MdePkg/Library/BaseLib/BaseLib.inf = | 2 + > > MdePkg/Library/StandaloneMmRuntimeDxe/StandaloneMmRuntimeDxe.c > > | 36 + > > MdePkg/Library/StandaloneMmRuntimeDxe/StandaloneMmRuntimeDxe.inf > > | 36 + > > MdePkg/MdePkg.dec = | 12 + > > SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf = | > > 5 +- > > > > StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHob > > Lib.inf | 2 +- > > > > StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/StandaloneMmCore > > HobLibInternal.c | 64 ++ > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c > > | 655 ++++++++++++++++ > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf > > | 48 ++ > > > > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMm > > MemoryAllocationLib.c | 824 ++++++++++++++++++++ > > > > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMm > > MemoryAllocationLib.inf | 45 ++ > > > > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServi > > cesTableLib.c | 64 ++ > > > > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServi > > cesTableLib.inf | 36 + > > 39 files changed, 2929 insertions(+), 244 deletions(-) > > create mode 100644 > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > > create mode 100644 > > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandalo= n > > eMm.inf > > create mode 100644 > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf > > create mode 100644 MdePkg/Include/Library/StandaloneMmRuntimeDxe.h > > create mode 100644 MdePkg/Include/Library/StandaloneMmServicesTableLib= .h > > create mode 100644 MdePkg/Library/BaseLib/AArch64/AsmLfence.S > > create mode 100644 MdePkg/Library/BaseLib/AArch64/AsmLfence.asm > > create mode 100644 > > MdePkg/Library/StandaloneMmRuntimeDxe/StandaloneMmRuntimeDxe.c > > create mode 100644 > > MdePkg/Library/StandaloneMmRuntimeDxe/StandaloneMmRuntimeDxe.inf > > create mode 100644 > > StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/StandaloneMmCore > > HobLibInternal.c > > create mode 100644 > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c > > create mode 100644 > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf > > create mode 100644 > > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMm > > MemoryAllocationLib.c > > create mode 100644 > > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMm > > MemoryAllocationLib.inf > > create mode 100644 > > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServi > > cesTableLib.c > > create mode 100644 > > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServi > > cesTableLib.inf > > > > -- > > 2.7.4 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel