Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: fix !x86 builds (more)

["Michael D Kinney" <michael.d.kinney@intel.com>]

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
> 
>