* [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg
@ 2019-04-25 12:37 Liming Gao
2019-04-25 12:37 ` [Patch v3 1/3] NetworkPkg DSC: Add the required ARM library to pass ARM build Liming Gao
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Liming Gao @ 2019-04-25 12:37 UTC (permalink / raw)
To: devel
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1293
BZ 1293 requests to move Network modules from MdeModulePkg to NetworkPkg.
To keep the backword compatiblity, Network package level include DSC/FDF
are introduced to be used in the platform DSC/FDF files. When Network
modules are moved from MdeModulePkg to NetworkPkg, Network package level
include DSC/FDF will be updated together. There is no impact on the platform
DSC/FDF file.
This patch set is to introduce network package level include DSC/FDF files.
Bases on previous discussion and the existing usage case, build flag will be
used to enable/disable the network features. PCD control feature way can be
discussed later. And, to meet with the different usages, this patch set
introduces the separate DSC for Defines/Pcds/Libraries/Components (Patch 2)
, and also adds the package level combined DSC to include them all (Patch 3).
If platform wants to use the flexible way to enable Network feature, it can
use the separate DSCs. If the platform wants to directly enable Network
feature, it can use the combined package DSC file.
This patch set is to update NetworkPkg only. If there is no objection on this
proposal, the following changes to platform DSC/FDF will be made and sent for
review. By default, the platform DSC/FDF will use the combined DSC/FDF file.
If the platform owner wants to use the flexible way to enable Network feature,
please reply this mail.
Liming Gao (3):
NetworkPkg DSC: Add the required ARM library to pass ARM build
NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
NetworkPkg: Add package level include DSC file
NetworkPkg/Network.dsc.inc | 30 +++++++++
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 +++++
NetworkPkg/NetworkPkg.dsc | 25 +------
7 files changed, 311 insertions(+), 22 deletions(-)
create mode 100644 NetworkPkg/Network.dsc.inc
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
--
2.13.0.windows.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Patch v3 1/3] NetworkPkg DSC: Add the required ARM library to pass ARM build
2019-04-25 12:37 [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg Liming Gao
@ 2019-04-25 12:37 ` Liming Gao
2019-04-26 22:04 ` [edk2-devel] " Laszlo Ersek
2019-04-25 12:37 ` [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg Liming Gao
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Liming Gao @ 2019-04-25 12:37 UTC (permalink / raw)
To: devel
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
NetworkPkg/NetworkPkg.dsc | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/NetworkPkg/NetworkPkg.dsc b/NetworkPkg/NetworkPkg.dsc
index 66d43bec12..955e45e84d 100644
--- a/NetworkPkg/NetworkPkg.dsc
+++ b/NetworkPkg/NetworkPkg.dsc
@@ -55,6 +55,7 @@
FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
+ NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
[LibraryClasses.common.UEFI_DRIVER]
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
@@ -72,6 +73,7 @@
# [LibraryClasses.ARM] and NULL mean link this library into all ARM images.
#
NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
+ ArmSoftFloatLib|ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf
[PcdsFeatureFlag]
gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable|TRUE
@@ -115,7 +117,7 @@
NetworkPkg/Application/IpsecConfig/IpSecConfig.inf
NetworkPkg/Application/VConfig/VConfig.inf
-[Components.IA32, Components.X64]
+[Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
NetworkPkg/IpSecDxe/IpSecDxe.inf
NetworkPkg/IScsiDxe/IScsiDxe.inf
NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
--
2.13.0.windows.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
2019-04-25 12:37 [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg Liming Gao
2019-04-25 12:37 ` [Patch v3 1/3] NetworkPkg DSC: Add the required ARM library to pass ARM build Liming Gao
@ 2019-04-25 12:37 ` Liming Gao
2019-04-29 13:05 ` [edk2-devel] " Laszlo Ersek
2019-04-25 12:37 ` [Patch v3 3/3] NetworkPkg: Add package level include DSC file Liming Gao
2019-04-29 13:10 ` [edk2-devel] [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg Laszlo Ersek
3 siblings, 1 reply; 18+ messages in thread
From: Liming Gao @ 2019-04-25 12:37 UTC (permalink / raw)
To: devel; +Cc: Jiaxin Wu, Ting Ye, Fu Siyuan
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_ISCSI_ENABLE = TRUE
DEFINE NETWORK_VLAN_ENABLE = TRUE
DEFINE PLATFORMX64_ENABLE = FALSE
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>
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"
+# 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>
+#
+# 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
+
+!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
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
+!endif
+
+!ifndef PLATFORMX64_ENABLE
+ #
+ # PLATFORMX64_ENABLE is set to TRUE when PEI is IA32 and DXE is X64 platform
+ #
+ DEFINE PLATFORMX64_ENABLE = FALSE
+!endif
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
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
--
2.13.0.windows.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Patch v3 3/3] NetworkPkg: Add package level include DSC file
2019-04-25 12:37 [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg Liming Gao
2019-04-25 12:37 ` [Patch v3 1/3] NetworkPkg DSC: Add the required ARM library to pass ARM build Liming Gao
2019-04-25 12:37 ` [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg Liming Gao
@ 2019-04-25 12:37 ` Liming Gao
2019-04-29 12:03 ` [edk2-devel] " Laszlo Ersek
2019-04-29 13:10 ` [edk2-devel] [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg Laszlo Ersek
3 siblings, 1 reply; 18+ messages in thread
From: Liming Gao @ 2019-04-25 12:37 UTC (permalink / raw)
To: devel
Platform DSC can include Network.dsc.inc to enable network features.
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
NetworkPkg/Network.dsc.inc | 30 ++++++++++++++++++++++++++++++
NetworkPkg/NetworkPkg.dsc | 23 +----------------------
2 files changed, 31 insertions(+), 22 deletions(-)
create mode 100644 NetworkPkg/Network.dsc.inc
diff --git a/NetworkPkg/Network.dsc.inc b/NetworkPkg/Network.dsc.inc
new file mode 100644
index 0000000000..5a808be4e5
--- /dev/null
+++ b/NetworkPkg/Network.dsc.inc
@@ -0,0 +1,30 @@
+## @file
+# Network DSC include file for Platform DSC
+#
+# This file includes all required information to enable Network features.
+# It can be included to a platform DSC file by using "!include NetworkPkg/Network.dsc.inc".
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+!include NetworkPkg/NetworkDefines.dsc.inc
+
+[PcdsFixedAtBuild]
+!include NetworkPkg/NetworkPcds.dsc.inc
+
+[LibraryClasses]
+!include NetworkPkg/NetworkLibs.dsc.inc
+
+!if $(PLATFORMX64_ENABLE) == TRUE
+[Components.X64]
+!include NetworkPkg/NetworkComponents.dsc.inc
+
+!else
+[Components.IA32, Components.X64, Components.ARM, Components.AARCH6]
+!include NetworkPkg/NetworkComponents.dsc.inc
+
+!endif
diff --git a/NetworkPkg/NetworkPkg.dsc b/NetworkPkg/NetworkPkg.dsc
index 955e45e84d..4cec3199ec 100644
--- a/NetworkPkg/NetworkPkg.dsc
+++ b/NetworkPkg/NetworkPkg.dsc
@@ -41,12 +41,6 @@
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
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
@@ -103,26 +97,11 @@
###################################################################################################
[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
NetworkPkg/WifiConnectionManagerDxe/WifiConnectionManagerDxe.inf
- NetworkPkg/Application/IpsecConfig/IpSecConfig.inf
NetworkPkg/Application/VConfig/VConfig.inf
-[Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
- NetworkPkg/IpSecDxe/IpSecDxe.inf
- NetworkPkg/IScsiDxe/IScsiDxe.inf
- NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
- NetworkPkg/TlsDxe/TlsDxe.inf
- NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
+ !include NetworkPkg/Network.dsc.inc
[BuildOptions]
*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
--
2.13.0.windows.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 1/3] NetworkPkg DSC: Add the required ARM library to pass ARM build
2019-04-25 12:37 ` [Patch v3 1/3] NetworkPkg DSC: Add the required ARM library to pass ARM build Liming Gao
@ 2019-04-26 22:04 ` Laszlo Ersek
2019-04-26 22:19 ` Ard Biesheuvel
0 siblings, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2019-04-26 22:04 UTC (permalink / raw)
To: devel, liming.gao, Leif Lindholm, Ard Biesheuvel
On 04/25/19 14:37, Liming Gao wrote:
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> ---
> NetworkPkg/NetworkPkg.dsc | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/NetworkPkg.dsc b/NetworkPkg/NetworkPkg.dsc
> index 66d43bec12..955e45e84d 100644
> --- a/NetworkPkg/NetworkPkg.dsc
> +++ b/NetworkPkg/NetworkPkg.dsc
> @@ -55,6 +55,7 @@
> FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
> FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
> SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
> + NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
"MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf" seems to be
ARM/AARCH64 only. Packages that are not inherently specific to
ARM/AARCH64 include this library resolution only in the following sections:
[LibraryClasses.ARM, LibraryClasses.AARCH64]
But the above hunk, from NetworkPkg.dsc, seems to fall under
[LibraryClasses]. I think that might break NetworkPkg.dsc builds on
IA32/X64.
>
> [LibraryClasses.common.UEFI_DRIVER]
> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> @@ -72,6 +73,7 @@
> # [LibraryClasses.ARM] and NULL mean link this library into all ARM images.
> #
> NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> + ArmSoftFloatLib|ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf
>
> [PcdsFeatureFlag]
> gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable|TRUE
I'll let Ard & Leif comment on this.
> @@ -115,7 +117,7 @@
> NetworkPkg/Application/IpsecConfig/IpSecConfig.inf
> NetworkPkg/Application/VConfig/VConfig.inf
>
> -[Components.IA32, Components.X64]
> +[Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
> NetworkPkg/IpSecDxe/IpSecDxe.inf
> NetworkPkg/IScsiDxe/IScsiDxe.inf
> NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
>
Shouldn't we just merge this section into [Components] above?
Or are these modules unsuitable for EBC?
Thanks
Laszlo
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 1/3] NetworkPkg DSC: Add the required ARM library to pass ARM build
2019-04-26 22:04 ` [edk2-devel] " Laszlo Ersek
@ 2019-04-26 22:19 ` Ard Biesheuvel
2019-04-29 2:02 ` Liming Gao
0 siblings, 1 reply; 18+ messages in thread
From: Ard Biesheuvel @ 2019-04-26 22:19 UTC (permalink / raw)
To: Laszlo Ersek, Kinney, Michael D
Cc: edk2-devel-groups-io, Gao, Liming, Leif Lindholm
On Sat, 27 Apr 2019 at 00:04, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 04/25/19 14:37, Liming Gao wrote:
> > Signed-off-by: Liming Gao <liming.gao@intel.com>
> > ---
> > NetworkPkg/NetworkPkg.dsc | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/NetworkPkg/NetworkPkg.dsc b/NetworkPkg/NetworkPkg.dsc
> > index 66d43bec12..955e45e84d 100644
> > --- a/NetworkPkg/NetworkPkg.dsc
> > +++ b/NetworkPkg/NetworkPkg.dsc
> > @@ -55,6 +55,7 @@
> > FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
> > FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
> > SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
> > + NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
>
> "MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf" seems to be
> ARM/AARCH64 only. Packages that are not inherently specific to
> ARM/AARCH64 include this library resolution only in the following sections:
>
> [LibraryClasses.ARM, LibraryClasses.AARCH64]
>
> But the above hunk, from NetworkPkg.dsc, seems to fall under
> [LibraryClasses]. I think that might break NetworkPkg.dsc builds on
> IA32/X64.
>
> >
> > [LibraryClasses.common.UEFI_DRIVER]
> > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> > @@ -72,6 +73,7 @@
> > # [LibraryClasses.ARM] and NULL mean link this library into all ARM images.
> > #
> > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> > + ArmSoftFloatLib|ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf
> >
> > [PcdsFeatureFlag]
> > gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable|TRUE
>
> I'll let Ard & Leif comment on this.
>
Sigh.
The RNG code in OpenSSL uses a 'double' to record the current entropy
level, even though the value is in the range [0 .. 31] (IIRC).
Unfortunately, this does imply that any .DSC that incorporates TLS or
other crypto code for ARM needs this resolution to be included.
>
> > @@ -115,7 +117,7 @@
> > NetworkPkg/Application/IpsecConfig/IpSecConfig.inf
> > NetworkPkg/Application/VConfig/VConfig.inf
> >
> > -[Components.IA32, Components.X64]
> > +[Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
> > NetworkPkg/IpSecDxe/IpSecDxe.inf
> > NetworkPkg/IScsiDxe/IScsiDxe.inf
> > NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> >
>
> Shouldn't we just merge this section into [Components] above?
>
> Or are these modules unsuitable for EBC?
>
(+ Mike)
This came up at the plugfest: is it really necessary to keep building
arbitrary modules using EBC? EBC is no longer mandatory, and the
compiler diagnostics of the other toolchains are much better than they
were 10+ years ago.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 1/3] NetworkPkg DSC: Add the required ARM library to pass ARM build
2019-04-26 22:19 ` Ard Biesheuvel
@ 2019-04-29 2:02 ` Liming Gao
0 siblings, 0 replies; 18+ messages in thread
From: Liming Gao @ 2019-04-29 2:02 UTC (permalink / raw)
To: Ard Biesheuvel, Laszlo Ersek, Kinney, Michael D
Cc: edk2-devel-groups-io, Leif Lindholm
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Saturday, April 27, 2019 6:19 AM
>To: Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D
><michael.d.kinney@intel.com>
>Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Gao, Liming
><liming.gao@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>
>Subject: Re: [edk2-devel] [Patch v3 1/3] NetworkPkg DSC: Add the required
>ARM library to pass ARM build
>
>On Sat, 27 Apr 2019 at 00:04, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 04/25/19 14:37, Liming Gao wrote:
>> > Signed-off-by: Liming Gao <liming.gao@intel.com>
>> > ---
>> > NetworkPkg/NetworkPkg.dsc | 4 +++-
>> > 1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/NetworkPkg/NetworkPkg.dsc b/NetworkPkg/NetworkPkg.dsc
>> > index 66d43bec12..955e45e84d 100644
>> > --- a/NetworkPkg/NetworkPkg.dsc
>> > +++ b/NetworkPkg/NetworkPkg.dsc
>> > @@ -55,6 +55,7 @@
>> > FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
>> >
>FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
>> > SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
>> > + NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
>>
>> "MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf" seems to be
>> ARM/AARCH64 only. Packages that are not inherently specific to
>> ARM/AARCH64 include this library resolution only in the following sections:
>>
>> [LibraryClasses.ARM, LibraryClasses.AARCH64]
>>
>> But the above hunk, from NetworkPkg.dsc, seems to fall under
>> [LibraryClasses]. I think that might break NetworkPkg.dsc builds on
>> IA32/X64.
Agree. I will move it to ARM arch section.
>>
>> >
>> > [LibraryClasses.common.UEFI_DRIVER]
>> > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>> > @@ -72,6 +73,7 @@
>> > # [LibraryClasses.ARM] and NULL mean link this library into all ARM
>images.
>> > #
>> > NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>> > + ArmSoftFloatLib|ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.inf
>> >
>> > [PcdsFeatureFlag]
>> > gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable|TRUE
>>
>> I'll let Ard & Leif comment on this.
>>
>
>Sigh.
>
>The RNG code in OpenSSL uses a 'double' to record the current entropy
>level, even though the value is in the range [0 .. 31] (IIRC).
>Unfortunately, this does imply that any .DSC that incorporates TLS or
>other crypto code for ARM needs this resolution to be included.
>
>
>>
>> > @@ -115,7 +117,7 @@
>> > NetworkPkg/Application/IpsecConfig/IpSecConfig.inf
>> > NetworkPkg/Application/VConfig/VConfig.inf
>> >
>> > -[Components.IA32, Components.X64]
>> > +[Components.IA32, Components.X64, Components.ARM,
>Components.AARCH64]
>> > NetworkPkg/IpSecDxe/IpSecDxe.inf
>> > NetworkPkg/IScsiDxe/IScsiDxe.inf
>> > NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
>> >
>>
>> Shouldn't we just merge this section into [Components] above?
>>
>> Or are these modules unsuitable for EBC?
>>
>
>(+ Mike)
>
>This came up at the plugfest: is it really necessary to keep building
>arbitrary modules using EBC? EBC is no longer mandatory, and the
>compiler diagnostics of the other toolchains are much better than they
>were 10+ years ago.
Now, those modules can't pass build on EBC, because of CryptoLib. I agree EBC compiler is too old. I agree to remove it.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 3/3] NetworkPkg: Add package level include DSC file
2019-04-25 12:37 ` [Patch v3 3/3] NetworkPkg: Add package level include DSC file Liming Gao
@ 2019-04-29 12:03 ` Laszlo Ersek
2019-04-29 14:15 ` Liming Gao
0 siblings, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2019-04-29 12:03 UTC (permalink / raw)
To: devel, liming.gao
On 04/25/19 14:37, Liming Gao wrote:
> Platform DSC can include Network.dsc.inc to enable network features.
>
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> ---
> NetworkPkg/Network.dsc.inc | 30 ++++++++++++++++++++++++++++++
> NetworkPkg/NetworkPkg.dsc | 23 +----------------------
> 2 files changed, 31 insertions(+), 22 deletions(-)
> create mode 100644 NetworkPkg/Network.dsc.inc
>
> diff --git a/NetworkPkg/Network.dsc.inc b/NetworkPkg/Network.dsc.inc
> new file mode 100644
> index 0000000000..5a808be4e5
> --- /dev/null
> +++ b/NetworkPkg/Network.dsc.inc
> @@ -0,0 +1,30 @@
> +## @file
> +# Network DSC include file for Platform DSC
> +#
> +# This file includes all required information to enable Network features.
> +# It can be included to a platform DSC file by using "!include NetworkPkg/Network.dsc.inc".
> +#
> +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +!include NetworkPkg/NetworkDefines.dsc.inc
> +
> +[PcdsFixedAtBuild]
> +!include NetworkPkg/NetworkPcds.dsc.inc
> +
> +[LibraryClasses]
> +!include NetworkPkg/NetworkLibs.dsc.inc
> +
> +!if $(PLATFORMX64_ENABLE) == TRUE
> +[Components.X64]
> +!include NetworkPkg/NetworkComponents.dsc.inc
> +
> +!else
> +[Components.IA32, Components.X64, Components.ARM, Components.AARCH6]
I'm only commenting on this patch because I had to look up
"PLATFORMX64_ENABLE" here, while reviewing "v3 2/3".
My observation is that "Components.AARCH6" has a typo -- regardless of
semantics (which I'm not attempting to review here), that should be
spelled "Components.AARCH64".
Thanks
Laszlo
> +!include NetworkPkg/NetworkComponents.dsc.inc
> +
> +!endif
> diff --git a/NetworkPkg/NetworkPkg.dsc b/NetworkPkg/NetworkPkg.dsc
> index 955e45e84d..4cec3199ec 100644
> --- a/NetworkPkg/NetworkPkg.dsc
> +++ b/NetworkPkg/NetworkPkg.dsc
> @@ -41,12 +41,6 @@
> 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
> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> @@ -103,26 +97,11 @@
> ###################################################################################################
>
> [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
> NetworkPkg/WifiConnectionManagerDxe/WifiConnectionManagerDxe.inf
>
> - NetworkPkg/Application/IpsecConfig/IpSecConfig.inf
> NetworkPkg/Application/VConfig/VConfig.inf
>
> -[Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
> - NetworkPkg/IpSecDxe/IpSecDxe.inf
> - NetworkPkg/IScsiDxe/IScsiDxe.inf
> - NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> - NetworkPkg/TlsDxe/TlsDxe.inf
> - NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> + !include NetworkPkg/Network.dsc.inc
>
> [BuildOptions]
> *_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
2019-04-25 12:37 ` [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg Liming Gao
@ 2019-04-29 13:05 ` Laszlo Ersek
2019-04-29 14:18 ` Liming Gao
2019-05-05 15:21 ` Liming Gao
0 siblings, 2 replies; 18+ messages in thread
From: Laszlo Ersek @ 2019-04-29 13:05 UTC (permalink / raw)
To: devel, liming.gao; +Cc: Jiaxin Wu, Ting Ye, Fu Siyuan
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg
2019-04-25 12:37 [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg Liming Gao
` (2 preceding siblings ...)
2019-04-25 12:37 ` [Patch v3 3/3] NetworkPkg: Add package level include DSC file Liming Gao
@ 2019-04-29 13:10 ` Laszlo Ersek
2019-04-29 14:19 ` Liming Gao
3 siblings, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2019-04-29 13:10 UTC (permalink / raw)
To: devel, liming.gao
On 04/25/19 14:37, Liming Gao wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1293
> BZ 1293 requests to move Network modules from MdeModulePkg to NetworkPkg.
> To keep the backword compatiblity, Network package level include DSC/FDF
> are introduced to be used in the platform DSC/FDF files. When Network
> modules are moved from MdeModulePkg to NetworkPkg, Network package level
> include DSC/FDF will be updated together. There is no impact on the platform
> DSC/FDF file.
>
> This patch set is to introduce network package level include DSC/FDF files.
> Bases on previous discussion and the existing usage case, build flag will be
> used to enable/disable the network features. PCD control feature way can be
> discussed later. And, to meet with the different usages, this patch set
> introduces the separate DSC for Defines/Pcds/Libraries/Components (Patch 2)
> , and also adds the package level combined DSC to include them all (Patch 3).
> If platform wants to use the flexible way to enable Network feature, it can
> use the separate DSCs. If the platform wants to directly enable Network
> feature, it can use the combined package DSC file.
>
> This patch set is to update NetworkPkg only. If there is no objection on this
> proposal, the following changes to platform DSC/FDF will be made and sent for
> review. By default, the platform DSC/FDF will use the combined DSC/FDF file.
> If the platform owner wants to use the flexible way to enable Network feature,
> please reply this mail.
Yes, I'd like both OvmfPkg and ArmVirtPkg platforms to use the
standalone include files from patch #2.
As I mentioned in my review of patch #2, the new flags should be
appropriate replacements for OvmfPkg and ArmVirtPkg, out of the box,
except for NETWORK_TLS_ENABLE.
- For ArmVirt, we're going to address TLS_ENABLE separately (see
<https://bugzilla.tianocore.org/show_bug.cgi?id=1009>).
- In OvmfPkg, we'll keep TLS_ENABLE, and set NETWORK_TLS_ENABLE to FALSE
(see the explanation in my review).
Thanks!
Laszlo
> Liming Gao (3):
> NetworkPkg DSC: Add the required ARM library to pass ARM build
> NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
> NetworkPkg: Add package level include DSC file
>
> NetworkPkg/Network.dsc.inc | 30 +++++++++
> 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 +++++
> NetworkPkg/NetworkPkg.dsc | 25 +------
> 7 files changed, 311 insertions(+), 22 deletions(-)
> create mode 100644 NetworkPkg/Network.dsc.inc
> 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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 3/3] NetworkPkg: Add package level include DSC file
2019-04-29 12:03 ` [edk2-devel] " Laszlo Ersek
@ 2019-04-29 14:15 ` Liming Gao
0 siblings, 0 replies; 18+ messages in thread
From: Liming Gao @ 2019-04-29 14:15 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Monday, April 29, 2019 8:04 PM
> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [Patch v3 3/3] NetworkPkg: Add package level include DSC file
>
> On 04/25/19 14:37, Liming Gao wrote:
> > Platform DSC can include Network.dsc.inc to enable network features.
> >
> > Signed-off-by: Liming Gao <liming.gao@intel.com>
> > ---
> > NetworkPkg/Network.dsc.inc | 30 ++++++++++++++++++++++++++++++
> > NetworkPkg/NetworkPkg.dsc | 23 +----------------------
> > 2 files changed, 31 insertions(+), 22 deletions(-)
> > create mode 100644 NetworkPkg/Network.dsc.inc
> >
> > diff --git a/NetworkPkg/Network.dsc.inc b/NetworkPkg/Network.dsc.inc
> > new file mode 100644
> > index 0000000000..5a808be4e5
> > --- /dev/null
> > +++ b/NetworkPkg/Network.dsc.inc
> > @@ -0,0 +1,30 @@
> > +## @file
> > +# Network DSC include file for Platform DSC
> > +#
> > +# This file includes all required information to enable Network features.
> > +# It can be included to a platform DSC file by using "!include NetworkPkg/Network.dsc.inc".
> > +#
> > +# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> > +#
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +##
> > +
> > +[Defines]
> > +!include NetworkPkg/NetworkDefines.dsc.inc
> > +
> > +[PcdsFixedAtBuild]
> > +!include NetworkPkg/NetworkPcds.dsc.inc
> > +
> > +[LibraryClasses]
> > +!include NetworkPkg/NetworkLibs.dsc.inc
> > +
> > +!if $(PLATFORMX64_ENABLE) == TRUE
> > +[Components.X64]
> > +!include NetworkPkg/NetworkComponents.dsc.inc
> > +
> > +!else
> > +[Components.IA32, Components.X64, Components.ARM, Components.AARCH6]
>
> I'm only commenting on this patch because I had to look up
> "PLATFORMX64_ENABLE" here, while reviewing "v3 2/3".
I will move PLATFORMX64_ENABLE flag in Network.dsc.inc [Defines] sections.
So, NetworkDefines.dsc.inc can be clean.
>
> My observation is that "Components.AARCH6" has a typo -- regardless of
> semantics (which I'm not attempting to review here), that should be
> spelled "Components.AARCH64".
Thanks for catch it. I will correct it in next version patch.
>
> Thanks
> Laszlo
>
> > +!include NetworkPkg/NetworkComponents.dsc.inc
> > +
> > +!endif
> > diff --git a/NetworkPkg/NetworkPkg.dsc b/NetworkPkg/NetworkPkg.dsc
> > index 955e45e84d..4cec3199ec 100644
> > --- a/NetworkPkg/NetworkPkg.dsc
> > +++ b/NetworkPkg/NetworkPkg.dsc
> > @@ -41,12 +41,6 @@
> > 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
> > BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> > IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> > @@ -103,26 +97,11 @@
> > ###################################################################################################
> >
> > [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
> > NetworkPkg/WifiConnectionManagerDxe/WifiConnectionManagerDxe.inf
> >
> > - NetworkPkg/Application/IpsecConfig/IpSecConfig.inf
> > NetworkPkg/Application/VConfig/VConfig.inf
> >
> > -[Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
> > - NetworkPkg/IpSecDxe/IpSecDxe.inf
> > - NetworkPkg/IScsiDxe/IScsiDxe.inf
> > - NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> > - NetworkPkg/TlsDxe/TlsDxe.inf
> > - NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> > + !include NetworkPkg/Network.dsc.inc
> >
> > [BuildOptions]
> > *_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
> >
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
2019-04-29 13:05 ` [edk2-devel] " Laszlo Ersek
@ 2019-04-29 14:18 ` Liming Gao
2019-05-05 15:21 ` Liming Gao
1 sibling, 0 replies; 18+ messages in thread
From: Liming Gao @ 2019-04-29 14:18 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Wu, Jiaxin, Ye, Ting, Fu, Siyuan
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, April 29, 2019 9:05 PM
> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
>
> 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.)
>
Agree.
> >
> > 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.
>
I will move this flag into Patch (v3 3/3)
> >
> > 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
>
Yes. I will try to catch 201905 release. I will update releasing planning and notes.
> >
> > 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg
2019-04-29 13:10 ` [edk2-devel] [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg Laszlo Ersek
@ 2019-04-29 14:19 ` Liming Gao
0 siblings, 0 replies; 18+ messages in thread
From: Liming Gao @ 2019-04-29 14:19 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Monday, April 29, 2019 9:11 PM
> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg
>
> On 04/25/19 14:37, Liming Gao wrote:
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1293
> > BZ 1293 requests to move Network modules from MdeModulePkg to NetworkPkg.
> > To keep the backword compatiblity, Network package level include DSC/FDF
> > are introduced to be used in the platform DSC/FDF files. When Network
> > modules are moved from MdeModulePkg to NetworkPkg, Network package level
> > include DSC/FDF will be updated together. There is no impact on the platform
> > DSC/FDF file.
> >
> > This patch set is to introduce network package level include DSC/FDF files.
> > Bases on previous discussion and the existing usage case, build flag will be
> > used to enable/disable the network features. PCD control feature way can be
> > discussed later. And, to meet with the different usages, this patch set
> > introduces the separate DSC for Defines/Pcds/Libraries/Components (Patch 2)
> > , and also adds the package level combined DSC to include them all (Patch 3).
> > If platform wants to use the flexible way to enable Network feature, it can
> > use the separate DSCs. If the platform wants to directly enable Network
> > feature, it can use the combined package DSC file.
> >
> > This patch set is to update NetworkPkg only. If there is no objection on this
> > proposal, the following changes to platform DSC/FDF will be made and sent for
> > review. By default, the platform DSC/FDF will use the combined DSC/FDF file.
> > If the platform owner wants to use the flexible way to enable Network feature,
> > please reply this mail.
>
> Yes, I'd like both OvmfPkg and ArmVirtPkg platforms to use the
> standalone include files from patch #2.
Thanks for your message. I will take it.
>
> As I mentioned in my review of patch #2, the new flags should be
> appropriate replacements for OvmfPkg and ArmVirtPkg, out of the box,
> except for NETWORK_TLS_ENABLE.
OK. I will replace other except for NETWORK_TLS_ENABLE.
>
> - For ArmVirt, we're going to address TLS_ENABLE separately (see
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1009>).
>
> - In OvmfPkg, we'll keep TLS_ENABLE, and set NETWORK_TLS_ENABLE to FALSE
> (see the explanation in my review).
>
> Thanks!
> Laszlo
>
> > Liming Gao (3):
> > NetworkPkg DSC: Add the required ARM library to pass ARM build
> > NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
> > NetworkPkg: Add package level include DSC file
> >
> > NetworkPkg/Network.dsc.inc | 30 +++++++++
> > 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 +++++
> > NetworkPkg/NetworkPkg.dsc | 25 +------
> > 7 files changed, 311 insertions(+), 22 deletions(-)
> > create mode 100644 NetworkPkg/Network.dsc.inc
> > 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
> >
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
2019-04-29 13:05 ` [edk2-devel] " Laszlo Ersek
2019-04-29 14:18 ` Liming Gao
@ 2019-05-05 15:21 ` Liming Gao
2019-05-06 18:23 ` Laszlo Ersek
2019-05-07 1:22 ` Siyuan, Fu
1 sibling, 2 replies; 18+ messages in thread
From: Liming Gao @ 2019-05-05 15:21 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Wu, Jiaxin, Ye, Ting, Fu, Siyuan
Reply for the comments in the patch content.
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, April 29, 2019 9:05 PM
> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
>
> 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.
OK.
>
> > +# 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.)
I reuse this patch sent last year. I will update it to 2019 in next version.
>
> > +#
> > +# 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.)
>
This is wrong. I will fix it in next version.
>
> (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.)
Yes. ARP is IPv4 only. NetworkPkg maintain can help confirm it.
>
> > +
> > +!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.
After include NetworkPkg/NetworkComponents.dsc.inc, you can override TlsAuthConfigDxe.inf
with the below to match Ovmf usage.
NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
<LibraryClasses>
NULL|OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
}
>
> > 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.
TlsAuthConfigDxe can link the different library instance to override the one in Network common DSC.
So, NETWORK_TLS_ENABLE can be set to TRUE.
>
> (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.
>
We can decide when it happen.
> > +!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].)
It doesn't cover IA32, X64, ARM and AARCH64 for NetworkPkg.dsc
>
> > 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.
The library instance will be built if it is consumed by any driver.
If no driver consumes the library, this library will not be built.
So, they are specified without the build flag. I agree to add the comment for them.
>
> (11) I think the following lib class resolution is missing:
>
> TlsLib|CryptoPkg/Library/TlsLib/TlsLib.inf
>
NetworkPkg/NetworkLibs.dsc.inc includes the network specific library instances.
If TlsLib is designed only for network, I agree to add it.
>
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
2019-05-05 15:21 ` Liming Gao
@ 2019-05-06 18:23 ` Laszlo Ersek
2019-05-06 18:49 ` Andrew Fish
2019-05-07 1:22 ` Siyuan, Fu
1 sibling, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2019-05-06 18:23 UTC (permalink / raw)
To: devel, liming.gao; +Cc: Wu, Jiaxin, Ye, Ting, Fu, Siyuan
On 05/05/19 17:21, Liming Gao wrote:
> Reply for the comments in the patch content.
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, April 29, 2019 9:05 PM
>> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
>> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
>> Subject: Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
>>
>> On 04/25/19 14:37, Liming Gao wrote:
[...]
>>> 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.
>
> After include NetworkPkg/NetworkComponents.dsc.inc, you can override TlsAuthConfigDxe.inf
> with the below to match Ovmf usage.
> NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
> <LibraryClasses>
> NULL|OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
> }
Oh, that's very interesting. Is this a documented feature of the DSC
files (from the DSC spec), or just something that happens to work with
BaseTools?
In other words, are DSC files officially permitted to reference the same
component INF file multiple times, and the last reference will take
effect (including PCD and lib overrides)?
(And thank you for the rest of the answers as well.)
Cheers,
Laszlo
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
2019-05-06 18:23 ` Laszlo Ersek
@ 2019-05-06 18:49 ` Andrew Fish
2019-05-07 4:23 ` Liming Gao
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Fish @ 2019-05-06 18:49 UTC (permalink / raw)
To: devel, Laszlo Ersek; +Cc: liming.gao, Wu, Jiaxin, Ye, Ting, Fu, Siyuan
> On May 6, 2019, at 11:23 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 05/05/19 17:21, Liming Gao wrote:
>> Reply for the comments in the patch content.
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Monday, April 29, 2019 9:05 PM
>>> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
>>> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
>>> Subject: Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
>>>
>>> On 04/25/19 14:37, Liming Gao wrote:
>
> [...]
>
>>>> 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.
>>
>> After include NetworkPkg/NetworkComponents.dsc.inc, you can override TlsAuthConfigDxe.inf
>> with the below to match Ovmf usage.
>> NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
>> <LibraryClasses>
>> NULL|OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
>> }
>
> Oh, that's very interesting. Is this a documented feature of the DSC
> files (from the DSC spec), or just something that happens to work with
> BaseTools?
>
> In other words, are DSC files officially permitted to reference the same
> component INF file multiple times, and the last reference will take
> effect (including PCD and lib overrides)?
>
Laszlo,
Seems like this behavior would be good to define if it is not documented. I would expect an error (multiple definition), or a warning (last one winning).
For best compatibility promoting the current behavior, assuming it makes sense, is probably the way to go.
Thanks,
Andrew Fish
>
> (And thank you for the rest of the answers as well.)
>
> Cheers,
> Laszlo
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
2019-05-05 15:21 ` Liming Gao
2019-05-06 18:23 ` Laszlo Ersek
@ 2019-05-07 1:22 ` Siyuan, Fu
1 sibling, 0 replies; 18+ messages in thread
From: Siyuan, Fu @ 2019-05-07 1:22 UTC (permalink / raw)
To: Gao, Liming, Laszlo Ersek, devel@edk2.groups.io; +Cc: Wu, Jiaxin, Ye, Ting
Comments for Laszlo's question (11).
> -----Original Message-----
> From: Gao, Liming
> Sent: Sunday, May 5, 2019 11:22 PM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Ye, Ting <ting.ye@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Subject: RE: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include
> segment files to NetworkPkg.
>
> Reply for the comments in the patch content.
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Monday, April 29, 2019 9:05 PM
> > To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
> > Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Ye, Ting <ting.ye@intel.com>; Fu,
> Siyuan <siyuan.fu@intel.com>
> > Subject: Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include
> segment files to NetworkPkg.
> >
> > 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.
> > >
[...]
> >
> > (11) I think the following lib class resolution is missing:
> >
> > TlsLib|CryptoPkg/Library/TlsLib/TlsLib.inf
> >
>
> NetworkPkg/NetworkLibs.dsc.inc includes the network specific library instances.
> If TlsLib is designed only for network, I agree to add it.
The TlsLib is a generic library not only for network stack. Only TlsAuthConfigDxe
and TlsDxe in NetworkPkg are network specific.
>
> >
> > 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
2019-05-06 18:49 ` Andrew Fish
@ 2019-05-07 4:23 ` Liming Gao
0 siblings, 0 replies; 18+ messages in thread
From: Liming Gao @ 2019-05-07 4:23 UTC (permalink / raw)
To: afish@apple.com, devel@edk2.groups.io, Laszlo Ersek
Cc: Wu, Jiaxin, Ye, Ting, Fu, Siyuan
> -----Original Message-----
> From: afish@apple.com [mailto:afish@apple.com]
> Sent: Tuesday, May 7, 2019 2:49 AM
> To: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>
> Cc: Gao, Liming <liming.gao@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Ye, Ting <ting.ye@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Subject: Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
>
>
>
> > On May 6, 2019, at 11:23 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > On 05/05/19 17:21, Liming Gao wrote:
> >> Reply for the comments in the patch content.
> >>> -----Original Message-----
> >>> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >>> Sent: Monday, April 29, 2019 9:05 PM
> >>> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>
> >>> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> >>> Subject: Re: [edk2-devel] [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.
> >>>
> >>> On 04/25/19 14:37, Liming Gao wrote:
> >
> > [...]
> >
> >>>> 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.
> >>
> >> After include NetworkPkg/NetworkComponents.dsc.inc, you can override TlsAuthConfigDxe.inf
> >> with the below to match Ovmf usage.
> >> NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
> >> <LibraryClasses>
> >> NULL|OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
> >> }
> >
> > Oh, that's very interesting. Is this a documented feature of the DSC
> > files (from the DSC spec), or just something that happens to work with
> > BaseTools?
> >
> > In other words, are DSC files officially permitted to reference the same
> > component INF file multiple times, and the last reference will take
> > effect (including PCD and lib overrides)?
> >
BZ https://bugzilla.tianocore.org/show_bug.cgi?id=1449 for this support. It is in edk2 201903 stable tag.
I will add this change in edk2 201903 notes. Bob will update build spec for this change.
>
> Laszlo,
>
> Seems like this behavior would be good to define if it is not documented. I would expect an error (multiple definition), or a warning (last
> one winning).
>
> For best compatibility promoting the current behavior, assuming it makes sense, is probably the way to go.
>
> Thanks,
>
> Andrew Fish
>
>
> >
> > (And thank you for the rest of the answers as well.)
> >
> > Cheers,
> > Laszlo
> >
> >
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-05-07 4:23 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-04-25 12:37 [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg Liming Gao
2019-04-25 12:37 ` [Patch v3 1/3] NetworkPkg DSC: Add the required ARM library to pass ARM build Liming Gao
2019-04-26 22:04 ` [edk2-devel] " Laszlo Ersek
2019-04-26 22:19 ` Ard Biesheuvel
2019-04-29 2:02 ` Liming Gao
2019-04-25 12:37 ` [Patch v3 2/3] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg Liming Gao
2019-04-29 13:05 ` [edk2-devel] " Laszlo Ersek
2019-04-29 14:18 ` Liming Gao
2019-05-05 15:21 ` Liming Gao
2019-05-06 18:23 ` Laszlo Ersek
2019-05-06 18:49 ` Andrew Fish
2019-05-07 4:23 ` Liming Gao
2019-05-07 1:22 ` Siyuan, Fu
2019-04-25 12:37 ` [Patch v3 3/3] NetworkPkg: Add package level include DSC file Liming Gao
2019-04-29 12:03 ` [edk2-devel] " Laszlo Ersek
2019-04-29 14:15 ` Liming Gao
2019-04-29 13:10 ` [edk2-devel] [Patch v3 0/3] Add package level include DSC/FDF in NetworkPkg Laszlo Ersek
2019-04-29 14:19 ` Liming Gao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox