Re: [PATCH] MdeModulePkg/MdeModulePkg.dec: Fix EBC build failure of PciBus driver

[Laszlo Ersek <lersek@redhat.com>]

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 <lersek@redhat.com>

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 <lersek@redhat.com>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org <edk2-
>> 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 <lersek@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.ni@intel.com>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@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.