Re: PciBusDxe: PCI-Express bug with dynamic PcdPciExpressBaseAddress

[Laszlo Ersek <lersek@redhat.com>]

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