From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D665E2117B54F for ; Tue, 6 Nov 2018 07:18:48 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3AEDF30DDBAF; Tue, 6 Nov 2018 15:18:48 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-95.rdu2.redhat.com [10.10.120.95]) by smtp.corp.redhat.com (Postfix) with ESMTP id B52341711C; Tue, 6 Nov 2018 15:18:46 +0000 (UTC) To: Fu Siyuan , edk2-devel@lists.01.org References: <20181106012403.84760-1-siyuan.fu@intel.com> <20181106012403.84760-2-siyuan.fu@intel.com> From: Laszlo Ersek Message-ID: <91d253ae-9d0c-e28b-1bda-1be98cee4340@redhat.com> Date: Tue, 6 Nov 2018 16:18:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181106012403.84760-2-siyuan.fu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Tue, 06 Nov 2018 15:18:48 +0000 (UTC) Subject: Re: [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Nov 2018 15:18:49 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Ard Biesheuvel > Cc: Julien Grall > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Fu Siyuan > --- > 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