Re: PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Laszlo Ersek <lersek@redhat.com>]

(+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