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 1071D1A1E30 for ; Tue, 11 Oct 2016 03:52:32 -0700 (PDT) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 67840C05490E; Tue, 11 Oct 2016 10:52:31 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-32.phx2.redhat.com [10.3.116.32]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9BAqT3l021506; Tue, 11 Oct 2016 06:52:30 -0400 To: Ruiyu Ni , edk2-devel@ml01.01.org References: <20161011050113.197540-1-ruiyu.ni@intel.com> Cc: Ard Biesheuvel From: Laszlo Ersek Message-ID: Date: Tue, 11 Oct 2016 12:52:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20161011050113.197540-1-ruiyu.ni@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Tue, 11 Oct 2016 10:52:31 +0000 (UTC) Subject: Re: [PATCH] MdeModulePkg/MdeModulePkg.dec: Fix EBC build failure of PciBus driver X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Oct 2016 10:52:32 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 10/11/16 07:01, Ruiyu Ni wrote: > When PciBus is built as EBC, PcdPciDegradeResourceForOptionRom does > not have associated value resulting build failure. > The patch sets the default value to TRUE, covering the EBC ARCH. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ruiyu Ni > Cc: Ard Biesheuvel > --- > MdeModulePkg/MdeModulePkg.dec | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index f870b83..42fef75 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -746,7 +746,6 @@ [PcdsFeatureFlag] > # @Prompt Turn on PS2 Mouse Extended Verification > gEfiMdeModulePkgTokenSpaceGuid.PcdPs2MouseExtendedVerification|TRUE|BOOLEAN|0x00010075 > > -[PcdsFeatureFlag.X64] > ## Indicates whether 64-bit PCI MMIO BARs should degrade to 32-bit in the presence of an option ROM > # On X64 platforms, Option ROMs may contain code that executes in the context of a legacy BIOS (CSM), > # which requires that all PCI MMIO BARs are located below 4 GB > Hmmmm, I wonder if this is the right thing to do. As far as I understand the original patch (commit 065ae7d717f9e), it added PcdPciDegradeResourceForOptionRom twice to the DEC file expressly for the purpose of providing different defaults (per arch), without having to update the DSC files of existing platforms. The original patch didn't name EBC in either of the sections -- which is why the EBC compilation would fail --, but I don't think that including EBC in either section (manually or implicitly, as illustrated by the patch) would be correct. Namely, EBC is an instruction set that is independent of the platform that executes it. The suggested patch is correct if the EBC build of PciBusDxe is expected to run on x64 platforms, but it is incorrect if the exact same binary is expected to run on aarch64 platforms. Meaning that for EBC, *both* default values (TRUE and FALSE) are incorrect on some platforms. With that in mind, I propose that we declare PcdPciDegradeResourceForOptionRom only once in MdeModulePkg.dec, regardless of architecture -- that is, in the plain [PcdsFeatureFlag] section --, and require all platforms that include PciBusDxe to set the feature flag in their DSCs if they disagree with the (now centralized) default. In practical terms, this would turn this patch into a series of patches, first adding the DSC changes -- for platforms that are in-tree --, and then unifying the declaration. I expect this will create some churn for out-of-tree modules, but that seems justified -- considering EBC, the PCD would have to be customized in some platform DSCs *anyway*, regardless of what default we picked for EBC. The following DSC files include PciBusDxe.inf: ArmVirtPkg/ArmVirtQemu.dsc ArmVirtPkg/ArmVirtQemuKernel.dsc CorebootPayloadPkg/CorebootPayloadPkgIa32.dsc CorebootPayloadPkg/CorebootPayloadPkgIa32X64.dsc EmulatorPkg/EmulatorPkg.dsc MdeModulePkg/MdeModulePkg.dsc Nt32Pkg/Nt32Pkg.dsc OvmfPkg/OvmfPkgIa32.dsc OvmfPkg/OvmfPkgIa32X64.dsc OvmfPkg/OvmfPkgX64.dsc QuarkPlatformPkg/Quark.dsc QuarkPlatformPkg/QuarkMin.dsc Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc Vlv2TbltDevicePkg/PlatformPkgIA32.dsc Vlv2TbltDevicePkg/PlatformPkgX64.dsc Just my two cents. Thanks Laszlo