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

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

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

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

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