From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1182A21ECCB00 for ; Wed, 20 Sep 2017 11:11:57 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8C24EC0587ED; Wed, 20 Sep 2017 18:15:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 8C24EC0587ED Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-23.rdu2.redhat.com [10.10.120.23]) by smtp.corp.redhat.com (Postfix) with ESMTP id E31DA6060A; Wed, 20 Sep 2017 18:15:00 +0000 (UTC) To: Leif Lindholm , edk2-devel@lists.01.org Cc: Michael D Kinney , Jordan Justen , Andrew Fish , Ard Biesheuvel References: <20170920172755.22767-1-leif.lindholm@linaro.org> From: Laszlo Ersek Message-ID: Date: Wed, 20 Sep 2017 20:14:59 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170920172755.22767-1-leif.lindholm@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 20 Sep 2017 18:15:02 +0000 (UTC) Subject: Re: [RFC 0/6] Create central repository for boilerplate configuration X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Sep 2017 18:11:57 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >