Re: [PATCH 00/13] Extend secure variable service to be usable from Standalone MM

["Gao, Liming" <liming.gao@intel.com>]

Jagadeesh:
  MdeModulePkg Variable service/Fault tolerant/Nor Flash driver depends on StandaloneMmServicesTableLib library class header file. This header file is added into MdePkg. It has two interfaces. One is global gMmst, another is function InMm(). So, there is no dependency issue here. 
And, MdePkg adds one StandaloneMmServicesTableLib library INF with empty implementation, this library is just for build. It sets gMmst=NULL, and always return FASLE in InMm(). This library can be used in MdeModulePkg.dsc to make Variable driver pass build. There is also no dependency issue here. Last, Platform DSC file will refer to the real StandaloneMmServicesTableLib library INF from StandaloneMmPkg. 

Thanks
Liming
> -----Original Message-----
> From: Jagadeesh Ujja [mailto:jagadeesh.ujja@arm.com]
> Sent: Tuesday, December 18, 2018 7:19 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: edk2-devel@lists.01.org; Zhang, Chao B <chao.b.zhang@intel.com>
> Subject: Re: [edk2] [PATCH 00/13] Extend secure variable service to be usable from Standalone MM
> 
> Hi Liming,
> 
> On Tue, Dec 18, 2018 at 10:07 AM Gao, Liming <liming.gao@intel.com> wrote:
> >	
> > Jagadeesh:
> >   StandaloneMmServicesTableLib library class header file is added into MdePkg. 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 “StandaloneMmPkg”.
> 
> - MmServicesTableLib|StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
> - MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/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/FaultTolerantWriteStandaloneMm.inf
> 
> These drivers look for “gMmst” which is defined in
> “MmServicesTableLib”. Ideally,
> “StandaloneMmPkg/Include/Library/StandaloneMmServicesTableLib.h”
> should have defined “gMmst” as an “extern EFI_MM_SYSTEM_TABLE
> *gMmst;”.
> In which case, we would have to add
> “StandaloneMmPkg/StandaloneMmPkg.dec” in other drivers listed below.
> 
> - MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> - MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> 
> This will make “edk2 packages” to be depended on
> "StandaloneMmPkg/StandaloneMmPkg.dec".
> 
> To avoid this, “StandaloneMmPkg/Include/Library/StandaloneMmServicesTableLib.h”
> is moved to “MdePkg/Include/Library/StandaloneMmServicesTableLib.h”.
> But, the implementation of “MmServicesTableLib” comes from
> “StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf”.
> 
> 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 <liming.gao@intel.com>
> > >Cc: edk2-devel@lists.01.org; Zhang, Chao B <chao.b.zhang@intel.com>;
> > >leif.lindholm@linaro.org; ard.biesheuvel@linaro.org
> > >Subject: Re: [edk2] [PATCH 00/13] Extend secure variable service to be usable
> > >from Standalone MM
> > >
> > >Hi Liming,
> > >
> > >On Mon, Dec 17, 2018 at 7:15 AM Gao, Liming <liming.gao@intel.com> 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 <liming.gao@intel.com>; Zhang,
> > >> >Chao B <chao.b.zhang@intel.com>; leif.lindholm@linaro.org;
> > >> >ard.biesheuvel@linaro.org
> > >> >Subject: [PATCH 00/13] Extend secure variable service to be usable from
> > >> >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 and
> > >> >    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 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 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 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.
> > >> >
> > >> >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 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
> > >|
> > >> >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