Re: [RFC 0/6] Create central repository for boilerplate configuration

[Leif Lindholm <leif.lindholm@linaro.org>]

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
> > 
>