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 DBC0321A143EF for ; Wed, 21 Nov 2018 02:46:43 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 28F0489AD6; Wed, 21 Nov 2018 10:46:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-240.rdu2.redhat.com [10.10.120.240]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9849D277CA; Wed, 21 Nov 2018 10:46:41 +0000 (UTC) To: Fu Siyuan Cc: edk2-devel@lists.01.org, Ting Ye , Jiaxin Wu References: <20181121052819.15744-1-siyuan.fu@intel.com> <20181121052819.15744-2-siyuan.fu@intel.com> From: Laszlo Ersek Message-ID: <1f37c165-aa7e-03d2-218d-b0c394c1e2fe@redhat.com> Date: Wed, 21 Nov 2018 11:46:40 +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: <20181121052819.15744-2-siyuan.fu@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Wed, 21 Nov 2018 10:46:43 +0000 (UTC) Subject: Re: [PATCH 1/6] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg. 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: Wed, 21 Nov 2018 10:46:45 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/21/18 06:28, Fu Siyuan wrote: > The "Network.dsc.inc" and "Network.fdf.inc" are added for platform owner to > easily enable/disable network stack support on their platform, by adding > !include NetworkPkg/Network.dsc.inc > and > !include NetworkPkg/Network.fdf.inc > to their platform DSC/FDF files. > > A set of flags can be changed before the include line or in build command > line ("-D FLAG=VALUE") to enable or disable related feature set. > > The default value of these flags are: > DEFINE NETWORK_ENABLE = TRUE > DEFINE NETWORK_SNP_ENABLE = TRUE > DEFINE NETWORK_IP4_ENABLE = TRUE > DEFINE NETWORK_IP6_ENABLE = TRUE > DEFINE NETWORK_TLS_ENABLE = TRUE > DEFINE NETWORK_HTTP_BOOT_ENABLE = TRUE > DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE > DEFINE NETWORK_IPSEC_ENABLE = TRUE > DEFINE NETWORK_ISCSI_ENABLE = TRUE > DEFINE NETWORK_VLAN_ENABLE = TRUE (1) This table is inconsistent with regard to alignment. In some cases, there are attempts to align the equal signs, and the assigned values (such as NETWORK_ENABLE and NETWORK_ALLOW_HTTP_CONNECTIONS), however, as a whole, the table is inconsistent. Please align all the equal signs and the assigned values to the longest macro name, namely NETWORK_ALLOW_HTTP_CONNECTIONS. > Detail description of each flag is in Network.dsc.inc file. > > Related BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1293 > > Cc: Jiaxin Wu > Cc: Ting Ye > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Fu Siyuan > --- > NetworkPkg/Network.dsc.inc | 203 ++++++++++++++++++++ > NetworkPkg/Network.fdf.inc | 81 ++++++++ > 2 files changed, 284 insertions(+) > > diff --git a/NetworkPkg/Network.dsc.inc b/NetworkPkg/Network.dsc.inc > new file mode 100644 > index 000000000000..50cf93ba816a > --- /dev/null > +++ b/NetworkPkg/Network.dsc.inc > @@ -0,0 +1,203 @@ > +## @file > +# Network DSC include file for All Architectures. > +# > +# This file can be included to a platform DSC by using "!include NetworkPkg/Network.dsc.inc" > +# to add edk2 network stack drivers. > +# Below flags can be changed on the command line to enable or disable related feature > +# support. > +# -D FLAG=VALUE > +# The default value of these flags are: > +# DEFINE NETWORK_ENABLE = TRUE > +# DEFINE NETWORK_SNP_ENABLE = TRUE > +# DEFINE NETWORK_IP4_ENABLE = TRUE > +# DEFINE NETWORK_IP6_ENABLE = TRUE > +# DEFINE NETWORK_TLS_ENABLE = TRUE > +# DEFINE NETWORK_HTTP_BOOT_ENABLE = TRUE > +# DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE > +# DEFINE NETWORK_IPSEC_ENABLE = TRUE > +# DEFINE NETWORK_ISCSI_ENABLE = TRUE > +# DEFINE NETWORK_VLAN_ENABLE = TRUE (2) Same as (1). > +# > +# Copyright (c) 2018, Intel Corporation. All rights reserved.
> +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +## > + > +[Defines] > +!ifndef NETWORK_ENABLE > + # > + # This flag is to enable or disable the whole network stack. > + # These can be changed on the command line. > + # -D FLAG=VALUE (3) I suggest dropping the statement "These can be changed on the command line". I also suggest dropping the generic "-D FLAG=VALUE" line. Both of those apply to all settings, and they are well explained in the general description near the top. > + # > + DEFINE NETWORK_ENABLE = TRUE > +!endif > + > +!ifndef NETWORK_SNP_ENABLE > + # > + # This flag is to include the common SNP driver or not. > + # These can be changed on the command line. > + # -D FLAG=VALUE > + # > + DEFINE NETWORK_SNP_ENABLE = TRUE > +!endif > + > +!ifndef NETWORK_IP4_ENABLE > + # > + # This flag is to enable or disable IPv4 network stack. > + # These can be changed on the command line. > + # -D FLAG=VALUE > + # > + DEFINE NETWORK_IP4_ENABLE = TRUE > +!endif > + > +!ifndef NETWORK_IP6_ENABLE > + # > + # This flag is to enable or disable IPv6 network stack. > + # These can be changed on the command line. > + # -D FLAG=VALUE > + # > + DEFINE NETWORK_IP6_ENABLE = TRUE > +!endif > + > +!ifndef NETWORK_TLS_ENABLE > + # > + # This flag is to enable or disable TLS feature. > + # These can be changed on the command line. > + # -D FLAG=VALUE > + # > + # Note: TLS feature highly depends on the OpenSSL building. To enable this > + # feature, please follow the instructions found in the file "Patch-HOWTO.txt" (4) The file is now called "OpenSSL-HOWTO.txt". (5) Please strip all space characters directly preceding CRLFs. (There are multiple instances in this patch.) > + # located in CryptoPkg\Library\OpensslLib to enable the OpenSSL building first. > + # > + DEFINE NETWORK_TLS_ENABLE = TRUE > +!endif > + > +!ifndef NETWORK_HTTP_BOOT_ENABLE > + # > + # This flag is to enable or disable HTTP(s) boot feature. (6) I think we should spell it as HTTP(S), upper-case "S". "HTTPs" looks strange. > + # These can be changed on the command line. > + # -D FLAG=VALUE > + # > + DEFINE NETWORK_HTTP_BOOT_ENABLE = TRUE > +!endif > + > +!ifndef NETWORK_ALLOW_HTTP_CONNECTIONS > + # > + # Indicates whether HTTP connections (i.e., unsecured) are permitted or not. > + # -D FLAG=VALUE > + # > + # Note: If NETWORK_ALLOW_HTTP_CONNECTIONS is TRUE, HTTP connections are allowed. > + # Both the "https://" and "http://" URI schemes are permitted. Otherwise, HTTP > + # connections are denied. Only the "https://" URI scheme is permitted. > + # > + DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE > +!endif > + > +!ifndef NETWORK_IPSEC_ENABLE > + # > + # This flag is to enable or disable IPsec feature. > + # These can be changed on the command line. > + # -D FLAG=VALUE > + # > + DEFINE NETWORK_IPSEC_ENABLE = TRUE > +!endif (7) If IPSEC requires OpenSSL, please spell that out here. > + > +!ifndef NETWORK_ISCSI_ENABLE > + # > + # This flag is to enable or disable iSCSI feature. (8) Please make a note here too about the OpenSSL dependency. (9) Regarding OpenSSL dependencies, I see that lower down, this patch resolves some library classes to specific library instances. I guess that's OK, given that a platform is never expected to resolve these library classes to different (possibly platform-specific) library instances. However, OpenSSL sits somewhere in the middle. It is true that a platform is not expected provide its own OpensslLib instance. On the other hand, the platform *should* make a choice between "OpensslLib.inf" and "OpensslLibCrypto.inf". And, in my opinion, this DSC include file should support the platform owner in making that choice. I can see multiple options here. (9a) Dependent on various NETWORK_* flags, resolve the OpensslLib class globally, in this DSC include file. However, I think this isn't flexible enough, for all platform needs. (9b) Do not resolve OpensslLib globally, but make sure OpensslLib is resolved for *individual* NetworkPkg modules on a case-by-case basis (using the INF-scoped syntax in this DSC include). This approach would mean that TLS drivers get "OpensslLib.inf" hooked into them, while IScsiDxe only gets "OpensslLibCrypto.inf". Ultimately, if someone looks at a build report file, they should be able to decide quickly whether the full-blown "OpensslLib.inf" lib instance was used at all. (9c) Don't resolve OpensslLib at all, but precisely document the lib instance expectations near the NETWORK_* flags. I.e., wherever you mention OpenSSL as a dependency (NETWORK_TLS_ENABLE, NETWORK_ISCSI_ENABLE, and possibly NETWORK_IPSEC_ENABLE), please also explain the minimum required OpenSSL lib instance. > + # These can be changed on the command line. > + # -D FLAG=VALUE > + # > + DEFINE NETWORK_ISCSI_ENABLE = TRUE > +!endif > + > +!ifndef NETWORK_VLAN_ENABLE > + # > + # This flag is to enable or disable VLAN feature. > + # These can be changed on the command line. > + # -D FLAG=VALUE > + # > + DEFINE NETWORK_VLAN_ENABLE = TRUE > +!endif > + > +[LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION] > + DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf > + NetLib|MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf > + IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf > + UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf > + TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf > + HttpLib|MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.inf (10) Does this DSC include file actually refer to any UEFI_APPLICATION? > + > +[PcdsFixedAtBuild] > +!if $(NETWORK_ALLOW_HTTP_CONNECTIONS) == TRUE > + gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE > +!endif (11) I'm not sure this is flexible enough. First, in "OvmfPkg/OvmfPkgIa32X64.dsc", we set the PCD only under [PcdsFixedAtBuild.X64], not under [PcdsFixedAtBuild]. I agree that in practice, such a change shouldn't be a problem however. Second, a more practical observation: NetworkPkg.dec declares this PCD not just as fixed, but also as patchable-in-module. As far as I understand, the above DSC include hunk will prevent platforms from using the PCD as patchable. I think the most flexible option would be to simply remove the NETWORK_ALLOW_HTTP_CONNECTIONS build flag, from this patch, and to allow platforms to set the PCD however they want. A build macro ("-D") is not expressive enough for this. Also remember that "--pcd" can be passed on the build command line too, so not much usability/convenience is lost by removing NETWORK_ALLOW_HTTP_CONNECTIONS. > + > +[Components] (12) How is this going to work with multi-arch platform builds, such as "OvmfPkg/OvmfPkgIa32X64.dsc", where the PEI phase is 32-bit, and the DXE phase is 64-bit? I don't think "OvmfPkgIa32X64.dsc" should build the networking modules for 32-bit too. They would never be included in the final flash device, so it's wasted compilation. Can we introduce separate DSC include files (fragments) for each of the DSC file sections? That is, we could have: - a "NetworkDefines.dsc.inc" for the [Defines] section(s), - a "NetworkLibs.dsc.inc" for the [LibraryClasses*] section(s), - a "NetworkPcds.dsc.inc" for the [Pcds*] section(s), - a "NetworkComponents.dsc.inc" for the [Components*] section(s). Then the platform DSC would be responsible for spelling out the precise section header it wants, and then include the matching DSC include file right below that. In other words, can we split this DSC include into multiple files, at the currently shown section headers, and remove the section headers themselves? > +!if $(NETWORK_ENABLE) == TRUE > + !if ($(NETWORK_IP4_ENABLE) == FALSE) AND ($(NETWORK_IP6_ENABLE) == FALSE) > + Must select at least IP4 or IP6 stack if NETWORK_ENABLE is set to TRUE. > + !endif (13) Did you mean to use the !error statement, introduced for: https://bugzilla.tianocore.org/show_bug.cgi?id=701 ? (The question applies to the rest of the error messages below.) > + > + !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) AND ($(NETWORK_TLS_ENABLE) == FALSE) AND ($(NETWORK_ALLOW_HTTP_CONNECTIONS) == FALSE) > + Must enable TLS to support HTTPs, or allow unsecured HTTP connection, if NETWORK_HTTP_BOOT_ENABLE is set to TRUE. > + !endif (14) If you agree with the split I'm suggesting under (12): can we move these checks into the [Defines] fragment? > + > + MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf > + > + !if $(NETWORK_SNP_ENABLE) == TRUE > + MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf > + !endif > + > + MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf > + NetworkPkg/TcpDxe/TcpDxe.inf > + NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf > + > + !if $(NETWORK_IP4_ENABLE) == TRUE > + MdeModulePkg/Universal/Network/ArpDxe/ArpDxe.inf > + MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Dxe.inf > + MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Dxe.inf > + MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf > + MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf > + !endif > + > + !if $(NETWORK_IP6_ENABLE) == TRUE > + NetworkPkg/Ip6Dxe/Ip6Dxe.inf > + NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf > + NetworkPkg/Udp6Dxe/Udp6Dxe.inf > + NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf > + !endif > + > + !if $(NETWORK_HTTP_BOOT_ENABLE) == TRUE > + NetworkPkg/DnsDxe/DnsDxe.inf > + NetworkPkg/HttpDxe/HttpDxe.inf > + NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf > + NetworkPkg/HttpBootDxe/HttpBootDxe.inf > + !endif > + > + !if $(NETWORK_IPSEC_ENABLE) == TRUE > + NetworkPkg/IpSecDxe/IpSecDxe.inf > + !endif > + > + !if $(NETWORK_TLS_ENABLE) == TRUE > + NetworkPkg/TlsDxe/TlsDxe.inf > + NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf > + !endif (15) Unfortunately, this isn't flexible enough for OVMF. OVMF hooks OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf into TlsAuthConfigDxe via NULL class resolution -- for setting up the CA certificates and cipher suites, in volatile UEFI variables, just in time. > + > + !if $(NETWORK_ISCSI_ENABLE) == TRUE > + NetworkPkg/IScsiDxe/IScsiDxe.inf > + !endif > + > + !if $(NETWORK_VLAN_ENABLE) == TRUE > + MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf > + !endif > + > +!endif > diff --git a/NetworkPkg/Network.fdf.inc b/NetworkPkg/Network.fdf.inc > new file mode 100644 > index 000000000000..481dbb368d23 > --- /dev/null > +++ b/NetworkPkg/Network.fdf.inc > @@ -0,0 +1,81 @@ > +## @file > +# Network FDF include file for All Architectures. > +# > +# This file can be included to a platform FDF by using "!include NetworkPkg/Network.fdf.inc" > +# to add edk2 network stack drivers. > +# Below flags can be changed on the command line to enable or disable related feature > +# support. > +# -D FLAG=VALUE > +# The default value of these flags are: > +# DEFINE NETWORK_ENABLE = TRUE > +# DEFINE NETWORK_SNP_ENABLE = TRUE > +# DEFINE NETWORK_IP4_ENABLE = TRUE > +# DEFINE NETWORK_IP6_ENABLE = TRUE > +# DEFINE NETWORK_TLS_ENABLE = TRUE > +# DEFINE NETWORK_HTTP_BOOT_ENABLE = TRUE > +# DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE > +# DEFINE NETWORK_IPSEC_ENABLE = TRUE > +# DEFINE NETWORK_ISCSI_ENABLE = TRUE > +# DEFINE NETWORK_VLAN_ENABLE = TRUE (16) I suggest removing this duplicated documentation, and referring the reader to the DSC include files instead. > +# > +# Copyright (c) 2018, Intel Corporation. All rights reserved.
> +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > + > +!if $(NETWORK_ENABLE) == TRUE > + !if $(NETWORK_SNP_ENABLE) == TRUE > + INF MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf (17) There is only one space character after "INF". It is not consistent with the rest (the rest uses two space characters). > + !endif > + > + INF MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf > + INF MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf (18) The DSC include file references the module INF files in a different order: DpcDxe, SnpDxe, MnpDxe. I suggest sticking with the same order in the FDF file, for easier maintenance. Please verify this (i.e. the listing order) for the rest of the drivers too. > + INF NetworkPkg/TcpDxe/TcpDxe.inf > + INF NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf > + > + !if $(NETWORK_IP4_ENABLE) == TRUE > + INF MdeModulePkg/Universal/Network/ArpDxe/ArpDxe.inf > + INF MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Dxe.inf > + INF MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf > + INF MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Dxe.inf > + INF MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf > + !endif > + > + !if $(NETWORK_IP6_ENABLE) == TRUE > + INF NetworkPkg/Ip6Dxe/Ip6Dxe.inf > + INF NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf > + INF NetworkPkg/Udp6Dxe/Udp6Dxe.inf > + INF NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf > + !endif > + > + !if $(NETWORK_HTTP_BOOT_ENABLE) == TRUE > + INF NetworkPkg/DnsDxe/DnsDxe.inf > + INF NetworkPkg/HttpDxe/HttpDxe.inf > + INF NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf > + INF NetworkPkg/HttpBootDxe/HttpBootDxe.inf > + !endif > + > + !if $(NETWORK_IPSEC_ENABLE) == TRUE > + INF NetworkPkg/IpSecDxe/IpSecDxe.inf > + !endif > + > + !if $(NETWORK_TLS_ENABLE) == TRUE > + INF NetworkPkg/TlsDxe/TlsDxe.inf > + INF NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf > + !endif > + > + !if $(NETWORK_VLAN_ENABLE) == TRUE > + INF MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf > + !endif > + > + !if $(NETWORK_ISCSI_ENABLE) == TRUE > + INF NetworkPkg/IScsiDxe/IScsiDxe.inf > + !endif > + > +!endif > Thanks, Laszlo