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 9500921B02822 for ; Mon, 5 Nov 2018 14:36:40 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1A3AF81DF0; Mon, 5 Nov 2018 22:36:40 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-125-224.rdu2.redhat.com [10.10.125.224]) by smtp.corp.redhat.com (Postfix) with ESMTP id D992513ABE; Mon, 5 Nov 2018 22:36:38 +0000 (UTC) To: Fu Siyuan References: <20181105104918.82104-1-siyuan.fu@intel.com> <20181105104918.82104-3-siyuan.fu@intel.com> From: Laszlo Ersek Cc: edk2-devel@lists.01.org, "Leif Lindholm (Linaro address)" , Ard Biesheuvel Message-ID: Date: Mon, 5 Nov 2018 23:36:37 +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: <20181105104918.82104-3-siyuan.fu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Mon, 05 Nov 2018 22:36:40 +0000 (UTC) Subject: Re: [PATCH v2 2/3] 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: Mon, 05 Nov 2018 22:36:40 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/05/18 11:49, Fu Siyuan wrote: > 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/ArmVirtQemu.dsc | 13 ++++++------- > ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 10 +++------- > ArmVirtPkg/ArmVirtQemuKernel.dsc | 13 ++++++------- > 3 files changed, 15 insertions(+), 21 deletions(-) > > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc > index 885c6b14b844..0f403973bea0 100644 > --- a/ArmVirtPkg/ArmVirtQemu.dsc > +++ b/ArmVirtPkg/ArmVirtQemu.dsc > @@ -70,6 +70,9 @@ [LibraryClasses.common.PEIM] > > [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 (1) I couldn't participate in the discussion last week; I was away. On my return, I have now seen multiple related threads. I guess I can describe my general concern here. My general concern is that the edk2 network stack can no longer be built without OpenSSL. (As long as we include the iSCSI driver in the "edk2 network stack".) Is that intentional? I'm not asking for additional documentation regarding this fact, given commit 0bcbdf9c7445 ("NetworkPkg/IScsiDxe: Add the clarification compared to IScsiDxe in MdeModulePkg.", 2018-09-27). I'm just asking if we've considered this and find it acceptable. (2) Once we remove the IPv4-only drivers, the INF file comments added in: 897720daef33 NetworkPkg/TcpDxe: Add the clarification compared to Tcp4Dxe in MdeModulePkg. 0bcbdf9c7445 NetworkPkg/IScsiDxe: Add the clarification compared to IScsiDxe in MdeModulePkg. 24c55f5dcc31 NetworkPkg/UefiPxeBcDxe: Add the clarification compared to UefiPxeBcDxe in MdeModulePkg. should be updated, because the comparisons to MdeModulePkg drivers will no longer make sense. (3) These library class resolutions are already spelled out in "ArmVirtPkg/ArmVirt.dsc.inc". Please see under the comment # # CryptoPkg libraries needed by multiple firmware features # We shouldn't duplicate those lib class resolutions. (4) In particular, the "CryptoPkg/Library/OpensslLib/OpensslLib.inf" instance contains TLS support, and it is overkill for just IPv6. The "OpensslLibCrypto.inf" instance is sufficient. > > ################################################################################ > # > @@ -346,18 +349,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 (5) This change will break the build (without NETWORK_IP6_ENABLE). Namely, "NetworkPkg/IScsiDxe/IScsiDxe.inf" would be compiled unconditionally. However, "NetworkPkg/IScsiDxe/IScsiDxe.inf" depends on the TcpIoLib class -- as the sole driver in the edk2 tree --, and we only resolve that lib class (in "ArmVirtPkg/ArmVirt.dsc.inc") if NETWORK_IP6_ENABLE is defined. * If we decide that "NetworkPkg/IScsiDxe/IScsiDxe.inf" is an integral part of the edk2 network driver stack (even without NETWORK_IP6_ENABLE), then: - we should make the current TcpIoLib class resolution unconditional, - we should make the current IntrinsicLib / OpensslLib / BaseCryptLib resolutions unconditional, * Otherwise (= if we consider "NetworkPkg/IScsiDxe/IScsiDxe.inf" optional for networking), we should introduce NETWORK_ISCSI_ENABLE, and *replace* NETWORK_IP6_ENABLE with NETWORK_ISCSI_ENABLE in the above lib class resolutions. (And also make the DSC / FDF inclusion of "NetworkPkg/IScsiDxe/IScsiDxe.inf" dependent on the new NETWORK_ISCSI_ENABLE.) Thanks Laszlo > 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 >