From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: bob.c.feng@intel.com) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by groups.io with SMTP; Tue, 27 Aug 2019 18:10:14 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Aug 2019 18:10:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,439,1559545200"; d="scan'208";a="185468005" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga006.jf.intel.com with ESMTP; 27 Aug 2019 18:10:13 -0700 Received: from fmsmsx123.amr.corp.intel.com (10.18.125.38) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 27 Aug 2019 18:10:13 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx123.amr.corp.intel.com (10.18.125.38) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 27 Aug 2019 18:10:13 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.112]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.250]) with mapi id 14.03.0439.000; Wed, 28 Aug 2019 09:10:11 +0800 From: "Bob Feng" 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" Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) Thread-Topic: [edk2-devel] [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) Thread-Index: AQHVXRpY5arIgQvgu02IszvaB0AAyKcO+++AgADEBxA= Date: Wed, 28 Aug 2019 01:10:10 +0000 Message-ID: <08650203BA1BD64D8AD9B6D5D74A85D161529DE9@SHSMSX104.ccr.corp.intel.com> References: <20190827124328.9034-1-leif.lindholm@linaro.org> <6ce1988a-bd79-893e-5d8d-724b98329ab9@redhat.com> <20190827205926.GG29255@bivouac.eciton.net> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: bob.c.feng@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I'll work on this Basetools regression issue today. Thanks, Bob -----Original Message----- From: Kinney, Michael D=20 Sent: Wednesday, August 28, 2019 5:27 AM 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 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 t= o invert the priority 4 and priority 5 items: 1. associated with the INF file in the [Components] secti= on 2. [LibraryClasses.$(Arch).$(MODULE_TYPE), LibraryClasses.$(Arch).$(MODU= LE_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 p= revious behavior would be restored and no DSC file changes would be require= d. This means an arch specific mapping for all module types is higher prio= rity 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=20 > Leif Lindholm > Sent: Tuesday, August 27, 2019 1:59 PM > To: Laszlo Ersek > Cc: devel@edk2.groups.io; Andrew Fish ; Kinney,=20 > Michael D ; Baptiste Gerondeau=20 > ; Wang, Jian J ;= =20 > Wu, Hao A ; Feng, Bob C ;=20 > Gao, Liming > Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: fix > !x86 builds (more) >=20 > +Bob, Liming, >=20 > 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,=20 > > > 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=3D2134 > > > > > > Signed-off-by: Leif Lindholm > > > > Reported-by: Baptiste Gerondeau > > > > Cc: Jian J Wang > > > Cc: Hao A Wu > > > --- > > > > > > I don't understand how the above would appear to > work back when I > > > submitted the previous patch but not work now, but I > haven't dug > > > into it deeper. Including the x86-specific > LockBoxLib in the .common > > > section is however clearly not correct. > > > > I agree with you that the present situation is not > correct. > > > > According to: > > > > > > https://edk2-docs.gitbooks.io/edk-ii-dsc- > specification/2_dsc_overview/ > > 26_[libraryclasses]_section_processing.html > > > > the library class resolutions take effect in the > following order > > (entries near the top have higher priority): > > > > > 1. associated with the INF file in > the [Components] > > > section 2. [LibraryClasses.$(Arch).$(MODULE_TYPE), > > > LibraryClasses.$(Arch).$(MODULE_TYPE)] > > > 3. [LibraryClasses.$(Arch).$(MODULE_TYPE)] > > > 4. [LibraryClasses.common.$(MODULE_TYPE)] > > > 5. [LibraryClasses.$(Arch)] > > > 6. [LibraryClasses.common] or [LibraryClasses] > > > > (Side comment 1: levels #2 and #3 look very similar; I > think the > > difference is that #2 is supposed to be a multi-arch > and/or > > multi-module-type section, while #3 is a single-arch > and > > single-module-type section.) > > > > Commit 4a1f6b85c184 ("MdeModulePkg: add LockBoxNullLib > for !IA32/X64 > > in .dsc", 2019-03-27) provided a LockBoxLib resolution > at level #5: >=20 > Yes. >=20 > > > [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. >=20 > Hmm. That's not great. > Anyway, I stopped being lazy and did a bisect. >=20 > The culprit is > e8449e1d8e3b ("BaseTools: Decouple AutoGen Objects"), marked as=20 > resolving https://bugzilla.tianocore.org/show_bug.cgi?id=3D1875. >=20 > This also affects SignedCapsulePkg/SignedCapsulePkg.dsc > (although once addressed, AARCH64 also needs a NULL entry added for=20 > CompilerIntrinsicsLib. >=20 > > (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. >=20 > Yes. > But it is also a fundamental change in tool behaviour introduced on 9=20 > August. I am really uncomfortable about this making it into the=20 > release this week - but I also believe this is the foundation for the=20 > multiprocess autogen. >=20 > Since you have very helpfully analyzed *what* changed ... would the=20 > better "fix" for 2019.08 be to intentionally break the new code to=20 > conform to the old behaviour - and then revert that patch after the=20 > tag? >=20 > If we do that, this patch could then wait and indeed be merged as part= =20 > of the same set. >=20 > > 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=20 > > > 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. >=20 > Yes, fair point. >=20 > > > > > > [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. >=20 > Well, this fixes the current issue. I completely agree the file could=20 > benefit from some overall restructuring. >=20 > > > [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 >=20 > Thanks! >=20 > > Do wait for maintainer review, of course. >=20 > Of course. >=20 > Best Regards, >=20 > Leif >=20 >=20