Re: [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.

[Laszlo Ersek <lersek@redhat.com>]

On 11/06/18 02:24, Fu Siyuan wrote:
> V3:
> Remove duplicate library added in v2 patch, since ArmVirtPkg.dsc.inc
> already have them. Just remove the if...end there is enough.
> 
> V2:
> Add missing library instance for NetworkPkg iSCSI driver.
> 
> This patch replaces the MdeModulePkg TCP, PXE and iSCSI driver with those
> ones in NetworkPkg. These 3 drivers in MdeModulePkg are not being actively
> maintained and will be removed from edk2 master soon.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Julien Grall <julien.grall@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Fu Siyuan <siyuan.fu@intel.com>
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc           |  2 --
>  ArmVirtPkg/ArmVirtQemu.dsc           | 10 +++-------
>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 10 +++-------
>  ArmVirtPkg/ArmVirtQemuKernel.dsc     | 13 ++++++-------
>  4 files changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 70a0ac4d786c..ebabf3f47ec9 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -149,11 +149,9 @@ [LibraryClasses.common]
>    #
>    # CryptoPkg libraries needed by multiple firmware features
>    #
> -!if ($(SECURE_BOOT_ENABLE) == TRUE) || ($(NETWORK_IP6_ENABLE) == TRUE)
>    IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
>    OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> -!endif
>  
>    #
>    # Secure Boot dependencies

This part looks good to me.

> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 885c6b14b844..b3f1b23e3890 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -346,18 +346,14 @@ [Components.common]
>    MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf
>    MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
>    MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
> +  NetworkPkg/TcpDxe/TcpDxe.inf
> +  NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> +  NetworkPkg/IScsiDxe/IScsiDxe.inf
>  !if $(NETWORK_IP6_ENABLE) == TRUE
>    NetworkPkg/Ip6Dxe/Ip6Dxe.inf
> -  NetworkPkg/TcpDxe/TcpDxe.inf
>    NetworkPkg/Udp6Dxe/Udp6Dxe.inf
>    NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf
>    NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf
> -  NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> -  NetworkPkg/IScsiDxe/IScsiDxe.inf
> -!else
> -  MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf
> -  MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf
> -  MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf
>  !endif
>  !if $(HTTP_BOOT_ENABLE) == TRUE
>    NetworkPkg/DnsDxe/DnsDxe.inf

This looks fine as well.

> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> index a6390bd4b841..3316f982695f 100644
> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> @@ -126,18 +126,14 @@ [FV.FvMain]
>    INF MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf
>    INF MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
>    INF MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
> +  INF NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> +  INF NetworkPkg/IScsiDxe/IScsiDxe.inf
> +  INF NetworkPkg/TcpDxe/TcpDxe.inf
>  !if $(NETWORK_IP6_ENABLE) == TRUE
>    INF NetworkPkg/Ip6Dxe/Ip6Dxe.inf
> -  INF NetworkPkg/TcpDxe/TcpDxe.inf
>    INF NetworkPkg/Udp6Dxe/Udp6Dxe.inf
>    INF NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf
>    INF NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf
> -  INF NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> -  INF NetworkPkg/IScsiDxe/IScsiDxe.inf
> -!else
> -  INF MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf
> -  INF MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf
> -  INF MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf
>  !endif
>  !if $(HTTP_BOOT_ENABLE) == TRUE
>    INF NetworkPkg/DnsDxe/DnsDxe.inf

(1) Small wart: in the new (added) lines, we might want to stick with
the order TcpDxe, UefiPxeBcDxe, IScsiDxe. Functionally, this is totally
irrelevant, but it's easier to read (compare: (a) the DSC file above,
(b) the lines being removed).

> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 434d6861a56f..4920a66f2fdb 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -67,6 +67,9 @@ [LibraryClasses.common]
>  
>  [LibraryClasses.common.UEFI_DRIVER]
>    UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> +  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> +  OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> +  IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
>  
>  [BuildOptions.ARM.EDKII.SEC, BuildOptions.ARM.EDKII.BASE]
>    # Avoid MOVT/MOVW instruction pairs in code that may end up in the PIE

(2) I think this hunk should have been dropped, in the v3 iteration; the
lib class resolutions are now unconditional in the dsc.inc file. (And
those are also more correct, due to specifying OpensslLibCrypto, not
OpensslLib.)

> @@ -335,18 +338,14 @@ [Components.common]
>    MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf
>    MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
>    MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
> +  NetworkPkg/TcpDxe/TcpDxe.inf
> +  NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> +  NetworkPkg/IScsiDxe/IScsiDxe.inf
>  !if $(NETWORK_IP6_ENABLE) == TRUE
>    NetworkPkg/Ip6Dxe/Ip6Dxe.inf
> -  NetworkPkg/TcpDxe/TcpDxe.inf
>    NetworkPkg/Udp6Dxe/Udp6Dxe.inf
>    NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf
>    NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf
> -  NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> -  NetworkPkg/IScsiDxe/IScsiDxe.inf
> -!else
> -  MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf
> -  MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf
> -  MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf
>  !endif
>  !if $(HTTP_BOOT_ENABLE) == TRUE
>    NetworkPkg/DnsDxe/DnsDxe.inf
> 

This hunk looks OK to me.

Thanks
Laszlo