From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0C9C92194EB70 for ; Wed, 20 Mar 2019 07:51:43 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9B9A2C05D3F9; Wed, 20 Mar 2019 14:51:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-91.rdu2.redhat.com [10.10.120.91]) by smtp.corp.redhat.com (Postfix) with ESMTP id 63A635D71C; Wed, 20 Mar 2019 14:51:40 +0000 (UTC) To: Leif Lindholm , edk2-devel@lists.01.org Cc: ard.biesheuvel@linaro.org, Jian J Wang , Hao Wu , Ray Ni , Star Zeng , Andrew Fish , Michael D Kinney References: <20190318145625.29000-1-leif.lindholm@linaro.org> From: Laszlo Ersek Message-ID: <87af2f8b-a8f7-a437-a4ab-019178bb1f13@redhat.com> Date: Wed, 20 Mar 2019 15:51:39 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190318145625.29000-1-leif.lindholm@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 20 Mar 2019 14:51:42 +0000 (UTC) Subject: Re: [RFC PATCH] MdeModulePkg: add LockBoxNullLib for !IA32/X64 in .dsc 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: Wed, 20 Mar 2019 14:51:44 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Leif, On 03/18/19 15:56, Leif Lindholm wrote: > Commit 05fd2a926833 > ("MdeModulePkg/NvmExpressPei: Consume S3StorageDeviceInitList LockBox") > added a dependency on LockBoxLib to NvmExpressPei, causing builds using > MdeModulePkg.dsc to fail on architectures other than IA32/X64 with > missing reference to > gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode. > > Add a resolution for LockBoxNullLib for ARM/AARCH64 to restore builds. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Leif Lindholm > --- > > Note: this patch hides the symptom, but this isn't really the fix I > would like to see. > > The build error is caused by the chain of: > 1) NvmExpressPei depending on LockBoxLib > 2) LockBoxLib being mapped to SmmLockBoxPeiLib in [LibraryClasses.common.PEIM] > 3) SmmLockBoxPeiLib depending on PcdDxeIplSwitchToLongMode > 4) PcdDxeIplSwitchToLongMode being declared in > [PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64] in MdeModulePkg.dsc > > Now, an alternative quick-fix would be to move the PEIM LockBoxLib mapping > into a [LibraryClasses.IA32.PEIM, LibraryClasses.X64.PEIM] > section. But that would leave NvmExpressPei unbuildable on anything not > IA32/X64. > > Another option would be to add default declaration (for all other > architectures) of FALSE for PcdDxeIplSwitchToLongMode in MdeModulePkg.dec, > but the current way this is expressed seems to treat this as an > architecture-specific feature (which it is). > > What I believe would be the cleanest solution would be to abstract > NvmExpressPei to the point where it can function without the LockBoxLib. > But regardless, it does not look valid to me for something as > architecture-specific as MdeModulePkg/Library/SmmLockBoxLib/ to live under > .common sections in the .dsc. (And if this changes at some point, because we implement an ARM/AARCH64 equivalent based on StandaloneMmPkg, we will need > a major refactoring of that library anyway.) > > / > Leif > > MdeModulePkg/MdeModulePkg.dsc | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc > index 6cd1727a0d..6e27e9cb68 100644 > --- a/MdeModulePkg/MdeModulePkg.dsc > +++ b/MdeModulePkg/MdeModulePkg.dsc > @@ -178,6 +178,7 @@ [LibraryClasses.common.MM_STANDALONE] > [LibraryClasses.ARM, LibraryClasses.AARCH64] > ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf > ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf > + LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf > > # > # It is not possible to prevent ARM compiler calls to generic intrinsic functions. > I think this patch is exactly the right solution. The code added in commit 05fd2a926833 is gated by (BootMode == BOOT_ON_S3_RESUME). That condition can never evaluate to TRUE on ARM/AARCH64, presently. Accordingly, the stated goal of the commit doesn't apply to ARM/AARCH64: The purpose is to perform an on-demand (partial) NVM Express device enumeration/initialization to benefit the S3 resume performance. Given that the RestoreLockBox() calls are never reached (which is correct, by design, at the present level of ACPI S3 enablement in edk2 for ARM/AARCH64), causing the lockbox APIs to "do nothing beyond compile" is exactly right. IMO anyway. Once ARM/AARCH64 grow S3 support, a functional and secure LockBox will have to be part of that. Perhaps it will use "standalone MM"; I'm not sure. The point is, once the goal of the commit starts applying to ARM/AARCH64, a functional LockBox will have been implemented for ARM/AARCH64; and that lib instance will certainly not depend on PcdDxeIplSwitchToLongMode. Until such time, this patch is fine. Reviewed-by: Laszlo Ersek Thanks Laszlo