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
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
