From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 813FF2118F366 for ; Thu, 22 Nov 2018 07:48:16 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E76203001BF9; Thu, 22 Nov 2018 15:48:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-200.rdu2.redhat.com [10.10.120.200]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4286A608E7; Thu, 22 Nov 2018 15:48:13 +0000 (UTC) To: "Gao, Liming" , "Fu, Siyuan" , "edk2-devel@lists.01.org" Cc: "Ni, Ruiyu" , "Ye, Ting" , "Justen, Jordan L" , "Wu, Hao A" , "Wu, Jiaxin" , Anthony Perard , "Wei, David" References: <20181122052153.89464-1-siyuan.fu@intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E36FFDD@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <5d2be195-63c4-f0d6-7102-22f0da2d1464@redhat.com> Date: Thu, 22 Nov 2018 16:48:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E36FFDD@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Thu, 22 Nov 2018 15:48:16 +0000 (UTC) Subject: Re: [PATCH v2 0/6] Add DSC/FDF include segment files for network stack X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Nov 2018 15:48:16 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/22/18 07:14, Gao, Liming wrote: > Siyuan: > Thanks for your contribution. I really like this idea to share the > common DSC/FDF between all platform DSC/FDFs. Now, FixedAtBuild PCD > can also be used in the conditional statement. Its value can be > specified by build command with --pcd option. So, I suggest to use > FixedAtBuild PCD for network feature instead of MACRO. I would like > to recommend to use PCD both for the build and firmware > configuration. For this case, one UINT16 FixedAtBuildPcd can be > introduced in NetworkPkg.dec. Its different bit will be for the > different network features. Below is the example. I disagree. A bitmask is a very good representation for *computers*, to handle a set of features. On the other hand, a bitmask is a catastrophically bad representation for *humans*, when they are trying to configure a platform build (that is, writing a build command line), selecting the firmware features they want. It is hard to compute, and it is extremely hard to grep for. It is trivial to grep a build script for "NETWORK_TLS_ENABLE". It is much harder to grep the same script for PcdNetworkFeatureMask, and then check whether bit#4 is set in the value. > Besides, I think *.dsc.inc files need to include section header. If > so, *.dsc.inc is the standalone dsc file. It can easily be included in > platform DSC file. I disagree. For a platform, spelling out the section header is not a big burden, but it provides a lot of flexibility. For example, it can restrict the scope of the included text (e.g., component list) to a specific architecture only. For another example, the platform's section header can specify the flavor of the PCDs for which the included text provides the default values (fixed PCD, patchable PCD, even dynamic(ex) PCD). Standalone DSC file is the wrong thing to aim for, in this instance -- in my opinion anyway. Yes, it appears simpler, but it loses too much flexibility. In my earlier comments I pointed out that the "allow plaintext HTTP connections" PCD was declared in NetworkPkg.dec as either fixed or patchable. The dsc.inc file should not squander that flexibility and dictate fixed only. > Especially for library instance, the different library instance may be > for the different module type. Without section header, they can be > placed into one *Libs.dsc.inc file. Ugh... I'm now condfused. Now it seems that you and I are asking for the same thing actually. Did you perhaps mean, above, that "*.dsc.inc files need *NOT* include section header"? > [Defines] > DEFINE NETWORK_ENABLE = 0x1 > DEFINE NETWORK_SNP_ENABLE = 0x2 > DEFINE NETWORK_IP4_ENABLE = 0x4 > DEFINE NETWORK_IP6_ENABLE = 0x8 > DEFINE NETWORK_TLS_ENABLE = 0x10 > DEFINE NETWORK_HTTP_BOOT_ENABLE = 0x20 > DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = 0x40 > DEFINE NETWORK_IPSEC_ENABLE = 0x80 > DEFINE NETWORK_ISCSI_ENABLE = 0x100 > DEFINE NETWORK_VLAN_ENABLE = 0x200 > > [PcdsFixedAtBuild] > gEfiNetworkPkgTokenSpaceGuid.PcdNetworkFeatureMask|0xFFFF > > [PcdsFixedAtBuild] > !if gEfiNetworkPkgTokenSpaceGuid.PcdNetworkFeatureMask & NETWORK_ALLOW_HTTP_CONNECTIONS == NETWORK_ALLOW_HTTP_CONNECTIONS > gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE > !endif Sure, this looks good in the DSC include file, but it looks very bad on the build command line. Compare: build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -t GCC48 -b NOOPT \ -D SECURE_BOOT_ENABLE -D SMM_REQUIRE \ -D NETWORK_ENABLE -D NETWORK_IP4_ENABLE \ -D NETWORK_HTTP_BOOT_ENABLE -D NETWORK_TLS_ENABLE versus build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -t GCC48 -b NOOPT \ -D SECURE_BOOT_ENABLE -D SMM_REQUIRE \ --pcd gEfiNetworkPkgTokenSpaceGuid.PcdNetworkFeatureMask=0x0035 First, composing the second command line takes at least four times as long for me (calculating the value 0x35). Second, while grep NETWORK_IP4_ENABLE will easily match the first command (in a shell script or similar), the same search would require an awkward, ad-hoc parser for matching the second command. Thanks Laszlo >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fu >> Siyuan >> Sent: Thursday, November 22, 2018 1:22 PM >> To: edk2-devel@lists.01.org >> Cc: Ni, Ruiyu ; Ye, Ting ; Justen, >> Jordan L ; Wu, Hao A ; Wu, >> Jiaxin ; Anthony Perard ; >> Laszlo Ersek ; Wei, David >> Subject: [edk2] [PATCH v2 0/6] Add DSC/FDF include segment files for >> network stack >> >> There is a patch to remove the redudant IP4 only iSCSI/PXE/TCP drivers >>from MdeModulePkg, which has been reviewed before edk2-stable201811 >> tag. >> And we also have plan to move all network related libraries/modules to >> NetworkPkg. In order to make these change more smoothly, a set of >> fragment >> files (2 for DSC and 1 for FDF) are provided for platform to enable the >> network stack support, without directly reference the INF module path. >> >> Patch 1/6 adds centralized dsc/fdf include files to NetworkPkg, with >> a set of flags for feature set enable/disable. >> Patch 2~6 updates edk2 platform dsc/fdf files to use the new include >> files, instead of reference the module INF. >> >> v2: >> 1. Split the "Network.dsc.inc" in to 4 files for different sections in DSC >> file. This could provide more flexibility to platform owner to use the >> include files. >> 2. Clarify the OpenSSL dependency of TLS, iSCSI and IPsec enable flag. >> 3. Use "!error" statement for incorrect flag value check. >> 4. Update platform DSC/FDF to use the new include files. >> 5. Other decoration work according to Laszlo's comments. >> >> >> >> Fu Siyuan (6): >> NetworkPkg: Add DSC/FDF include segment files to NetworkPkg. >> Nt32Pkg: Update DSC/FDF to use NetworkPkg's include fragment file. >> ArmVirtPkg: Update DSC/FDF to use NetworkPkg's include fragment file. >> EmulatorPkg: Update DSC/FDF to use NetworkPkg's include fragment file. >> OvmfPkg: Update DSC/FDF to use NetworkPkg's include fragment file. >> Vlv2TbltDevicePkg: Update DSC/FDF to use NetworkPkg's include fragment >> file. >> >> ArmVirtPkg/ArmVirt.dsc.inc | 11 +- >> ArmVirtPkg/ArmVirtQemu.dsc | 46 ++----- >> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 28 +--- >> ArmVirtPkg/ArmVirtQemuKernel.dsc | 46 ++----- >> EmulatorPkg/EmulatorPkg.dsc | 39 ++++-- >> EmulatorPkg/EmulatorPkg.fdf | 10 +- >> NetworkPkg/Network.fdf.inc | 69 ++++++++++ >> NetworkPkg/NetworkComponents.dsc.inc | 71 ++++++++++ >> NetworkPkg/NetworkDefines.dsc.inc | 138 ++++++++++++++++++++ >> NetworkPkg/NetworkLibs.dsc.inc | 25 ++++ >> NetworkPkg/NetworkPcds.dsc.inc | 22 ++++ >> NetworkPkg/NetworkPkg.dsc | 28 +--- >> Nt32Pkg/Nt32Pkg.dsc | 104 ++++----------- >> Nt32Pkg/Nt32Pkg.fdf | 27 +--- >> OvmfPkg/OvmfPkgIa32.dsc | 75 +++++------ >> OvmfPkg/OvmfPkgIa32.fdf | 27 +--- >> OvmfPkg/OvmfPkgIa32X64.dsc | 76 +++++------ >> OvmfPkg/OvmfPkgIa32X64.fdf | 27 +--- >> OvmfPkg/OvmfPkgX64.dsc | 75 +++++------ >> OvmfPkg/OvmfPkgX64.fdf | 27 +--- >> Vlv2TbltDevicePkg/PlatformPkg.fdf | 25 +--- >> Vlv2TbltDevicePkg/PlatformPkgConfig.dsc | 11 +- >> Vlv2TbltDevicePkg/PlatformPkgGcc.fdf | 25 +--- >> Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc | 52 +++----- >> Vlv2TbltDevicePkg/PlatformPkgIA32.dsc | 52 +++----- >> Vlv2TbltDevicePkg/PlatformPkgX64.dsc | 52 +++----- >> 26 files changed, 573 insertions(+), 615 deletions(-) >> 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 >> >> Cc: Jiaxin Wu >> Cc: Ting Ye >> Cc: Ruiyu Ni >> Cc: Hao Wu >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Cc: Julien Grall >> Cc: Jordan Justen >> Cc: Andrew Fish >> Cc: Ruiyu Ni >> Cc: Anthony Perard >> Cc: David Wei >> Cc: Mang Guo >> >> -- >> 2.19.1.windows.1 >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel