* [PATCH 1/1] MdeModulePkg: fix !x86 builds (more)
@ 2019-08-27 12:43 Leif Lindholm
2019-08-27 19:26 ` Laszlo Ersek
0 siblings, 1 reply; 10+ messages in thread
From: Leif Lindholm @ 2019-08-27 12:43 UTC (permalink / raw)
To: devel
Cc: Andrew Fish, Laszlo Ersek, Michael D Kinney, Baptiste Gerondeau,
Jian J Wang, Hao A Wu
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.lindholm@linaro.org>
Reported-by: Baptiste Gerondeau <baptiste.gerondeau@linaro.org>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@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 think a fix for this issue needs to go into 2019.08, but if
someone has a prettier suggestion, I am not wedded to this one.
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
[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
[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]
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) 2019-08-27 12:43 [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) Leif Lindholm @ 2019-08-27 19:26 ` Laszlo Ersek 2019-08-27 20:59 ` Leif Lindholm 2019-08-27 21:00 ` Michael D Kinney 0 siblings, 2 replies; 10+ messages in thread From: Laszlo Ersek @ 2019-08-27 19:26 UTC (permalink / raw) To: Leif Lindholm, devel Cc: Andrew Fish, Michael D Kinney, Baptiste Gerondeau, Jian J Wang, Hao A Wu 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.lindholm@linaro.org> > Reported-by: Baptiste Gerondeau <baptiste.gerondeau@linaro.org> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Hao A Wu <hao.a.wu@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: > [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 <lersek@redhat.com> Do wait for maintainer review, of course. Thanks, Laszlo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) 2019-08-27 19:26 ` Laszlo Ersek @ 2019-08-27 20:59 ` Leif Lindholm 2019-08-27 21:26 ` [edk2-devel] " Michael D Kinney 2019-08-28 1:40 ` Wu, Hao A 2019-08-27 21:00 ` Michael D Kinney 1 sibling, 2 replies; 10+ messages in thread From: Leif Lindholm @ 2019-08-27 20:59 UTC (permalink / raw) To: Laszlo Ersek Cc: devel, Andrew Fish, Michael D Kinney, Baptiste Gerondeau, Jian J Wang, Hao A Wu, Feng, Bob C, Liming Gao +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.lindholm@linaro.org> > > Reported-by: Baptiste Gerondeau <baptiste.gerondeau@linaro.org> > > Cc: Jian J Wang <jian.j.wang@intel.com> > > Cc: Hao A Wu <hao.a.wu@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 <lersek@redhat.com> Thanks! > Do wait for maintainer review, of course. Of course. Best Regards, Leif ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) 2019-08-27 20:59 ` Leif Lindholm @ 2019-08-27 21:26 ` Michael D Kinney 2019-08-28 1:10 ` Bob Feng 2019-08-28 1:40 ` Wu, Hao A 1 sibling, 1 reply; 10+ messages in thread From: Michael D Kinney @ 2019-08-27 21:26 UTC (permalink / raw) To: devel@edk2.groups.io, leif.lindholm@linaro.org, Laszlo Ersek, Kinney, Michael D Cc: Andrew Fish, Baptiste Gerondeau, Wang, Jian J, Wu, Hao A, Feng, Bob C, Gao, Liming Leif, Looking at the DSC Spec in looks like the priority change from Aug 9 was to invert the priority 4 and priority 5 items: 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] If BaseTools were updated to make (5) higher priority than (4), then the previous behavior would be restored and no DSC file changes would be required. This means an arch specific mapping for all module types is higher priority than a module specific mapping for all archs. We can update DSC Spec to match the behavior that has been implemented for a long time. Mike > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] > On Behalf Of Leif Lindholm > Sent: Tuesday, August 27, 2019 1:59 PM > To: Laszlo Ersek <lersek@redhat.com> > Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; > Kinney, Michael D <michael.d.kinney@intel.com>; Baptiste > Gerondeau <baptiste.gerondeau@linaro.org>; Wang, Jian J > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; > Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming > <liming.gao@intel.com> > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: fix > !x86 builds (more) > > +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.lindholm@linaro.org> > > > Reported-by: Baptiste Gerondeau > <baptiste.gerondeau@linaro.org> > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > Cc: Hao A Wu <hao.a.wu@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/LockBoxNu > llLib.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/PeiMemoryAllocationLi > b/PeiMemoryAllocationLib.inf > > > > > > > ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedS > ectionLib/Pe > > > iExtractGuidedSectionLib.inf > > > + > > > +[LibraryClasses.IA32.PEIM,LibraryClasses.X64.PEIM] > > > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > PeiLib.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/SmmLockBox > DxeLib.inf > > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationL > ib/UefiMemoryAllocationLib.inf > > > > ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedS > ectionLib/DxeExtractGuidedSectionLib.inf > > > > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCaps > uleLib.inf > > > > > > > +[LibraryClasses.IA32.DXE_DRIVER,LibraryClasses.X64.DXE_ > DRIVER] > > > + > > > > +LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBo > xDxeLib.inf > > > + > > > [LibraryClasses.common.DXE_RUNTIME_DRIVER] > > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationL > ib/UefiMemoryAllocationLib.inf > > > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibC > onOut.inf > > > - > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > DxeLib.inf > > > > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRunt > imeCapsuleLi > > > b.inf > > > > > > > +[LibraryClasses.IA32.DXE_RUNTIME_DRIVER,LibraryClasses. > X64.DXE_RUNT > > > +IME_DRIVER] > > > + > > > > +LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBo > xDxeLib.inf > > > + > > > [LibraryClasses.common.SMM_CORE] > > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > > > > > > MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemory > AllocationLi > > > b/PiSmmCoreMemoryAllocationLib.inf > > > @@ -143,13 +149,17 @@ > [LibraryClasses.common.DXE_SMM_DRIVER] > > > > MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLi > b/SmmMemoryAllocationLib.inf > > > > MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmS > ervicesTableLib.inf > > > > > > > SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/S > mmServicesTa > > > bleLib.inf > > > + SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf > > > + > > > > +[LibraryClasses.IA32.DXE_SMM_DRIVER,LibraryClasses.X64. > DXE_SMM_DRIV > > > +ER] > > > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > SmmLib.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/UefiMemoryAllocationL > ib/UefiMemoryAllocationLib.inf > > > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibC > onOut.inf > > > + > > > > +[LibraryClasses.IA32.UEFI_DRIVER,LibraryClasses.X64.UEF > I_DRIVER] > > > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > DxeLib.inf > > > > > > [LibraryClasses.common.UEFI_APPLICATION] > > > > > > > With (1) fixed (feel free to correct that just before > pushing): > > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > Thanks! > > > Do wait for maintainer review, of course. > > Of course. > > Best Regards, > > Leif > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) 2019-08-27 21:26 ` [edk2-devel] " Michael D Kinney @ 2019-08-28 1:10 ` Bob Feng 0 siblings, 0 replies; 10+ messages in thread From: Bob Feng @ 2019-08-28 1:10 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io, leif.lindholm@linaro.org, Laszlo Ersek Cc: Andrew Fish, Baptiste Gerondeau, Wang, Jian J, Wu, Hao A, Gao, Liming I'll work on this Basetools regression issue today. Thanks, Bob -----Original Message----- From: Kinney, Michael D Sent: Wednesday, August 28, 2019 5:27 AM To: devel@edk2.groups.io; leif.lindholm@linaro.org; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com> Cc: Andrew Fish <afish@apple.com>; Baptiste Gerondeau <baptiste.gerondeau@linaro.org>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com> Subject: RE: [edk2-devel] [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) Leif, Looking at the DSC Spec in looks like the priority change from Aug 9 was to invert the priority 4 and priority 5 items: 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] If BaseTools were updated to make (5) higher priority than (4), then the previous behavior would be restored and no DSC file changes would be required. This means an arch specific mapping for all module types is higher priority than a module specific mapping for all archs. We can update DSC Spec to match the behavior that has been implemented for a long time. Mike > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Leif Lindholm > Sent: Tuesday, August 27, 2019 1:59 PM > To: Laszlo Ersek <lersek@redhat.com> > Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; Kinney, > Michael D <michael.d.kinney@intel.com>; Baptiste Gerondeau > <baptiste.gerondeau@linaro.org>; Wang, Jian J <jian.j.wang@intel.com>; > Wu, Hao A <hao.a.wu@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; > Gao, Liming <liming.gao@intel.com> > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: fix > !x86 builds (more) > > +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.lindholm@linaro.org> > > > Reported-by: Baptiste Gerondeau > <baptiste.gerondeau@linaro.org> > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > Cc: Hao A Wu <hao.a.wu@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/LockBoxNu > llLib.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/PeiMemoryAllocationLi > b/PeiMemoryAllocationLib.inf > > > > > > > ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedS > ectionLib/Pe > > > iExtractGuidedSectionLib.inf > > > + > > > +[LibraryClasses.IA32.PEIM,LibraryClasses.X64.PEIM] > > > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > PeiLib.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/SmmLockBox > DxeLib.inf > > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationL > ib/UefiMemoryAllocationLib.inf > > > > ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedS > ectionLib/DxeExtractGuidedSectionLib.inf > > > > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCaps > uleLib.inf > > > > > > > +[LibraryClasses.IA32.DXE_DRIVER,LibraryClasses.X64.DXE_ > DRIVER] > > > + > > > > +LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBo > xDxeLib.inf > > > + > > > [LibraryClasses.common.DXE_RUNTIME_DRIVER] > > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationL > ib/UefiMemoryAllocationLib.inf > > > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibC > onOut.inf > > > - > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > DxeLib.inf > > > > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRunt > imeCapsuleLi > > > b.inf > > > > > > > +[LibraryClasses.IA32.DXE_RUNTIME_DRIVER,LibraryClasses. > X64.DXE_RUNT > > > +IME_DRIVER] > > > + > > > > +LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBo > xDxeLib.inf > > > + > > > [LibraryClasses.common.SMM_CORE] > > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > > > > > > MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemory > AllocationLi > > > b/PiSmmCoreMemoryAllocationLib.inf > > > @@ -143,13 +149,17 @@ > [LibraryClasses.common.DXE_SMM_DRIVER] > > > > MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLi > b/SmmMemoryAllocationLib.inf > > > > MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmS > ervicesTableLib.inf > > > > > > > SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/S > mmServicesTa > > > bleLib.inf > > > + SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf > > > + > > > > +[LibraryClasses.IA32.DXE_SMM_DRIVER,LibraryClasses.X64. > DXE_SMM_DRIV > > > +ER] > > > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > SmmLib.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/UefiMemoryAllocationL > ib/UefiMemoryAllocationLib.inf > > > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibC > onOut.inf > > > + > > > > +[LibraryClasses.IA32.UEFI_DRIVER,LibraryClasses.X64.UEF > I_DRIVER] > > > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > DxeLib.inf > > > > > > [LibraryClasses.common.UEFI_APPLICATION] > > > > > > > With (1) fixed (feel free to correct that just before > pushing): > > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > Thanks! > > > Do wait for maintainer review, of course. > > Of course. > > Best Regards, > > Leif > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) 2019-08-27 20:59 ` Leif Lindholm 2019-08-27 21:26 ` [edk2-devel] " Michael D Kinney @ 2019-08-28 1:40 ` Wu, Hao A 2019-08-28 5:24 ` Michael D Kinney 2019-08-28 9:24 ` [edk2-devel] " Leif Lindholm 1 sibling, 2 replies; 10+ messages in thread From: Wu, Hao A @ 2019-08-28 1:40 UTC (permalink / raw) To: Leif Lindholm, Laszlo Ersek, Kinney, Michael D Cc: devel@edk2.groups.io, Andrew Fish, Baptiste Gerondeau, Wang, Jian J, Feng, Bob C, Gao, Liming > -----Original Message----- > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > Sent: Wednesday, August 28, 2019 4:59 AM > To: Laszlo Ersek > Cc: devel@edk2.groups.io; Andrew Fish; Kinney, Michael D; Baptiste > Gerondeau; Wang, Jian J; Wu, Hao A; Feng, Bob C; Gao, Liming > Subject: Re: [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) > > +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.lindholm@linaro.org> > > > Reported-by: Baptiste Gerondeau <baptiste.gerondeau@linaro.org> > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > Cc: Hao A Wu <hao.a.wu@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/PeiMemory > AllocationLib.inf > > > > ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedSectionLib/PeiEx > tractGuidedSectionLib.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.in > f > > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemo > ryAllocationLib.inf > > > > ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeE > xtractGuidedSectionLib.inf > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf > > > > > > +[LibraryClasses.IA32.DXE_DRIVER,LibraryClasses.X64.DXE_DRIVER] > > > + > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.in > f > > > + > > > [LibraryClasses.common.DXE_RUNTIME_DRIVER] > > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemo > ryAllocationLib.inf > > > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf > > > - > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.in > f > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsule > Lib.inf > > > > > > > +[LibraryClasses.IA32.DXE_RUNTIME_DRIVER,LibraryClasses.X64.DXE_RUNTI > ME_DRIVER] > > > + > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.in > f > > > + > > > [LibraryClasses.common.SMM_CORE] > > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > > > MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryAllocatio > nLib/PiSmmCoreMemoryAllocationLib.inf > > > @@ -143,13 +149,17 @@ [LibraryClasses.common.DXE_SMM_DRIVER] > > > > MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMe > moryAllocationLib.inf > > > > MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTabl > eLib.inf > > > > SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesT > ableLib.inf > > > + SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf > > > + > > > > +[LibraryClasses.IA32.DXE_SMM_DRIVER,LibraryClasses.X64.DXE_SMM_DRI > VER] > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.i > nf > > > - 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/UefiMemo > ryAllocationLib.inf > > > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf > > > + > > > +[LibraryClasses.IA32.UEFI_DRIVER,LibraryClasses.X64.UEFI_DRIVER] > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.in > f > > > > > > [LibraryClasses.common.UEFI_APPLICATION] > > > > > > > With (1) fixed (feel free to correct that just before pushing): > > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > Thanks! > > > Do wait for maintainer review, of course. > > Of course. Per my understanding to the analysis from Leif, Laszlo and Mike, the patch will depend on the precedence of the below rules: * [LibraryClasses.common.$(MODULE_TYPE)] * [LibraryClasses.$(Arch)] So for now, we should wait for the final call on the above open, right? Best Regards, Hao Wu > > Best Regards, > > Leif ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) 2019-08-28 1:40 ` Wu, Hao A @ 2019-08-28 5:24 ` Michael D Kinney 2019-08-28 5:43 ` Wu, Hao A 2019-08-28 9:24 ` [edk2-devel] " Leif Lindholm 1 sibling, 1 reply; 10+ messages in thread From: Michael D Kinney @ 2019-08-28 5:24 UTC (permalink / raw) To: Wu, Hao A, Leif Lindholm, Laszlo Ersek, Kinney, Michael D Cc: devel@edk2.groups.io, Andrew Fish, Baptiste Gerondeau, Wang, Jian J, Feng, Bob C, Gao, Liming Hao Wu, Please provide a patch to BaseTools to restore the previous behavior. We need that to review the complexity of the change to determine what to do. Without that information today, the release date of this stable tag is at risk. Mike > -----Original Message----- > From: Wu, Hao A > Sent: Tuesday, August 27, 2019 6:40 PM > To: Leif Lindholm <leif.lindholm@linaro.org>; Laszlo > Ersek <lersek@redhat.com>; Kinney, Michael D > <michael.d.kinney@intel.com> > Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; > Baptiste Gerondeau <baptiste.gerondeau@linaro.org>; Wang, > Jian J <jian.j.wang@intel.com>; Feng, Bob C > <bob.c.feng@intel.com>; Gao, Liming > <liming.gao@intel.com> > Subject: RE: [PATCH 1/1] MdeModulePkg: fix !x86 builds > (more) > > > -----Original Message----- > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > > Sent: Wednesday, August 28, 2019 4:59 AM > > To: Laszlo Ersek > > Cc: devel@edk2.groups.io; Andrew Fish; Kinney, Michael > D; Baptiste > > Gerondeau; Wang, Jian J; Wu, Hao A; Feng, Bob C; Gao, > Liming > > Subject: Re: [PATCH 1/1] MdeModulePkg: fix !x86 builds > (more) > > > > +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.lindholm@linaro.org> > > > > Reported-by: Baptiste Gerondeau > <baptiste.gerondeau@linaro.org> > > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > > Cc: Hao A Wu <hao.a.wu@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.ht > > ml > > > > > > 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/LockBoxNul > lLib.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 > /PeiMemory > > AllocationLib.inf > > > > > > > ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedSe > ctionLib/PeiE > > ExtractGuidedSectionLib|x > > tractGuidedSectionLib.inf > > > > + > > > > +[LibraryClasses.IA32.PEIM,LibraryClasses.X64.PEIM] > > > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxP > eiLib.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/SmmLockBoxD > xeLib.in > > f > > > > > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLi > b/UefiMemo > > ryAllocationLib.inf > > > > > > > ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSe > ctionLib/DxeE > > xtractGuidedSectionLib.inf > > > > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsu > leLib.inf > > > > > > > > > +[LibraryClasses.IA32.DXE_DRIVER,LibraryClasses.X64.DXE_D > RIVER] > > > > + > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD > xeLib.in > > f > > > > + > > > > [LibraryClasses.common.DXE_RUNTIME_DRIVER] > > > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > > > > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLi > b/UefiMemo > > ryAllocationLib.inf > > > > > > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibCo > nOut.inf > > > > - > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD > xeLib.in > > f > > > > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRunti > meCapsule > > Lib.inf > > > > > > > > > > > +[LibraryClasses.IA32.DXE_RUNTIME_DRIVER,LibraryClasses.X > 64.DXE_RUNTI > > ME_DRIVER] > > > > + > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD > xeLib.in > > f > > > > + > > > > [LibraryClasses.common.SMM_CORE] > > > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > > > > > > MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryA > llocatio > > nLib/PiSmmCoreMemoryAllocationLib.inf > > > > @@ -143,13 +149,17 @@ > [LibraryClasses.common.DXE_SMM_DRIVER] > > > > > > > MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib > /SmmMe > > moryAllocationLib.inf > > > > > > > MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmSe > rvicesTabl > > eLib.inf > > > > > > > SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/Sm > mServicesT > > ableLib.inf > > > > + SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf > > > > + > > > > > > > +[LibraryClasses.IA32.DXE_SMM_DRIVER,LibraryClasses.X64.D > XE_SMM_DRI > > VER] > > > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxS > mmLib.i > > nf > > > > - 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/UefiMemoryAllocationLi > b/UefiMemo > > ryAllocationLib.inf > > > > > > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibCo > nOut.inf > > > > + > > > > > +[LibraryClasses.IA32.UEFI_DRIVER,LibraryClasses.X64.UEFI > _DRIVER] > > > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD > xeLib.in > > f > > > > > > > > [LibraryClasses.common.UEFI_APPLICATION] > > > > > > > > > > With (1) fixed (feel free to correct that just before > pushing): > > > > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > > > Thanks! > > > > > Do wait for maintainer review, of course. > > > > Of course. > > > Per my understanding to the analysis from Leif, Laszlo > and Mike, the patch will depend on the precedence of the > below rules: > > * [LibraryClasses.common.$(MODULE_TYPE)] > * [LibraryClasses.$(Arch)] > > So for now, we should wait for the final call on the > above open, right? > > Best Regards, > Hao Wu > > > > > > Best Regards, > > > > Leif ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) 2019-08-28 5:24 ` Michael D Kinney @ 2019-08-28 5:43 ` Wu, Hao A 0 siblings, 0 replies; 10+ messages in thread From: Wu, Hao A @ 2019-08-28 5:43 UTC (permalink / raw) To: Kinney, Michael D, Leif Lindholm, Laszlo Ersek, Feng, Bob C Cc: devel@edk2.groups.io, Andrew Fish, Baptiste Gerondeau, Wang, Jian J, Gao, Liming > -----Original Message----- > From: Kinney, Michael D > Sent: Wednesday, August 28, 2019 1:25 PM > To: Wu, Hao A; Leif Lindholm; Laszlo Ersek; Kinney, Michael D > Cc: devel@edk2.groups.io; Andrew Fish; Baptiste Gerondeau; Wang, Jian J; > Feng, Bob C; Gao, Liming > Subject: RE: [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) > > Hao Wu, > > Please provide a patch to BaseTools to restore the previous behavior. > > We need that to review the complexity of the change to determine > what to do. Without that information today, the release date > of this stable tag is at risk. Hello Mike, I confirmed with Bob that he is preparing a new BaseTools patch to restore the previous behavior. Best Regards, Hao Wu > > Mike > > > -----Original Message----- > > From: Wu, Hao A > > Sent: Tuesday, August 27, 2019 6:40 PM > > To: Leif Lindholm <leif.lindholm@linaro.org>; Laszlo > > Ersek <lersek@redhat.com>; Kinney, Michael D > > <michael.d.kinney@intel.com> > > Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; > > Baptiste Gerondeau <baptiste.gerondeau@linaro.org>; Wang, > > Jian J <jian.j.wang@intel.com>; Feng, Bob C > > <bob.c.feng@intel.com>; Gao, Liming > > <liming.gao@intel.com> > > Subject: RE: [PATCH 1/1] MdeModulePkg: fix !x86 builds > > (more) > > > > > -----Original Message----- > > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > > > Sent: Wednesday, August 28, 2019 4:59 AM > > > To: Laszlo Ersek > > > Cc: devel@edk2.groups.io; Andrew Fish; Kinney, Michael > > D; Baptiste > > > Gerondeau; Wang, Jian J; Wu, Hao A; Feng, Bob C; Gao, > > Liming > > > Subject: Re: [PATCH 1/1] MdeModulePkg: fix !x86 builds > > (more) > > > > > > +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.lindholm@linaro.org> > > > > > Reported-by: Baptiste Gerondeau > > <baptiste.gerondeau@linaro.org> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > > > Cc: Hao A Wu <hao.a.wu@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.ht > > > ml > > > > > > > > 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/LockBoxNul > > lLib.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 > > /PeiMemory > > > AllocationLib.inf > > > > > > > > > > ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedSe > > ctionLib/PeiE > > > ExtractGuidedSectionLib|x > > > tractGuidedSectionLib.inf > > > > > + > > > > > +[LibraryClasses.IA32.PEIM,LibraryClasses.X64.PEIM] > > > > > > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxP > > eiLib.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/SmmLockBoxD > > xeLib.in > > > f > > > > > > > > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLi > > b/UefiMemo > > > ryAllocationLib.inf > > > > > > > > > > ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSe > > ctionLib/DxeE > > > xtractGuidedSectionLib.inf > > > > > > > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsu > > leLib.inf > > > > > > > > > > > > +[LibraryClasses.IA32.DXE_DRIVER,LibraryClasses.X64.DXE_D > > RIVER] > > > > > + > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD > > xeLib.in > > > f > > > > > + > > > > > [LibraryClasses.common.DXE_RUNTIME_DRIVER] > > > > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > > > > > > > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLi > > b/UefiMemo > > > ryAllocationLib.inf > > > > > > > > > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibCo > > nOut.inf > > > > > - > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD > > xeLib.in > > > f > > > > > > > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRunti > > meCapsule > > > Lib.inf > > > > > > > > > > > > > > > +[LibraryClasses.IA32.DXE_RUNTIME_DRIVER,LibraryClasses.X > > 64.DXE_RUNTI > > > ME_DRIVER] > > > > > + > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD > > xeLib.in > > > f > > > > > + > > > > > [LibraryClasses.common.SMM_CORE] > > > > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > > > > > > > > > MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryA > > llocatio > > > nLib/PiSmmCoreMemoryAllocationLib.inf > > > > > @@ -143,13 +149,17 @@ > > [LibraryClasses.common.DXE_SMM_DRIVER] > > > > > > > > > > MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib > > /SmmMe > > > moryAllocationLib.inf > > > > > > > > > > MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmSe > > rvicesTabl > > > eLib.inf > > > > > > > > > > SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/Sm > > mServicesT > > > ableLib.inf > > > > > + SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf > > > > > + > > > > > > > > > > +[LibraryClasses.IA32.DXE_SMM_DRIVER,LibraryClasses.X64.D > > XE_SMM_DRI > > > VER] > > > > > > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxS > > mmLib.i > > > nf > > > > > - 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/UefiMemoryAllocationLi > > b/UefiMemo > > > ryAllocationLib.inf > > > > > > > > > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibCo > > nOut.inf > > > > > + > > > > > > > +[LibraryClasses.IA32.UEFI_DRIVER,LibraryClasses.X64.UEFI > > _DRIVER] > > > > > > > > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD > > xeLib.in > > > f > > > > > > > > > > [LibraryClasses.common.UEFI_APPLICATION] > > > > > > > > > > > > > With (1) fixed (feel free to correct that just before > > pushing): > > > > > > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > > > > > Thanks! > > > > > > > Do wait for maintainer review, of course. > > > > > > Of course. > > > > > > Per my understanding to the analysis from Leif, Laszlo > > and Mike, the patch will depend on the precedence of the > > below rules: > > > > * [LibraryClasses.common.$(MODULE_TYPE)] > > * [LibraryClasses.$(Arch)] > > > > So for now, we should wait for the final call on the > > above open, right? > > > > Best Regards, > > Hao Wu > > > > > > > > > > Best Regards, > > > > > > Leif ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) 2019-08-28 1:40 ` Wu, Hao A 2019-08-28 5:24 ` Michael D Kinney @ 2019-08-28 9:24 ` Leif Lindholm 1 sibling, 0 replies; 10+ messages in thread From: Leif Lindholm @ 2019-08-28 9:24 UTC (permalink / raw) To: devel, hao.a.wu Cc: Laszlo Ersek, Kinney, Michael D, Andrew Fish, Baptiste Gerondeau, Wang, Jian J, Feng, Bob C, Gao, Liming On Wed, Aug 28, 2019 at 01:40:22AM +0000, Wu, Hao A wrote: > Per my understanding to the analysis from Leif, Laszlo and Mike, the patch > will depend on the precedence of the below rules: > > * [LibraryClasses.common.$(MODULE_TYPE)] > * [LibraryClasses.$(Arch)] > > So for now, we should wait for the final call on the above open, right? Well, it is a bugfix regardless. But if the BaseTools behaviour is restored to what it was before 9 August, the current state has no observable side effects. And since we are in a hard freeze, I think this fix should wait until after the tag. Best Regards, Leif ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) 2019-08-27 19:26 ` Laszlo Ersek 2019-08-27 20:59 ` Leif Lindholm @ 2019-08-27 21:00 ` Michael D Kinney 1 sibling, 0 replies; 10+ messages in thread From: Michael D Kinney @ 2019-08-27 21:00 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, Leif Lindholm, Kinney, Michael D, Gao, Liming, Feng, Bob C Cc: Andrew Fish, Baptiste Gerondeau, Wang, Jian J, Wu, Hao A Laszlo, Thanks for the analysis of the DSC Specification. It appears a behavior change was introduced with the following commit. https://github.com/tianocore/edk2/commit/e8449e1d8e3b40186eb16ff25242397cffb00a63 The new code follows the DSC Specification, but changes the library selection priority. The EBC library class selection for LockBoxLib has been the following for a long time: [LibraryClasses.EBC] LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf e8449e1d8e fails for EBC for the SmmLockLock mapping 197ca7febf builds for EBC We need to decide if we prefer the priority documented in the DSC Specification or the priority in BaseTools before e8449e1d8e. We will likely need to wait a couple of hours for Liming and Bob to be online and review this issue. Best regards, Mike > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] > On Behalf Of Laszlo Ersek > Sent: Tuesday, August 27, 2019 12:26 PM > To: Leif Lindholm <leif.lindholm@linaro.org>; > devel@edk2.groups.io > Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Baptiste Gerondeau > <baptiste.gerondeau@linaro.org>; Wang, Jian J > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com> > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: fix > !x86 builds (more) > > 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.lindholm@linaro.org> > > Reported-by: Baptiste Gerondeau > <baptiste.gerondeau@linaro.org> > > Cc: Jian J Wang <jian.j.wang@intel.com> > > Cc: Hao A Wu <hao.a.wu@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: > > > [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/LockBoxNu > llLib.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/PeiMemoryAllocationLi > b/PeiMemoryAllocationLib.inf > > > > > ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedS > ectionLib/PeiE > > xtractGuidedSectionLib.inf > > + > > +[LibraryClasses.IA32.PEIM,LibraryClasses.X64.PEIM] > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > PeiLib.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/SmmLockBox > DxeLib.inf > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationL > ib/UefiMemoryAllocationLib.inf > > > ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedS > ectionLib/DxeExtractGuidedSectionLib.inf > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCaps > uleLib.inf > > > > > +[LibraryClasses.IA32.DXE_DRIVER,LibraryClasses.X64.DXE_ > DRIVER] > > + > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > DxeLib.inf > > + > > [LibraryClasses.common.DXE_RUNTIME_DRIVER] > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationL > ib/UefiMemoryAllocationLib.inf > > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibC > onOut.inf > > - > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > DxeLib.inf > > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRunt > imeCapsuleLib. > > inf > > > > > +[LibraryClasses.IA32.DXE_RUNTIME_DRIVER,LibraryClasses. > X64.DXE_RUNTIM > > +E_DRIVER] > > + > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > DxeLib.inf > > + > > [LibraryClasses.common.SMM_CORE] > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > > > > MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemory > AllocationLib/ > > PiSmmCoreMemoryAllocationLib.inf @@ -143,13 +149,17 @@ > > [LibraryClasses.common.DXE_SMM_DRIVER] > > > MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLi > b/SmmMemoryAllocationLib.inf > > > MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmS > ervicesTableLib.inf > > > > > SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/S > mmServicesTabl > > eLib.inf > > + SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf > > + > > > +[LibraryClasses.IA32.DXE_SMM_DRIVER,LibraryClasses.X64. > DXE_SMM_DRIVER > > +] > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > SmmLib.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/UefiMemoryAllocationL > ib/UefiMemoryAllocationLib.inf > > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibC > onOut.inf > > + > > > +[LibraryClasses.IA32.UEFI_DRIVER,LibraryClasses.X64.UEF > I_DRIVER] > > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox > DxeLib.inf > > > > [LibraryClasses.common.UEFI_APPLICATION] > > > > With (1) fixed (feel free to correct that just before > pushing): > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > Do wait for maintainer review, of course. > > Thanks, > Laszlo > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-08-28 9:24 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-08-27 12:43 [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) Leif Lindholm 2019-08-27 19:26 ` Laszlo Ersek 2019-08-27 20:59 ` Leif Lindholm 2019-08-27 21:26 ` [edk2-devel] " Michael D Kinney 2019-08-28 1:10 ` Bob Feng 2019-08-28 1:40 ` Wu, Hao A 2019-08-28 5:24 ` Michael D Kinney 2019-08-28 5:43 ` Wu, Hao A 2019-08-28 9:24 ` [edk2-devel] " Leif Lindholm 2019-08-27 21:00 ` Michael D Kinney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox