On 04/08/16 11:45, Ard Biesheuvel wrote: > Instead of using a constructor, which may reference a dynamic PCD which is > set by the DXE entry point of its user, defer the assignment of the global > mPciExpressBaseAddress until the first the library is actually invoked. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <[email protected]> > --- > ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf | > 1 - > ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c | > 16 ++++------------ > 2 files changed, 4 insertions(+), 13 deletions(-) > > diff --git > a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf > b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf > index f6a346d49f22..5374f1b9a369 100644 > --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf > +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf > @@ -23,7 +23,6 @@ [Defines] > MODULE_TYPE = BASE > VERSION_STRING = 1.0 > LIBRARY_CLASS = PciExpressLib|DXE_DRIVER UEFI_DRIVER > UEFI_APPLICATION > - CONSTRUCTOR = PciExpressLibInitialize > > # > # VALID_ARCHITECTURES = ARM AARCH64 > diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > index 6479f53b3714..0ce6d118483b 100644 > --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > @@ -68,18 +68,7 @@ PciExpressRegisterForRuntimeAccess ( > return RETURN_UNSUPPORTED; > } > > -STATIC UINT64 mPciExpressBaseAddress; > - > -RETURN_STATUS > -EFIAPI > -PciExpressLibInitialize ( > - VOID > - ) > -{ > - mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress); > - return RETURN_SUCCESS; > -} > - > +STATIC UINT64 mPciExpressBaseAddress = MAX_UINT64; > > /** > Gets the base address of PCI Express. > @@ -92,6 +81,9 @@ GetPciExpressBaseAddress ( > VOID > ) > { > + if (mPciExpressBaseAddress == MAX_UINT64) { > + mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress); > + } > return (VOID*)(UINTN) mPciExpressBaseAddress; > } > >
I think this patch is not the right approach. I would like to preserve the ability to write a DXE_DRIVER that accesses PCI config space through this PciExpressLib instance, regardless of its own dispatch order against PciHostBridgeDxe. (Of course, given that PCI may not be available at all in the guest, such a DXE_DRIVER will have to check PCI presence explicitly in its entry point function, similarly to how PciHostBridgeDxe looks at PcdPciExpressBaseAddress at startup.) Therefore, I suggest the following: - In our DSC files, set PcdPciExpressBaseAddress to MAX_UINT64. This value is never a valid MMCONFIG base address. It shall mean that PcdPciExpressBaseAddress has not been detected yet. We should continue to interpret the value 0 as "detected but unavailable". Any other value means "detected and available". - Please keep the structure of this library instance (constructor etc). In the constructor, if the value of PcdPciExpressBaseAddress is not MAX_UINT64, then assign it to mPciExpressBaseAddress, and return immediately with success. Otherwise, parse the DTB as minimally as possible, using the client protocol (-> depex needed), just to determine the value of PcdPciExpressBaseAddress. If PCI is present, set the PCD to the (nonzero) base address. If PCI is absent (or there is some other parse error), set the PCD to zero. Either way, set mPciExpressBaseAddress to the same value, and then return with success. For the next patch (i.e., PciHostBridgeDxe): - The PCI presence check should be preserved in PciHostBridgeDxe, in the entry point function (PCD-based). This would (see above) consume the product of our one library instance that is linked into PciHostBridgeDxe itself, *or* into one of those future DXE_DRIVERs that I mentioned above. Because PciHostBridgeDxe itself links against the library, the PCD will never be MAX_UINT64, when the driver entry point checks it. - If that check passes, then PciHostBridgeDxe should implement its own, more complete FDT traversal (using the FDT client protocol), in order to find all the other characteristics of the pci-host node that it needs for operating. - Going forward, the PCDs (from ArmPkg) that are currently used for communicating these "other characteristics" from VirtFdtDxe to PciHostBridgeDxe should be eliminated from ArmVirtPkg. - Drop everything else too that becomes unused (to state the obvious). Do you agree? Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
