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 E1A1F2119F05E for ; Thu, 13 Dec 2018 04:00:33 -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 9B835EBD for ; Thu, 13 Dec 2018 04:00:33 -0800 (PST) Received: from mail-ed1-f48.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 4799F3F71D for ; Thu, 13 Dec 2018 04:00:33 -0800 (PST) Received: by mail-ed1-f48.google.com with SMTP id f23so1818374edb.3 for ; Thu, 13 Dec 2018 04:00:33 -0800 (PST) X-Gm-Message-State: AA+aEWaYLSjbIaXERdGABp8cEpRnhSw9NmNuV96A/gVmXiLb8IF79fF7 tb+gSPHK0M45W+YORP5DZymhT8cFbRzUsFCpThQ= X-Google-Smtp-Source: AFSGD/U7Ai2hdrc5ZoGDeQ/m+ZJSoJZr5dbE0rXmQVK0eLEtEMNWO0CYQZyCI2nmcG3QdFIBho8NlXvFQgcna0p3+4A= X-Received: by 2002:aa7:d817:: with SMTP id v23mr22282171edq.255.1544702431516; Thu, 13 Dec 2018 04:00:31 -0800 (PST) MIME-Version: 1.0 References: <1544509302-1000-1-git-send-email-jagadeesh.ujja@arm.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E38BAC8@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E38BAC8@SHSMSX104.ccr.corp.intel.com> From: Jagadeesh Ujja Date: Thu, 13 Dec 2018 17:30:20 +0530 X-Gmail-Original-Message-ID: Message-ID: To: liming.gao@intel.com Cc: edk2-devel@lists.01.org, chao.b.zhang@intel.com, Leif Lindholm 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: Thu, 13 Dec 2018 12:00:34 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Liming On Wed, Dec 12, 2018 at 8:44 PM Gao, Liming wrote: > > 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 on pr= evious 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". > 2. Pcd is for Standalone MM Code, not specific for Variable. So, I sugges= t to use the generic name PcdStandaloneMmCodeEnabled. Its description is al= so 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". > 3. Library header file name (StandaloneMmServicesTableLib.h) is also libr= ary class name. Library class name and header file mapping is required to b= e listed in MdePkg.dec file [LibraryClasses] section. And, this header file= doesn't need to include Library/DebugLib.h, because it doesn't depend on i= t. > 4. Library implementation INF file (StandaloneMmRuntimeDxe.inf) should li= st its library class name in LIBRARY_CLASS of [Defines] section. Its librar= y class name is StandaloneMmServicesTableLib. And, MdePkg library implement= ation depends on MdePkg.dec only in [Packages] section. > 5. Library implementation should implement all interfaces defined in libr= ary class header file. StandaloneMmRuntimeDxe library should initialize gMm= st as NULL if it has no real value. StandaloneMmRuntimeDxe library doesn't = depend on any other library class. It doesn't need to list other library cl= ass in its [LibraryClasses] section of INF file. Point 3, 4 and 5 will be fixed > 6. When other module depends on this library class header file, it should= list StandaloneMmServicesTableLib in its [LibraryClasses] section of INF f= ile. > 7. Platform DSC also needs to list LibraryClassName|Library implementatio= n 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, because i= t is X86 specific API. I suggest to update Variable driver with the wrapper= function FenceFunc() for AsmLfence() and MemoryFence(). FenceFunc can be i= mplemented for the different arch in Variable driver. Variable driver will = call FenceFunc() instead of AsmLfence(). So, only variable driver is requir= ed 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. > > -----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 ; Zhang,= Chao B ; leif.lindholm@linaro.org > > Subject: [RFC PATCH v4 00/12] Extend secure variable service to be usab= le 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 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 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 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 RFC patch series extends the existing secure variable service supp= ort 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 va= riable > > 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 appr= oach 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 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 = | 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/FaultTolerantWriteStandal= 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/VariableSmmRuntimeDxe.inf = | 3 + > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf = | 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/StandaloneMmCoreHobLib.= inf | 2 +- > > StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/StandaloneMmCoreHob= LibInternal.c | 64 ++ > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c = | 655 > > ++++++++++++++++ > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf = | 48 ++ > > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMe= moryAllocationLib.c | 824 > > ++++++++++++++++++++ > > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMe= moryAllocationLib.inf | 45 ++ > > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServi= cesTableLib.c | 64 ++ > > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServi= cesTableLib.inf | 36 + > > 37 files changed, 2894 insertions(+), 231 deletions(-) > > create mode 100644 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandalo= neMm.inf > > create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultT= olerantWriteStandaloneMm.inf > > create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable= StandaloneMm.inf > > 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/StandaloneMmR= untimeDxe.c > > create mode 100644 MdePkg/Library/StandaloneMmRuntimeDxe/StandaloneMmR= untimeDxe.inf > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/= StandaloneMmCoreHobLibInternal.c > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmHobLib/Standalo= neMmHobLib.c > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmHobLib/Standalo= neMmHobLib.inf > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmMemoryAllocatio= nLib/StandaloneMmMemoryAllocationLib.c > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmMemoryAllocatio= nLib/StandaloneMmMemoryAllocationLib.inf > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmServicesTableLi= b/StandaloneMmServicesTableLib.c > > create mode 100644 StandaloneMmPkg/Library/StandaloneMmServicesTableLi= b/StandaloneMmServicesTableLib.inf > > > > -- > > 2.7.4 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel