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

From: "Wang, Jian J" <jian.j.wang@intel.com>
To: Jagadeesh Ujja <jagadeesh.ujja@arm.com>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	"Gao, Liming" <liming.gao@intel.com>,
	"Zhang, Chao B" <chao.b.zhang@intel.com>,
	"leif.lindholm@linaro.org" <leif.lindholm@linaro.org>,
	"ard.biesheuvel@linaro.org" <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH 00/13] Extend secure variable service to be usable from Standalone MM
Date: Thu, 3 Jan 2019 02:37:17 +0000	[thread overview]
Message-ID: <D827630B58408649ACB04F44C510003624EDFA51@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <CADpYukY1btdroJ+BudJebpVhR7cnuapucH8YtxE5ciLnA3yuRg@mail.gmail.com>

Hi Jagadeesh,

Since those code are used in different drivers, a new library would be better. For
example, we could have a CommonMmServicesLibrary, in which following interfaces
are defined ((just for example)

EFI_STATUS
MmstLocateProtocol(
  IN  EFI_GUID  *Protocol,
  IN  VOID      *Registration, OPTIONAL
  OUT VOID      **Interface
  )
{
  EFI_STATUS  Status;

  if (!PcdGetBool (PcdStandaloneMmVariableEnabled)) {
    Status = gSmst->SmmLocateProtocol (
                      &gEfiSmmSwapAddressRangeProtocolGuid,
                      NULL,
                      SarProtocol
                      );
  } else {
    Status = gMmst->MmLocateProtocol (
                      &gEfiSmmSwapAddressRangeProtocolGuid,
                      NULL,
                      SarProtocol
                      );
  }

  return Status;
}

Then you can use MmstLocateProtocol() in FaultTolerantWriteDxe, Variable driver, etc.
This applies to other interfaces like InstallProtocolInterface, AllocatePage in SMM/MM
system table.

Regards,
Jian

> -----Original Message-----
> From: Jagadeesh Ujja [mailto:jagadeesh.ujja@arm.com]
> Sent: Wednesday, January 02, 2019 9:19 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: 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: Re: [edk2] [PATCH 00/13] Extend secure variable service to be usable
> from Standalone MM
> 
> hi Jian,
> 
> On Fri, Dec 21, 2018 at 8:27 AM Wang, Jian J <jian.j.wang@intel.com> wrote:
> >
> > Jagadeesh,
> >
> > There're many places in this patch series where code similar to following is
> added.
> > It'd better to wrap them into module private functions or even a library, if
> necessary.
> > This can make the code cleaner (no if/else) and easier (central place) to
> maintain in
> > the future.
> >
> > +  if (!PcdGetBool (PcdStandaloneMmVariableEnabled)) {
> > +    Status = gSmst->SmmLocateProtocol (
> > +                      &gEfiSmmSwapAddressRangeProtocolGuid,
> > +                      NULL,
> > +                      SarProtocol
> > +                      );
> > +  } else {
> > +    Status = gMmst->MmLocateProtocol (
> > +                      &gEfiSmmSwapAddressRangeProtocolGuid,
> > +                      NULL,
> > +                      SarProtocol
> > +                      );
> > +  }
> >
> Thank you for your comment. This patch series try to reuse code as
> much as possible between MM and non-MM code. So, in some changes,
> if..else was used which helps to reuse most of the other bits of code.
> To address your comment, can you please let me know how we could avoid
> this if..else without duplicating the too much code. I am not clear
> about " module private functions or even a library" comment that you
> have made. Can you please help me with this.
> 
> Thanks,
> Jagadeesh.
> 
> > Regards,
> > Jian
> >
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > > Jagadeesh Ujja
> > > 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: [edk2] [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/FaultTolerantWriteStandalon
> > > eMm.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                                                            |  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/StandaloneMmCoreHob
> > > Lib.inf                   |   2 +-
> > >
> > >
> StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/StandaloneMmCore
> > > HobLibInternal.c         |  64 ++
> > >  StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
> > > | 655 ++++++++++++++++
> > >  StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
> > > |  48 ++
> > >
> > >
> StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMm
> > > MemoryAllocationLib.c   | 824 ++++++++++++++++++++
> > >
> > >
> StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMm
> > > MemoryAllocationLib.inf |  45 ++
> > >
> > >
> StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServi
> > > cesTableLib.c         |  64 ++
> > >
> > >
> StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServi
> > > cesTableLib.inf       |  36 +
> > >  39 files changed, 2929 insertions(+), 244 deletions(-)
> > >  create mode 100644
> > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
> > >  create mode 100644
> > >
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandalon
> > > eMm.inf
> > >  create mode 100644
> > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
> > >  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.inf
> > >  create mode 100644
> > >
> StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/StandaloneMmCore
> > > HobLibInternal.c
> > >  create mode 100644
> > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
> > >  create mode 100644
> > > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
> > >  create mode 100644
> > >
> StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMm
> > > MemoryAllocationLib.c
> > >  create mode 100644
> > >
> StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMm
> > > MemoryAllocationLib.inf
> > >  create mode 100644
> > >
> StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServi
> > > cesTableLib.c
> > >  create mode 100644
> > >
> StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmServi
> > > cesTableLib.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