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 4A00A2119EF26 for ; Tue, 18 Dec 2018 03:19:42 -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 87565A78 for ; Tue, 18 Dec 2018 03:19:42 -0800 (PST) Received: from mail-ed1-f43.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 2DC793F5C0 for ; Tue, 18 Dec 2018 03:19:42 -0800 (PST) Received: by mail-ed1-f43.google.com with SMTP id h15so13551761edb.4 for ; Tue, 18 Dec 2018 03:19:42 -0800 (PST) X-Gm-Message-State: AA+aEWa35lOkqxBVqh8CiE2UoR+0aZVKHsLBFqHxnJEXCDcBoJXp2OtR 5n2Nx3WqCYEqYoYRIdLzA9JJXfuA4Di3I+b/l+A= X-Google-Smtp-Source: AFSGD/ULp2xZurc35Yljdwt/p1UZDXKH9xPHhcElbH87q9Z5kOJ2QJcdoIXKbPHdSGB9FrxGurfDbsrfxomCFDGFUcE= X-Received: by 2002:aa7:d817:: with SMTP id v23mr16117941edq.255.1545131980420; Tue, 18 Dec 2018 03:19:40 -0800 (PST) MIME-Version: 1.0 References: <1544789607-11316-1-git-send-email-jagadeesh.ujja@arm.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E38D3C3@SHSMSX104.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E38DE09@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E38DE09@SHSMSX104.ccr.corp.intel.com> From: Jagadeesh Ujja Date: Tue, 18 Dec 2018 16:49:29 +0530 X-Gmail-Original-Message-ID: Message-ID: To: "Gao, Liming" Cc: "edk2-devel@lists.01.org" , "Zhang, Chao B" 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: Tue, 18 Dec 2018 11:19:43 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Liming, On Tue, Dec 18, 2018 at 10:07 AM Gao, Liming wrote: > > Jagadeesh: > StandaloneMmServicesTableLib library class header file is added into Md= ePkg. Its library implementation is in MdePkg and StandaloneMmPkg. The one = in MdePkg produces the dummy implementation, and the one in StandaloneMmPkg= produces the real implementation. I don't see the reason to separate this = library class. > In this patchset series, the Variable service/Fault tolerant/Nor Flash driver are refactored to be usable as MM_STANDALONE driver. These drivers uses the following libraries from =E2=80=9CStandaloneMmPkg=E2=80=9D= . - MmServicesTableLib|StandaloneMmPkg/Library/StandaloneMmServicesTableLib/S= tandaloneMmServicesTableLib.inf - MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAllocationL= ib/StandaloneMmMemoryAllocationLib.inf Variable MM_STANDALONE driver is located at - MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c - MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf FaultTolerant MM_STANDALONE is driver located at - MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c - MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandalone= Mm.inf These drivers look for =E2=80=9CgMmst=E2=80=9D which is defined in =E2=80=9CMmServicesTableLib=E2=80=9D. Ideally, =E2=80=9CStandaloneMmPkg/Include/Library/StandaloneMmServicesTableLib.h=E2= =80=9D should have defined =E2=80=9CgMmst=E2=80=9D as an =E2=80=9Cextern EFI_MM_SY= STEM_TABLE *gMmst;=E2=80=9D. In which case, we would have to add =E2=80=9CStandaloneMmPkg/StandaloneMmPkg.dec=E2=80=9D in other drivers list= ed below. - MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf - MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf This will make =E2=80=9Cedk2 packages=E2=80=9D to be depended on "StandaloneMmPkg/StandaloneMmPkg.dec". To avoid this, =E2=80=9CStandaloneMmPkg/Include/Library/StandaloneMmService= sTableLib.h=E2=80=9D is moved to =E2=80=9CMdePkg/Include/Library/StandaloneMmServicesTableLib.h= =E2=80=9D. But, the implementation of =E2=80=9CMmServicesTableLib=E2=80=9D comes from =E2=80=9CStandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmS= ervicesTableLib.inf=E2=80=9D. Thanks Jagadeesh > Thanks > Liming > >-----Original Message----- > >From: Jagadeesh Ujja [mailto:jagadeesh.ujja@arm.com] > >Sent: Monday, December 17, 2018 7:47 PM > >To: Gao, Liming > >Cc: edk2-devel@lists.01.org; Zhang, Chao B ; > >leif.lindholm@linaro.org; ard.biesheuvel@linaro.org > >Subject: Re: [edk2] [PATCH 00/13] Extend secure variable service to be u= sable > >from Standalone MM > > > >Hi Liming, > > > >On Mon, Dec 17, 2018 at 7:15 AM Gao, Liming wrote= : > >> > >> One question here. Why separate StandaloneMmServicesTableLib to two > >library classes? Current MdePkg\Include\Library\SmmServicesTableLib.h is > >one library class. > >MdePkg\Library\SmmServicesTableLib\SmmServicesTableLib.inf is its > >implementation. StandaloneMmServicesTableLib should be same to it. > >> StandaloneMmServicesTableLib is the library class. > >MdePkg\Library\StandaloneMmRuntimeDxe is its library instance. > >> > >Thanks for your review. > > > >The implementation of the "StandaloneMmServicesTableLib" library class > >is at "StandaloneMmPkg/Library/StandaloneMmServicesTableLib/". As this > >patchset reuses some of the DXE_DRIVER drivers as MM_STANDALONE > >drivers, the "StandaloneMmServicesTableLib" library class definition > >was placed within MdePkg. The reason for splitting the library class > >definition (in MdePkg) and its implementation (in StandaloneMmPkg) was > >due to your comment that "edk2 packages" should not have any reference > >to StandaloneMmPkg.dec. > > > >The "StandaloneMmRuntimeDxe" library now just has an implementation of > >InMm(). And so, this can be kept as a separate library with no > >dependency on StandaloneMmPkg. So this was the reason to split > >"StandaloneMmRuntimeDxe" and "StandaloneMmServicesTableLib" into two > >separate libraries. > > > >thanks > >Jagadeesh > >> Thanks > >> Liming > >> >-----Original Message----- > >> >From: Jagadeesh Ujja [mailto:jagadeesh.ujja@arm.com] > >> >Sent: Friday, December 14, 2018 8:13 PM > >> >To: edk2-devel@lists.01.org; Gao, Liming ; Zhan= g, > >> >Chao B ; leif.lindholm@linaro.org; > >> >ard.biesheuvel@linaro.org > >> >Subject: [PATCH 00/13] Extend secure variable service to be usable fr= om > >> >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 an= d > >> > 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_secure_vars) > >> > > >> >Changes since RFC v3: > >> >- Addressed all the comments from Liming Gao > >> > - Added a AArch64 implementation of AsmLfence which is a wrapper fo= r > >> > MemoryFence. The changes in variable service driver in v3 of this > >> > patchset that used MemoryFence instead of AsmLfence have been > >> >removed. > >> > - 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 suppor= t for > >> >use with Standalone MM. This is applicable to paltforms that use > >Standalone > >> >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= of > >> >StandaloneMmPkg into the edk2's StandaloneMmPkg. The existing secure > >> >variable > >> >service implementation supports only the traditional MM mode and so t= he > >> >rest > >> >of the patches extends the existing secure variable service support t= o 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 drive= r > >> > 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 thi= s > >> > 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/FaultTolerantWriteStan > >dal > >> >oneMm.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/VariableSmmRuntimeDx > >e.i > >> >nf | 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/StandaloneMmCor > >eH > >> >obLib.inf | 2 +- > >> > > >> >StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/StandaloneM > >mC > >> >oreHobLibInternal.c | 64 ++ > >> > > >StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c > >> >| 655 ++++++++++++++++ > >> > > >StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf > >> >| 48 ++ > >> > > >> >StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/Standalo > >ne > >> >MmMemoryAllocationLib.c | 824 ++++++++++++++++++++ > >> > > >> >StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/Standalo > >ne > >> >MmMemoryAllocationLib.inf | 45 ++ > >> > > >> >StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneM > >mS > >> >ervicesTableLib.c | 64 ++ > >> > > >> >StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneM > >mS > >> >ervicesTableLib.inf | 36 + > >> > 39 files changed, 2929 insertions(+), 244 deletions(-) > >> > create mode 100644 > >> >ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > >> > create mode 100644 > >> >MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStan > >dal > >> >oneMm.inf > >> > create mode 100644 > >> >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i > >nf > >> > 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.i > >nf > >> > create mode 100644 > >> >StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/StandaloneM > >mC > >> >oreHobLibInternal.c > >> > create mode 100644 > >> >StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c > >> > create mode 100644 > >> >StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.i > >nf > >> > create mode 100644 > >> >StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/Standalo > >ne > >> >MmMemoryAllocationLib.c > >> > create mode 100644 > >> >StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/Standalo > >ne > >> >MmMemoryAllocationLib.inf > >> > create mode 100644 > >> >StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneM > >mS > >> >ervicesTableLib.c > >> > create mode 100644 > >> >StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneM > >mS > >> >ervicesTableLib.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