Hi Liming, On Thu, Dec 13, 2018 at 8:02 PM Gao, Liming <[email protected]> wrote: > > I add my comments.
Thanks for the clarification. Will fix the patches accordingly. Regards, Jagadeesh. > > > -----Original Message----- > > From: Jagadeesh Ujja [mailto:[email protected]] > > Sent: Thursday, December 13, 2018 8:00 PM > > To: Gao, Liming <[email protected]> > > Cc: [email protected]; Zhang, Chao B <[email protected]>; Leif > > Lindholm <[email protected]> > > 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 <[email protected]> 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 > > > 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 fork > edk2 tree? If so, it will be easy for review. > > > > 2. Pcd is for Standalone MM Code, not specific for Variable. So, I > > > suggest 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/TimerWrapper.c. So, I understand it is > general purpose, not only for Variable. > If it is for Variable only, please define this PCD into MdeModulePkg instead > 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. And, > > 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) should > > > list its library class name in LIBRARY_CLASS of [Defines] > > section. Its library class name is StandaloneMmServicesTableLib. And, > > MdePkg 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. > > StandaloneMmRuntimeDxe library doesn't depend on any other library class. It > > doesn't need to list other library class 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 file. > > > 7. Platform DSC also needs to list LibraryClassName|Library > > > implementation 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 > > > it is X86 specific API. I suggest to update Variable driver with > > the wrapper function FenceFunc() for AsmLfence() and MemoryFence(). > > FenceFunc can be implemented for the different arch in > > Variable driver. Variable driver will call FenceFunc() instead of > > AsmLfence(). 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 have > 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 to > current patch. But, I would like to see one AsmLfence() declaration in > BaseLib.h. > Its description can list the different behavior for the different processors. > Then, you add its implementation for Arm and AArch64 arch. > > > > > -----Original Message----- > > > > From: Jagadeesh Ujja [mailto:[email protected]] > > > > Sent: Tuesday, December 11, 2018 2:22 PM > > > > To: [email protected]; Gao, Liming <[email protected]>; Zhang, > > > > Chao B <[email protected]>; > > [email protected] > > > > 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 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 “TimerWrapp.c” 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 > > > > support 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 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 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/FaultTolerantWriteStandaloneMm.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/StandaloneMmCoreHobLibInternal.c > > > > | 64 ++ > > > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c > > > > | 655 > > > > ++++++++++++++++ > > > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf > > > > | 48 ++ > > > > > > > > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.c > > > > | 824 > > > > ++++++++++++++++++++ > > > > > > > > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf > > > > | 45 ++ > > > > > > > > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.c > > > > | 64 ++ > > > > > > > > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf > > > > | 36 + > > > > 37 files changed, 2894 insertions(+), 231 deletions(-) > > > > create mode 100644 > > > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > > > > create mode 100644 > > > > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf > > > > create mode 100644 > > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.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/StandaloneMmRuntimeDxe.c > > > > create mode 100644 > > > > MdePkg/Library/StandaloneMmRuntimeDxe/StandaloneMmRuntimeDxe.inf > > > > create mode 100644 > > > > StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/StandaloneMmCoreHobLibInternal.c > > > > create mode 100644 > > > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c > > > > create mode 100644 > > > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf > > > > create mode 100644 > > > > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.c > > > > create mode 100644 > > > > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf > > > > create mode 100644 > > > > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.c > > > > create mode 100644 > > > > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf > > > > > > > > -- > > > > 2.7.4 > > > > > > _______________________________________________ > > > edk2-devel mailing list > > > [email protected] > > > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
