+Bob, Liming, On Tue, Aug 27, 2019 at 09:26:05PM +0200, Laszlo Ersek wrote: > 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 <leif.lindh...@linaro.org> > > Reported-by: Baptiste Gerondeau <baptiste.gerond...@linaro.org> > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Hao A Wu <hao.a...@intel.com> > > --- > > > > 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. <LibraryClasses> 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:
Yes. > > [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. Hmm. That's not great. Anyway, I stopped being lazy and did a bisect. The culprit is e8449e1d8e3b ("BaseTools: Decouple AutoGen Objects"), marked as resolving https://bugzilla.tianocore.org/show_bug.cgi?id=1875. This also affects SignedCapsulePkg/SignedCapsulePkg.dsc (although once addressed, AARCH64 also needs a NULL entry added for CompilerIntrinsicsLib. > (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. Yes. But it is also a fundamental change in tool behaviour introduced on 9 August. I am really uncomfortable about this making it into the release this week - but I also believe this is the foundation for the multiprocess autogen. Since you have very helpfully analyzed *what* changed ... would the better "fix" for 2019.08 be to intentionally break the new code to conform to the old behaviour - and then revert that patch after the tag? If we do that, this patch could then wait and indeed be merged as part of the same set. > 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. Yes, fair point. > > > > [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. Well, this fixes the current issue. I completely agree the file could benefit from some overall restructuring. > > [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 <ler...@redhat.com> Thanks! > Do wait for maintainer review, of course. Of course. Best Regards, Leif -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46489): https://edk2.groups.io/g/devel/message/46489 Mute This Topic: https://groups.io/mt/33045351/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-