[PATCH] ArmVirtPkg: use protocol-based DevicePathLib instance for most DXE modules

[PATCH] ArmVirtPkg: use protocol-based DevicePathLib instance for most DXE modules

[Laszlo Ersek]

Port OvmfPkg commit 5c3481b0b611e to ArmVirtPkg. Some explanation should
be in order (because 5c3481b0b611e doesn't offer any):

- The UefiDevicePathLibDevicePathProtocol instance uses the Device Path
  Utilities Protocol, produced by DevicePathDxe, for formatting and
  parsing the textual device path representation. This allows for a
  lighter weight lib instance that gets linked into several DXE modules.
  In comparison, the more standalone UefiDevicePathLib instance includes
  the formatting and parsing routines in every client module.

- The DXE core needs DevicePathLib before it dispatches DevicePathDxe, so
  it needs to stick with the standalone instance.

- DevicePathDxe itself also needs the standalone instance, for
  implementing the protocol.

- The DXE-phase PCD driver, "MdeModulePkg/Universal/PCD/Dxe/Pcd.inf",
  depends on DevicePathLib via UefiLib and DxeServicesLib at the least; so
  with this update, it inherits a dependency on the protocol. In reverse,
  DevicePathDxe depends on the PCD Protocol, via PcdLib. The cycle is
  broken by using BasePcdLibNull in DevicePathDxe. That restricts it to
  FixedAtBuild, Patch, and FeatureFlag PCDs, but that's fine.

Example space savings (using ArmVirtQemu and the GCC5 toolchain):
- NOOPT:   187KB in FVMAIN, 12KB in FVMAIN_COMPACT
- DEBUG:   147KB in FVMAIN, 20KB in FVMAIN_COMPACT
- RELEASE: 123KB in FVMAIN, 17KB in FVMAIN_COMPACT

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Julien Grall <julien.grall@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=940
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    Repo:   https://github.com/lersek/edk2.git
    Branch: armvirt_devpathlib

 ArmVirtPkg/ArmVirt.dsc.inc       | 2 +-
 ArmVirtPkg/ArmVirtQemu.dsc       | 7 ++++++-
 ArmVirtPkg/ArmVirtQemuKernel.dsc | 7 ++++++-
 ArmVirtPkg/ArmVirtXen.dsc        | 7 ++++++-
 4 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index cde514958da2..35bccb3dc1f4 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -48,7 +48,7 @@ [LibraryClasses.common]
   UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
   HobLib|ArmVirtPkg/Library/ArmVirtDxeHobLib/ArmVirtDxeHobLib.inf
   UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
-  DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
+  DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.inf
   UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
   DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
   DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 824070edc2a9..d74feb709cd1 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -243,6 +243,7 @@ [Components.common]
   MdeModulePkg/Core/Dxe/DxeMain.inf {
     <LibraryClasses>
       NULL|MdeModulePkg/Library/DxeCrc32GuidedSectionExtractLib/DxeCrc32GuidedSectionExtractLib.inf
+      DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
   }
   MdeModulePkg/Universal/PCD/Dxe/Pcd.inf {
     <LibraryClasses>
@@ -319,7 +320,11 @@ [Components.common]
   #
   # Bds
   #
-  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
+  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf {
+    <LibraryClasses>
+      DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
+      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  }
   MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
   MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
   MdeModulePkg/Universal/DriverHealthManagerDxe/DriverHealthManagerDxe.inf
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index 2368ba283bff..1e823aeab7c0 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -232,6 +232,7 @@ [Components.common]
   MdeModulePkg/Core/Dxe/DxeMain.inf {
     <LibraryClasses>
       NULL|MdeModulePkg/Library/DxeCrc32GuidedSectionExtractLib/DxeCrc32GuidedSectionExtractLib.inf
+      DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
   }
   MdeModulePkg/Universal/PCD/Dxe/Pcd.inf {
     <LibraryClasses>
@@ -308,7 +309,11 @@ [Components.common]
   #
   # Bds
   #
-  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
+  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf {
+    <LibraryClasses>
+      DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
+      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  }
   MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
   MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
   MdeModulePkg/Universal/DriverHealthManagerDxe/DriverHealthManagerDxe.inf
diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
index 175b56d10c8f..e0fb4b47cf63 100644
--- a/ArmVirtPkg/ArmVirtXen.dsc
+++ b/ArmVirtPkg/ArmVirtXen.dsc
@@ -153,6 +153,7 @@ [Components.common]
   MdeModulePkg/Core/Dxe/DxeMain.inf {
     <LibraryClasses>
       NULL|MdeModulePkg/Library/DxeCrc32GuidedSectionExtractLib/DxeCrc32GuidedSectionExtractLib.inf
+      DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
   }
   MdeModulePkg/Universal/PCD/Dxe/Pcd.inf {
     <LibraryClasses>
@@ -205,7 +206,11 @@ [Components.common]
   #
   # Bds
   #
-  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
+  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf {
+    <LibraryClasses>
+      DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
+      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  }
   MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
   MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
   MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH] ArmVirtPkg: use protocol-based DevicePathLib instance for most DXE modules

[Ard Biesheuvel]

On 24 April 2018 at 02:50, Laszlo Ersek <lersek@redhat.com> wrote:
> Port OvmfPkg commit 5c3481b0b611e to ArmVirtPkg. Some explanation should
> be in order (because 5c3481b0b611e doesn't offer any):
>
> - The UefiDevicePathLibDevicePathProtocol instance uses the Device Path
>   Utilities Protocol, produced by DevicePathDxe, for formatting and
>   parsing the textual device path representation. This allows for a
>   lighter weight lib instance that gets linked into several DXE modules.
>   In comparison, the more standalone UefiDevicePathLib instance includes
>   the formatting and parsing routines in every client module.
>
> - The DXE core needs DevicePathLib before it dispatches DevicePathDxe, so
>   it needs to stick with the standalone instance.
>
> - DevicePathDxe itself also needs the standalone instance, for
>   implementing the protocol.
>
> - The DXE-phase PCD driver, "MdeModulePkg/Universal/PCD/Dxe/Pcd.inf",
>   depends on DevicePathLib via UefiLib and DxeServicesLib at the least; so
>   with this update, it inherits a dependency on the protocol. In reverse,
>   DevicePathDxe depends on the PCD Protocol, via PcdLib. The cycle is
>   broken by using BasePcdLibNull in DevicePathDxe. That restricts it to
>   FixedAtBuild, Patch, and FeatureFlag PCDs, but that's fine.
>
> Example space savings (using ArmVirtQemu and the GCC5 toolchain):
> - NOOPT:   187KB in FVMAIN, 12KB in FVMAIN_COMPACT
> - DEBUG:   147KB in FVMAIN, 20KB in FVMAIN_COMPACT
> - RELEASE: 123KB in FVMAIN, 17KB in FVMAIN_COMPACT
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Julien Grall <julien.grall@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=940
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>
> Notes:
>     Repo:   https://github.com/lersek/edk2.git
>     Branch: armvirt_devpathlib
>
>  ArmVirtPkg/ArmVirt.dsc.inc       | 2 +-
>  ArmVirtPkg/ArmVirtQemu.dsc       | 7 ++++++-
>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 7 ++++++-
>  ArmVirtPkg/ArmVirtXen.dsc        | 7 ++++++-
>  4 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index cde514958da2..35bccb3dc1f4 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -48,7 +48,7 @@ [LibraryClasses.common]
>    UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
>    HobLib|ArmVirtPkg/Library/ArmVirtDxeHobLib/ArmVirtDxeHobLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
> -  DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
> +  DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.inf
>    UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
>    DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
>    DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 824070edc2a9..d74feb709cd1 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -243,6 +243,7 @@ [Components.common]
>    MdeModulePkg/Core/Dxe/DxeMain.inf {
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/DxeCrc32GuidedSectionExtractLib/DxeCrc32GuidedSectionExtractLib.inf
> +      DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
>    }
>    MdeModulePkg/Universal/PCD/Dxe/Pcd.inf {
>      <LibraryClasses>
> @@ -319,7 +320,11 @@ [Components.common]
>    #
>    # Bds
>    #
> -  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
> +  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf {
> +    <LibraryClasses>
> +      DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
> +      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> +  }
>    MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>    MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>    MdeModulePkg/Universal/DriverHealthManagerDxe/DriverHealthManagerDxe.inf
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 2368ba283bff..1e823aeab7c0 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -232,6 +232,7 @@ [Components.common]
>    MdeModulePkg/Core/Dxe/DxeMain.inf {
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/DxeCrc32GuidedSectionExtractLib/DxeCrc32GuidedSectionExtractLib.inf
> +      DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
>    }
>    MdeModulePkg/Universal/PCD/Dxe/Pcd.inf {
>      <LibraryClasses>
> @@ -308,7 +309,11 @@ [Components.common]
>    #
>    # Bds
>    #
> -  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
> +  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf {
> +    <LibraryClasses>
> +      DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
> +      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> +  }
>    MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>    MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>    MdeModulePkg/Universal/DriverHealthManagerDxe/DriverHealthManagerDxe.inf
> diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
> index 175b56d10c8f..e0fb4b47cf63 100644
> --- a/ArmVirtPkg/ArmVirtXen.dsc
> +++ b/ArmVirtPkg/ArmVirtXen.dsc
> @@ -153,6 +153,7 @@ [Components.common]
>    MdeModulePkg/Core/Dxe/DxeMain.inf {
>      <LibraryClasses>
>        NULL|MdeModulePkg/Library/DxeCrc32GuidedSectionExtractLib/DxeCrc32GuidedSectionExtractLib.inf
> +      DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
>    }
>    MdeModulePkg/Universal/PCD/Dxe/Pcd.inf {
>      <LibraryClasses>
> @@ -205,7 +206,11 @@ [Components.common]
>    #
>    # Bds
>    #
> -  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
> +  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf {
> +    <LibraryClasses>
> +      DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
> +      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> +  }
>    MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>    MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>    MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> --
> 2.14.1.3.gb7cf6e02401b
>

Re: [PATCH] ArmVirtPkg: use protocol-based DevicePathLib instance for most DXE modules

[Laszlo Ersek]

On 04/24/18 08:16, Ard Biesheuvel wrote:
> On 24 April 2018 at 02:50, Laszlo Ersek <lersek@redhat.com> wrote:
>> Port OvmfPkg commit 5c3481b0b611e to ArmVirtPkg. Some explanation should
>> be in order (because 5c3481b0b611e doesn't offer any):
>>
>> - The UefiDevicePathLibDevicePathProtocol instance uses the Device Path
>>   Utilities Protocol, produced by DevicePathDxe, for formatting and
>>   parsing the textual device path representation. This allows for a
>>   lighter weight lib instance that gets linked into several DXE modules.
>>   In comparison, the more standalone UefiDevicePathLib instance includes
>>   the formatting and parsing routines in every client module.
>>
>> - The DXE core needs DevicePathLib before it dispatches DevicePathDxe, so
>>   it needs to stick with the standalone instance.
>>
>> - DevicePathDxe itself also needs the standalone instance, for
>>   implementing the protocol.
>>
>> - The DXE-phase PCD driver, "MdeModulePkg/Universal/PCD/Dxe/Pcd.inf",
>>   depends on DevicePathLib via UefiLib and DxeServicesLib at the least; so
>>   with this update, it inherits a dependency on the protocol. In reverse,
>>   DevicePathDxe depends on the PCD Protocol, via PcdLib. The cycle is
>>   broken by using BasePcdLibNull in DevicePathDxe. That restricts it to
>>   FixedAtBuild, Patch, and FeatureFlag PCDs, but that's fine.
>>
>> Example space savings (using ArmVirtQemu and the GCC5 toolchain):
>> - NOOPT:   187KB in FVMAIN, 12KB in FVMAIN_COMPACT
>> - DEBUG:   147KB in FVMAIN, 20KB in FVMAIN_COMPACT
>> - RELEASE: 123KB in FVMAIN, 17KB in FVMAIN_COMPACT
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Julien Grall <julien.grall@linaro.org>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=940
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thanks, Ard!

Julien, can you please fetch the patch:

>> Notes:
>>     Repo:   https://github.com/lersek/edk2.git
>>     Branch: armvirt_devpathlib

and regression-test it on Xen? I'd prefer to push the patch with your T-b.

Thanks!
Laszlo

Re: [PATCH] ArmVirtPkg: use protocol-based DevicePathLib instance for most DXE modules

[Laszlo Ersek]

On 04/24/18 08:16, Ard Biesheuvel wrote:
> On 24 April 2018 at 02:50, Laszlo Ersek <lersek@redhat.com> wrote:
>> Port OvmfPkg commit 5c3481b0b611e to ArmVirtPkg. Some explanation should
>> be in order (because 5c3481b0b611e doesn't offer any):
>>
>> - The UefiDevicePathLibDevicePathProtocol instance uses the Device Path
>>   Utilities Protocol, produced by DevicePathDxe, for formatting and
>>   parsing the textual device path representation. This allows for a
>>   lighter weight lib instance that gets linked into several DXE modules.
>>   In comparison, the more standalone UefiDevicePathLib instance includes
>>   the formatting and parsing routines in every client module.
>>
>> - The DXE core needs DevicePathLib before it dispatches DevicePathDxe, so
>>   it needs to stick with the standalone instance.
>>
>> - DevicePathDxe itself also needs the standalone instance, for
>>   implementing the protocol.
>>
>> - The DXE-phase PCD driver, "MdeModulePkg/Universal/PCD/Dxe/Pcd.inf",
>>   depends on DevicePathLib via UefiLib and DxeServicesLib at the least; so
>>   with this update, it inherits a dependency on the protocol. In reverse,
>>   DevicePathDxe depends on the PCD Protocol, via PcdLib. The cycle is
>>   broken by using BasePcdLibNull in DevicePathDxe. That restricts it to
>>   FixedAtBuild, Patch, and FeatureFlag PCDs, but that's fine.
>>
>> Example space savings (using ArmVirtQemu and the GCC5 toolchain):
>> - NOOPT:   187KB in FVMAIN, 12KB in FVMAIN_COMPACT
>> - DEBUG:   147KB in FVMAIN, 20KB in FVMAIN_COMPACT
>> - RELEASE: 123KB in FVMAIN, 17KB in FVMAIN_COMPACT
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Julien Grall <julien.grall@linaro.org>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=940
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Commit f9dff9028950.

Thanks!
Laszlo