From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x229.google.com (mail-wr0-x229.google.com [IPv6:2a00:1450:400c:c0c::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2BD61208F7A9E for ; Wed, 20 Sep 2017 14:06:02 -0700 (PDT) Received: by mail-wr0-x229.google.com with SMTP id v109so3152570wrc.1 for ; Wed, 20 Sep 2017 14:09:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Qe5RVllJvpxX4hIEtsOQL0ckmHRO1ieKqm4R3htzCjg=; b=V4SNPJENqjAczyWymraG/HYXa0dtaWzV4s5OHPx1ytLgeoZ9UuDCSqqRiNqvjfbOe0 zDHK8LvLxGhz1MyWXVWhvyFx2HH3kkPcd512xwtC1Y7liM2kQ1jfS10QD9kVls8bR2E9 ILoubMH1CSDJ5sAisslejfjE9E53LWmr0ey6o= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Qe5RVllJvpxX4hIEtsOQL0ckmHRO1ieKqm4R3htzCjg=; b=mr+8sAn9sWeKsz+5rT7xTQ3eUoF0hZIHazcTZvMBdfnySJ1VHXeIojITZbcktR55lU ARqpeFmgP41xzhDofzs+a3PRd/cNbRwkE2IlhufK4NpSLnVhc0SWgkm4FjXL57q/MH88 agwdZFpWDQXQ5yEOXufPM9kFP1Bx+uhFImfJ8pLs0rKwn28HlbDbi+0FldBuoq//iuE1 GRXU/uh8eATGVfmoFlsVxMvrsNjmkSYNFJiVdCZMq65LdP2/Ok9GuuUutV59OY3yW9U8 SJpGnz0Hcbs4zPXsReHPHFP/dtXavAzRFOLkwwCJbD4zYrqixWNMbeyBNinBIZXCIDzn vgmg== X-Gm-Message-State: AHPjjUhJ0YWUcwhoV66hxw897bKMGdKjkMQZPw0bUUulY2LoNnLthwt8 xcIfNAzPCVnkT7Wgfl4eFLfz2A== X-Google-Smtp-Source: AOwi7QCgjrSGOxFo8pJvJ9pydDWFvMGzExM/sXQOPp62UAKwM/ZLOIerO7C8psszoarxj3HtSp8peA== X-Received: by 10.223.184.251 with SMTP id c56mr14400wrg.145.1505941746802; Wed, 20 Sep 2017 14:09:06 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 204sm126272wms.1.2017.09.20.14.09.05 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 20 Sep 2017 14:09:05 -0700 (PDT) Date: Wed, 20 Sep 2017 22:09:03 +0100 From: Leif Lindholm To: Laszlo Ersek Cc: edk2-devel@lists.01.org, Michael D Kinney , Jordan Justen , Andrew Fish , Ard Biesheuvel Message-ID: <20170920210903.tdpj6prki4ikrlth@bivouac.eciton.net> References: <20170920172755.22767-1-leif.lindholm@linaro.org> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) 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 21:06:02 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > > >