From: "Gao, Liming" <liming.gao@intel.com>
To: Jagadeesh Ujja <jagadeesh.ujja@arm.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
"Zhang, Chao B" <chao.b.zhang@intel.com>,
"leif.lindholm@linaro.org" <leif.lindholm@linaro.org>
Subject: Re: [RFC PATCH v4 00/12] Extend secure variable service to be usable from Standalone MM
Date: Wed, 12 Dec 2018 15:13:56 +0000 [thread overview]
Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E38BAC8@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <1544509302-1000-1-git-send-email-jagadeesh.ujja@arm.com>
This version is better. I have some comments on edk2 coding style.
1. This patch set can't be applied in edk2 trunk. Seemly, they base on previous version edk2.
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.
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.
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.
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.
> -----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
next prev parent reply other threads:[~2018-12-12 15:14 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-11 6:21 [RFC PATCH v4 00/12] Extend secure variable service to be usable from Standalone MM Jagadeesh Ujja
2018-12-11 6:21 ` [RFC PATCH v4 01/12] StandaloneMmPkg: Pull in additonal libraries from staging branch Jagadeesh Ujja
2018-12-11 6:21 ` [RFC PATCH v4 02/12] MdePkg: Add a PCD to enable secure storage of variables Jagadeesh Ujja
2018-12-11 6:21 ` [RFC PATCH v4 03/12] MdePkg/Include: add StandaloneMmServicesTableLib header file Jagadeesh Ujja
2018-12-11 6:21 ` [RFC PATCH v4 04/12] MdePkg/Library/BaseLib/AArch64: Add AsmLfence function Jagadeesh Ujja
2018-12-11 6:21 ` [RFC PATCH v4 05/12] MdePkg/Library: Add StandaloneMmRuntimeDxe library Jagadeesh Ujja
2018-12-11 6:21 ` [RFC PATCH v4 06/12] MdeModulePkg/FaultTolerantWriteDxe: allow reusability as a MM driver Jagadeesh Ujja
2018-12-11 6:21 ` [RFC PATCH v4 07/12] MdeModulePkg/Variable/RuntimeDxe: adapt for usability with MM Standalone Jagadeesh Ujja
2018-12-11 6:21 ` [RFC PATCH v4 08/12] MdeModulePkg/Variable/RuntimeDxe: adapt as a MM Standalone driver Jagadeesh Ujja
2018-12-11 6:21 ` [RFC PATCH v4 09/12] MdeModulePkg/VarCheckLib: allow MM_STANDALONE drivers to use this library Jagadeesh Ujja
2018-12-11 6:21 ` [RFC PATCH v4 10/12] ArmPlatformPkg/NorFlashDxe: allow reusability as a MM driver Jagadeesh Ujja
2018-12-11 6:21 ` [RFC PATCH v4 11/12] SecurityPkg/AuthVariableLib: allow MM_STANDALONE drivers to use this library Jagadeesh Ujja
2018-12-17 4:50 ` Zhang, Chao B
2018-12-11 6:21 ` [RFC PATCH v4 12/12] CryptoPkg/BaseCryptLib: " Jagadeesh Ujja
2018-12-12 15:13 ` Gao, Liming [this message]
2018-12-13 12:00 ` [RFC PATCH v4 00/12] Extend secure variable service to be usable from Standalone MM Jagadeesh Ujja
2018-12-13 14:32 ` Gao, Liming
2018-12-14 11:10 ` Jagadeesh Ujja
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4A89E2EF3DFEDB4C8BFDE51014F606A14E38BAC8@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox