[PATCH 1/1] MdeModulePkg: fix !x86 builds (more)

[PATCH 1/1] MdeModulePkg: fix !x86 builds (more)

[Leif Lindholm]

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

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

[Laszlo Ersek]

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

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

[Leif Lindholm]

+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

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

[Michael D Kinney]

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

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

[Michael D Kinney]

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

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

[Bob Feng]

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

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

[Wu, Hao A]

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

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

[Michael D Kinney]

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

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

[Wu, Hao A]

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

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

[Leif Lindholm]

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