[PATCH v3 0/1] Delete TCP, PXE, iSCSI driver in MdeModulePkg.

[PATCH v3 0/1] Delete TCP, PXE, iSCSI driver in MdeModulePkg.

[Fu Siyuan]

V3 Update:
1. Changes for OvmfPkg and Vlv2TbltDevicePkg have been committed so not 
   included in this v3 patch.
2. Remove ArmVirtPkg duplicate library added in v2 patch.

V2 Update:
1. Changes for Nt32Pkg and EmulatorPkg have been committed so not 
   included in this v2 patch.
2. Add missing library instance for NetworkPkg iSCSI driver.
3. The removal of the 3 modules from MdeModulePkg will not be included
   in edk2-stable201811 tag, so not included in this v2 patch.

This patch series is to delete the Tcp4Dxe, UefiPxeBcDxe and IScsi4Dxe
drivers in MdeModulePkg. These drivers will not be maintained and can't
co-work with the dual-stack drivers in NetworkPkg.

People should use below NetworkPkg drivers instead:
  NetworkPkg/IScsiDxe/IScsiDxe.inf
  NetworkPkg/TcpDxe/TcpDxe.inf
  NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
These drivers are actively maintained with more bug fixes and new feature
support.

The MdeModulePkg iSCSI driver implements its own Md5.c instead of using
standard crypto API provided by OpenSSL, which is not allowed by edk2
security development guide line. NetworkPkg iSCSI driver doesn't have
this issue and so need to be built with OpenSSL library instance now.
Please add below library instance to your DSC file.
[LibraryClasses.common.UEFI_DRIVER]
  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
  OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
  IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf

Patch 1~5 update edk2 platform DSC/FDF files to use NetworkPkg drivers.
Patch 6 deletes the TCP,PXE,iSCSI driver in MdeModulePkg.
Patch 7 removes some clarification in NetworkPkg drivers since the
related driver has been deleted in Patch 6.

Fu Siyuan (1):
  ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Julien Grall <julien.grall@linaro.org>

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

-- 
2.19.1.windows.1

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

[Fu Siyuan]

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
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
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
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
@@ -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
-- 
2.19.1.windows.1

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

[Ard Biesheuvel]

On 6 November 2018 at 02:24, Fu Siyuan <siyuan.fu@intel.com> 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.
>

Please don't put the patch revision history in the commit log. Put it
below the ---

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

... here ...

The patch looks fine to me

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

but please don't merge it until after the next stable tag has been created

>  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
> 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
> 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
> 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
> @@ -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
> --
> 2.19.1.windows.1
>

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

On 11/06/18 13:32, Ard Biesheuvel wrote:
> On 6 November 2018 at 02:24, Fu Siyuan <siyuan.fu@intel.com> 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.
>>
> 
> Please don't put the patch revision history in the commit log. Put it
> below the ---
> 
>> 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>
>> ---
> 
> ... here ...
> 
> The patch looks fine to me
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> but please don't merge it until after the next stable tag has been created

This is not a bad idea (see also your discussion with Leif); however it
does create a bit of inconsistency with how the other platform DSC/FDF
files have been handled. (The changes have been pushed for those.)

Again, I don't disagree, and I don't mind if ArmVirt is handled
differently. It's just that we should have handled this more uniformly,
I believe.


In retrospect, I would have also appreciated if the patches had
referenced <https://bugzilla.tianocore.org/show_bug.cgi?id=1278>, even
though they only implement "prep" work for now, on the platform DSC/FDF
level, and not the actual driver removal.

For example, the important explanation about MdeModulePkg's iSCSI driver
implementing its own MD5 algo cannot be connected to the OVMF commit now
(d2f1f6423bd1). I have copied the most relevant passage from the cover
letter of this series into TianoCore BZ#1278, but the commit in question
doesn't reference any BZ, so the link cannot be established.

Thanks
Laszlo

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

[Ard Biesheuvel]

On 6 November 2018 at 16:24, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/06/18 13:32, Ard Biesheuvel wrote:
>> On 6 November 2018 at 02:24, Fu Siyuan <siyuan.fu@intel.com> 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.
>>>
>>
>> Please don't put the patch revision history in the commit log. Put it
>> below the ---
>>
>>> 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>
>>> ---
>>
>> ... here ...
>>
>> The patch looks fine to me
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> but please don't merge it until after the next stable tag has been created
>
> This is not a bad idea (see also your discussion with Leif); however it
> does create a bit of inconsistency with how the other platform DSC/FDF
> files have been handled. (The changes have been pushed for those.)
>
> Again, I don't disagree, and I don't mind if ArmVirt is handled
> differently. It's just that we should have handled this more uniformly,
> I believe.
>

Yes - as I replied to Leif, I am not going to obsess about this. But
the point of stable tags is not to rush things in at the last minute.

>
> In retrospect, I would have also appreciated if the patches had
> referenced <https://bugzilla.tianocore.org/show_bug.cgi?id=1278>, even
> though they only implement "prep" work for now, on the platform DSC/FDF
> level, and not the actual driver removal.
>
> For example, the important explanation about MdeModulePkg's iSCSI driver
> implementing its own MD5 algo cannot be connected to the OVMF commit now
> (d2f1f6423bd1). I have copied the most relevant passage from the cover
> letter of this series into TianoCore BZ#1278, but the commit in question
> doesn't reference any BZ, so the link cannot be established.
>
> Thanks
> Laszlo

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

[Fu, Siyuan]

Hi, Arb

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, November 7, 2018 12:39 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Fu, Siyuan <siyuan.fu@intel.com>; edk2-devel@lists.01.org; Julien
> Grall <julien.grall@linaro.org>
> Subject: Re: [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers
> from platform DSC/FDF.
> 
> On 6 November 2018 at 16:24, Laszlo Ersek <lersek@redhat.com> wrote:
> > On 11/06/18 13:32, Ard Biesheuvel wrote:
> >> On 6 November 2018 at 02:24, Fu Siyuan <siyuan.fu@intel.com> 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.
> >>>
> >>
> >> Please don't put the patch revision history in the commit log. Put it
> >> below the ---
> >>
> >>> 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>
> >>> ---
> >>
> >> ... here ...
> >>
> >> The patch looks fine to me
> >>
> >> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>
> >> but please don't merge it until after the next stable tag has been
> created
> >
> > This is not a bad idea (see also your discussion with Leif); however it
> > does create a bit of inconsistency with how the other platform DSC/FDF
> > files have been handled. (The changes have been pushed for those.)
> >
> > Again, I don't disagree, and I don't mind if ArmVirt is handled
> > differently. It's just that we should have handled this more uniformly,
> > I believe.
> >
> 
> Yes - as I replied to Leif, I am not going to obsess about this. But
> the point of stable tags is not to rush things in at the last minute.

OK I will not commit this change to ArmVirt until the stable tag is made.
Sorry for the last minute notification of this change, and I can fully
understand your concern, that's why we accepted Leif that still keep
these MdeModulePkg drivers in this stable tag.

Thanks for review.
Siyuan

> 
> >
> > In retrospect, I would have also appreciated if the patches had
> > referenced <https://bugzilla.tianocore.org/show_bug.cgi?id=1278>, even
> > though they only implement "prep" work for now, on the platform DSC/FDF
> > level, and not the actual driver removal.
> >
> > For example, the important explanation about MdeModulePkg's iSCSI driver
> > implementing its own MD5 algo cannot be connected to the OVMF commit now
> > (d2f1f6423bd1). I have copied the most relevant passage from the cover
> > letter of this series into TianoCore BZ#1278, but the commit in question
> > doesn't reference any BZ, so the link cannot be established.
> >
> > Thanks
> > Laszlo

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

[Fu, Siyuan]

Hi, Arb

Do you think if it's ok to commit this patch to ArmVirtPkg now?

BestRegards
Fu Siyuan

> -----Original Message-----
> From: Fu, Siyuan
> Sent: Wednesday, November 7, 2018 8:59 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel@lists.01.org; Julien Grall <julien.grall@linaro.org>; Laszlo
> Ersek <lersek@redhat.com>
> Subject: RE: [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers from
> platform DSC/FDF.
> 
> Hi, Arb
> 
> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > Sent: Wednesday, November 7, 2018 12:39 AM
> > To: Laszlo Ersek <lersek@redhat.com>
> > Cc: Fu, Siyuan <siyuan.fu@intel.com>; edk2-devel@lists.01.org; Julien
> > Grall <julien.grall@linaro.org>
> > Subject: Re: [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers
> > from platform DSC/FDF.
> >
> > On 6 November 2018 at 16:24, Laszlo Ersek <lersek@redhat.com> wrote:
> > > On 11/06/18 13:32, Ard Biesheuvel wrote:
> > >> On 6 November 2018 at 02:24, Fu Siyuan <siyuan.fu@intel.com> 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.
> > >>>
> > >>
> > >> Please don't put the patch revision history in the commit log. Put it
> > >> below the ---
> > >>
> > >>> 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>
> > >>> ---
> > >>
> > >> ... here ...
> > >>
> > >> The patch looks fine to me
> > >>
> > >> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > >>
> > >> but please don't merge it until after the next stable tag has been
> > created
> > >
> > > This is not a bad idea (see also your discussion with Leif); however it
> > > does create a bit of inconsistency with how the other platform DSC/FDF
> > > files have been handled. (The changes have been pushed for those.)
> > >
> > > Again, I don't disagree, and I don't mind if ArmVirt is handled
> > > differently. It's just that we should have handled this more uniformly,
> > > I believe.
> > >
> >
> > Yes - as I replied to Leif, I am not going to obsess about this. But
> > the point of stable tags is not to rush things in at the last minute.
> 
> OK I will not commit this change to ArmVirt until the stable tag is made.
> Sorry for the last minute notification of this change, and I can fully
> understand your concern, that's why we accepted Leif that still keep
> these MdeModulePkg drivers in this stable tag.
> 
> Thanks for review.
> Siyuan
> 
> >
> > >
> > > In retrospect, I would have also appreciated if the patches had
> > > referenced <https://bugzilla.tianocore.org/show_bug.cgi?id=1278>, even
> > > though they only implement "prep" work for now, on the platform DSC/FDF
> > > level, and not the actual driver removal.
> > >
> > > For example, the important explanation about MdeModulePkg's iSCSI driver
> > > implementing its own MD5 algo cannot be connected to the OVMF commit now
> > > (d2f1f6423bd1). I have copied the most relevant passage from the cover
> > > letter of this series into TianoCore BZ#1278, but the commit in question
> > > doesn't reference any BZ, so the link cannot be established.
> > >
> > > Thanks
> > > Laszlo

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

[Ard Biesheuvel]

On Fri, 14 Dec 2018 at 07:11, Fu, Siyuan <siyuan.fu@intel.com> wrote:
>
> Hi, Arb
>
> Do you think if it's ok to commit this patch to ArmVirtPkg now?
>

Sure, please go ahead

> > -----Original Message-----
> > From: Fu, Siyuan
> > Sent: Wednesday, November 7, 2018 8:59 AM
> > To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: edk2-devel@lists.01.org; Julien Grall <julien.grall@linaro.org>; Laszlo
> > Ersek <lersek@redhat.com>
> > Subject: RE: [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers from
> > platform DSC/FDF.
> >
> > Hi, Arb
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > > Sent: Wednesday, November 7, 2018 12:39 AM
> > > To: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Fu, Siyuan <siyuan.fu@intel.com>; edk2-devel@lists.01.org; Julien
> > > Grall <julien.grall@linaro.org>
> > > Subject: Re: [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers
> > > from platform DSC/FDF.
> > >
> > > On 6 November 2018 at 16:24, Laszlo Ersek <lersek@redhat.com> wrote:
> > > > On 11/06/18 13:32, Ard Biesheuvel wrote:
> > > >> On 6 November 2018 at 02:24, Fu Siyuan <siyuan.fu@intel.com> 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.
> > > >>>
> > > >>
> > > >> Please don't put the patch revision history in the commit log. Put it
> > > >> below the ---
> > > >>
> > > >>> 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>
> > > >>> ---
> > > >>
> > > >> ... here ...
> > > >>
> > > >> The patch looks fine to me
> > > >>
> > > >> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > >>
> > > >> but please don't merge it until after the next stable tag has been
> > > created
> > > >
> > > > This is not a bad idea (see also your discussion with Leif); however it
> > > > does create a bit of inconsistency with how the other platform DSC/FDF
> > > > files have been handled. (The changes have been pushed for those.)
> > > >
> > > > Again, I don't disagree, and I don't mind if ArmVirt is handled
> > > > differently. It's just that we should have handled this more uniformly,
> > > > I believe.
> > > >
> > >
> > > Yes - as I replied to Leif, I am not going to obsess about this. But
> > > the point of stable tags is not to rush things in at the last minute.
> >
> > OK I will not commit this change to ArmVirt until the stable tag is made.
> > Sorry for the last minute notification of this change, and I can fully
> > understand your concern, that's why we accepted Leif that still keep
> > these MdeModulePkg drivers in this stable tag.
> >
> > Thanks for review.
> > Siyuan
> >
> > >
> > > >
> > > > In retrospect, I would have also appreciated if the patches had
> > > > referenced <https://bugzilla.tianocore.org/show_bug.cgi?id=1278>, even
> > > > though they only implement "prep" work for now, on the platform DSC/FDF
> > > > level, and not the actual driver removal.
> > > >
> > > > For example, the important explanation about MdeModulePkg's iSCSI driver
> > > > implementing its own MD5 algo cannot be connected to the OVMF commit now
> > > > (d2f1f6423bd1). I have copied the most relevant passage from the cover
> > > > letter of this series into TianoCore BZ#1278, but the commit in question
> > > > doesn't reference any BZ, so the link cannot be established.
> > > >
> > > > Thanks
> > > > Laszlo