Re: PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Laszlo Ersek <lersek@redhat.com>]

On 09/07/18 19:01, Liran Alon wrote:
> 
> 
>> 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.

There are two reasons, one related to project governance, and another
technical.

(1) The reason related to project management is that modifying core edk2
libraries and drivers (such that live in MdePkg and MdeModulePkg) is
usually difficult. The requirement that the code be correct and pass
technical review is the easy part; the hard part is that you can sell
your use case enough for your change to be accepted for MdePkg /
MdeModulePkg. I'm pretty unhappy about this state of affairs myself, but
it is what it is. (I do sympathize with the MdePkg / MdeModulePkg
maintainers as well, though! Their responsibility is big.) So, in many
cases, it is a lot easier to enable your platform by cloning a library
instance and modifying it. After all, that's what library *classes* were
invented for -- to have multiple instances, accommodating different
platforms.

(2) The technical reason is that the library instance in question is
called "BasePciExpressLib".

Given a library class called "FoobarLib", an instance that implements
the class is usually named

- with a prefix that identifies the firmware phase / module type that
the lib instance (= implementation) targets, i.e. those that the lib
instance is usable within,

- with an optional suffix that hints at the implementation details /
behavior of the library instance.

"BasePciExpressLib" has the prefix "Base", meaning that it is supposed
to be usable in all types of firmware modules, even in SEC and PEIMs --
which may not have access to writeable memory except stack (i.e.
writeable global variables). Therefore modifying the original lib
instance so that it depend on writeable globals wouldn't have been right.

We could have attempted to add the new library instance under MdePkg,
and not ArmVirtPkg, but that's just a (more difficult) special case of
point (1).

(Obviously if you try to apply the nomenclature I describe above to
"BaseCachingPciExpressLib" as well, you'll see that it doesn't match.
And that's because, when I invented the name for that lib instance, in
2015, I didn't know about the naming rules myself. :) In reality the lib
instance should be called "DxePciExpressLibCaching" -- with "Dxe" for
prefix, and "Caching" for suffix.)

Thanks
Laszlo

> 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