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

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

I add my comments. 

> -----Original Message-----
> From: Jagadeesh Ujja [mailto:jagadeesh.ujja@arm.com]
> Sent: Thursday, December 13, 2018 8:00 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: edk2-devel@lists.01.org; Zhang, Chao B <chao.b.zhang@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>
> 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 <liming.gao@intel.com> 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:jagadeesh.ujja@arm.com]
> > > Sent: Tuesday, December 11, 2018 2:22 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
> > > 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
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel