From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::441; helo=mail-wr1-x441.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com [IPv6:2a00:1450:4864:20::441]) (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 524AF21959CB2 for ; Wed, 20 Mar 2019 10:43:24 -0700 (PDT) Received: by mail-wr1-x441.google.com with SMTP id d17so3729333wre.10 for ; Wed, 20 Mar 2019 10:43:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=yLKI7J/8MRRmKl1Ao+igNBdFyf/uwgtBVDF4+Y8ThFY=; b=PjRnovAGTGQPEmo5C6an604jzLMzQWsIzoZ91GJe3j/He7JhLQSTTMgzqrLCka/BZj DggS3SkgwA+cV/dEZblH+B7ENIxTL/SD1sgybnC05j+ftUJ6dSQK6Bm4AIVpqlJplgB6 7sylpCoorIcPSn93XowaFsOrICRtzTGacDhC/3DFS+QEUILR61ZqwUY+7bahMtCaa8M6 ZCnLC3VzAAVrWh4z7x3bfLSMbgH4fBaskKVaT3EmL4/ned144wONHzp/SANnEQGQbtzk VQDMY5wW9/9hLN4CGZYtCS/Mv0zyuOwewJ+pyK4qVODA2kNANzaQU9tTQ+KSLQJ8f+JH 6bNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=yLKI7J/8MRRmKl1Ao+igNBdFyf/uwgtBVDF4+Y8ThFY=; b=HNyMsAj3u2lpLo+Z68ouA996j5PvdHvfcSLwEbaf6M/60fYpq7plYs6m1ZwleHcFQg 411AQI4h6lQwtry0E7WKyis3Eny/yct9QvFZXZrC6JPDiwdCzK3HeGbzEzpEoEAsExnx u+HR1Dh0m+ZuxxyaeXJf0J2LB52WjZYb0DosRATnMdATV+gJ2JHsyqRKQdzItDH1OllF 4qNGnwtanOCgDMOqDxbf5xBkBTA7WigLONRu8UmSds6pbce5W8L3E/2Tl/shVoHO02ot JX0TBeS2q7/apbch2CVPvJI6bn1Pw/1u9TRd0Re/KKW+/W4+K4dsI0198uprCPzrsWKB QHKQ== X-Gm-Message-State: APjAAAUb+lfyhhNKOX+QMa3ajCTMNUX2gUNR3u6G38dt4rqs+Ca0bka2 TKdsSnV21nDyBERduXXvhqw4wg== X-Google-Smtp-Source: APXvYqwAaRHfhNHpfoA+9feE0Qolj+/qYx0sHOoIYbgmpZX1stv2nJzAJAbdydidiGpTeqSK2Wo+sw== X-Received: by 2002:adf:fdcd:: with SMTP id i13mr21718024wrs.212.1553103802672; Wed, 20 Mar 2019 10:43:22 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 93sm5421319wrh.15.2019.03.20.10.43.21 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 20 Mar 2019 10:43:21 -0700 (PDT) Date: Wed, 20 Mar 2019 17:43:20 +0000 From: Leif Lindholm To: Laszlo Ersek Cc: edk2-devel@lists.01.org, ard.biesheuvel@linaro.org, Jian J Wang , Hao Wu , Ray Ni , Star Zeng , Andrew Fish , Michael D Kinney Message-ID: <20190320174320.rzuprck56u2f5d5u@bivouac.eciton.net> References: <20190318145625.29000-1-leif.lindholm@linaro.org> <87af2f8b-a8f7-a437-a4ab-019178bb1f13@redhat.com> MIME-Version: 1.0 In-Reply-To: <87af2f8b-a8f7-a437-a4ab-019178bb1f13@redhat.com> User-Agent: NeoMutt/20170113 (1.7.2) 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 17:43:25 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Mar 20, 2019 at 03:51:39PM +0100, Laszlo Ersek wrote: > 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. OK, I buy that argument. *But* I still think the IA32/X64 specific library mappings should be moved out of .common LibraryClass sections. Regards, Leif > Reviewed-by: Laszlo Ersek > > Thanks > Laszlo