Re: [PATCH 1/6] NetworkPkg: Add DSC/FDF include segment files to NetworkPkg.

[Laszlo Ersek <lersek@redhat.com>]

On 11/21/18 12:53, Fu, Siyuan wrote:
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, November 21, 2018 6:47 PM
>> To: Fu, Siyuan <siyuan.fu@intel.com>
>> Cc: edk2-devel@lists.01.org; Ye, Ting <ting.ye@intel.com>; Wu, Jiaxin
>> <jiaxin.wu@intel.com>
>> Subject: Re: [edk2] [PATCH 1/6] NetworkPkg: Add DSC/FDF include segment
>> files to NetworkPkg.

[...]

>>> +[PcdsFixedAtBuild]
>>> +!if $(NETWORK_ALLOW_HTTP_CONNECTIONS) == TRUE
>>> +  gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE
>>> +!endif
>>
>> (11) I'm not sure this is flexible enough.
>>
>> First, in "OvmfPkg/OvmfPkgIa32X64.dsc", we set the PCD only under
>> [PcdsFixedAtBuild.X64], not under [PcdsFixedAtBuild]. I agree that in
>> practice, such a change shouldn't be a problem however.
>>
>> Second, a more practical observation: NetworkPkg.dec declares this PCD
>> not just as fixed, but also as patchable-in-module. As far as I
>> understand, the above DSC include hunk will prevent platforms from using
>> the PCD as patchable.
>>
>> I think the most flexible option would be to simply remove the
>> NETWORK_ALLOW_HTTP_CONNECTIONS build flag, from this patch, and to allow
>> platforms to set the PCD however they want. A build macro ("-D") is not
>> expressive enough for this. Also remember that "--pcd" can be passed on
>> the build command line too, so not much usability/convenience is lost by
>> removing NETWORK_ALLOW_HTTP_CONNECTIONS.
> 
> I'm OK to remove this flag.
> 
>>
>>> +
>>> +[Components]
>>
>> (12) How is this going to work with multi-arch platform builds, such as
>> "OvmfPkg/OvmfPkgIa32X64.dsc", where the PEI phase is 32-bit, and the DXE
>> phase is 64-bit?
>>
>> I don't think "OvmfPkgIa32X64.dsc" should build the networking modules
>> for 32-bit too. They would never be included in the final flash device,
>> so it's wasted compilation.
>>
>> Can we introduce separate DSC include files (fragments) for each of the
>> DSC file sections? That is, we could have:
>>
>> - 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).
>>
>> Then the platform DSC would be responsible for spelling out the precise
>> section header it wants, and then include the matching DSC include file
>> right below that.
>>
>> In other words, can we split this DSC include into multiple files, at
>> the currently shown section headers, and remove the section headers
>> themselves?
> 
> It's quite a good suggestion.
> 
> My initial intention is to make the include file as simple as possible,
> to minimize the platform owner's work, so I just provide 1 include file
> for DSC, and you are correct that it was done at the cost of losing
> flexibility and wasting build time. 
> 
> Now I think even we have 4 separate DSC include files, it's still much
> easier to organize than original 20 more INF, and with much more flexibility
> to platform owner.
> 
> This could also solve the problem (11).

Right, this would restore flexibility to the PCD settings as well.

[...]

>>> +  !if $(NETWORK_TLS_ENABLE) == TRUE
>>> +    NetworkPkg/TlsDxe/TlsDxe.inf
>>> +    NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
>>> +  !endif
>>
>> (15) Unfortunately, this isn't flexible enough for OVMF. OVMF hooks
>>
>>   OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
>>
>> into TlsAuthConfigDxe via NULL class resolution -- for setting up the CA
>> certificates and cipher suites, in volatile UEFI variables, just in time.
> 
> You are correct, that's why I leave the original "TLS_ENABLE" flag and set
> NETWORK_TLS_ENABLE to false in OVMF package's patch. If a platform want to
> override a driver or library component, it should disable the relative
> NETWORK_*** flag for the include file, and add the override component in
> its DSC/FDF separately.
> 
> I haven't figure out a good solution except this method.

(See also my OvmfPkg patch comments:)

I think this method can work well; the only thing we should be careful
about IMO is that the platform-specific flag should really be clear
about it being platform specific. Hence my earlier suggestion to rename
TLS_ENABLE in OVMF to PLATFORM_TLS_ENABLE.

Because, just "TLS_ENABLE" is a little bit ambiguous (to me anyway) in
whether it utilizes a pre-packaged core feature, or a platform-specific
inclusion of the feature.

Thank you!
Laszlo