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 <ler...@redhat.com> Thanks Laszlo > > Thanks/Ray > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >> Sent: Wednesday, October 12, 2016 1:04 AM >> To: Laszlo Ersek <ler...@redhat.com> >> Cc: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org <edk2- >> de...@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 <ler...@redhat.com> 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 <ruiyu...@intel.com> >>>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >>>> --- >>>> 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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
