From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 27 Aug 2019 12:26:16 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 33E6F307D8E3; Tue, 27 Aug 2019 19:26:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.80]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1C69E60CD0; Tue, 27 Aug 2019 19:26:12 +0000 (UTC) Subject: Re: [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) To: Leif Lindholm , devel@edk2.groups.io Cc: Andrew Fish , Michael D Kinney , Baptiste Gerondeau , Jian J Wang , Hao A Wu References: <20190827124328.9034-1-leif.lindholm@linaro.org> From: "Laszlo Ersek" Message-ID: <6ce1988a-bd79-893e-5d8d-724b98329ab9@redhat.com> Date: Tue, 27 Aug 2019 21:26:05 +0200 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: <20190827124328.9034-1-leif.lindholm@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.48]); Tue, 27 Aug 2019 19:26:16 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Leif, On 08/27/19 14:43, Leif Lindholm wrote: > Commit 4a1f6b85c184 > ("MdeModulePkg: add LockBoxNullLib for !IA32/X64 in .dsc") > added an ARM/AARCH64 resolution for LockBoxLib. However, this failed > to address the overrides provided for PEIM, DXE_DRIVER, > DXE_RUNTIME_DRIVER, DXE_SMM_DRIVER and UEFI_DRIVER, so any modules > of those classes still failed to build. > > Break these out properly into their own LibraryClasses sections. > > Resolves BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2134 > > Signed-off-by: Leif Lindholm > Reported-by: Baptiste Gerondeau > Cc: Jian J Wang > Cc: Hao A Wu > --- > > I don't understand how the above would appear to work back when I > submitted the previous patch but not work now, but I haven't dug > into it deeper. Including the x86-specific LockBoxLib in the > .common section is however clearly not correct. I agree with you that the present situation is not correct. According to: https://edk2-docs.gitbooks.io/edk-ii-dsc-specification/2_dsc_overview/26_[libraryclasses]_section_processing.html the library class resolutions take effect in the following order (entries near the top have higher priority): > 1. associated with the INF file in the [Components] section > 2. [LibraryClasses.$(Arch).$(MODULE_TYPE), LibraryClasses.$(Arch).$(MODULE_TYPE)] > 3. [LibraryClasses.$(Arch).$(MODULE_TYPE)] > 4. [LibraryClasses.common.$(MODULE_TYPE)] > 5. [LibraryClasses.$(Arch)] > 6. [LibraryClasses.common] or [LibraryClasses] (Side comment 1: levels #2 and #3 look very similar; I think the difference is that #2 is supposed to be a multi-arch and/or multi-module-type section, while #3 is a single-arch and single-module-type section.) Commit 4a1f6b85c184 ("MdeModulePkg: add LockBoxNullLib for !IA32/X64 in .dsc", 2019-03-27) provided a LockBoxLib resolution at level #5: > [LibraryClasses.ARM, LibraryClasses.AARCH64] However, the other LockBoxLib resolutions are at level #4: > [LibraryClasses.common.PEIM] > [LibraryClasses.common.DXE_DRIVER] > [LibraryClasses.common.DXE_RUNTIME_DRIVER] > [LibraryClasses.common.DXE_SMM_DRIVER] > [LibraryClasses.common.UEFI_DRIVER] So the latter taking priority is actually specified behavior. (Side comment 2: EBC is in the same boat, from commit cbcccd2c9d93 ("Update Code to pass EBC compiler", 2013-05-13): > [LibraryClasses.EBC] > LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf ) As to why this breakage was not exposed right at commit 4a1f6b85c184 -- I have no idea. Perhaps it was hidden by a BaseTools issue that has been fixed meanwhile. On 08/27/19 14:43, Leif Lindholm wrote: > I think a fix for this issue needs to go into 2019.08, I agree the problem should be fixed in 2019.08 -- taking your word that commit 4a1f6b85c184 *appeared* to fix the MdeModulePkg.dsc build for ARM/AARCH64, we now have a regression since that commit (dated 2019-03-27). > but if someone has a prettier suggestion, I am not wedded to this one. I think this is good enough. The lib class resolutions are raised to level #2, but they will no longer match ARM / AARCH64, so your level#5 addition from commit 4a1f6b85c184 will take effect. > > MdeModulePkg/MdeModulePkg.dsc | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc > index 4320839abfb5..15ba96cecbed 100644 > --- a/MdeModulePkg/MdeModulePkg.dsc > +++ b/MdeModulePkg/MdeModulePkg.dsc > @@ -109,6 +109,8 @@ [LibraryClasses.common.PEIM] > HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf > MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf > ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedSectionLib/PeiExtractGuidedSectionLib.inf > + > +[LibraryClasses.IA32.PEIM,LibraryClasses.X64.PEIM] > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf (1) I suggest replacing "," with ", ". (That's more consistent with preexistent section headers in the DSC file.) Applies to the other new section headers too. > > [LibraryClasses.common.DXE_CORE] > @@ -118,18 +120,22 @@ [LibraryClasses.common.DXE_CORE] > > [LibraryClasses.common.DXE_DRIVER] > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > - LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf > ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf > > +[LibraryClasses.IA32.DXE_DRIVER,LibraryClasses.X64.DXE_DRIVER] > + LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf > + > [LibraryClasses.common.DXE_RUNTIME_DRIVER] > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf > - LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf > > +[LibraryClasses.IA32.DXE_RUNTIME_DRIVER,LibraryClasses.X64.DXE_RUNTIME_DRIVER] > + LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf > + > [LibraryClasses.common.SMM_CORE] > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf > @@ -143,13 +149,17 @@ [LibraryClasses.common.DXE_SMM_DRIVER] > MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf > MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf > SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf > + SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf > + > +[LibraryClasses.IA32.DXE_SMM_DRIVER,LibraryClasses.X64.DXE_SMM_DRIVER] > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.inf > - SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf > I wonder if this is really necessary. I would assume all the DXE_SMM_DRIVER modules are listed under [Components.IA32, Components.X64] already. But, this hunk certainly doesn't hurt. > [LibraryClasses.common.UEFI_DRIVER] > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf > + > +[LibraryClasses.IA32.UEFI_DRIVER,LibraryClasses.X64.UEFI_DRIVER] > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf > > [LibraryClasses.common.UEFI_APPLICATION] > With (1) fixed (feel free to correct that just before pushing): Reviewed-by: Laszlo Ersek Do wait for maintainer review, of course. Thanks, Laszlo