Re: PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Liran Alon <liran.alon@oracle.com>]

> On 7 Sep 2018, at 11:44, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> (+Ard)
> 
> On 09/06/18 21:08, Nikita Leshenko wrote:
>> Hi,
>> 
>> We ran into a bug in EDK2 relating to PCI-Express in PciBusDxe. Here's the flow
>> of the bug:
>> 
>> 1. PciBusDxe/PciEnumeratorSupport.c: Function BarExisted probes a BAR. It raises
>>   TPL to TPL_HIGH_LEVEL to avoid timer interrupts while probing the BAR and
>>   calls PciIo->Pci.Write.
>> 2. BasePciExpressLib/PciExpressLib.c: The write reaches PciExpressWrite32, which
>>   calls GetPciExpressBaseAddress.
>> 3. GetPciExpressBaseAddress retrieves the address from PcdPciExpressBaseAddress.
>> 4. Reading the PCD calls DxePcdGet64 -> GetWorker ->
>>   EfiAcquireLock(&mPcdDatabaseLock), which is at TPL_NOTIFY level. This crashes
>>   the firmware because step 1 raised the TPL to TPL_HIGH_LEVEL.
>> 
>> This doesn't happen when PcdPciExpressBaseAddress is fixed at build (because
>> then the read is optimized to a static global variable), but when the PCD is
>> dynamic PCI-Express is broken.
>> 
>> Does anybody have a suggestion for fixing it?
>> 
>> Options we thought about:
>> - Change mPcdDatabaseLock.Tpl to TPL_HIGH_LEVEL
>> - Don't use a PCD for the base address, put it in a static global variable and
>>  create functions to set and retrieve it.
> 
> In the ArmVirtPkg platforms, we also set "PcdPciExpressBaseAddress"
> dynamically. And, we implemented your second option above; see:
> 
>  ArmVirtPkg/Library/BaseCachingPciExpressLib/
> 
> Relevant commits:
> 
> - ad3359eb43a9 ("ArmVirtualizationPkg: clone BasePciExpressLib, cache
> PCIe config base", 2015-02-23)
> - a06d0bb58eb9 ("ArmVirtPkg/BaseCachingPciExpressLib: depend on
> PciPcdProducerLib", 2016-04-12)
> 
> (In fact, commit ad3359eb43a9 documents the exact issue you report here.)
> 
> Thanks
> Laszlo

Thanks Lazlo. Weren’t aware of this solution in the ArmVirtPkg platforms.

However, I wonder why the solution was to clone the MdePkg/Library/BasePciExpressLib rather than
change the original library itself?

That is, what was the reason for not just adding a library constructor to MdePkg/Library/BasePciExpressLib
to cache PcdPciExpressBaseAddress in a global var? This seem to solve the issues for all platforms.
If PcdPciExpressBaseAddress is fixed and doesn’t change dynamically, then the caching of the value in a global var
should be harmless (besides adding an extra read from global-var). If it does change dynamically, 
MdePkg/Library/BasePciExpressLib have the issue discussed in this email thread.

Thanks,
-Liran