On 11 April 2016 at 16:12, Laszlo Ersek <[email protected]> wrote: > On 04/11/16 15:43, Ard Biesheuvel wrote: >> 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. > > Okay, understood. > >> 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. > > The point is exactly that it's not "subsequent". >
I meant subsequent in Git commit log time, not in boot time. > Really, it is dictated by the PI and UEFI abstractions (and to some > extent the edk2 code base): > > - The PciIo protocol interfaces come from the generic PCI bus driver, > which is a UEFI_DRIVER, conforming to the UEFI driver model. Which > implies that drivers that consume PciIo protocol interfaces: > > - have to be UEFI drivers themselves (conforming to the UEFI driver > model), > > - or must register protocol notify callbacks for PciIo (and then > their behavior would depend on the platform BDS policy, which > decides about what UEFI drivers connect what devices) > > - Whereas the PI spec dictates, for some protocols, to be produced by > DXE_(RUNTIME_)DRIVERs, way before BDS is entered (and PciIo instances > are produced). If these protocols are implementable by accessing the > PCI config space of a (few) fixed B/D/F(s) on a given platform, then > those accesses will occur before the PCI bus driver is dispatched (as > part of BDS). > >> 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, > > Add it where? > > And add it on top of what else, or instead of what? > >> 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. > > Hmm. I assume you mean you'll add the assert in the entry point of > PciHostBridgeDxe? > > I agree that >1 should be asserted against, but ==1 is not guaranteed. > ==0 should be handled dynamically. > I simply want to check that the 'pci-ecam-generic' DT node we end up consuming is the only one that exists in the device tree. In fact, I think it makes sense to assert that PcdPciExpressBaseAddress equals the DT node reg property when we encounter it. > Do you have a fresh v3-wip branch somewhere? I'd like to try expressing > my proposal for patches #13 and #14 in code. (If you are okay with that.) > Sure, give me 30 mins. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
