Re: [PATCH v2 0/6] Add DSC/FDF include segment files for network stack

["Gao, Liming" <liming.gao@intel.com>]

Laszlo:
  
Thanks
Liming
>-----Original Message-----
>From: Laszlo Ersek [mailto:lersek@redhat.com]
>Sent: Saturday, November 24, 2018 2:36 AM
>To: Gao, Liming <liming.gao@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>;
>edk2-devel@lists.01.org
>Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Ye, Ting <ting.ye@intel.com>; Justen,
>Jordan L <jordan.l.justen@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Wu,
>Jiaxin <jiaxin.wu@intel.com>; Anthony Perard <anthony.perard@citrix.com>;
>Wei, David <david.wei@intel.com>
>Subject: Re: [edk2] [PATCH v2 0/6] Add DSC/FDF include segment files for
>network stack
>
>On 11/23/18 17:02, Gao, Liming wrote:
>> Laszlo:
>>   I add my comments.
>>
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Thursday, November 22, 2018 11:48 PM
>>> To: Gao, Liming <liming.gao@intel.com>; Fu, Siyuan
><siyuan.fu@intel.com>; edk2-devel@lists.01.org
>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Ye, Ting <ting.ye@intel.com>; Justen,
>Jordan L <jordan.l.justen@intel.com>; Wu, Hao A
>>> <hao.a.wu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Anthony
>Perard <anthony.perard@citrix.com>; Wei, David
>>> <david.wei@intel.com>
>>> Subject: Re: [edk2] [PATCH v2 0/6] Add DSC/FDF include segment files for
>network stack
>>>
>>> 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.
>> If BitMask is not good for this case, BOOLEAN type FixedAtBuildPcd can be
>defined to match current macro one by one.
>> For example, gEfiNetworkPkgTokenSpaceGuid.PcdNetworkEnable is added
>to map macro NETWORK_ENABLE.
>
>I agree that this is technically doable, and that the results (regarding
>the build script syntax) are very similar.
>
>But, how is this an improvement over the current -D flags?
>
PCD has been used to configure the firmware setting. It can also be used for the firmware build configuration. 
I would like to introduce PCD as the same configuration way. If so,  new users don't need to remember other syntax

>>
>>>
>>>> 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.
>> This is not TRUE. Dynamic PCD has the different value format, such as
>DynamicHii PCD.
>
>I think DynamicHII is the only exception here, the rest can be described
>with
>
>  TokenSpaceGuid.PcdName|Value
>
>This format should work in [PcdsDynamicDefault], [PcdsFixedAtBuild], and
>[PcdsPatchableInModule] sections too. So platforms should have the
>freedom to choose.
>
I want to discuss the general rules on how to share the package configuration 
in the different platform DSC/FDF. NetworkPkg is the first case. After it, 
other package maintainer may provide his package setting with the similar style. 
In my idea, each package has one *Pcds.dsc.inc, *Libs.dsc.inc, 
*ComponentsPei.dsc.inc and *ComponentsDxe.dsc.inc. Normal platform 
developers just need to include them in the platform DSC. Advanced platform
user may modify Pcd type/value and Library class selection in platform DSC.
Its purpose is to simply the platform DSC. I agree this style will not have the 
same flexibility. But, to keep the flexibility, every PCD may have its Pcds.dsc.inc, 
because the different PCD may be configured to the different type. 
NetworkPkg provides one only PCD. So, it has one Pcds.dsc.inc. If other package 
provides 3 PCDs, there will be three *Pcds.dsc.inc files. If so, I don't think we
need to add those *Pcds.dsc.inc. User can directly add Pcd in Platform DSC file 
instead of include *Pcds.dsc.inc file. 

>In the other case, if the DSC include file contains the section header
>as well, then the platform loses this choice. The concrete example was
>that NetworkPkg.dec declares "PcdAllowHttpConnections" as both
>[PcdsFixedAtBuild] and [PcdsPatchableInModule], but in v1, the DSC
>include file restricted it to [PcdsFixedAtBuild] only. IMO, if the DEC
>file permits several types, then the DSC include file should restrict
>those types as little as possible.
>
>> Without section header, DynamicHii PCD can't be specified together with
>FixedAtBuild Pcd.
>
>Indeed, but it's still better to lose only DynamicHii (when using a DSC
>include file) than to lose every type permitted by the DEC file, except
>the one type picked by the DSC include file.
>
>> Package level *.pcd.inc provides the recommended type and setting.
>> Platform can override Pcd value and type In their DSC file. BaseTools
>supports PCD value override,
>> doesn't support PCD type override. This can be supported.
>
>Then I don't think that introducing a separate -D flag (or a separate
>fixed-at-build boolean PCD) for controlling the default value of
>"PcdAllowHttpConnections" buys us much.
>
For this case, PcdAllowHttpConnections can be used to control the build configuration and firmware functionality. 
We don't need to introduce the additional MACRO for it. This is another benefit of PCD. 
>>>> 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"?
>>>
>> I miss not.
>
>Wait, now I am even more confused. Did you mean that you did *not* miss
>the word "not", or that you missed it?
>
I lost 'not' in previous statement. 
Previous: Without section header, they can be placed into one *Libs.dsc.inc file.
Correct:  Without section header, they can't be placed into one *Libs.dsc.inc file.
>> I mean without section header, they can't be placed into one *Libs.dsc.inc
>file.
>> Below is one example. One Libs.dsc.inc can list the different library instance
>for the different module type.
>>
>> [LibraryClasses]
>>   TimerLib|PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>> [LibraryClasses.common.SEC]
>>   TimerLib|PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
>> [LibraryClasses.common.PEI_CORE, LibraryClasses.common.PEIM]
>>   TimerLib|PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.inf
>
>I understand the example, but this is a totally different use case, from
>the NetworkPkg libraries. For the TimerLib class, it is expected (by
>design) that different firmware phases / module types use different
>library instances.
>
>That's not the case for the six NetworkPkg lib classes in question. Each
>of those classes has a single instance, and there's no reason for
>implementing other instances. Platforms have no need to customize them,
>and they are used by DXE phase (and later), boot time only, modules
>anyway. They only exist as library classes for better code structuring /
>code sharing, not for customization.
>
>So the actual class -> instance resolutions will never change. What may
>change however is how widely a platform wants to employ these
>resolutions. Maybe DXE_DRIVER and UEFI_DRIVER modules only. Maybe
>UEFI_APPLICATION too. Maybe X64 only. Maybe IA32 and X64 both. And so
>on.
>
>If we put the section headers in the DSC include file, then the platform
>loses this customization possibility.
NetworkPkg case is simple. Those six library instances are for the same module type and ARCHs. 
I don't see the different usage in the platform DSC. Similar to PCD, to keep the flexibility, every
Library instance may have its *.Libs.dsc.inc. If so, it is not necessary to add *.Libs.dsc.inc.
>>
>>>> [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
>>
>> After define FixedAtBuild BOOLEAN type PCD, build command can be
>specified as below.
>> --pcd supports PcdName without PcdTokenSpaceGuid only if PcdName has
>no confliction.
>>
>>    build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -t GCC48 -b NOOPT \
>>      -D SECURE_BOOT_ENABLE -D SMM_REQUIRE \
>>      --pcd PcdNetworkEnable=TRUE
>
>Yes, I agree that this reads nicely, and it is easy to grep for.
>
>But, again, what does it buy us over "-D"?
>
>Thanks!
>Laszlo