+Ard, +Leif
On 11/22/18 06:21, Fu Siyuan 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.
>
> 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
>
> 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>
> ---
>
> Notes:
> v2:
> 1. Split the "Network.dsc.inc" in to 4 files for different sections in DSC
> file. This could provide more flexibility to platform owner to use the
> include files.
> 2. Clarify the OpenSSL dependency of TLS, iSCSI and IPsec enable flag.
> 3. Use "!error" statement for incorrect flag value check.
> 4. Other decoration work according to Laszlo's comments.
>
> NetworkPkg/Network.fdf.inc | 69 ++++++++++
> NetworkPkg/NetworkComponents.dsc.inc | 71 ++++++++++
> NetworkPkg/NetworkDefines.dsc.inc | 138 ++++++++++++++++++++
> NetworkPkg/NetworkLibs.dsc.inc | 25 ++++
> NetworkPkg/NetworkPcds.dsc.inc | 22 ++++
> NetworkPkg/NetworkPkg.dsc | 28 +---
> 6 files changed, 331 insertions(+), 22 deletions(-)
>
> diff --git a/NetworkPkg/Network.fdf.inc b/NetworkPkg/Network.fdf.inc
> new file mode 100644
> index 000000000000..abd4c6c363d5
> --- /dev/null
> +++ b/NetworkPkg/Network.fdf.inc
> @@ -0,0 +1,69 @@
> +## @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 EDKII network stack drivers according to the value of flags
> described in
> +# "NetworkDefines.dsc.inc".
> +#
> +# 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
> + 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
> +
> + !if $(NETWORK_ISCSI_ENABLE) == TRUE
> + INF NetworkPkg/IScsiDxe/IScsiDxe.inf
> + !endif
> +
> + !if $(NETWORK_IPSEC_ENABLE) == TRUE
> + INF NetworkPkg/IpSecDxe/IpSecDxe.inf
> + !endif
> +!endif
This part looks good!
> diff --git a/NetworkPkg/NetworkComponents.dsc.inc
> b/NetworkPkg/NetworkComponents.dsc.inc
> new file mode 100644
> index 000000000000..8074489b8e06
> --- /dev/null
> +++ b/NetworkPkg/NetworkComponents.dsc.inc
> @@ -0,0 +1,71 @@
> +## @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) 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
> + 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
> +
> + !if $(NETWORK_IPSEC_ENABLE) == TRUE
> + NetworkPkg/IpSecDxe/IpSecDxe.inf
> + !endif
> +!endif
This also looks great; it matches the order & formatting of the FDF
include file too.
> diff --git a/NetworkPkg/NetworkDefines.dsc.inc
> b/NetworkPkg/NetworkDefines.dsc.inc
> new file mode 100644
> index 000000000000..648c065baadb
> --- /dev/null
> +++ b/NetworkPkg/NetworkDefines.dsc.inc
> @@ -0,0 +1,138 @@
> +## @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_IPSEC_ENABLE = TRUE
> +# DEFINE NETWORK_ISCSI_ENABLE = TRUE
> +# DEFINE NETWORK_VLAN_ENABLE = TRUE
> +#
> +# 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.
> +#
> +##
> +
> +!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
Nice, thanks!
> +
> +!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
Very clear. Great.
> +
> +!ifndef NETWORK_IPSEC_ENABLE
> + #
> + # This flag is to enable or disable IPsec 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 IPsec.
> + #
> + DEFINE NETWORK_IPSEC_ENABLE = TRUE
> +!endif
> +
Ditto.
One request for this file, at a higher level:
(1) The order of the default settings in this file *almost* matches the
order of the flags in the summary, at the top -- but not completely. Can
you please reorder the defaults so they match the summary? Basically:
- move the NETWORK_ISCSI_ENABLE to the end,
- then move NETWORK_VLAN_ENABLE to the end (after NETWORK_ISCSI_ENABLE).
That should restore the right order.
> +!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
> +!endif
Nice.
> diff --git a/NetworkPkg/NetworkLibs.dsc.inc b/NetworkPkg/NetworkLibs.dsc.inc
> new file mode 100644
> index 000000000000..67d09c262074
> --- /dev/null
> +++ b/NetworkPkg/NetworkLibs.dsc.inc
> @@ -0,0 +1,25 @@
> +## @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) 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.
> +#
> +##
> +
> + 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
In theory, it would be possible to wrap this part into a NETWORK_ENABLE
condition, and then resolve some of the library classes conditional on
further flags, such as HttpLib dependent on NETWORK_HTTP_BOOT_ENABLE.
However:
- these library instances are not meant to be replaced by other libary
instances, when a platform needs the library classes for any purpose
(i.e., these are single-instance lib classes),
- and even if the platform doesn't need some (or all) of these lib
classes, resolving them does no harm (negligible performance impact).
So I agree to keep this part simple.
> diff --git a/NetworkPkg/NetworkPcds.dsc.inc b/NetworkPkg/NetworkPcds.dsc.inc
> new file mode 100644
> index 000000000000..3eee5b3ae0bf
> --- /dev/null
> +++ b/NetworkPkg/NetworkPcds.dsc.inc
> @@ -0,0 +1,22 @@
> +## @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) 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_ALLOW_HTTP_CONNECTIONS) == TRUE
> + gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE
> +!endif
(2) Looks good, but I think the PCD setting should be restricted
*additionally* to NETWORK_ENABLE && NETWORK_HTTP_BOOT_ENABLE.
> diff --git a/NetworkPkg/NetworkPkg.dsc b/NetworkPkg/NetworkPkg.dsc
> index b543caa08fb1..654f73785054 100644
> --- a/NetworkPkg/NetworkPkg.dsc
> +++ b/NetworkPkg/NetworkPkg.dsc
> @@ -24,6 +24,8 @@ [Defines]
> BUILD_TARGETS = DEBUG|RELEASE|NOOPT
> SKUID_IDENTIFIER = DEFAULT
>
> +!include NetworkPkg/NetworkDefines.dsc.inc
> +
> [LibraryClasses]
> DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
> BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> @@ -47,12 +49,8 @@ [LibraryClasses]
>
> DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
> SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
>
> - 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
> +!include NetworkPkg/NetworkLibs.dsc.inc
> +
> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> @@ -86,6 +84,7 @@ [PcdsFeatureFlag]
> [PcdsFixedAtBuild]
> gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x2f
> gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80000000
> +!include NetworkPkg/NetworkPcds.dsc.inc
>
>
> ###################################################################################################
> #
> @@ -107,25 +106,10 @@ [PcdsFixedAtBuild]
>
> ###################################################################################################
>
> [Components]
> - NetworkPkg/Ip6Dxe/Ip6Dxe.inf
> - NetworkPkg/TcpDxe/TcpDxe.inf
> - NetworkPkg/Udp6Dxe/Udp6Dxe.inf
> - NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf
> - NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf
> - NetworkPkg/DnsDxe/DnsDxe.inf
> - NetworkPkg/HttpDxe/HttpDxe.inf
> - NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf
> - NetworkPkg/HttpBootDxe/HttpBootDxe.inf
> +!include NetworkPkg/NetworkComponents.dsc.inc
>
> NetworkPkg/Application/IpsecConfig/IpSecConfig.inf
> NetworkPkg/Application/VConfig/VConfig.inf
>
> -[Components.IA32, Components.X64]
> - NetworkPkg/IpSecDxe/IpSecDxe.inf
> - NetworkPkg/IScsiDxe/IScsiDxe.inf
> - NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> - NetworkPkg/TlsDxe/TlsDxe.inf
> - NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> -
> [BuildOptions]
> *_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
>
This part makes me curious. :)
It's good that the defaults for the new build flags ensure that
NetworkPkg.dsc's coverage not shrink -- everything that used to be built
by default continues to be built by default. However:
(3) This change will build IpSecDxe, IScsiDxe, UefiPxeBcDxe, TlsDxe and
TlsAuthConfigDxe on ARM and AARCH64 as well. I actually welcome that,
but that's a functional change, and so it should be a separate patch.
Namely, please *prepend* a patch to the series that simply deletes the
"[Components.IA32, Components.X64]" line, bringing all these drivers to
ARM/AARCH64, in the NetworkPkg.dsc build. Once we validate the new patch
separately (simply by test-building it), then the current patch can
simply replace the component list with the !include directive -- and
such a replacement won't incur arch-specific changes.
(4) I'm noticing there are two applications too. Should we move them to
"NetworkApps.dsc.inc" and "NetworkApps.fdf.inc", perhaps?
And in those include files, the individual apps would be gated by
NETWORK_ENABLE, plus NETWORK_IPSEC_ENABLE / NETWORK_VLAN_ENABLE.
This is a tricky question, so I don't mind if we solve it later, or
don't solve it at all. It is tricky because:
- Even if a platform builds these features into the platform firmware,
it might not want to offer these applications -- and then we shouldn't
even build the applications. Therefore, we shouldn't simply move the
applications to the components include file. Hence "NetworkApps.dsc.inc".
- Assuming a platform *does* want to offer these applications (and hence
builds them through the suggested "NetworkApps.dsc.inc"), the apps could
be offered in a different firmware volume, or even on external
(remvoable) media. That means that we shouldn't simply add the apps to
the FDF include file. Hence "NetworkApps.fdf.inc".
Anyway, this is just my curiosity. :)
In summary, I would be OK with you fixing up (1) and (2) on push (and I
considered givig my R-b on that condition, in advance). And, I'm fine
ignoring (4) totally at the moment. However, for (3), I'd really like if
we could build-test that change in separation. And then the next version
(v3) of this specific patch would be totally architecture-independent.
Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
