On 11 April 2016 at 15:34, Laszlo Ersek <[email protected]> wrote: > On 04/11/16 14:17, Ard Biesheuvel wrote: >> On 8 April 2016 at 21:15, Laszlo Ersek <[email protected]> wrote: > > [snip] > >>> 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, > > I disagree. > > There are several points in your one paragraph above, so I'll have to > elaborate :) (I didn't want to fragment your paragraph into individual > sentences.) > > Long version > ------------ > > What you are saying (if I understand it correctly) means that any > DXE_DRIVER that needs PCI config space access would have to prepare > itself for an as-yet uninitialized PCD, and perform the initialization > (= the parsing of "reg") itself, explicitly. > > I think this is wrong. I think my point may not have been clear enough: > >>> 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. > > (Or maybe it was clear, and it's just me not understanding your point.) >
No, I did not get the PCI config space nuance here. I also think that is highly unlikely (and a worse layering violation than the one we are dealing with) to enumerate PCI host bridges dynamically via DT and subsequently wire up a driver to a fixed B/D/F. But I don't feel strongly about this at all, to be honest. The only thing I will do is add an assertion that there is only a single 'pci-ecam-generic' DT node, since having multiple will break lots of other stuff anyway, and it also validates the assumption that the contents of PcdPciExpressBaseAddress correspond with the DT node we are dealing with. Thanks, Ard. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
