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:[email protected]]
> Sent: Wednesday, January 02, 2019 9:19 PM
> To: Wang, Jian J <[email protected]>
> Cc: [email protected]; Gao, Liming <[email protected]>; Zhang, Chao
> B <[email protected]>; [email protected];
> [email protected]
> 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 <[email protected]> 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:[email protected]] On Behalf Of
> > > Jagadeesh Ujja
> > > Sent: Friday, December 14, 2018 8:13 PM
> > > To: [email protected]; Gao, Liming <[email protected]>; Zhang,
> Chao
> > > B <[email protected]>; [email protected];
> > > [email protected]
> > > 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
> > > [email protected]
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> > _______________________________________________
> > edk2-devel mailing list
> > [email protected]
> > https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
