From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::d41; helo=mail-io1-xd41.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io1-xd41.google.com (mail-io1-xd41.google.com [IPv6:2607:f8b0:4864:20::d41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 644FC211ACEBE for ; Mon, 7 Jan 2019 05:32:36 -0800 (PST) Received: by mail-io1-xd41.google.com with SMTP id b23so255224ios.10 for ; Mon, 07 Jan 2019 05:32:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=keX5xP71nys82b7IUpFtnfXCbMQvmOYD1UbV2aNMT6E=; b=YoFcT7QSu+da6EA1fnYmDf5b9+qrIev1zlrAmNByiJVhtDdenlUsuMOieXD1nlYtL7 E6DG84CvDx3JwEf57QPy4X6U1+nBLv2+bOxIPeFuc5aywGvJ5rA4F2oRwZleiwYPQ3kj 8H/1Uz+u+t/rb2MUNJIoHCSEaf4EBFOO2pvmM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=keX5xP71nys82b7IUpFtnfXCbMQvmOYD1UbV2aNMT6E=; b=KB9pUgDIvUIm7mRNDrdPHjMEgXlS97U2nRZUXgCYCW8RNxhk6fvNHD4tjHaTfONEvy gdOr3i27ztmU4qiNlUd7jPuSNXLvJwQ0wgp5wibxfgMTc7YXO1rq0+ixFhHoidiZXMMR oFebznVDYJm/5xKO+kY056FkkFZ/6mTUugqSReF7I6QyqXTgCHv9tIqU2rnyEGiZ9aX4 jJmMy1/gw7B+D86n50l9xizhDTptRm/8yDgRB9w3rlquSdZ8mW611VlcO6gHoMsnE60/ cSSgQSBgXC8YGSknMScxSfkggw/CXbNlwQEk9Qfgly8WzCcgeYYlQI+yDQJwBr4ucKT0 BVKg== X-Gm-Message-State: AJcUukfzfUVQ8CHyQbOnN+x2v3HDDd6KKyfdH2ulhMSQz8Td8aGUb/iz 25phyvm+WCEuV+BCQcGW5d1iku3BkcZVea6dz48F0g== X-Google-Smtp-Source: ALg8bN6ZnBHVdj04zWwlsPVOS77WIqOs+/3cYpsvjXFBqGm2iUYlBSY8qRiFvBPDFMRqLZFxc/RRzZi84tq1cU1dKRw= X-Received: by 2002:a6b:5d01:: with SMTP id r1mr40829024iob.170.1546867955007; Mon, 07 Jan 2019 05:32:35 -0800 (PST) MIME-Version: 1.0 References: <1546866566-21085-1-git-send-email-jagadeesh.ujja@arm.com> In-Reply-To: <1546866566-21085-1-git-send-email-jagadeesh.ujja@arm.com> From: Ard Biesheuvel Date: Mon, 7 Jan 2019 14:32:23 +0100 Message-ID: To: Jagadeesh Ujja Cc: "edk2-devel@lists.01.org" , "Gao, Liming" , "Zhang, Chao B" , Leif Lindholm , Thomas Panakamattam Abraham , Achin Gupta , Supreeth Venkatesh , Jian J Wang Subject: Re: [PATCH v3 00/11] Extend secure variable service to be usable from Standalone MM X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Jan 2019 13:32:36 -0000 X-List-Received-Date: Mon, 07 Jan 2019 13:32:36 -0000 X-List-Received-Date: Mon, 07 Jan 2019 13:32:36 -0000 X-List-Received-Date: Mon, 07 Jan 2019 13:32:36 -0000 X-List-Received-Date: Mon, 07 Jan 2019 13:32:36 -0000 X-List-Received-Date: Mon, 07 Jan 2019 13:32:36 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 7 Jan 2019 at 14:09, Jagadeesh Ujja wrote: > > In-Reply-To: > > Changes since v2: > -Addressed the comments from Jian Wang > - CommonMmServicesLib library implemented in MdePkg. > - Picked the Reviewed-by tags from Ard Biesheuvel. > Jagadeesh, I have been very clear about how the PCD approach is not acceptable. Also, as I have attempted to explain, gMmst and gSmst are not fundamentally, different, so having a library that encapsulates choosing between the two for each MMST service is wrong as well. I am happy to discuss this further, and to explain in more detail if necess= ary. However, *please* stop sending new revisions of your series until we reached an agreement between us and with the MdePkg maintainers. This way, you are just wasting reviewer bandwidth. --=20 Ard. > Changes since v1: > -Addressed the comments from Liming Gao > - StandaloneMmServicesTableLib library implemented in MdePkg. > - Addressed all the comments from Ard Biesheuvel. > - For comment from Jian Wang about avoiding if..else, this > requires a bit more clarity and so this comment has not > been addressed. > - All the patches in this series can be pulled from > https://github.com/jagadeeshujja/edk2.git branch: topics/aarch64_secure= _vars > > 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 =E2=80=9CTimerWrapp.c=E2=80=9D file and add a ap= propriate > 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 fo= r > use with Standalone MM. This is applicable to paltforms that use Standalo= ne > Management Mode to protect access to non-volatile memory (NOR flash in ca= se > 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 vari= able > service implementation supports only the traditional MM mode and so the r= est > of the patches extends the existing secure variable service support to be > useable with Standalone MM mode as well. > > Jagadeesh Ujja (11): > StandaloneMmPkg: Remove MM_STANDALONE LIBRARY_CLASS from > StandaloneMmCoreHobLib > StandaloneMmPkg: Adding the library packages used by MM_STANDALONE > drivers > MdeModulePkg: Add a PCD to indicate Standalone MM supports secure > variable > MdePkg/Include: Add StandaloneMmServicesTableLib library > MdePkg/Library: Add CommonMmServicesLib 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 > CryptoPkg/BaseCryptLib: allow MM_STANDALONE drivers to use SmmCryptLib > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h = | 4 +- > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf = | 2 + > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c = | 96 +- > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c = | 1339 ++++++++++++++++++++ > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf = | 76 ++ > CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf = | 2 +- > MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf = | 5 +- > MdeModulePkg/MdeModulePkg.dec = | 5 + > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf = | 1 + > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c = | 149 ++- > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf = | 4 +- > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandalon= eMm.inf | 102 ++ > 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 = | 165 ++- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf = | 2 + > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c = | 31 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf = | 3 + > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf = | 133 ++ > MdePkg/Include/Library/CommonMmServicesLibrary.h = | 131 ++ > MdePkg/Include/Library/StandaloneMmServicesTableLib.h = | 43 + > MdePkg/Library/CommonMmServicesLibrary/CommonMmServicesLibrary.c = | 224 ++++ > MdePkg/Library/CommonMmServicesLibrary/CommonMmServicesLibrary.inf = | 42 + > MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib= .c | 39 + > MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib= .inf | 36 + > MdePkg/MdePkg.dec = | 4 + > StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.in= f | 2 +- > StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/StandaloneMmCoreHobLi= bInternal.c | 64 + > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c = | 651 ++++++++++ > StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf = | 48 + > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemo= ryAllocationLib.c | 823 ++++++++++++ > StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemo= ryAllocationLib.inf | 45 + > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmService= sTableLib.c | 64 + > StandaloneMmPkg/Library/StandaloneMmServicesTableLib/StandaloneMmService= sTableLib.inf | 36 + > 35 files changed, 4257 insertions(+), 179 deletions(-) > create mode 100644 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandalone= Mm.c > create mode 100644 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandalone= Mm.inf > create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTol= erantWriteStandaloneMm.inf > create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSt= andaloneMm.inf > create mode 100644 MdePkg/Include/Library/CommonMmServicesLibrary.h > create mode 100644 MdePkg/Include/Library/StandaloneMmServicesTableLib.h > create mode 100644 MdePkg/Library/CommonMmServicesLibrary/CommonMmServic= esLibrary.c > create mode 100644 MdePkg/Library/CommonMmServicesLibrary/CommonMmServic= esLibrary.inf > create mode 100644 MdePkg/Library/StandaloneMmServicesTableLib/Standalon= eMmServicesTableLib.c > create mode 100644 MdePkg/Library/StandaloneMmServicesTableLib/Standalon= eMmServicesTableLib.inf > create mode 100644 StandaloneMmPkg/Library/StandaloneMmHobLib/AArch64/St= andaloneMmCoreHobLibInternal.c > create mode 100644 StandaloneMmPkg/Library/StandaloneMmHobLib/Standalone= MmHobLib.c > create mode 100644 StandaloneMmPkg/Library/StandaloneMmHobLib/Standalone= MmHobLib.inf > create mode 100644 StandaloneMmPkg/Library/StandaloneMmMemoryAllocationL= ib/StandaloneMmMemoryAllocationLib.c > create mode 100644 StandaloneMmPkg/Library/StandaloneMmMemoryAllocationL= ib/StandaloneMmMemoryAllocationLib.inf > create mode 100644 StandaloneMmPkg/Library/StandaloneMmServicesTableLib/= StandaloneMmServicesTableLib.c > create mode 100644 StandaloneMmPkg/Library/StandaloneMmServicesTableLib/= StandaloneMmServicesTableLib.inf > > -- > 2.7.4 >