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 2FAD7211A43A0 for ; Fri, 14 Dec 2018 03:10:38 -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 B2BAD15AD for ; Fri, 14 Dec 2018 03:10:37 -0800 (PST) Received: from mail-ed1-f51.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 5C5973F721 for ; Fri, 14 Dec 2018 03:10:37 -0800 (PST) Received: by mail-ed1-f51.google.com with SMTP id p6so4651990eds.0 for ; Fri, 14 Dec 2018 03:10:37 -0800 (PST) X-Gm-Message-State: AA+aEWZEYvVd+NyM+K9peV/6zTKH7Uvb/wpgO8HUWsjRjGLrk48gdiVb PkSNU87MIF+z9Ak9W7Ay9rnX4e2jr2WE1f6kj9U= X-Google-Smtp-Source: AFSGD/VUBoDOJVbDf/I/B2vYkR6NmRmUrZHf58z1VDEa3pxyde2PELxc82seRvAWL+kaAEBSIE/jeLZbw9KLznEJWBA= X-Received: by 2002:a17:906:cb2:: with SMTP id k18-v6mr2158951ejh.129.1544785835699; Fri, 14 Dec 2018 03:10:35 -0800 (PST) MIME-Version: 1.0 References: <1544509302-1000-1-git-send-email-jagadeesh.ujja@arm.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E38BAC8@SHSMSX104.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E38C550@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E38C550@SHSMSX104.ccr.corp.intel.com> From: Jagadeesh Ujja Date: Fri, 14 Dec 2018 16:40:24 +0530 X-Gmail-Original-Message-ID: Message-ID: To: "Gao, Liming" Cc: "edk2-devel@lists.01.org" , "Zhang, Chao B" Subject: Re: [RFC PATCH v4 00/12] 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: Fri, 14 Dec 2018 11:10:38 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Liming, On Thu, Dec 13, 2018 at 8:02 PM Gao, Liming wrote: > > I add my comments. Thanks for the clarification. Will fix the patches accordingly. Regards, Jagadeesh. > > > -----Original Message----- > > From: Jagadeesh Ujja [mailto:jagadeesh.ujja@arm.com] > > Sent: Thursday, December 13, 2018 8:00 PM > > To: Gao, Liming > > Cc: edk2-devel@lists.01.org; Zhang, Chao B ; Le= if Lindholm > > Subject: Re: [edk2] [RFC PATCH v4 00/12] Extend secure variable service= to be usable from Standalone MM > > > > Hi Liming > > > > On Wed, Dec 12, 2018 at 8:44 PM Gao, Liming wrot= e: > > > > > > This version is better. I have some comments on edk2 coding style. > > > > Thank you for your review. Please see reply to your comments below. > > > > > > > > 1. This patch set can't be applied in edk2 trunk. Seemly, they base o= n previous version edk2. > > > > The v4 patchset was based on the tip of the edk2 master branch on the > > day it was posted. The commit id on which this series was based is > > "f7f94ffe". > > > So, can you fork edk2 tree and upload these changes into your branch in f= ork edk2 tree? If so, it will be easy for review. > > > > 2. Pcd is for Standalone MM Code, not specific for Variable. So, I su= ggest to use the generic name PcdStandaloneMmCodeEnabled. Its > > description is also required to be updated. > > > > The intention of the changes done in the patchset is to reuse the > > variable service driver in MM_STANDALONE mode. There could be > > platforms that enable Standalone MM mode but would not want a secure > > storage for EFI variables. In which case, the PCD named > > PcdStandaloneMmCodeEnabled would not be sufficient. And this the > > reason it was named " PcdStandaloneMmVariableEnabled". > > > I see this PCD is also used in CryptoPkg/Library/BaseCryptLib/SysCall/Tim= erWrapper.c. So, I understand it is general purpose, not only for Variable. > If it is for Variable only, please define this PCD into MdeModulePkg inst= ead of MdePkg. > > > > 3. Library header file name (StandaloneMmServicesTableLib.h) is also = library class name. Library class name and header file mapping > > is required to be listed in MdePkg.dec file [LibraryClasses] section. A= nd, this header file doesn't need to include Library/DebugLib.h, > > because it doesn't depend on it. > > > 4. Library implementation INF file (StandaloneMmRuntimeDxe.inf) shoul= d list its library class name in LIBRARY_CLASS of [Defines] > > section. Its library class name is StandaloneMmServicesTableLib. And, M= dePkg library implementation depends on MdePkg.dec only in > > [Packages] section. > > > 5. Library implementation should implement all interfaces defined in = library class header file. StandaloneMmRuntimeDxe library > > should initialize gMmst as NULL if it has no real value. StandaloneMmRu= ntimeDxe library doesn't depend on any other library class. It > > doesn't need to list other library class in its [LibraryClasses] sectio= n of INF file. > > > > Point 3, 4 and 5 will be fixed > > > > > 6. When other module depends on this library class header file, it sh= ould list StandaloneMmServicesTableLib in its [LibraryClasses] > > section of INF file. > > > 7. Platform DSC also needs to list LibraryClassName|Library implement= ation INF in [LibraryClasses] section. > > > > Points 6 and 7 are taken care and are part of edk2platform specific > > changes, will post those changes soon > > > > > 8. I don't suggest to add AsmLfence API in BaseLib for AArch64, becau= se it is X86 specific API. I suggest to update Variable driver with > > the wrapper function FenceFunc() for AsmLfence() and MemoryFence(). Fen= ceFunc can be implemented for the different arch in > > Variable driver. Variable driver will call FenceFunc() instead of AsmLf= ence(). So, only variable driver is required to be updated. There is > > no change in BaseLib. > > > > > Okay, the variable driver can be updated to call a wrapper > > "FenceFunc()" but wouldn't it be useful to add the architecture > > specific implantation of this in BaseLib. In that way, not just the > > variable driver but other drivers can use this implementation of > > "FenceFunc()". For instance, > > FaultTolerantWriteDxe/FaultTolerantWriteSmm.c does calls to > > AsmLfence() and an architecture specific implementation of > > "FenceFunc()" in BaseLib can be reused in FaultTolerantWriteDxe driver > > as well. > > > Now, there are two drivers VariableSmm and FaultTolerantWriteSmm that hav= e this usage. For sharing the code, I prefer not to add new API, and > change AsmLfence() as the generic API for all processors. It is similar t= o current patch. But, I would like to see one AsmLfence() declaration in Ba= seLib.h. > Its description can list the different behavior for the different process= ors. Then, you add its implementation for Arm and AArch64 arch. > > > > > -----Original Message----- > > > > From: Jagadeesh Ujja [mailto:jagadeesh.ujja@arm.com] > > > > Sent: Tuesday, December 11, 2018 2:22 PM > > > > To: edk2-devel@lists.01.org; Gao, Liming ; Zh= ang, Chao B ; > > leif.lindholm@linaro.org > > > > Subject: [RFC PATCH v4 00/12] Extend secure variable service to be = usable from Standalone MM > > > > > > > > Changes since 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 th= is > > > > patchset that used MemoryFence instead of AsmLfence have been r= emoved. > > > > - 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 ad= d a appropriate > > > > implementation of this for MM Standalone mode code. > > > > > > > > Changes since v2: > > > > - Added 'Contributed-under' tag, removed Change-ID tag and > > > > maintained a single signed-off-by for the all the patches. > > > > > > > > Changes since v1: > > > > - Addressed all the comments from Liming Gao > > > > - Removed the use of #ifdef/#else/#endif and used a Pcd instead t= o > > > > 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 cod= e. > > > > > > > > This RFC patch series extends the existing secure variable service = support for > > > > use with Standalone MM. This is applicable to paltforms that use St= andalone > > > > 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 bran= ch of > > > > StandaloneMmPkg into the edk2's StandaloneMmPkg. The existing secur= e 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. > > > > > > > > This patch series is being posted as an RFC to get feedback on the = approach taken > > > > in these patches. > > > > > > > > Jagadeesh Ujja (12): > > > > StandaloneMmPkg: Pull in additonal libraries from staging branch > > > > MdePkg: Add a PCD to enable secure storage of variables > > > > 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 dri= ver > > > > 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 t= his > > > > 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 = | 4 +- > > > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf = | 3 + > > > > 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/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe= .inf | 1 + > > > > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm= .c | 203 +++-- > > > > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSta= ndaloneMm.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.i= nf | 3 + > > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.in= f | 132 ++++ > > > > MdePkg/Include/Library/BaseLib.h = | 10 + > > > > MdePkg/Include/Library/StandaloneMmServicesTableLib.h = | 45 ++ > > > > 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 = | 42 + > > > > MdePkg/MdePkg.dec = | 5 + > > > > SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf = | 5 +- > > > > StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHob= Lib.inf | 2 +- > > > > StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/StandaloneMmCor= eHobLibInternal.c | 64 ++ > > > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c = | 655 > > > > ++++++++++++++++ > > > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf = | 48 ++ > > > > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/Standalone= MmMemoryAllocationLib.c | 824 > > > > ++++++++++++++++++++ > > > > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/Standalone= MmMemoryAllocationLib.inf | 45 ++ > > > > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmS= ervicesTableLib.c | 64 ++ > > > > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmS= ervicesTableLib.inf | 36 + > > > > 37 files changed, 2894 insertions(+), 231 deletions(-) > > > > create mode 100644 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStan= daloneMm.inf > > > > create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/Fa= ultTolerantWriteStandaloneMm.inf > > > > create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/Vari= ableStandaloneMm.inf > > > > create mode 100644 MdePkg/Include/Library/StandaloneMmServicesTabl= eLib.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/Standalon= eMmRuntimeDxe.c > > > > create mode 100644 MdePkg/Library/StandaloneMmRuntimeDxe/Standalon= eMmRuntimeDxe.inf > > > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmHobLib/AArc= h64/StandaloneMmCoreHobLibInternal.c > > > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmHobLib/Stan= daloneMmHobLib.c > > > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmHobLib/Stan= daloneMmHobLib.inf > > > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmMemoryAlloc= ationLib/StandaloneMmMemoryAllocationLib.c > > > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmMemoryAlloc= ationLib/StandaloneMmMemoryAllocationLib.inf > > > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmServicesTab= leLib/StandaloneMmServicesTableLib.c > > > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmServicesTab= leLib/StandaloneMmServicesTableLib.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