Re: [PATCH 1/6] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.

[Laszlo Ersek <lersek@redhat.com>]

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 <jiaxin.wu@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Fu Siyuan <siyuan.fu@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.

> +  #
> +  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.

> +  # 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.<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
> 

Thanks,
Laszlo