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

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

My comment is below. 

1. Please don't update MemoryFence() implementation. It will impact all consumer code. AsmLfence() is X86 specific API. You can implement the internal function in the arch specific source file to call AsmLfence() for X86 and call MemoryFence() for ARM. This internal function will be called in the common logic. 
2. On StandaloneMmServicesTableLib.h, I suggest to add it into MdePkg, and add StandaloneMmRuntimeDxe library into MdePkg. This library sets gMmst is NULL, and always return FALSE in InMm(). 
3. On PcdStandaloneMmEnable, I also suggest to add it into MdePkg. It can be used to control the driver logic in the different packages. 

With 2 & 3, other edk2 packages don't need to depend on StandaloneMmPkg/StandaloneMmPkg.dec

> -----Original Message-----
> From: Jagadeesh Ujja [mailto:jagadeesh.ujja@arm.com]
> Sent: Wednesday, November 28, 2018 5:35 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: [RFC PATCH v3 00/11] Extend secure variable service to be usable from Standalone MM
> 
> 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 (11):
>   MdeModulePkg/Variable: replace all uses of AsmLfence with MemoryFence
>   StandaloneMmPkg: Pull in additonal libraries from staging branch
>   MdeModulePkg/Library: Add StandaloneMmRuntimeDxe library
>   ArmPlatformPkg/NorFlashDxe: allow reusability as a MM driver
>   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
>   SecurityPkg/AuthVariableLib: allow MM_STANDALONE drivers to use this
>     library
>   MdeModulePkg/VarCheckLib: allow MM_STANDALONE drivers to use this
>     library
>   CryptoPkg/BaseCryptLib: allow MM_STANDALONE drivers to use this
>     library
>   CryptoPkg/BaseCryptLib: Hack to get time in MM Standalone mode
> 
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> |   3 +
>  ArmPlatformPkg/Drivers/NorFlashDxe/{NorFlashDxe.inf => NorFlashStandaloneMm.inf}
> |  28 +-
>  CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> |   8 +-
>  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> |   5 +
>  MdeModulePkg/Library/{VarCheckLib/VarCheckLib.inf => StandaloneMmRuntimeDxe/StandaloneMmRuntimeDxe.inf}
> |  22 +-
>  MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
> |   5 +-
>  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> |   2 +
>  MdeModulePkg/Universal/FaultTolerantWriteDxe/{FaultTolerantWriteDxe.inf => FaultTolerantWriteStandaloneMm.inf}           |
> 53 +-
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> |   2 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
> |   4 +
>  MdeModulePkg/Universal/Variable/RuntimeDxe/{VariableRuntimeDxe.inf => VariableStandaloneMm.inf}
> | 107 ++-
>  SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> |   5 +-
>  StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf
> |   2 +-
>  StandaloneMmPkg/Library/{StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf =>
> StandaloneMmHobLib/StandaloneMmHobLib.inf} |  11 +-
>  StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf
> |  45 ++
>  StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
> |  36 +
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> |   5 +-
>  MdeModulePkg/Include/Library/StandaloneMmRuntimeDxe.h
> |  39 +
>  StandaloneMmPkg/Include/Library/StandaloneMmServicesTableLib.h
> |  47 ++
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashBlockIoDxe.c
> |   2 +-
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> | 211 ++++-
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> |  88 ++-
>  CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c
> |  27 +-
>  MdeModulePkg/Library/StandaloneMmRuntimeDxe/StandaloneMmRuntimeDxe.c
> |  36 +
>  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
> | 207 +++--
>  MdeModulePkg/Universal/FaultTolerantWriteDxe/UpdateWorkingBlock.c
> |  27 +-
>  MdeModulePkg/Universal/Variable/RuntimeDxe/LoadFenceSmm.c
> |   2 +-
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> |  37 +-
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> | 201 ++++-
>  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
> |  31 +-
>  MdePkg/Library/BaseLib/X86MemoryFence.c
> |   2 +-
>  StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/StandaloneMmCoreHobLibInternal.c
> |  64 ++
>  StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
> | 655 ++++++++++++++++
>  StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.c
> | 824 ++++++++++++++++++++
>  StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.c
> |  64 ++
>  35 files changed, 2564 insertions(+), 343 deletions(-)
>  copy ArmPlatformPkg/Drivers/NorFlashDxe/{NorFlashDxe.inf => NorFlashStandaloneMm.inf} (71%)
>  copy MdeModulePkg/Library/{VarCheckLib/VarCheckLib.inf => StandaloneMmRuntimeDxe/StandaloneMmRuntimeDxe.inf} (51%)
>  copy MdeModulePkg/Universal/FaultTolerantWriteDxe/{FaultTolerantWriteDxe.inf => FaultTolerantWriteStandaloneMm.inf} (54%)
>  copy MdeModulePkg/Universal/Variable/RuntimeDxe/{VariableRuntimeDxe.inf => VariableStandaloneMm.inf} (54%)
>  copy StandaloneMmPkg/Library/{StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf =>
> StandaloneMmHobLib/StandaloneMmHobLib.inf} (79%)
>  create mode 100644 StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf
>  create mode 100644 StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
>  create mode 100644 MdeModulePkg/Include/Library/StandaloneMmRuntimeDxe.h
>  create mode 100644 StandaloneMmPkg/Include/Library/StandaloneMmServicesTableLib.h
>  create mode 100644 MdeModulePkg/Library/StandaloneMmRuntimeDxe/StandaloneMmRuntimeDxe.c
>  create mode 100644 StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/StandaloneMmCoreHobLibInternal.c
>  create mode 100644 StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
>  create mode 100644 StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.c
>  create mode 100644 StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.c
> 
> --
> 2.7.4