Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.

["Laszlo Ersek" <lersek@redhat.com>]

On 04/25/19 14:37, Liming Gao wrote:
> This patch provides a set of include segment files for platform owner to
> easily enable/disable network stack support on their platform.
> 
> For DSC, there are:
> - 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).
> For FDF, there is:
> - a "Network.fdf.inc" for the [Fv*] section(s).
> 
> These files can be added to the platform DSC/FDF file by using
>   !include NetworkPkg/xxx
> where "xxx" is the *.inc file name.
> 
> 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, please
> check "NetworkDefines.dsc.inc" for a detail description of each flag.

(1) Please clarify this paragraph as follows:

A platform DSC file can diverge from the defaults in
"NetworkDefines.dsc.inc" by setting the individual DEFINEs before
including "NetworkDefines.dsc.inc".

(The current paragraph does say "before the include line", but it
doesn't say *which* include line. I had to check
"NetworkDefines.dsc.inc" to see that it (correctly) uses "!ifndef
FLAG...", to understand the idea.)

> 
> 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_ISCSI_ENABLE           = TRUE
>   DEFINE NETWORK_VLAN_ENABLE            = TRUE
>   DEFINE PLATFORMX64_ENABLE             = FALSE

(2) The PLATFORMX64_ENABLE flag looks useless; nothing depends on it.

... looking at the next patch (v3 3/3), it seems that PLATFORMX64_ENABLE
has a use after all -- but it only matters for platforms that include
the "high-level" include files.

Please modify the commit message on this patch (2/3), and also
"NetworkDefines.dsc.inc", to explain PLATFORMX64_ENABLE. (I.e. both the
purpose of the PLATFORMX64_ENABLE flag, and also the fact that it can be
ignored by platforms that include the individual INC files.)

... Actually, under point (9) below, I will suggest dropping
PLATFORMX64_ENABLE altogether.

> 
> Related BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1293

(3) Please include this BZ in the release planning wiki page; it is
important to platforms:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning

> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> Signed-off-by: Fu Siyuan <siyuan.fu@intel.com>
> ---
>  NetworkPkg/Network.fdf.inc           |  56 ++++++++++++++++
>  NetworkPkg/NetworkComponents.dsc.inc |  61 +++++++++++++++++
>  NetworkPkg/NetworkDefines.dsc.inc    | 126 +++++++++++++++++++++++++++++++++++
>  NetworkPkg/NetworkLibs.dsc.inc       |  19 ++++++
>  NetworkPkg/NetworkPcds.dsc.inc       |  16 +++++
>  5 files changed, 278 insertions(+)
>  create mode 100644 NetworkPkg/Network.fdf.inc
>  create mode 100644 NetworkPkg/NetworkComponents.dsc.inc
>  create mode 100644 NetworkPkg/NetworkDefines.dsc.inc
>  create mode 100644 NetworkPkg/NetworkLibs.dsc.inc
>  create mode 100644 NetworkPkg/NetworkPcds.dsc.inc
> 
> diff --git a/NetworkPkg/Network.fdf.inc b/NetworkPkg/Network.fdf.inc
> new file mode 100644
> index 0000000000..8518bad12c
> --- /dev/null
> +++ b/NetworkPkg/Network.fdf.inc
> @@ -0,0 +1,56 @@
> +## @file
> +# Network FDF include file for All Architectures.
> +#
> +# This file can be included to a platform FDF by using "!include NetworkPkg/Network.fdf.inc"

(4) Not too important, but I suggest rewrapping all comments to 80 chars
if possible.

> +# to add EDKII network stack drivers according to the value of flags described in
> +# "NetworkPkg/Network.dsc.inc".
> +#
> +# Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>

(5) We should probably say 2019, or 2018-2019 here. (The rest of the new
files say 2019.)

> +#
> +#    SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +
> +!if $(NETWORK_ENABLE) == TRUE
> +  INF  MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
> +
> +  !if $(NETWORK_SNP_ENABLE) == TRUE
> +    INF  MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
> +  !endif
> +
> +  !if $(NETWORK_VLAN_ENABLE) == TRUE
> +    INF  MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf
> +  !endif
> +
> +  INF  MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf
> +
> +  !if $(NETWORK_IP4_ENABLE) == TRUE
> +    INF  MdeModulePkg/Universal/Network/ArpDxe/ArpDxe.inf
> +    INF  MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Dxe.inf
> +    INF  MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Dxe.inf
> +    INF  MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
> +    INF  MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
> +  !endif
> +
> +  !if $(NETWORK_IP6_ENABLE) == TRUE
> +    INF  NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf
> +    INF  NetworkPkg/Ip6Dxe/Ip6Dxe.inf
> +    INF  NetworkPkg/Udp6Dxe/Udp6Dxe.inf
> +    INF  NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf
> +  !endif
> +
> +  INF  NetworkPkg/TcpDxe/TcpDxe.inf
> +  INF  NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> +
> +  !if $(NETWORK_TLS_ENABLE) == TRUE
> +    INF  NetworkPkg/TlsDxe/TlsDxe.inf
> +    INF  NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.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

(6) This doesn't cover NETWORK_ISCSI_ENABLE,
"NetworkPkg/IScsiDxe/IScsiDxe.inf".

(The DSC file does.)


(7) Is ArpDxe really specific to IPv4? I grepped the INF files for
"gEfiArpProtocolGuid" and "gEfiArpServiceBindingProtocolGuid", and two
drivers consume those:

- Ip4Dxe
- UefiPxeBcDxe

The latter driver (UefiPxeBcDxe) is not specific to IPv4 vs. IPv6.
Therefore, should we include ArpDxe simply under "NETWORK_ENABLE"?

(I see that, elsewhere, we have an !error if the platform or user tries
to exclude both IPv4 and IPv6, and that's OK. However, if the platform
or user picks IPv6 only, then the above will prevent them from having
ArpDxe, and then PXE boot might not work.)

Hmmmm I could be wrong about this; in the UefiPxeBcDxe driver, the only
references to the ARP protocol GUIDs are in the functions
- PxeBcGetNicByIp4Children
- PxeBcDestroyIp4Children
- PxeBcCreateIp4Children

OK. I guess ARP is an IPv4-only requirement then.


Otherwise, this FDF include looks good, checked against the OVMF FDF
files, and from "ArmVirtQemuFvMain.fdf.inc". (ArmVirtXen.fdf doesn't
include the network stack.)

> +
> +!endif
> diff --git a/NetworkPkg/NetworkComponents.dsc.inc b/NetworkPkg/NetworkComponents.dsc.inc
> new file mode 100644
> index 0000000000..aede5ea8be
> --- /dev/null
> +++ b/NetworkPkg/NetworkComponents.dsc.inc
> @@ -0,0 +1,61 @@
> +## @file
> +# Network DSC include file for [Components*] section of all Architectures.
> +#
> +# This file can be included to the [Components*] section(s) of a platform DSC file
> +# by using "!include NetworkPkg/NetworkComponents.dsc.inc" to specify the INF files
> +# of EDKII network drivers according to the value of flags described in
> +# "NetworkDefines.dsc.inc".
> +#
> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +#
> +#    SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +!if $(NETWORK_ENABLE) == TRUE
> +  MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
> +
> +  !if $(NETWORK_SNP_ENABLE) == TRUE
> +    MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
> +  !endif
> +
> +  !if $(NETWORK_VLAN_ENABLE) == TRUE
> +    MdeModulePkg/Universal/Network/VlanConfigDxe/VlanConfigDxe.inf
> +  !endif
> +
> +  MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.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/Udp4Dxe/Udp4Dxe.inf
> +    MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
> +  !endif
> +
> +  !if $(NETWORK_IP6_ENABLE) == TRUE
> +    NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf
> +    NetworkPkg/Ip6Dxe/Ip6Dxe.inf
> +    NetworkPkg/Udp6Dxe/Udp6Dxe.inf
> +    NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf
> +  !endif
> +
> +  NetworkPkg/TcpDxe/TcpDxe.inf
> +  NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> +
> +  !if $(NETWORK_TLS_ENABLE) == TRUE
> +    NetworkPkg/TlsDxe/TlsDxe.inf
> +    NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.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_ISCSI_ENABLE) == TRUE
> +    NetworkPkg/IScsiDxe/IScsiDxe.inf
> +  !endif
> +!endif

OK, this matches the FDF include file (except for IScsiDxe, but that's a
problem I pointed out under (6)).

The NETWORK_TLS_ENABLE part looks good too. It's worth noting that it
won't be suitable for OVMF, because OVMF hooks TlsAuthConfigLib into
TlsAuthConfigDxe, for dynamically setting the variables
"HttpTlsCipherList" and "TlsCaCertificate".

But, that's not a problem for this generic DSC include file. OVMF can
simply set NETWORK_TLS_ENABLE to FALSE, and preserve its own (current)
TLS_ENABLE build flag, and everything in the DSC/FDF that depends on
that platform build flag.

> diff --git a/NetworkPkg/NetworkDefines.dsc.inc b/NetworkPkg/NetworkDefines.dsc.inc
> new file mode 100644
> index 0000000000..7a318c98ca
> --- /dev/null
> +++ b/NetworkPkg/NetworkDefines.dsc.inc
> @@ -0,0 +1,126 @@
> +## @file
> +# Network DSC include file for [Defines] section of all Architectures.
> +#
> +# This file can be included to the [Defines] section of a platform DSC file by
> +# using "!include NetworkPkg/NetworkDefines.dsc.inc" to set default value of
> +# flags if they are not defined somewhere else, and also check the value to see
> +# if there is any conflict.
> +#
> +# These flags can be defined before the !include line, or 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_ISCSI_ENABLE           = TRUE
> +#   DEFINE NETWORK_VLAN_ENABLE            = TRUE
> +#   DEFINE PLATFORMX64_ENABLE             = FALSE
> +#
> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +#
> +#    SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +!ifndef NETWORK_ENABLE
> +  #
> +  # This flag is to enable or disable the whole network stack.
> +  #
> +  DEFINE NETWORK_ENABLE = TRUE
> +!endif
> +
> +!ifndef NETWORK_SNP_ENABLE
> +  #
> +  # This flag is to include the common SNP driver or not.
> +  #
> +  DEFINE NETWORK_SNP_ENABLE = TRUE
> +!endif
> +
> +!ifndef NETWORK_VLAN_ENABLE
> +  #
> +  # This flag is to enable or disable VLAN feature.
> +  #
> +  DEFINE NETWORK_VLAN_ENABLE = TRUE
> +!endif
> +
> +!ifndef NETWORK_IP4_ENABLE
> +  #
> +  # This flag is to enable or disable IPv4 network stack.
> +  #
> +  DEFINE NETWORK_IP4_ENABLE = TRUE
> +!endif
> +
> +!ifndef NETWORK_IP6_ENABLE
> +  #
> +  # This flag is to enable or disable IPv6 network stack.
> +  #
> +  DEFINE NETWORK_IP6_ENABLE = TRUE
> +!endif
> +
> +!ifndef NETWORK_TLS_ENABLE
> +  #
> +  # This flag is to enable or disable TLS feature.
> +  #
> +  # Note: This feature depends on the OpenSSL building. To enable this feature, please
> +  #       follow the instructions found in the file "OpenSSL-HOWTO.txt" located in
> +  #       CryptoPkg\Library\OpensslLib to enable the OpenSSL building first.
> +  #       The OpensslLib.inf library instance should be used since libssl is required.
> +  #
> +  DEFINE NETWORK_TLS_ENABLE = TRUE
> +!endif
> +
> +!ifndef NETWORK_HTTP_BOOT_ENABLE
> +  #
> +  # This flag is to enable or disable HTTP(S) boot feature.
> +  #
> +  DEFINE NETWORK_HTTP_BOOT_ENABLE = TRUE
> +!endif
> +
> +!ifndef NETWORK_ALLOW_HTTP_CONNECTIONS
> +  #
> +  # Indicates whether HTTP connections (i.e., unsecured) are permitted or not.
> +  #
> +  # 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_ISCSI_ENABLE
> +  #
> +  # This flag is to enable or disable iSCSI feature.
> +  #
> +  # Note: This feature depends on the OpenSSL building. To enable this feature, please
> +  #       follow the instructions found in the file "OpenSSL-HOWTO.txt" located in
> +  #       CryptoPkg\Library\OpensslLib to enable the OpenSSL building first.
> +  #       Both OpensslLib.inf and OpensslLibCrypto.inf library instance can be used
> +  #       since libssl is not required for iSCSI.
> +  #
> +  DEFINE NETWORK_ISCSI_ENABLE = TRUE
> +!endif
> +
> +!if $(NETWORK_ENABLE) == TRUE
> +  #
> +  # Check the flags to see if there is any conflict.
> +  #
> +  !if ($(NETWORK_IP4_ENABLE) == FALSE) AND ($(NETWORK_IP6_ENABLE) == FALSE)
> +    !error "Must enable at least IP4 or IP6 stack if NETWORK_ENABLE is set to TRUE!"
> +  !endif
> +
> +  !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) AND ($(NETWORK_TLS_ENABLE) == FALSE) AND ($(NETWORK_ALLOW_HTTP_CONNECTIONS) == FALSE)
> +    !error "Must enable TLS to support HTTPS, or allow unsecured HTTP connection, if NETWORK_HTTP_BOOT_ENABLE is set to TRUE!"
> +  !endif

In practice, this is fine, from an OvmfPkg and ArmVirtPkg perspective,
so I'm not asking for the code to be changed. But, I think it's worth
raising the following at least in theory:

Above I mentioned that platforms might want to set "NETWORK_TLS_ENABLE"
to FALSE, but still include the TLS drivers with various library
instances hooked into them. In that case, the sanity check above will
force them, unjustifiedly, to permit unsecured HTTP connections, because
it will think that TLS is absent.

(8) Therefore, should we introduce an "override" flag for the last
sanity check above?

Again, this is not a practical concern for me -- both OvmfPkg and
ArmVirtPkg enable unsecured HTTP connections already, so the above check
will never fire in those platforms. I'm fine if we decide to address
this "problem" first when an actual platform runs into it.

> +!endif
> +
> +!ifndef PLATFORMX64_ENABLE
> +  #
> +  # PLATFORMX64_ENABLE is set to TRUE when PEI is IA32 and DXE is X64 platform
> +  #
> +  DEFINE PLATFORMX64_ENABLE = FALSE
> +!endif

(9) I commented on this flag under (2) above already. But, now that I'm
reading the explanation too, and re-checking patch "v3 3/3", I think we
should simply *eliminate* this define, and in patch 3, we should use:

  !if ("X64" in $(ARCH))

  [Components.X64]
  !include NetworkPkg/NetworkComponents.dsc.inc

  !else

  [Components.IA32, Components.ARM, Components.AARCH64]
  !include NetworkPkg/NetworkComponents.dsc.inc

  !endif

Because, the first branch will:

- cover IA32 PEI + X64 DXE
  (include the network stack in DXE only)

- cover X64 PEI+DXE
  (as if we had written just [Components]

and the second branch will:
- cover IA32 PEI+DXE
- cover ARM PEI+DXE
- cover AARCH64 PEI+DXE
- exclude EBC altogether

(In fact, if we were willing to ignore EBC, the second branch could
simply say [Components].)

> diff --git a/NetworkPkg/NetworkLibs.dsc.inc b/NetworkPkg/NetworkLibs.dsc.inc
> new file mode 100644
> index 0000000000..dac6b37c6a
> --- /dev/null
> +++ b/NetworkPkg/NetworkLibs.dsc.inc
> @@ -0,0 +1,19 @@
> +## @file
> +# Network DSC include file for [LibraryClasses*] section of all Architectures.
> +#
> +# This file can be included to the [LibraryClasses*] section(s) of a platform DSC file
> +# by using "!include NetworkPkg/NetworkLibs.dsc.inc" to specify the library instances
> +# of EDKII network library classes.
> +#
> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +#
> +#    SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +  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) If we decide to provide all these lib class resolutions without
checking the DEFINEs -- e.g., we decide to resolve HttpLib without
checking HTTP_BOOT_ENABLE --, then that's fine, but IMO we should add a
simple comment about it.

(11) I think the following lib class resolution is missing:

  TlsLib|CryptoPkg/Library/TlsLib/TlsLib.inf


Other than that, I agree this include file is good. The OpenSSL lib
class resolution can depend on many factors, and the NetworkPkg aspects
are already well explained in the DSC include file, near the
NETWORK_TLS_ENABLE and NETWORK_ISCSI_ENABLE flags.



> diff --git a/NetworkPkg/NetworkPcds.dsc.inc b/NetworkPkg/NetworkPcds.dsc.inc
> new file mode 100644
> index 0000000000..f874b382ef
> --- /dev/null
> +++ b/NetworkPkg/NetworkPcds.dsc.inc
> @@ -0,0 +1,16 @@
> +## @file
> +# Network DSC include file for [Pcds*] section of all Architectures.
> +#
> +# This file can be included to the [Pcds*] section(s) of a platform DSC file
> +# by using "!include NetworkPkg/NetworkPcds.dsc.inc" to specify PCD settings
> +# according to the value of flags described in "NetworkDefines.dsc.inc".
> +#
> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +#
> +#    SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +!if $(NETWORK_ALLOW_HTTP_CONNECTIONS) == TRUE
> +  gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE
> +!endif
> 

This file looks good.


The "v3 2/3" patch makes me happy -- thank you for providing the include
snippets without section headers, letting the platform provide its own
section headers!

Laszlo