> On 7 Sep 2018, at 11:44, Laszlo Ersek <[email protected]> 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
