ping On 05/04/18 23:36, Laszlo Ersek wrote: > Repo: https://github.com/lersek/edk2.git > Branch: pci_cap > > In message [1], Jordan suggested a general "capabilities helper lib that > could iterate & read the PCI capability structs". Patch #1 introduces > that library (class and central BASE instance) to MdePkg; the library > class is called PciCapLib. > > [1] > http://mid.mail-archive.com/150696619010.2454.9266470245604846575@jljusten-skl > > PciCapLib offers APIs to parse capabilities lists, and to locate, > describe, read and write capabilities. > > The library class targets a wide range of client module types; for > example, platform PEIMs, platform DXE drivers, and UEFI drivers that > follow the UEFI Driver Model. For this reason, the library class is > designed with PCI config access abstracted away, even beyond the PPI / > Protocol level, since those cannot cross the PEI / DXE boundary. > > This (minimal) config space access method abstraction is called > PCI_CAP_DEV. It is *conceptually* a protocol. Client modules plug > PCI_CAP_DEV instances into PciCapLib for config space access, similarly > to how DiskIo and BlockIo instances are plugged into filesystem drivers. > Similarly to the filesystem driver example, PciCapLib is an example for > the "strategy pattern". > > Patches #2 and #3 introduce small "factory" libraries that help client > modules produce PCI_CAP_DEV instances: > > - PciCapPciSegmentLib -- incl. class and central BASE instance -- from > patch #2 produces PCI_CAP_DEV instances on top of PciSegmentLib, using > the "Segment:Bus:Device.Function" notation. This is meant for platform > modules (PEIMs and DXE drivers), which generally have a working > PciSegmentLib instance available. > > - PciCapPciIoLib -- incl. class and central UEFI instance -- from patch > #3 lets client modules produce PCI_CAP_DEV instances on top of > EFI_PCI_IO_PROTOCOL instances. This is meant for UEFI drivers. > > Patches #4 and #5 resolve the three new lib classes to their respective > central instances in the OvmfPkg and ArmVirtPkg DSC files. > > Patch #6 converts OvmfPkg/PciHotPlugInitDxe to the new APIs, from manual > capability list parsing. Because PciHotPlugInitDxe is a platform DXE > driver, this conversion uses PciCapPciSegmentLib and PciCapLib. > > Patch #7 converts OvmfPkg/Virtio10Dxe to the new APIs, from manual > capability list parsing. Because Virtio10Dxe is a UEFI driver, this > conversion uses PciCapPciIoLib and PciCapLib. > > ----[ jump to the patches now, and return if you have questions: I > have answers, but you might find them too verbose in advance ]---- > > Q1: If PCI config access needs to be abstracted from PciCapLib, then why > don't we provide multiple instances of PciCapLib? In other words, > why the PCI_CAP_DEV "protocol" rather than multiple instances of > PciCapLib? > > A1: The point of lib classes and instances is that the class provides a > common interface, and the instances provide different > implementations. We have the opposite use case here (hence the > "strategy pattern"): the implementation of the higher level > algorithm is common, and the objects that plug into it differ at the > *interface* level ("Segment:Bus:Device.Function" notation versus > PciIo protocol). > > None of those notations are suitable for inclusion in the PciCapLib > *class*; a more abstract interface is needed for identifying the PCI > device to the library, for parsing of capabilities lists. > > Q2: OK, let's say we can't (and shouldn't) include both notations in the > PciCapLib class; can we settle on one of them, rather than > PCI_CAP_DEV? > > A2: No, we can't. PciIo is obviously unusable in PEIMs (and in quite a > few platform DXE drivers too). > > It takes a bit longer to explain why the other choice, i.e. adopting > the "Segment:Bus:Device.Function" notation for the PciCapLib > interface, is not good either. It has to do with the portability of > UEFI drivers that bind PciIo protocol instances: > > It is indeed easy enough to call PciIo->GetLocation() and plug the > output values into a theoretical "Segment:Bus:Device.Function" > interface. However, about the only two things that could consume > such location identifiers and actually *implement* config space > access would be: > > (a) the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read/Write member > functions, where the member functions themselves take the > "Bus:Device.Function" part, and the "Segment" part must be used > for selecting the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instance > itself, through the SegmentNumber member; > > (b) the PciSegmentLib interfaces. > > Building a PciCapLib instance that uses (a) into a UEFI driver is a > bad idea, because there are platforms that provide PciIo instances > -- hence the driver can generally run on them -- but don't provide > PciRootBridgeIo instances. > > Building a PciCapLib instance that uses (b) into a UEFI driver is > worse. The PciSegmentLib interfaces are not capable of returning > errors. For example, if a device appears as PCI Express (implying > its extended config space exists, potentially containing extended > capabilities), but the underlying platform does not support extended > config space access (for example because it only supports > 0xCF8/0xCFC, and not ECAM), then peeking at the extended config > space of the device may invoke undefined behavior within > PciSegmentLib, and the PciCapLib instance will be none the wiser. > > (In fact, for the same reason, even PciCapPciSegmentLib has to > extend the "Segment:Bus:Device.Function" notation slightly; so that > such issues are caught *before* the call is made to PciSegmentLib.) > > Q3: Wait, I thought it wasn't possible for a PCI Express device to > appear on a platform that lacked ECAM. > > A3: Is that a question? :) Yes, it is plenty possible [2]. Same way as > garbage / looped config spaces are [3]. > > [2] https://github.com/tianocore/edk2/commit/014b472053ae3 > [3] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=02d578e5edd9 > > Cc: Ard Biesheuvel <[email protected]> > Cc: Jordan Justen <[email protected]> > Cc: Liming Gao <[email protected]> > Cc: Michael D Kinney <[email protected]> > > Thanks, > Laszlo > > Laszlo Ersek (7): > MdePkg: introduce PciCapLib > MdePkg: introduce PciCapPciSegmentLib > MdePkg: introduce PciCapPciIoLib > OvmfPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib > ArmVirtPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib > OvmfPkg/PciHotPlugInitDxe: convert to PciCapLib > OvmfPkg/Virtio10Dxe: convert to PciCapLib > > ArmVirtPkg/ArmVirt.dsc.inc | 3 + > MdePkg/Include/Library/PciCapLib.h | 429 > +++++++++ > MdePkg/Include/Library/PciCapPciIoLib.h | 58 ++ > MdePkg/Include/Library/PciCapPciSegmentLib.h | 82 ++ > MdePkg/Library/BasePciCapLib/BasePciCapLib.c | 1007 > ++++++++++++++++++++ > MdePkg/Library/BasePciCapLib/BasePciCapLib.h | 60 ++ > MdePkg/Library/BasePciCapLib/BasePciCapLib.inf | 37 + > MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c | 226 > +++++ > MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h | 47 + > MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf | 34 + > MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c | 243 > +++++ > MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h | 44 + > MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf | 35 + > MdePkg/MdePkg.dec | 14 + > MdePkg/MdePkg.dsc | 3 + > OvmfPkg/Include/IndustryStandard/Virtio10.h | 7 +- > OvmfPkg/OvmfPkgIa32.dsc | 3 + > OvmfPkg/OvmfPkgIa32X64.dsc | 3 + > OvmfPkg/OvmfPkgX64.dsc | 3 + > OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c | 267 > ++---- > OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf | 5 + > OvmfPkg/Virtio10Dxe/Virtio10.c | 135 +-- > OvmfPkg/Virtio10Dxe/Virtio10.inf | 2 + > 23 files changed, 2485 insertions(+), 262 deletions(-) > create mode 100644 MdePkg/Include/Library/PciCapLib.h > create mode 100644 MdePkg/Include/Library/PciCapPciIoLib.h > create mode 100644 MdePkg/Include/Library/PciCapPciSegmentLib.h > create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.c > create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.h > create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.inf > create mode 100644 > MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c > create mode 100644 > MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h > create mode 100644 > MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf > create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c > create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h > create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf >
_______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
