PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Nikita Leshenko]

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.

Thanks,
Nikita

Re: PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Ni, Ruiyu]

Mike,
Do you think that maybe just raising to TPL_NOTIFY is enough for PCI BAR probing?
Though the timer interrupt still triggers, all callbacks are suspended in TPL_NOTIFY.

Thanks,
Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Nikita
> Leshenko
> Sent: Friday, September 7, 2018 3:09 AM
> To: edk2-devel@lists.01.org
> Cc: Liran Alon <liran.alon@oracle.com>
> Subject: [edk2] PciBusDxe: PCI-Express bug with dynamic
> PcdPciExpressBaseAddress
> 
> 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.
> 
> Thanks,
> Nikita
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Laszlo Ersek]

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

Re: PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Liran Alon]

> 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

Re: PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Laszlo Ersek]

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

Re: PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Nikita Leshenko]

> On 11 Sep 2018, at 15:34, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> "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 for the detailed explanation. I guess we have no choice
but to copy BaseCachingPciExpressLib (renamed to
DxePciExpressLibCaching) from ArmVirt to OVMF as well.

In order to prevent such bugs in the future, what do you think
about changing the PCD reference in BasePciExpressLib.inf (in
MdePkg) from [Pcd] to [FixedPcd]? This way a module that has a
dynamic PcdPciExpressBaseAddress will fail to link with the
default PCIE library. Is it practical to get such change into
MdePkg despite their strict change policy?

Nikita

> 
> Thanks
> Laszlo
> 

Re: PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Laszlo Ersek]

On 09/13/18 14:27, Nikita Leshenko wrote:
> 
> 
>> On 11 Sep 2018, at 15:34, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> "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 for the detailed explanation. I guess we have no choice
> but to copy BaseCachingPciExpressLib (renamed to
> DxePciExpressLibCaching) from ArmVirt to OVMF as well.

Uhh, hold on for a minute :) , "no choice but" is a bit of a stretch.
Where does OVMF enter the picture to begin with?

Between OvmfPkg and ArmVirtPkg, we have a sort-of rule that ArmVirtPkg
is allowed to consume OvmfPkg content, but not vice versa. So, we could
move (and rename) the lib instance from ArmVirtPkg to OvmfPkg, update
the reference(s) in ArmVirtPkg accordingly, and then consume the lib
instance in OvmfPkg as well... *if* there was any reason to consume the
lib instance in OvmfPkg in the first place.

(Of course if, by "we", you meant an internal Oracle use case, then I'm
not trying to pry. I took "we" as "upstream community". Emails are
ambiguous! :) )

> In order to prevent such bugs in the future, what do you think
> about changing the PCD reference in BasePciExpressLib.inf (in
> MdePkg) from [Pcd] to [FixedPcd]? This way a module that has a
> dynamic PcdPciExpressBaseAddress will fail to link with the
> default PCIE library. Is it practical to get such change into
> MdePkg despite their strict change policy?

I certainly think that's a valid approach if the PCD can only be Fixed
by design. However, judging by Ray's (as yet unanswered) followup
question in the thread:

http://mid.mail-archive.com/734D49CCEBEEF84792F5B80ED585239D5BDF99C7@SHSMSX104.ccr.corp.intel.com

I believe "by design" might not be a done deal just yet. Perhaps
BasePciExpressLib can be fixed to work with a dynamic PCD (and then we
should drop BaseCachingPciExpressLib in ArmVirtPkg too, and consume the
fixed BasePciExpressLib instance).

I suggest that you please file a bug for MdeModulePkg /
BasePciExpressLib in the TianoCore Bugzilla
<https://bugzilla.tianocore.org/>, referencing this discussion from the
mailing list archive
<https://lists.01.org/pipermail/edk2-devel/2018-September/029427.html>,
and request that (a) either the dynamic PCD use case be fixed, or (b)
the code own up to the restriction and be explicit about FixedPCD, both
in the INF file(s) and the C-language APIs.

Thanks!
Laszlo

Re: PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Nikita Leshenko]

> On 13 Sep 2018, at 15:15, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 09/13/18 14:27, Nikita Leshenko wrote:
>> 
>> Thanks for the detailed explanation. I guess we have no choice
>> but to copy BaseCachingPciExpressLib (renamed to
>> DxePciExpressLibCaching) from ArmVirt to OVMF as well.
> 
> Uhh, hold on for a minute :) , "no choice but" is a bit of a stretch.
> Where does OVMF enter the picture to begin with?
> 
> Between OvmfPkg and ArmVirtPkg, we have a sort-of rule that ArmVirtPkg
> is allowed to consume OvmfPkg content, but not vice versa. So, we could
> move (and rename) the lib instance from ArmVirtPkg to OvmfPkg, update
> the reference(s) in ArmVirtPkg accordingly, and then consume the lib
> instance in OvmfPkg as well... *if* there was any reason to consume the
> lib instance in OvmfPkg in the first place.
> 
> (Of course if, by "we", you meant an internal Oracle use case, then I'm
> not trying to pry. I took "we" as "upstream community". Emails are
> ambiguous! :) )

Yes, I originally meant in upstream community. But I agree with your
comments below, so yes we definitely have a choice :)

> 
> I suggest that you please file a bug for MdeModulePkg /
> BasePciExpressLib in the TianoCore Bugzilla, referencing this discussion from the
> mailing list archive

Sure, I filed the bug at
https://bugzilla.tianocore.org/show_bug.cgi?id=1178

-Nikita

> <https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.01.org_pipermail_edk2-2Ddevel_2018-2DSeptember_029427.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=JD7W0KpKqI3xo5AglC-aIVDRz_ysy5CrQRnZ9Jb7je0&m=kTbOaTuIdbwwl68uu5hSfrQkVeQGmOgSZZgr8guTIAc&s=dBONCYfrLeiUrFWuFes0jATimnx7m278xMXfzfQauqA&e=>,
> and request that (a) either the dynamic PCD use case be fixed, or (b)
> the code own up to the restriction and be explicit about FixedPCD, both
> in the INF file(s) and the C-language APIs.
> 
> Thanks!
> Laszlo