On 8 April 2016 at 21:15, Laszlo Ersek <[email protected]> wrote: > 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? >
Yes, that makes sense. I coded up the first part, which is straightforward enough, but for the second part, I think it is better to deal with the MAX_UINT64 case in the code as well. This way, the module and the library are independent, and we don't ignore the 'reg' property in the module, which is a bit strange as well considering that we do consume the rest of the DT node, Thanks, Ard. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
