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.) The idea is this: assume for the sake of argument that the "virt" machine type grows a special (board-specific) PCI B/D/F that is necessary for implementing a given PI or UEFI protocol, in a DXE_DRIVER module, without relying on the PCI bus driver and/or PciHostBridgeDxe. A good example for this, on x86 / OVMF, is the SmmControl2Dxe driver: - It implements EFI_SMM_CONTROL2_PROTOCOL, in a DXE_RUNTIME_DRIVER. - It accesses the config space registers of some fixed location PCI devices directly with PciLib. (Those B/D/F constants come from Intel's ICH9 / Q35 specs.) - Its entry point function can run before or after the entry point function of PciHostBridgeDxe. This kind of activity is valid for a DXE_(RUNTIME_)DRIVER, and I would like to keep it available to ArmVirtPkg. Now, the problem we have here is that PCI(e) is entirely optional in the "virt" machtype of today, whereas the PciLib / PciExpressLib / PciSegmentLib classes all assume that PCI(e) is available unconditionally (and the only variable is "where"). So, these library classes do not expose a function or variable (or a boolean PCD for that matter) that allows client modules to interrogate the presense of PCI. This is the reason why we have to commit a layering violation in all our PCI-consuming DXE_DRIVER modules, and abuse PcdPciExpressBaseAddress to see if PCI(e) is available at all. (Normally a DXE_DRIVER that consumes PciLib / PciExpressLib / PciSegmentLib has zero business looking at this PCD at all!) But, I would like to keep this layering violation as minimal as possible, and keep the detection of the MMCONFIG base address outside of the client modules. Which is why I disagree with "deal[ing] with the MAX_UINT64 case in the [client] code as well". Namely, if a DXE_DRIVER like I described above does come along sometime later, we will have to duplicate the handling of MAX_UINT64 in it (the parsing of "reg"). IOW, both drivers would have to be explicitly aware of the question, "am I the first guy here that wants PCIe config space access?" In the current (un-refactored) code, the "reg" property controls the ConfigBase setting. This property is different from the other properties because: - All DXE_DRIVER modules that I describe above (that is, clients of PciLib / PciExpressLib / PciSegmentLib) need to rely on it. (Indirectly of course, i.e., through the library instance). - Simultaneously, "reg" is the only such property. Namely, "bus-range" is irrelevant for a DXE_DRIVER that wants config space access to a predefined B/D/F location. And "ranges" is entirely irrelevant for config space. Its entries describe IO and MMIO ranges, hence "ranges" falls outside of the domain of PciLib / PciExpressLib / PciSegmentLib. Thus, "bus-range" and "ranges" are relevant only to PciHostBridgeDxe. This is why I suggest to: - handle the "reg" property in the library instance, - handle everything except the "reg" property in PciHostBridgeDxe. I don't think that an argument like "we consume the rest of the DT node in PciHostBridgeDxe anyway" is very relevant here. After all, we handle the entire device tree in VirtFdtDxe at the moment, and your primary goal is to split that up along functional lines. Well, speaking in UEFI and edk2 terms, such a functional line happens to be drawn through the "pci-host-ecam-generic" node of the DT, between the "reg" property, and other properties. Short version ------------- - The PciLib & co. classes allow a client module to access PCI config space. Enumeration, discovery, valid ranges etc. are not their goal. Instead, they should enable a client module that knows exact B/D/F locations (from hardware specs) to perform such config space accesses. - Our PciExpressLib instance should support this goal as well, regardless of which of its client modules is dispatched first. Which implies that the "reg" property should be transparently parsed in the library constructor, so the PciLib & co interfaces just work. - This parsing is expensive (even if it means just the one "reg" property). Therefore we should cache the parsed value. The common solution for caching the MMCONFIG base address is to save it in PcdPciExpressBaseAddress. - Everything else (the "ranges" and the "bus-range" properties) are unrelated to the PciLib & co. classes, hence they should be handled by PciHostBridgeDxe separately, explicitly. - The PciLib & co. classes don't allow a client module to ask "is PCI(e) available at all?" In order to cope with this, we need to violate the PciLib & co. abstraction a bit, but only as little as necessary. Our current way is to look at PcdPciExpressBaseAddress in the entry point of each such client module. This makes the "availability question" overlap the "caching question". If you want, we can introduce a new dynamic boolean PCD for the availability question separately. Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
