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 F3E091A1E30 for ; Wed, 12 Oct 2016 00:08:38 -0700 (PDT) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (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 6213B4E02D; Wed, 12 Oct 2016 07:08:38 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-37.phx2.redhat.com [10.3.116.37]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9C78awA012706; Wed, 12 Oct 2016 03:08:37 -0400 To: "Ni, Ruiyu" , Ard Biesheuvel References: <20161011050113.197540-1-ruiyu.ni@intel.com> <734D49CCEBEEF84792F5B80ED585239D58E29EAD@SHSMSX104.ccr.corp.intel.com> Cc: "edk2-devel@lists.01.org" From: Laszlo Ersek Message-ID: Date: Wed, 12 Oct 2016 09:08:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D58E29EAD@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 12 Oct 2016 07:08:38 +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: Wed, 12 Oct 2016 07:08:39 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 10/12/16 04:36, Ni, Ruiyu wrote: > I agree with Ard. Building PciBusDxe driver as EBC ARCH is supported from tool perspective, but doesn't > make much sense in real world. > So the patch is just to resolve the build failure in EBC ARCH. If Ard is fine with the EBC default for PcdPciDegradeResourceForOptionRom that this patch results in, then I don't object. Acked-by: Laszlo Ersek Thanks Laszlo > > Thanks/Ray > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: Wednesday, October 12, 2016 1:04 AM >> To: Laszlo Ersek >> Cc: Ni, Ruiyu ; edk2-devel@lists.01.org > devel@ml01.01.org> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/MdeModulePkg.dec: Fix EBC >> build failure of PciBus driver >> >> On 11 October 2016 at 11:52, Laszlo Ersek wrote: >>> 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|TR >> UE|B >>>> OOLEAN|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. >>> >> >> This may be true, but do we care? Building this driver as EBC is a validation >> exercise more than anything else, and so how an EBC PciBusDxe module >> should behave on a 64-bit architecture in the presence of an option ROM is >> strictly hypothetical. (Note that EBC is primarily intended for the use in option >> ROMS, and given that we need this driver to dispatch option ROMs in the >> first place, I would expect platforms that require this driver to ship with a >> native build of it.) >> >>> 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 >>> >> >> This would be the strictly correct way, but given the above, I don't really see >> the point. >> >> Thanks, >> Ard.