Hi, Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, November 21, 2018 6:47 PM > To: Fu, Siyuan <siyuan...@intel.com> > Cc: edk2-devel@lists.01.org; Ye, Ting <ting...@intel.com>; Wu, Jiaxin > <jiaxin...@intel.com> > Subject: Re: [edk2] [PATCH 1/6] NetworkPkg: Add DSC/FDF include segment > files to NetworkPkg. > > 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.
Agree, I Will fix this in v2 patch. > > > 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 <jiaxin...@intel.com> > > Cc: Ting Ye <ting...@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Fu Siyuan <siyuan...@intel.com> > > --- > > 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.<BR> > > +# > > +# 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. You are correct. These lines were added before I wrote the file header, they are redundant now since the file header already describe it. Will remove it in V2 patch. > > > + # > > + 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 <LibraryClasses> 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. I also considered this problem when creating this patch, about whether I should resolve OpensslLib instance in NetworkPkg's include file. Actually I hesitated between your option 9a and 9b and can't decide which one is better, so I choose to only resolve network specific libraries and leave all other library to platform owner. You are correct that I should add more description about the dependency of Openssl, so the platform owner could get enough knowledge to make right choice. So I prefer your 9c solution, will add this in the v2 patch. > > > + # 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? No, will remove the "LibraryClasses.common.UEFI_APPLICATION" in v2 patch. > > > + > > +[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. I'm OK to remove this flag. > > > + > > +[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? It's quite a good suggestion. My initial intention is to make the include file as simple as possible, to minimize the platform owner's work, so I just provide 1 include file for DSC, and you are correct that it was done at the cost of losing flexibility and wasting build time. Now I think even we have 4 separate DSC include files, it's still much easier to organize than original 20 more INF, and with much more flexibility to platform owner. This could also solve the problem (11). > > > +!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 Yes and I'm not aware of this statement support, thanks. > > (14) If you agree with the split I'm suggesting under (12): can we move > these checks into the [Defines] fragment? Sure > > > + > > + 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. You are correct, that's why I leave the original "TLS_ENABLE" flag and set NETWORK_TLS_ENABLE to false in OVMF package's patch. If a platform want to override a driver or library component, it should disable the relative NETWORK_*** flag for the include file, and add the override component in its DSC/FDF separately. I haven't figure out a good solution except this method. > > > + > > + !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. It's OK with me. > > > +# > > +# Copyright (c) 2018, Intel Corporation. All rights reserved.<BR> > > +# > > +# 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 > > Finally, really thanks for your carefully and patiently review on this patch, and very good suggestions. > > Thanks, > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
