I add my comments. > -----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
