From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 65EC3203BEA2D for ; Fri, 4 May 2018 14:36:41 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 15E9AF641F; Fri, 4 May 2018 21:36:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-129.rdu2.redhat.com [10.10.120.129]) by smtp.corp.redhat.com (Postfix) with ESMTP id BE0E12024CA5; Fri, 4 May 2018 21:36:39 +0000 (UTC) From: Laszlo Ersek To: edk2-devel-01 Cc: Ard Biesheuvel , Jordan Justen , Liming Gao , Michael D Kinney Date: Fri, 4 May 2018 23:36:30 +0200 Message-Id: <20180504213637.11266-1-lersek@redhat.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Fri, 04 May 2018 21:36:41 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Fri, 04 May 2018 21:36:41 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: [PATCH 0/7] MdePkg, OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 May 2018 21:36:42 -0000 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 Cc: Jordan Justen Cc: Liming Gao Cc: Michael D Kinney 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 -- 2.14.1.3.gb7cf6e02401b