| Message ID | 20170920172755.22767-1-leif.lindholm@linaro.org |
|---|---|
| Headers | show |
| Series | Create central repository for boilerplate configuration | expand |
On 09/20/17 19:27, Leif Lindholm wrote: > An awful lot of platform configuration is just repeated verbatim for > every platform. This is my first stab at eliminating some of this > redundancy. > > I have additional bits as work in progress, but before I sink too much > time into it, I would like to try to gather feedback on this approach > (all the way down to directory structure). > > This first round deals with basic network support and Secure Boot > requirements. I can't comment on the general "ConfigPkg" question, and the directory structure; I'll just assume that it's OK that way. (It will need documented maintainers though.) I do have some initial comments: (1) Please update your git config so that the diff hunk headers display the INI-style section name that's being modified. Please search the following sections for "ini": https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05 https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09 Normally this is plain "helpful" for reviewers, but given the subject of this set, I'd say it's "important" (to me as a reviewer anyway), to see what section lines are removed from. (2) Replacing a build define called FOOBAR with CONFIG_FOOBAR will break all downstream build scripts. Is the CONFIG_ prefix a requirement? (3) I think PCDs should not be included in ConfigPkg DSC include files, even if several platforms set the same value. The set of libraries and driver modules commonly used for a given feature is mostly constant across platforms (and it is easy to extend, incrementally); but I don't think the same holds for PCDs. Especially if a user wants to change a PCD for one platform but not the other. Even if repeated settings for a PCD worked (all on the same level of "specificity"), I'd find the result confusing. (4) The Ia32X64 build of OVMF is a bit trickier than the rest; DXE modules should be built for X64, while PEI modules should be built for IA32. You can see this in the names of the [Components.IA32] and [Components.X64] sections. (Point (1) above helps with this.) For example, in patch #1 you add "ArpDxe" to [Components], but in patch #3 you remove it from [Components.X64]. (5) In patch #4, there's a section for [LibraryClasses.ARM, LibraryClasses.AARCH64] -- why is it specific to ARM/AARCH64? If others like the ConfigPkg idea, I'd like to review the set (or v2) again, in more detail, but for that, please fix the patch formatting first, as requested in (1). Thanks! Laszlo > > Leif Lindholm (6): > ConfigPkg: add new package for holding common config fragments > ArmVirtPkg: use ConfigPkg for common network items > OvmfPkg: use ConfigPkg for common network items > ConfigPkg: add common Security settings > ArmVirtPkg: use ConfigPkg for common security items > OvmfPkg: use ConfigPkg for common security items > > ArmVirtPkg/ArmVirt.dsc.inc | 25 ++-------- > ArmVirtPkg/ArmVirtQemu.dsc | 46 +++--------------- > ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 24 ++-------- > ArmVirtPkg/ArmVirtQemuKernel.dsc | 46 +++--------------- > ConfigPkg/Network/Network.dsc.inc | 92 ++++++++++++++++++++++++++++++++++++ > ConfigPkg/Network/Network.fdf.inc | 47 ++++++++++++++++++ > ConfigPkg/Security/Security.dsc.inc | 67 ++++++++++++++++++++++++++ > ConfigPkg/Security/Security.fdf.inc | 17 +++++++ > OvmfPkg/OvmfPkgIa32.dsc | 92 ++++-------------------------------- > OvmfPkg/OvmfPkgIa32.fdf | 37 +-------------- > OvmfPkg/OvmfPkgIa32X64.dsc | 90 ++++------------------------------- > OvmfPkg/OvmfPkgIa32X64.fdf | 37 +-------------- > OvmfPkg/OvmfPkgX64.dsc | 92 ++++-------------------------------- > OvmfPkg/OvmfPkgX64.fdf | 37 +-------------- > 14 files changed, 276 insertions(+), 473 deletions(-) > create mode 100644 ConfigPkg/Network/Network.dsc.inc > create mode 100644 ConfigPkg/Network/Network.fdf.inc > create mode 100644 ConfigPkg/Security/Security.dsc.inc > create mode 100644 ConfigPkg/Security/Security.fdf.inc > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Wed, Sep 20, 2017 at 08:14:59PM +0200, Laszlo Ersek wrote: > On 09/20/17 19:27, Leif Lindholm wrote: > > An awful lot of platform configuration is just repeated verbatim for > > every platform. This is my first stab at eliminating some of this > > redundancy. > > > > I have additional bits as work in progress, but before I sink too much > > time into it, I would like to try to gather feedback on this approach > > (all the way down to directory structure). > > > > This first round deals with basic network support and Secure Boot > > requirements. > > I can't comment on the general "ConfigPkg" question, and the directory > structure; I'll just assume that it's OK that way. (It will need > documented maintainers though.) Of course - I just thought I'd wait with that until we'd been through whether it should live in MdeModulePkg or ... > I do have some initial comments: > > (1) Please update your git config so that the diff hunk headers display > the INI-style section name that's being modified. Please search the > following sections for "ini": Agh, sorry - I lost that when I reinstalled my build machine a month or so back. I did do 9, but I missed out rerunning git config diff.ini.xfuncname '^\[[A-Za-z0-9_., ]+]' > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05 > > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09 > > Normally this is plain "helpful" for reviewers, but given the subject of > this set, I'd say it's "important" (to me as a reviewer anyway), to see > what section lines are removed from. Indeed. Pure oversight. Now addressed. > (2) Replacing a build define called FOOBAR with CONFIG_FOOBAR will break > all downstream build scripts. Is the CONFIG_ prefix a requirement? It was explicitly intended to break compatibility, to ensure we didn't end up with things accidentally working until something unrelated changed in the future. Also a subject for discussion - hence the RFC. > (3) I think PCDs should not be included in ConfigPkg DSC include files, > even if several platforms set the same value. The set of libraries and > driver modules commonly used for a given feature is mostly constant > across platforms (and it is easy to extend, incrementally); but I don't > think the same holds for PCDs. Especially if a user wants to change a > PCD for one platform but not the other. Even if repeated settings for a > PCD worked (all on the same level of "specificity"), I'd find the result > confusing. Also a subject for discussion. My intent was that if most of the open source platforms had an override on the default of a particular Pcd, we could override it in the config fragments without changing the .dec (and affecting non-public ports). Individual platforms can still override (again). Also a subject for discussion. > (4) The Ia32X64 build of OVMF is a bit trickier than the rest; DXE > modules should be built for X64, while PEI modules should be built for > IA32. You can see this in the names of the [Components.IA32] and > [Components.X64] sections. (Point (1) above helps with this.) > > For example, in patch #1 you add "ArpDxe" to [Components], but in patch > #3 you remove it from [Components.X64]. I'll be honest, I don't have any system on which I can currently build IA32 - I get "unknown relocations" whenever I try. So Ia32X64 was only compile tested, not linked. > (5) In patch #4, there's a section for [LibraryClasses.ARM, > LibraryClasses.AARCH64] -- why is it specific to ARM/AARCH64? These mappings appeared to be necessary only for these architectures at the time. The section started up larger than it ended up, It is quite likely further work on Common.dsc.inc would remove more of these. > If others like the ConfigPkg idea, I'd like to review the set (or v2) > again, in more detail, but for that, please fix the patch formatting > first, as requested in (1). As stated above, pure oversight. / Leif > Thanks! > Laszlo > > > > > Leif Lindholm (6): > > ConfigPkg: add new package for holding common config fragments > > ArmVirtPkg: use ConfigPkg for common network items > > OvmfPkg: use ConfigPkg for common network items > > ConfigPkg: add common Security settings > > ArmVirtPkg: use ConfigPkg for common security items > > OvmfPkg: use ConfigPkg for common security items > > > > ArmVirtPkg/ArmVirt.dsc.inc | 25 ++-------- > > ArmVirtPkg/ArmVirtQemu.dsc | 46 +++--------------- > > ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 24 ++-------- > > ArmVirtPkg/ArmVirtQemuKernel.dsc | 46 +++--------------- > > ConfigPkg/Network/Network.dsc.inc | 92 ++++++++++++++++++++++++++++++++++++ > > ConfigPkg/Network/Network.fdf.inc | 47 ++++++++++++++++++ > > ConfigPkg/Security/Security.dsc.inc | 67 ++++++++++++++++++++++++++ > > ConfigPkg/Security/Security.fdf.inc | 17 +++++++ > > OvmfPkg/OvmfPkgIa32.dsc | 92 ++++-------------------------------- > > OvmfPkg/OvmfPkgIa32.fdf | 37 +-------------- > > OvmfPkg/OvmfPkgIa32X64.dsc | 90 ++++------------------------------- > > OvmfPkg/OvmfPkgIa32X64.fdf | 37 +-------------- > > OvmfPkg/OvmfPkgX64.dsc | 92 ++++-------------------------------- > > OvmfPkg/OvmfPkgX64.fdf | 37 +-------------- > > 14 files changed, 276 insertions(+), 473 deletions(-) > > create mode 100644 ConfigPkg/Network/Network.dsc.inc > > create mode 100644 ConfigPkg/Network/Network.fdf.inc > > create mode 100644 ConfigPkg/Security/Security.dsc.inc > > create mode 100644 ConfigPkg/Security/Security.fdf.inc > > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Leif
It is good to see such configuration.
Here is some of my thought:
1) We have similar idea. But there is one key difference is that we do not use MACRO, but use PCD to control feature selection.
For example, we defined
gPlatformModuleTokenSpaceGuid.PcdUefiSecureBootEnable|FALSE|BOOLEAN|0xF00000A4
gPlatformModuleTokenSpaceGuid.PcdTpm2Enable |FALSE|BOOLEAN|0xF00000A5
gPlatformModuleTokenSpaceGuid.PcdPerformanceEnable |FALSE|BOOLEAN|0xF00000A6
in https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
And they are used in https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Platform/Intel/MinPlatformPkg/Include/Fdf/CoreDxeInclude.fdf
I suggest we consider using PCD for configuration.
2) I do not suggest we use configuration for library, if this library only has one instance.
For example:
+ BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+ IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
If no module need this library, this library will be ignored directly.
We can always leave BaseCryptLib there. No impact.
3) For below NULL library, we need put configuration around the NULL library instance, instead of the module. As such we may add more NULL instance, such as DxeTpm2MeasureBootLib.
+!if $(CONFIG_SECURE_BOOT_ENABLE) == TRUE
+ MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
+ <LibraryClasses>
+ NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
+ }
To
MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
<LibraryClasses>
!if gSecurityTokenSpaceGuid.PcdUefiSecureBootEnable == TRUE
NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
!endif
!if gSecurityTokenSpaceGuid.PcdTpm2Enable == TRUE
NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
!endif
}
Thank you
Yao Jiewen
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Leif
> Lindholm
> Sent: Thursday, September 21, 2017 1:28 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Andrew Fish
> <afish@apple.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [edk2] [RFC 0/6] Create central repository for boilerplate configuration
>
> An awful lot of platform configuration is just repeated verbatim for
> every platform. This is my first stab at eliminating some of this
> redundancy.
>
> I have additional bits as work in progress, but before I sink too much
> time into it, I would like to try to gather feedback on this approach
> (all the way down to directory structure).
>
> This first round deals with basic network support and Secure Boot
> requirements.
>
> Leif Lindholm (6):
> ConfigPkg: add new package for holding common config fragments
> ArmVirtPkg: use ConfigPkg for common network items
> OvmfPkg: use ConfigPkg for common network items
> ConfigPkg: add common Security settings
> ArmVirtPkg: use ConfigPkg for common security items
> OvmfPkg: use ConfigPkg for common security items
>
> ArmVirtPkg/ArmVirt.dsc.inc | 25 ++--------
> ArmVirtPkg/ArmVirtQemu.dsc | 46 +++---------------
> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 24 ++--------
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 46 +++---------------
> ConfigPkg/Network/Network.dsc.inc | 92
> ++++++++++++++++++++++++++++++++++++
> ConfigPkg/Network/Network.fdf.inc | 47 ++++++++++++++++++
> ConfigPkg/Security/Security.dsc.inc | 67 ++++++++++++++++++++++++++
> ConfigPkg/Security/Security.fdf.inc | 17 +++++++
> OvmfPkg/OvmfPkgIa32.dsc | 92 ++++--------------------------------
> OvmfPkg/OvmfPkgIa32.fdf | 37 +--------------
> OvmfPkg/OvmfPkgIa32X64.dsc | 90 ++++-------------------------------
> OvmfPkg/OvmfPkgIa32X64.fdf | 37 +--------------
> OvmfPkg/OvmfPkgX64.dsc | 92
> ++++--------------------------------
> OvmfPkg/OvmfPkgX64.fdf | 37 +--------------
> 14 files changed, 276 insertions(+), 473 deletions(-)
> create mode 100644 ConfigPkg/Network/Network.dsc.inc
> create mode 100644 ConfigPkg/Network/Network.fdf.inc
> create mode 100644 ConfigPkg/Security/Security.dsc.inc
> create mode 100644 ConfigPkg/Security/Security.fdf.inc
>
> --
> 2.11.0
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Something I have found useful is leaving the intended file extension at the end of the file name. That way, you still know it is intended to be "!include"ed, but any editor file extensions you have set up still work on these files. ConfigPkg/Security/Security.dsc.inc => ConfigPkg/Security/Security.inc.dsc Since DSC can merge sections from different files, one *.inc.dsc file is sufficient with multiple sections. For FDF files, I've found that sections cannot be merged so it might be better to indicate where pieces are intended to be included *.pei.inc.fdf and *.dxe.inc.fdf. Also, these files probably shouldn't contain section headers because you don't know what a platform might call the sections/FVs. (general statements, not critique on this RFC) This allows people to "!include" directly and know where the pieces are intended to go. Plus it gives a quick view indication in the platform DSC and FDF files that things have been "!include"ed in the correct sections of the file. Thanks, GARRETT KIRKENDALL SMTS Firmware Engineer | CTE 7171 Southwest Parkway, Austin, TX 78735 USA AMD facebook | amd.com > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Leif Lindholm > Sent: Wednesday, September 20, 2017 12:28 PM > To: edk2-devel@lists.01.org > Cc: Michael D Kinney <michael.d.kinney@intel.com>; Jordan Justen > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Andrew Fish > <afish@apple.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> > Subject: [edk2] [RFC 0/6] Create central repository for boilerplate > configuration > > An awful lot of platform configuration is just repeated verbatim for every > platform. This is my first stab at eliminating some of this redundancy. > > I have additional bits as work in progress, but before I sink too much > time into it, I would like to try to gather feedback on this approach (all > the way down to directory structure). > > This first round deals with basic network support and Secure Boot > requirements. > > Leif Lindholm (6): > ConfigPkg: add new package for holding common config fragments > ArmVirtPkg: use ConfigPkg for common network items > OvmfPkg: use ConfigPkg for common network items > ConfigPkg: add common Security settings > ArmVirtPkg: use ConfigPkg for common security items > OvmfPkg: use ConfigPkg for common security items > > ArmVirtPkg/ArmVirt.dsc.inc | 25 ++-------- > ArmVirtPkg/ArmVirtQemu.dsc | 46 +++--------------- > ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 24 ++-------- > ArmVirtPkg/ArmVirtQemuKernel.dsc | 46 +++--------------- > ConfigPkg/Network/Network.dsc.inc | 92 > ++++++++++++++++++++++++++++++++++++ > ConfigPkg/Network/Network.fdf.inc | 47 ++++++++++++++++++ > ConfigPkg/Security/Security.dsc.inc | 67 ++++++++++++++++++++++++++ > ConfigPkg/Security/Security.fdf.inc | 17 +++++++ > OvmfPkg/OvmfPkgIa32.dsc | 92 ++++--------------------------- > ----- > OvmfPkg/OvmfPkgIa32.fdf | 37 +-------------- > OvmfPkg/OvmfPkgIa32X64.dsc | 90 ++++--------------------------- > ---- > OvmfPkg/OvmfPkgIa32X64.fdf | 37 +-------------- > OvmfPkg/OvmfPkgX64.dsc | 92 ++++--------------------------- > ----- > OvmfPkg/OvmfPkgX64.fdf | 37 +-------------- > 14 files changed, 276 insertions(+), 473 deletions(-) create mode 100644 > ConfigPkg/Network/Network.dsc.inc create mode 100644 > ConfigPkg/Network/Network.fdf.inc create mode 100644 > ConfigPkg/Security/Security.dsc.inc > create mode 100644 ConfigPkg/Security/Security.fdf.inc > > -- > 2.11.0 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 09/20/17 23:09, Leif Lindholm wrote: > On Wed, Sep 20, 2017 at 08:14:59PM +0200, Laszlo Ersek wrote: >> (2) Replacing a build define called FOOBAR with CONFIG_FOOBAR will break >> all downstream build scripts. Is the CONFIG_ prefix a requirement? > > It was explicitly intended to break compatibility, to ensure we didn't > end up with things accidentally working until something unrelated > changed in the future. Interesting idea. I guess we could try to reach out to all of the "repeat builders" of OVMF. > >> (3) I think PCDs should not be included in ConfigPkg DSC include files, >> even if several platforms set the same value. The set of libraries and >> driver modules commonly used for a given feature is mostly constant >> across platforms (and it is easy to extend, incrementally); but I don't >> think the same holds for PCDs. Especially if a user wants to change a >> PCD for one platform but not the other. Even if repeated settings for a >> PCD worked (all on the same level of "specificity"), I'd find the result >> confusing. > > Also a subject for discussion. > My intent was that if most of the open source platforms had an > override on the default of a particular Pcd, we could override it in > the config fragments without changing the .dec (and affecting > non-public ports). Right, that's great... > Individual platforms can still override (again). ... but this "again" part is what confuses me (assuming it would technically work). We'd have a PCD default in the .dec, then a setting in the central .dsc.inc that ultimately qualifies as a platform-level setting, and finally a setting in the actual platform .dsc, which *also* qualifies as a platform-level setting. IOW, one in the .dec, and two in the (final) .dsc. I have no clue if this works, but even if it does, the priority could depend on the order of inclusion, which I find confusing. Liming, Yonghong, can you guys please comment on this? Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Fri, Sep 22, 2017 at 01:20:57PM +0200, Laszlo Ersek wrote: > On 09/20/17 23:09, Leif Lindholm wrote: > > On Wed, Sep 20, 2017 at 08:14:59PM +0200, Laszlo Ersek wrote: > > >> (2) Replacing a build define called FOOBAR with CONFIG_FOOBAR will break > >> all downstream build scripts. Is the CONFIG_ prefix a requirement? > > > > It was explicitly intended to break compatibility, to ensure we didn't > > end up with things accidentally working until something unrelated > > changed in the future. > > Interesting idea. I guess we could try to reach out to all of the > "repeat builders" of OVMF. The proposal to drive the CONFIG options as Pcds would also be a solution to this issue. > >> (3) I think PCDs should not be included in ConfigPkg DSC include files, > >> even if several platforms set the same value. The set of libraries and > >> driver modules commonly used for a given feature is mostly constant > >> across platforms (and it is easy to extend, incrementally); but I don't > >> think the same holds for PCDs. Especially if a user wants to change a > >> PCD for one platform but not the other. Even if repeated settings for a > >> PCD worked (all on the same level of "specificity"), I'd find the result > >> confusing. > > > > Also a subject for discussion. > > My intent was that if most of the open source platforms had an > > override on the default of a particular Pcd, we could override it in > > the config fragments without changing the .dec (and affecting > > non-public ports). > > Right, that's great... > > > Individual platforms can still override (again). > > ... but this "again" part is what confuses me (assuming it would > technically work). We'd have a PCD default in the .dec, then a setting > in the central .dsc.inc that ultimately qualifies as a platform-level > setting, and finally a setting in the actual platform .dsc, which *also* > qualifies as a platform-level setting. IOW, one in the .dec, and two in > the (final) .dsc. Yes. But I cannot think of another way of handling it, that does not also mean stuffing a lot of boiler plate back into the platform-level files. > I have no clue if this works, but even if it does, the priority could > depend on the order of inclusion, which I find confusing. Oh, definitely. But also under complete control of the platform. Potentially, if this becomes some great success story, we will want to extend the build command with a separate [includes] section to give greater control over enforcing order. > Liming, Yonghong, can you guys please comment on this? Yes, please :) Regards, Leif _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo and Leif: PCD value are the position relation. In DSC file, the latter one will override the first one. But, PCD type can't be overridden. For example, if PCD is listed in [PcdsFixedAtBuild] section in the config.inc, PCD can't be listed in [PcdsPatchableInModule] section in Platform.dsc. I think Platform may not only override PCD value, but also override PCD type. To meet with platform requirement, we had better not place PCD setting in common DSC file. Thanks Liming >-----Original Message----- >From: Leif Lindholm [mailto:leif.lindholm@linaro.org] >Sent: Sunday, September 24, 2017 12:58 AM >To: Laszlo Ersek <lersek@redhat.com> >Cc: edk2-devel@lists.01.org; Kinney, Michael D ><michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; >Andrew Fish <afish@apple.com>; Ard Biesheuvel ><ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>; Zhu, >Yonghong <yonghong.zhu@intel.com> >Subject: Re: [edk2] [RFC 0/6] Create central repository for boilerplate >configuration > >On Fri, Sep 22, 2017 at 01:20:57PM +0200, Laszlo Ersek wrote: >> On 09/20/17 23:09, Leif Lindholm wrote: >> > On Wed, Sep 20, 2017 at 08:14:59PM +0200, Laszlo Ersek wrote: >> >> >> (2) Replacing a build define called FOOBAR with CONFIG_FOOBAR will >break >> >> all downstream build scripts. Is the CONFIG_ prefix a requirement? >> > >> > It was explicitly intended to break compatibility, to ensure we didn't >> > end up with things accidentally working until something unrelated >> > changed in the future. >> >> Interesting idea. I guess we could try to reach out to all of the >> "repeat builders" of OVMF. > >The proposal to drive the CONFIG options as Pcds would also be a >solution to this issue. > >> >> (3) I think PCDs should not be included in ConfigPkg DSC include files, >> >> even if several platforms set the same value. The set of libraries and >> >> driver modules commonly used for a given feature is mostly constant >> >> across platforms (and it is easy to extend, incrementally); but I don't >> >> think the same holds for PCDs. Especially if a user wants to change a >> >> PCD for one platform but not the other. Even if repeated settings for a >> >> PCD worked (all on the same level of "specificity"), I'd find the result >> >> confusing. >> > >> > Also a subject for discussion. >> > My intent was that if most of the open source platforms had an >> > override on the default of a particular Pcd, we could override it in >> > the config fragments without changing the .dec (and affecting >> > non-public ports). >> >> Right, that's great... >> >> > Individual platforms can still override (again). >> >> ... but this "again" part is what confuses me (assuming it would >> technically work). We'd have a PCD default in the .dec, then a setting >> in the central .dsc.inc that ultimately qualifies as a platform-level >> setting, and finally a setting in the actual platform .dsc, which *also* >> qualifies as a platform-level setting. IOW, one in the .dec, and two in >> the (final) .dsc. > >Yes. But I cannot think of another way of handling it, that does not >also mean stuffing a lot of boiler plate back into the platform-level >files. > >> I have no clue if this works, but even if it does, the priority could >> depend on the order of inclusion, which I find confusing. > >Oh, definitely. But also under complete control of the platform. > >Potentially, if this becomes some great success story, we will want to >extend the build command with a separate [includes] section to give >greater control over enforcing order. > >> Liming, Yonghong, can you guys please comment on this? > >Yes, please :) > >Regards, > >Leif _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel