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
Acked-by: Laszlo Ersek <ler...@redhat.com>
>> -----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>; email@example.com <edk2-
>> 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
>>>> ## 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
>> 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:
>> This would be the strictly correct way, but given the above, I don't really
>> the point.
edk2-devel mailing list