Re: [RFC PATCH 0/9] Extend secure variable service to be usable from Standalone MM

From: "Gao, Liming" <liming.gao@intel.com>
To: Jagadeesh Ujja <jagadeesh.ujja@arm.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Subject: Re: [RFC PATCH 0/9] Extend secure variable service to be usable from Standalone MM
Date: Wed, 31 Oct 2018 13:58:55 +0000	[thread overview]
Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E3629A5@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <20181031110947.6305-1-jagadeesh.ujja@arm.com>

Jagadeesh:
  I have some comments. 
1) Edk2 don't use #ifdef/#else/#endif to switch the code logic. You can define PCD to switch the code logic bases on PCD setting. 
2) Edk2 MdeModulePkg should not depend on platform packages from edk2-platforms. MdeModulePkg includes the generic modules those are platform intendent. 
3) mMmst is used widely. You can also introduce MmServiceTableLib for it. 
4) CryptoPkg\Library\BaseCryptLib\BaseCryptLib.inf can be reused only if you provide one dummy implementation UefiRuntimeServiceTableLib for MM_STANDALONE usage. 

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jagadeesh Ujja
> Sent: Wednesday, October 31, 2018 7:10 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [RFC PATCH 0/9] Extend secure variable service to be usable from Standalone MM
> 
> 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 (9):
>   StandaloneMmPkg: Pull in additonal libraries from staging branch
>   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
>   CryptoPkg/BaseCryptLib: Hack to get time in MM Standalone mode
>   SecurityPkg/AuthVariableLib:allow reusability as MM_STANDALONE
>   MdeModulePkg VarCheckLib: allow reusability as MM_STANDALONE
>   CryptoPkg/BaseCryptLib: allow MM_STANDALONE drivers to use this library
> 
>  .../Drivers/NorFlashDxe/NorFlashBlockIoDxe.c       |   5 +-
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c   | 116 ++-
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h   |  15 +-
>  .../Drivers/NorFlashDxe/NorFlashFvbDxe.c           |  26 +-
>  .../Drivers/NorFlashDxe/NorFlashMmStandalone.inf   |  76 ++
>  .../BaseCryptLib/BaseCryptLibMmStandalone.inf      | 106 +++
>  .../Library/BaseCryptLib/SysCall/TimerWrapper.c    |  19 +-
>  MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf   |   5 +-
>  .../FaultTolerantWriteMmStandalone.inf             | 100 +++
>  .../FaultTolerantWriteDxe/FaultTolerantWriteSmm.c  |  72 +-
>  .../FaultTolerantWriteDxe/UpdateWorkingBlock.c     |   8 +-
>  .../Universal/Variable/RuntimeDxe/Variable.c       |  16 +-
>  .../Universal/Variable/RuntimeDxe/Variable.h       |   8 +
>  .../Variable/RuntimeDxe/VariableMmStandalone.inf   | 134 ++++
>  .../Universal/Variable/RuntimeDxe/VariableSmm.c    |  73 +-
>  .../Variable/RuntimeDxe/VariableSmmRuntimeDxe.c    |  20 +-
>  .../Library/AuthVariableLib/AuthVariableLib.inf    |   5 +-
>  .../StandaloneMmCoreHobLib.inf                     |   2 +-
>  .../AArch64/StandaloneMmCoreHobLibInternal.c       |  64 ++
>  .../StandaloneMmHobLib/StandaloneMmHobLib.c        | 655 ++++++++++++++++
>  .../StandaloneMmHobLib/StandaloneMmHobLib.inf      |  48 ++
>  .../StandaloneMmMemoryAllocationLib.c              | 824 +++++++++++++++++++++
>  .../StandaloneMmMemoryAllocationLib.inf            |  45 ++
>  .../StandaloneMmServicesTableLib.c                 |  40 +
>  .../StandaloneMmServicesTableLib.inf               |  36 +
>  25 files changed, 2470 insertions(+), 48 deletions(-)
>  create mode 100644 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashMmStandalone.inf
>  create mode 100644 CryptoPkg/Library/BaseCryptLib/BaseCryptLibMmStandalone.inf
>  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteMmStandalone.inf
>  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableMmStandalone.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
> 
> --
> 1.9.1
> 
> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel