On 07/15/16 07:18, Laszlo Ersek wrote: > On 07/15/16 02:12, Jordan Justen wrote: >> On 2016-07-13 07:36:58, Laszlo Ersek wrote:
>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >>> index 805650059e96..8af326778205 100644 >>> --- a/OvmfPkg/OvmfPkgIa32.dsc >>> +++ b/OvmfPkg/OvmfPkgIa32.dsc >>> @@ -212,6 +212,7 @@ [LibraryClasses.common.PEIM] >>> !ifdef $(SOURCE_DEBUG_ENABLE) >>> >>> DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf >>> !endif >>> + >>> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf >>> >>> [LibraryClasses.common.DXE_CORE] >>> HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf >>> @@ -519,6 +520,10 @@ [Components] >>> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf >>> } >>> !endif >>> + UefiCpuPkg/CpuMpPei/CpuMpPei.inf { >>> + <LibraryClasses> >>> + PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf >>> + } >> >> It looks like we have this (PcdLib) overridden in nearly every PEIM. I >> think we should update the default library mapping. > > Before posting this version of the series, I actually started writing > that patch, but I abandoned it. I no longer remember why. I think the > patch didn't save as much room in the DSC file as I hoped it would. > > I will post a follow-up patch so we can investigate this separately. I remember now, after reviewing the build report file: Upon seeing those individual PcdLib resolutions, for almost every PEIM we have, I figured we should flip the [LibraryClasses] default to PeiPcdLib, from BasePcdLibNull. We have the following settings now: [LibraryClasses] PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf [LibraryClasses.common] -- inherits the default -- [LibraryClasses.common.SEC] -- inherits the default -- [LibraryClasses.common.PEI_CORE] -- inherits the default -- [LibraryClasses.common.PEIM] -- inherits the default -- [LibraryClasses.common.DXE_CORE] PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf [LibraryClasses.common.DXE_RUNTIME_DRIVER] PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf [LibraryClasses.common.UEFI_DRIVER] PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf [LibraryClasses.common.DXE_DRIVER] PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf [LibraryClasses.common.UEFI_APPLICATION] PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf [LibraryClasses.common.DXE_SMM_DRIVER] PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf [LibraryClasses.common.SMM_CORE] PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf In particular, after reviewing the build report file, the following modules use BasePcdLibNull: IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei.inf MdeModulePkg/Core/Pei/PeiMain.inf MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf MdeModulePkg/Universal/PCD/Dxe/Pcd.inf MdeModulePkg/Universal/PCD/Pei/Pcd.inf OvmfPkg/Sec/SecMain.inf >From these, DevicePathDxe and PCD/Dxe don't matter (they are DXE_DRIVER >modules, they are covered by their [LibraryClasses.common.DXE_DRIVER] default, >and must override that default unconditionally). So, we are left with 4 modules (1 SEC, 1 PEI_CORE, 2 PEIMs) that would have to override their inherited PcdLib resolution to BasePcdLibNull if we changed the [LibraryClasses] default to PeiPcdLib: - OvmfPkg/Sec/SecMain.inf - MdeModulePkg/Core/Pei/PeiMain.inf - MdeModulePkg/Universal/PCD/Pei/Pcd.inf - IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei.inf This means 5 "compensatory" changes (*) in total (<LibraryClasses> for 4 modules, plus the [LibraryClasses] hunk), just so we can remove the current, explicit PeiPcdLib resolutions from 5 PEIMs: - MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf - OvmfPkg/PlatformPei/PlatformPei.inf - UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf - OvmfPkg/SmmAccess/SmmAccessPei.inf - UefiCpuPkg/CpuMpPei/CpuMpPei.inf ((*) Well, PCD/Pei already spells out BasePcdLibNull, for no good reason (at the moment), so that wouldn't be a *change*, but it would be an override that would *remain* in the DSCs.) This looked like "churn for nothing": > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 022661628dbe..c01bca9a2198 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -68,7 +68,7 @@ [SkuIds] > > ################################################################################ > [LibraryClasses] > HexDumpLib|OvmfPkg/Library/HexDumpLib/HexDumpLib.inf > - PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf > + PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf > PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf > BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf > @@ -496,43 +496,36 @@ [Components] > OvmfPkg/Sec/SecMain.inf { > <LibraryClasses> > > NULL|IntelFrameworkModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf > + PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf > } > > # > # PEI Phase modules > # > - MdeModulePkg/Core/Pei/PeiMain.inf > + MdeModulePkg/Core/Pei/PeiMain.inf { > + <LibraryClasses> > + PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf > + } > MdeModulePkg/Universal/PCD/Pei/Pcd.inf { > <LibraryClasses> > PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf > } > - IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei.inf > - MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf { > + IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei.inf { > <LibraryClasses> > - PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > - } > - > - OvmfPkg/PlatformPei/PlatformPei.inf { > - <LibraryClasses> > - PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > + PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf > } > + MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > + OvmfPkg/PlatformPei/PlatformPei.inf > UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf { > <LibraryClasses> > - PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > !if $(SMM_REQUIRE) == TRUE > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf > !endif > } > !if $(SMM_REQUIRE) == TRUE > - OvmfPkg/SmmAccess/SmmAccessPei.inf { > - <LibraryClasses> > - PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > - } > + OvmfPkg/SmmAccess/SmmAccessPei.inf > !endif > - UefiCpuPkg/CpuMpPei/CpuMpPei.inf { > - <LibraryClasses> > - PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > - } > + UefiCpuPkg/CpuMpPei/CpuMpPei.inf > > # > # DXE Phase modules Now, if I change the PcdLib resolution for [LibraryClasses.common.PEIM] only -- I haven't checked this before --, we get > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 022661628dbe..3fdc8231f514 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -199,6 +199,7 @@ [LibraryClasses.common.PEI_CORE] > PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf > > [LibraryClasses.common.PEIM] > + PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf > > PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf > PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf > @@ -506,33 +507,22 @@ [Components] > <LibraryClasses> > PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf > } > - IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei.inf > - MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf { > + IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei.inf { > <LibraryClasses> > - PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > - } > - > - OvmfPkg/PlatformPei/PlatformPei.inf { > - <LibraryClasses> > - PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > + PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf > } > + MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > + OvmfPkg/PlatformPei/PlatformPei.inf > UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf { > <LibraryClasses> > - PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > !if $(SMM_REQUIRE) == TRUE > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf > !endif > } > !if $(SMM_REQUIRE) == TRUE > - OvmfPkg/SmmAccess/SmmAccessPei.inf { > - <LibraryClasses> > - PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > - } > + OvmfPkg/SmmAccess/SmmAccessPei.inf > !endif > - UefiCpuPkg/CpuMpPei/CpuMpPei.inf { > - <LibraryClasses> > - PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > - } > + UefiCpuPkg/CpuMpPei/CpuMpPei.inf > > # > # DXE Phase modules In this case, we trade the current 5 overrides (to PeiPcdLib) for a change of default plus one new override (BasePcdLibNull for StatusCodePei). Should I submit this second change? Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel